feat(packagesettings): add ExternalCommands model for env filtering - #1266
Conversation
📝 WalkthroughWalkthroughAdds Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 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/packagesettings/_models.py`:
- Around line 176-182: Add a concise docstring to the public
ExternalCommands.model_post_init override describing its environment-pattern
initialization, while preserving the existing delete_env and keep_env behavior.
In `@tests/test_packagesettings.py`:
- Around line 970-988: Update test_external_commands_valid to verify
ExternalCommands instances are frozen by attempting to assign a field and
asserting pydantic.ValidationError is raised. Keep the existing
valid-configuration assertions unchanged and retain the docstring’s
frozen-behavior claim.
🪄 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 Plus
Run ID: 453aa429-40c9-4b5e-8ec1-c72cd6118d4d
📒 Files selected for processing (4)
src/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_settings.pytests/test_packagesettings.py
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks good! I have left couple of suggestions that are non blocking and can be done in a follow up
3e3c63a to
062cd8e
Compare
|
@Mergifyio rebase |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
eef968a to
5edf620
Compare
There was a problem hiding this comment.
This looks good to me! I am moving forward and approving this.
cc @LalatenduMohanty @smoparth
|
@Mergifyio rebase |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
5edf620 to
951de65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/packagesettings/_models.py`:
- Around line 155-172: Update validate_delete_env to detect wildcard pattern
overlaps between delete_env and DEFAULT_KEEP_ENV or keep_env, not only exact
string matches. Use the same full-match semantics as _keep_re to identify any
delete entry that would also be retained by a keep pattern, and raise the
existing validation error with the conflicting entries; preserve the bare-* and
non-overlapping behavior.
🪄 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 Plus
Run ID: c15c95c8-f8a9-4ac3-b739-4cb5e098e86b
📒 Files selected for processing (4)
src/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_settings.pytests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/fromager/packagesettings/init.py
- tests/test_packagesettings.py
- src/fromager/packagesettings/_settings.py
951de65 to
3cabc3f
Compare
|
@smoparth Thanks for the feedback! I have deviated slightly from my original proposal to address some design flaws. I like to protect users from misconfiguration. For example I included the POSIX compliance check for env var names to prevent typos like Here is a full diff between the initial commit and the current implementation (generated by Claude) Proxy variables removed from
|
smoparth
left a comment
smoparth
left a comment
There was a problem hiding this comment.
LGTM! Left a non-blocking comment from maintenance perspective.
Add `ExternalCommands` Pydantic model with `keep_env` / `delete_env` pattern lists, a `filter_env()` method, and a `DEFAULT_KEEP_ENV` class variable for essential variables (HOME, PATH, LC_*, TERM, TZ, TMPDIR, etc.). `delete_env` matching is case-insensitive so credentials cannot slip through due to unexpected capitalisation. Non-POSIX env var keys are always stripped by `filter_env()`. Not yet wired into `external_commands.run()`. See: python-wheel-build#1083 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Christian Heimes <cheimes@redhat.com>
3cabc3f to
680d644
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/fromager/packagesettings/_models.py (1)
170-192: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOverlap check still only catches exact-string collisions, not wildcard conflicts.
This is the same gap flagged in a previous review round:
delete_env=["AWS_SECRET_KEY"]combined withkeep_env=["AWS_*"](or anyDEFAULT_KEEP_ENVwildcard) passes validation, but at runtime_keep_rewillfullmatchAWS_SECRET_KEYandfilter_envkeeps it anyway — silently defeating the user's explicit intent to strip that variable. The added comment (lines 180-184) documents the limitation but doesn't fix it.🔐 Proposed fix: detect pattern-level overlap, not just exact matches
+def _patterns_conflict(a: str, b: str) -> bool: + """True if pattern *a* would also match anything pattern *b* matches (or vice versa).""" + a_base, a_wild = a.rstrip("*"), a.endswith("*") + b_base, b_wild = b.rstrip("*"), b.endswith("*") + if a_wild and b_wild: + return a_base.startswith(b_base) or b_base.startswith(a_base) + if a_wild: + return b_base.startswith(a_base) + if b_wild: + return a_base.startswith(b_base) + return a_base == b_base + + `@pydantic.model_validator`(mode="after") def validate_delete_env(self) -> typing.Self: """Validate ``delete_env`` for conflicts and redundancy.""" if not self.delete_env: return self if "*" in self.delete_env and len(self.delete_env) > 1: raise ValueError( "delete_env: bare '*' must be the only entry, " "additional patterns are redundant" ) keep = set(self.DEFAULT_KEEP_ENV) | set(self.keep_env) - overlap = keep & set(self.delete_env) + overlap = { + d for d in self.delete_env if any(_patterns_conflict(k, d) for k in keep) + } if overlap: raise ValueError( f"delete_env overlaps with keep_env / DEFAULT_KEEP_ENV: " f"{sorted(overlap)}" ) return self🤖 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/packagesettings/_models.py` around lines 170 - 192, Update validate_delete_env in the package settings model to detect wildcard pattern conflicts, not only exact-string overlap. Compare each delete_env entry against keep_env and DEFAULT_KEEP_ENV using the same fullmatch semantics as _keep_re, so values such as AWS_SECRET_KEY conflict with AWS_* and equivalent wildcard combinations are rejected while preserving the existing bare-* validation and error behavior.
🤖 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 `@docs/proposals/filter-env.md`:
- Around line 83-86: Update the no-configuration behavior in the proposal to
state that empty keep_env and delete_env settings still sanitize the environment
by removing keys that are not valid POSIX names, while leaving valid keys
unfiltered. Reconcile the earlier “no filtering” wording with this default
sanitization behavior and retain the existing unconditional-removal rule.
---
Duplicate comments:
In `@src/fromager/packagesettings/_models.py`:
- Around line 170-192: Update validate_delete_env in the package settings model
to detect wildcard pattern conflicts, not only exact-string overlap. Compare
each delete_env entry against keep_env and DEFAULT_KEEP_ENV using the same
fullmatch semantics as _keep_re, so values such as AWS_SECRET_KEY conflict with
AWS_* and equivalent wildcard combinations are rejected while preserving the
existing bare-* validation and error behavior.
🪄 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 Plus
Run ID: 10260aed-ec3b-452e-a3e7-8a66dd7d0368
📒 Files selected for processing (5)
docs/proposals/filter-env.mdsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_settings.pytests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/fromager/packagesettings/init.py
- src/fromager/packagesettings/_settings.py
- tests/test_packagesettings.py
|
Tick the box to add this pull request to the merge queue (same as
|
3 participants
Pull Request Description
What
Add
ExternalCommandsPydantic model withkeep_env/delete_envpattern lists, afilter_env()method, and aDEFAULT_KEEP_ENVclass variable for essential variables (HOME, PATH, LC_*, etc.).Not yet wired into
external_commands.run().Why
See: #1083