Repository navigation
Read the cargo config chain once (#1832) - #1849
Conversation
Assisted-by: Claude Code:claude-opus-5-5
The build-cache cleaner walked the .cargo/config chain itself and preferred config.toml when both config and config.toml existed. Cargo reads the extensionless file, so the cleaner removed fingerprints from a target dir cargo never used. ConfigDirs::read now takes its file list from source_replacement::config_layer_paths, which the source replacement reader already uses, and keeps only the [build] lookup. It also stops reading $CARGO_HOME twice when an ancestor's .cargo is it. Fixes #1832 Assisted-by: Claude Code:claude-opus-5-5
build_dirs walked the ancestors of the cwd as given, and --cwd defaults to ".", whose ancestors stop at ".". Run from a workspace member, the cleaner found neither the workspace root nor a parent .cargo/config, so it invalidated no build dir at all. It now walks the absolute path. The config file probe also consults the open group commit, as the read that follows it does, so the two agree on captured files. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
Assisted-by: Claude Code:claude-opus-5-5
std::path::absolute keeps .., so --cwd ../app walked the ancestors of the directory being left and could take its build-dir. Fold the absolute cwd lexically, as cargo_crawler::resolve_for does. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review 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 2c715eb. Configure here.
|
[agent] CI on 2c715eb failed in
I've re-run the failed jobs once. Generated by Claude Code |
|
[agent] The re-run lost its Depot runners again. Generated by Claude Code |
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
Final review briefWhat it does: Fixes #1832. Risk: low. It is one private function, reached only from Look here:
Verified: I read the full diff against #1832 and checked the precedence against cargo. The fold keeps root and drive prefixes, and on Windows Changes I made: none. Open questions: none. One nit: the chdir test restores the working directory without a drop guard, though it does restore it before asserting. Auto-merge (squash) is armed, so approving sends this straight to the merge queue. Generated by Claude Code |
Wenxin Jiang (Wenxin-Jiang)
left a comment
There was a problem hiding this comment.
[agent] Reviewed 2c715eb185b6868047164544be5c81988aec1934. Checked shared Cargo config precedence, path bases, and relative-cwd ancestor traversal. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1832
Summary
The cargo config chain was walked twice.
cargo_build_cache::ConfigDirs::readpreferred.cargo/config.tomlwhen bothconfigandconfig.tomlexisted. Cargo, andsource_replacement::config_layer_paths, prefer the extensionlessconfig. So afterapply/rollbackthe build-cache cleaner removed fingerprints from a target dir cargo never used, and cargo kept linking stale code. This is register row A01 (register; living document Part 3, duplication).I checked cargo's precedence with real cargo 1.93:
warning: both .cargo/config and .cargo/config.toml exist. Using .cargo/config.Changes
ConfigDirs::readnow takes its file list fromconfig_layer_paths. I deleted its own ancestor walk, its$CARGO_HOMEpush and its file pick, keeping only the[build] target-dir/build-dirlookup. Two side effects:$CARGO_HOMEis no longer read twice when an ancestor's.cargois that directory, and the$CARGO_HOMEbase falls back to the home itself, not"", when it has no parent./code-review highfound this in the touched function; it was already broken onmain.--cwddefaults to., andPath::new(".").ancestors()stops at., so from a workspace member the cleaner found neither the workspace root nor a parent.cargo/config, and invalidated no build dir.build_dirs_withnow walksstd::path::absolute(cwd)with..folded byrelpath::normalize_lexically_keeping_escapes. This is the same foldcargo_crawler::resolve_fordoes. Without the fold,--cwd ../appwould walk the directory it left, which Bugbot caught.group_commit::existsfirst, asread_regular_to_string_syncdoes. The old code chose the file by attempting the capture-aware read.ConfigDirscomment no longer restates the (wrong) file order; it points atconfig_layer_paths.Tests
build_dirs_read_the_legacy_config_when_both_exist: with both files present, the build dirs are exactly[legacy]. Red on the old pick:left: [".../ws/modern"] right: [".../ws/legacy"].build_dirs_walk_above_a_relative_cwd(#[serial], chdir, asnuget_config::a_relative_start_reads_parent_configsdoes)."."from a member finds the parent config'sout; red withoutabsolute:left: [] right: [".../ws/out"]."../ws/member"from a sibling ignores the sibling'sbuild-dir; red without the fold:left: [ws/out, left/wrong].build_dirs_honours_env_config_and_workspace_rootand thesource_replacementtests are unchanged and pass.Size
git diff --stat: 1 file, +78/−16. Production is about +12/−16 and tests +66.Behavior change
Bug fixes only: the cleaner now targets the directory cargo builds into when both config files exist and when
--cwdis relative. No output bytes, JSON shapes or exit codes change.Review findings not fixed
$CARGO_HOME-dedup test here.config_layer_pathshas its own tests insource_replacement.rs.Verification
cargo clippy --workspace --all-features -- -D warningsis clean.cargo test -p socket-patch-core --lib: 6991 passed and 4 failed. All 4 are read-only-permission tests incopy_tree,vlt_heal,pypi_poetryandpypi_requirementsthat fail only because the sandbox runs as root. They are unrelated to this diff.cfg(windows)/cfg(unix)code.Risk
Low. It touches one private function in one file, whose callers are
applyandrollbackviainvalidate_project.🤖 Generated with Claude Code
https://claude.ai/code/session_011f9aeQ57i1EpKYcB1Ks8rf
Note
Low Risk
Scoped to private build-directory discovery used by apply/rollback cache invalidation; behavior fixes only, with no auth or data-path changes.
Overview
Fixes build-cache invalidation after apply/rollback so it clears fingerprints in the same
target/build-dircargo actually uses.ConfigDirs::readno longer walks.cargoconfig on its own. It reusesconfig_layer_paths, matching cargo’s precedence (.cargo/configover.cargo/config.tomlwhen both exist). That stops the cleaner from wiping the wrong directory and leaving stale.rliblinks.build_dirs_withnow resolves a relative--cwd(e.g..or../ws/member) viaabsolute+ lexical normalize before ancestor walks, so workspace roots and parent configs are found when run from a member crate. Config file probing uses the samegroup_commit::existscheck as reads elsewhere.Two tests lock in legacy-config precedence and relative-cwd discovery; no CLI output or exit-code changes.
Reviewed by Cursor Bugbot for commit 2c715eb. Configure here.
Generated by Claude Code