fix(scripts): fail loud when absorbed-heading fork-point read errors

[kyle-sexton]

Fixes #2324

Summary

• Replace the absorbed-heading check's git show ... 2>/dev/null | changelog_versions process substitution with git ls-tree probing plus command-substitution git show whose exit status is checked.
• A genuine git read failure now exits 2 with a diagnostic instead of reading as "nothing to preserve" and passing (fail-open).

Test plan

• bash scripts/check-changelog-parity.test.sh (63/0)
• New fault-injection fixture: git shim that fails git show only for CHANGELOG.md paths

Related

• check-changelog-parity: a merge-forward that absorbs the predecessor release section passes all three modes — deleted headings are unpoliced #2264 / fix(scripts): reject absorbed changelog release headings (#2264) #2290 — original absorbed-heading gate
• fix(scripts): key the absorbed-heading check on the version, not the line (#2327) #2341 — version-keyed comparison (false-positive fix)
• check-changelog-parity: absorbed-heading preservation is scoped to bumped plugins, so a changelog-only change set and every convention changelog are unpoliced #2342 — broader scope hardening (changelog-only + convention changelogs) tracked separately

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

---

Security Review — PR #2369

• Gather context (PR diff, changed files)
• Review scripts/check-changelog-parity.sh for injection / trust-boundary issues
• Review scripts/check-changelog-parity.test.sh for fault-injection fixture safety
• Post findings

No security issues found.

This PR is a narrowly-scoped bug fix that converts a fail-open code path into fail-closed:

• scripts/check-changelog-parity.sh (lines 450–470): the old git show "$merge_base:$changelog" 2>/dev/null | changelog_versions - inside a process substitution (< <(...)) silently swallowed git show's exit status — a genuine read failure (corrupt/missing object, shallow clone) was indistinguishable from "no changelog existed at the fork point," which let the absorbed-heading gate pass when it should have failed. The new code separates the check into an existence probe (git ls-tree -r --name-only "$merge_base" -- "$changelog") and an explicit command-substitution read (base_body="$(git show "$merge_base:$changelog")") with the exit status checked directly, exiting 2 with a diagnostic on failure instead of degrading to an empty list.
• All git invocations pass $merge_base, $changelog, and $manifest as discrete arguments (not through eval/sh -c/string-built commands), so there's no shell-injection surface even though changelog/manifest paths are derived from plugin directory names discovered in the (potentially attacker-controlled, in a fork-PR context) repo tree. The rev:path colon-splitting in git show only splits on the first colon, so a crafted directory name containing a colon doesn't cause path/rev confusion.
• The new error paths only echo git ref/path strings to stderr for diagnostics — no secrets, tokens, or credentials are read or logged anywhere in this diff.
• scripts/check-changelog-parity.test.sh's new fixture builds a fake git shim in a mktemp -d directory and prepends it to PATH for a single subshell invocation of the script under test, then removes it — a standard, self-contained fault-injection technique with no effect outside the test's own subshell/temp dir.

No injection, authorization, credential-handling, or fail-open/permission-widening concerns identified in the changed files.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b393097e60

ℹ️ 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 58s —— View job

---

Code review

• Gather context (diff, CLAUDE.md, REVIEW.md)
• Understand the request
• Review scripts/check-changelog-parity.sh changes
• Review scripts/check-changelog-parity.test.sh changes
• Post findings

This repo has a claude-security-review workflow, so this review stays in the code-review lane (correctness, tests, maintainability) and leaves security scope to that lane.

Note on verification: I could not execute bash scripts/check-changelog-parity.test.sh or even bash -n the script in this sandbox (both require approval that isn't available in this lane), so the PR's Test plan (63/0, new fixture) is author-claimed and unverified by me. I instead traced the new control flow by hand against the surrounding code.

What the fix does

Replaces git show "$merge_base:$changelog" 2>/dev/null | changelog_versions - (process substitution — swallows git show's exit status, so a genuine read failure silently reads as "no fork-point headings" and the absorbed-heading gate passes) with a git ls-tree presence probe followed by a status-checked git show in command substitution, exiting 2 with a diagnostic on either failing. The git ls-tree-before-git cat-file -e reasoning (a missing path is exit 0/empty output; a bad rev is non-zero either way) is correct and matches real git behavior. Logic reads soundly to me.

Findings

1. The same fail-open pattern this PR fixes is still present two call sites earlier, in the same loop — 🔴 Important (design consistency / correctness completeness)

scripts/check-changelog-parity.sh#L389 and #L401:

base_version="$(git show "$base:$manifest" 2>/dev/null | jq -r '.version // empty' 2>/dev/null || true)"
...
fork_version="$(git show "$merge_base:$manifest" 2>/dev/null | jq -r '.version // empty' 2>/dev/null || true)"

Both still use 2>/dev/null | ... || true to read the plugin manifest at $base/$merge_base — the exact fail-open shape #2324 was filed against, just for plugin.json instead of CHANGELOG.md. A genuine git show failure here (corrupt object, shallow-clone gap) reads as base_version/fork_version empty, which at L392 is read as "new plugin, not in scope" (continue, skipping the whole bump-parity check for that plugin) and at L402 is read as "version unchanged" (also continue) — silently skipping the gate entirely rather than just missing one sub-check. That's a strictly larger blast radius than the bug this PR fixes.

This may be intentionally deferred — the PR description scopes itself to "absorbed-heading fork-point read" and points to #2342 for "broader scope hardening," though that issue's stated scope (changelog-only + convention changelogs) doesn't obviously cover these two manifest reads. Worth confirming whether it's tracked, since as written this PR's fix doesn't close the fail-open class for this function, just one instance of it.

2. Inline: test coverage gap — the new git ls-tree failure branch (the more realistic real-world trigger, e.g. a shallow clone missing the fork-point commit) has no fault-injection fixture; only the git show failure branch is tested.

3. Inline: portability nit — the test's git shim hardcodes /usr/bin/git as the real-git fallback, which won't resolve on macOS/Nix/other layouts, unlike the rest of the file's portability-conscious style.

Nothing else stood out — the process-substitution-to-command-substitution swap is a well-known correct pattern for this exact problem, the diagnostic messages accurately reflect the failing command, and set -uo pipefail (no -e) makes the manual status checks here necessary rather than redundant.

[github-actions]

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

[kyle-sexton]

Superseded by #2370, which landed --check-preserved with the same fail-loud git ls-tree/git show discipline for absorbed-heading detection. Rebasing onto current main leaves no unique diff in --check-bump (that path was removed in #2370).