fix(repo): purge PR #178 WIP dump and stop git escaping into the real repository - #201
Merged
Conversation
added 5 commits
August 10, 2026 20:02
PR #178 ("Recovery/mcb sanitized wip") merged 619 files / +135,696 lines into develop without review, versioning one workstation's configuration and session residue as if it were source. The live consequence was a red CI: .cargo/config.toml pins linker = "clang" with -fuse-ld=mold, and mold does not exist on GitHub runners, so run 31430407703 failed at exit code 2 with even libc, serde, quote and proc-macro2 build scripts unable to link. The mold error was the symptom; the disease is that CI ran under a build contract that only exists on the operator machine. Untrack, keeping files on disk: - machine-local config: .cargo/, .vscode/, .waza.yaml, opencode.jsonc - tool output: *violations*.txt (6), qlty.*.sarif, build_err.txt, clippy_output.txt, validate_output.txt, test_003_warnings.txt, pr116_comments.txt, staged_files.txt, coverage/lcov.info - artifacts: libdefine_validator_test.rlib, data/vectors/, tests/mcb.2026-02-10 (server stdout), test_file.txt - scratch: tmp_sisyphus_test.txt, .beads.gate.lock - context/: parallel docs tree outside the docs/ SSOT that documented dill and figment, both removed by ADR-050 and ADR-051 Harden .gitignore, the barrier that let the dump in. It carried an explicit !.vscode/settings.json exception forcing personal editor state to be tracked, and a dead !.github/workflows/release.yml rule for a file since renamed to release-binaries.yml. Both corrected, and each purged class now has a blocking rule, verified by an empty git status --untracked-files=all. Net: -82,181 lines. Refs: mcb-okll
run_git set current_dir but left the inherited git environment intact. git resolves GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE and GIT_CONFIG ahead of the working directory, so whenever the suite ran inside a git operation — a pre-commit or pre-push hook, a rebase, a merge — every helper call was redirected from its TempDir onto the surrounding real repository. Reproduced by exporting only GIT_DIR before the suite: 10 of 17 vcs tests fail with "could not lock config file /home/marlonsc/mcb/.git/config" and "Unable to create .git/worktrees/purge-pr178-wip-dump/index.lock". The decisive symptom is a TempDir `git commit` executing the real repository's pre-commit pipeline, running the project's own Python suite and reformatting a source file from inside a unit test. run_git now clears the six inherited path variables, disables system and global config, and points core.hooksPath at /dev/null, so a test repository cannot reach outside itself or execute project tooling. Same command and same exported GIT_DIR after the fix: 17 passed, and the run drops from 22.68s to 0.07s now that tests no longer trigger the host hook pipeline. Covered by tests asserting the helper repository is self-contained, carries its initial commit through git's own surface, and is not captured by an ambient work tree — exercised through the public helper API, with no process-wide environment mutation so the suite stays parallel-safe. Refs: mcb-k5yl
test_gap2_vcs_list_repositories_discovers_repos drove git through its own
raw Command::new("git"), duplicating logic the domain test helper already
owns. That copy inherits GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE, so
under a pre-push hook the fixture repository was created against the
surrounding repository instead of the temp dir, and the assertion found no
discovered repo.
Route the fixture through run_git, the single owner, which clears the
inherited variables and disables hooks and system/global config.
Reproduced with the hook environment exported: the test failed before and
now passes; gap_fixes_e2e is 6 passed under the same conditions.
Refs: mcb-k5yl
… helper
create_git_repo_fixture drove git through its own raw Command::new("git"),
the third copy of logic the domain test helper owns. That copy inherits
GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE, so under a pre-push hook the
fixture was created against the surrounding repository and
test_vcs_index_repository_success plus the three
test_vcs_list_repositories_cases found no repository.
Route the fixture through run_git, the single owner, and set the commit
identity on the repository itself now that the helper no longer reads
global config.
Reproduced with the hook environment exported: the four tests failed
before and handlers::vcs_tests is now 7 passed under the same conditions.
Refs: mcb-k5yl
enumerate_with_git passed `-C workspace_root`, but git resolves GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE and GIT_COMMON_DIR ahead of `-C`. Whenever validation runs from inside a git operation — a pre-commit hook, a rebase, a merge — the inherited environment redirected `git ls-files` to the surrounding repository, so the inventory described a different tree than the one the caller asked about. This is a production defect, not a test-only concern: validation is invoked from the pre-commit path, exactly where the environment is set. Clear the inherited variables before invoking git, so the inventory always describes workspace_root. The fixture in run_context_tests drove git through its own raw Command as well; it now routes through the shared run_git helper. Reproduced with the hook environment exported: the test failed before and run_context_tests is now 2 passed under the same conditions. Refs: mcb-k5yl
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (76)
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 |
Contributor
PR Summary by QodoPurge accidental WIP dump; isolate spawned git from ambient repository state
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Confidence score: 4/5
- In
crates/mcb-domain/tests/unit/utils/git_helpers_tests.rs, the isolation check is currently vacuous because the child git process points to a non-existentGIT_DIR, so the assertion can pass without proving isolation; this could mask real repo-escape regressions in future changes — create the decoy git dir (or assert the command succeeds before evaluating output). - In
crates/mcb-domain/src/utils/tests/git_helpers.rs, cleanup does not clearGIT_CONFIG_COUNTand relatedGIT_CONFIG_KEY_<n>/GIT_CONFIG_VALUE_<n>variables, so test-level git config injection can leak between tests and produce flaky or misleading results — explicitly unset the full injected env variable set during teardown.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/mcb-domain/src/utils/tests/git_helpers.rs">
<violation number="1" location="crates/mcb-domain/src/utils/tests/git_helpers.rs:25">
P3: The stated goal is that a test repository can never reach outside itself, but the cleanup leaves git's environment-config injection variables uncleared. `GIT_CONFIG_COUNT` together with `GIT_CONFIG_KEY_<n>`/`GIT_CONFIG_VALUE_<n>` (and `GIT_CONFIG_PARAMETERS`) let an ambient environment feed config into every git invocation regardless of `-c`/repo config, so config from the surrounding hook environment (e.g. `core.hooksPath`, `safe.directory`, `user.*`) can still leak into the test repo. Worth clearing them alongside the six list entries for full isolation. If you're confident the CI/hook environment never sets these, this can be deferred, but it's the remaining vector for the environment leakage this PR targets.</violation>
</file>
<file name="crates/mcb-domain/tests/unit/utils/git_helpers_tests.rs">
<violation number="1" location="crates/mcb-domain/tests/unit/utils/git_helpers_tests.rs:49">
P2: The isolation test passes vacuously: the child git is pointed at a GIT_DIR (decoy.path()/decoy.git) that is never created, so it errors out (exit 128) with empty stdout and the assert `!seen.contains(decoy.path())` is satisfied regardless of whether the helpers actually isolate. The decoy needs to be a real git repository (e.g. initialized work tree with a valid GIT_DIR) so a leaking implementation would resolve toplevel to the decoy and fail the assert; otherwise a regression in run_git isolation ships undetected.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| let output = Command::new("git") | ||
| .args(["rev-parse", "--show-toplevel"]) | ||
| .current_dir(path) | ||
| .env("GIT_DIR", decoy.path().join("decoy.git")) |
There was a problem hiding this comment.
P2: The isolation test passes vacuously: the child git is pointed at a GIT_DIR (decoy.path()/decoy.git) that is never created, so it errors out (exit 128) with empty stdout and the assert !seen.contains(decoy.path()) is satisfied regardless of whether the helpers actually isolate. The decoy needs to be a real git repository (e.g. initialized work tree with a valid GIT_DIR) so a leaking implementation would resolve toplevel to the decoy and fail the assert; otherwise a regression in run_git isolation ships undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/tests/unit/utils/git_helpers_tests.rs, line 49:
<comment>The isolation test passes vacuously: the child git is pointed at a GIT_DIR (decoy.path()/decoy.git) that is never created, so it errors out (exit 128) with empty stdout and the assert `!seen.contains(decoy.path())` is satisfied regardless of whether the helpers actually isolate. The decoy needs to be a real git repository (e.g. initialized work tree with a valid GIT_DIR) so a leaking implementation would resolve toplevel to the decoy and fail the assert; otherwise a regression in run_git isolation ships undetected.</comment>
<file context>
@@ -0,0 +1,119 @@
+ let output = Command::new("git")
+ .args(["rev-parse", "--show-toplevel"])
+ .current_dir(path)
+ .env("GIT_DIR", decoy.path().join("decoy.git"))
+ .env("GIT_WORK_TREE", decoy.path())
+ .env("GIT_INDEX_FILE", decoy.path().join("decoy.index"))
</file context>
| /// Returns an error if the git command fails. | ||
| pub fn run_git(dir: &Path, args: &[&str]) -> TestResult<()> { | ||
| let output = Command::new("git") | ||
| const INHERITED_GIT_VARS: [&str; 6] = [ |
There was a problem hiding this comment.
P3: The stated goal is that a test repository can never reach outside itself, but the cleanup leaves git's environment-config injection variables uncleared. GIT_CONFIG_COUNT together with GIT_CONFIG_KEY_<n>/GIT_CONFIG_VALUE_<n> (and GIT_CONFIG_PARAMETERS) let an ambient environment feed config into every git invocation regardless of -c/repo config, so config from the surrounding hook environment (e.g. core.hooksPath, safe.directory, user.*) can still leak into the test repo. Worth clearing them alongside the six list entries for full isolation. If you're confident the CI/hook environment never sets these, this can be deferred, but it's the remaining vector for the environment leakage this PR targets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/src/utils/tests/git_helpers.rs, line 25:
<comment>The stated goal is that a test repository can never reach outside itself, but the cleanup leaves git's environment-config injection variables uncleared. `GIT_CONFIG_COUNT` together with `GIT_CONFIG_KEY_<n>`/`GIT_CONFIG_VALUE_<n>` (and `GIT_CONFIG_PARAMETERS`) let an ambient environment feed config into every git invocation regardless of `-c`/repo config, so config from the surrounding hook environment (e.g. `core.hooksPath`, `safe.directory`, `user.*`) can still leak into the test repo. Worth clearing them alongside the six list entries for full isolation. If you're confident the CI/hook environment never sets these, this can be deferred, but it's the remaining vector for the environment leakage this PR targets.</comment>
<file context>
@@ -8,13 +8,40 @@ use std::path::Path;
/// Returns an error if the git command fails.
pub fn run_git(dir: &Path, args: &[&str]) -> TestResult<()> {
- let output = Command::new("git")
+ const INHERITED_GIT_VARS: [&str; 6] = [
+ "GIT_DIR",
+ "GIT_WORK_TREE",
</file context>
Contributor
Code Review by Qodo
1. Nonportable git null paths
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+41
to
+44
| .env("GIT_CONFIG_NOSYSTEM", "1") | ||
| .env("GIT_CONFIG_GLOBAL", "/dev/null") | ||
| .arg("-c") | ||
| .arg("core.hooksPath=/dev/null") |
Contributor
There was a problem hiding this comment.
1. Nonportable git null paths 🐞 Bug ☼ Reliability
run_git() hardcodes /dev/null for GIT_CONFIG_GLOBAL and core.hooksPath, which makes behavior platform-dependent (notably on Windows runners in CI) and can cause git commands in tests to fail or behave inconsistently across OSes.
Agent Prompt
## Issue description
`run_git()` currently disables global/system config and hooks using hardcoded Unix paths (`/dev/null`). This is OS-dependent and can behave differently (or fail) on Windows/macOS/Linux.
## Issue Context
The helper is now used by multiple test suites, and CI includes a Windows job, so any OS-specific assumption here affects build/test reliability.
## Fix Focus Areas
- crates/mcb-domain/src/utils/tests/git_helpers.rs[24-55]
## Suggested fix
- Replace `GIT_CONFIG_GLOBAL=/dev/null` with a portable approach:
- Option A: create an empty temp file inside `dir` (e.g. `dir/.gitconfig-empty`) and point `GIT_CONFIG_GLOBAL` to that path.
- Option B: conditionalize the null device (`"NUL"` on Windows, `"/dev/null"` elsewhere).
- Replace `core.hooksPath=/dev/null` with a real empty directory:
- Ensure a directory exists under `dir` (e.g. `dir/.mcb-empty-hooks/`) before invoking git, then set `core.hooksPath=<that-dir>`.
- This avoids relying on `/dev/null` being a directory-like sink and stays portable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+53
to
+55
| .output()?; | ||
|
|
||
| Ok(String::from_utf8_lossy(&output.stdout).trim().to_owned()) |
Contributor
There was a problem hiding this comment.
2. Git isolation test false-positive 🐞 Bug ⚙ Maintainability
toplevel_seen_with_hook_environment() ignores the child git exit status and returns trimmed stdout even on failure, so helper_repository_is_not_captured_by_ambient_git_environment can pass with an empty stdout rather than actually proving isolation.
Agent Prompt
## Issue description
The new isolation test can incorrectly pass when the injected hook-like environment causes `git rev-parse` to fail (non-zero status). Because the test does not assert success, stdout may be empty and still satisfy the negative assertion.
## Issue Context
These tests are meant to guard against regressions where helper git commands escape into the surrounding repository; a false-positive weakens that protection.
## Fix Focus Areas
- crates/mcb-domain/tests/unit/utils/git_helpers_tests.rs[42-105]
## Suggested fix
- In `toplevel_seen_with_hook_environment()`, check `output.status.success()` and return an error (including stderr) when it is not.
- If you truly want to allow “refuses” as a valid outcome, make that explicit and assert on the specific failure mode (e.g., expected stderr message), rather than silently treating any failure as success.
- Optionally strengthen the test by creating an actual decoy repo in `decoy` (via `run_git`) so a leaking implementation deterministically resolves to the decoy path and the assertion becomes meaningful.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
-3
to
-6
| # MANDATORY: sccache as the Rust compiler wrapper. Set here so EVERY cargo | ||
| # invocation uses it, even when running `cargo check` / `cargo build` directly | ||
| # without make. sccache eliminates redundant rebuilds across sessions. | ||
| rustc-wrapper = "sccache" |
Contributor
There was a problem hiding this comment.
3. Stale cargo-config references 🐞 Bug ⚙ Maintainability
After deleting .cargo/config.toml and adding /.cargo/ to .gitignore, multiple docs/comments still state that .cargo/config.toml sets rustc-wrapper, job limits, and env vars, which is now misleading for contributors and CI expectations.
Agent Prompt
## Issue description
`.cargo/config.toml` is removed/blocked from git, but repo documentation and Make comments still describe it as an active configuration source (sccache wrapper, jobs, env vars). This misleads developers about how builds are configured now.
## Issue Context
The PR’s intent is to purge machine-local configuration; documentation should reflect the new source of truth (or remove the claim if there is no replacement).
## Fix Focus Areas
- custom.mk[8-11]
- docs/developer/BENCHMARK.md[88-101]
- docs/developer/SERENA.md[339-395]
- .gitignore[283-292]
## Suggested fix
- Update `custom.mk` comment to remove the claim that `.cargo/config.toml` sets `rustc-wrapper`.
- Update BENCHMARK.md / SERENA.md to either:
- describe the new mechanism for these settings (if they still exist), or
- explicitly state they were removed and why.
- Ensure any CI docs mentioning `.github/workflows/ci.yml` actually match the workflow configuration.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
marlonsc
approved these changes
Aug 11, 2026
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
2 participants
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.
Why
CI has been red on
develop. The failure surfaced as a linker error, but thelinker was a symptom.
.cargo/config.tomlpinslinker = "clang"with-fuse-ld=mold. GitHubrunners have no
mold, so run 31430407703 failed at exit code 2 with evenlibc,serde,quoteandproc-macro2build scripts unable to link. CI wasexecuting under a build contract that exists only on one workstation.
That file reached
developthrough PR #178 ("Recovery/mcb sanitized wip"),which merged 619 files / +135,696 lines without review. Its lineage is
entirely
chore(checkpoint): temporary work-in-progress snapshotandchore(emergency): checkpoint worktree before cleanup— machine configurationand session residue versioned as if it were source.
What this changes
1. Purge the dump and close the barrier that let it in (
54ae30a6d)Untracked, kept on disk:
.cargo/,.vscode/,.waza.yaml,opencode.jsonc*violations*.txt,qlty.*.sarif,build_err.txt,clippy_output.txt,validate_output.txt,test_003_warnings.txt,pr116_comments.txt,staged_files.txt,coverage/lcov.infolibdefine_validator_test.rlib,data/vectors/,tests/mcb.2026-02-10(server stdout),test_file.txttmp_sisyphus_test.txt,.beads.gate.lockcontext/— parallel tree outside thedocs/SSOT, documentingdillandfigment, both removed by ADR-050 / ADR-051.gitignorecarried an explicit!.vscode/settings.jsonexception forcingpersonal editor state to be tracked, plus a dead
!.github/workflows/release.ymlrule for a file long renamed to
release-binaries.yml. Both corrected; everypurged class now has a blocking rule, each anchored with a leading
/so testfixtures that legitimately ship their own
.cargo/orcontext/stay tracked.Verified by an empty
git status --untracked-files=all.2. Stop test and validation code from escaping into the real repository
(
282f1fa8e,582196b7a,e4761a80b,338300c8c)Removing
.cargo/config.tomllet the pre-push gate run properly for the firsttime, and it immediately caught a second, deeper defect.
gitresolvesGIT_DIR,GIT_WORK_TREE,GIT_INDEX_FILE,GIT_CONFIG,GIT_OBJECT_DIRECTORYandGIT_COMMON_DIRahead of both the workingdirectory and the
-Cflag. Any code invokinggitfrom inside a gitoperation — a hook, a rebase, a merge — is silently redirected onto the
surrounding repository.
The decisive symptom: a
git commitinside a testTempDirexecuted thisrepository's pre-commit pipeline — running the project's Python suite and
reformatting a source file from within a unit test.
Fixed at the shared owner (
run_git), then the three consumers that hadduplicated the logic with a raw
Command::new("git").One of them is a production defect, not a test concern:
mcb-validate::run_context::enumerate_with_gitpassed-C workspace_rootbutinherited
GIT_DIRoverrode it, so the validator listed files from the wrongrepository when invoked from the pre-commit path where that variable is set.
Evidence
RED → GREEN under the exact failing condition (hook environment exported):
mcb-providersvcsmcb-servere2e goldenmcb-servere2e gap_fixesmcb-serverintegration vcsmcb-validaterun_contextFull gate
MCB_MODEL_ID=test-model bash scripts/lib/mcb.sh run cargo test --workspace --all-targets→ exit 0, 1759 tests ok, 0 failures.Push accepted with the complete pre-push chain green: ruff, mypy, 67 pytest,
cargo fmt --check,clippy -D warnings, workspace tests, doc tests,validate quick,guard: clean.Baseline control:
origin/develop@04ed18520in a clean worktree ranmcb-servere2e = 145 passed — confirming these failures were the environmentleak surfacing once the gate could run, not regressions introduced here.
Net
−82,144 LOC.
Refs: mcb-okll, mcb-k5yl
Summary by cubic
Removes the accidental PR #178 WIP dump and hardens git isolation so CI runs reliably and tests don’t affect the real repo. Restores green CI, fixes a production validation bug, and removes ~82k lines of junk and machine-local files.
Bug Fixes
.cargo/config.tomlthat pinnedclang/-fuse-ld=mold, which broke GitHub runner linking.GIT_*vars and disabling hooks/system/global config in the sharedrun_githelper and inmcb-validate::enumerate_with_git(production fix).Refactors
context/docs tree..gitignorerules at repo root and removed bad exceptions; verified clean status.Linear: aligns with mcb-okll (purge WIP dump) and mcb-k5yl (git isolation).
Written for commit 7ca73c1. Summary will update on new commits.