Conversation
`every_variable_rtk_reads_is_redirected_for_a_child` skipped the three resolver files outright. The exemption is there because a resolver defines the accessors, so its own `user_env::var_os(name)` names no variable the scan can resolve — but skipping the whole file also hides any read in it that *does* name one, and `user_dirs` is where a new path override would naturally be written. Forgive the forwarding instead of the file: a read whose name does not resolve is allowed in a resolver and still refused everywhere else, and a literal read is collected wherever it appears. A variable added to `user_dirs` now has to be redirected for a spawned child like any other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 Automated PR Analysis
SummaryThis PR tightens an isolation test guard so that resolver files (like src/core/user_dirs.rs) are no longer wholesale exempted from the scan that checks every environment variable read is redirected for child processes; now only unresolvable names (forwarding calls) are forgiven in resolvers, while literal variable reads anywhere, including in resolvers, are still caught. It's a follow-up to PR #4038 addressing a review comment about test coverage gaps. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #4038, where @TaKO8Ki pointed out that the new tests pinned only
HOMEand should pin the other directory variables the waytests/hook_warning_scope_test.rsdoes. That was right, and it raised the question of what stops the next one slipping through.Mostly, something already does.
every_variable_rtk_reads_is_redirected_for_a_childscans the source for every read through the accessors, refuses any whose name it cannot resolve to a literal, and asserts each one is removed, pinned to a fixed value, or pinned inside the scratch directory. A variable rtk learns to read tomorrow fails the suite until it is redirected.With one gap: the scan
continues on the threeRESOLVERSfiles. The exemption earns its place — a resolver defines the accessors, souser_env::var_os(name)names no variable — but it is all-or-nothing, so a literal read in one is invisible.src/core/user_dirs.rsis in that list, and it is the module for resolving user locations, which makes it the most likely place for the next agent home or path override to be written.Measured
Adding a read of a new
FUTUREAGENT_HOME, redirected nowhere, and runningevery_variable_rtk_reads_is_redirected_for_a_child:src/hooks/init/mod.rssrc/hooks/init/mod.rs: FUTUREAGENT_HOMEsrc/core/user_dirs.rssrc/core/user_dirs.rs: FUTUREAGENT_HOMEFix
Forgive the forwarding rather than the file: an unresolvable name is allowed in a resolver and still refused everywhere else, and a literal is collected wherever it appears.
only_user_dirs_resolves_user_locationskeeps its own use ofRESOLVERSunchanged — there the exemption is correct, since those files are the chokepoint.No new exemptions were needed: the only accessor read in a resolver today is that one forwarding call.
Not covered
Variables read by a dependency rather than by rtk —
dirsconsultingXDG_DATA_HOME, orAPPDATA/USERPROFILEon Windows.redirect_rtk_datapins those, but no scan of rtk's own source can see them, so a change insidedirswould not fail anything. Catching that needs a check on the symptom instead of the source (assert a spawned child wrote nothing outside the scratch directory), which is a larger piece of work than this.I also tried the blunter version first —
env_clear()on the child plus a keep-list, so an unknown variable is absent by construction. It failsevery_variable_rtk_reads_is_redirected_for_a_childandonly_user_dirs_resolves_user_locations, because it replaces a guard that fails loudly at the source with a silent keep-list. Not worth the trade.Gate
cargo fmt --allclean,cargo clippy --all-targetsclean,cargo test --all4161 passed / 0 failed, Windows cross-check clean.🤖 Generated with Claude Code