Skip to content

fix(ci): stop has_heading's early exit from SIGPIPE-failing the parity gate - #2159

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/changelog-parity-sigpipe
Aug 10, 2026
Merged

fix(ci): stop has_heading's early exit from SIGPIPE-failing the parity gate#2159
kyle-sexton merged 1 commit into
mainfrom
fix/changelog-parity-sigpipe

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes #2158

Problem

changelog-parity-gate failed PR #2130 twice with UNDOCUMENTED BUMP: markdown-format went 0.11.3 -> 0.11.4 ... even though plugins/markdown-format/CHANGELOG.md carries ## [0.11.4] at line 6, column one — a required merge gate confidently asserting the opposite of the truth, while the same command passed locally. PR #2135 then failed the same gate on every one of its sixteen bumped plugins (smallest flagged changelog: 15 KB). The regression landed on main at 15:19:19Z in #2154 and blocks every PR that bumps a plugin whose changelog exceeds roughly one stdio buffer (~4 KB) — which newest-first ordering makes essentially all of them.

Blast radius — precisely

Confined to the --check-bump path: has_heading is defined inside that branch and called in exactly two places (the head-side check and the base-side git show "$base:$changelog" | has_heading). --check and --check-order read changelogs through changelog_versions, whose grep -oE stages drain stdin with no early exit and cannot take SIGPIPE. So the failure class is exactly "PRs that bump a manifest version"; both failing call sites go through the one function this PR fixes.

Root cause

has_heading runs a pipeline under set -o pipefail whose reader exits on first match:

rendered_lines - | awk -v h="$heading" 'index($0, h) == 1 { found = 1; exit } END { exit !found }'

The newest heading sits near the top, so the reader exits while rendered_lines is still writing; the writer dies of SIGPIPE (141) and pipefail reports the pipeline — the FOUND heading — as a failure. Reproduced deterministically in an ubuntu:24.04 container at the exact CI merge commit ba4b72fb: PIPESTATUS=141 0 and the byte-identical CI error under gawk (what the ubuntu-24.04 runner resolves /usr/bin/awk to — gawk outranks mawk in the alternatives system, and only the gawk mechanism explains CI failing 15 KB files). mawk survives the closed pipe and passes at every size tested, and Windows/MSYS process timing lets the writer finish first — which is why the failure existed only in CI. The suite's 55 fixtures all fit in one buffer — hence PASS=55 in the very job that then failed on the real file.

Fix

The reader consumes to EOF; END { exit !found } decides. Correct by construction: no reader exits early, so no writer can ever take SIGPIPE, under any awk — the failure is impossible, not rarer. Chosen over restoring the pre-#2154 single-awk form because it preserves the one-tracker-three-modes property (rendered_lines shared by all modes, so they cannot drift). Both has_heading call sites are covered (same function); the script has no other early-exiting reader downstream of a pipe (changelog_versions greps drain stdin; the grep -m1 format probe reads a file directly, not a pipe).

Regression fixture — with its fails-against-unfixed proof, per engine

New --check-bump case: ~260 KB changelog, new ## [1.1.0] entry near the top, expected pass. 260 KB deliberately exceeds the 64 KB pipe capacity, so against the unfixed script the writer blocks mid-write and the SIGPIPE is deterministic, not a winnable race.

script gawk mawk
unfixed FAIL=1 (exact #2130 error text) PASS (mawk survives the closed pipe)
fixed 56/56 56/56

The fixture's guard is therefore engine-conditional — it discriminates only where awk resolves to gawk, which is what the runner resolves; this is recorded in the fixture comment. The fix itself is engine-independent. Windows local: 56/56 fixed.

Related

🤖 Generated with Claude Code

@cursor

cursor Bot commented Aug 10, 2026

Copy link
Copy Markdown

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 Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 25s —— View job


Review complete

  • Gather context (PR diff, CLAUDE.md, changed files)
  • Understand the request (code review of correctness/maintainability)
  • Review scripts/check-changelog-parity.sh fix
  • Review scripts/check-changelog-parity.test.sh regression fixture
  • Post findings as inline comments + summary

Scope check: No claude-security-review.yml findings apply here (no auth, tenant data, secrets, or externally-built process/query strings touched), so per REVIEW.md this stays entirely in the code-review lane. This isn't a plugin-manifest/marketplace-schema/hook-contract change either, so the fresh-docs mandate in CLAUDE.md doesn't apply.

Correctness of the fix (scripts/check-changelog-parity.sh lines 442–447): the diagnosis and fix both check out on a static read. awk's exit inside a main-body rule jumps straight to END and stops consuming input — it doesn't drain the rest of stdin first. Under set -uo pipefail (line 63), that means the upstream rendered_lines awk can still be mid-write() on a large file when the reader's pipe closes, so the writer takes SIGPIPE (141) and pipefail reports the pipeline as failed even though the heading was genuinely found = 1. Dropping the exit so the rule only sets found and lets END { exit !found } decide preserves the exact same pass/fail semantics (found is monotonic — matching more than once is harmless) while forcing the reader to consume all of stdin, which is the correct general fix for this pipefail/SIGPIPE class rather than a narrow patch. I checked the rest of the file for the same early-exit-under-pipefail shape (the changelog_versions pipeline at line 199 uses grep/grep, neither of which short-circuits) and didn't find another instance.

Test coverage (scripts/check-changelog-parity.test.sh lines 220–245): the new fixture's size/shape reasoning is sound — a match on line 3 of a ~260 KB file forces the upstream awk to still be writing when a would-be early-exiting reader closes, whereas the existing 55 fixtures are all small enough that rendered_lines' writes complete before the reader could close the pipe (no SIGPIPE possible regardless of early-exit), which is why they never caught this. The test is well-placed next to the existing bump-passes case and follows the file's established style (mk_repo/git_init/mk_plugin helpers, rc/out capture pattern used by other tests). No set -e in either script, so the out="$(...)" / rc=$? sequencing used here is safe.

Not independently verified: I was not able to execute the test suite or shellcheck in this sandbox (Bash tool calls to run them were blocked pending approval that isn't available in this review lane), so I can't confirm the 56/56 pass claim or the container-reproduction (PIPESTATUS=141 0, PASS=55 FAIL=1) myself — those are author-claimed in the PR description and should be treated as unverified until CI runs. Everything above is from static reading of the diff and surrounding file, not execution.

No correctness, error-handling, or maintainability issues found. The comment additions accurately document the pipefail/SIGPIPE constraint for future readers, consistent with this file's existing heavily-annotated style.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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


Security review complete

  • Gather context (PR diff, changed files)
  • Review scripts/check-changelog-parity.sh changes for security issues
  • Review scripts/check-changelog-parity.test.sh changes for security issues
  • Post findings

Scope reviewed: the full diff (origin/main...HEAD), 2 files:

  • scripts/check-changelog-parity.sh (+10/-1) — removes the exit from the has_heading awk reader's match action and adds an explanatory comment on the pipefail/SIGPIPE constraint.
  • scripts/check-changelog-parity.test.sh (+27/-0) — adds a ~260 KB changelog regression fixture exercising the fix.

Findings: none.

Notes on what I specifically checked and ruled out:

  • Injection surface: awk -v h="$heading" '...' at check-changelog-parity.sh#L436-L440 passes $heading through -v variable assignment, not string-interpolated into the awk program text, so no version string (however crafted) can inject awk code. This was true before the change and is unaffected by it.
  • Behavioral change is a correctness fix, not a trust-boundary change: removing the early exit makes the reader consume its full input instead of racing the writer; it doesn't alter what input is trusted, doesn't widen what the gate accepts, and doesn't change any permission, credential, or secret handling. If anything it makes the gate more conservative (scans the whole file rather than stopping at the first hit).
  • Test fixture: the new test builds a throwaway repo under a temp dir with static/loop-generated content (printf, a bounded for loop with a fixed literal string) and runs git/the script against it — no untrusted input, no eval, no injectable interpolation.
  • No workflow, permissions, or CI config changes — both changed files are shell scripts under scripts/; .github/workflows/* is untouched.
  • Supply-chain / unpinned-action / dangerous-trigger / permissions concerns are out of scope here per zizmor's advisory lane, and moot anyway since no workflow files changed.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

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

@kyle-sexton
kyle-sexton force-pushed the fix/changelog-parity-sigpipe branch from 9742d1e to 00ce31a Compare August 10, 2026 16:30
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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


Review complete

  • Gather context (PR diff, CLAUDE.md, changed files)
  • Understand the request (code review of correctness/maintainability)
  • Review scripts/check-changelog-parity.sh fix
  • Review scripts/check-changelog-parity.test.sh regression fixture
  • Post findings as inline comment + summary

Scope check: This is a two-file shell-script diff with no plugin manifest, marketplace schema, hook contract, or documented harness behavior touched, so the CLAUDE.md fresh-docs mandate doesn't apply. Per this lane's split, security is out of scope here (already covered by the dedicated security-review lane on this same commit, which reported no findings).

Correctness of the fix (check-changelog-parity.sh#L442-L446): confirmed by reading the surrounding pipeline, not just the diff. rendered_lines (L143-L182) is a straight-line filter with no early exit of its own, so it's exactly has_heading's inner awk reader that was racing it under set -uo pipefail (L63). Dropping the match-rule exit so only END { exit !found } decides is the correct general fix for that pipefail/SIGPIPE class: found is monotonic (re-setting it on a second match is harmless), and the reader now always drains its input, so the upstream writer can never see a closed pipe mid-write. I checked the file's other awk/grep pipelines (changelog_versions at L199-L202) for the same shape and found none — this really was the only early-exiting reader in the script.

Test coverage (check-changelog-parity.test.sh#L220-L252): the fixture is well-placed next to the existing bump-passes case, reuses the established mk_repo/git_init/mk_plugin helpers, and its size/shape reasoning holds up — a match 3 lines into a ~281 KB file (4000 padding lines × ~72 bytes) forces rendered_lines' awk to still be mid-write when an early-exiting reader would close the pipe, unlike the existing small fixtures which complete before any reader could close early. Left one inline suggestion on this fixture: its regression coverage is real for CI (ubuntu-24.04 per .github/workflows/ci.yml L495, gawk per the PR description) but is a silent no-op wherever awk resolves to mawk, since the test doesn't assert or select an engine — not a blocker, just worth making loud rather than quiet.

Not independently verified: I don't have execution access in this sandbox, so I can't confirm the 56/56 pass claim or the container reproduction (PIPESTATUS=141 0, PASS=55 FAIL=1) myself — those remain author-claimed per the PR description, consistent with the prior review pass on this same commit.

No correctness, error-handling, or maintainability blockers found — one non-blocking suggestion posted inline.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 57s —— View job


Security review complete

  • Gather context (PR diff at HEAD 00ce31a, changed files)
  • Review scripts/check-changelog-parity.sh changes for security issues
  • Review scripts/check-changelog-parity.test.sh changes for security issues
  • Post findings

Scope reviewed: git diff origin/main...HEAD, 2 files:

  • scripts/check-changelog-parity.sh (+10/-1) — removes the exit from the has_heading awk reader's match action and adds an explanatory comment on the pipefail/SIGPIPE constraint (lines 429–447).
  • scripts/check-changelog-parity.test.sh (+33/-0) — adds a ~260 KB changelog regression fixture exercising the fix.

Findings: none.

Same conclusion as the prior security pass on this PR — the diff content is unchanged, only re-verified against the current HEAD SHA.

What was specifically checked and ruled out:

  • Injection surface: awk -v h="$heading" '...' at check-changelog-parity.sh#L441-L445 passes $heading through -v variable assignment, not string-interpolated into the awk program text, so no version string can inject awk code. This was true before the change and is untouched by the diff — only the match-action body (index($0, h) == 1 { found = 1 }, dropping the inline exit) and the END block's decision logic are affected.
  • Trust boundary / behavior: the change makes the reader consume all of stdin instead of racing the writer and exiting early. It doesn't alter what input is trusted, doesn't widen what the gate accepts as a valid bump, and touches no credential, secret, or permission handling. Net effect is more conservative (full-file scan vs. early stop), not more permissive.
  • Test fixture: builds a throwaway repo under a temp dir with static/loop-generated content (printf, a bounded for loop over a fixed literal string) and runs git/the script against it. No untrusted input, no eval, no injectable interpolation.
  • No workflow or permissions changes — both files are shell scripts under scripts/; nothing under .github/workflows/ is touched, so supply-chain/unpinned-action/dangerous-trigger/permissions concerns (zizmor's lane) don't apply and are moot regardless.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@kyle-sexton
kyle-sexton force-pushed the fix/changelog-parity-sigpipe branch from 00ce31a to d5e2f0e Compare August 10, 2026 16:32
Comment thread scripts/check-changelog-parity.test.sh Outdated
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 45s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review scripts/check-changelog-parity.sh changes for security issues
  • Review scripts/check-changelog-parity.test.sh changes for security issues
  • Post findings

Scope reviewed: git diff origin/main...HEAD at HEAD d5e2f0e, 2 files:

  • scripts/check-changelog-parity.sh (+10/-1) — removes the inline exit from the has_heading awk reader's match action (found = 1 { exit }found = 1), leaving END { exit !found } as the sole decision point, plus an explanatory comment on the pipefail/SIGPIPE constraint (lines 429–447).
  • scripts/check-changelog-parity.test.sh (+33/-0) — adds a ~260 KB changelog regression fixture exercising the fix (lines 220–252).

Findings: none.

What was specifically checked and ruled out:

  • Injection surface: awk -v h="$heading" '...' at check-changelog-parity.sh#L441-L445 passes $heading through -v variable assignment, never string-interpolated into the awk program text, so no version string (however crafted) can inject awk code. This is unchanged by the diff — only the match-action body and control flow are touched, not how $heading reaches awk.
  • Trust-boundary / behavioral impact: dropping the early exit only makes the reader consume the rest of its stdin before END decides; it doesn't change what input is trusted, what the gate accepts as a valid bump, or touch any credential/secret/permission handling. The change is strictly more conservative (full-file scan) than before, not more permissive — no way this widens the gate's acceptance criteria or creates a bypass.
  • Test fixture: builds a throwaway repo under a temp dir with static/loop-generated content (printf, a bounded arithmetic for loop emitting a fixed literal string) and runs git/the script against it locally. No untrusted input, no eval, no injectable interpolation, no network access.
  • No workflow, permissions, or CI config changes — both changed files are shell scripts under scripts/; nothing under .github/workflows/ is touched, so supply-chain/unpinned-action/dangerous-trigger/permissions concerns (zizmor's advisory lane) don't apply and are moot regardless.

Consistent with the prior security passes on this PR (unchanged diff content, re-verified against current HEAD). No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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


Reviewing PR #2159

  • Gather context (diff, CLAUDE.md, changed files)
  • Understand the request (code review)
  • Review scripts/check-changelog-parity.sh fix
  • Review scripts/check-changelog-parity.test.sh regression fixture
  • Post findings

Review complete

Scope: Security is covered by the dedicated security lane (already run twice on this PR with no findings), so this pass is correctness/maintainability only, per REVIEW.md's lane split. This isn't a plugin manifest/marketplace-schema/hook-contract change, so the fresh-docs mandate doesn't apply.

Fix correctnessscripts/check-changelog-parity.sh#L433-L446: confirmed by reading the surrounding code, not by execution. set -uo pipefail is set at line 63. With pipefail, a pipeline's status is the rightmost non-zero exit among its stages — so if the upstream rendered_lines awk is killed by SIGPIPE (141) while the downstream reader already exited 0 after an early match, the pipeline reports 141 despite the heading being genuinely found. Dropping the exit from the match rule (index($0, h) == 1 { found = 1 }) so only END { exit !found } decides forces the reader to drain all of stdin, which removes the race entirely — the writer can no longer receive SIGPIPE, on any awk implementation. This is the correct general fix for the pipefail/early-exit-reader class, not a narrow patch for the one file that triggered it.

I checked the rest of the file for the same shape (early-exit reader piped from a writer, under pipefail) and found only one other pipeline, changelog_versions at line 199 (rendered_lines | grep -oE | grep -oE) — neither grep stage short-circuits before EOF, so it isn't exposed to this bug, before or after this PR.

Test coveragescripts/check-changelog-parity.test.sh#L220-L253: the fixture's reasoning is sound — the new heading sits at line 3 of a ~277 KB file (4000 padding lines × ~71 bytes), forcing rendered_lines to still be mid-write when an early-exiting reader would close the pipe, which is exactly the condition the 55 pre-existing (small) fixtures can never reach. It follows the file's established helper/style conventions (mk_repo/git_init/mk_plugin, the out/rc capture pattern used by the neighboring negative-path tests), and the padding lines start with -, not ##, so they can't accidentally satisfy changelog_versions' heading regex and skew an unrelated check.

Not independently verified: Bash tool calls that would execute the test suite (or a standalone pipefail/SIGPIPE repro) were blocked pending approval unavailable in this review lane, so I could not confirm the 56/56 pass claim, the container reproduction, or the PASS=55 FAIL=1 claim myself — those remain author-claimed and should be treated as unverified until CI runs. Everything above is from static reading of the diff and the surrounding file.

No correctness, error-handling, or maintainability issues found — no inline comments filed.

…y gate

The --check-bump heading probe exited on first match while rendered_lines
was still writing. Under the script's pipefail, the writer's SIGPIPE death
(exit 141) became the pipeline's status, so a correctly documented bump in
any changelog larger than one stdio buffer — and the newest heading is
always near the top — was reported as UNDOCUMENTED BUMP. gawk, the CI
runner's awk, loses that race deterministically; the suite's small
fixtures fit in one buffer and never tripped it, which is how the gate
shipped green at PASS=55 and then failed the first real bump PR (#2130).

Scan the whole input instead of exiting on first match, and add a
large-changelog fixture (~260 KB, entry near the top) that fails against
the early-exit reader.

Closes #2158

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kyle-sexton
kyle-sexton force-pushed the fix/changelog-parity-sigpipe branch from d5e2f0e to da6de3f Compare August 10, 2026 16:51
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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


Reviewing PR #2159 (new commit)

  • Gather context (diff, CLAUDE.md, changed files)
  • Understand the request (code review)
  • Review scripts/check-changelog-parity.sh fix
  • Review scripts/check-changelog-parity.test.sh regression fixture (gawk-forcing update)
  • Post findings

Review complete

What changed since the last review pass: scripts/check-changelog-parity.sh is byte-identical in substance to what was already reviewed (the has_heading fix at L442-L446 — dropping the match-rule exit so END { exit !found } is the sole decision point — remains correct: it forces the reader to drain all of stdin, so the upstream rendered_lines writer can never see a closed pipe under pipefail, on any awk). The test file grew from +33 to +41 lines, adding exactly the fix requested in the prior inline SUGGESTION on this PR.

The gawk-forcing fix (check-changelog-parity.test.sh#L220-L259): this directly resolves the earlier finding that the SIGPIPE fixture was a silent no-op under mawk. The new code:

  • Builds a $repo/bin/awk shim that execs gawk, and prepends it to PATH only for the single --check-bump invocation under test — correct, since both awk call sites in the script (rendered_lines at L144 and has_heading at L443) invoke plain unqualified awk, so the shim reaches both.
  • Skips loudly (echo "SKIP: ..." >&2) rather than silently passing when command -v gawk fails, so a non-gawk dev machine gets a visible signal instead of false confidence.
  • The skip path doesn't touch PASS/FAIL counters (L12-L20), and the suite's exit status is ((FAIL == 0)) (L824) rather than a hardcoded total, so an environment-dependent skip can't desync the suite's pass/fail accounting.
  • rm -rf "$repo" runs unconditionally after the if/else, so the shim directory is cleaned up on both the run and skip paths.

This is exactly the fix suggested previously (force gawk, skip with a clear message if unavailable) and it's implemented correctly — no gaps found on a static read.

Not independently verified: Bash tool calls that would execute the test suite are blocked pending approval unavailable in this review lane, so I could not run scripts/check-changelog-parity.test.sh myself to confirm the fixture actually passes/skips as designed, nor confirm which awk this sandbox resolves to. This remains consistent with every prior review pass on this PR — verification is static-read only.

No correctness, error-handling, or maintainability issues found. No new inline comments — the one open SUGGESTION from the prior pass has been addressed by this commit.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 59s —— View job


Security review complete

  • Gather context (PR diff at HEAD da6de3f, changed files)
  • Review scripts/check-changelog-parity.sh changes for security issues
  • Review scripts/check-changelog-parity.test.sh changes for security issues
  • Post findings

Scope reviewed: git diff origin/main...HEAD, 2 files:

  • scripts/check-changelog-parity.sh (+10/-1) — removes the inline exit from the has_heading awk reader's match action (found = 1; exitfound = 1), leaving END { exit !found } as the sole decision point, plus an explanatory comment on the pipefail/SIGPIPE constraint (lines 429–446).
  • scripts/check-changelog-parity.test.sh (+41/-0) — adds a ~260 KB changelog regression fixture, now with a PATH shim that forces gawk and skips loudly if unavailable (lines 220–260).

This is a new commit (da6de3f) since the prior reviews on this PR; the only new logic is the gawk-forcing shim added in response to the earlier inline suggestion about the test being a silent no-op under mawk.

Findings: none.

What was specifically checked, including the new shim:

  • PATH-shim injection risk: the new code at check-changelog-parity.test.sh#L242-L247 writes a wrapper script (#!/bin/sh\nexec gawk "$@"\n) to $repo/bin/awk, chmod +xs it, and prepends $repo/bin to PATH for one subshell invocation of the script under test. $repo comes from mk_repo() (line 25), which is mktemp -d — not attacker-influenced — and the wrapper's content is a static literal, not built from any variable. No command/argument injection, no untrusted data reaching exec, and the PATH mutation is scoped to a single bash -c-style subshell invocation (out="$(cd "$repo" && PATH=... bash ...)"), so it can't leak into the rest of the test run.
  • Injection surface in the fix itself: unchanged from prior review — awk -v h="$heading" '...' passes $heading through -v variable assignment, not string-interpolated into program text, so no version string can inject awk code. This diff only touches the match-action body and control flow, not how $heading reaches awk.
  • Trust-boundary / behavioral impact: dropping the early exit only makes the reader consume all of stdin before END decides; it doesn't change what input is trusted or widen what the gate accepts as a valid bump — strictly more conservative (full-file scan), not more permissive.
  • No workflow, permissions, or CI config changes — both changed files are shell scripts under scripts/; nothing under .github/workflows/ is touched, so supply-chain/unpinned-action/dangerous-trigger/permissions concerns (zizmor's advisory lane) don't apply.

Consistent with the prior security passes on this PR. No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@kyle-sexton
kyle-sexton merged commit 416a61f into main Aug 10, 2026
33 checks passed
@kyle-sexton
kyle-sexton deleted the fix/changelog-parity-sigpipe branch August 10, 2026 17:02
kyle-sexton added a commit that referenced this pull request Aug 10, 2026
Takes main's scripts/check-changelog-parity.sh and its test suite verbatim.

#2159 landed the same root-cause fix this branch had staged -- dropping
has_heading's early exit so the matcher cannot SIGPIPE rendered_lines under
pipefail -- so the local change is redundant and main's version wins. This
branch goes back to being a pure songwriting content pass.

The one piece not in #2159 is hardening the `-` stdin operand into a
zero-argument call. That was defensive rather than demonstrated: the SIGPIPE
race is the confirmed cause and is now fixed, and nothing has shown a `-`
operand failing anywhere. Left out rather than carried on speculation; it can
be raised on its own if evidence for it ever appears.
kyle-sexton added a commit that referenced this pull request Aug 11, 2026
…ub list read and drop the positional-index verifications (#2163)

No linked issue

## Summary

Every GitHub REST list endpoint this repository's instructions read
returns **30 items per page** by default and reports nothing when it
truncates. A dozen documented call sites read them with no pagination,
so the guidance told operators and agents to draw conclusions from
silently partial data.

### The check-runs case, and the false conclusion it already caused

Reproduced deterministically against this repository's own PR heads with
the bare form:

```
2895890: total_count=33, returned=30
580fd09: total_count=33, returned=30
435b2fe: total_count=33, returned=30
```

On all three the dropped set was identical — the three earliest-started
checks:

```
2026-08-10T02:38:01Z  GitGuardian Security Checks
2026-08-10T02:38:03Z  pr-issue-linkage / pr-issue-linkage
2026-08-10T02:38:04Z  do-not-merge / do-not-merge
```

`do-not-merge / do-not-merge` is a **required status context** for
`main` (ruleset `17989001`). It is a metadata-only `pull_request_target`
job that reads label metadata and runs no head code, so it completes in
about three seconds — well before the heavy `pull_request` matrix. That
is exactly why it is always among the first started, and therefore
always the first truncated away.

A prior reading of this query concluded that `do-not-merge` "never
attaches to a head SHA" on three separate PRs, and recorded it as an
established finding. It was false. On `2895890c` the context attached at
`02:38:04Z`, completed `success` at `02:38:07Z`, and was green on every
one of those heads. The query was truncating.

Under 31 total checks nothing truncates, which is why earlier
occurrences looked like intermittent flakiness that resolved itself.

### The comment and review case, which is worse

`issues/<n>/comments`, `pulls/<n>/comments`, and `pulls/<n>/reviews` are
returned **oldest-first** (verified — ascending `created_at` /
`submitted_at`). An unpaginated read therefore drops exactly the
*newest* items: the only ones a monitoring poll or a "did my reply
post?" check cares about.

Two call sites paired that list with `.[-1]`. That shape does not omit —
it answers, plausibly, and wrongly, because `.[-1]` on a truncated
oldest-first page is the **30th-oldest** item:

```
issue #657  33 comments | true latest 2026-07-22T18:47:05Z | '.[-1]' unpaginated 2026-07-22T07:20:46Z   (11.5 h stale)
issue #502  31 comments | true latest 2026-07-23T23:29:21Z | '.[-1]' unpaginated 2026-07-23T23:28:27Z
```

Both are **issues**, not pull requests — the endpoint and the mechanism
are identical, but an earlier draft of this PR miscited them as PRs in
prose presented as measurement, and that is corrected here and in the
shipped docs.

Rule 3 was found the same way, by the same class of defect, in this
change set itself: two commands here reduced across pages inside `--jq`,
one of them in the file that states the rule. Gate 5's codex-comment
count printed `10 10 10 3` over four pages instead of `33`; the
work-item-tracker recipe emitted four separately-sorted arrays instead
of one sorted list. Both now slurp with `jq -s` and flatten with
`.[][]`. The rule draws the line that makes it usable: **element-wise
filters (`select`, `map` over `.[]`) are safe under `--paginate` because
their results concatenate; folds are not.**

## Fix

Every corrected site uses `--paginate` with `per_page=100`, matching the
form
`plugins/source-control/skills/pull-request/scripts/fetch-annotations.sh:111`
already used — that script was already correct and is untouched here.

The `.[-1]` verifications now select on the fix SHA instead, so the
query states what it is asserting and cannot be satisfied by another
author's comment.

### The sentinel

Pagination alone moves the cliff from 30 to 100 rather than removing it.
`readiness.md` gains a `Reading GitHub list APIs` section stating three
rules once, with Gate 1 pointing at it rather than restating:

1. **Paginate every list read.**
2. **Never pair a positional index with a list.**
3. **Never reduce across pages inside `--jq`.**

For endpoints that report a total, assert against it. Rule 3 is why the
naive assertion is wrong: with `--paginate`, `--jq` runs **per page**,
so it reports one page at a time —

```
$ gh api --paginate ".../check-runs?per_page=10" --jq '"total_count=\(.total_count) returned=\(.check_runs|length)"'
total_count=35 returned=10
total_count=35 returned=10
total_count=35 returned=10
total_count=35 returned=5
```

— so the published form slurps the page stream first:

```bash
gh api --paginate "repos/{owner}/{repo}/commits/<sha>/check-runs?per_page=100" \
  | jq -s -r '"total_count=\(.[0].total_count) returned=\([.[].check_runs[]] | length)"'
```

Verified against a live head at `per_page=100` (one page) and at a
forced small page size (four pages), reporting `total_count=35
returned=35` both times. `.[0].total_count` is sound because every page
repeats the same total.

`/annotations` is deliberately given its own form rather than the same
one: that endpoint returns a **bare array with no envelope and no
`total_count`** (verified), so the completeness assertion is unavailable
there, `--paginate` is the only guard, and its pages combine with `add`
rather than through a `.check_runs` wrapper.

## Call sites left unchanged, with reasons

- `plugins/kindle-dedrm/skills/manage/scripts/check-drift.sh:76` and its
documented twin `references/workflow.md:40` — `Satsuoni/DeDRM_tools` has
**20 releases total** and the newest **is** a prerelease (`v10.0.28`,
index 0), so nothing truncates today. It is also the one site where a
mechanical `--paginate` would be the wrong fix: it would walk every
release ever published to find a match that is always on page 1. The
right shape there is a bounded `per_page=100`, which is a different
decision from the one this PR makes. Follow-up.
- `plugins/discovery/skills/research/context/discipline.md:91,224` —
prose examples, and `releases/latest` is the correct single-resource
form anyway.
- `plugins/source-control/reference/review-discipline.md:108,109` — a
prose inventory of which endpoints get read, not runnable commands.
- `docs/topics/autonomy-ignition/PLAN.md:127` — a sanity-check line
inside `### Phase 1 … [DONE]`, a historical record of a completed phase
against a single-gate scratch repo, not guidance anyone would copy
today.
- `/replies` POSTs and `issues/comments/<id>` single-resource GETs
throughout — neither paginates.
-
`plugins/source-control/skills/pull-request/scripts/fetch-annotations.sh:111,157`
and
`plugins/source-control/scripts/fetch-all-pr-comments.sh:128,152,177` —
already correct.

Nothing unpaginated remains in `source-control`, `review`, or
`work-items` that is a list read.

## Known-red check — RESOLVED, kept for the record

**Current state: `changelog-parity-gate` is green.** #2159 merged, this
branch merged `main` forward, and the gate now passes on the same 267 KB
changelog that failed deterministically before. The account below is
what the red meant while it lasted; it is retained because a reviewer
reading this PR's check history will see those failures and deserves to
know they were never this diff's.

<details>
<summary>What the red was</summary>

`changelog-parity-gate` failed on this PR, and the failure was **not**
caused by this diff.

`scripts/check-changelog-parity.sh`'s `has_heading` is `rendered_lines -
| awk '…{exit}'` under `set -o pipefail`. The reader exits on the first
match — line 6, since the newest heading is at the top — while the
producer keeps writing; past the pipe buffer it takes SIGPIPE (141),
`pipefail` propagates it, and a heading present at line 6 column 1 is
reported as absent. `plugins/source-control/CHANGELOG.md` is 267 KB, the
largest in the repo.

It is a race on pipe scheduling rather than a size threshold, and it is
sensitive to the awk implementation and host. Measured locally (GNU Awk
5.4.0 under MSYS), only the largest file failed:

```
source-control  266794 B  rc=141
work-items      115774 B  rc=0
review           43944 B  rc=0
```

The runner is worse. This PR's actual `changelog-parity-gate` run failed
**all three** plugins, including the 44 KB one that passes locally:

```
UNDOCUMENTED BUMP: review went 0.18.0 -> 0.18.1 but plugins/review/CHANGELOG.md has no '## [0.18.1]' entry at head.
UNDOCUMENTED BUMP: source-control went 0.51.4 -> 0.51.5 but plugins/source-control/CHANGELOG.md has no '## [0.51.5]' entry at head.
UNDOCUMENTED BUMP: work-items went 0.35.0 -> 0.35.1 but plugins/work-items/CHANGELOG.md has no '## [0.35.1]' entry at head.
```

Every one of those three headings is present at line 6, column 1.
Independently confirmed per file: the heading list differs from `main`
by exactly one addition, with none deleted, renamed, or absorbed. The
gate's own 55 self-tests pass in the same run — no existing fixture is
large enough to cross the buffer.

`ci-status` fails only as the aggregate of that one job. Every other
check on this head is green, and all four required contexts attached:

```
pr-title / pr-title                completed success
do-not-merge / do-not-merge        completed success
ci-status                          completed failure   (aggregate of changelog-parity-gate)
security-review / security-review  completed success
```

Fixed on `main` by #2159.

</details>

**Post-merge validation of #2159, unplanned but worth recording.**
`plugins/source-control/CHANGELOG.md` is the largest changelog in the
repo and the one that failed *deterministically* rather than as a race.
After merging #2159 forward it passes all three gate modes — `--check`,
`--check-bump`, `--check-order` — against the merged tree. That is
independent evidence the fix closed the class rather than moving the
boundary.

One correction to the mechanism as first written here: the discriminator
is the **awk engine and host**, not file size. CI resolves `awk` to
gawk, which chunks its writes, so the reader's early exit strands every
later chunk; mawk cannot produce the failure at any size. That is why a
44 KB changelog failed on CI while passing locally under MSYS gawk — and
why the local byte numbers above should not be read as a threshold.

## Test plan

- Bare and corrected `check-runs` forms run against live heads
`2895890c`, `580fd090`, `435b2fef`, `193c9d2e`, `043d60ce`; dropped sets
computed by jq set difference, not read off a list.
- Published sentinel run verbatim at `per_page=100` and at a forced
small page size; multi-page behaviour of `--jq` exhibited, not asserted.
- `/annotations` response shape checked directly (`type=array`,
`has_total_count=false`); published annotations form run against a check
run carrying one annotation.
- Ordering of all three comment/review endpoints verified ascending;
`#657` and `#502` figures re-derived.
- Every replacement query run against live data with a positive match,
not just a clean exit.
- `scripts/check-changelog-parity.sh --check` and `--check-order` pass;
`--check-bump` fails for the reason above.
- `scripts/check-cross-plugin-source-drift.sh` rc=0;
`scripts/check-contract-clause-coverage.py` passes.
- `markdownlint-cli2` clean over 78 files.
- Diff is `.md` and `.json` only — no shell files touched, so no
shellcheck run applies.
- CHANGELOG heading lists diffed against `main`: exactly one heading
added per file, none deleted, renamed, or absorbed.

## Related

- #2159 — fixes the `changelog-parity-gate` SIGPIPE regression this PR's
red check is caused by.
-
`plugins/source-control/skills/pull-request/scripts/fetch-annotations.sh:111`
— the in-repo precedent every corrected call site matches.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
`check-changelog-parity.sh` policed the bump and never the preservation.
A merge-forward that writes the new release under the PREVIOUS release's
heading — absorbing it, or relabelling that heading to the new version —
deletes a released section, and all three modes passed: `--check-bump`
asks only about the bumped version, `--check-order` reads a gap in the
sequence as correctly ordered, and `--check` compares the manifest
against the changelog maximum. Git leaves no conflict marker behind for
that resolution either, so nothing caught it.

Adds a fourth mode, `--check-preserved <base-ref>`: every `## [<version>]`
heading a touched changelog carried at the FORK POINT must still be there
at head.

Three decisions:

- **Its own mode, not a `--check-bump` addition.** Preservation is a
  different concern and a wider one: the bump gate is scoped to plugins
  whose manifest version changed, which is exactly where an absorbed
  heading would NOT be looked for. The new mode sweeps every changed
  changelog under both roots `--check-order` reads, including the
  convention changelogs no manifest versions.
- **Fork point, never the base tip.** A branch that has not integrated
  main would read every heading main added after the fork as deleted — a
  false positive on a required gate. The two coincide in the scenario
  this catches, because absorbing a section requires having merged main
  forward in the first place.
- **No exemption list.** Keep a Changelog keeps a yanked release's
  heading and marks it `[YANKED]` rather than deleting it, and a note
  written against a version the manifest then SKIPPED keeps its heading
  too, annotated in place — the manifest cannot be bumped back down onto
  the skipped number, which `--check-bump` reads as a regression. Both
  removals an author might reach for therefore have a legal non-deleting
  form. The one deletion in recent history, 04822fc folding
  docs-hygiene's never-released `## [0.9.7]` into `## [0.10.0]`, is that
  second shape — and in the diff it is indistinguishable from the
  absorption this gate exists to catch, which is why the remedy is to
  annotate rather than to hand a required gate an off switch.

The failure message names relabelling explicitly, since that resolution
reads as a deletion and the author needs to recognise what happened, and
branches for the whole-file-deletion case, where "restore the heading
above the entry that replaced it" names an entry that does not exist.

Reading discipline is unchanged from the #2154/#2159 fix: no reader in
the new path exits before EOF, so no writer can take SIGPIPE under
pipefail on any awk. Existence at the fork point is probed with
`git ls-tree` rather than `git cat-file -e`, which cannot distinguish a
missing path from an unusable rev (both exit 128) and would fail every
change set that ADDS a changelog.

Twenty-one new cases, including the three-mode gap assertion on the
absorption tree, a stale-branch false-positive guard, the `[YANKED]`
form, a plugin removal, and the large-changelog SIGPIPE fixture under
forced gawk.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…line (#2327) (#2341)

## Summary

#2290's absorbed-heading check diffs whole heading **lines**:
`changelog_bracket_headings()` is `grep -E` without `-o`, so it emits
the entire line, and `comm -23` compares those lines. Any edit to an
existing release heading therefore reads as a deletion — including both
annotations [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
itself prescribes, which **preserve** the heading they annotate.

Measured against the shipped script on `main`, copied unmodified into a
throwaway repo (#2327 carries the full reproduction):

| change | heading at head | `--check-bump` on `main` |
|---|---|---|
| mark a pulled release | `## [1.0.0] - 2026-01-01 [YANKED]` |
**EXIT=1** `ABSORBED CHANGELOG HEADING … ## [1.0.0]` |
| date an existing release | `## [1.0.0] - 2026-01-01` | **EXIT=1**
`ABSORBED CHANGELOG HEADING … ## [1.0.0]` |

`1.0.0` is documented in both. Nothing was absorbed. This is a
**required** merge check, so a false positive here blocks every
in-flight PR that touches a heading line, not just its author's.

## Fix

Key the comparison on the **version** each heading names, through the
shared `changelog_versions` extractor, instead of on the rendered line.
`## [1.0.0]` and `## [1.0.0] - 2014-12-13 [YANKED]` are one release, so
annotating passes; renaming `## [0.51.8]` to `## [0.51.9]` still deletes
the version `0.51.8`, so a relabel is still caught.

`changelog_bracket_headings()` is dropped. It was a second reader of the
same file, which the header's own `rendered_lines` note argues against —
*"One tracker, not three, so the modes cannot drift."* The failure
message still prints `## [<version>]`, so nothing downstream of it
changes.

**Two behaviour changes worth naming**, both consequences of keying on
the version:

- **Reformatting a heading between the bracketed and unbracketed forms
is no longer a deletion.** The format of a release entry is `CHANGELOG
FORMAT`'s concern, not preservation's.
- **An unbracketed heading is now protected.** A bracketed-only matcher
could not see `## 0.9.0 — 2025-12-01` at all, so absorbing that section
was invisible. Both directions have a test.

## Deliberately out of scope

All pre-existing from #2290 and tracked in #2327 — a false positive on a
required gate is worth a surgical fix, not a redesign in the same change
set:

- The check lives inside `--check-bump`'s loop, so an absorption in a
change set that does **not** bump the manifest is unpoliced (verified
EXIT=0).
- `docs/conventions/*/CHANGELOG.md` is outside `--check-bump`'s scope
entirely, so an absorption there is unpoliced (verified EXIT=0).
- The `2>/dev/null` process substitution on `git show` is fail-open: a
git read that genuinely fails yields an empty fork-point set and reads
as "nothing to preserve".

#2327 sketches the fuller alternative (preservation as its own diff mode
over every changed changelog under both roots), with a worked
implementation on `fix/2264-changelog-preservation`.

## Test plan

**Suite: `PASS=62 FAIL=0`** (57 pre-existing + 5 new), `bash
scripts/check-changelog-parity.test.sh`.

New cases:

- `dating an existing heading is not an absorbed heading` — the FP
above.
- `marking a release '[YANKED]' preserves its heading and passes
--check-bump` — the FP above.
- `a relabelled predecessor heading still fails --check-bump` — the
check must not go soft.
- `reformatting a heading between the two accepted forms is not an
absorbed heading`.
- `absorbing an unbracketed release section is caught` — coverage a
bracketed-only matcher could not have.

`#2290`'s own absorption test is untouched and still passes, asserting
on `## [0.51.8]` in the output.

**The new tests are load-bearing** — established before the fix existed,
by running `main`'s shipped script unmodified against the same two
fixtures the new tests use (full transcript in #2327):

| fixture | `main`'s script | this branch |
|---|---|---|
| `## [1.0.0]` → `## [1.0.0] - 2026-01-01 [YANKED]` | **EXIT=1**
`ABSORBED CHANGELOG HEADING … ## [1.0.0]` | EXIT=0 |
| `## [1.0.0]` → `## [1.0.0] - 2026-01-01` | **EXIT=1** `ABSORBED
CHANGELOG HEADING … ## [1.0.0]` | EXIT=0 |

The symmetric in-tree check — restoring only
`scripts/check-changelog-parity.sh` from `origin/main` and re-running
the suite — was **not** run: the command was refused by a local
permission classifier, and it was not reshaped to get around it. The
table above is the same evidence by a different route.

**Zero false positives on real history.** `main` is squash-merged, so
each commit on it *is* a merged PR. Every one of the 60 most recent
commits touching `plugins/*/CHANGELOG.md` was replayed as its own PR
(tree at `C`, base `C^1`) in a throwaway local clone, running this
branch's `--check-bump`:

| | |
|---|---|
| commits replayed | **59** (of 60; `cf743d61` could not be checked out)
|
| clean | **58** |
| fired | **1** |
| of which `ABSORBED CHANGELOG HEADING` | **0** |

The single fire is `78dbb10e`, reporting `UNDOCUMENTED BUMP` and three
`VERSION REGRESSION`s — none from the check this PR touches.

**The zero is honest, and it is itself a finding.** The one commit in
recent history that really does delete a release heading — `04822fc4`,
which folded `docs-hygiene`'s never-released `## [0.9.7]` into `##
[0.10.0]` — is in the replay window (verified) and is *not* caught,
because it changed **no manifest at all** (`git diff --name-only
04822fc^1 04822fc` lists only `ci.yml`, that changelog, and the two
gate files). Living inside `--check-bump`'s loop, the check never
reaches a plugin whose version did not change. That is #2327 item 3, now
demonstrated on real history rather than a synthetic fixture. This PR
does not change that scoping — it fixes the false positives — so the
sweep's role here is to show the fix **introduces no new fires** across
59 real commits, not to demonstrate detection. Detection is demonstrated
by the unit cases.

Replaying a squash commit against its parent is not identical to what CI
saw at merge time — CI resolved the fork point of the original branch,
which for a stale branch sat further back — so fires from
`--check-bump`'s *other* checks (`VERSION REGRESSION`, `UNDOCUMENTED
BUMP`) are artifacts of comparing against a parent the branch never saw,
plus checks that did not exist when those commits merged. The
**absorbed-heading** check is unaffected: `C^1` is an ancestor of `C`,
so the fork point and the parent coincide and the comparison reads
exactly the diff that commit introduced.

**Other gates, locally:** `shellcheck --rcfile=.shellcheckrc -x` on both
files → clean; `scripts/check-shell-portability.sh --paths` on both
files → `No unexcused GNU-only constructs in 2 shell file(s)`.

**Conflict-marker sweep**, both files: `<<<<<<<` 0, `|||||||` 0,
`>>>>>>>` 0, `=======` 4 — all four pre-existing `# ==== <section> ====`
banner comments in the test file, none introduced here (0 marker-shaped
lines among this diff's additions).

## Related

Closes #2327 — the live false positives it reports. Its remaining items
(the scoping gaps and the fail-open fork-point read, all pre-existing
from #2290 and untouched here) were split into **#2342** so this close
is honest rather than partial.

#2290 introduced the check being repaired; #2264 is the defect it
closed. #2154 / #2159 are the SIGPIPE regression in this same gate — the
reading discipline is untouched here, since neither `changelog_versions`
nor `mapfile` exits before EOF.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

changelog-parity gate: has_heading's early-exit reader SIGPIPEs rendered_lines under pipefail, failing correctly documented bumps

1 participant