refactor(scripts): route hand-rolled git init through test-git-helpers

[kyle-sexton]

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.ai/code/session_016CWMh6HAsgWWi9tLw76hZR

---

Generated by Claude Code

[cursor]

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]

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

---

Claude finished the code review

• Gather context (PR diff, changed files)
• Run /review:code-review procedure
• Post findings (inline + summary)

Summary

Reviewed the diff (gh pr diff 2916) against scripts/test-git-helpers.sh. This is a clean, mechanical refactor: three test suites (check-shell-portability.test.sh, check-skill-portability.test.sh, sync-standards-contract.test.sh) now source test-git-helpers.sh and call git_init_test_repo instead of hand-rolling git init -q + git config user.email/name.

Verified:

• git_init_test_repo (scripts/test-git-helpers.sh#L67-L74) is a strict superset of what each inline block did — same user.email/user.name semantics (value changed from test@example.com to t@t.test, but no test in the three files asserts on the literal email string), plus it adds the git_init_safe inside-checkout guard and commit.gpgsign=false/core.autocrlf=false hardening the PR description calls out.
• Each conversion site's surrounding code (e.g. scripts/check-shell-portability.test.sh:1543-1546, scripts/check-skill-portability.test.sh:669-672, scripts/sync-standards-contract.test.sh:78-80) already does cd "$fx"/passes an absolute fixture path before calling the helper, so git_init_safe's git rev-parse --show-toplevel guard resolves against the fixture location as intended, not the real checkout.
• The . "$SELF_DIR/test-git-helpers.sh" source line in each file is added after SELF_DIR is defined, so the relative path resolves correctly.
• check-stale-base-overlap.test.sh is correctly left unconverted — it does call git init -q -b main (scripts/check-stale-base-overlap.test.sh:21), which git_init_test_repo/git_init_safe has no -b seam for, matching the PR's stated rationale.
• This pattern (source test-git-helpers.sh, call git_init_test_repo/git_test_config) is already the established convention elsewhere in scripts/ (e.g. check-changelog-parity.test.sh, check-silent-revert.test.sh, check-docs-only.test.sh), so this PR brings the three converted files in line with existing precedent rather than introducing a new pattern.

I was not able to execute bash scripts/*.test.sh or shellcheck myself in this sandbox (Bash execution required interactive approval that wasn't available), so the PASS/FAIL counts and shellcheck-clean claim in the description are author-reported and unverified by me — my review is based on static reading of the diff and the helper implementation, which is internally consistent and behaviorally equivalent (plus safer) versus what it replaces.

No blocking findings. No inline comments posted — the change is a straightforward, low-risk, structure-only substitution with no correctness or behavior concerns.

[claude]

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

---

Security review — /review:security-review

• Gather context (diff via gh pr diff 2916, read scripts/test-git-helpers.sh)
• Apply skip gate
• Report result

Skip gate: no security-relevant surface.

All three changed files are test harnesses (*.test.sh) that only touch throwaway git fixtures created under a temp directory during test runs:

• scripts/check-shell-portability.test.sh
• scripts/check-skill-portability.test.sh
• scripts/sync-standards-contract.test.sh

The change replaces hand-rolled git init + git config user.email/user.name blocks with the shared git_init_test_repo from scripts/test-git-helpers.sh. I read the helper: it routes through git_init_safe, which refuses to initialize a repo inside the current checkout tree (the #2839 leak class), and it pins commit.gpgsign=false / core.autocrlf=false via -c flags scoped to each invocation rather than writable global/local config. No untrusted input, no secrets/credentials, no CI workflow or permissions changes, and no new injection surface is introduced — this is a pure refactor onto an existing, already-reviewed helper, and it strictly improves the isolation posture (previously ad hoc, inconsistent init blocks) rather than weakening it.

No security findings.

[github-actions]

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

[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 @kyle-sexton's task in 1m 17s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarially validate any candidates
• Post final findings

No security issues found.

This PR is a test-only refactor that replaces five hand-rolled git init + identity-config blocks (in check-shell-portability.test.sh, check-skill-portability.test.sh, and sync-standards-contract.test.sh) with calls to the shared git_init_test_repo helper in scripts/test-git-helpers.sh.

I reviewed the diff (git diff origin/main HEAD) and the helper itself:

• git_init_test_repo delegates to git_init_safe, which refuses to git init inside the current repository checkout (the guard that addressed the test(scripts): scope portability-suite fixture git commands with -C #2839/test-fixture isolation: git -C does not survive an exported GIT_DIR #2840 fixture-identity-leak class), and additionally pins commit.gpgsign=false and core.autocrlf=false — this is strictly a hardening improvement over the inline blocks it replaces (two of the three converted files previously set neither).
• Sourcing test-git-helpers.sh at the top of each suite also clears the inherited GIT_DIR/GIT_WORK_TREE/GIT_CONFIG/etc. environment for the whole suite, which is the actual isolation mechanism per the file's header — this is a net-positive for the isolation invariant, not a new surface.
• The corresponding drop of these three suites from scripts/fixture-git-isolation-baseline.txt is consistent with check-fixture-git-isolation.sh's SOURCE-detection logic (a suite that sources an isolating harness by basename is no longer counted as debt), so no gate is being weakened or bypassed.
• No new user-controlled input, injection surface, credential handling, or CI/workflow permission changes are introduced. check-stale-base-overlap.test.sh was correctly left unconverted per the PR description (its fixture needs init -b, which the helper doesn't yet support) — leaving it out is not a security gap since it wasn't converted at all.

This is a clean, low-risk test-infrastructure change that closes a security-relevant gap rather than opening one.

[claude]

Claude finished @kyle-sexton's task in 2m 30s —— View job

---

Code Review

• Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
• Review changed files for correctness/maintainability
• Post findings

No blocking findings. This is a clean, well-scoped mechanical refactor.

What I checked:

• Diffed the PR against origin/main (gh pr diff 2916) and read all four changed files in full context.
• Confirmed git_init_test_repo in scripts/test-git-helpers.sh is a behavior-preserving replacement for the inline git init -q && git config user.email ... && git config user.name ... blocks it replaces in all three converted suites — the only observable difference is the throwaway identity string (t@t.test vs test@example.com), which no assertion in any of the three suites depends on (verified via grep for both strings across scripts/).
• Verified the &&-chain semantics are preserved: git_init_test_repo "$fx" && returns the same "did the last write succeed" signal as the three chained lines it replaces, and it's invoked inside the same $( cd "$fx" && ... ) command-substitution subshell, so the sourced function is visible without needing export -f.
• Verified the fixture-git-isolation-baseline.txt removals against scripts/check-fixture-git-isolation.sh's actual mechanism: it resolves sourced harnesses by basename against the tracked tree (check-fixture-git-isolation.sh#L49-L54), so the three suites newly sourcing test-git-helpers.sh do in fact drop off the debt list — this isn't an unverified claim, the gate's own logic confirms it.
• Confirmed check-stale-base-overlap.test.sh is correctly left unconverted: it uses git init -q -b main (check-stale-base-overlap.test.sh#L21), and git_init_test_repo has no seam for a branch name, matching the PR's stated reason and the baseline file's own inline comment.

Not independently verified: this review lane's tool grants are read-only (gh pr diff/view/comment/review, Read, Glob, Grep) — I could not execute the test suites or shellcheck myself, so the PASS/FAIL counts and shellcheck-clean claim in the description are author-reported and unverified by me, not independently confirmed. Static reading of the diffs found no correctness or behavioral-drift issues.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db2b27ce62

ℹ️ 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 1m 30s —— View job

---

Security review complete

• Gather context (PR diff origin/main HEAD, changed files, custom instructions)
• Invoke /review:security-review procedure
• Post findings

No outstanding security issues.

This is a test-only refactor routing five hand-rolled git init + identity-config fixture blocks (across check-shell-portability.test.sh, check-skill-portability.test.sh, sync-standards-contract.test.sh) through the shared git_init_test_repo/git_init_safe helpers in scripts/test-git-helpers.sh.

Codex's P1 finding is already fixed on this HEAD. Codex flagged that sync-standards-contract.test.sh's git_fixture didn't stop when git_init_test_repo refused to init (e.g. TMPDIR resolving inside the checkout), letting the subsequent git -C "$fixture" add -A/commit fall through to the enclosing real repository. Commit 8e52e96 (the current PR head, pushed after that comment) added || return 1 immediately after the call:

git_fixture() {
  local fixture="$1"
  # On refusal (e.g. TMPDIR inside the checkout) stop before add/commit can
  # resolve to the enclosing real repository.
  git_init_test_repo "$fixture" || return 1
  git -C "$fixture" add -A
  git -C "$fixture" commit -qm base
  git -C "$fixture" rev-parse HEAD
}

This closes the exact failure mode Codex described — add -A/commit are now unreachable once git_init_test_repo (via git_init_safe's git rev-parse --show-toplevel guard) refuses to init inside the checkout tree.

I verified the two remaining conversion sites don't have the same gap: check-shell-portability.test.sh:1544-1550 and check-skill-portability.test.sh:670-675 already chain git_init_test_repo "$fx" && into the rest of the fixture-setup pipeline inside a $(...) subshell, so a refusal there short-circuits the whole chain before any further git command runs — no fix needed for those two.

Other checks:

• No untrusted/user-controlled input, no secrets/credential handling, no CI/workflow permission changes.
• git_test_config pins identity + commit.gpgsign=false/core.autocrlf=false via scoped -c flags rather than writable config, consistent across all three converted sites.
• check-stale-base-overlap.test.sh remains correctly unconverted (needs init -b, which the helper has no seam for yet, per the PR description).
• scripts/fixture-git-isolation-baseline.txt drops exactly the three converted suites, consistent with check-fixture-git-isolation.sh's source-detection logic — no gate weakened.

No further action needed from a security standpoint.

[claude]

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

---

Claude finished the code review

• Gather context (PR diff, changed files, test-git-helpers.sh)
• Run /review:code-review procedure
• Evaluate Codex's finding about git_init_test_repo failure propagation
• Post findings (inline + summary)

Summary

Reviewed the full diff (85265a0..HEAD, matching all 4 changed files/SHAs in the PR context) against scripts/test-git-helpers.sh.

Codex's P1 finding is already fixed on this branch. The finding was raised against commit db2b27c (git_fixture in sync-standards-contract.test.sh called git_init_test_repo without checking its return status, so a refusal — e.g. TMPDIR inside the checkout — would fall through to git -C "$fixture" add -A/commit, which could stage/commit against the real repo). Commit 8e52e96 ("fix(scripts): propagate git_init_test_repo refusal in git_fixture before add/commit can reach the real repo"), already on this branch, addresses it directly:

git_init_test_repo "$fixture" || return 1
git -C "$fixture" add -A
git -C "$fixture" commit -qm base

(scripts/sync-standards-contract.test.sh#L79-L83)

I verified the failure now short-circuits before add/commit can run, and separately confirmed the other two converted call sites (check-shell-portability.test.sh, check-skill-portability.test.sh) were never exposed to this class of bug — there, git_init_test_repo "$fx" sits inside a single &&-chained $(...) subshell alongside the commit, e.g. scripts/check-shell-portability.test.sh#L1544-L1550, so a refusal there already short-circuits the whole chain and no downstream git command runs.

Other checks:

• git_init_test_repo/git_init_safe (scripts/test-git-helpers.sh#L46-L79) is a strict superset of the inline blocks it replaces — same identity semantics plus the inside-checkout guard (test(scripts): scope portability-suite fixture git commands with -C #2839/test-fixture isolation: git -C does not survive an exported GIT_DIR #2840 class) and gpgsign/autocrlf hardening.
• The removal of the three converted suites from scripts/fixture-git-isolation-baseline.txt is consistent with check-fixture-git-isolation.sh's sources_isolating_harness detection (scripts/check-fixture-git-isolation.sh#L337-L376) — a suite sourcing test-git-helpers.sh is now correctly counted as isolated, so no gate is being weakened.
• check-stale-base-overlap.test.sh is correctly left unconverted (needs init -b, which the helper has no seam for), matching the PR description.

No inline comments were needed since the one substantive finding raised on this PR (Codex's) was already resolved by a later commit on the branch. No blocking findings remain.