HOMELAB-1679: fix(forgejo): pin the merge POST to the checked head — head_commit_id (LR-20261111-017, review B5) #126

Merged

pi-landing merged 1 commit from lr-20261111-017-headpin into main 2026-11-11 14:39:15 +00:00

Conversation 3 Commits 1 Files changed 3 +108 -2

pi-landing commented 2026-11-11 14:20:02 +00:00

Collaborator

Copy link

Spec 017 (l), homelab-88 review finding B5 — a live gap in today's landing tool, fixed first per El Director. This PR touches the merge write path, so it is gate-changing: two pane seats plus homelab-88's file review land before it merges.

The race

forgejo_merge_pr judged a head in assertLandingLock and then discarded it. Between that judgement and the merge POST — the lock's reads, up to 10 s of seat lookups, credential staging, the identity GET — a push can land a new head, and Forgejo would merge that unchecked head (no verdicts, no GO cover it).

The fix

assertLandingLock now returns the judged headSha, and the merge write sends head_commit_id: <judged head> in the mergeAsLanding body. Forgejo answers 409 when the live head has moved; nothing merges that the lock did not judge.

Proven

• New tests/landing-head-pin-017.test.ts: asserts the merge POST body carries head_commit_id equal to the exact head the lock verified (and keeps the pin when the caller also passes a method/branch-deletion). Both tests fail on the old code (demonstrated via git stash: 2 failed on base, 2 passed with the fix) — El Director's red-on-old-code requirement.
• vitest run: 51 files, 1214 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.
• no independent walk: implementer-run checks only.

Lab-Run: LR-20261111-017

Spec 017 (l), homelab-88 review finding **B5** — a live gap in today's landing tool, fixed first per El Director. This PR touches the merge write path, so it is gate-changing: two pane seats plus homelab-88's file review land before it merges. ## The race `forgejo_merge_pr` judged a head in `assertLandingLock` and then discarded it. Between that judgement and the merge POST — the lock's reads, up to 10 s of seat lookups, credential staging, the identity GET — a push can land a new head, and Forgejo would merge that unchecked head (no verdicts, no GO cover it). ## The fix `assertLandingLock` now returns the judged `headSha`, and the merge write sends `head_commit_id: <judged head>` in the `mergeAsLanding` body. Forgejo answers **409** when the live head has moved; nothing merges that the lock did not judge. ## Proven - New `tests/landing-head-pin-017.test.ts`: asserts the merge POST body carries `head_commit_id` equal to the exact head the lock verified (and keeps the pin when the caller also passes a method/branch-deletion). **Both tests fail on the old code** (demonstrated via `git stash`: 2 failed on base, 2 passed with the fix) — El Director's red-on-old-code requirement. - `vitest run`: 51 files, 1214 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. - **no independent walk: implementer-run checks only.** Lab-Run: LR-20261111-017

pi-landing added 1 commit 2026-11-11 14:20:02 +00:00

fix(forgejo): pin the merge POST to the checked head (LR-20261111-017, review B5) ... f903a3a808

assertLandingLock judged a head, then discarded it: everything between the
judgement and the merge POST — the lock's reads, up to 10s of seat lookups,
credential staging, the identity GET — is a window in which a push can land
a new head, and Forgejo would merge that unchecked head. The merge write now
carries head_commit_id = the judged head; Forgejo answers 409 when the live
head has moved. Homelab-88 review B5 (spec 017 (l)); tests fail on the old
code.

Lab-Run: LR-20261111-017

pi-landing commented 2026-11-11 14:20:44 +00:00

Author

Collaborator

Copy link

infra review (homelab-88), file review-88.md — the finding this PR fixes, quoted verbatim:

B5. R2 "head == PR's CURRENT head": time-of-check/time-of-use race.

• Where: forgejo_merge_pr calls mergeAsLanding(..., { Do, DeleteBranchAfterMerge }) without head_commit_id.
• Failure scenario: a worker pushes a fix after assertLandingLock reads the head and before the POST. The time between includes up to 10 s of seat lookups plus OpenBao credential staging. Forgejo then merges the new head, which has no GO and no seats.
• Fix: send head_commit_id: evidence.headSha (Forgejo returns 409 if the head moved). Add a caller test asserting the merge POST body carries the exact head. This race already existed for every leg; this spec is what makes it a hard requirement.

This PR is the quoted fix. Record note only, not a verdict — the pane seats own the verdicts.

infra review (homelab-88), file review-88.md — the finding this PR fixes, quoted verbatim: > **B5. R2 "head == PR's CURRENT head": time-of-check/time-of-use race.** > - **Where:** `forgejo_merge_pr` calls `mergeAsLanding(..., { Do, DeleteBranchAfterMerge })` without `head_commit_id`. > - **Failure scenario:** a worker pushes a fix after `assertLandingLock` reads the head and before the POST. The time between includes up to 10 s of seat lookups plus OpenBao credential staging. Forgejo then merges the new head, which has no GO and no seats. > - **Fix:** send `head_commit_id: evidence.headSha` (Forgejo returns 409 if the head moved). Add a caller test asserting the merge POST body carries the exact head. This race already existed for every leg; this spec is what makes it a hard requirement. This PR is the quoted fix. Record note only, not a verdict — the pane seats own the verdicts.

ld-reviewer commented 2026-11-11 14:23:10 +00:00

Collaborator

Copy link

LD-VERDICT APPROVED head=f903a3a80868ffb13fe17180ac440ccde21049f1 session=LR-20261111-017-seatA-20261111T1420Z

Reviewed the pinned head f903a3a (PR head read at review time matched the pin exactly). Diff vs origin/main is 3 files, +108/−2: forgejo/api.ts, forgejo/index.ts, forgejo/tests/landing-head-pin-017.test.ts.

(1) The pin is the JUDGED head, threaded, never a re-read.

• One head read feeds the judgement: pr.head.sha is captured once in collectLandingEvidence (index.ts:322) and handed to evaluateLandingLock (index.ts:427) — the same value the CI leg and the exact-head verdict ledger are measured against.
• assertLandingLock returns that same field, not a fresh fetch (index.ts:445): headSha: evidence.headSha.
• The merge body sends it (index.ts:842): head_commit_id: lock.headSha, added to the one write at api.ts:1533 (POST repos/…/pulls/{n}/merge, body = caller data verbatim). Field spelling matches Forgejo's MergePullRequestOption, so the compare-and-swap is the forge's own 409.
• Every merge path the tool exposes is covered: forgejo_merge_pr is the only write tool (index.ts:744), it calls mergeAsLanding exactly once (index.ts:830), and mergeAsLanding → mergePR is the only path to the endpoint (api.ts:1559). forgejo_landing_census issues no write. No fallback, no conditional, no disable parameter.

(2) Nothing else changed. The api.ts hunks are additive type fields plus doc comments; the index.ts hunks are the widened return type, the extra returned field, and the body field with comments. No behaviour drift, no new tool, parameter or surface. tsc --noEmit exits 0 on the pinned head.

(3) The test genuinely fails on the old code. Verified directly: with tests/landing-head-pin-017.test.ts kept and api.ts+index.ts reverted to origin/main, both tests fail (2 failed) with exactly the missing head_commit_id in the POST body; restored to the pinned head, both pass (2 passed). The assertions check the full 40-hex sha from the fake PR head, so a re-read or a truncated prefix would not satisfy them.
Full suite on the pinned head: 51 files, 1214 pass, 1 fail — tests/merge-identity.test.ts:143, the documented pre-existing ambient-landing-token failure in this sandbox, not introduced here.

Holds: none.

Observation (non-blocking, not a hold): head_commit_id is optional at the library type level (api.ts:1529 and api.ts:1561). The one production caller always supplies it, so no exposed path can omit the pin today; a future direct caller of mergeAsLanding could.

No code edited; the red-on-old-code check was a temporary local revert in my own clone, restored and confirmed clean at f903a3a before posting.

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

LD-VERDICT APPROVED head=f903a3a80868ffb13fe17180ac440ccde21049f1 session=LR-20261111-017-seatA-20261111T1420Z Reviewed the pinned head f903a3a (PR head read at review time matched the pin exactly). Diff vs origin/main is 3 files, +108/−2: forgejo/api.ts, forgejo/index.ts, forgejo/tests/landing-head-pin-017.test.ts. (1) The pin is the JUDGED head, threaded, never a re-read. - One head read feeds the judgement: `pr.head.sha` is captured once in collectLandingEvidence (index.ts:322) and handed to evaluateLandingLock (index.ts:427) — the same value the CI leg and the exact-head verdict ledger are measured against. - assertLandingLock returns that same field, not a fresh fetch (index.ts:445): `headSha: evidence.headSha`. - The merge body sends it (index.ts:842): `head_commit_id: lock.headSha`, added to the one write at api.ts:1533 (`POST repos/…/pulls/{n}/merge`, body = caller data verbatim). Field spelling matches Forgejo's MergePullRequestOption, so the compare-and-swap is the forge's own 409. - Every merge path the tool exposes is covered: `forgejo_merge_pr` is the only write tool (index.ts:744), it calls mergeAsLanding exactly once (index.ts:830), and mergeAsLanding → mergePR is the only path to the endpoint (api.ts:1559). `forgejo_landing_census` issues no write. No fallback, no conditional, no disable parameter. (2) Nothing else changed. The api.ts hunks are additive type fields plus doc comments; the index.ts hunks are the widened return type, the extra returned field, and the body field with comments. No behaviour drift, no new tool, parameter or surface. `tsc --noEmit` exits 0 on the pinned head. (3) The test genuinely fails on the old code. Verified directly: with tests/landing-head-pin-017.test.ts kept and api.ts+index.ts reverted to origin/main, both tests fail (2 failed) with exactly the missing `head_commit_id` in the POST body; restored to the pinned head, both pass (2 passed). The assertions check the full 40-hex sha from the fake PR head, so a re-read or a truncated prefix would not satisfy them. Full suite on the pinned head: 51 files, 1214 pass, 1 fail — tests/merge-identity.test.ts:143, the documented pre-existing ambient-landing-token failure in this sandbox, not introduced here. Holds: none. Observation (non-blocking, not a hold): `head_commit_id` is optional at the library type level (api.ts:1529 and api.ts:1561). The one production caller always supplies it, so no exposed path can omit the pin today; a future direct caller of mergeAsLanding could. No code edited; the red-on-old-code check was a temporary local revert in my own clone, restored and confirmed clean at f903a3a before posting. model: omniroute/deepseek-flash session: LR-20261111-017-seatA-20261111T1420Z

ld-reviewer commented 2026-11-11 14:28:16 +00:00

Collaborator

Copy link

LD-VERDICT APPROVED head=f903a3a80868ffb13fe17180ac440ccde21049f1 session=LR-20261111-017-seatB-20261111T1426Z

Judged the diff at the pinned head only (PR reports head f903a3a808, base 0c5ef847a3 — matches the pin; branch lr-20261111-017-headpin fetched, clone read-only in my own folder).

Scope: 3 files, +108/−2 — forgejo/api.ts (one optional field added to each of mergePR / mergeAsLanding's body type, comments only otherwise), forgejo/index.ts (assertLandingLock carries the judged head out; merge body sends it), one new test file. No tool parameters added, no new tool, no other behaviour touched.

The pin is the JUDGED head, not a re-read:

• One read produces the sha: collectLandingEvidence takes pr.head.sha into headSha and throws when it is empty (index.ts:317-323), so a lock cannot pass with a blank pin.
• That same value is what the lock judges: index.ts:426 passes headSha: evidence.headSha into evaluateLandingLock (CI, verdict ledger, dangerous-class, holds all evaluated against it).
• The same value is carried out after the gate passes: index.ts:441-445 returns headSha: evidence.headSha with no read in between.
• The write uses it unconditionally: index.ts:838 head_commit_id: lock.headSha, and api.ts:1533 forwards the body verbatim (data ?? {} → JSON.stringify at the request layer). No re-read of the PR head exists on the path between judgement and POST, and head_commit_id is not an exposed tool parameter, so a caller cannot override or drop it.
• Single merge path: forgejo_merge_pr is the only merge tool (index.ts:744) and api.ts:1533 is the only merge POST in the extension — the pin is on all of it.
• Failure mode is the right one: there is no catch around the write (only a finally cleanup at index.ts:887), so a 409 from a moved head propagates as a tool error instead of merging an unjudged head.

Red on old code (verified, not taken on trust): kept tests/landing-head-pin-017.test.ts, restored api.ts and index.ts to origin/main, ran the file → 2 failed (assertion shows the POST body missing head_commit_id); restored the change → 2 passed. The second test also guards that Do / DeleteBranchAfterMerge survive alongside the pin.

Suite: 51 files, 1214 passed, 1 failed — only tests/merge-identity.test.ts ("merges with the dedicated landing credential, never the operator token"), which fails identically on the untouched base in this sandbox (ambient landing token): pre-existing, not a hold. tsc --noEmit clean.

Holds: none.

Non-blocking observation (not a hold, no behavioural gap today): head_commit_id is declared optional in both body types (api.ts:1529, api.ts:1566), so the pin is enforced at the single call site rather than by the write helper — a future second caller of mergeAsLanding could omit it silently. Worth a required-field tightening if a second merge path ever appears.

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

LD-VERDICT APPROVED head=f903a3a80868ffb13fe17180ac440ccde21049f1 session=LR-20261111-017-seatB-20261111T1426Z Judged the diff at the pinned head only (PR reports head f903a3a80868ffb13fe17180ac440ccde21049f1, base 0c5ef847a37e88f1382c9f27c6cd8f384398ae8e — matches the pin; branch lr-20261111-017-headpin fetched, clone read-only in my own folder). Scope: 3 files, +108/−2 — forgejo/api.ts (one optional field added to each of mergePR / mergeAsLanding's body type, comments only otherwise), forgejo/index.ts (assertLandingLock carries the judged head out; merge body sends it), one new test file. No tool parameters added, no new tool, no other behaviour touched. The pin is the JUDGED head, not a re-read: - One read produces the sha: collectLandingEvidence takes pr.head.sha into `headSha` and throws when it is empty (index.ts:317-323), so a lock cannot pass with a blank pin. - That same value is what the lock judges: index.ts:426 passes `headSha: evidence.headSha` into evaluateLandingLock (CI, verdict ledger, dangerous-class, holds all evaluated against it). - The same value is carried out after the gate passes: index.ts:441-445 returns `headSha: evidence.headSha` with no read in between. - The write uses it unconditionally: index.ts:838 `head_commit_id: lock.headSha`, and api.ts:1533 forwards the body verbatim (`data ?? {}` → JSON.stringify at the request layer). No re-read of the PR head exists on the path between judgement and POST, and `head_commit_id` is not an exposed tool parameter, so a caller cannot override or drop it. - Single merge path: forgejo_merge_pr is the only merge tool (index.ts:744) and api.ts:1533 is the only merge POST in the extension — the pin is on all of it. - Failure mode is the right one: there is no catch around the write (only a `finally` cleanup at index.ts:887), so a 409 from a moved head propagates as a tool error instead of merging an unjudged head. Red on old code (verified, not taken on trust): kept tests/landing-head-pin-017.test.ts, restored api.ts and index.ts to origin/main, ran the file → 2 failed (assertion shows the POST body missing head_commit_id); restored the change → 2 passed. The second test also guards that Do / DeleteBranchAfterMerge survive alongside the pin. Suite: 51 files, 1214 passed, 1 failed — only tests/merge-identity.test.ts ("merges with the dedicated landing credential, never the operator token"), which fails identically on the untouched base in this sandbox (ambient landing token): pre-existing, not a hold. `tsc --noEmit` clean. Holds: none. Non-blocking observation (not a hold, no behavioural gap today): `head_commit_id` is declared optional in both body types (api.ts:1529, api.ts:1566), so the pin is enforced at the single call site rather than by the write helper — a future second caller of mergeAsLanding could omit it silently. Worth a required-field tightening if a second merge path ever appears. model: omniroute/deepseek-flash session: LR-20261111-017-seatB-20261111T1426Z

pi-landing referenced this pull request from a commit 2026-11-11 14:30:04 +00:00

feat(forgejo): the landing census gate for infra-core and prod (LR-20261111-017, Amendments 1/2/4)

pi-landing referenced this pull request 2026-11-11 14:30:19 +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 merged commit 491882a605 into main 2026-11-11 14:39:15 +00:00

pi-landing referenced this pull request from a commit 2026-11-11 14:39:17 +00:00

Merge pull request 'HOMELAB-1679: fix(forgejo): pin the merge POST to the checked head — head_commit_id (LR-20261111-017, review B5)' (#126) from lr-20261111-017-headpin into main

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

feat(forgejo): the landing census gate for infra-core and prod (LR-20261111-017, Amendments 1/2/4)

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

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

pi-landing referenced this pull request 2026-11-11 15:09:32 +00:00

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

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

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!126

No description provided.