fix: require GIT_CONFIG for fixture-isolation credit and join shell continuations

[kyle-sexton]

Closes #2889
Closes #2893

Summary

scripts/check-fixture-git-isolation.sh now requires GIT_CONFIG in the CREDIT signal, next to GIT_DIR and GIT_WORK_TREE. Official git-config ENVIRONMENT docs: if no --file is given, GIT_CONFIG is used as --file. That is a second leak path, not another spelling of discovery — git -C <fixture> config user.email X still writes through an inherited GIT_CONFIG after GIT_DIR is cleared. Scenario D in scripts/test-git-helpers.test.sh already proves the mechanism.

The shell arm now joins physical lines while the previous line ends in an unescaped backslash, so a continued git … \ / unset … \ is one unit (the same recall #2876 gave the Python arm for formatter-wrapped argv). Two correct clear idioms the gate rejected are now credited: a module-level Python constant iterated into os.environ.pop, and env -u / --unset naming every required variable.

Isolating harnesses and self-clears that already unset the discovery pair now also unset GIT_CONFIG, with plugin patch bumps only where those files live under a plugin. The grandfathered baseline is not drained (#2872) — 33 entries remain, none stale.

Fix

In scripts/check-fixture-git-isolation.sh:

• CREDIT NREQ is 3 (GIT_DIR, GIT_WORK_TREE, GIT_CONFIG) in both language arms.
• The shell arm joins physical lines while the previous line ends in an unescaped backslash, then matches unset and env -u/--unset against that logical line.
• The Python arm follows a module-level name bound to a literal of the required names into a later for _v in NAME: os.environ.pop(_v) — the loop-variable tie still applies, so a mention-only constant or a rebound constant does not credit.

Isolating helpers that already unset the discovery pair now also unset GIT_CONFIG (lib/hook-utils.test.sh, plugin test helpers, four claude-memory suites, two skill-quality suites). Plugin patch bumps: claude-memory 0.11.2, repo-hygiene 0.10.8, skill-quality 0.17.4, source-control 0.54.15, work-items 0.39.11. No grandfathered baseline entry was added or deleted.

Test plan

• bash scripts/check-fixture-git-isolation.test.sh — ALL PASS, including new cases:
  • discovery-pair clear without GIT_CONFIG is a violation (shell and Python)
  • backslash-continued git … init is still a fixture
  • backslash-continued unset of all three passes
  • env -u of all three passes; without GIT_CONFIG it does not
  • env -u wrapping one fixture command does not credit an unwrapped identity write (separate lines or one ;-joined line)
  • module-level constant iterated into pop passes; mention-only and rebound constant do not
  • live corpus still clean against the untouched baseline
• scripts/check-fixture-git-isolation.sh --check — OK (54 isolated, 33 baselined)
• bash scripts/test-git-helpers.test.sh — 9 passed, including scenario D (GIT_CONFIG)
• scripts/check-changelog-parity.sh --check / --check-order / --check-bump origin/main / --check-preserved origin/main — pass

Verification

Same evidence as the test plan, run from /tmp/wt-2889 against this branch before push. The live-corpus case inside the self-test is the canary that the 33-entry baseline is still exact (no stale line, no new violation).

Related

• Closes Widen the fixture-isolation credit signal to require GIT_CONFIG #2889 (CREDIT must name GIT_CONFIG)
• Closes Extend the fixture-isolation gate's logical-line join to the shell arm, and credit two correct clear idioms it rejects #2893 (shell logical-line join; credit env -u and a module-level constant)
• Refs Drain scripts/fixture-git-isolation-baseline.txt (36 grandfathered suites) #2872 (baseline drain is a later PR; this one does not remove entries)
• Refs test-fixture isolation: git -C does not survive an exported GIT_DIR #2840 (the gate) and fix: isolate test fixtures from the inherited git environment (#2840) #2876 (Python logical-line join)
• https://git-scm.com/docs/git-config (ENVIRONMENT: GIT_CONFIG is used as --file)

[claude]

Claude finished @kyle-sexton's task in 1m 10s —— View job

---

Security review — /review:security-review

Task: Run /review:security-review for PR #3086 (HEAD 7074abf6a816d8da83aee963948e5f9a5311ebb2) per the pinned instructions.

• Gather context (PR diff, changed files)
• Invoke /review:security-review skill and follow its instructions
• Post findings / summary

Result: skip gate — no security-relevant surface

I reviewed the full diff (git diff origin/main...HEAD, 23 files) against plugins/review/skills/security-review/SKILL.md's criteria (logic/authz/injection flaws, unsafe secret handling, dangerous GitHub Actions patterns, supply-chain risk).

This PR is entirely internal CI/dev-tooling hygiene work:

• scripts/check-fixture-git-isolation.sh (blob) — an awk-based static lint gate that scans the repo's own tracked *.sh/*.py test files for git-environment leaks. It adds GIT_CONFIG to the required-clear set and joins backslash-continued shell lines before matching. All patterns are built from static, hardcoded strings (REQ[]); no untrusted/external input is interpolated into a shell command or regex, and there's no eval, network call, or secret handling anywhere in the script.
• scripts/check-fixture-git-isolation.test.sh — test fixtures exercising the gate above; no production or trust-boundary code.
• Plugin test-helper files (lib/hook-utils.test.sh, and the various *-test-helpers.sh / *.test.sh under plugins/*) — each adds GIT_CONFIG to an existing unset ... list so fixture git repos can't leak identity writes into the real repo. These are net-positive isolation fixes to test code, not security regressions.
• plugins/*/.claude-plugin/plugin.json — patch-version bumps only.
• plugins/*/CHANGELOG.md, scripts/fixture-git-isolation-baseline.txt — documentation/changelog and a comment-line update in the baseline file.

No GitHub Actions workflow files are touched, no authN/authZ logic, no injection surface (command/SQL/path/template), and no token/secret/credential handling anywhere in the diff. Per the skill's skip gate, I'm reporting no findings and stopping here — this lane's job is logic/trust-boundary security reasoning, and there's no security-relevant surface in this change to reason about.

No security issues found.

[github-actions]

Last security-reviewed head: 7074abf6a816d8da83aee963948e5f9a5311ebb2. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7074abf6a8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 9m 15s —— View job

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md guidelines)
• Review scripts/check-fixture-git-isolation.sh changes (GIT_CONFIG requirement, shell line-joining)
• Review scripts/check-fixture-git-isolation.test.sh new test cases
• Review plugin fixture/version-bump changes
• Review baseline file change
• Post findings

This is the CI code-review lane (/review:code-review), scoped to correctness/maintainability over the files changed in this PR (git diff origin/main...HEAD).

No blocking findings. I traced the actual risk this change carries — tightening the CREDIT signal from 2 required vars (GIT_DIR, GIT_WORK_TREE) to 3 (+ GIT_CONFIG) can silently flip any suite that relied on a sourced harness for its isolation credit into a new, un-baselined violation the moment that harness stops satisfying the stricter check. I verified this directly rather than trusting the PR's own test-plan output:

• Confirmed lib/hook-utils.sh:405 (and 16 plugin-local byte-identical copies, e.g. plugins/guardrails/hooks/hook-utils.sh#L405) still unset GIT_DIR GIT_WORK_TREE GIT_COMMON_DIR GIT_CEILING_DIRECTORIES GIT_DISCOVERY_ACROSS_FILESYSTEM — no GIT_CONFIG — so under the new 3-var rule these files no longer qualify as an "isolating harness" for sources_isolating_harness().
• Grepped every .test.sh/test_*.py fixture-building suite that sources one of these hook-utils.sh copies. Only two do: lib/hook-utils.test.sh and plugins/claude-ops/hooks/claude-ops-paths.test.sh (plus plugins/guardrails/hooks/require-jq-notice-isolation.test.sh, which also sources it). lib/hook-utils.test.sh is fixed directly in this PR by adding its own unset GIT_DIR GIT_WORK_TREE GIT_CONFIG (lib/hook-utils.test.sh:10-11), and the other two also source a plugin *-test-helpers.sh that already independently carries GIT_CONFIG (confirmed in guardrails-test-helpers.sh:18 and claude-ops-test-helpers.sh:18, both pre-existing/untouched by this PR). So no suite in the live corpus actually depends solely on the stale hook-utils.sh copies for credit — no regression.
• Cross-checked all four standalone Python fixture suites in the corpus (scripts/test_check_contract_clause_coverage.py, test_prune_babysit_worktrees.py, test_hygiene.py, test_audit_skill_visibility.py) — all already name GIT_CONFIG in their pop tuples pre-existing this PR.
• Manually simulated the new awk logic (is_cont, unset_clears, env_clears, sh_logical, the Python module-level CONST/rebind tie) against the new test cases in scripts/check-fixture-git-isolation.test.sh line by line; the anchoring (^[ \t]*unset[ \t]) correctly prevents a spurious backslash-join from crediting a clear that isn't actually a leading unset/env statement, and the rebound-constant clear-before-set ordering is correct.
• Confirmed the CHANGELOG/plugin.json version bumps (claude-memory 0.11.1→0.11.2, repo-hygiene 0.10.7→0.10.8, skill-quality 0.17.3→0.17.4, source-control 0.19.x, work-items 0.39.9→0.39.10) line up with the actual harness edits under each plugin.

Not independently verified (this lane's sandbox doesn't allow executing arbitrary scripts, only a fixed allowlist of read-only commands): I could not run bash scripts/check-fixture-git-isolation.test.sh, scripts/check-fixture-git-isolation.sh --check, or scripts/check-changelog-parity.sh. The PR body's stated results for those (all-pass, OK (54 isolated, 33 baselined)) are author-claimed, not something I confirmed by execution — my confidence above comes from static tracing of the corpus, not from running the gate.

One non-blocking observation, not raised as a finding since it isn't a live regression and isn't touched by this diff: lib/hook-utils.sh's hook::in_git_working_tree() unset (and its 16 plugin copies) still omits GIT_CONFIG. That's fine for its own purpose (a read-only git rev-parse, which GIT_CONFIG doesn't affect), but it means those files no longer register as an "isolating harness" for the gate's own heuristic — worth a follow-up if a future suite ever sources hook-utils.sh alone (no *-test-helpers.sh) for its isolation credit.
· branch

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 6m 6s —— View job

---

/review:code-review — PR #3086

• Gather context (PR diff, changed files)
• Invoke /review:code-review skill and follow its instructions
• Post findings as inline comments / summary

Summary

Reviewed the full diff (gh pr diff 3086, 23 files, HEAD 89056d1) against plugins/review/skills/code-review/SKILL.md's criteria, scoped to correctness/maintainability (security is this repo's dedicated /review:security-review lane, which already ran and skip-gated on this PR as internal CI-tooling with no security-relevant surface).

Both chatgpt-codex-connector's prior findings (per-command env -u promoted to file-wide credit; a rebound Python constant keeping a stale binding) were addressed in the follow-up commit 89056d1 ("do not file-credit a per-command env -u or a rebound constant"), with matching new regression cases added to scripts/check-fixture-git-isolation.test.sh. Verified by reading the diff of that commit directly — the SH_ENV_GAP gate and the CONST any-assignment-clears-first logic both do what the commit message claims for the cases the existing tests exercise.

One finding survived, posted inline (scripts/check-fixture-git-isolation.sh:330-340):

The SH_ENV_GAP fix for the per-command env -u credit is incomplete — it only catches an unwrapped fixture command on a separate physical line from its env-wrapped sibling (which is all the new env-mixed.test.sh case covers). When two fixture commands — one env-wrapped, one bare — share a single ;/&&-joined logical line, env_clears(s) still sees all three required names anywhere in that joined string and wrongly credits the whole line, so a bare identity write right next to a wrapped init on one line still gets a clean "isolated" verdict. Concrete repro and a suggested fix direction are in the inline comment.

No other correctness or maintainability issues found in the rest of the diff (the GIT_CONFIG-credit widening, the shared harness/test-helper updates across claude-memory/repo-hygiene/skill-quality/source-control/work-items, the changelog/version bumps, and the backslash-continuation join logic all look correct and are covered by the accompanying test suite).

[claude]

Claude finished @cursor[bot]'s task in 2m 20s —— View job

---

/review:code-review — PR #3086

• Gather context (PR diff, changed files, comments, prior reviews)
• Invoke /review:code-review skill and follow its instructions
• Post findings as inline comments / summary

Summary

This is review pass #3 on this PR (claude-review lane), scoped per plugins/review/skills/code-review/SKILL.md to correctness/maintainability. Since the last code-review pass (head 89056d1), the only new work-specific commit is a5ce90a ("fix: score env -u isolation per shell statement") — everything else that changed the diff stat (b21705f) is origin/main's tip merged in via d3a545b, not new authored work on this branch.

a5ce90a directly fixes the P1 I flagged in my previous pass (env-wrap credit collapsing when two fixture commands share one ;/&&/||-joined logical line). I re-verified the fix rather than trusting the PR's own claim:

• Static trace of the new logic (scripts/check-fixture-git-isolation.sh#L327-L351): sh_logical now splits the logical line into statements on /[ \t]*(\|\||&&|;)[ \t]*/ and calls is_fixture/unset_clears/env_clears per segment instead of once over the whole joined string. I hand-traced the new env-semi.test.sh regression case (env -u GIT_DIR -u GIT_WORK_TREE -u GIT_CONFIG git -C "$d" init -q; git -C "$d" config user.email test@example.com) through the awk logic step by step: segment 1 (init, env-wrapped) sets SH_ENV=1; segment 2 (bare config user.email) is a fixture with no wrap, so SH_ENV_GAP=1; sh_flush()'s gate SH_UNSET || (SH_ENV && !SH_ENV_GAP) correctly evaluates false → no CLEARS emitted → violation correctly reported. This matches the exact repro from my prior finding.
• Regression checked against pre-existing tests that use &&/; with git fixture commands — e.g. mixed-ident.test.sh (scripts/check-fixture-git-isolation.test.sh:334, git -C "$d" config user.email ... && git config --get user.name) — the per-segment split still correctly isolates the write-bearing segment; no regression from switching to per-statement scoring.
• unset semantics are unaffected: unset_clears was already self-truncating to one statement via its own sub(/;.*/, "", u), and being process-wide, crediting from any one segment in the loop still correctly marks SH_UNSET=1 for the whole file — consistent with the existing "one unset isolates every later command" rule.
• The new test case (scripts/check-fixture-git-isolation.test.sh:787-804, "env -u and an unwrapped write on ONE ; line is still a gap") pins exactly the scenario my prior finding described and is well-targeted, not just a copy of the existing two-physical-line env-mixed case.

Not independently executed: this lane's sandbox does not have Bash approval for running bash scripts/check-fixture-git-isolation.test.sh (only gh pr diff/view/comment/review are pre-approved) — the PR's and my own confidence here rest on static/manual trace of the awk logic against the new test's literal input, not on an observed test run. Flagging this so a human can confirm the actual ALL PASS output if that matters for merge confidence.

No new blocking findings. The remaining scope of the PR (GIT_CONFIG credit widening, Python module-level constant/rebind handling, harness unset updates across claude-memory/repo-hygiene/skill-quality/source-control/work-items, changelog/version bumps) is unchanged since my prior pass at 89056d1 and was already reviewed clean there.
· branch