test(scripts): scope portability-suite fixture git commands with -C - #2839
Merged
Conversation
The two portability suites build their diff-mode fixtures by chaining `cd "$fx" && git init -q && git config user.email test@example.com && ...` inside a command-substitution subshell. That chain IS correctly cwd-scoped and does not leak (evidence in the PR body), but it leaves lines reading literally `git config user.email test@example.com` at statement position in a test file -- one copy-paste away from writing the test identity into a real repository's .git/config, which is how a contributor's commit was re-authored and PR #2827 had to be abandoned. Adopt the `git -C "$fixture" ...` idiom the sibling fixtures in this same directory already establish (check-stale-base-overlap.test.sh:22, sync-standards-contract.test.sh:69), so every fixture-writing command names its target repository instead of implying it from an enclosing cd. The `cd "$fx"` stays: the gate under test is invoked as `bash scripts/check-*.sh` and needs the fixture as its working directory. Fixture isolation only -- no assertion or coverage change. 21 lines changed, all of them git invocations. check-shell-portability.test.sh PASS=333 FAIL=0; check-skill-portability.test.sh PASS=89 FAIL=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018S8a1S71VxhLTRWBtMuEvp
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 25s —— View job Claude code review
Scope: FindingsNone blocking. This is a mechanical, low-risk change — every hunk adds I traced each of the three converted fixture blocks (verified against the diff and surrounding context):
The PR body itself is unusually thorough about scope: it's honest that this isn't a demonstrated fix for the originally reported leak (the Verification note: I was not able to execute |
|
Claude finished @kyle-sexton's task in 39s —— View job Security reviewSkip gate: no security-relevant surface. Both changed files ( No GitHub Actions workflow files, permissions, secrets handling, or dependency pins are touched. The PR's own body is candid that this is an idiom-consistency change (matching the No security issues found.
|
|
Last security-reviewed head: |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
This was referenced Aug 16, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 16, 2026
## Summary
`ci-status` is the single check the org `ci-gate` ruleset keys on, and
its own comment calls its
`needs` list "the single source of truth for the lane list". Nothing
enforced the other direction. A
job defined in `.github/workflows/ci.yml` but absent from that list
still runs, still reports, and
still turns red in the run list — while `ci-status` reports success and
the merge proceeds. The
`hook-utils-windows` comment already states the doctrine in prose ("a
lane missing from that list is
informational no matter how loudly a comment here calls it a gate");
prose is not a gate.
The gap was computed, not read. Against `origin/main` at `2d20a277`
(i.e. after #2841 landed and
added two lanes of its own):
```console
$ bash derive-gap.sh
defined jobs (incl. ci-status): 37
ci-status.needs entries: 34
--- defined but NOT in ci-status.needs (excluding ci-status itself) ---
managed-scope-sync
state-key-sync
--- in ci-status.needs but NOT defined ---
```
(The issue's "38 defined / 34 named" was an eyeball count on a pre-#2841
tree; the derived numbers
differ, the two missing lanes do not.)
## Fix
**Both missing lanes gate — neither omission was deliberate.**
| Job | Decision | Why |
| --- | --- | --- |
| `managed-scope-sync` | **Add to `needs`** | Structurally identical to
the four sync lanes already in `needs` (`hook-utils-sync`,
`parse-concern-value-sync`, `resolve-convention-pattern-sync`,
`standards-contract-sync`): same `--check` / lib self-test /
`--check-bump` triple. Not advisory, not schedule- or event-scoped — the
`if: github.event_name == 'pull_request'` sits on the bump **step**,
never on the job, so the job runs and reports on every PR and every
push. `scripts/cross-plugin-source-registry.txt` already advertises it
as the dedicated check for `lib/managed-scope.sh`. |
| `state-key-sync` | **Add to `needs`** | Same shape, same registry
claim for `lib/state-key.sh`. |
No job in this workflow is intentionally omitted, so this PR ships
**zero** opt-out annotations.
`cross-plugin-source-drift` (already in `needs`) covers a *drifted copy*
of either cluster, so this
was never a total hole — but it does not cover the `--check-bump` half
(lib changed, carrying plugin
version not bumped), which only the dedicated lanes run.
**The guard.** `scripts/check-lane-coverage.sh --check` proves the
defined-job set and
`ci-status.needs` are equal in both directions, and is itself wired into
`ci-status.needs` as
`lane-coverage-gate` (self-covering: its own absence from `needs` would
be caught by itself). It
reports four classes:
- `UNGATED LANE` — defined, not in `needs`, not annotated. The class
#2856 filed.
- `DANGLING NEED` — a `needs` entry naming no defined job.
- `STALE OPT-OUT` — a job annotated as deliberately ungated that *is* in
`needs`.
- `BARE OPT-OUT` — `# lane-coverage-ok:` with no reason after the colon.
**Required, not advisory** — deliberately. An advisory lane-coverage
check would be a green-and-silent
surface whose entire purpose is detecting green-and-silent surfaces,
which
`docs/conventions/liveness-assertion/` names as non-conforming. The
false-positive risk that normally
argues for advisory is absent here: the gate reads one file, takes no
diff, no base ref and no
network, and every YAML shape it does not model (flow-sequence `needs:
[a, b]`, scalar `needs:`, an
aggregate with no `needs:`, an unmodelled 2-space key under `jobs:`)
exits **2 (inconclusive)** rather
than 0 — reporting coverage from a file it did not parse would be the
gate committing the very defect
it detects.
**The opt-out** is the same annotated-exemption shape `#
silent-skip-ok:` uses for
`scripts/check-silent-skips.sh` — a `# lane-coverage-ok: <reason>`
comment in the contiguous 2-space
block immediately above the job key, or trailing the key itself. The
reason lives next to the thing it
excuses, in the file a reviewer is already reading, with no separate
list to drift. It carries a stale
guard (the annotation cannot outlive what it excuses) and a bare
annotation **fails** rather than
passing as "annotated", so it can never become a silent off switch.
No job's logic, `runs-on`, triggers, or `if:` conditions changed. The
`ci.yml` diff is
**31 insertions, 0 deletions**: three `needs:` entries and one new job
block.
## Verification
**The guard fires on the real defect and stops firing once fixed** —
constructed and run, not reasoned
about. On the unfixed `ci.yml` (pre-edit, at `origin/main`):
```console
$ bash scripts/check-lane-coverage.sh --check ; echo "EXIT=$?"
UNGATED LANE: job 'managed-scope-sync' is defined in .github/workflows/ci.yml but absent from ci-status.needs, so it cannot gate a merge. ...
UNGATED LANE: job 'state-key-sync' is defined in .github/workflows/ci.yml but absent from ci-status.needs, so it cannot gate a merge. ...
check-lane-coverage: 2 coverage defect(s) in .github/workflows/ci.yml
EXIT=1
```
After the wiring, the same derivation returns an empty gap and the gate
is green:
```console
$ bash derive-gap.sh
defined jobs (incl. ci-status): 38
ci-status.needs entries: 37
--- defined but NOT in ci-status.needs (excluding ci-status itself) ---
--- in ci-status.needs but NOT defined ---
$ bash scripts/check-lane-coverage.sh --check
check-lane-coverage: .github/workflows/ci.yml — all 37 lane(s) reachable from ci-status.needs
```
## Test plan
`scripts/check-lane-coverage.test.sh` — 20 cases, synthetic workflow
fixtures built per case, all
`ALL PASS`:
- a job absent from `needs` fails **1** and names the job; every job
present passes **0**
- annotated opt-out passes **0**; trailing-comment form passes **0**
- opt-out with no reason fails **1** (`BARE OPT-OUT`)
- opt-out on a job that *is* in `needs` fails **1** (`STALE OPT-OUT`)
- an annotation separated from its key by a blank line does **not**
exempt the job
- `needs` entry naming no defined job fails **1** (`DANGLING NEED`)
- flow-sequence `needs`, scalar `needs`, absent `needs`, empty `needs`,
unmodelled 2-space key,
unknown aggregate id, missing file, bad usage, no `jobs:` mapping — all
exit **2**, never 0
- the repository's own `ci.yml` exits **0**
Fixtures are plain files under `mktemp` addressed by absolute path — no
scratch git repo, so the
`git config user.email` caller-config-clobber class (#2839) cannot recur
here.
Also run clean locally: `shellcheck` and `actionlint` on the touched
files;
`check-shell-portability.sh --paths` (both new scripts);
`check-silent-skips.sh`;
`check-discriminating-test-skips.sh`; `check-orphaned-fixtures.sh`;
`check-changelog-parity.sh
--check` and `--check-bump origin/main` (no plugin touched, so no
version bump is owed);
`check-stale-base-overlap.sh --check origin/main`; `affected-tests.sh`
(both new files map to a suite
— no unmapped-file error); `affected-tests.test.sh` (32/32) and
`check-docs-only.test.sh` (21/21),
the two suites the selector picked for the `ci.yml` change.
An independent fresh-context verifier re-derived both sets with its own
script, rebuilt the
fired/not-fired evidence from scratch, and diff-checked that no job's
logic, triggers, or `if:`
conditions changed.
## Related
- Fixes #2856
- Refs #2834, #2841 — #2841 is where "a new job is automatically
required" was tested and found
false; it added `windows-path-emit-gate` and `windows-path-emit-windows`
and wired both by hand.
This PR makes the hand-wiring mechanical. Branched off `origin/main`
after #2841 merged.
- Refs #532 — `docs/conventions/liveness-assertion/`, the false-green
class this defect belongs to.
- Does not touch `scripts/check-silent-revert.sh` or
`scripts/silent-revert-incidents.txt` (#2843).
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
12 tasks
kyle-sexton
added a commit
that referenced
this pull request
Aug 17, 2026
…#2876) ## Summary A test suite that builds a git fixture can write that fixture's identity into the **caller's** repository. This makes that impossible in the shared harnesses, fixes the Python suite that actually caused it, and adds a CI gate so a new suite cannot reintroduce it. **A correction to the framing in #2840 first, because the fix depends on it.** The issue says "`GIT_DIR` is exactly what git exports into every hook it invokes", implying hooks are the footgun. Measured, that is not the mechanism: - `git -C <dir>` **does** chdir. What an exported **absolute** `GIT_DIR` overrides is repository **discovery**, and `git config` writes its default `--local` scope to whatever `--git-dir` finally resolves to. So under an absolute `GIT_DIR`, `git -C <fixture> config user.email X` writes the **caller's** config and leaves the fixture with **no `.git` at all**. - The **relative** form (`GIT_DIR=.git`) is safe, because `-C` chdirs first and `.git` then resolves against the new cwd. So the footgun is the **absolute** form, not the mere presence of `GIT_DIR`. Confirmed on this machine: no git hook exists at any scope, `core.hooksPath` is unset everywhere, and the sole Claude Code PreToolUse hook invokes no git — the exported `GIT_DIR` in the real incident came from an ad-hoc command, not a hook. The general shape is worth stating, because it is what justifies clearing the environment rather than patching call sites: **an ambient environment variable silently redirected a git operation that looked correct at the call site.** The same class bit this repo a second time this week through `MSYS_NO_PATHCONV`, where an exported value leaked across a compound command and made `git worktree add` create a directory off the wrong drive. Test fixtures must not inherit ambient git-relevant environment at all. Two adjacent leak paths, both live here, that the issue does not mention and that rule out the narrower `-c` transient fix: - `git -C <linked worktree> config user.email` writes the **main** repo's **shared** `.git/config` — git creates no `config.worktree`. This repo runs dozens of linked worktrees. - `git -C <non-repo dir nested inside a repo> config` walks **upward** and writes the enclosing repo's config. `git -C <dir> -c user.email=... <cmd>` prevents config poisoning but still operates on the wrong repository, and does nothing for either path above. Clearing the environment is the fix that covers all three. Why this matters operationally: a poisoned `user.email` silently re-authors commits. Such a commit fails this repo's `required_signatures` rule with `no_user` and **cannot be force-pushed over** — the branch has to be abandoned and rebuilt. That is #2827 -> #2830. Closes #2840 ## Fix **1. `scripts/test-git-helpers.sh`** — clears `GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_COMMON_DIR GIT_PREFIX GIT_OBJECT_DIRECTORY` once at **source time**, so every current and future consumer is immunized without a per-call-site edit. The same clear is added to `scripts/run-plugin-tests.sh` and the two plugin test harnesses (`claude-ops`, `guardrails`). The guard lives **inside** the harnesses and test files, not as a CI-side `env -u` wrapper. The poisoning incident was an ad-hoc **local** run, and no single test runner exists (CI names each suite individually). A wrapper would not have prevented the actual incident. **2. Python** — `plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py` is the file that fired. Its `create_checkout` runs `git -C <checkout> config user.email test@example.com`. One module-level `os.environ.pop` loop at import, not 46 call-site edits: fixtures are built from two `TestCase` classes with no shared `setUp`, none of the git subprocess calls pass an explicit `env=`, and spawned subprocesses inherit `os.environ`, so one clear covers every path. Because it runs at **import**, it takes effect under CI's `GuardTests`-only invocation too. The same guard is added to the only two other Python suites in the repo that build git fixtures: `test_prune_babysit_worktrees.py`, which also builds a **linked worktree**, and `test_check_contract_clause_coverage.py`, which runs `git init` with `cwd=` and no path argument at all — under a leaked `GIT_DIR` that initializes the caller's gitdir and stages into the caller's index. **3. `scripts/check-fixture-git-isolation.sh`** — a new gate, wired into `ci.yml` as `fixture-git-isolation-gate` and registered in `ci-status.needs`. **Why a standalone gate and not a class inside `scripts/check-shell-portability.sh`** (which was the original plan): that gate is scoped to `*.sh`. The file that caused the incident is a `test_*.py`. **A gate that structurally cannot see the file that fired is not a gate.** The standalone `scripts/check-*.sh` + `scripts/*-baseline.txt` pair is also this repo's established shape for exactly this kind of check (`check-orphaned-fixtures.sh`, `check-changelog-parity.sh`), so this conforms to the existing pattern rather than adding a second way. A useful side effect: `scripts/check-shell-portability.sh` is **untouched**, so this PR does not contend with the other lane working there. Gate details: - Covers `*.test.sh`, `test_*.py`, `*_test.py`. The Python clearing check is **file-scoped**, not line-scoped, because the idiom spans lines. - Detection intents cover the identity-**write** spellings a `config`-adjacent pattern missed (`git config --local user.email`, `git -c <k>=<v> init`) while **excluding** the `--get` / `--list` **read** spellings, which cannot poison anything. - A suite whose subject IS this mechanism opts out with a line-anchored `fixture-isolation-scope:` declaration, mirroring the existing `portability-scope:` precedent. Anchoring at the start of the comment content is deliberate: prose mentioning the token must not silently exempt a file. This is distinct from the baseline — a declaration records a permanent property, the baseline records drainable debt. ## Verification **Discrimination proof — the regression test fails pre-fix and passes post-fix.** `scripts/test-git-helpers.test.sh` covers all three leak paths. It honors `TEST_GIT_HELPERS_UNDER_TEST` so the pre-fix run needs no tracked-file edit. Every assertion reads the caller identity with `config --local --get`; a plain `--get` falls through to `~/.gitconfig` at rc=0 and would mask the leak. Pre-fix (the `origin/main` harness): ``` harness under test: .../prefix/test-git-helpers.sh FAIL: A absolute GIT_DIR: caller identity POISONED (before=sentinel@example.invalid after=t@t.test) FAIL: A absolute GIT_DIR: fixture has NO .git — the work went to the caller instead FAIL: B linked worktree: caller identity POISONED (before=sentinel@example.invalid after=t@t.test) FAIL: B linked worktree: fixture has NO .git — the work went to the caller instead FAIL: C nested non-repo dir: caller identity POISONED (before=sentinel@example.invalid after=nested@example.invalid) FAIL: C nested non-repo dir: fixture has NO .git — the work went to the caller instead FAIL: harness does not clear: GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_COMMON_DIR GIT_OBJECT_DIRECTORY passed: 0 failed: 7 ``` Post-fix (the shipped harness): ``` harness under test: .../scripts/test-git-helpers.sh ok: A absolute GIT_DIR: caller identity intact (sentinel@example.invalid) ok: A absolute GIT_DIR: fixture owns its own .git ok: B linked worktree: caller identity intact (sentinel@example.invalid) ok: B linked worktree: fixture owns its own .git ok: C nested non-repo dir: caller identity intact (sentinel@example.invalid) ok: C nested non-repo dir: fixture owns its own .git ok: harness clears every discovery-redirecting variable passed: 7 failed: 0 ``` Scenario B independently confirms the shared-config path: with `GIT_DIR` set to a **linked worktree's** gitdir, the write landed in the **main** clone's config and `extensions.worktreeConfig` was never created. **Gate self-test — 23 cases, all pass**, including a live-corpus canary: ``` ok - unknown flag exits 2 with usage ok - unisolated fixture suite is a violation (exit 1, names the file) ok - suite that unsets GIT_DIR/GIT_WORK_TREE passes ok - unsetting GIT_DIR alone is still a violation ok - sourcing a harness that clears the environment passes ok - a harness that stops clearing re-exposes its sourcing suites ok - a baselined violation is grandfathered ok - a stale baseline entry fails (a line cannot outlive its debt) ok - a suite that builds no fixture is not conscripted ok - a comment mentioning git init does not conscript a suite ok - --list reports verdicts and exits 0 ok - an identity write via 'config --local' is a violation ok - an identity read alone does not conscript a suite ok - a read sharing a line does not suppress an identity write ok - 'config --local --get user.email' is a read, not a fixture write ok - 'git -c <k>=<v> init' is a violation ok - python: unisolated fixture suite is a violation ok - python: popping GIT_DIR and GIT_WORK_TREE passes ok - python: popping GIT_DIR alone is still a violation ok - python: naming the variables without popping is still a violation ok - a declared fixture-isolation scope exempts the suite ok - a mid-line mention of the scope token does not exempt ok - live corpus passes against scripts/fixture-git-isolation-baseline.txt ALL PASS ``` **Gate against the real tree:** ``` $ scripts/check-fixture-git-isolation.sh --check fixture git isolation: OK (45 isolated, 36 baselined) # rc=0 $ scripts/check-fixture-git-isolation.sh --list | tail -1 45 isolated, 36 baselined, 0 violating ``` **False-positive rate, measured and disclosed.** The shipped detector classifies **81 files as fixture-builders (78 shell + 3 Python)**, re-derived from the shipped code rather than quoted from an earlier revision. An independent audit of the flagged set found **0 read-only false positives**, **0** files selected only by a string literal, and **0** selected only by the broad `worktree add` intent — the added intents conscript no file on their own. (That audit ran against an 80-file snapshot taken one commit earlier; the single added file is this gate's own self-test, which gained fixture text when the cases below were written. An earlier pass reporting 137 identity triggers across 75 shell suites predates the broadened detection entirely and is superseded.) Three known limits are recorded rather than hidden: - A harness sourced through a **variable** (`. "$HELPER"`) is invisible to the harness-resolution branch, which needs a literal `.sh` token. Only `scripts/test-git-helpers.test.sh` does this, and it carries an explicit scope declaration instead. - A file holding a `fixture-isolation-scope:` line as **test data** would be exempted by it. Verdict order mitigates this — actually clearing outranks declaring — so the only file reported `declared` is the one counter-fixture that genuinely cannot clear. - `--list` exits 0 unconditionally by design; `--check` is the failing mode. `--list` also prints the baselined count net of stale entries, so while draining the baseline the authoritative count is that file's own line count. Two false positives were found and closed during development rather than shipped: - `audit-fleet.test.sh`, conscripted by `config --get-all user.name` — a read. That is why the read spellings are excluded. - A line-wide read exclusion that would have let one `--get` suppress a real identity write sharing the line. Reads are now removed extent by extent. Measured before changing anything: four lines in the tracked corpus carry both shapes and all four are pure reads, so this closed a latent hole rather than a live one. Both directions carry test cases. ## Test plan All run locally on Windows / Git Bash; the output above is real, not paraphrased. - [x] `bash scripts/test-git-helpers.test.sh` — 7 pass / 0 fail (rc=0) - [x] same with `TEST_GIT_HELPERS_UNDER_TEST` = the `origin/main` harness — 0 pass / 7 fail (rc=1), proving discrimination - [x] `bash scripts/check-fixture-git-isolation.test.sh` — 21 cases, ALL PASS (rc=0) - [x] `scripts/check-fixture-git-isolation.sh --check` — rc=0 - [x] `python -m unittest test_hygiene.GuardTests` (the class CI runs) — `Ran 127 tests ... OK (skipped=3)` - [x] `python -m unittest test_hygiene` (whole module) — `Ran 303 tests`, 2 failures, both reproducing **identically on unmodified `origin/main`**, so pre-existing and not introduced here (tracked in #2871) - [x] `python -m unittest discover -s tests -p 'test_*.py'` (babysit-prs) — `Ran 643 tests ... OK` - [x] `python scripts/test_check_contract_clause_coverage.py` — `Ran 24 tests ... OK` - [x] `shellcheck --rcfile=.shellcheckrc -x` on every changed `.sh` — rc=0 each - [x] `shfmt -d -i 2 -ci` on every changed `.sh` — clean - [x] `ruff check` on all three changed `.py` — `All checks passed!` - [x] exec bits: both new `.sh` recorded `100755` in the index, the baseline `.txt` `100644` Not verified locally: GitHub-side execution of the new `ci.yml` job — that is what this PR's own CI run establishes. ## Related - #2840 — the issue this closes. Its "hooks export `GIT_DIR`" framing is corrected above; the absolute-vs-relative distinction is the actual mechanism. - #2827 / #2830 — the abandoned-and-rebuilt PR pair this leak produced. - #2839 — adopted the `git -C` idiom and deliberately deferred this hardening. - #2871 — **filed by this lane**: nine of ten `TestCase` classes in `test_hygiene.py` never run in CI, including the one holding the function that caused the incident. Measured; flipping to the whole module would land red, so it needs the staged baseline path. Deliberately **not** changed here — this PR is a fixture-isolation fix, not a CI-topology change. - #2872 — **filed by this lane**: drain the 36 grandfathered entries in `scripts/fixture-git-isolation-baseline.txt`. Staged behind the gate so it lands green; a gate that lands red is a gate on its way to being disabled. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 17, 2026
#2916) No linked issue ## Summary Structure-only apply-lane batch from the first `/coupling:reduce` dogfood pass over `scripts/`: five fixture-construction sites across three test suites hand-rolled the `git init` + identity-config block that `scripts/test-git-helpers.sh` already publishes, bypassing its inside-checkout safety guard (the class that bit as #2839) and its gpgsign/autocrlf hardening. ## Fix Source `test-git-helpers.sh` in `check-shell-portability.test.sh`, `check-skill-portability.test.sh`, and `sync-standards-contract.test.sh`, and replace each inline init+config block with `git_init_test_repo`. Net −6 lines. `check-stale-base-overlap.test.sh` is deliberately not converted — its fixture needs `init -b`, which the helper has no seam for yet (tracked in #2914's deferred item). ## Verification - `check-shell-portability.test.sh`: PASS=333 FAIL=0 - `check-skill-portability.test.sh`: PASS=89 FAIL=0 - `sync-standards-contract.test.sh`: PASS=12 FAIL=0 - `shellcheck` clean on all three files ## Related Refs #2914 (route-lane findings from the same pass); companion to #2913 (the skill that produced this batch) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016CWMh6HAsgWWi9tLw76hZR --- _Generated by [Claude Code](https://claude.ai/code/session_016CWMh6HAsgWWi9tLw76hZR)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
What
The two portability suites build their diff-mode fixtures like this:
This PR names the fixture on every fixture-writing git command, adopting the
git -C "$fixture" ...idiom the sibling fixtures in the same directory alreadyestablish (
scripts/check-stale-base-overlap.test.sh:22,scripts/sync-standards-contract.test.sh:69).21 lines changed, all of them git invocations. No assertion or coverage change.
Honest finding: the named sites were already cwd-scoped
The motivating report described these as unscoped commands writing into the
caller's
.git/config. They are not, under normal invocation. Thecd "$fx" &&guard at the head of the
&&chain scopes them, and I could not reproduce a leak:Whole-suite before/after, against the shared
.git/configthis worktree and themain clone both use:
user.*beforeuser.*afterHEAD~1copies)HEAD~1copies)So this is not a demonstrated bug fix. It is an idiom-consistency and
copy-paste-hazard change: a line reading literally
git config user.email test@example.comat statement position in a test file is one paste away frompoisoning a real repository, which is the plausible route to the #2827 damage.
(The branch name says
fix/; the change is hygiene. The landed squash committakes this PR's title and body, so history records it accurately.)
Note the
cd "$fx"is retained deliberately and stays load-bearing — therelative
printf … > 'plugins/…'writes and the relativebash scripts/check-*-portability.shinvocation all need cwd =$fx. So-Chere is a second, explicit scoping mechanism alongside the existing implicit
one, not a replacement for it.
$fxis alwaysmktemp -doutput, i.e.absolute, so
-C "$fx"resolves correctly from any cwd.Follow-up finding:
-Cis not a fixture-isolation guaranteeThe one way I could manufacture the leak is an exported
GIT_DIR— theenvironment git hands to every hook it invokes.
git config's default--localscope follows
GIT_DIR, not the working directory:git -C "$dir"does not fix that — I verified the prescribed idiom isequally vulnerable (the fixture ends up with no
.gitat all and the caller'sconfig is poisoned):
Only
unset GIT_DIR GIT_WORK_TREE(orenv -u) actually isolates a fixture.I did not build that here — it is a new shape across ~30 test files and is
not justified by any demonstrated need in this repo (no
core.hooksPath, noinstalled hooks, and CI invokes the suites as plain steps). Filing it as a
finding instead: the established
git -Cfixture idiom is a readability andcopy-paste guard, not an isolation guarantee.
Sweep
Three passes, all for repo-writing git verbs
(
config|init|add|commit|checkout|switch|branch|remote|tag|reset|rm|mv|stash|update-ref|worktree|clone|push|fetch)at shell-statement position without
-C:*.test.sh/tests//run-tests.shfiles — 285 candidate hits.*.ps1 *.psm1 *.py *.js *.ts— every hit is a comment, docstring, orstring literal; no executable git write command in any of them.
*.shfiles that buildmktemp -dfixtures but are not named liketests (so pass 1 would have missed them) — 2 hits, both
git init -q -b main "$repo"inplugins/source-control/skills/worktree/fixtures/.Every site is scoped, by one of:
-C <dir>— e.g.sync-standards-contract.test.sh:69.git init -q "$REPO",git clone -q <src> <dest>(the repo-hygiene, source-control, docs-hygiene and preflight fixtures).
cdinto the fixture,(cd "$d" || exit 1; …)or(cd "$d" && …),which also covers the
git init -q .sites (exec-bit-check.test.sh:25,block-noncanonical-commit.test.sh:358) — those are cwd-scoped by theenclosing
cd, not by their.argument.git config -f <file>(preflight.test.sh:416,417,495).Everything else that matched is a quoted test payload — permission-rule strings
like
Bash(git push), PowerShell here-string bodies,make_skillheredoccontent — never executed. No remaining unscoped site.
One structural note, reported not touched (the file is owned by a concurrent
lane):
scripts/check-stale-base-overlap.test.shscopes its baregit checkout/git add/git commitwith a top-levelcd "$repo" || exit 1rather than a subshell-local one. It is guarded, so it is scoped — but it is more
fragile than the sibling pattern, since the cwd persists into everything after it.
Verification
bash scripts/check-shell-portability.test.sh-> PASS=333 FAIL=0bash scripts/check-skill-portability.test.sh-> PASS=89 FAIL=0(it is scoped to
plugins/<name>/; there is no root CHANGELOG.md).changelog-parity-gatepasses on this PR, confirming it.Pass counts are identical pre- and post-change (333 and 89), which is the
coverage-unchanged proof: the diff touches 21 lines, all of them git
invocations, and no
ok "/fail "assertion line.Related
No linked issue. This is fixture-isolation hygiene in two test suites; it closes
no GitHub issue.
git -Cdoes not survive an exported GIT_DIR #2840 — the follow-up finding this PR surfaced but deliberately does not fix:git -Cdoes not survive an exportedGIT_DIR, so the established idiom is acopy-paste guard rather than an isolation guarantee.
commit re-authored to
test@example.comfailedrequired_signatureswithno_userand could not be force-pushed over.scripts/check-stale-base-overlap.test.shandscripts/sync-standards-contract.test.sh— the sibling fixtures whose idiomthis PR adopts. Neither file is modified here (both are owned by concurrent
lanes); they are read-only prior art.
🤖 Generated with Claude Code
https://claude.ai/code/session_018S8a1S71VxhLTRWBtMuEvp
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com