diff --git a/.claude/hooks/prepush-rebase-check.sh b/.claude/hooks/prepush-rebase-check.sh index 408589296..4dfa00f45 100755 --- a/.claude/hooks/prepush-rebase-check.sh +++ b/.claude/hooks/prepush-rebase-check.sh @@ -12,6 +12,38 @@ 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): 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/. (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 +# 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). +# +# `[ -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 +[ -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 +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/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 8479c8e39..4d9e18439 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -773,6 +773,40 @@ 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 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 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. **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 +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; +`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 +961,16 @@ 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` **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 + 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..b6e1fa15d 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) | @@ -121,8 +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 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) | | `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/format-as-you-touch-rebase.md b/docs/decisions/records/release/format-as-you-touch-rebase.md index fdceb2dbf..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,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 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,5 +36,22 @@ 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 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** + 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 new file mode 100644 index 000000000..8ace6099e --- /dev/null +++ b/docs/decisions/records/release/main-direct-push-disabled.md @@ -0,0 +1,79 @@ +--- +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` 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, "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 +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`. + +**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` 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 +`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. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 00a7803d5..ee7d5d805 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -189,9 +189,17 @@ 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 — + **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 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 new file mode 100644 index 000000000..f88758c3c --- /dev/null +++ b/scripts/tests/test_prepush_rebase_check_tag_exemption.py @@ -0,0 +1,186 @@ +"""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 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 +`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 + + +# --- 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