fix(743): make review-verdict/h10 unskippable — disable direct push to main + admin force-merge; fix(719) tag-only push #749

Merged

timothy merged 5 commits from fix/743-push-protection into main 2026-08-06 00:58:17 +02:00

Conversation 1 Commits 5 Files Changed 10

+401 -19

5 Commits

Author	SHA1	Message	Date
timothyandClaude Opus 5	f822e4737c	docs(743): label the second attested claim, close the survivor list, state the rule in CLAUDE.md ... Round 3 returned MERGEABLE with three LOW documentation findings. Batched before posting the verdict, since a new sha voids both the CI run and the verdict. - `ci-cd.md` labelled the unprobed half of the `enable_push` bullet but stated the `block_admin_merge_override` counterfactual flatly one bullet below — the same measured-vs-attested flattening round 2 fixed, one site over. Now labelled, with why it was not probed (verifying it means merging an unreviewed PR). - `release.verdict-status-check` said "what survives is the forgery list above". That record's job is enumerating survivors, so an unqualified "what survives is X" reads as exhaustive — and it omitted the admin residual, which is a SKIP route rather than a forgery one. Added. - `CLAUDE.md` never learned the rule. It is the always-read surface, and it still framed a direct `git push origin main` as a live path while describing a docs-only *push* exemption for a push the server now refuses. My corpus sweep covered `docs/` and missed the file that carries the docs-update rule. Note on what remains unverified rather than closed: neither direction of `block_admin_merge_override` was measured, and whether Gitea treats an ABSENT required context as blocking (versus satisfied) is asserted by our docs but not proven — the combined status on this PR reads `success` with `review-verdict/h10` absent. Both belong to #747's re-verification sweep. Verification: 441/441 script tests; decisions-validate OK. refs #743 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-06 00:35:41 +02:00
timothyandClaude Opus 5	6af65ba5c5	fix(743): re-tense the third stale site, and pin the two surviving mutants ... Round 2 of review. One blocking finding, and it is the same defect class as round 1's: a present-tense claim that this PR falsified. `release.verdict-status-check` — the record ABOUT the h10 status check — still said "direct pushes to `main` are server-side permitted, so the gate can be skipped without forging anything". A reader resolving that key from the catalog would conclude the control does not exist. Round 1 corrected `ci-cd.md` and `ci.actions-credential-scoping` and I stopped at the two sites I had edited, instead of sweeping the corpus by SUBJECT. Swept properly this time (`server-side permitted`, `bypassable`, `without forging`, `push whitelist`, `enable_push`): this was the only remaining stale site. Test gaps the reviewer found by mutation testing, now closed. Both mutants SURVIVED the suite as shipped — the round-1 fixes were correct but unpinned: - dropping `|| [ -n "${_h11_local_ref:-}" ]` → an unterminated final line is dropped. Two directions, and the dangerous one is not the obvious one: a dropped *branch* line leaves only tag refs and grants the exemption to a push containing a branch. Both pinned. - dropping `[ -t 0 ] ||` → the hook hangs forever on an interactive run. Pinned with a real pty and an explicit timeout, so a regression fails cleanly rather than hanging a CI job. Verified the mutant is killed by exactly that test (and that it dies via the timeout, 32s). Also from review, non-blocking: - `ci-cd.md:951` cited `enable_push: false` alone as what closed #743 — the precise thing the new record says never to do, since the force-merge route also skipped the gate with no forgery. Now cites both fields. - `ci-cd.md` flattened measured and source-attested into one 403: only the contents API was probed; the web editor/upload/apply-patch paths share the predicate but were not. Separated. - `format-as-you-touch-rebase` still said "the documented sequence" and "always" for the release-cut behind-ness. `docs/ci-cd.md` documents the tag step, not the release-notes-PR flow, and the frequency is attested by one observed cut. Attributed to #719 instead. - Documented the operator recovery path. `block_admin_merge_override: true` removes the `force_merge` escape that used to unstick a wrongly-red required context — that escape WAS the bypass, so it is gone by design, and the recovery (fix the status; last resort PATCH the field, merge, set it back) needed to be written down rather than left implicit in a residual. Verification: 441/441 script tests; decisions-validate OK; PyYAML parses all 193 records; both mutants confirmed killed and the hook restored byte-identical. fixes #719 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-06 00:10:45 +02:00
timothyandClaude Opus 5	6d80343320	fix(743): close the admin force-merge bypass; the push half alone was not enough ... Independent review found the record repeated on the merge path exactly the mistake it had just diagnosed on the push path. The push argument was: a whitelist naming `timothy` closes nothing, because `timothy` is the identity every credential already holds. The merge path had the identical shape and went unchecked — `block_admin_merge_override` defaults to `false`, so `CanBypassBranchProtection` returns true for a repo admin and `POST /pulls/{n}/merge` with `force_merge: true` merges straight past a missing or red `review-verdict/h10`. One API call, no forgery, no PATCH — cheaper than the push route this change had just removed. So `enable_push: false` alone did NOT make the gate load-bearing, which is what the record's headline sentence claimed. `main` now carries both fields; they are one control and neither is citable alone. An admin-shaped control that exempts the only admin exempts everybody. Other review findings addressed: - H11's owning record (`release.format-as-you-touch-rebase`) now documents the #719 tag-only carve-out. It is a narrowing of an existing convention, so it amends that record rather than adding a new one — including the two details that are easy to regress (the .husky/pre-push forwarding, without which the exemption is dead code the unit tests still pass over; and the at-least-one- ref guard against vacuous exemption). - The record now states which write surfaces were enumerated and how each was established — contents-API refusal is MEASURED here (403 `user cannot commit to repo`), apply-patch/revert/cherry-pick are source-attested only. The admin force-merge bypass is likewise marked source-attested, not probed: probing it means merging an unreviewed PR. - prepush-rebase-check.sh: process a final ref line with no trailing newline (previously dropped, which silently reinstated the #719 block), and skip the stdin read on a TTY so an interactive run does not hang. - Corrected a citation the review caught: docs/ci-cd.md documents the tag step, not a release-notes-PR flow. Cite #719 for the observed flow instead. Also fixed a frontmatter break this round introduced: a `: ` inside the unquoted `rule:` scalar. PyYAML rejected it while the dependency-free reader accepted it, so only `scripts/tests` caught it. Verification: 438/438 script tests pass; decisions-validate OK; PyYAML parses all three touched records. refs #743 #719 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-05 23:52:28 +02:00
timothyandClaude Opus 5	b91707b707	fix(719): exempt tag-only pushes from the H11 branch-freshness check ... H11 (.claude/hooks/prepush-rebase-check.sh) refuses to push a branch that is behind origin/main. It fired on tag-only pushes too, breaking every release cut: docs/ci-cd.md's "Cutting a release" flow lands a release-notes commit via PR and then tags that merge commit, so the local branch is always one commit behind origin/main at tag time. A tag push cannot revert anyone's merged work, which is the failure H11 exists to prevent, so skip the freshness check when every ref being pushed is under refs/tags/. .husky/pre-push previously consumed pre-push's stdin ref lines and forwarded them only to prepush-donewhen.sh; prepush-rebase-check.sh got none. Forward the captured $_prepush_refs to it too, or the new logic is dead. Guard against the vacuous-truth case explicitly required by #719: "all pushed refs are tags" is trivially true over zero ref lines (manual run, forgotten forwarding), which would silently disable H11 for every push. Require at least one parsed ref line before granting the exemption. Adds scripts/tests/test_prepush_rebase_check_tag_exemption.py using real local git repos (bare origin + a work tree pushed one commit behind it) to exercise git fetch/merge-base/rev-list against a genuinely-moved origin: tag-only allowed, branch-only still blocked, mixed branch+tag still blocked, and zero ref lines still blocked (the vacuous-truth guard). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-05 23:33:10 +02:00
timothyandClaude Opus 5	691a7acc77	fix(743): disable direct pushes to main so review-verdict/h10 is on the only path to main ... Gitea evaluates `status_check_contexts` when it MERGES a PR. A direct `git push origin HEAD:main` never consults them, so the whole h10 gate was skippable with no forgery — strictly cheaper than every route enumerated in #697. `main` now carries `enable_push: false`. Measured on this instance (Gitea 1.27.1) against a throwaway `probe-743-*` rule rather than against `main`: enable_push: false -> push by timothy (site admin) REFUSED, pre-receive hook declined enable_push_whitelist + ["timothy"] -> identical push SUCCEEDED That second line is why this is a DISABLE and not a whitelist: #743 offered the two as interchangeable, but the only write accounts here are `timothy` (site admin) and `renovate`, and every credential in the threat model — agent sessions, PATs, the injected GITEA_TOKEN — acts as `timothy`. A whitelist naming `timothy` would have ticked the box and closed nothing. Then demonstrated on `main` itself, per the issue's Done-when: a direct push was refused, and a tag-only push from the same worktree succeeded (tags are governed by `tag_protections`, which is empty). The release cut is unaffected. What this closes: the write-only credential routes — the injected GITEA_TOKEN, RENOVATE_TOKEN, any non-admin collaborator PAT. What it does NOT close: an admin credential can PATCH the protection off, push, and restore it. Recorded as an accepted residual rather than implied to be covered. Also corrects two claims the probe contradicted, and one that the mid-session Gitea upgrade (1.25.4 -> 1.27.1) invalidated: - ci-cd.md and ci.actions-credential-scoping both said "a push whitelist would close more of this class than the 1.26 upgrade". The whitelist form closes nothing here; corrected in place. - ci.actions-credential-scoping's rule said "do NOT add a `permissions:` key while this instance is below Gitea 1.26.0". That precondition no longer holds at 1.27.1, so the directive now misleads. Corrected — while noting the consequence is still UNVERIFIED: `/api/v1/settings/actions` 404s at 1.27.1, so whether `permissions:` binds here was not probed. The upgrade alone is not evidence the constraint works. - That record's 1.25.4 measurements are now dated, not current. Flagged as such rather than silently re-pinned to a version they were never taken on. #743's fourth box (docker-build.yml `persist-credentials: false`) is decided in the record and deliberately not done here: two of its checkout steps run `git fetch ... || true` feeding the changed-file skip logic, so a credential regression would be silent rather than loud. Drop the `|| true` masking first. fixes #743 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-05 23:31:45 +02:00