timothy
5e7623b8d5
Two real defects, and three docs claims that were simply wrong. jq-preflight.sh parsed the version by stripping around the first `-` and `.`, which assumed the format is exactly `jq-X.Y`. A build printing `jq version 1.6` left major empty; the sanity check concatenated major+minor into "6", which is non-empty and all-digits, so it PASSED. The floor comparison then ran `[ "" -lt 1 ]`, which errors — and `set -e` exempts a failing command in an `if` condition, so the conditional read false and the script exited 0 having asserted nothing, after printing a plausible "parsed" line. The silently-untested-axis failure this script exists to eliminate, reproduced inside the script itself. Now parsed by explicit regex, failing closed with a diagnosis when there is no <digits>.<digits> match. Also: `--expect` with no value exited 1 with empty output on both streams. The hook's exit-status check was pinned by nothing: mutating `if files=$(...)` into `files=$(...) || true; files_complete=yes` left the ENTIRE suite green. It survived only by redundancy — the script writes stdout once, right before exit 0, so failures also happen to yield empty stdout and `[ -n "$files" ]` catches it. Safe by accident, which is the exact criticism this branch levels at the old code. Four tests now pin it, with a stub that FAILS while emitting a docs-only list (the one case redundancy cannot absorb) plus a positive control proving the harness can see the difference. Verified: the mutation now turns exactly those tests red. Docs corrections. The record claimed the --expect pin was safe because script-tests is "advisory, not a required check" — false. The merge-consent hook reads the COMBINED status (ci.advisory-red-blocks-the-merge-gate, #598), so firing the tripwire blocks every non-docs-only merge until someone re-pins. Kept anyway, for a stated reason, but no longer described as free. The record also asserted in the present tense that review-verdict.yml checks out the base ref; it has no checkout step at all, so that is now a future-tense requirement on the follow-up. And the documented .status allow-list named GitHub's `removed`, which the code rejects. The drift-guard regex anchored on `?limit=`, so a re-inlined copy written `files?page=1&limit=50` would have walked past it. Decisions-Edit: yes
7.7 KiB
key, title, status, since, supersedes, superseded-by, rule, signals, mechanics
| key | title | status | since | supersedes | superseded-by | rule | signals | mechanics |
|---|---|---|---|---|---|---|---|---|
| ci.shared-pr-file-enumeration | 2026-07-26 — `scripts/pr-changed-files.sh` is the ONE enumeration of a PR's changed files; the advisory hook and the enforced gate share mechanism, never policy (#649) | active | 2026-07-26 | none | none | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | duplicated PR file enumeration, enforced gate weaker than advisory hook, docs-only allow-list drift, shared mechanism not shared policy, pr-changed-files.sh, checkout base ref not PR head, gate judging its own PR, exhaustiveness bug in a security predicate · paths: `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml` · issues: #643, #648, #649 | `scripts/pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha>` -> stdout newline-delimited paths, exit 0 only if complete and bound to the given sha; callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml` |
Landing note. This record describes the end state. It is delivered over two PRs: the shared script, its tests and the hook rewiring first; the
review-verdict.ymlrewiring second, once the script exists onmain. The reason is the base-ref checkout below — a workflow that runs the base version of the gate cannot call a script the base does not yet have.
Before #649, the PR changed-file enumeration existed as two independent implementations. That would
be an ordinary duplication smell anywhere else; here it was actively dangerous, because the two
copies had unequal consequence. The advisory hook's failure mode is a human permission prompt — a
missed guard there just means a person gets asked instead of an automatic decision. The enforced
workflow's failure mode is a success write to review-verdict/h10, the one status branch
protection actually requires — a missed guard there merges an unreviewed PR with nobody asked at all.
The drift that motivated this. Four rounds of #643 hardening landed entirely on the copy with the
lower stakes. The hook accumulated CR/LF rejection, .. rejection, a closed .status allow-list,
previous_filename validation on every row (not just renamed), termination only on a validated
empty page, and head-sha binding — while the enforced workflow kept the original, weaker logic. Its
fail-closed behavior on a garbage API response was incidental (an empty n erroring a bash
conditional to false), not a designed property. The gate with real authority was strictly weaker than
the gate with none, which is the wrong way around by construction, not by anyone's mistake in a
single review — nothing in the original layout forced the two to move together.
Why the fix is "one script, two callers" rather than "copy the hardening across." Copying keeps
the two-implementation shape; the next hardening round would only need to happen twice again, and
there is no mechanism that would surface a second drift before it mattered. Extracting
scripts/pr-changed-files.sh makes the enumeration a single artifact with a single test suite
(scripts/tests/test_pr_changed_files.py), so a future guard is added once and both callers get it
atomically.
Mechanism, not policy — the two allow-lists stay separate on purpose. The extracted script answers exactly one question: "what is the complete set of paths this PR touches, at one head, or can we not tell?" It does not decide whether that set makes the PR docs-only. Each caller keeps its own classification:
- The hook's docs-only pattern also lets
.claude/,.gitea/,.husky/through, which is safe there only because a non-match falls through to a human prompt rather than an auto-grant. - The workflow's is narrower, because there a match posts a green status with nobody in the loop, and
both docs-only and Renovate exemptions are void when the PR touches
.claude/,.gitea/,.husky/,scripts/ordocker/ci/— the gate must not be able to exempt itself from review by editing itself.
Merging the two allow-lists would have quietly widened the enforced exemption to match the advisory one, turning a difference that exists for a reason into an accident of refactoring. Sharing the enumeration closes the drift that actually caused harm without touching the part that was correctly different.
What the shared script owns. Six guards, all now exercised by one test suite instead of a subset in each caller:
- CR/LF rejection and
..rejection on every path. - A closed
.statusallow-list —added/deleted/changed/modified/renamed/copied, not an open denylist. Notechangedanddeletedare the values live Gitea 1.25.4 actually emits;modifiedis accepted alongsidechangedbecause a closed list built from the wrong vocabulary would gate every genuine docs-only PR. GitHub'sremovedis deliberately not in the list — an earlier draft of this record said it was, which would have sent a maintainer looking for a value the code rejects. previous_filenamevalidated on every row the extraction consumes, not only rows whose.statusisrenamed— amodified/copiedrow can still carry it, and an earlier fix that validated only therenamedcase was found incomplete for exactly this reason (seeci.script-tests-jobfor the review trail).- Termination only on a validated empty page — Gitea's paging can return fewer rows than requested well before the real end of the list, so "short page" is not a valid termination signal.
- Head-sha binding: the head is re-read after enumeration, and the caller must refuse to trust the list if it moved mid-enumeration, since paging is several round-trips and a force-push between them would otherwise yield a list belonging to no single commit.
Base-ref checkout — a REQUIREMENT on the follow-up, not yet a property of the tree. When
review-verdict.yml is wired to this script it MUST check out the PR's BASE ref
(ref: ${{ github.event.pull_request.base.sha }}, plus persist-credentials: false), never the
head, so the workflow that judges a PR cannot be rewritten by that same PR to weaken its own
judgment.
Stated in the future tense deliberately. Today review-verdict.yml has no actions/checkout step
at all — it is a pure inline run: — so nothing about base-vs-head holds of it right now, and the
PROTECTED path list is the only thing standing between a PR and the gate. An earlier draft asserted
this in the present tense; a reader resolving this record from the catalog would have concluded the
self-rewrite hole was already closed. Adding a bare run: line in the follow-up is not
sufficient — the checkout and its ref are the security-relevant parts.
Severity, stated honestly. Every enumeration defect found in this area (#643) downgraded a mechanical deny/ask to a human prompt on the hook side; none produced a silent self-merge on their own. It is still a real weakening worth fixing — the whole point of #649 is that the same class of bug on the enforced copy would not have been merely a downgrade.