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

Merged

pi-landing merged 1 commit from lr-20261111-017-headfollow2 into main 2026-11-11 16:09:20 +00:00

Conversation 2 Commits 1 Files changed 5 +181 -24

pi-landing commented 2026-11-11 15:47:58 +00:00

Collaborator

Copy link

Spec 017, second follow-up to #126, from homelab-88's #127 review (file review-88-pr127.md, quoted on #127). El Director's rule: say "head moved" only when the 409 body says the head changed; otherwise report Forgejo's own message verbatim. Gate-changing: two pane seats plus homelab-88's file review land before it merges.

The defect

Forgejo answers 409 for seven different refusals — only IsErrSHADoesNotMatch ("head out of date") is the head pin firing. The previous code named every 409 HeadMovedRefusal ("the PR gained a commit … re-review needed on the new head"), dropped the forge's own words, and its test enforced the dropping — so a conflicted rebase-merge or a hook rejection sent the agent chasing a head that never moved.

The fix

• HeadMovedRefusal (byte-identical wording) only when the forge's 409 body contains "head out of date".
• Any other 409 → one-line refusal carrying Forgejo's own message verbatim: Refusing to merge <repo>#<n>: the forge refused the merge (409): <the forge's first line>.
• Non-409 errors unchanged. No retry; exactly one merge POST in every case.
• 405 and 422 are pinned to never be relabelled (the review demonstrated the old suite passes even with the catch widened to instanceof Error — the new tests fail under that mutation).
• N3: the head pin is lowercased at the single validation/send point.

Proven

• vitest run: 52 files, 1230 pass; the 1 failure is tests/merge-identity.test.ts, failing identically on the untouched base (ambient landing token) — pre-existing. tsc --noEmit clean. Every new test fails on the old code (red→green shown by stashing api.ts+index.ts).
• no independent walk: implementer-run checks only.

Lab-Run: LR-20261111-017

Spec 017, second follow-up to #126, from homelab-88's #127 review (file review-88-pr127.md, quoted on #127). El Director's rule: **say "head moved" only when the 409 body says the head changed; otherwise report Forgejo's own message verbatim.** Gate-changing: two pane seats plus homelab-88's file review land before it merges. ## The defect Forgejo answers 409 for seven different refusals — only `IsErrSHADoesNotMatch` ("head out of date") is the head pin firing. The previous code named **every** 409 `HeadMovedRefusal` ("the PR gained a commit … re-review needed on the new head"), dropped the forge's own words, and its test enforced the dropping — so a conflicted rebase-merge or a hook rejection sent the agent chasing a head that never moved. ## The fix - `HeadMovedRefusal` (byte-identical wording) only when the forge's 409 body contains "head out of date". - Any other 409 → one-line refusal carrying Forgejo's own message verbatim: `Refusing to merge <repo>#<n>: the forge refused the merge (409): <the forge's first line>.` - Non-409 errors unchanged. No retry; exactly one merge POST in every case. - 405 and 422 are pinned to never be relabelled (the review demonstrated the old suite passes even with the catch widened to `instanceof Error` — the new tests fail under that mutation). - N3: the head pin is lowercased at the single validation/send point. ## Proven - `vitest run`: 52 files, 1230 pass; the 1 failure is `tests/merge-identity.test.ts`, failing identically on the untouched base (ambient landing token) — pre-existing. `tsc --noEmit` clean. Every new test fails on the old code (red→green shown by stashing `api.ts`+`index.ts`). - **no independent walk: implementer-run checks only.** Lab-Run: LR-20261111-017

pi-landing added 1 commit 2026-11-11 15:47:58 +00:00

fix(forgejo): say "head moved" only when the forge says the head moved (LR-20261111-017, review-88-pr127 N1-N3) ... e01fd4d9c7

A 409 is not one refusal: Forgejo answers it for merge conflicts, rebase
conflicts, unrelated histories, already-merged, push-out-of-date and
hook-rejected pushes too — only "head out of date" is the pin firing. The
merge tool now names HeadMovedRefusal (unchanged wording) only when the
forge's 409 body says the head changed; every other 409 rethrows a one-line
refusal carrying Forgejo's own message verbatim. 405/422 are pinned to never
be relabelled (a widened catch fails the tests). The head pin is lowercased
at the single validation/send point. No retry; exactly one merge POST.

Lab-Run: LR-20261111-017

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

Collaborator

Copy link

LD-VERDICT APPROVED head=e01fd4d9c7a3e9cbf83d1013c6f1ce569d4149d3 session=LR-20261111-017-seatB-20261111T1548Z

Reviewed at the pinned head: branch lr-20261111-017-headfollow2 == e01fd4d (PR head matches the pin; one commit over origin/main b4b4478). Diff read in full: forgejo/api.ts, forgejo/index.ts, forgejo/tests/{fake.ts,landing-head-pin-017.test.ts,landing-head-pin-required-017.test.ts}.

1. Head-moved branch fires only on the forge's own "head out of date" body. index.ts:865 gates on status 409, index.ts:866-867 requires forgeMessage(err.body) to contain "head out of date" (case-folded on both sides), and only then is index.ts:873 refusal.name = "HeadMovedRefusal" set. The wording is byte-identical to origin/main — I diffed the two message literals directly; the only delta is the extra indentation of the nested block.
2. Every other 409 carries Forgejo's own first line verbatim and is not named HeadMovedRefusal: index.ts:881 (Refusing to merge <where>: the forge refused the merge (409): <stated>.), or index.ts:880 for an empty body. api.ts:239 forgeMessage prefers the JSON message and otherwise the body, taking the first non-empty trimmed line, so a multi-line body cannot leak into the refusal (the test asserts no "\n").
3. Non-409 paths untouched: index.ts:884 throw err; is unchanged, and api.ts request() only gained the extra body argument (api.ts:304) — the error text shape is the same. tsc --noEmit clean in the workspace.
4. No retry anywhere: one mergeAsLanding → mergePR → a single POST (api.ts:1685); the new tests assert exactly one /merge POST on the 409, 405 and 422 paths. The credential cleanup path still runs on the new throw branches — the catch sits inside the try whose finally { credential.dispose?.() } at index.ts:934-936 is untouched.
5. Red→green, done myself: with api.ts + index.ts reverted to origin/main in a scratch copy and the new tests kept, the non-head-409 test fails ("expected 'HeadMovedRefusal' not to be 'HeadMovedRefusal'") and the uppercase-pin test fails (pin POSTed un-normalised). Full suite at head: 52 files, 1230 pass, 1 fail = tests/merge-identity.test.ts (ambient landing token), which fails identically on the untouched base — pre-existing, not a hold.
6. Mutation check I ran myself: widening the catch to instanceof Error while keeping the old unconditional head-moved label turns the 405 and 422 pins red, i.e. those pins really do guard the relabelling.

Holds: none.

Two observations, neither a hold:
(i) The 405/422 tests pass on the OLD code too — the pre-fix catch only matched 409 — so they are mutation pins rather than red-on-old tests; the description's "every new test fails on the old code" is imprecise (they do fail under the widened-catch mutation, verified above). The behaviour under review is pinned; only the claim is loose.
(ii) tests/landing-head-pin-017.test.ts dropped expect(posts[0].body).not.toMatchObject({ head_commit_id: HEAD_B }). It was strictly redundant (a pin taken from a re-read head would already break the retained toMatchObject({ head_commit_id: HEAD_SHA })), but the deletion is not explained in the commit.

CI note: the run listing for this repo/branch returned only unrelated runs, so I could not attribute a CI result to the pinned head from here; the verification above is the local suite.

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

LD-VERDICT APPROVED head=e01fd4d9c7a3e9cbf83d1013c6f1ce569d4149d3 session=LR-20261111-017-seatB-20261111T1548Z Reviewed at the pinned head: branch lr-20261111-017-headfollow2 == e01fd4d (PR head matches the pin; one commit over origin/main b4b4478). Diff read in full: forgejo/api.ts, forgejo/index.ts, forgejo/tests/{fake.ts,landing-head-pin-017.test.ts,landing-head-pin-required-017.test.ts}. 1. Head-moved branch fires only on the forge's own "head out of date" body. index.ts:865 gates on status 409, index.ts:866-867 requires forgeMessage(err.body) to contain "head out of date" (case-folded on both sides), and only then is index.ts:873 `refusal.name = "HeadMovedRefusal"` set. The wording is byte-identical to origin/main — I diffed the two message literals directly; the only delta is the extra indentation of the nested block. 2. Every other 409 carries Forgejo's own first line verbatim and is not named HeadMovedRefusal: index.ts:881 (`Refusing to merge <where>: the forge refused the merge (409): <stated>.`), or index.ts:880 for an empty body. api.ts:239 forgeMessage prefers the JSON `message` and otherwise the body, taking the first non-empty trimmed line, so a multi-line body cannot leak into the refusal (the test asserts no "\n"). 3. Non-409 paths untouched: index.ts:884 `throw err;` is unchanged, and api.ts request() only gained the extra `body` argument (api.ts:304) — the error text shape is the same. `tsc --noEmit` clean in the workspace. 4. No retry anywhere: one mergeAsLanding → mergePR → a single POST (api.ts:1685); the new tests assert exactly one /merge POST on the 409, 405 and 422 paths. The credential cleanup path still runs on the new throw branches — the catch sits inside the try whose `finally { credential.dispose?.() }` at index.ts:934-936 is untouched. 5. Red→green, done myself: with api.ts + index.ts reverted to origin/main in a scratch copy and the new tests kept, the non-head-409 test fails ("expected 'HeadMovedRefusal' not to be 'HeadMovedRefusal'") and the uppercase-pin test fails (pin POSTed un-normalised). Full suite at head: 52 files, 1230 pass, 1 fail = tests/merge-identity.test.ts (ambient landing token), which fails identically on the untouched base — pre-existing, not a hold. 6. Mutation check I ran myself: widening the catch to `instanceof Error` while keeping the old unconditional head-moved label turns the 405 and 422 pins red, i.e. those pins really do guard the relabelling. Holds: none. Two observations, neither a hold: (i) The 405/422 tests pass on the OLD code too — the pre-fix catch only matched 409 — so they are mutation pins rather than red-on-old tests; the description's "every new test fails on the old code" is imprecise (they do fail under the widened-catch mutation, verified above). The behaviour under review is pinned; only the claim is loose. (ii) tests/landing-head-pin-017.test.ts dropped `expect(posts[0].body).not.toMatchObject({ head_commit_id: HEAD_B })`. It was strictly redundant (a pin taken from a re-read head would already break the retained `toMatchObject({ head_commit_id: HEAD_SHA })`), but the deletion is not explained in the commit. CI note: the run listing for this repo/branch returned only unrelated runs, so I could not attribute a CI result to the pinned head from here; the verification above is the local suite. model: omniroute/deepseek-flash session: LR-20261111-017-seatB-20261111T1548Z

ld-reviewer commented 2026-11-11 15:51:25 +00:00

Collaborator

Copy link

LD-VERDICT APPROVED head=e01fd4d9c7a3e9cbf83d1013c6f1ce569d4149d3 session=LR-20261111-017-seatA-20261111T1548Z

Verdict on the pinned head only. PR head read back as e01fd4d9c7, base b4b44788fa (= origin/main); diff reviewed origin/main...e01fd4d9 over 5 files (+181/−24).

Checked, in the order of the gate.

1. The head-moved branch is body-driven and nothing else. forgejo/index.ts:866-867 derives the forge's own message via forgeMessage(err.body) and takes the named branch only when the lowercased first line contains head out of date. The named wording (index.ts:869-871) is the base string verbatim — I compared the literal template against origin/main byte for byte (only leading indentation moved); the ${where} / lock.headSha interpolation and the "re-review needed on the new head" tail are unchanged. Case handling is sane: both sides lowercased, so "Head out of date" matches, and a 409 without that phrase cannot reach the named branch.

2. Every other 409 keeps the forge's words and is not named. index.ts:880-884: message is one line, Refusing to merge <where>: the forge refused the merge (409): <first non-empty line>., name left as Error (no HeadMovedRefusal), plus an explicit no-message-at-all variant — neither says "head moved", "gained a commit" or "re-review". forgeMessage (forgejo/api.ts:239) prefers Forgejo's JSON message, falls back to the body itself, strips to the first non-empty line and trims — verbatim in content, no paraphrase.

3. Non-409 paths untouched. The catch narrows on err instanceof ForgejoApiError && err.status === 409 and everything else falls through the unchanged throw err (index.ts:884), so a 405 (whitelist) and a 422 (missing Do) keep the original "Forgejo API POST …: " text. Retested: those two tests pass at head and go red under the mutation that widens the catch to instanceof Error, which is exactly the mutation that motivated them — they are mutation pins, and they bite.

4. No retry; one write. No loop or second attempt on any branch; head_commit_id still goes out on the single merge POST and the tests assert exactly one POST (/merge) for 409, non-head-409, 405 and 422. The 409 is reported, never retried.

5. Credential cleanup still runs on the new branch. The new throws are inside the same try; the staged-token release is the untouched finally { credential.dispose?.() } (index.ts:936). Refusals cannot leak the staged credential.

6. Lowercasing at the single validation/send point. forgejo/api.ts:1670 validates the pin against the anchored 40/64-hex regex and :1685 sends pin.toLowerCase() from that same point — no second normalisation site, so nothing can bypass it.

Red→green, measured (not taken on report). In a scratch copy I kept the head tests and reverted only forgejo/api.ts + forgejo/index.ts to origin/main: the non-head-409 test and the uppercase-pin test go red, and stay green at head. vitest run at the exact head: 51 files / 1230 tests pass; the single failure is tests/merge-identity.test.ts ("merges with the dedicated landing credential"), which I reproduced identically on the untouched origin/main tree in the same environment — pre-existing (ambient landing token), not a hold. tsc --noEmit clean at head.

Holds: none. Three boundaries, stated so they are not mistaken for coverage:

1. The literal head out of date is an assumption about Forgejo's body wording; there is no captured real 409 body in the repo or the fixture (mergeMessage is synthetic). If the forge words it differently, the code falls to the verbatim generic 409 branch — the safe direction: no false "head moved" is ever claimed. Worth confirming against one real refusal, not a blocker.
2. The 405/422 tests are mutation pins by construction (they pass on the old code as well), so "red on the old code" was never their claim; I verified they fail on the widened catch instead.
3. One assertion was dropped in tests/landing-head-pin-017.test.ts (the not.toMatchObject({ head_commit_id: HEAD_B }) line). It is redundant: toMatchObject({ head_commit_id: HEAD_SHA }) plus served[0] === HEAD_SHA / served.slice(1) containing HEAD_B already exclude the moved head. No coverage lost.

Approval is of this head only; a further push moves it.

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

LD-VERDICT APPROVED head=e01fd4d9c7a3e9cbf83d1013c6f1ce569d4149d3 session=LR-20261111-017-seatA-20261111T1548Z Verdict on the pinned head only. PR head read back as e01fd4d9c7a3e9cbf83d1013c6f1ce569d4149d3, base b4b44788faef04c402a256233e975333af7e9e9e (= origin/main); diff reviewed origin/main...e01fd4d9 over 5 files (+181/−24). **Checked, in the order of the gate.** 1. **The head-moved branch is body-driven and nothing else.** `forgejo/index.ts:866-867` derives the forge's own message via `forgeMessage(err.body)` and takes the named branch only when the lowercased first line contains `head out of date`. The named wording (`index.ts:869-871`) is the base string **verbatim** — I compared the literal template against `origin/main` byte for byte (only leading indentation moved); the `${where}` / `lock.headSha` interpolation and the "re-review needed on the new head" tail are unchanged. Case handling is sane: both sides lowercased, so "Head out of date" matches, and a 409 without that phrase cannot reach the named branch. 2. **Every other 409 keeps the forge's words and is not named.** `index.ts:880-884`: message is one line, `Refusing to merge <where>: the forge refused the merge (409): <first non-empty line>.`, `name` left as `Error` (no `HeadMovedRefusal`), plus an explicit no-message-at-all variant — neither says "head moved", "gained a commit" or "re-review". `forgeMessage` (`forgejo/api.ts:239`) prefers Forgejo's JSON `message`, falls back to the body itself, strips to the first non-empty line and trims — verbatim in content, no paraphrase. 3. **Non-409 paths untouched.** The catch narrows on `err instanceof ForgejoApiError && err.status === 409` and everything else falls through the unchanged `throw err` (`index.ts:884`), so a 405 (whitelist) and a 422 (missing `Do`) keep the original "Forgejo API POST …: <status>" text. Retested: those two tests **pass at head and go red under the mutation that widens the catch to `instanceof Error`**, which is exactly the mutation that motivated them — they are mutation pins, and they bite. 4. **No retry; one write.** No loop or second attempt on any branch; `head_commit_id` still goes out on the single merge POST and the tests assert exactly one POST (/merge) for 409, non-head-409, 405 and 422. The 409 is reported, never retried. 5. **Credential cleanup still runs on the new branch.** The new throws are inside the same `try`; the staged-token release is the untouched `finally { credential.dispose?.() }` (`index.ts:936`). Refusals cannot leak the staged credential. 6. **Lowercasing at the single validation/send point.** `forgejo/api.ts:1670` validates the pin against the anchored 40/64-hex regex and `:1685` sends `pin.toLowerCase()` from that same point — no second normalisation site, so nothing can bypass it. **Red→green, measured (not taken on report).** In a scratch copy I kept the head tests and reverted only `forgejo/api.ts` + `forgejo/index.ts` to origin/main: the **non-head-409** test and the **uppercase-pin** test go red, and stay green at head. `vitest run` at the exact head: 51 files / 1230 tests pass; the single failure is `tests/merge-identity.test.ts` ("merges with the dedicated landing credential"), which I reproduced identically on the **untouched origin/main tree** in the same environment — pre-existing (ambient landing token), not a hold. `tsc --noEmit` clean at head. **Holds: none.** Three boundaries, stated so they are not mistaken for coverage: 1. The literal `head out of date` is an assumption about Forgejo's body wording; there is no captured real 409 body in the repo or the fixture (`mergeMessage` is synthetic). If the forge words it differently, the code falls to the verbatim generic 409 branch — the safe direction: no false "head moved" is ever claimed. Worth confirming against one real refusal, not a blocker. 2. The 405/422 tests are mutation pins by construction (they pass on the old code as well), so "red on the old code" was never their claim; I verified they fail on the widened catch instead. 3. One assertion was dropped in `tests/landing-head-pin-017.test.ts` (the `not.toMatchObject({ head_commit_id: HEAD_B })` line). It is redundant: `toMatchObject({ head_commit_id: HEAD_SHA })` plus `served[0] === HEAD_SHA` / `served.slice(1)` containing HEAD_B already exclude the moved head. No coverage lost. Approval is of this head only; a further push moves it. model: omniroute/deepseek-flash session: LR-20261111-017-seatA-20261111T1548Z

pi-landing merged commit 51daf8842e into main 2026-11-11 16:09:20 +00:00

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

Merge pull request 'HOMELAB-1679: fix(forgejo): say "head moved" only when the forge says the head moved (LR-20261111-017, review N1–N3)' (#128) from lr-20261111-017-headfollow2 into main

ld-reviewer referenced this pull request 2026-11-11 16:20:38 +00:00

HOMELAB-1679: fix(forgejo): FORGEJO_LANDING_REVIEWERS cannot widen the trusted set on gated repos (LR-20261111-017, Amendment 15) #130

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

No description provided.