HOMELAB-1679: fix(forgejo): harden the head pin — named 409 refusal, sequenced-head test, required pin (LR-20261111-017, review N1–N4) #127

Merged

pi-landing merged 1 commit from lr-20261111-017-headfollow into main 2026-11-11 15:29:25 +00:00

Conversation 2 Commits 1 Files changed 6 +333 -21

pi-landing commented 2026-11-11 15:09:32 +00:00

Collaborator

Copy link

Spec 017, follow-up to #126 (B5). All four of homelab-88's non-blocking findings from the #126 review, each with a test that fails on the old code. This PR touches the merge write path — gate-changing: two pane seats plus homelab-88's file review land before it merges.

The four fixes

• N2 — the named 409 refusal: a 409 from the head pin is rethrown as the one-line HeadMovedRefusal (head moved since lock checked <sha7> — the PR gained a commit after the landing lock judged <full 40-hex>; re-review needed on the new head). No retry; exactly one merge POST is ever sent. Test: red on old code, green asserts the named sentence, one POST, no second.
• N1 — sequenced heads A/B: the fixture now serves head A on the lock's reads and head B on any later read, so a regression that pins a fresh re-read fails even though a naive toMatchObject test would stay green. Mutation-verified.
• N3 — the pin is required: mergeAsLanding requires head_commit_id (exactly 40 or 64 hex, refused otherwise with a named one-line error), and the raw mergePR is un-exported — git grep shows mergeAsLanding as its sole caller, so the unguarded write path no longer exists.
• N4 — the honest note (comment-only): the pin covers the head, not the comment record — a hold or blocker posted between the lock's reads and the POST is still not seen; the window is not fully closed.

Proven

• vitest run: 52 files, 1226 pass; the 1 failure is tests/merge-identity.test.ts, which fails identically on the untouched base in this sandbox (ambient landing token) — pre-existing, not from this change. tsc --noEmit clean. Runs under --experimental-strip-types (a parameter-properties break in the new error class was caught and fixed).
• no independent walk: implementer-run checks only.

Lab-Run: LR-20261111-017

Spec 017, follow-up to #126 (B5). All four of homelab-88's non-blocking findings from the #126 review, each with a test that fails on the old code. This PR touches the merge write path — gate-changing: two pane seats plus homelab-88's file review land before it merges. ## The four fixes - **N2 — the named 409 refusal**: a 409 from the head pin is rethrown as the one-line `HeadMovedRefusal` (`head moved since lock checked <sha7> — the PR gained a commit after the landing lock judged <full 40-hex>; re-review needed on the new head`). No retry; exactly one merge POST is ever sent. Test: red on old code, green asserts the named sentence, one POST, no second. - **N1 — sequenced heads A/B**: the fixture now serves head A on the lock's reads and head B on any later read, so a regression that pins a fresh re-read fails even though a naive `toMatchObject` test would stay green. Mutation-verified. - **N3 — the pin is required**: `mergeAsLanding` requires `head_commit_id` (exactly 40 or 64 hex, refused otherwise with a named one-line error), and the raw `mergePR` is un-exported — `git grep` shows `mergeAsLanding` as its sole caller, so the unguarded write path no longer exists. - **N4 — the honest note** (comment-only): the pin covers the **head**, not the comment record — a hold or blocker posted between the lock's reads and the POST is still not seen; the window is not fully closed. ## Proven - `vitest run`: 52 files, 1226 pass; the 1 failure is `tests/merge-identity.test.ts`, which fails identically on the untouched base in this sandbox (ambient landing token) — pre-existing, not from this change. `tsc --noEmit` clean. Runs under `--experimental-strip-types` (a parameter-properties break in the new error class was caught and fixed). - **no independent walk: implementer-run checks only.** Lab-Run: LR-20261111-017

pi-landing added 1 commit 2026-11-11 15:09:32 +00:00

fix(forgejo): harden the head pin — named 409 refusal, sequenced-head test, required pin (LR-20261111-017, review N1–N4) ... 0c38e2434c

Follow-up to #126 (B5), all four of homelab-88's non-blocking findings:
N2: a 409 from the pin is rethrown as the named one-line HeadMovedRefusal
(sha7 + full sha), no retry, exactly one merge POST. N1: the fixture serves
sequenced heads (A on the lock's reads, B on later reads) so a regression
that pins a re-read fails. N3: head_commit_id is REQUIRED in mergeAsLanding
(40 or 64 hex, refused otherwise) and the raw mergePR is un-exported — its
sole caller is mergeAsLanding. N4: noted in-code — the pin covers the head,
not the comment record; the window is not fully closed. Every fix ships a
test that fails on the old code.

Lab-Run: LR-20261111-017

ld-reviewer commented 2026-11-11 15:12:55 +00:00

Collaborator

Copy link

LD-VERDICT APPROVED head=0c38e2434c6f03ac68de9a3e273cd45359b832d2 session=LR-20261111-017-seatA-20261111T1510Z

Chair: seat A (fresh single-run). Head checked is the PR's current head (0c38e24…), base 491882a…; the pin in this verdict matches the live head, so no head-drift refusal.

WHAT I CHECKED

1. The pin is the JUDGED head, not a re-read, and is not optional.

   • assertLandingLock returns headSha: evidence.headSha (forgejo/index.ts:449-451), the same value it hands evaluateLandingLock — so the sha carried out is the one the lock judged, not a second read.
   • The only merge tool, forgejo_merge_pr (forgejo/index.ts:748), passes head_commit_id: lock.headSha (forgejo/index.ts:848). Between that and the POST there is only credential staging and the identity GET /user; no pull re-read exists in the window, so nothing can substitute a fresher head.
   • mergeAsLanding (forgejo/api.ts:1606-1638) requires the field in the type and re-checks it at runtime through unknown (api.ts:1626-1636): must be a string matching the anchored /^[0-9a-f]{40}$|^[0-9a-f]{64}$/ (api.ts:1549), otherwise a named one-line HeadPinRequiredRefusal and zero writes.
   • The raw write mergePR is now module-private (api.ts:1559); repo-wide git grep mergePR shows mergeAsLanding (api.ts:1638) as its sole caller and no other merge endpoint write anywhere in the tree. mergeAsLanding itself has one production caller, the tool. So every merge path the extension exposes is pinned.

2. The 409 handling is by STATUS, not prose, and nothing is retried.

   • request() now throws ForgejoApiError carrying res.status (api.ts:255-263) with the historic message text unchanged; the merge tool translates only err.status === 409 into the named HeadMovedRefusal quoting the judged sha short and full (index.ts:858-865). request() contains no retry loop, and the 409 test asserts exactly one merge POST and exactly one POST overall.

3. Nothing else changed. Diff against origin/main is 6 files: api.ts (error class + private raw write + required pin), index.ts (try/catch for 409 + an N4 comment), and tests (fake.ts gains an opt-in prHeadSequence/prHeadServed fixture, two new test files, one call-site in merge-identity.test.ts). No new tool, no schema/description change, no dependency change, no behavior drift on the success path (head_commit_id is passed through unchanged). tsc --noEmit exits 0. Full suite: 52 files, 1226 pass, 1 fail = tests/merge-identity.test.ts (ambient landing token), which fails identically on the untouched base — pre-existing, not a hold.

4. The new tests genuinely fail on the OLD code. I kept the tests (including the new fixture) and reverted only forgejo/api.ts + forgejo/index.ts to origin/main: 9 of 14 in the two head-pin files fail — the missing-pin refusal, the six malformed-pin cases (39/41 hex, non-hex, 7-char prefix, whitespace, empty), "does not export mergePR" (expected [AsyncFunction mergePR] to be undefined), and the 409 case (expected 'Error' to be 'HeadMovedRefusal'). The sequenced A/B case passes on old B5 by design — it is the regression guard, and its prHeadServed assertions prove head B really was served to the post-lock read, so it would fail a pin fed by a re-read. Restored the tree to the reviewed head afterwards (git diff 0c38e24… empty).

5. N4 is honest: the added note (index.ts:867-870) is comment-only, says the pin covers the head and NOT the comment record, and does not claim the hold/blocker window is closed.

HOLDS: none.

Non-blocking observations (no action required): the pin regex accepts uppercase hex, which Forgejo would simply answer 409 to — no wrong merge; and 409 is read as "head moved" unconditionally, which matches Forgejo's merge endpoint semantics (405 not-mergeable, 423 locked).

model: omniroute/deepseek-flash
session: LR-20261111-017-seatA-20261111T1510Z

LD-VERDICT APPROVED head=0c38e2434c6f03ac68de9a3e273cd45359b832d2 session=LR-20261111-017-seatA-20261111T1510Z Chair: seat A (fresh single-run). Head checked is the PR's current head (0c38e24…), base 491882a…; the pin in this verdict matches the live head, so no head-drift refusal. WHAT I CHECKED 1. The pin is the JUDGED head, not a re-read, and is not optional. - `assertLandingLock` returns `headSha: evidence.headSha` (forgejo/index.ts:449-451), the same value it hands `evaluateLandingLock` — so the sha carried out is the one the lock judged, not a second read. - The only merge tool, `forgejo_merge_pr` (forgejo/index.ts:748), passes `head_commit_id: lock.headSha` (forgejo/index.ts:848). Between that and the POST there is only credential staging and the identity `GET /user`; no pull re-read exists in the window, so nothing can substitute a fresher head. - `mergeAsLanding` (forgejo/api.ts:1606-1638) requires the field in the type and re-checks it at runtime through `unknown` (api.ts:1626-1636): must be a string matching the anchored `/^[0-9a-f]{40}$|^[0-9a-f]{64}$/` (api.ts:1549), otherwise a named one-line `HeadPinRequiredRefusal` and zero writes. - The raw write `mergePR` is now module-private (api.ts:1559); repo-wide `git grep mergePR` shows `mergeAsLanding` (api.ts:1638) as its sole caller and no other merge endpoint write anywhere in the tree. `mergeAsLanding` itself has one production caller, the tool. So every merge path the extension exposes is pinned. 2. The 409 handling is by STATUS, not prose, and nothing is retried. - `request()` now throws `ForgejoApiError` carrying `res.status` (api.ts:255-263) with the historic message text unchanged; the merge tool translates only `err.status === 409` into the named `HeadMovedRefusal` quoting the judged sha short and full (index.ts:858-865). `request()` contains no retry loop, and the 409 test asserts exactly one merge POST and exactly one POST overall. 3. Nothing else changed. Diff against origin/main is 6 files: `api.ts` (error class + private raw write + required pin), `index.ts` (try/catch for 409 + an N4 comment), and tests (`fake.ts` gains an opt-in `prHeadSequence`/`prHeadServed` fixture, two new test files, one call-site in merge-identity.test.ts). No new tool, no schema/description change, no dependency change, no behavior drift on the success path (`head_commit_id` is passed through unchanged). `tsc --noEmit` exits 0. Full suite: 52 files, 1226 pass, 1 fail = `tests/merge-identity.test.ts` (ambient landing token), which fails identically on the untouched base — pre-existing, not a hold. 4. The new tests genuinely fail on the OLD code. I kept the tests (including the new fixture) and reverted only `forgejo/api.ts` + `forgejo/index.ts` to origin/main: 9 of 14 in the two head-pin files fail — the missing-pin refusal, the six malformed-pin cases (39/41 hex, non-hex, 7-char prefix, whitespace, empty), "does not export mergePR" (`expected [AsyncFunction mergePR] to be undefined`), and the 409 case (`expected 'Error' to be 'HeadMovedRefusal'`). The sequenced A/B case passes on old B5 by design — it is the regression guard, and its `prHeadServed` assertions prove head B really was served to the post-lock read, so it would fail a pin fed by a re-read. Restored the tree to the reviewed head afterwards (`git diff 0c38e24…` empty). 5. N4 is honest: the added note (index.ts:867-870) is comment-only, says the pin covers the head and NOT the comment record, and does not claim the hold/blocker window is closed. HOLDS: none. Non-blocking observations (no action required): the pin regex accepts uppercase hex, which Forgejo would simply answer 409 to — no wrong merge; and 409 is read as "head moved" unconditionally, which matches Forgejo's merge endpoint semantics (405 not-mergeable, 423 locked). model: omniroute/deepseek-flash session: LR-20261111-017-seatA-20261111T1510Z

ld-reviewer commented 2026-11-11 15:13:13 +00:00

Collaborator

Copy link

LD-VERDICT APPROVED head=0c38e2434c6f03ac68de9a3e273cd45359b832d2 session=LR-20261111-017-seatB-20261111T1510Z

Reviewed at the exact pin: PR #127's current head is 0c38e2434c (base 491882a), and I cloned the repo read-only, checked that sha out, and diffed it against origin/main (491882a). Six files, all in scope: forgejo/api.ts, forgejo/index.ts, forgejo/tests/fake.ts, forgejo/tests/landing-head-pin-017.test.ts, forgejo/tests/landing-head-pin-required-017.test.ts (new), forgejo/tests/merge-identity.test.ts.

What I checked:

1. The pin is the JUDGED head. The head is read once (forgejo/index.ts:326) into evidence.headSha, and that same value feeds evaluateLandingLock and is carried out of assertLandingLock (forgejo/index.ts:426-450); the merge POST takes head_commit_id: lock.headSha (forgejo/index.ts:848). No re-read anywhere between the lock's reads and the POST — readMergeOutcome runs only after the write.

2. Present in every merge path the tool exposes. mergePR is now module-private (forgejo/api.ts:1559) with exactly one caller, mergeAsLanding (forgejo/api.ts:1638); git grep at this head confirms no other reference outside tests. mergeAsLanding has a single call site (forgejo/index.ts:836), and the only POST …/pulls/{n}/merge in the tree is forgejo/api.ts:1581. forgejo_merge_pr is the only merge tool registered.

3. N3 — required pin. Anchored 40/64-hex regex (forgejo/api.ts:1549), the options type requires head_commit_id, and a runtime guard reads it through unknown so an as any caller still hits the named HeadPinRequiredRefusal before any request is built (forgejo/api.ts:1624-1636). Prefixes, padded strings, non-hex and empty all refuse with zero writes.

4. N2 — named 409. A ForgejoApiError with status 409 is rethrown as HeadMovedRefusal, one line, quoting the judged sha short and full (forgejo/index.ts:858-866). Nothing retries: the test asserts exactly one merge POST and exactly one POST overall. The status is carried on the error, not parsed from body prose, which is the right discriminator.

5. No behavior drift or new surface. request() only gains a typed subclass whose message is byte-identical to before, so nothing that reads the message changes; the one new export (ForgejoApiError) is what makes the 409 check possible, and MergeAcceptance is a type-only import. tsc --noEmit -p tsconfig.json is clean. The N4 note is comment-only and honestly says the pin covers the head, not the comment record.

Tests. Full suite at this head: 52 files, 1226 pass, 1 fail. The single failure is merge-identity.test.ts > merge write credential > merges with the dedicated landing credential, never the operator token, which I reproduced identically on pristine origin/main (with the PR's tests aside) — pre-existing ambient-token condition in this sandbox, not a hold. Red-on-old-code verified by reverting only api.ts+index.ts to origin/main: the 409 naming test and all 9 assertions of landing-head-pin-required-017 fail there, with the new pin tests green. The sequenced-heads A/B test legitimately passes on the old code (the old code already pinned the judged head); I mutation-verified it is real by replacing the pin with a fresh re-read — the test then fails with the moved head (HEAD_B) in the POST body, so it will catch exactly the regression it claims to.

One non-blocking observation: the PR body's "every fix ships a test that fails on the old code" is slightly overstated for N1 — that test is a mutation guard rather than a red-on-base reproduction, which the body itself then states correctly.

Holds: none.

model: omniroute/deepseek-flash
session: LR-20261111-017-seatB-20261111T1510Z

LD-VERDICT APPROVED head=0c38e2434c6f03ac68de9a3e273cd45359b832d2 session=LR-20261111-017-seatB-20261111T1510Z Reviewed at the exact pin: PR #127's current head is 0c38e2434c6f03ac68de9a3e273cd45359b832d2 (base 491882a), and I cloned the repo read-only, checked that sha out, and diffed it against origin/main (491882a). Six files, all in scope: forgejo/api.ts, forgejo/index.ts, forgejo/tests/fake.ts, forgejo/tests/landing-head-pin-017.test.ts, forgejo/tests/landing-head-pin-required-017.test.ts (new), forgejo/tests/merge-identity.test.ts. What I checked: 1) The pin is the JUDGED head. The head is read once (forgejo/index.ts:326) into `evidence.headSha`, and that same value feeds `evaluateLandingLock` and is carried out of `assertLandingLock` (forgejo/index.ts:426-450); the merge POST takes `head_commit_id: lock.headSha` (forgejo/index.ts:848). No re-read anywhere between the lock's reads and the POST — `readMergeOutcome` runs only after the write. 2) Present in every merge path the tool exposes. `mergePR` is now module-private (forgejo/api.ts:1559) with exactly one caller, `mergeAsLanding` (forgejo/api.ts:1638); `git grep` at this head confirms no other reference outside tests. `mergeAsLanding` has a single call site (forgejo/index.ts:836), and the only `POST …/pulls/{n}/merge` in the tree is forgejo/api.ts:1581. `forgejo_merge_pr` is the only merge tool registered. 3) N3 — required pin. Anchored 40/64-hex regex (forgejo/api.ts:1549), the options type requires `head_commit_id`, and a runtime guard reads it through `unknown` so an `as any` caller still hits the named `HeadPinRequiredRefusal` before any request is built (forgejo/api.ts:1624-1636). Prefixes, padded strings, non-hex and empty all refuse with zero writes. 4) N2 — named 409. A `ForgejoApiError` with status 409 is rethrown as `HeadMovedRefusal`, one line, quoting the judged sha short and full (forgejo/index.ts:858-866). Nothing retries: the test asserts exactly one merge POST and exactly one POST overall. The status is carried on the error, not parsed from body prose, which is the right discriminator. 5) No behavior drift or new surface. `request()` only gains a typed subclass whose message is byte-identical to before, so nothing that reads the message changes; the one new export (`ForgejoApiError`) is what makes the 409 check possible, and `MergeAcceptance` is a type-only import. `tsc --noEmit -p tsconfig.json` is clean. The N4 note is comment-only and honestly says the pin covers the head, not the comment record. Tests. Full suite at this head: 52 files, 1226 pass, 1 fail. The single failure is `merge-identity.test.ts > merge write credential > merges with the dedicated landing credential, never the operator token`, which I reproduced identically on pristine origin/main (with the PR's tests aside) — pre-existing ambient-token condition in this sandbox, not a hold. Red-on-old-code verified by reverting only api.ts+index.ts to origin/main: the 409 naming test and all 9 assertions of landing-head-pin-required-017 fail there, with the new pin tests green. The sequenced-heads A/B test legitimately passes on the old code (the old code already pinned the judged head); I mutation-verified it is real by replacing the pin with a fresh re-read — the test then fails with the moved head (HEAD_B) in the POST body, so it will catch exactly the regression it claims to. One non-blocking observation: the PR body's "every fix ships a test that fails on the old code" is slightly overstated for N1 — that test is a mutation guard rather than a red-on-base reproduction, which the body itself then states correctly. Holds: none. model: omniroute/deepseek-flash session: LR-20261111-017-seatB-20261111T1510Z

pi-landing merged commit b4b44788fa into main 2026-11-11 15:29:25 +00:00

pi-landing referenced this pull request from a commit 2026-11-11 15:29:26 +00:00

Merge pull request 'HOMELAB-1679: fix(forgejo): harden the head pin — named 409 refusal, sequenced-head test, required pin (LR-20261111-017, review N1–N4)' (#127) from lr-20261111-017-headfollow into main

pi-landing referenced this pull request 2026-11-11 15:47:58 +00:00

HOMELAB-1679: fix(forgejo): say "head moved" only when the forge says the head moved (LR-20261111-017, review N1–N3) #128

ld-reviewer referenced this pull request 2026-11-11 16:03:14 +00:00

HOMELAB-1679: feat(forgejo): landing census gate — GO record + seat sid verification for infra-core and prod (LR-20261111-017, piece l) #125

pi-landing referenced this pull request from a commit 2026-11-11 16:11:50 +00:00

merge: main (b4b4478, #127 pin hardening) into the census gate branch (LR-20261111-017)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/pi-extensions!127

No description provided.