fix(tests): stop git_init from persisting identity into real repos

[kyle-sexton]

Fixes #2162.

Adds scripts/test-git-helpers.sh with git_init_safe (refuses paths inside the current checkout) and git_test_config (per-invocation identity). Adopted across the six harnesses named in the issue.

Related

Fixes #2162. Shared git test harness adoption across plugin test scripts.

[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 10s —— View job

---

Security Review

• Gather context (PR diff, changed files)
• Review scripts/test-git-helpers.sh (new helper: git_init_safe, git_test_config)
• Review adoption in the 5 modified test harness files
• Check for injection / path-traversal / privilege issues
• Post findings

Scope: This PR only touches test harness scripts (*.test.sh) and adds scripts/test-git-helpers.sh. No production code, no GitHub Actions workflow files, no secrets, no permissions/config changes.

No security issues found.

Notes from the review:

• scripts/test-git-helpers.sh is a net security improvement: it replaces persistent git config user.email/name/... calls (which wrote throwaway test identity into whatever .git/config happened to be targeted) with per-invocation -c overrides via git_test_config, and adds git_init_safe, which resolves both the target dir and the current repo's toplevel with cd ... && pwd and refuses to git init if the target path is the repo root or nested under it. That directly closes the "identity persisted into a real repo" failure mode the PR title describes.
• The path-containment check at L27-L36 quotes "$abs_repo" in the case pattern, so any glob metacharacters in the real repo path (*, ?, [) are treated literally rather than as pattern syntax — the containment check can't be tricked by a repo path containing those characters.
• All target directories passed to git_init_safe/git_test_config across the five adopted harnesses come from mktemp -d, not from any untrusted/external input (PR diff content, network data, etc.), so there's no attacker-controlled path or injection surface here — this is CI-local throwaway-fixture plumbing.
• Minor (informational, not a finding worth fixing): git_init_safe's containment check compares cd "$dir" && pwd against git rev-parse --show-toplevel piped through the same cd/pwd; on a platform where $TMPDIR (or the repo checkout path) traverses a symlink, the two resolved forms could theoretically diverge and let a symlinked path slip past the check. Not exploitable here since no adversarial input reaches $dir, and CI runners don't symlink /tmp, so I'm not raising it as a finding — just noting it for awareness if this helper is ever reused somewhere with less-trusted paths.

Supply-chain/unpinned-action/permissions/trigger-pattern concerns are out of scope for this lane per instructions (zizmor's advisory covers those), and this PR doesn't touch any workflow files anyway.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3167fff12

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

---

Code review

• Gather context (CLAUDE.md, REVIEW.md, PR diff)
• Understand the request (code review)
• Review scripts/test-git-helpers.sh (new file)
• Review adoption across the 6 harness test files
• Post findings

Reviewed against REVIEW.md's code-review lane scope (correctness, design, error handling, tests, maintainability; security is out of scope here since .github/workflows/claude-security-review.yml exists). CLAUDE.md is empty at both the repo root and .claude-pr/, so no repo-specific conventions apply beyond REVIEW.md.

Note: I was not able to execute the shell test suites or shellcheck in this environment (tool permission approval for bash <script> / shellcheck was unavailable), so the review below is static-analysis only — I did not independently run scripts/*.test.sh to confirm they pass. Any pass/fail claims in the PR description are author-claimed and unverified by me.

scripts/test-git-helpers.sh (new)

The core design is sound: git_test_config uses git -c ... (per-invocation, never written to disk) instead of git config (which persists into .git/config), and git_init_safe adds an explicit guard that refuses to init a path inside the current checkout by comparing against git rev-parse --show-toplevel. That combination directly addresses the "identity/init leaking into a real repo" failure mode. No correctness issues found in the helper itself.

Two inline suggestions posted

• scripts/check-changelog-parity.test.sh (git_commit(), lines 38–43): the new shared helper drops the && that used to make commit skip when add -A failed. Low practical risk for throwaway fixtures, but worth keeping the fail-fast chain since other call sites will inherit this helper's semantics.
• plugins/claude-ops/skills/plugins/scripts/fleet-state.test.sh:344: this harness hand-rolls the same -c user.email=... -c user.name=... -c commit.gpgsign=false flags instead of sourcing scripts/test-git-helpers.sh, so it fixes the identity-persistence half but doesn't get the git_init_safe path guard the other five harnesses now have. Likely fine in practice (the fixture dir is always under $TEST_TMPDIR), but worth a comment explaining why this one is hand-rolled, for future maintainers.

Cross-file observation (not part of this diff's file set, flagged as a completeness note rather than a defect in the changed lines)

The same anti-pattern this PR fixes still exists, unmigrated, in three other harnesses that weren't touched here:

• scripts/check-shell-portability.test.sh#L1541-L1543 — cd "$fx" && git init -q && git config user.email ... && git config user.name ...
• scripts/check-skill-portability.test.sh#L620-L623 — identical pattern
• scripts/sync-standards-contract.test.sh#L64-L74 — git_fixture() still calls git -C "$fixture" config user.email/name (persists to that fixture's .git/config) rather than the new -c-flag pattern

These predate this PR and are outside its stated "six harnesses," so this isn't a regression from this diff — but if the goal (per the PR title) is to stop test fixtures from persisting throwaway identity anywhere, these three are the same bug class left open. Worth a quick follow-up issue/PR to migrate them onto scripts/test-git-helpers.sh (or an equivalent for sync-standards-contract.test.sh, which is a plain -C-style helper and could adopt git_test_config directly).

Everything else

The mechanical conversions in affected-tests.test.sh, check-changed-skills.test.sh, check-docs-only.test.sh, and the bulk of check-changelog-parity.test.sh are straightforward, correct substitutions of git_init_safe/git_test_config for the old inline git init/git config sequences — behavior is preserved (same config values: user.email, user.name, commit.gpgsign=false, core.autocrlf=false).

[github-actions]

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

[claude]

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

---

Security Review

• Gather context (PR diff, changed files)
• Review scripts/test-git-helpers.sh (new helper: git_init_safe, git_test_config)
• Review adoption in the modified test harness files
• Check for injection / path-traversal / privilege issues / dangerous Actions patterns
• Post findings

Scope: This PR only touches test harness scripts (*.test.sh) and adds scripts/test-git-helpers.sh. No production code, no GitHub Actions workflow files, no secrets, no permissions/config changes — zizmor's advisory lane doesn't apply here since there's nothing for it to cover.

---

Finding 1 — git_init_safe's containment guard is not enforced by any caller (fail-open)

Severity: IMPORTANT · Confidence: Medium-High

git_init_safe returns 1 when it refuses to initialize a path inside the current checkout (L33-L36). That guard is the entire security purpose of this PR — it's what's supposed to stop throwaway test identity/commits from landing in the real repository (issue #2162). But every call site invokes it as a bare statement and none of the five harnesses run with set -e (all use set -uo pipefail):

• scripts/affected-tests.test.sh#L47 — git_init_safe "$dir" (return value discarded)
• scripts/check-changed-skills.test.sh#L40
• scripts/check-docs-only.test.sh#L30
• scripts/check-changelog-parity.test.sh#L34-L35 — the local git_init() wrapper just forwards to git_init_safe "$1", same gap

If the guard trips (e.g. TMPDIR resolves to a path inside the checkout — plausible in sandboxed/containerized CI runners, or any environment where ${TMPDIR:-/tmp} is repo-relative), mk_repo/git_init logs a warning to stderr and returns 1, but every one of these set -uo pipefail scripts (no -e) continues past it. The very next lines call git_test_config "$dir" add -A / commit against a directory with no .git of its own — git -C walks up the filesystem to find the enclosing repository, meaning add -A (no pathspec) stages the entire real working tree, not just the fixture directory, and commit creates a real commit in the actual repo's history. That's precisely the failure mode this PR sets out to fix, reintroduced silently the moment the guard's precondition is hit, with no visible failure other than an easily-missed stderr line and (likely) a downstream test assertion failure that doesn't explain the root cause.

This was independently flagged by Codex on affected-tests.test.sh:47 alone; the same gap exists at all four call sites shown above, not just that one. Recommend git_init_safe "$dir" || return 1 (or || exit 1 at top level) at each call site — or making the harnesses set -e-clean and relying on that, if that's already the intended invariant elsewhere in this codebase.

Finding 2 — fleet-state.test.sh never got the containment guard at all

Severity: SUGGESTION · Confidence: High

plugins/claude-ops/skills/plugins/scripts/fleet-state.test.sh#L344 fixes the identity-persistence half (-c user.email=... instead of git config) but calls raw git init -q directly — it doesn't source scripts/test-git-helpers.sh or get git_init_safe's repo-containment check. project_dir here is always derived from a fresh mktemp -d (TEST_TMPDIR), so today's practical exposure is low and this is the same root cause as Finding 1 (repo-local TMPDIR), not a new one. Already noted in the prior code-review pass as a maintainability gap; flagging here too since it's the one call site in this PR's diff with zero containment protection, for completeness of the security posture this PR claims to establish across "the six harnesses."

---

No injection, path-traversal-via-attacker-input, or credential/secret-handling issues found — all directory arguments passed to these helpers originate from mktemp -d within the test process itself, not from any untrusted/external input. git_init_safe's repo-root comparison (L32-L33) correctly quotes the pattern, so it can't be defeated by glob metacharacters in the checkout path.

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31564785955
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31564785922
• Failure class: other
• Last SDK result: (no execution file was produced)

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).