feat(constraints): add per-specifier provenance tracking#1189
Conversation
When multiple constraint files are merged, engineers had no way to
determine which input file contributed each specifier. This adds
provenance tracking so every constraint records its source file.
- Add `_provenance` dict to `Constraints` class, grouped by source file
- Make `source` a required keyword-only parameter on `add_constraint()`
- Add `get_provenance()` public method returning `{source: [lines]}`
- Include inline provenance comments in `merged-constraints.txt` output
- Enrich `InvalidConstraintError` messages with source file info
- Add provenance to resolver rejection logs and exception messages
- Add `_format_provenance()` helper for human-readable formatting
Co-Authored-By: Claude <claude@anthropic.com>
Closes: python-wheel-build#1186
Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
📝 WalkthroughWalkthroughThis PR implements per-constraint-file provenance tracking in the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fromager/constraints.py`:
- Around line 168-177: get_provenance currently returns a shallow copy of
self._provenance so callers can mutate nested lists and change internal state;
update get_provenance to return a deep copy of the provenance mapping (e.g., use
copy.deepcopy on self._provenance[canonicalize_name(name)] or construct a new
dict copying each list) so callers cannot modify internal data structures
accessed via get_provenance; ensure you still canonicalize the name with
canonicalize_name(name) and return an empty dict when missing.
In `@tests/test_constraints.py`:
- Around line 306-307: Update the test that asserts InvalidConstraintError from
c.add_constraint to ensure the exception message contains both conflicting
sources: change the pytest.raises regex (the match= argument) to require both
"base.txt" and "override.txt" (for example using a positive lookahead pattern
like (?=.*base\.txt)(?=.*override\.txt)) so the test fails if either source is
missing from the error message; target the assertion surrounding the
c.add_constraint call that raises InvalidConstraintError.
- Around line 275-281: The test test_provenance_returns_copy currently only
verifies the outer dict is copied; update it to also check for nested mutation
leakage by mutating a list value returned by Constraints.get_provenance and
asserting that the internal provenance inside the Constraints instance is not
changed. Specifically, after using Constraints.add_constraint("foo>=1.0",
source="a.txt") call c.get_provenance("foo"), append/modify the returned list
(from the dict value) and then call c.get_provenance("foo") again to assert the
original list value remains unchanged, ensuring get_provenance returns
deep-copied (or otherwise protected) list values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46b1bdaf-7ff4-4858-b8d0-c2bc42261980
📒 Files selected for processing (5)
src/fromager/constraints.pysrc/fromager/resolver.pytests/test_constraints.pytests/test_context.pytests/test_resolver.py
| def get_provenance(self, name: str) -> dict[str, list[str]]: | ||
| """Return provenance info for *name*. | ||
|
|
||
| Returns: | ||
| Mapping of ``{source_file: [original_constraint_lines]}``, | ||
| or an empty dict if the package has no constraints. | ||
|
|
||
| .. versionadded:: 0.84.0 | ||
| """ | ||
| return dict(self._provenance.get(canonicalize_name(name), {})) |
There was a problem hiding this comment.
Return a deep copy from get_provenance() to prevent internal state mutation.
The current return value is a shallow copy; callers can mutate nested lists and silently alter self._provenance, which then changes dump_constraints() output and provenance shown in exceptions/logs.
Suggested fix
def get_provenance(self, name: str) -> dict[str, list[str]]:
@@
- return dict(self._provenance.get(canonicalize_name(name), {}))
+ prov = self._provenance.get(canonicalize_name(name), {})
+ return {source: list(lines) for source, lines in prov.items()}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_provenance(self, name: str) -> dict[str, list[str]]: | |
| """Return provenance info for *name*. | |
| Returns: | |
| Mapping of ``{source_file: [original_constraint_lines]}``, | |
| or an empty dict if the package has no constraints. | |
| .. versionadded:: 0.84.0 | |
| """ | |
| return dict(self._provenance.get(canonicalize_name(name), {})) | |
| def get_provenance(self, name: str) -> dict[str, list[str]]: | |
| """Return provenance info for *name*. | |
| Returns: | |
| Mapping of ``{source_file: [original_constraint_lines]}``, | |
| or an empty dict if the package has no constraints. | |
| .. versionadded:: 0.84.0 | |
| """ | |
| prov = self._provenance.get(canonicalize_name(name), {}) | |
| return {source: list(lines) for source, lines in prov.items()} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fromager/constraints.py` around lines 168 - 177, get_provenance currently
returns a shallow copy of self._provenance so callers can mutate nested lists
and change internal state; update get_provenance to return a deep copy of the
provenance mapping (e.g., use copy.deepcopy on
self._provenance[canonicalize_name(name)] or construct a new dict copying each
list) so callers cannot modify internal data structures accessed via
get_provenance; ensure you still canonicalize the name with
canonicalize_name(name) and return an empty dict when missing.
| def test_provenance_returns_copy() -> None: | ||
| """get_provenance returns a copy, not the internal dict.""" | ||
| c = Constraints() | ||
| c.add_constraint("foo>=1.0", source="a.txt") | ||
| prov = c.get_provenance("foo") | ||
| prov["injected.txt"] = ["foo<99"] | ||
| assert "injected.txt" not in c.get_provenance("foo") |
There was a problem hiding this comment.
test_provenance_returns_copy misses nested mutation leakage
This only proves the outer dict is copied; list values can still be mutated through the returned object, which can silently corrupt internal provenance state.
Suggested test strengthening
def test_provenance_returns_copy() -> None:
"""get_provenance returns a copy, not the internal dict."""
c = Constraints()
c.add_constraint("foo>=1.0", source="a.txt")
prov = c.get_provenance("foo")
prov["injected.txt"] = ["foo<99"]
assert "injected.txt" not in c.get_provenance("foo")
+ prov["a.txt"].append("foo<99")
+ assert c.get_provenance("foo")["a.txt"] == ["foo>=1.0"]As per coding guidelines: "tests/**: Verify test actually tests the intended behavior. Check for missing edge cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_constraints.py` around lines 275 - 281, The test
test_provenance_returns_copy currently only verifies the outer dict is copied;
update it to also check for nested mutation leakage by mutating a list value
returned by Constraints.get_provenance and asserting that the internal
provenance inside the Constraints instance is not changed. Specifically, after
using Constraints.add_constraint("foo>=1.0", source="a.txt") call
c.get_provenance("foo"), append/modify the returned list (from the dict value)
and then call c.get_provenance("foo") again to assert the original list value
remains unchanged, ensuring get_provenance returns deep-copied (or otherwise
protected) list values.
| with pytest.raises(InvalidConstraintError, match=r"base\.txt"): | ||
| c.add_constraint("foo<1.0", source="/constraints/override.txt") |
There was a problem hiding this comment.
Assert both conflicting sources in the error message
The current regex only checks base.txt; it won’t fail if the new source (override.txt) is missing from the exception text.
Suggested assertion update
- with pytest.raises(InvalidConstraintError, match=r"base\.txt"):
+ with pytest.raises(
+ InvalidConstraintError,
+ match=r"(base\.txt.*override\.txt|override\.txt.*base\.txt)",
+ ):
c.add_constraint("foo<1.0", source="/constraints/override.txt")As per coding guidelines: "tests/**: Verify test actually tests the intended behavior. Check for missing edge cases."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(InvalidConstraintError, match=r"base\.txt"): | |
| c.add_constraint("foo<1.0", source="/constraints/override.txt") | |
| with pytest.raises( | |
| InvalidConstraintError, | |
| match=r"(base\.txt.*override\.txt|override\.txt.*base\.txt)", | |
| ): | |
| c.add_constraint("foo<1.0", source="/constraints/override.txt") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_constraints.py` around lines 306 - 307, Update the test that
asserts InvalidConstraintError from c.add_constraint to ensure the exception
message contains both conflicting sources: change the pytest.raises regex (the
match= argument) to require both "base.txt" and "override.txt" (for example
using a positive lookahead pattern like (?=.*base\.txt)(?=.*override\.txt)) so
the test fails if either source is missing from the error message; target the
assertion surrounding the c.add_constraint call that raises
InvalidConstraintError.
1 participant
When multiple constraint files are merged, engineers had no way to determine which input file contributed each specifier. This adds provenance tracking so every constraint records its source file.
_provenancedict toConstraintsclass, grouped by source filesourcea required keyword-only parameter onadd_constraint()get_provenance()public method returning{source: [lines]}merged-constraints.txtoutputInvalidConstraintErrormessages with source file info_format_provenance()helper for human-readable formattingCloses: #1186
Pull Request Description
What
Why