Repository navigation
Keep composer unit tests off the host's env - #2064
Conversation
resolve_local_vendor_dir_sync, local_install_unread and the global vendor-dir lookup read COMPOSER_VENDOR_DIR, and COMPOSER_HOME / HOME / APPDATA for the user-level config.json, straight from the process. Unit tests reach them through the crawler, so a developer's COMPOSER_VENDOR_DIR (or a vendor-dir in ~/.composer/config.json) moves every fixture's vendor tree: on main `cargo test -p socket-patch-core --lib` fails 10 composer_crawler tests with COMPOSER_VENDOR_DIR set. In unit-test builds these lookups now read only the vars a test holds with an EnvGuard (test_env::guarded_var), the pattern #2057 used for go_env. Production and integration-test builds read the process env exactly as before. A serial regression test sets the host vars raw and checks they're hidden, and that a held value is still honoured. Refs #1919. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeSSzeJ3EZNDhV5v3Vj
|
bugbot run Generated by Claude Code |
PR SummaryLow Risk Overview
A new Reviewed by Cursor Bugbot for commit eda0096. Configure here. |
From /code-review high on the PR: - The regression test set HOME raw, which non-serial tests running alongside could read. It now sets only COMPOSER_VENDOR_DIR, which nothing outside the lookups under test reads, and a drop guard puts the previous value back even if an assertion fails. - test_env::guarded_var dropped non-UTF-8 values, while production reads var_os. New guarded_var_os keeps the bytes, and composer's process_var uses it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeSSzeJ3EZNDhV5v3Vj
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit eda0096. Configure here.
|
Ready for review (burn-down agent).
Generated by Claude Code |
Final review briefWhat it does. Test-only change. Composer's three vendor-dir lookups ( Risk: low. The production path is unchanged ( Look here
Verified
Changes I made: none. Open questions: none blocking.
Auto-merge is armed, so approving sends this straight to the merge queue. Generated by Claude Code |
Problem
Composer's vendor-dir lookups read the developer's environment in unit tests. On main (
de81f1b),cargo test -p socket-patch-core --libpasses clean (7,720 tests) but fails 10crawlers::composer_crawlertests whenCOMPOSER_VENDOR_DIRis set. Avendor-dirin the developer's own~/.composer/config.json(or underCOMPOSER_HOME/APPDATA) has the same effect, because the user-level config is found throughHOME.CI impact today: none. No workflow sets these, and CI has been green: zero failed or re-run merge_group runs, and zero merge-queue evictions in the last 24h. With nothing merge-blocking or flaky to fix, this run took the next item in the hermetic-test series (#2054–#2057, all merged), from the hostile-env probe on #1919. That probe didn't cover the Composer variables.
Root cause
resolve_local_vendor_dir_sync,local_install_unreadandglobal_vendor_dir_syncpass|name| std::env::var_os(name)intoresolve_vendor_dir. That function honoursCOMPOSER_VENDOR_DIRfirst, then the user config throughlocal_composer_home.Fix
These three lookups now go through one
process_var. In production and integration-test builds it isstd::env::var_os, so nothing changes. In unit-test builds it istest_env::guarded_var_os(added here;guarded_varis now built on it), so a test sees only the vars it holds with anEnvGuard. That's the same pattern #2057 used forgo_env. The injected-closure tests that already coverresolve_vendor_dir's cascade are unaffected.Proof
COMPOSER_VENDOR_DIR=elsewhereon the core lib tests: 10 failed → 0 (7,721 passed).#[serial]regression testvendor_dir_lookups_ignore_the_hosts_composer_env: it setsCOMPOSER_VENDOR_DIRraw, the way a host would, and asserts the fixture still resolves<project>/vendor. A drop guard restores the previous value even if an assertion fails. It also asserts that a value held withEnvGuardis honoured. Withprocess_varreverted tostd::env::var_os, the test fails withhost env hidden.cargo clippy --locked -p socket-patch-core --all-features -- -D warnings(the CI invocation) is clean. The file is rustfmt'd.No tests removed or moved.
/code-review high
I ran it on the first commit and fixed 3 findings in
eda0096:HOME, which non-serial tests running alongside could read.test_env::guarded_var_oskeeps non-UTF-8 values, matching production'svar_os.3 findings were not fixed:
get_composer_home(COMPOSER_HOME, and thecomposer global config homesubprocess, which reads the real env anyway) anduses_xdg()still read the host env on the global-discovery path. Neither caused a failure in the probe, and making them hermetic would also mean faking the subprocess, so they're left for Crawler unit tests read the developer's real HOME, YARN_NODE_LINKER and VIRTUAL_ENV: 12 pkg_managers/pnpm_layout tests fail under a user nodeLinker setting #1919.env_var_osshim to replace the per-modulecfg(test)pairs (here and ingo_env) is a reasonable refactor, but it's a separate topic from this fix.Also found (for #1919, not in this PR)
On the same probe,
YARN_NODE_LINKER=pnpstill fails 7 tests (npm_crawler1,shared_store2,npm_flavor2,yarn_berry_lock2), and aHOMEwithnodeLinker: pnpin.yarnrc.ymlfailsnpm_flavor::vendorable_probe_refuses_berry_configured_for_pnp. These are the same modules as theYARN_RC_FILENAMEfollow-ups already listed there.https://claude.ai/code/session_011f9aeSSzeJ3EZNDhV5v3Vj
Generated by Claude Code