fix(scripts): harden changelog absorbed-heading gate fail-closed

[kyle-sexton]

Hardens changelog absorbed-heading detection to fail closed on unreadable base blobs.

Fixes #2324

Summary

Hardens changelog absorbed-heading detection to fail closed on unreadable base blobs.

Related

N/A

[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 2m 59s —— View job

---

Security Review

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

This PR only touches scripts/check-changelog-parity.sh and its test suite — a CI gate script invoked by the changelog-parity-gate job in .github/workflows/ci.yml on a plain pull_request trigger (not pull_request_target), with persist-credentials: false and no secrets in scope. No workflow file, permissions block, or action pins changed in this PR, so the supply-chain/trigger/permissions lane is out of scope here (and is zizmor's territory anyway).

All new git invocations (git cat-file -e "$merge_base:$changelog", git show "$merge_base:$changelog") are consistently double-quoted with variables that are either a computed 40-char SHA ($merge_base, already validated via git merge-base ... || exit 2) or a path drawn from git diff --name-only filtered to the fixed plugins/*/CHANGELOG.md / docs/conventions/*/CHANGELOG.md shape — no shell/command injection surface.

One logic finding worth flagging (correctness of the "fail-closed" claim, not an actively exploitable bypass):

SUGGESTION / confidence: PLAUSIBLE — missing_preserved_headings()'s new-file probe can silently convert a genuine git failure into a false "pass," which is the exact failure mode the file's own --check-preserved implementation was deliberately written to avoid.

scripts/check-changelog-parity.sh#L245-L247:

if ! git cat-file -e "$merge_base:$changelog" 2>/dev/null; then
    return 0
fi

git cat-file -e <rev>:<path> exits non-zero both when the path is legitimately absent at that rev and when the git invocation genuinely fails — it can't distinguish the two (documented exit 128 in both cases). This function treats any non-zero exit as "the changelog is new at the fork point, nothing to preserve" and returns success. That is precisely the ambiguity the existing --check-preserved implementation calls out and deliberately avoids by using git ls-tree -r --name-only instead — see its own comment at scripts/check-changelog-parity.sh#L486-L488: "git cat-file -e cannot make that distinction — it exits 128 for both... A failure must never read as 'nothing to preserve'."

So a genuine git-read failure at that exact path (e.g. a corrupted/partial object store, or any transient git cat-file failure) would be silently swallowed here and the absorbed-heading check for that changelog would be skipped rather than failing the gate — the opposite of "fail-closed."

Why this doesn't currently look exploitable as a merge-gate bypass: the sibling --check-preserved step (unchanged by this PR, still using the correct ls-tree probe) runs later in the same CI job (changelog-parity-gate in ci.yml) against the same touched_changelogs set, with no continue-on-error. A real absorbed/deleted heading would still fail the job via that step even if this new --check-bump path mis-fires open. There's also no way for a PR author to deterministically force a cat-file failure at a specific path via crafted diff content, given fetch-depth: 0 and a $merge_base that's already independently validated. So this is a robustness/defense-in-depth gap in the new detection path, not a demonstrated way to get an absorbed release note past CI today — worth tightening (mirror the ls-tree probe used by --check-preserved) so the two implementations don't diverge in fail-safety, but not a standalone exploitable bypass given the current job wiring.

No other injection, authorization, credential-handling, or trust-boundary issues found in this diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54394d0bfc

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

---

Review: fix(scripts): harden changelog absorbed-heading gate fail-closed

Reviewed scripts/check-changelog-parity.sh and scripts/check-changelog-parity.test.sh against origin/main. This is the code-review lane (a claude-security-review workflow exists in this repo, so no security findings are reported here — that scope belongs to the security lane).

Todo

• Fetch PR diff and read changed files
• Read repo review guidelines (REVIEW.md, CLAUDE.md)
• Analyze missing_preserved_headings and its integration into --check-bump
• Cross-check against the sibling --check-preserved logic for consistency
• Review test coverage additions
• Post findings as inline review comments
• Finalize summary

Findings (posted inline on the changed lines):

1. 🔴 check-changelog-parity.sh#L245 — the new missing_preserved_headings probes fork-point existence with git cat-file -e, which can't distinguish "path absent" from "git read failed." The sibling --check-preserved logic a few hundred lines below solves the identical $merge_base:$changelog probe with git ls-tree specifically to avoid that ambiguity, and its comment there explicitly states cat-file -e is unsuitable for this — the new function's own comment claims the opposite. Currently masked in CI by fetch-depth: 0, but not fail-closed for a local/shallower run.
2. 🟡 check-changelog-parity.sh#L253 — base_versions/head_versions aren't declared local in the new function (unlike its other locals), leaking to global scope. Harmless today since --check-bump/--check-preserved never run in the same invocation, but inconsistent with the file's otherwise careful scoping.
3. 🟡 check-changelog-parity.sh#L558 — the new absorbed_reported dedup guard is dead code: touched_changelogs is already deduplicated when built (seen_changelog in the shared prologue), so no path can repeat.

Other notes

• The --check-bump scope change (reasoning over touched_changelogs rather than only bumped_candidate) correctly closes the "changelog absorbed a heading with no manifest bump" gap the new tests exercise, and mirrors --check-preserved's existing fork-point (not base-tip) comparison.
• Test additions (ABSORBED HEADING, ABSORBED without manifest bump) look well targeted at the two gaps described in the PR body, and the removal of --check-bump $base from the old "should still pass" list in the pre-existing absorbed-section test is correctly replaced with a dedicated assertion that it now fails.
• I was not able to execute the test suite or shellcheck myself in this environment (tool permissions blocked non-git shell execution), so test-pass claims in the PR description are author-reported and unverified by me — only the diff and script logic above were independently analyzed.

[github-actions]

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

[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/31591407312
• 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).

[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/31591407171
• 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.