From 691a7acc777313c724ff1927cedef763e3271286 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 5 Aug 2026 23:31:45 +0200 Subject: [PATCH 1/5] fix(743): disable direct pushes to main so review-verdict/h10 is on the only path to main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/ci-cd.md | 23 ++++++- docs/decisions/README.md | 3 +- .../records/ci/actions-credential-scoping.md | 25 ++++++-- .../release/main-direct-push-disabled.md | 61 +++++++++++++++++++ 4 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 docs/decisions/records/release/main-direct-push-disabled.md diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 8479c8e39..b7e49e0de 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -773,6 +773,17 @@ from `Build ErsatzTV Image / …` to `PR Gates / …`) does not affect merges. T unreviewed commit from merging** (ersatztv#622). It is not produced by a job's success/failure; it is a commit status that `scripts/post-review-verdict.sh` POSTs onto one specific sha. +**`main` is PR-only, and that is what makes the check load-bearing** (ersatztv#743, +`release.main-direct-push-disabled`). Gitea evaluates `status_check_contexts` when it **merges a +PR** — a direct `git push origin HEAD:main` never consults them. So until 2026-08-05 the entire gate +was skippable with no forgery at all, which was cheaper than every route enumerated in #697. `main` +now carries `enable_push: false`: a direct push is refused server-side at pre-receive +(`Not allowed to push to protected branch main`), for every account including a site admin. +Practical consequences: **every** change to `main` goes through a PR, including a one-line docs fix; +and the client-side Husky guards (H6/H11/H13) remain useful friction but were never the control — +they are fail-open and `--no-verify` bypasses them. Tag pushes are unaffected (separate mechanism; +`tag_protections` is empty), so the release cut in "Cutting a release" still works unchanged. + **The hole it closes.** `pretooluse-merge-consent.sh` proves its three consent conditions at the moment the merge tool is called. Pass `merge_when_checks_succeed=true` and Gitea performs the merge *later*, against whatever head is green then — while the Done-when and review-verdict checks were @@ -927,9 +938,15 @@ as establishing that the gate cannot be forged (see the residual below, and ersa workflow, though: Gitea injects a write-capable `GITEA_TOKEN` into every job and branch protection binds the *context*, not its issuer. Gitea >=1.26 with the Actions default set to **Restricted** (server-management#714) binds the injected token, but does not close the class either — not against - a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). **And none of it is necessary: - direct pushes to `main` are server-side permitted, so the gate can be skipped without any forgery - (ersatztv#743) — a push whitelist would close more of this class than the upgrade does.** The + a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). **And none of it was necessary: + direct pushes to `main` were server-side permitted, so the gate could be skipped without any forgery + (ersatztv#743). That is now CLOSED — `main` carries `enable_push: false`, so it is reachable only + through the PR merge path, the one path on which Gitea evaluates `status_check_contexts` + (`release.main-direct-push-disabled`).** Note the fix is *disabling* push, not whitelisting it: a + push whitelist naming `timothy` was measured to still admit the push, and `timothy` is the identity + every session, PAT and injected `GITEA_TOKEN` already acts as, so the whitelist form would have + closed nothing. The block binds a site admin at pre-receive but not a credential that can first + PATCH branch protection off — an accepted residual, recorded in that decision. The exemption path has separate defects of its own (ersatztv#698). One operational consequence of the trigger change: a PR whose base is not `main` now gets **no** `review-verdict/h10` at all. That is fail-closed. `edited` **is** now among the trigger's `types` diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 029ca7310..a7231ae23 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -36,7 +36,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | | `channel.origin-marker` | A new `Channel.Origin` (`ChannelOrigin` enum — `Unknown`/`UserCreated`/`AutoTuned`) records how a channel row was created and is stamped exactly once at insert (`AutoTuned` in `CreateChannelFromLineupHandler`, `UserCreated` in `CreateChannelHandler`), and is never mutated on a later edit. It is surfaced as a raw `origin` field on `ChannelResponseModel`; the SPA badges only `AutoTuned`. Rows predating the column read `Unknown` — provenance is **not** back-filled. | 2026-07-23 | [link](records/channel/origin-marker.md) | -| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. Do NOT add a `permissions:` key to constrain the injected `GITEA_TOKEN` while this instance is below Gitea 1.26.0 — it is silently a NO-OP there, which is worse than absent because it reads in review as a constraint. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). This record closes ONE route. It does not close the class, and four later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | +| `ci.actions-credential-scoping` | Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. Do NOT add a `permissions:` key to constrain the injected `GITEA_TOKEN` on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP, which is worse than absent because it reads in review as a constraint. That version precondition NO LONGER HOLDS: this instance was upgraded 1.25.4 -> 1.27.1 on 2026-08-05. What has NOT changed is that the consequence is unverified — whether `permissions:` is honored here, and what this instance's default Actions token permission is, were both left UNPROBED (there is still no API surface: `/api/v1/settings/actions` 404s at 1.27.1). Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). This record closes ONE route. It does not close the class, and four later sections say exactly what survives — read them before citing this record as a mitigation. | 2026-08-05 | [link](records/ci/actions-credential-scoping.md) | | `ci.batch-pushes-no-cancel-route` | Hold review fixes, doc corrections and format fixes locally and push **once** — a superseded run cannot be cancelled from the agent side and holds a runner slot until it finishes. | 2026-07-21 | [link](records/ci/batch-pushes-no-cancel-route.md) | | `ci.build-once-rejected` | CI build-once (a shared compile artifact across jobs) was implemented, measured, and rejected for a 40-85% wall-clock regression; keep the #420 cross-run tree-identity skip instead. | 2026-07-18 | [link](records/ci/build-once-rejected.md) | | `ci.cancelled-is-not-a-verdict` | Treat a `cancelled` conclusion as "no verdict" — never as pass or fail — and report FAILED and CANCELLED counts separately in any CI monitor. | 2026-07-21 | [link](records/ci/cancelled-is-not-a-verdict.md) | @@ -123,6 +123,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `release.done-when-merge-consent` | A PR may merge only when its linked issue's `## Done-when` checklist is fully ticked and the PR's CI is green, enforced by a PreToolUse hook on the Gitea merge tool (deny/allow/ask) plus a pre-push backstop for direct pushes to main. | 2026-07-12 | [link](records/release/done-when-merge-consent.md) | | `release.format-as-you-touch-rebase` | A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR's changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. | 2026-07-12 | [link](records/release/format-as-you-touch-rebase.md) | | `release.live-e2e-required` | A PR that changes an API write-path handler must include a live-E2E pass (driving the real endpoint/screen and confirming the round-trip through a subsequent read), not only unit/characterization tests, and must state whether live-E2E ran or wasn't required. | 2026-07-12 | [link](records/release/live-e2e-required.md) | +| `release.main-direct-push-disabled` | Branch protection on `main` carries `enable_push: false`, so `main` is reachable ONLY through the PR merge path — the one path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. This is what makes merge-consent derived rather than merely conventional: before it, `git push origin HEAD:main` skipped the gate with no forgery, which was strictly cheaper than every route enumerated in #697. Do NOT "soften" this to a push WHITELIST: measured on this instance, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as — so the whitelist form closes nothing while reading in review as a control. The block binds a site admin at pre-receive; it does NOT bind a credential that can first PATCH branch protection, which an admin token can (see the residual below). Tag pushes are unaffected — a separate mechanism (`tag_protections`) governs them, so the release cut still works. | 2026-08-05 | [link](records/release/main-direct-push-disabled.md) | | `release.merge-consent-autogrant` | When Done-when boxes are ticked, CI is green, and a fresh positive Review-verdict references head, the merge-consent hook emits `permissionDecision: allow` to actually suppress the redundant mechanical prompt — the derived state IS the consent, no separate conversational confirmation on that path. | 2026-07-12 | [link](records/release/merge-consent-autogrant.md) | | `release.migration-rehearsal-prodcopy` | Before promoting a migration-bearing release, rehearse the new image's migrations against a throwaway copy of the latest prod backup (`scripts/migration-smoke.sh`), gating PASS on the migrator's completion log line rather than HTTP readiness alone. | 2026-07-12 | [link](records/release/migration-rehearsal-prodcopy.md) | | `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 6b0b296bf..d3335546c 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -5,7 +5,7 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. Do NOT add a `permissions:` key to constrain the injected `GITEA_TOKEN` while this instance is below Gitea 1.26.0 — it is silently a NO-OP there, which is worse than absent because it reads in review as a constraint. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). This record closes ONE route. It does not close the class, and four later sections say exactly what survives — read them before citing this record as a mitigation.' +rule: 'Any credential reachable from an Actions job is scoped to what that job needs. The container-registry secret `REGISTRY_PASSWORD` is a personal access token scoped `write:package` + `read:repository` — never an account PASSWORD. This matters because Gitea has NO `status` token scope: `POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so ANY credential that can write the repository can forge `review-verdict/h10`, the required context that is supposed to make merge-consent derived rather than assertable. Package-write IS a separate scope, so the registry credential can be made status-incapable at no cost: `scripts/ci-detect-already-validated.sh` only GETs. Do NOT add a `permissions:` key to constrain the injected `GITEA_TOKEN` on the assumption that it binds — below Gitea 1.26.0 it is silently a NO-OP, which is worse than absent because it reads in review as a constraint. That version precondition NO LONGER HOLDS: this instance was upgraded 1.25.4 -> 1.27.1 on 2026-08-05. What has NOT changed is that the consequence is unverified — whether `permissions:` is honored here, and what this instance''s default Actions token permission is, were both left UNPROBED (there is still no API surface: `/api/v1/settings/actions` 404s at 1.27.1). Probe before relying on it; do not read the upgrade alone as the constraint now working. Scoping is necessary and not sufficient: it bounds what a job may DO, never whether attacker YAML runs at all, so a self-referencing trigger needs its own filter (`ci-image.yml`, tracked in #744 — deliberately NOT bundled here, because editing that file re-points `ci-image-pin` at the editing commit and reddens a blocking job). This record closes ONE route. It does not close the class, and four later sections say exactly what survives — read them before citing this record as a mitigation.' signals: 'admin password in CI secrets, registry credential scope, ETV_STATUS_AUTH can write statuses, forge review-verdict/h10, head-resolved workflow holds credentials, Gitea token scopes, no status scope, write:package vs write:repository, permissions key no-op, GITEA_TOKEN default read/write, Restricted default token permissions, orphan secret, deploy key in secret store, toolchain image overwrite, prod floating tag write · paths: `.gitea/workflows/docker-build.yml`, `.gitea/workflows/ci-image.yml`, `.gitea/workflows/renovate.yml`, `scripts/ci-detect-already-validated.sh` · issues: #697, #672, #698, #742, #743, #420, server-management#714' mechanics: 'PAT `ci-registry-scoped-697`, scopes `write:package,read:repository`, stored as repo Actions secret `REGISTRY_PASSWORD`; `REGISTRY_USER` remains `timothy`. Verified 2026-08-05 on Gitea 1.25.4: registry push of a probe tag SUCCEEDED; `GET /commits/{sha}/status` 200; `POST /statuses/{sha}` REFUSED HTTP 403 `token does not have at least one of required scope(s), required=[write:repository], token scope=write:package,read:repository`. Probe artifacts deleted, confirmed 404. NOT measured with this token: the `container:` pull, the buildcache write and the base-image pull. Those rest on Gitea''s scope model (write implies read per category, read at tag `v1.25.4`) — INFERRED. Note WHICH run proves which: only the `container:` pull is exercised by a PR. `cache-to`/`cache-from` and the base-image pull are confined to the `build` job, which carries `if: github.event_name != ''pull_request''`, so they are first exercised on the post-merge push to `main` — AFTER the merge gate has passed. A wrong inference there reddens main, not the PR.' --- @@ -59,15 +59,30 @@ and jobs reach the runner's docker daemon. docs, NOT verified (2026-08-05): `permissions:` landed in 1.26.0 (Gitea PR #36173); no `app.ini` lever at any version; Gitea rejects GitHub's `statuses`/`checks` scopes. +**Version caveat — this record's measurements are pinned to 1.25.4, the instance is now 1.27.1.** +The instance was upgraded mid-session on 2026-08-05 (#743). Everything above measured on 1.25.4 is +therefore a *dated* claim, not a current one: the scope enum, the `reqRepoWriter` gate and the 403 +probe were all taken pre-upgrade and have NOT been re-run. They are recorded honestly as of their +date and are the best evidence available, but do not cite them as current behaviour without +re-probing. Re-verification of the 1.25.4-pinned claims across the CI docs is tracked separately. + **Surviving routes — this record is not a mitigation for any of them.** `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same store, posting with a real creator, and cannot be scoped down because Renovate needs repo write (#742). The injected `GITEA_TOKEN` is write-capable in every job; -only Gitea >=1.26 with the Actions default set to **Restricted** binds it (server-management#714). A +only Gitea >=1.26 with the Actions default set to **Restricted** binds it (server-management#714) — +the version half of that condition is now satisfied (1.27.1) but the *default* half is unverified, so +treat this route as still open until probed. A collaborator's own token always can. `docker-build.yml` publishes `:prod` from a `v*` tag push and a tag may point at ANY commit — a prod image with no PR, review or status (tag protections are empty). -**And none of it is necessary: direct pushes to `main` are server-side permitted, so the gate is -bypassable with no forgery at all (#743)** — a push whitelist closes more of this class than the 1.26 -upgrade. Treat this list as "at least these", never exhaustive. `SERVERMGMT_DEPLOY_KEY` remains in the +**And none of it was necessary: direct pushes to `main` were server-side permitted, so the gate was +bypassable with no forgery at all (#743).** That route is now closed — `main` carries +`enable_push: false` (`release.main-direct-push-disabled`), which removes `main` as a destination for +every write-only credential in this list, including the injected `GITEA_TOKEN` and `RENOVATE_TOKEN`. +It does not remove them as *forgery* routes on the PR path, and it does not bind an admin credential, +which can PATCH the protection off first. Correction to this record's earlier wording: a push +*whitelist* would NOT have closed more of the class than the upgrade — measured 2026-08-05, a +whitelist naming `timothy` still admitted the push, and every credential here acts as `timothy`. +Treat this list as "at least these", never exhaustive. `SERVERMGMT_DEPLOY_KEY` remains in the store though its `bump-prod-compose` job went in `1b5efd7b9`, and its key on `timothy/server-management` is `read_only: false` — write access to the repo holding prod's GitOps stack definitions. Left in place by explicit decision 2026-08-05; recorded so it is accepted, not forgotten. Severity throughout: push diff --git a/docs/decisions/records/release/main-direct-push-disabled.md b/docs/decisions/records/release/main-direct-push-disabled.md new file mode 100644 index 000000000..b68f7ce50 --- /dev/null +++ b/docs/decisions/records/release/main-direct-push-disabled.md @@ -0,0 +1,61 @@ +--- +key: release.main-direct-push-disabled +title: '2026-08-05 — `main` refuses direct pushes (`enable_push: false`), because a push whitelist would have been a no-op here (#743)' +status: active +since: '2026-08-05' +supersedes: none +superseded-by: none +rule: 'Branch protection on `main` carries `enable_push: false`, so `main` is reachable ONLY through the PR merge path — the one path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. This is what makes merge-consent derived rather than merely conventional: before it, `git push origin HEAD:main` skipped the gate with no forgery, which was strictly cheaper than every route enumerated in #697. Do NOT "soften" this to a push WHITELIST: measured on this instance, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as — so the whitelist form closes nothing while reading in review as a control. The block binds a site admin at pre-receive; it does NOT bind a credential that can first PATCH branch protection, which an admin token can (see the residual below). Tag pushes are unaffected — a separate mechanism (`tag_protections`) governs them, so the release cut still works.' +signals: 'direct push to main, push whitelist, enable_push false, branch protection bypass, review-verdict/h10 bypassable without forging, merge consent derived not asserted, pre-receive hook declined, Not allowed to push to protected branch, protected branch, tag_protections, release tag push, GITEA_TOKEN repo write, RENOVATE_TOKEN, site admin bypass, PR-only flow · paths: `docs/ci-cd.md` · issues: #743, #697, #698, #622, #672, #706, #742, server-management#714' +mechanics: 'Gitea 1.27.1. `PATCH /api/v1/repos/timothy/ersatztv/branch_protections/main` with `{"enable_push": false}`; whitelist fields left off (`enable_push_whitelist: false`, empty arrays). Verified 2026-08-05 against a throwaway `probe-743-*` rule rather than against `main`: with `enable_push: false` a push by `timothy` (site admin) was REFUSED — `pre-receive hook declined`, `Not allowed to push to protected branch`; after PATCHing the same rule to `enable_push: true` + `enable_push_whitelist: true` + `push_whitelist_usernames: ["timothy"]` the identical push SUCCEEDED. Then on `main` itself: `git push origin HEAD:main` REFUSED, and a tag-only push SUCCEEDED in the same worktree. `GET .../tag_protections` returns `[]`. All probe artifacts (rule, branch, tag) deleted and confirmed gone; `origin/main` head unchanged at `08e95f9ec` throughout.' +--- + +**Why a whitelist was the wrong shape.** #743 proposed "a push whitelist on `main` (or disable direct +push entirely)" as if the two were interchangeable. They are not, and which one is right depends on a +fact about *this* instance: the only accounts with repository write are `timothy` (a site admin) and +`renovate`. Every credential in the threat model — an agent session, a collaborator PAT, the +`GITEA_TOKEN` Gitea injects into every Actions job — authenticates as one of those two, and +overwhelmingly as `timothy`. A whitelist admitting `timothy` therefore admits precisely the identity +the control is supposed to constrain. It would have ticked the issue's box while changing nothing. +This was measured, not reasoned: the same push was refused under `enable_push: false` and accepted +under a whitelist naming `timothy`. + +**What this actually closes, and what it does not.** It closes the *write-only* credential routes, +which is most of #743's own "who can do it" list: the injected `GITEA_TOKEN` (repo write, not admin), +`RENOVATE_TOKEN`, and any non-admin collaborator PAT. Those can no longer reach `main` at all, by any +path that skips the gate. + +It does **not** close the admin route. `timothy` is a site admin, so a credential holding that +identity can `PATCH` the branch protection off, push, and restore it — the exact sequence used to +*prove* the semantics above. Closing that requires agent sessions to run as a scoped non-admin +credential, which is a different change with its own cost (packages live in a user namespace; see the +"Admin ownership is a real residual" section of `ci.actions-credential-scoping`). Recorded as an +accepted residual rather than fixed here, so it is not mistaken for covered. The severity bound from +#697 and #743 is unchanged throughout: push access is required, so this is a compromised contributor +or a subverted automated session, never an anonymous attacker. + +**Why the release cut does not deadlock.** #743 flagged that the tag path had to keep working, and +#719 documents H11 blocking a tag-only push on every release cut. Branch protection is scoped to +`refs/heads/main`; tags are governed by an entirely separate mechanism, and `tag_protections` on this +repo is empty, so tag pushes are unrestricted by anything except ordinary write permission. Demonstrated +rather than assumed: from one worktree, the branch push to `main` was refused and a tag push succeeded. +Do not conflate the two mechanisms — disabling branch push says nothing about tags, and a future +tag-protection rule would not inherit from this one. + +**The `docker-build.yml` `persist-credentials` question (#743's fourth box), decided and deferred.** +Its six `actions/checkout` steps omit `persist-credentials: false`, so a head-resolved job keeps a +write-capable credential in `.git/config`. It *should* be set — but not blind, and not in this PR, +because two steps run `git fetch --no-tags --depth=100 origin "$base_ref" || true` and feed the result +into the changed-file skip logic. That `|| true` means a credential regression does not fail the job; +it silently yields an empty changed-file set, and the skip logic then reads "nothing changed". The repo +is public, so anonymous fetch is *expected* to cover it — expected is not measured, and the failure +mode is silent, which is the shape that has burned this repo before. The correct order is: drop the +`|| true` masking so a fetch failure is loud, then set `persist-credentials: false` and confirm both +jobs still compute a non-empty changed set on a PR that genuinely changes files. + +**Why this is not redundant with the Husky pre-push hooks.** `.husky/pre-push` guards (H6 done-when, +H11 rebase, H13 clean worktree) are client-side and deliberately fail-open — a git hook cannot prompt. +They are not installed in CI, not present in a fresh clone until `husky` runs, and `--no-verify` +bypasses them, which the worktree workflow uses routinely. They are good friction against mistakes and +were never a control against a credential. This record is the server-side half; the hooks remain useful +and unchanged. -- 2.47.3 From b91707b707d7d7fd6bc9e84aa3c3a83755381ff3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 5 Aug 2026 23:32:00 +0200 Subject: [PATCH 2/5] 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) --- .claude/hooks/prepush-rebase-check.sh | 26 ++++ .husky/pre-push | 5 +- ...test_prepush_rebase_check_tag_exemption.py | 131 ++++++++++++++++++ 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 scripts/tests/test_prepush_rebase_check_tag_exemption.py diff --git a/.claude/hooks/prepush-rebase-check.sh b/.claude/hooks/prepush-rebase-check.sh index 408589296..3a65bdc95 100755 --- a/.claude/hooks/prepush-rebase-check.sh +++ b/.claude/hooks/prepush-rebase-check.sh @@ -12,6 +12,32 @@ set -uo pipefail [ "${ETV_SKIP_REBASE_CHECK:-}" = "1" ] && exit 0 git rev-parse --git-dir >/dev/null 2>&1 || exit 0 +# Tag-only push exemption (ersatztv#719): a release cut tags the merge commit of a just-merged +# release-notes PR (docs/ci-cd.md -> "Cutting a release"), so the local branch is ALWAYS 1 commit +# behind origin/main at tag time -- H11 blocked every release. A tag push cannot revert anyone's +# merged work, which is the failure mode H11 exists to prevent, so skip the freshness check when +# EVERY ref being pushed is under refs/tags/. +# +# Read pushed refs from stdin: git feeds pre-push hooks one line per ref, " +# " (.husky/pre-push forwards the lines it already captured). Ignore blank +# lines. VACUOUS-TRUTH GUARD: "all refs are tags" is trivially true when there are zero ref lines +# (hook run manually, stdin not forwarded, etc.) -- that would silently disable H11 for every push. +# Require at least one parsed ref line before granting the exemption; with zero lines, fall through +# to the existing branch-freshness check below (current behavior preserved). +_h11_refs_seen=0 +_h11_all_tags=1 +while IFS=' ' read -r _h11_local_ref _h11_local_sha _h11_remote_ref _h11_remote_sha; do + [ -z "${_h11_local_ref:-}" ] && continue + _h11_refs_seen=1 + case "${_h11_remote_ref:-}" in + refs/tags/*) ;; + *) _h11_all_tags=0 ;; + esac +done +if [ "$_h11_refs_seen" = "1" ] && [ "$_h11_all_tags" = "1" ]; then + exit 0 +fi + # Best-effort fetch of the latest main; offline / no network -> don't block. git fetch origin main --quiet 2>/dev/null || exit 0 git rev-parse --verify --quiet origin/main >/dev/null 2>&1 || exit 0 diff --git a/.husky/pre-push b/.husky/pre-push index 345e96343..5b9c1dc02 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -12,8 +12,9 @@ unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE # H11 (ersatztv#311): refuse to push a branch that is BEHIND origin/main — rebase, don't merge # main in (a merge drags in files you never touched, e.g. legacy-BOM .cs, and trips the format -# hook on code that isn't yours). Fail-open; escape with ETV_SKIP_REBASE_CHECK=1. -./.claude/hooks/prepush-rebase-check.sh || exit 1 +# hook on code that isn't yours). Fail-open; escape with ETV_SKIP_REBASE_CHECK=1. Exempts a +# tag-only push (ersatztv#719) — forward the ref lines captured above so it can tell. +printf '%s\n' "$_prepush_refs" | ./.claude/hooks/prepush-rebase-check.sh || exit 1 # H13 (ersatztv#416 session): refuse to push when a file in the pushed diff still has uncommitted # working-tree/index changes — the pushed commit wouldn't match what you built/reviewed (the #416 diff --git a/scripts/tests/test_prepush_rebase_check_tag_exemption.py b/scripts/tests/test_prepush_rebase_check_tag_exemption.py new file mode 100644 index 000000000..379e3c540 --- /dev/null +++ b/scripts/tests/test_prepush_rebase_check_tag_exemption.py @@ -0,0 +1,131 @@ +"""Tests for the tag-only-push exemption in `.claude/hooks/prepush-rebase-check.sh` (ersatztv#719). + +H11 refuses to push a branch that is behind `origin/main`, to force a rebase instead of a merge. +But the documented release flow (`docs/ci-cd.md` -> "Cutting a release") tags the merge commit of a +just-merged release-notes PR, so the local branch is *always* one commit behind `origin/main` at tag +time -- H11 blocked every release cut. A tag push cannot revert anyone's merged work (the failure +mode H11 exists to prevent), so the fix skips the freshness check when EVERY ref being pushed is +under `refs/tags/`. + +These tests use real local git repositories (a bare "origin" plus a work tree pushed one commit +behind it) rather than stubbing `git`, because the hook's decision hinges on genuine +`git fetch` / `merge-base` / `rev-list` behavior against an origin that has moved. + +`test_zero_ref_lines_does_not_exempt` is the load-bearing negative case from the issue: "all pushed +refs are tags" is vacuously true over zero ref lines, so a naive implementation would disable H11 +entirely whenever stdin is empty (hook run manually, or a caller that forgot to forward it). The fix +must require at least one parsed ref line before granting the exemption. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +HOOK = REPO_ROOT / ".claude" / "hooks" / "prepush-rebase-check.sh" + +DUMMY_SHA_A = "a" * 40 +DUMMY_SHA_B = "b" * 40 + + +def _git(args, cwd): + r = subprocess.run(["git", *args], cwd=str(cwd), capture_output=True, text=True) + assert r.returncode == 0, f"git {' '.join(args)} failed: {r.stderr}" + return r.stdout + + +@pytest.fixture +def behind_repo(tmp_path): + """A work tree whose local `main` is exactly one commit behind `origin/main`.""" + origin = tmp_path / "origin.git" + _git(["init", "--bare", "-q", str(origin)], cwd=tmp_path) + + work = tmp_path / "work" + _git(["init", "-q", "-b", "main", str(work)], cwd=tmp_path) + _git(["config", "user.email", "test@example.com"], cwd=work) + _git(["config", "user.name", "Test"], cwd=work) + (work / "f.txt").write_text("one\n") + _git(["add", "f.txt"], cwd=work) + _git(["commit", "-q", "-m", "initial"], cwd=work) + _git(["remote", "add", "origin", str(origin)], cwd=work) + _git(["push", "-q", "-u", "origin", "main"], cwd=work) + # The bare repo's HEAD symref still points at the (nonexistent) default branch until something + # sets it explicitly; without this, `git clone` below checks out an unborn HEAD and "main" never + # exists as a local branch in `advancer`. + _git(["symbolic-ref", "HEAD", "refs/heads/main"], cwd=origin) + + # Advance origin/main independently, via a second clone, so `work`'s local `main` falls behind. + advancer = tmp_path / "advancer" + _git(["clone", "-q", str(origin), str(advancer)], cwd=tmp_path) + _git(["config", "user.email", "test@example.com"], cwd=advancer) + _git(["config", "user.name", "Test"], cwd=advancer) + (advancer / "f.txt").write_text("two\n") + _git(["add", "f.txt"], cwd=advancer) + _git(["commit", "-q", "-m", "advance"], cwd=advancer) + _git(["push", "-q", "origin", "main"], cwd=advancer) + + return work + + +def _run_hook(cwd, stdin_text): + env = dict(os.environ) + for k in ("GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE"): + env.pop(k, None) + return subprocess.run( + ["bash", str(HOOK)], + cwd=str(cwd), + input=stdin_text, + env=env, + capture_output=True, + text=True, + ) + + +def test_tag_only_push_from_a_behind_branch_is_allowed(behind_repo): + """The fix: a tag-only push must not be blocked by H11 even though the branch is behind.""" + stdin = f"refs/tags/v1.0.0 {DUMMY_SHA_A} refs/tags/v1.0.0 {DUMMY_SHA_B}\n" + r = _run_hook(behind_repo, stdin) + assert r.returncode == 0, f"tag-only push was blocked: {r.stdout}{r.stderr}" + + +def test_tag_only_push_ignores_blank_lines(behind_repo): + stdin = f"\nrefs/tags/v1.0.0 {DUMMY_SHA_A} refs/tags/v1.0.0 {DUMMY_SHA_B}\n\n" + r = _run_hook(behind_repo, stdin) + assert r.returncode == 0, f"tag-only push (with blank lines) was blocked: {r.stdout}{r.stderr}" + + +def test_negative_control_branch_push_from_behind_is_still_blocked(behind_repo): + """Required by #719: the fix must not weaken H11 for ordinary branch pushes.""" + stdin = f"refs/heads/feature {DUMMY_SHA_A} refs/heads/feature {DUMMY_SHA_B}\n" + r = _run_hook(behind_repo, stdin) + assert r.returncode == 1, "a branch push from a behind branch was allowed" + assert "H11" in r.stdout + + +def test_mixed_branch_and_tag_push_is_still_blocked(behind_repo): + stdin = ( + f"refs/heads/feature {DUMMY_SHA_A} refs/heads/feature {DUMMY_SHA_B}\n" + f"refs/tags/v1.0.0 {DUMMY_SHA_A} refs/tags/v1.0.0 {DUMMY_SHA_B}\n" + ) + r = _run_hook(behind_repo, stdin) + assert r.returncode == 1, "a mixed branch+tag push was allowed through the tag exemption" + assert "H11" in r.stdout + + +def test_zero_ref_lines_does_not_exempt(behind_repo): + """Vacuous-truth guard: 'all refs are tags' is trivially true over zero lines. Empty stdin + (hook run manually, or a caller that forgot to forward the ref lines) must fall through to the + existing behind-origin/main check, not silently disable H11.""" + r = _run_hook(behind_repo, "") + assert r.returncode == 1, "empty stdin vacuously granted the tag exemption" + assert "H11" in r.stdout + + +def test_zero_ref_lines_of_only_blank_lines_does_not_exempt(behind_repo): + r = _run_hook(behind_repo, "\n\n\n") + assert r.returncode == 1, "stdin of only blank lines vacuously granted the tag exemption" + assert "H11" in r.stdout -- 2.47.3 From 6d803433209d683e2ca38a33b738168cd382deb2 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 5 Aug 2026 23:52:28 +0200 Subject: [PATCH 3/5] fix(743): close the admin force-merge bypass; the push half alone was not enough MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .claude/hooks/prepush-rebase-check.sh | 16 ++++++--- docs/ci-cd.md | 20 +++++++---- docs/decisions/README.md | 4 +-- .../release/format-as-you-touch-rebase.md | 20 +++++++++-- .../release/main-direct-push-disabled.md | 36 ++++++++++++++----- ...test_prepush_rebase_check_tag_exemption.py | 11 +++--- 6 files changed, 78 insertions(+), 29 deletions(-) diff --git a/.claude/hooks/prepush-rebase-check.sh b/.claude/hooks/prepush-rebase-check.sh index 3a65bdc95..4dfa00f45 100755 --- a/.claude/hooks/prepush-rebase-check.sh +++ b/.claude/hooks/prepush-rebase-check.sh @@ -12,11 +12,11 @@ set -uo pipefail [ "${ETV_SKIP_REBASE_CHECK:-}" = "1" ] && exit 0 git rev-parse --git-dir >/dev/null 2>&1 || exit 0 -# Tag-only push exemption (ersatztv#719): a release cut tags the merge commit of a just-merged -# release-notes PR (docs/ci-cd.md -> "Cutting a release"), so the local branch is ALWAYS 1 commit -# behind origin/main at tag time -- H11 blocked every release. A tag push cannot revert anyone's +# Tag-only push exemption (ersatztv#719): the release cut tags a commit on main while the local +# branch sits 1 commit behind origin/main, so H11 blocked EVERY release -- and its "rebase first" +# advice did not even apply, since no branch was being pushed. A tag push cannot revert anyone's # merged work, which is the failure mode H11 exists to prevent, so skip the freshness check when -# EVERY ref being pushed is under refs/tags/. +# EVERY ref being pushed is under refs/tags/. (See #719 for the observed flow.) # # Read pushed refs from stdin: git feeds pre-push hooks one line per ref, " # " (.husky/pre-push forwards the lines it already captured). Ignore blank @@ -24,15 +24,21 @@ git rev-parse --git-dir >/dev/null 2>&1 || exit 0 # (hook run manually, stdin not forwarded, etc.) -- that would silently disable H11 for every push. # Require at least one parsed ref line before granting the exemption; with zero lines, fall through # to the existing branch-freshness check below (current behavior preserved). +# +# `[ -t 0 ] ||` so an interactive run does not hang waiting on a terminal: this script had no stdin +# reader before #719, and its own docs call "run by hand" a supported case. A TTY yields no ref +# lines, which is exactly the zero-line fall-through. _h11_refs_seen=0 _h11_all_tags=1 -while IFS=' ' read -r _h11_local_ref _h11_local_sha _h11_remote_ref _h11_remote_sha; do +[ -t 0 ] || while IFS=' ' read -r _h11_local_ref _h11_local_sha _h11_remote_ref _h11_remote_sha \ + || [ -n "${_h11_local_ref:-}" ]; do # `|| [ -n ... ]` also processes a final line with no trailing newline [ -z "${_h11_local_ref:-}" ] && continue _h11_refs_seen=1 case "${_h11_remote_ref:-}" in refs/tags/*) ;; *) _h11_all_tags=0 ;; esac + _h11_local_ref='' done if [ "$_h11_refs_seen" = "1" ] && [ "$_h11_all_tags" = "1" ]; then exit 0 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index b7e49e0de..a0e529161 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -773,12 +773,20 @@ from `Build ErsatzTV Image / …` to `PR Gates / …`) does not affect merges. T unreviewed commit from merging** (ersatztv#622). It is not produced by a job's success/failure; it is a commit status that `scripts/post-review-verdict.sh` POSTs onto one specific sha. -**`main` is PR-only, and that is what makes the check load-bearing** (ersatztv#743, -`release.main-direct-push-disabled`). Gitea evaluates `status_check_contexts` when it **merges a -PR** — a direct `git push origin HEAD:main` never consults them. So until 2026-08-05 the entire gate -was skippable with no forgery at all, which was cheaper than every route enumerated in #697. `main` -now carries `enable_push: false`: a direct push is refused server-side at pre-receive -(`Not allowed to push to protected branch main`), for every account including a site admin. +**`main` is PR-only AND admin-override-proof, and it takes both to make the check load-bearing** +(ersatztv#743, `release.main-direct-push-disabled`). Gitea evaluates `status_check_contexts` when it +**merges a PR** — a direct `git push origin HEAD:main` never consults them. So until 2026-08-05 the +entire gate was skippable with no forgery at all, which was cheaper than every route enumerated in +#697. `main` now carries **two** fields, and citing either alone is a mistake: + +- `enable_push: false` — a direct push is refused server-side at pre-receive (`Not allowed to push to + protected branch main`), for every account including a site admin. The contents API and web editor + are refused too (HTTP 403 `user cannot commit to repo`); they share the same predicate. +- `block_admin_merge_override: true` — without it (the default is `false`), a repo admin could + `POST /pulls/{n}/merge` with `force_merge: true` and merge straight past a missing or red + `review-verdict/h10`. Disabling push alone just moves the bypass from the push path to the merge + path, since `timothy` is admin and is the identity every session already uses. + Practical consequences: **every** change to `main` goes through a PR, including a one-line docs fix; and the client-side Husky guards (H6/H11/H13) remain useful friction but were never the control — they are fail-open and `--no-verify` bypasses them. Tag pushes are unaffected (separate mechanism; diff --git a/docs/decisions/README.md b/docs/decisions/README.md index a7231ae23..a394c1f4a 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -121,9 +121,9 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `process.subagent-drop-resume` | Treat a subagent connection drop as laptop sleep or transient network and re-resume via SendMessage — the work survives. | 2026-07-21 | [link](records/process/subagent-drop-resume.md) | | `release.api-contract-ci-gate` | A PR touching `ErsatzTV/Controllers/Api/**` or `ErsatzTV.Core/Api/**` must ship regenerated OpenAPI artifacts (`v1.json`, `v1.d.ts`, `endpoint-index.md`) in the same diff, enforced by a blocking `api-docs` CI job that regenerates-and-diffs against a fresh build. | 2026-07-12 | [link](records/release/api-contract-ci-gate.md) | | `release.done-when-merge-consent` | A PR may merge only when its linked issue's `## Done-when` checklist is fully ticked and the PR's CI is green, enforced by a PreToolUse hook on the Gitea merge tool (deny/allow/ask) plus a pre-push backstop for direct pushes to main. | 2026-07-12 | [link](records/release/done-when-merge-consent.md) | -| `release.format-as-you-touch-rebase` | A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR's changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. | 2026-07-12 | [link](records/release/format-as-you-touch-rebase.md) | +| `release.format-as-you-touch-rebase` | A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR's changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. H11 has ONE always-on carve-out, #719 — a push in which EVERY ref is under `refs/tags/` skips the freshness check, because a tag push cannot revert merged work, which is the failure mode H11 exists to prevent, and the release cut always tags from a branch that is 1 behind `origin/main`. A push mixing branch and tag refs is still blocked, and so is a push with zero parsed ref lines (the exemption requires at least one, so empty stdin cannot vacuously disable H11). | 2026-07-12 | [link](records/release/format-as-you-touch-rebase.md) | | `release.live-e2e-required` | A PR that changes an API write-path handler must include a live-E2E pass (driving the real endpoint/screen and confirming the round-trip through a subsequent read), not only unit/characterization tests, and must state whether live-E2E ran or wasn't required. | 2026-07-12 | [link](records/release/live-e2e-required.md) | -| `release.main-direct-push-disabled` | Branch protection on `main` carries `enable_push: false`, so `main` is reachable ONLY through the PR merge path — the one path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. This is what makes merge-consent derived rather than merely conventional: before it, `git push origin HEAD:main` skipped the gate with no forgery, which was strictly cheaper than every route enumerated in #697. Do NOT "soften" this to a push WHITELIST: measured on this instance, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as — so the whitelist form closes nothing while reading in review as a control. The block binds a site admin at pre-receive; it does NOT bind a credential that can first PATCH branch protection, which an admin token can (see the residual below). Tag pushes are unaffected — a separate mechanism (`tag_protections`) governs them, so the release cut still works. | 2026-08-05 | [link](records/release/main-direct-push-disabled.md) | +| `release.main-direct-push-disabled` | Branch protection on `main` carries `enable_push: false` AND `block_admin_merge_override: true`. Both halves are required and neither is sufficient. `enable_push: false` removes the direct-push path, leaving the PR merge path — the only path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. `block_admin_merge_override: true` then closes the force-merge bypass on that remaining path: with it false (the default), `CanBypassBranchProtection` returns true for a repo admin, so `POST /pulls/{n}/merge` with `force_merge: true` merges a PR whose `h10` is missing or red — one API call, no forgery, no PATCH. Do NOT "soften" the push half to a push WHITELIST: measured here, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as, so the whitelist form closes nothing while reading in review as a control. Same reasoning is why the admin-override half is needed: an admin-shaped control that exempts the only admin exempts everybody. What remains open: a credential that can PATCH branch protection off can still undo either half — an accepted residual, not a closed route. Tag pushes are unaffected (`tag_protections` governs those separately), so the release cut still works. | 2026-08-05 | [link](records/release/main-direct-push-disabled.md) | | `release.merge-consent-autogrant` | When Done-when boxes are ticked, CI is green, and a fresh positive Review-verdict references head, the merge-consent hook emits `permissionDecision: allow` to actually suppress the redundant mechanical prompt — the derived state IS the consent, no separate conversational confirmation on that path. | 2026-07-12 | [link](records/release/merge-consent-autogrant.md) | | `release.migration-rehearsal-prodcopy` | Before promoting a migration-bearing release, rehearse the new image's migrations against a throwaway copy of the latest prod backup (`scripts/migration-smoke.sh`), gating PASS on the migrator's completion log line rather than HTTP readiness alone. | 2026-07-12 | [link](records/release/migration-rehearsal-prodcopy.md) | | `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) | diff --git a/docs/decisions/records/release/format-as-you-touch-rebase.md b/docs/decisions/records/release/format-as-you-touch-rebase.md index fdceb2dbf..130c0fbfb 100644 --- a/docs/decisions/records/release/format-as-you-touch-rebase.md +++ b/docs/decisions/records/release/format-as-you-touch-rebase.md @@ -5,8 +5,8 @@ status: active since: '2026-07-12' supersedes: none superseded-by: none -rule: A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR's changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. -signals: 'format-as-you-touch, rebase not merge, BOM backlog · paths: `.husky/pre-push`, `.claude/hooks/prepush-rebase-check.sh` · issues: #311 (H11), #309, #310, #269, #312' +rule: 'A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR''s changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. H11 has ONE always-on carve-out, #719 — a push in which EVERY ref is under `refs/tags/` skips the freshness check, because a tag push cannot revert merged work, which is the failure mode H11 exists to prevent, and the release cut always tags from a branch that is 1 behind `origin/main`. A push mixing branch and tag refs is still blocked, and so is a push with zero parsed ref lines (the exemption requires at least one, so empty stdin cannot vacuously disable H11).' +signals: 'format-as-you-touch, rebase not merge, BOM backlog, tag-only push exemption, H11 blocks release cut, refs/tags pre-push, vacuous-truth guard · paths: `.husky/pre-push`, `.claude/hooks/prepush-rebase-check.sh`, `scripts/tests/test_prepush_rebase_check_tag_exemption.py` · issues: #311 (H11), #719, #309, #310, #269, #312' mechanics: '`docs/contributing.md` §7; `.claude/hooks/prepush-rebase-check.sh`; `npm run check:api`' --- @@ -36,5 +36,21 @@ git hook has no "ask"); deliberate escape `ETV_SKIP_REBASE_CHECK=1`. This supers guidance to "merge main into your PR branch." (After a rebase that conflicts in *generated* artifacts — v1.json/v1.d.ts/endpoint-index — regenerate, don't hand-resolve; `npm run check:api` guards.) +**2a. The tag-only carve-out (#719).** H11 as originally written fired on *every* release cut: the +documented sequence tags the merge commit of a just-merged release-notes PR, so the local branch is +always 1 behind `origin/main` at tag time, and the rebase advice it printed did not even apply — no +branch was being pushed. The hook now reads git's pre-push ref lines (` + `) and exits 0 when every parsed line's *remote* ref is under `refs/tags/`. +Two details are load-bearing and easy to regress: +- `.husky/pre-push` consumes stdin into `$_prepush_refs` before any guard runs, so it must **forward** + those lines (`printf '%s\n' "$_prepush_refs" | …`). Without that the check receives EOF and the + exemption is dead code that silently never fires. The unit tests drive the hook directly and would + still pass, so this wiring is not covered by them. +- The exemption requires **at least one** parsed ref line. "All refs are tags" is vacuously true for + zero lines, which would disable H11 for every push; with no lines the hook falls through to the + normal freshness check. `scripts/tests/test_prepush_rebase_check_tag_exemption.py` pins both the + negative control (branch push from a behind branch still blocked), the mixed branch+tag case, and + the two zero-line cases. + Rationale, as with the whole hook program: make the process rule a derivation/hook, not prose to remember (#303 methodology review). Tracked: #311; sibling #312 (H12 issue-qualification audit). diff --git a/docs/decisions/records/release/main-direct-push-disabled.md b/docs/decisions/records/release/main-direct-push-disabled.md index b68f7ce50..8ace6099e 100644 --- a/docs/decisions/records/release/main-direct-push-disabled.md +++ b/docs/decisions/records/release/main-direct-push-disabled.md @@ -5,9 +5,9 @@ status: active since: '2026-08-05' supersedes: none superseded-by: none -rule: 'Branch protection on `main` carries `enable_push: false`, so `main` is reachable ONLY through the PR merge path — the one path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. This is what makes merge-consent derived rather than merely conventional: before it, `git push origin HEAD:main` skipped the gate with no forgery, which was strictly cheaper than every route enumerated in #697. Do NOT "soften" this to a push WHITELIST: measured on this instance, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as — so the whitelist form closes nothing while reading in review as a control. The block binds a site admin at pre-receive; it does NOT bind a credential that can first PATCH branch protection, which an admin token can (see the residual below). Tag pushes are unaffected — a separate mechanism (`tag_protections`) governs them, so the release cut still works.' +rule: 'Branch protection on `main` carries `enable_push: false` AND `block_admin_merge_override: true`. Both halves are required and neither is sufficient. `enable_push: false` removes the direct-push path, leaving the PR merge path — the only path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. `block_admin_merge_override: true` then closes the force-merge bypass on that remaining path: with it false (the default), `CanBypassBranchProtection` returns true for a repo admin, so `POST /pulls/{n}/merge` with `force_merge: true` merges a PR whose `h10` is missing or red — one API call, no forgery, no PATCH. Do NOT "soften" the push half to a push WHITELIST: measured here, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as, so the whitelist form closes nothing while reading in review as a control. Same reasoning is why the admin-override half is needed: an admin-shaped control that exempts the only admin exempts everybody. What remains open: a credential that can PATCH branch protection off can still undo either half — an accepted residual, not a closed route. Tag pushes are unaffected (`tag_protections` governs those separately), so the release cut still works.' signals: 'direct push to main, push whitelist, enable_push false, branch protection bypass, review-verdict/h10 bypassable without forging, merge consent derived not asserted, pre-receive hook declined, Not allowed to push to protected branch, protected branch, tag_protections, release tag push, GITEA_TOKEN repo write, RENOVATE_TOKEN, site admin bypass, PR-only flow · paths: `docs/ci-cd.md` · issues: #743, #697, #698, #622, #672, #706, #742, server-management#714' -mechanics: 'Gitea 1.27.1. `PATCH /api/v1/repos/timothy/ersatztv/branch_protections/main` with `{"enable_push": false}`; whitelist fields left off (`enable_push_whitelist: false`, empty arrays). Verified 2026-08-05 against a throwaway `probe-743-*` rule rather than against `main`: with `enable_push: false` a push by `timothy` (site admin) was REFUSED — `pre-receive hook declined`, `Not allowed to push to protected branch`; after PATCHing the same rule to `enable_push: true` + `enable_push_whitelist: true` + `push_whitelist_usernames: ["timothy"]` the identical push SUCCEEDED. Then on `main` itself: `git push origin HEAD:main` REFUSED, and a tag-only push SUCCEEDED in the same worktree. `GET .../tag_protections` returns `[]`. All probe artifacts (rule, branch, tag) deleted and confirmed gone; `origin/main` head unchanged at `08e95f9ec` throughout.' +mechanics: 'Gitea 1.27.1. `PATCH /api/v1/repos/timothy/ersatztv/branch_protections/main` with `{"enable_push": false, "block_admin_merge_override": true}`; whitelist fields left off (`enable_push_whitelist: false`, empty arrays), `enable_force_push: false`, `enable_merge_whitelist: false`, `required_approvals: 0`. MEASURED 2026-08-05 against a throwaway `probe-743-*` rule rather than against `main`: with `enable_push: false` a push by `timothy` (site admin) was REFUSED — `pre-receive hook declined`, `Not allowed to push to protected branch`; after PATCHing the same rule to `enable_push: true` + `enable_push_whitelist: true` + `push_whitelist_usernames: ["timothy"]` the identical push SUCCEEDED. Separately probed on a second throwaway rule: a contents-API write (`PUT /repos/{o}/{r}/contents/{path}` with `branch` set to the protected branch) was REFUSED HTTP 403 `user cannot commit to repo [user: timothy]` — so the web-editor/API file-write surface does not bypass it either. Then on `main` itself: `git push origin HEAD:main` REFUSED, and a tag-only push SUCCEEDED from the same worktree. `GET .../tag_protections` returns `[]`; repo is `fork: false`, `mirror: false`. NOT measured, source-attested only (Gitea 1.27 `CanBypassBranchProtection`, `services/pull/check.go`, `routers/private/hook_pre_receive.go`): that `block_admin_merge_override: false` would have let an admin `force_merge` past the required contexts — the field was set to true rather than probed, since probing it means merging an unreviewed PR. All probe artifacts (two rules, two branches, one tag) deleted and confirmed gone; `origin/main` head unchanged at `08e95f9ec` throughout.' --- **Why a whitelist was the wrong shape.** #743 proposed "a push whitelist on `main` (or disable direct @@ -20,19 +20,37 @@ the control is supposed to constrain. It would have ticked the issue's box while This was measured, not reasoned: the same push was refused under `enable_push: false` and accepted under a whitelist naming `timothy`. +**Disabling push alone was NOT enough, and the reason is the same argument twice.** The first draft of +this record disabled direct push and concluded that `review-verdict/h10` was therefore load-bearing. +An independent review caught that this repeated on the merge path exactly the mistake it had just +diagnosed on the push path. The push argument was: a whitelist naming `timothy` fails because +`timothy` is the identity every credential already holds. The merge path had the identical shape — +`block_admin_merge_override` defaulted to `false`, so `CanBypassBranchProtection` returned true for a +repo admin and `POST /pulls/{n}/merge` with `force_merge: true` merged straight past a missing or red +`h10`. One API call, cheaper than the push route it replaced. **An admin-shaped control that exempts +the only admin exempts everybody.** Both fields are now set; treat them as one control, and never +cite `enable_push: false` alone as the reason the gate holds. + **What this actually closes, and what it does not.** It closes the *write-only* credential routes, which is most of #743's own "who can do it" list: the injected `GITEA_TOKEN` (repo write, not admin), `RENOVATE_TOKEN`, and any non-admin collaborator PAT. Those can no longer reach `main` at all, by any path that skips the gate. It does **not** close the admin route. `timothy` is a site admin, so a credential holding that -identity can `PATCH` the branch protection off, push, and restore it — the exact sequence used to -*prove* the semantics above. Closing that requires agent sessions to run as a scoped non-admin -credential, which is a different change with its own cost (packages live in a user namespace; see the -"Admin ownership is a real residual" section of `ci.actions-credential-scoping`). Recorded as an -accepted residual rather than fixed here, so it is not mistaken for covered. The severity bound from -#697 and #743 is unchanged throughout: push access is required, so this is a compromised contributor -or a subverted automated session, never an anonymous attacker. +identity can `PATCH` either field off, act, and restore it — the exact sequence used to *prove* the +push semantics above. Closing that requires agent sessions to run as a scoped non-admin credential, +which is a different change with its own cost (packages live in a user namespace; see the "Admin +ownership is a real residual" section of `ci.actions-credential-scoping`). Recorded as an accepted +residual rather than fixed here, so it is not mistaken for covered. The severity bound from #697 and +#743 is unchanged throughout: push access is required, so this is a compromised contributor or a +subverted automated session, never an anonymous attacker. + +**Which write surfaces were enumerated.** `git push` (measured, refused), the contents API and by +extension the web editor / upload path (measured on a probe branch, refused HTTP 403 — they share the +`CanUserPush` predicate, which has no admin special-case and no `unprotected_file_patterns` carve-out +since that field is empty), apply-patch / revert / cherry-pick (source-attested, same predicate), +force push (`enable_force_push: false`), default-branch deletion (separately refused), and fork-sync / +mirror (not applicable: `fork: false`, `mirror: false`). Merge remains the one intended path. **Why the release cut does not deadlock.** #743 flagged that the tag path had to keep working, and #719 documents H11 blocking a tag-only push on every release cut. Branch protection is scoped to diff --git a/scripts/tests/test_prepush_rebase_check_tag_exemption.py b/scripts/tests/test_prepush_rebase_check_tag_exemption.py index 379e3c540..6f3c28088 100644 --- a/scripts/tests/test_prepush_rebase_check_tag_exemption.py +++ b/scripts/tests/test_prepush_rebase_check_tag_exemption.py @@ -1,11 +1,12 @@ """Tests for the tag-only-push exemption in `.claude/hooks/prepush-rebase-check.sh` (ersatztv#719). H11 refuses to push a branch that is behind `origin/main`, to force a rebase instead of a merge. -But the documented release flow (`docs/ci-cd.md` -> "Cutting a release") tags the merge commit of a -just-merged release-notes PR, so the local branch is *always* one commit behind `origin/main` at tag -time -- H11 blocked every release cut. A tag push cannot revert anyone's merged work (the failure -mode H11 exists to prevent), so the fix skips the freshness check when EVERY ref being pushed is -under `refs/tags/`. +But the release cut tags a commit on `main` from a branch that is behind `origin/main`, so H11 +blocked every release -- and its "rebase first" advice did not even apply, because no branch was +being pushed. (Observed while cutting v26.13.0; see #719. `docs/ci-cd.md` -> "Cutting a release" +documents the tag step itself, not the release-notes-PR flow that puts the branch behind.) A tag +push cannot revert anyone's merged work (the failure mode H11 exists to prevent), so the fix skips +the freshness check when EVERY ref being pushed is under `refs/tags/`. These tests use real local git repositories (a bare "origin" plus a work tree pushed one commit behind it) rather than stubbing `git`, because the hook's decision hinges on genuine -- 2.47.3 From 6af65ba5c57fd0c02427285d55ea695e5ca3eb4d Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 00:10:33 +0200 Subject: [PATCH 4/5] fix(743): re-tense the third stale site, and pin the two surviving mutants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/ci-cd.md | 23 ++++++-- docs/decisions/README.md | 2 +- .../release/format-as-you-touch-rebase.md | 11 ++-- .../records/release/verdict-status-check.md | 10 ++-- ...test_prepush_rebase_check_tag_exemption.py | 54 +++++++++++++++++++ 5 files changed, 86 insertions(+), 14 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index a0e529161..285ebb12f 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -780,13 +780,25 @@ entire gate was skippable with no forgery at all, which was cheaper than every r #697. `main` now carries **two** fields, and citing either alone is a mistake: - `enable_push: false` — a direct push is refused server-side at pre-receive (`Not allowed to push to - protected branch main`), for every account including a site admin. The contents API and web editor - are refused too (HTTP 403 `user cannot commit to repo`); they share the same predicate. + protected branch main`), for every account including a site admin. The contents API is refused too + — measured, HTTP 403 `user cannot commit to repo`. The web editor, upload, apply-patch, revert and + cherry-pick paths share that same `CanUserPush` predicate and are therefore expected to refuse as + well, but were not probed (source-attested only). - `block_admin_merge_override: true` — without it (the default is `false`), a repo admin could `POST /pulls/{n}/merge` with `force_merge: true` and merge straight past a missing or red `review-verdict/h10`. Disabling push alone just moves the bypass from the push path to the merge path, since `timothy` is admin and is the identity every session already uses. +**Operator recovery when a required context gets stuck.** `block_admin_merge_override: true` removes +the "Merge (admin)" / `force_merge: true` escape that used to unstick a PR whose required context was +absent or wrongly red — a recurring situation here (a killed run overwriting a newer green, an +advisory red counted into the combined status, a gate workflow that cannot post). That escape is gone +*by design*: it was also the bypass. The supported recovery is to fix the status +(re-run the job, or re-post the verdict with `scripts/post-review-verdict.sh`); the last resort is to +`PATCH .../branch_protections/main` setting `block_admin_merge_override: false`, merge, and set it +straight back. Do the last one deliberately and say so in the PR — it is the one action that +re-opens the hole this section exists to close. + Practical consequences: **every** change to `main` goes through a PR, including a one-line docs fix; and the client-side Husky guards (H6/H11/H13) remain useful friction but were never the control — they are fail-open and `--no-verify` bypasses them. Tag pushes are unaffected (separate mechanism; @@ -948,9 +960,10 @@ as establishing that the gate cannot be forged (see the residual below, and ersa (server-management#714) binds the injected token, but does not close the class either — not against a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). **And none of it was necessary: direct pushes to `main` were server-side permitted, so the gate could be skipped without any forgery - (ersatztv#743). That is now CLOSED — `main` carries `enable_push: false`, so it is reachable only - through the PR merge path, the one path on which Gitea evaluates `status_check_contexts` - (`release.main-direct-push-disabled`).** Note the fix is *disabling* push, not whitelisting it: a + (ersatztv#743). That is now CLOSED — `main` carries `enable_push: false` **and** + `block_admin_merge_override: true`, so it is reachable only through the PR merge path, the one path + on which Gitea evaluates `status_check_contexts`, and an admin cannot `force_merge` past them + (`release.main-direct-push-disabled` — neither field is citable alone).** Note the fix is *disabling* push, not whitelisting it: a push whitelist naming `timothy` was measured to still admit the push, and `timothy` is the identity every session, PAT and injected `GITEA_TOKEN` already acts as, so the whitelist form would have closed nothing. The block binds a site admin at pre-receive but not a credential that can first diff --git a/docs/decisions/README.md b/docs/decisions/README.md index a394c1f4a..b6e1fa15d 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -121,7 +121,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `process.subagent-drop-resume` | Treat a subagent connection drop as laptop sleep or transient network and re-resume via SendMessage — the work survives. | 2026-07-21 | [link](records/process/subagent-drop-resume.md) | | `release.api-contract-ci-gate` | A PR touching `ErsatzTV/Controllers/Api/**` or `ErsatzTV.Core/Api/**` must ship regenerated OpenAPI artifacts (`v1.json`, `v1.d.ts`, `endpoint-index.md`) in the same diff, enforced by a blocking `api-docs` CI job that regenerates-and-diffs against a fresh build. | 2026-07-12 | [link](records/release/api-contract-ci-gate.md) | | `release.done-when-merge-consent` | A PR may merge only when its linked issue's `## Done-when` checklist is fully ticked and the PR's CI is green, enforced by a PreToolUse hook on the Gitea merge tool (deny/allow/ask) plus a pre-push backstop for direct pushes to main. | 2026-07-12 | [link](records/release/done-when-merge-consent.md) | -| `release.format-as-you-touch-rebase` | A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR's changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. H11 has ONE always-on carve-out, #719 — a push in which EVERY ref is under `refs/tags/` skips the freshness check, because a tag push cannot revert merged work, which is the failure mode H11 exists to prevent, and the release cut always tags from a branch that is 1 behind `origin/main`. A push mixing branch and tag refs is still blocked, and so is a push with zero parsed ref lines (the exemption requires at least one, so empty stdin cannot vacuously disable H11). | 2026-07-12 | [link](records/release/format-as-you-touch-rebase.md) | +| `release.format-as-you-touch-rebase` | A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR's changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. H11 has ONE always-on carve-out, #719 — a push in which EVERY ref is under `refs/tags/` skips the freshness check, because a tag push cannot revert merged work, which is the failure mode H11 exists to prevent, and the release cut tags from a branch that is behind `origin/main` (observed on the v26.13.0 cut, #719). A push mixing branch and tag refs is still blocked, and so is a push with zero parsed ref lines (the exemption requires at least one, so empty stdin cannot vacuously disable H11). | 2026-07-12 | [link](records/release/format-as-you-touch-rebase.md) | | `release.live-e2e-required` | A PR that changes an API write-path handler must include a live-E2E pass (driving the real endpoint/screen and confirming the round-trip through a subsequent read), not only unit/characterization tests, and must state whether live-E2E ran or wasn't required. | 2026-07-12 | [link](records/release/live-e2e-required.md) | | `release.main-direct-push-disabled` | Branch protection on `main` carries `enable_push: false` AND `block_admin_merge_override: true`. Both halves are required and neither is sufficient. `enable_push: false` removes the direct-push path, leaving the PR merge path — the only path on which Gitea evaluates `status_check_contexts`, and therefore the only path on which `review-verdict/h10` is consulted at all. `block_admin_merge_override: true` then closes the force-merge bypass on that remaining path: with it false (the default), `CanBypassBranchProtection` returns true for a repo admin, so `POST /pulls/{n}/merge` with `force_merge: true` merges a PR whose `h10` is missing or red — one API call, no forgery, no PATCH. Do NOT "soften" the push half to a push WHITELIST: measured here, a whitelist naming `timothy` still admits the push, and `timothy` is the identity every agent session, PAT and injected `GITEA_TOKEN` already acts as, so the whitelist form closes nothing while reading in review as a control. Same reasoning is why the admin-override half is needed: an admin-shaped control that exempts the only admin exempts everybody. What remains open: a credential that can PATCH branch protection off can still undo either half — an accepted residual, not a closed route. Tag pushes are unaffected (`tag_protections` governs those separately), so the release cut still works. | 2026-08-05 | [link](records/release/main-direct-push-disabled.md) | | `release.merge-consent-autogrant` | When Done-when boxes are ticked, CI is green, and a fresh positive Review-verdict references head, the merge-consent hook emits `permissionDecision: allow` to actually suppress the redundant mechanical prompt — the derived state IS the consent, no separate conversational confirmation on that path. | 2026-07-12 | [link](records/release/merge-consent-autogrant.md) | diff --git a/docs/decisions/records/release/format-as-you-touch-rebase.md b/docs/decisions/records/release/format-as-you-touch-rebase.md index 130c0fbfb..09f5d88cb 100644 --- a/docs/decisions/records/release/format-as-you-touch-rebase.md +++ b/docs/decisions/records/release/format-as-you-touch-rebase.md @@ -5,7 +5,7 @@ status: active since: '2026-07-12' supersedes: none superseded-by: none -rule: 'A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR''s changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. H11 has ONE always-on carve-out, #719 — a push in which EVERY ref is under `refs/tags/` skips the freshness check, because a tag push cannot revert merged work, which is the failure mode H11 exists to prevent, and the release cut always tags from a branch that is 1 behind `origin/main`. A push mixing branch and tag refs is still blocked, and so is a push with zero parsed ref lines (the exemption requires at least one, so empty stdin cannot vacuously disable H11).' +rule: 'A blocking `format` CI job runs `dotnet format --verify-no-changes` scoped only to the PR''s changed `.cs` files (never the legacy BOM backlog), and a PR branch must be kept current by rebasing on `origin/main` (never merging main in), enforced by `.husky/pre-push` → `prepush-rebase-check.sh`. H11 has ONE always-on carve-out, #719 — a push in which EVERY ref is under `refs/tags/` skips the freshness check, because a tag push cannot revert merged work, which is the failure mode H11 exists to prevent, and the release cut tags from a branch that is behind `origin/main` (observed on the v26.13.0 cut, #719). A push mixing branch and tag refs is still blocked, and so is a push with zero parsed ref lines (the exemption requires at least one, so empty stdin cannot vacuously disable H11).' signals: 'format-as-you-touch, rebase not merge, BOM backlog, tag-only push exemption, H11 blocks release cut, refs/tags pre-push, vacuous-truth guard · paths: `.husky/pre-push`, `.claude/hooks/prepush-rebase-check.sh`, `scripts/tests/test_prepush_rebase_check_tag_exemption.py` · issues: #311 (H11), #719, #309, #310, #269, #312' mechanics: '`docs/contributing.md` §7; `.claude/hooks/prepush-rebase-check.sh`; `npm run check:api`' --- @@ -36,10 +36,11 @@ git hook has no "ask"); deliberate escape `ETV_SKIP_REBASE_CHECK=1`. This supers guidance to "merge main into your PR branch." (After a rebase that conflicts in *generated* artifacts — v1.json/v1.d.ts/endpoint-index — regenerate, don't hand-resolve; `npm run check:api` guards.) -**2a. The tag-only carve-out (#719).** H11 as originally written fired on *every* release cut: the -documented sequence tags the merge commit of a just-merged release-notes PR, so the local branch is -always 1 behind `origin/main` at tag time, and the rebase advice it printed did not even apply — no -branch was being pushed. The hook now reads git's pre-push ref lines (` +**2a. The tag-only carve-out (#719).** H11 fired on the release cut: tagging a commit on `main` from +a branch that is behind `origin/main` tripped the freshness check, and the rebase advice it printed +did not even apply — no branch was being pushed. Observed while cutting `v26.13.0` (#719); note +`docs/ci-cd.md` → "Cutting a release" documents the tag step itself, not the release-notes-PR flow +that leaves the branch behind, so the frequency is attested by #719 rather than by that doc. The hook now reads git's pre-push ref lines (` `) and exits 0 when every parsed line's *remote* ref is under `refs/tags/`. Two details are load-bearing and easy to regress: - `.husky/pre-push` consumes stdin into `$_prepush_refs` before any guard runs, so it must **forward** diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 00a7803d5..d40a1e72d 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -189,9 +189,13 @@ described as one: inherited, #742), a collaborator's own token, and the `v*` tag push — which matters less for forging this status than for what else it does: `docker-build.yml` publishes `:prod` from a tagged ref, and a tag may point at any commit, so it ships a prod image with no PR, review or status. - None of which is even required — direct pushes to `main` are server-side permitted, so the gate - can be skipped without forging anything (#743). So the "careless - change rather than a hostile one" posture below still + None of which used to be even required — direct pushes to `main` were server-side permitted, so + the gate could be skipped without forging anything (#743). **That route is now closed** + (`release.main-direct-push-disabled`): `main` carries `enable_push: false` *and* + `block_admin_merge_override: true`, so every change reaches `main` through the PR merge path, + which is the only path on which these required contexts are evaluated. What survives is the + forgery list above — those routes post a status rather than skip it, so they are still real. So + the "careless change rather than a hostile one" posture below still describes the repo accurately — it is simply no longer *this* workflow that is the weakest link. An untrusted-contributor repo would still need the classification moved somewhere no PR can reach (server-side policy), not merely a base-pinned definition. diff --git a/scripts/tests/test_prepush_rebase_check_tag_exemption.py b/scripts/tests/test_prepush_rebase_check_tag_exemption.py index 6f3c28088..f88758c3c 100644 --- a/scripts/tests/test_prepush_rebase_check_tag_exemption.py +++ b/scripts/tests/test_prepush_rebase_check_tag_exemption.py @@ -130,3 +130,57 @@ def test_zero_ref_lines_of_only_blank_lines_does_not_exempt(behind_repo): r = _run_hook(behind_repo, "\n\n\n") assert r.returncode == 1, "stdin of only blank lines vacuously granted the tag exemption" assert "H11" in r.stdout + + +# --- final line with NO trailing newline ------------------------------------------------------- +# `read` returns non-zero on an unterminated final line, so a bare `while read` silently DROPS it. +# Both directions matter and they fail differently, which is why each is pinned: +# - tag-only, unterminated -> the line is dropped, no refs are seen, and H11 blocks the release +# tag push again, i.e. #719 quietly returns. +# - mixed, unterminated -> the BRANCH line is dropped, leaving only tag refs, and the +# exemption is granted for a push that includes a branch. That is the dangerous direction. +# Git always newline-terminates its ref lines and `.husky/pre-push` re-adds one via `printf '%s\n'`, +# so this is reachable only on a hand-piped run — but the guard is cheap and the failure is silent. + + +def test_unterminated_final_line_tag_only_is_still_exempt(behind_repo): + stdin = f"refs/tags/v1.0.0 {DUMMY_SHA_A} refs/tags/v1.0.0 {DUMMY_SHA_B}" # no trailing \n + r = _run_hook(behind_repo, stdin) + assert r.returncode == 0, f"unterminated tag-only line was dropped, reinstating #719: {r.stdout}" + + +def test_tty_stdin_does_not_hang_and_does_not_exempt(behind_repo): + """The hook gained a stdin reader in #719; before that it read nothing, and its own docs call + 'run by hand' a supported case. Without the `[ -t 0 ] ||` guard an interactive run blocks + forever waiting on the terminal. A pty gives it a real TTY on fd 0; the `timeout` turns a + regression into a clean failure instead of a hung CI job.""" + primary, secondary = os.openpty() + try: + r = subprocess.run( + ["bash", str(HOOK)], + cwd=str(behind_repo), + stdin=secondary, + capture_output=True, + text=True, + timeout=30, + ) + except subprocess.TimeoutExpired: + pytest.fail("hook hung on TTY stdin — the `[ -t 0 ] ||` guard is missing or ineffective") + finally: + os.close(primary) + os.close(secondary) + # A TTY yields no ref lines, so this is the zero-line fall-through: H11 still applies. + assert r.returncode == 1, "TTY stdin vacuously granted the tag exemption" + assert "H11" in r.stdout + + +def test_unterminated_final_branch_line_is_not_swallowed_into_the_exemption(behind_repo): + """The dangerous direction: if the unterminated BRANCH line is dropped, only tag refs remain + and a branch push wins the tag exemption.""" + stdin = ( + f"refs/tags/v1.0.0 {DUMMY_SHA_A} refs/tags/v1.0.0 {DUMMY_SHA_B}\n" + f"refs/heads/feature {DUMMY_SHA_A} refs/heads/feature {DUMMY_SHA_B}" # no trailing \n + ) + r = _run_hook(behind_repo, stdin) + assert r.returncode == 1, "an unterminated branch ref was swallowed into the tag exemption" + assert "H11" in r.stdout -- 2.47.3 From f822e4737c1999734049f43bc82baf50753538e5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 00:35:41 +0200 Subject: [PATCH 5/5] docs(743): label the second attested claim, close the survivor list, state the rule in CLAUDE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 6 ++++-- docs/ci-cd.md | 5 ++++- docs/decisions/records/release/verdict-status-check.md | 6 +++++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index bd0087990..7fb1ff4fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,9 +85,11 @@ Every task that closes a Gitea issue MUST complete ALL of these before it is con **Merge-consent is derived from state, not asserted (`## Done-when` convention — ersatztv#303 H6 + H10).** Any issue whose PR will merge to `main` should carry a `## Done-when` section in its **issue body** — a checklist of completion criteria (always include an "adversarial review passed" box; add per-issue criteria like tests-green, docs-updated, live-E2E). Two hooks derive merge-consent from it so a premature merge is blocked *by construction*, not by memory: - `pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **auto-grants** a merge (emits `permissionDecision: allow`, so **no** redundant mechanical prompt fires) only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked **and** a `Review-verdict:` comment references the PR's *current head sha* (**H10**); **denies** on an unticked box, red CI, or a stale/negative review verdict; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no `Review-verdict:` comment yet, no creds, Gitea down). On the auto-grant (satisfied) path the derived state **is** the consent — do not also ask conversationally to merge; a separate human confirmation is warranted only when the gate **asks** (ersatztv#314). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), run **`scripts/post-review-verdict.sh [note]`** — it posts both the `Review-verdict: … @ ` comment and the sha-bound `review-verdict/h10` commit status, proving the *latest* commit was reviewed rather than a stale earlier diff (ersatztv#242). Do not hand-write the comment: the **status** is the required check branch protection enforces, and a comment alone leaves it absent. - **The gate is enforced server-side, per sha (ersatztv#622).** `review-verdict/h10` is a required status check on `main`. Because a commit status belongs to one sha, a commit pushed *after* an auto-merge is scheduled clears it and blocks the merge — closing the hole where `merge_when_checks_succeed` froze consent at scheduling time and Gitea later merged an unreviewed head. Renovate-authored and docs-only PRs are auto-passed by `.gitea/workflows/review-verdict.yml`, **except** when they touch `.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`. See `docs/ci-cd.md` → Review-verdict gate. -- `.husky/pre-push` → `prepush-donewhen.sh` — a fail-open backstop that blocks a direct `git push origin main` whose commits `fix #N` an issue with unticked boxes. +- `.husky/pre-push` → `prepush-donewhen.sh` — a fail-open backstop that blocks a direct `git push origin main` whose commits `fix #N` an issue with unticked boxes. **Since ersatztv#743 that push can no longer happen at all** (see below), so this hook is now belt-and-braces for a path the server refuses. -Both need Gitea read creds in the env to enforce (**`ETV_GITEA_BASICAUTH=user:pass`** or `ETV_GITEA_TOKEN`; `ETV_GITEA_URL` overrides the base). Without them the merge hook asks and the push backstop is a no-op — the gate degrades to today's manual confirmation, never a silent pass. Docs-only PRs/pushes are exempt. +**`main` is PR-only — there is no direct-push path any more (ersatztv#743, `release.main-direct-push-disabled`).** Branch protection carries `enable_push: false` **and** `block_admin_merge_override: true`: a direct `git push origin HEAD:main` is refused server-side at pre-receive for every account including a site admin, the contents API is refused too, and an admin cannot `force_merge` past a missing or red required context. This is what makes `review-verdict/h10` load-bearing rather than conventional — Gitea only evaluates `status_check_contexts` on the PR merge path, so before this the whole gate was skippable with no forgery. Practically: **every** change to `main` goes through a PR, including a one-line docs fix. Tag pushes are unaffected (separate mechanism), so the release cut is unchanged. + +Both need Gitea read creds in the env to enforce (**`ETV_GITEA_BASICAUTH=user:pass`** or `ETV_GITEA_TOKEN`; `ETV_GITEA_URL` overrides the base). Without them the merge hook asks and the push backstop is a no-op — the gate degrades to today's manual confirmation, never a silent pass. Docs-only PRs are exempt from the *review-verdict* gate; the direct-push exemption is moot now that direct pushes are refused outright. **The 7 mandatory completion steps and the `## Closing record` comment template** live in the `closing-an-issue` skill (`.claude/skills/closing-an-issue/SKILL.md`) — invoke it (or `/done`) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 285ebb12f..4d9e18439 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -787,7 +787,10 @@ entire gate was skippable with no forgery at all, which was cheaper than every r - `block_admin_merge_override: true` — without it (the default is `false`), a repo admin could `POST /pulls/{n}/merge` with `force_merge: true` and merge straight past a missing or red `review-verdict/h10`. Disabling push alone just moves the bypass from the push path to the merge - path, since `timothy` is admin and is the identity every session already uses. + path, since `timothy` is admin and is the identity every session already uses. **Source-attested, + not probed** (Gitea 1.27 `CanBypassBranchProtection`): verifying it by experiment means merging an + unreviewed PR, so the field was set rather than measured. Setting it is safe under either + semantics; re-confirming the bypass itself rides with ersatztv#747. **Operator recovery when a required context gets stuck.** `block_admin_merge_override: true` removes the "Merge (admin)" / `force_merge: true` escape that used to unstick a PR whose required context was diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index d40a1e72d..ee7d5d805 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -194,7 +194,11 @@ described as one: (`release.main-direct-push-disabled`): `main` carries `enable_push: false` *and* `block_admin_merge_override: true`, so every change reaches `main` through the PR merge path, which is the only path on which these required contexts are evaluated. What survives is the - forgery list above — those routes post a status rather than skip it, so they are still real. So + forgery list above — those routes post a status rather than skip it, so they are still real — + **plus one skip route that is not forgery at all**: a credential that can `PATCH` branch + protection can turn either field off, act, and restore it. `timothy` is a site admin, so every + session holds that capability; it is an accepted residual, recorded in + `release.main-direct-push-disabled` and `ci.actions-credential-scoping`, not a closed route. So the "careless change rather than a hostile one" posture below still describes the repo accurately — it is simply no longer *this* workflow that is the weakest link. An untrusted-contributor repo would still need the classification moved somewhere no PR can -- 2.47.3