Repository navigation
fix(test): scrub the git environment in every test-fixture git write (#15246) - #15286
Merged
Merged
Conversation
…15246) A test that runs `git init`/`add`/`commit`/etc. in a tmp_path fixture with subprocess.run's default of inheriting os.environ writes to whatever GIT_DIR names under the pre-push hook -- the pushing worktree's own git directory, not the fixture -- exactly as autobot_shared/paths_test.py did when it dropped 86 tracked files from the live index. Fixes every other unscrubbed git write this sweep found and adds a durable guard against a fourth recurrence.
…it helper (#15246) _git() in services/knowledge/code_indexer.py passed -C root_dir with no env override, so an inherited GIT_DIR (this repo's own pre-push hook exports one) silently answers for the wrong repository instead of root_dir. Found live: this repo's own pre-push hook run of code_graph_provenance_test.py -- itself fixed in this PR to no longer corrupt the real repo -- failed for exactly this reason once the fixture repo was no longer confusable with the real one.
git rev-parse --show-toplevel/--git-common-dir had no protection from an inherited GIT_DIR. Every checkout in this repo is a worktree, and the pre-push hook exports GIT_DIR pointing at the pushing worktree's own git directory -- the installer then silently resolves the wrong repository root and destination for every downstream git call. Reproduced live: this repo's own pre-push hook run of repo_tests/git_hooks_installer_test.py installed hooks into the real repository instead of the test's throwaway fixture, once the fixture itself was no longer confusable with the real one (#15246's own fix). Unset once at the top, the shell-side equivalent of autobot_shared.paths.scrubbed_git_env().
…it reads (#15246) _tracked_files() and _is_executable() ran git ls-files with no env override, so an inherited GIT_DIR silently answers for the wrong repository instead of root. Reproduced live: this repo's own pre-push hook run of check_script_exec_bits_test.py, which calls these functions directly in-process (not through a subprocess whose env the test controls), failed once its own fixture was no longer confusable with the real repo.
…t-write guard (#15246) scripts/test_first_remediation.py matches the guard's own test-file naming heuristic (test_*.py, pytest's own collection glob) but is a CLI tool that deliberately creates real worktrees and pushes real branches -- the opposite of a throwaway fixture. The guard found it live on this PR's own push.
…ads (#15246) _is_git_write treated ANY dynamic argv element as making the verb unknowable, not just a dynamic element at the verb's own position -- so `git -C str(root) ls-files "*.py"` (a read, common and correct elsewhere in this suite) misclassified as an unresolvable write. Also: trusted_names now resolves `env = _test_git_env()` (a local variable holding an already-trusted wrapper's result, pre-commit-hardcoded-values_test.py's actual shape) and env={"PATH": ..., ...} with no `**` unpacking is accepted outright, since it cannot carry an ambient GIT_DIR it was never given. Fixes repo_tests/unreferenced_shell_script_test.py's one remaining genuine finding. All caught by the guard's own full-repository self-check on this PR's push.
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…branch commands (#15246) release()'s git worktree remove --force and git branch -D/-d, and _git_add_worktree()'s two git worktree add calls, ran with cwd=str(root) but no env= at all -- an inherited GIT_DIR overrides cwd, so under the pre-push hook this can force-remove a worktree and force-delete a branch in the real repository instead of the throwaway one autobot-backend/tasks/ test_task_workspace.py builds in tmp_path. Reachable in-process (allocate/ release/cleanup_stale are called directly, not via subprocess), so no test-side env= could have protected it -- the same shape as code_indexer.py and check_script_exec_bits.py, found via review rather than the guard, since the guard is scoped to test files. Verified every caller (heartbeat_scheduler, workspace_cleanup, work_item_service/handoff's release_for_task) passes an explicit repo_root or relies on _REPO_ROOT, never on ambient git state. A systematic re-sweep for the same shape (production code reachable in-process from a test with a temporary-repository fixture) found no further sites; broader unscrubbed production git calls with no test coverage at all are filed as #15287, out of scope for this issue.
Tracked 100644 despite .pre-commit-config.yaml invoking it as a bare `entry: tools/lint/check_git_write_env_scrubbed.py` (language: python), so pre-commit could not execute it -- the guard against tests silently corrupting the repository was itself silently not running. core.fileMode is not reliable here, so `git update-index --chmod=+x` (not a plain `chmod +x`) is what actually reaches the index; the working-tree bit is set to match so no unstaged mode diff remains.
…it-write guard (#15246) The reach-floor failure message's print() lacked the '# noqa: print' annotation its two siblings in this file already carry (and that check_git_toplevel_env_scrubbed.py, the guard this one is modeled on, uses throughout) -- the actual CI-blocking finding ("1 violation(s) on lines this PR added") was this line, not scripts/check_script_exec_bits.py's pre-existing prints, which the same hook run explicitly reported as "3 pre-existing violation(s) on lines this PR did not touch" and "Not failing the build for these." A CLI lint reporter's own findings-to-stderr output is the sanctioned exception here, not a logger call -- matching the established sibling pattern exactly.
…-pr_test.py to 747 (#15246) This PR's env-scrub edit shrank the file by four lines (751 -> 747), and the ratchet audits down as well as up: a ceiling left above the real size would silently re-permit growth back to the old number. Verified `git show HEAD:pipeline-scripts/check-pre-commit-hook-pr_test.py | wc -l` == 747 before editing. Lowered both KNOWN_LARGE (scripts/ python_file_size_known_large.py) and its paired RATCHET_BASELINE (repo_tests/python_file_size_ratchet_baseline.py) -- the audit checks them together. Cross-referenced every other file this PR touches against both tables: code_indexer.py is the only other entry among them (1179, unchanged, matches exactly); the remaining ~32 touched files carry no KNOWN_LARGE entry at all and stayed comfortably under 600 lines.
5 tasks
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
1 participant
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.
Closes #15246
Thinking Path
Started from the issue's premise that one found-while-changing-something instance is not evidence there is only one, so treated the twelve named files as a floor, not the answer: grepped every
_test.py/test_*.pyfor asubprocess-family call plus agitliteral, then hand-classified each hit as a genuine unscrubbed write, an already-scrubbed write, a deliberate real-repo read (exempt), or a false positive (agitmention in prose/a mocked call/a non-git command). That produced a ~60-file candidate list and, after classification, ~27 files with a genuine unscrubbed write.Verified criteria 1/2 (fresh worktree,
git add -Astages nothing) still hold post-#15253/#15262 before touching anything else:git status --porcelain=v1on a freshgit worktree addreturns 0 entries.Built the durable guard (
tools/lint/check_git_write_env_scrubbed.py) against the same census, matching the existingcheck_git_toplevel_env_scrubbed.py(#15176) precedent's AST-based shape rather than inventing a second mechanism. Then used the guard's own full-repository self-check as a second, independent sweep on every push — this repo's own pre-push hook runs the touched tests, and each run surfaced something the manual grep-based census had missed: a production function with the identical bug (only reachable because the test fixture was no longer confusable with the real repo, so the test's assertion started depending on the production code being right too), an operational CLI tool that matched the guard'stest_*.pynaming heuristic without being a test, and two classifier bugs in the guard itself (a dynamic path argument after an already-resolved read verb misclassified as an unknowable write; aenv = <trusted-function>()local-variable indirection the trust resolver didn't chase). Each is its own commit, in the order it was found.What Changed
Sweep and fix (test files, all via
autobot_shared.paths.scrubbed_git_env()):autobot-infrastructure/shared/scripts/hooks/pre-commit-*_test.pyfiles +lib/_common_test.py— each had a_git/_make_git_repofixture helper (init/config/add/commit) and severalbash <hook>invocations with noenv=at all.pipeline-scripts/check-pre-commit-hook-pr_test.py,pipeline-scripts/check_baseline_no_growth_test.py— same shape; the former needed a byte-tight rewrite to stay within its recordedKNOWN_LARGEline ceiling (751) while adding the fix.repo_tests/bandit_exclude_anchoring_test.py,repo_tests/flake8_exclude_anchoring_test.py— agit init -qin atmp_pathfixture (theirgit ls-filesreads against the real repo root are correctly untouched — deliberate).repo_tests/git_hooks_installer_test.py,repo_tests/release_range_test.py,scripts/lint_conventions_test.py,scripts/verify_done_test.py,scripts/check_script_exec_bits_test.py,repo_tests/unreferenced_shell_script_test.py— same_githelper shape.autobot-backend/services/knowledge/code_graph_provenance_test.py,autobot-backend/tasks/test_task_workspace.py— same shape, no shared helper (inlinesubprocess.runper call site).scripts/check_script_exec_bits_test.py's_GIT_ENVlooked scrubbed ({**os.environ, "GIT_CONFIG_GLOBAL": ...}) but only neutralised two config vars, notGIT_DIR— the same bug wearing a name that reads like the fix.Fixed but out of the test-file sweep (production code reachable in-process from a test's own git fixture, so no test-side
env=could protect it — found via the guard's own live pre-push runs and, for the last one, review):autobot-backend/services/knowledge/code_indexer.py's_git()— reachable in-process fromcode_graph_provenance_test.py.scripts/check_script_exec_bits.py's_tracked_files()/_is_executable()— reached in-process bycheck_script_exec_bits_test.py.scripts/install-git-hooks.sh(shell) —git rev-parse --show-toplevelwith no protection;repo_tests/git_hooks_installer_test.pyinvokes it as a real subprocess, sounset GIT_DIR GIT_WORK_TREE GIT_COMMON_DIR GIT_INDEX_FILEat the top of the script is the shell-side equivalent ofscrubbed_git_env()(thecheck_git_toplevel_env_scrubbed.pydocstring already names this as the intended shell fix, just not previously implemented anywhere).autobot-backend/services/task_workspace.py'srelease()(git worktree remove --force,git branch -D/-d) and_git_add_worktree()(twogit worktree addforms) — reachable in-process fromautobot-backend/tasks/test_task_workspace.py, which callsallocate/release/cleanup_staledirectly. Found in review, not by the guard (production files are out of its scope — see below). Verified every caller (heartbeat_scheduler.py,tasks/workspace_cleanup.py,llc/services/work_item_service.pyandhandoff.py'srelease_for_task) passes an explicitrepo_rootor falls back to the module's static_REPO_ROOT; none depends on an ambient git environment, so scrubbing changes nothing about which repository each caller targets.In-process production sweep, repeated with this shape specifically in mind ("production code reachable in-process from a test that constructs a temporary repository"): re-swept all git-invoking production code (
subprocess.run/Popen/check_output/check_callandasyncio.create_subprocess_exec— the latter family wasn't in the original census's grep pattern) and, for every unscrubbed site, checked whether any test imports the containing module and reaches it in-process. Found zero further sites: task_workspace.py was the only one. Every other unscrubbed production git call (skills/promoter.py::_lint_and_commit— agit add/git commitwith nocwd=at all, the worst-shaped one found;api/git_mcp.py;utils/branch_metrics.py;autobot-slm-backend/services/git_tracker.py;api/analytics_code_review.py/analytics_precommit.py;code_intelligence/precommit_analyzer.py/code_review_engine.py;scripts/utilities/optimize_agents.py;deployment_rollback.py;migrate_tests.py) is either never called from any test at all or is fully mocked/patched at every call site — confirmed per-file, not inferred. Filed as #15287 rather than fixed here: each needs the same caller-by-caller verification task_workspace.py just got, none is causing a test (or this PR's own pushes) to fail today, and rushing that verification under a push-is-blocked clock is the wrong way to do it.Exempted, with the reason recorded at each site (no code change):
autobot_shared/paths_test.py,.../pre-commit-hardcoded-values_test.py,co_change_test.py/git_history_crawler_test.py,repo_tests/collection_coverage_test.py— already scrubbed by prior PRs (fix(guards): scrub the git environment before asking git for the repo root (#15176) #15243, fix(guards): scope hardcoded-value test exemption to repo_tests/ support modules (#15273) #15282, flaky(code-intelligence): co_change_test fixture dies on a git exit-128 part-way through building its repo #13882-erahermetic_git_env).repo_tests/git_repo_root_scrub_test.py— the bug(guards): five scripts resolve the repo root without scrubbing GIT_DIR — two run as pre-commit hooks #15176 companion; deliberately exports an ambientGIT_DIRto reproduce the defect, but only for read-only--show-toplevelcalls, never a write.git ls-files/rev-parsewith an explicitcwd=<real repo root>— deliberate real-repo reads (repo_tests/*_anchoring_test.py,*_declared_test.py,tools/lint/check_git_safe_directory_test.py, etc.).scripts/test_first_remediation.py— an operational CLI tool (real worktrees, real pushes), not a test; allowlisted in the guard.Guard
tools/lint/check_git_write_env_scrubbed.py(+_test.py), wired into.pre-commit-config.yamlasgit-write-env-scrubbed, scoped to_test.py/test_*.pyfiles. AST-based, mirroringcheck_git_toplevel_env_scrubbed.py's shape: resolvessubprocessbindings, classifies agitargv's verb (skipping-C/-cflag pairs) againstWRITE_VERBS, and acceptsenv=when it resolves — directly, through a local trusted wrapper/variable, or as an unpack-free dict literal — toscrubbed_git_env/hermetic_git_env.Reach floor:
TEST_FILE_FLOOR = 1500against 2064 tracked_test.py/test_*.pyfiles measured at write time (scan_repo()walks and counts;main()fails loudly, not clean, if the walk lands below the floor).Scope recommendation (production vs. test files): left the guard scoped to test files, and filed #15287 rather than extending it here. The guard's AST machinery is reusable for production, but the production sweep above shows why extending it now would be premature: production callers are far more varied (an MCP tool surface, Celery workers, ansible tooling, CLI scripts with no tests at all) than a pytest fixture's, so a production-scoped version needs its own reach floor and a caller-by-caller review per write site before each is fixed — exactly the verification this PR just did for task_workspace.py, not a mechanical
env=insertion. That review is #15287's job; the guard (or a second one scoped to production) can follow once enough of that list is closed to know what a production allowlist actually needs to say.Contrast mutation:
test_the_unscrubbed_helper_is_reported/test_the_scrubbed_helper_is_acceptedrun the identical_githelper shape this PR fixed twice, differing by an added import plus theenv=keyword the import makes resolvable — the unscrubbed fixture is reported (1 finding,#15246in the message), the scrubbed one is clean.test_a_walk_below_the_floor_fails_main_loudlycovers the reach-floor failure mode directly (monkeypatchedscan_reporeturning(0, [])must still failmain, not pass it).Verification
python3 -m py_compile(parses/compiles, does not execute) andpython3 -m pyflakes— no new unused imports or syntax errors; three pre-existing unused-import findings (_common_test.py,pre-commit-no-tag-pinned-action_test.py,check_baseline_no_growth_test.py) confirmed present onorigin/Dev_new_guibefore this PR, left alone as out of scope.black --line-length 120 --checkagainst both the new and the pre-PR version, confirmed identical pre-existing-debt counts (0 new debt introduced anywhere; one file's debt count improved incidentally).pipeline-scripts/check-pre-commit-hook-pr_test.pyandautobot-backend/services/knowledge/code_indexer.pystayed at/under their recordedKNOWN_LARGE/ratchet line ceilings (747/751 and 1179/1179) — measured withwc -l, no ceiling raised.git status --porcelain=v1on a freshgit worktree addfromorigin/Dev_new_guireturns 0 entries — re-verified at this PR's base commit, not re-implemented.git pushran the repo's real pre-push hook five times against the touched tests over the course of this PR (including the coordinator-review round that foundtask_workspace.py); four runs failed on exactly the defect class this issue is about (each fixed in its own commit, described above), the fifth passed. No--no-verifyused at any point, and a remote-sideMerge branch 'Dev_new_gui'commit that appeared on the PR branch mid-review was reconciled with a normalgit merge(no rebase, no force-push).git push, exactly as the issue's hard constraints require.Model Used
Claude Opus 5 (1M context)