HOMELAB-2682: fix(forgejo): session-scoped verdict withdrawal (collapse characterized, then repaired) #106

Merged

pi-landing merged 4 commits from plane/HOMELAB-2682-collapse into main 2026-11-06 09:08:50 +00:00

Conversation 5 Commits 4 Files changed 6 +836 -79

pi-landing commented 2026-11-06 08:20:44 +00:00

Collaborator

Copy link

What the collapse actually does (measured, not read)

The report — merge logs saying 2 exact-head verdict(s) share reviewer key ld-reviewer/deepseek-flash/- (#A, #B) — one stance survives for withdrawal and supersession when #A and #B declare different sessions — reproduces exactly. session: is deliberately not an axis of reviewerKey(), so every fleet verdict lands on one key and ledger.current keeps the newest row.

Where it reaches, and where it does not:

Seam	Reads	Session-scoped?
ledger.current (approvals record, passing note)	unreduced → reduced by key	shown as the collapse
dangerous panel (sessionPanel)	exactHead	yes — the panel counted both, as observed
holds / blocker-finding withdrawal	exactHead + withdrawingVerdict	intended yes
untagged supersession (supersedeUntaggedVerdicts)	exactHead	no — but the rows declare no session to compare, and it fails closed

So the reduction is display/record only. But the withdrawal was not session-scoped in both directions, and it failed open.

Failing-test evidence (test commit 6dd7b5d)

forgejo/tests/landing-session-collapse.test.ts at the test-only commit:

× refuses: a row that declares NO session does not withdraw a hold that DOES
× refuses: a session-declared approval does not withdraw an undeclared hold
Tests  2 failed | 8 passed (10)

Both shapes merged the PR:

1. hold declaring session: <A> → later approval declaring model: and no session: ⇒ withdrawn, lock passes.
2. hold declaring no session → later approval from a different session ⇒ withdrawn, lock passes. This is HOMELAB-2661's forbidden move in the mirror direction.

Root cause: withdrawingVerdict() compared reviewerKey() and asked foreignSession(), which needs both sides to declare a session. On a half-labelled stream the missing declaration was read as "same session" — the collapse of distinct sessions becoming a withdrawal. "Either side declares no session ⇒ legacy" cannot tell "same session" from "nobody said".

The fix (2315039) — session-scoped withdrawal key

withdrawalKey(v) = reviewerKey(v) + declared session   // or the bare key when the row declares none

Two rows withdraw each other only when those keys are equal:

• both declare a session ⇒ equal sessions only (self-correction preserved);
• both declare none ⇒ unchanged: every pre-ruling corpus and the legacy owner panel are byte-identical (landing-corpus, landing-model-supersede, landing-hold-forms all green untouched);
• one declares and the other does not ⇒ never. That is the only behaviour removed, it fails closed, and the refusal text names both ways out (the holder's own labelled verdict, or an explicit resolved <ID> clearance).

reviewerKey() is still not re-keyed on session, so supersession and the reduced record are untouched and the dangerous panel keeps counting over the unreduced stream.

Also corrected: the note shipped the false claim "one stance survives for withdrawal and supersession" — untrue for withdrawal since HOMELAB-2661. It now says what the reduction is (the reviewer-key record keeps the newest row) and what decides a withdrawal (the declared session, over the unreduced stream). docs/forgejo.md leg 3 carries the same wording.

Three fixtures that pinned the fail-open were rewritten with their rationale in place: the two half-labelled withdrawals in landing-hold-session, and the landing-record self-withdrawal (its hold now declares the session that takes it back, plus a companion refusal case for the unverifiable shape).

Verification

• npm run typecheck clean.
• Deployed shape (npm run test:deployed): 31/31 files loaded and passed, 813/813 tests on the pinned tree.
• No force-push; pushed as pi-landing (GET /user = pi-landing before the push).

Residual, recorded not fixed

supersedeUntaggedVerdicts still folds an untagged row onto another session's model-tagged row (HOMELAB-2411's rule). The untagged row declares no session to compare, so the fallback is the documented one; the effect on the panel is a lost seat ⇒ a refusal, never a merge (pinned in the new test file). AGENTS.md's contract sentence still says "when both verdicts declare one" — it now understates the rule and wants a one-line correction in the homelab repo.

Do not merge: two gate seats follow.

## What the collapse actually does (measured, not read) The report — merge logs saying `2 exact-head verdict(s) share reviewer key ld-reviewer/deepseek-flash/- (#A, #B) — one stance survives for withdrawal and supersession` when #A and #B declare **different** sessions — reproduces exactly. `session:` is deliberately not an axis of `reviewerKey()`, so every fleet verdict lands on one key and `ledger.current` keeps the newest row. Where it reaches, and where it does not: | Seam | Reads | Session-scoped? | |---|---|---| | `ledger.current` (approvals record, passing note) | unreduced → **reduced by key** | shown as the collapse | | dangerous panel (`sessionPanel`) | `exactHead` | **yes** — the panel counted both, as observed | | holds / blocker-finding withdrawal | `exactHead` + `withdrawingVerdict` | intended yes | | untagged supersession (`supersedeUntaggedVerdicts`) | `exactHead` | no — but the rows declare no session to compare, and it fails closed | So the *reduction* is display/record only. **But the withdrawal was not session-scoped in both directions**, and it failed **open**. ## Failing-test evidence (test commit `6dd7b5d`) `forgejo/tests/landing-session-collapse.test.ts` at the test-only commit: ``` × refuses: a row that declares NO session does not withdraw a hold that DOES × refuses: a session-declared approval does not withdraw an undeclared hold Tests 2 failed | 8 passed (10) ``` Both shapes **merged the PR**: 1. hold declaring `session: <A>` → later approval declaring `model:` and **no** `session:` ⇒ withdrawn, lock passes. 2. hold declaring no session → later approval from a **different** session ⇒ withdrawn, lock passes. This is HOMELAB-2661's forbidden move in the mirror direction. Root cause: `withdrawingVerdict()` compared `reviewerKey()` and asked `foreignSession()`, which needs **both** sides to declare a session. On a half-labelled stream the missing declaration was read as "same session" — the collapse of distinct sessions becoming a withdrawal. "Either side declares no session ⇒ legacy" cannot tell "same session" from "nobody said". ## The fix (`2315039`) — session-scoped withdrawal key ```ts withdrawalKey(v) = reviewerKey(v) + declared session // or the bare key when the row declares none ``` Two rows withdraw each other only when those keys are equal: - both declare a session ⇒ equal sessions only (self-correction preserved); - both declare none ⇒ unchanged: every pre-ruling corpus and the legacy owner panel are byte-identical (`landing-corpus`, `landing-model-supersede`, `landing-hold-forms` all green untouched); - one declares and the other does not ⇒ never. That is the only behaviour removed, it fails **closed**, and the refusal text names both ways out (the holder's own labelled verdict, or an explicit `resolved <ID>` clearance). `reviewerKey()` is still not re-keyed on `session`, so supersession and the reduced record are untouched and the dangerous panel keeps counting over the unreduced stream. Also corrected: the note shipped the false claim *"one stance survives for withdrawal and supersession"* — untrue for withdrawal since HOMELAB-2661. It now says what the reduction is (the reviewer-key record keeps the newest row) and what decides a withdrawal (the declared session, over the unreduced stream). `docs/forgejo.md` leg 3 carries the same wording. Three fixtures that pinned the fail-open were rewritten with their rationale in place: the two half-labelled withdrawals in `landing-hold-session`, and the `landing-record` self-withdrawal (its hold now declares the session that takes it back, plus a companion refusal case for the unverifiable shape). ## Verification - `npm run typecheck` clean. - Deployed shape (`npm run test:deployed`): `31/31 files loaded and passed, 813/813 tests` on the pinned tree. - No force-push; pushed as `pi-landing` (`GET /user` = `pi-landing` before the push). ## Residual, recorded not fixed `supersedeUntaggedVerdicts` still folds an untagged row onto another session's model-tagged row (HOMELAB-2411's rule). The untagged row declares no session to compare, so the fallback is the documented one; the effect on the panel is a **lost** seat ⇒ a refusal, never a merge (pinned in the new test file). AGENTS.md's contract sentence still says "when both verdicts declare one" — it now understates the rule and wants a one-line correction in the homelab repo. Do not merge: two gate seats follow.

pi-landing added 2 commits 2026-11-06 08:20:44 +00:00

HOMELAB-2682: test(forgejo): characterize verdict collapse across declared sessions ... 6dd7b5dd97

The report: merge logs say "2 exact-head verdict(s) share reviewer key
ld-reviewer/deepseek-flash/- (#A, #B) — one stance survives for withdrawal and
supersession" even when #A and #B declare DIFFERENT sessions, while the
dangerous panel still counts both.

Pinned here, as measured:

- the collapse is real and is confined to the reviewer-key RECORD
  (ledger.current): two sessions reduce to the newest row, and that is where
  the note's sentence comes from;
- the dangerous panel counts over the UNREDUCED exact-head stream by declared
  session, so it is untouched — two sessions on one key are two seats;
- holds, blocker findings and the panel are session-scoped (HOMELAB-2661);
- TWO SHAPES STILL FAIL OPEN, and this test file pins the safe reading:
  a hold that declares `session: <A>` is withdrawn by a later `model:`-only
  approval, and an undeclared hold is withdrawn by a DIFFERENT session's
  approval. Both merge the PR. The withdrawal test cannot tell "same session"
  from "no session declared" because foreignSession() needs BOTH sides declared.

The two fail-open cases are RED at this commit (2 failed | 8 passed); the fix
commit follows.

HOMELAB-2682: fix(forgejo): key the withdrawal on the declared session, not on a both-sides predicate ... 2315039708

withdrawingVerdict() compared reviewerKey() and asked foreignSession() whether
the sessions disagreed — and foreignSession() needs BOTH sides to declare one.
So on a half-labelled stream the missing declaration was read as "same
session", and the collapse of distinct sessions became a withdrawal:

  - a hold declaring `session: <A>` was withdrawn by a later approval that
    declared `model:` and no `session:` at all;
  - a hold declaring no session was withdrawn by a DIFFERENT session's
    approval — HOMELAB-2661's forbidden move in the other direction.

Both shapes merged the PR (reproduced in the previous commit's two red tests).
"Either side declares no session => legacy" is the rail that fails open, because
it cannot distinguish "same session" from "nobody said".

The rule is now stated as a KEY instead:
  withdrawalKey(v) = reviewerKey(v) + the declared session, or the bare reviewer
  key when the row declares none.
Two rows withdraw each other only when those keys are equal. Consequences:

  - two rows that each declare a session: equal sessions only (self-correction);
  - two rows that declare NO session: unchanged, so every pre-ruling corpus and
    the legacy owner panel is byte-identical;
  - a row that declares a session and a row that does not: never, which is the
    only behaviour this removes — it fails CLOSED, and the refusal text names
    both ways out (the holder's own labelled verdict, or an explicit
    `resolved <ID>` clearance).

reviewerKey() is still NOT re-keyed on session: supersession and the reduced
ledger.current record are untouched, so the reported collapse stays byte-
identical and the dangerous panel keeps counting over the unreduced stream.

Also corrected: the passing note said a collapsed key means "one stance survives
for withdrawal and supersession", which stopped being true for withdrawal at
HOMELAB-2661. It now says what the reduction is (the reviewer-key record keeps
the newest row) and what decides a withdrawal (the declared session, over the
unreduced stream). docs/forgejo.md leg 3 carries the same wording.

Three fixtures that pinned the fail-open were updated with the rationale: the
two half-labelled withdrawals in landing-hold-session, and the landing-record
self-withdrawal (its hold now declares the session that takes it back, with a
companion case for the unverifiable shape).

ld-reviewer commented 2026-11-06 08:35:26 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 2315039708: REQUEST_CHANGES

model: deepseek-flash
session: sess-2682-seat2-deepseek-4f7c1a

Independent review seat 2 of 2 (adversarial, unit streams only — no real PR was merged or held). I read the diff, reproduced the fail-open at the parent, and probed the session-label surface with my own streams. The core repair is correct and earned: the half-labelled withdrawal hole is real, it is closed at this head, and every other divergence I could construct fails closed. One finding is not a merge-safety hole but is a defect this diff creates: it strands a shape and then names an exit that does not exist. That is a gate-usability defect, and gate guidance is this PR's deliverable.

What I verified (not asserted — measured)

• Fail-open reproduced at the parent. At 6dd7b5d (test-only commit on main): 2 failed | 8 passed (10) — exactly the two cases in the PR body. At this head: 10 passed (10).
• No new fail-open anywhere. withdrawingVerdict narrows; foreignSessionVerdict widens only in the direction that keeps findings OPEN. I could not construct a stream that merges at this head and refuses at main.
• Regression: full suite 31 files / 813 tests passed; deployed shape 31/31 files loaded and passed, 813/813 tests on the pinned tree (typebox 1.3.34); tsc --noEmit clean. Targeted: landing-bold-negation (33, #105 negation corpus), landing-blockers (131), landing-corpus (31), landing-model-supersede (26), landing-hold-forms (29), verdict-694-replay (19), lock-defects-2682 (18) — 287/287.
• Fixture rewrites are honest. The three reversals each carry their rationale and the landing-record self-withdrawal now pairs with a companion refusal case. Nothing was weakened silently, and the landing-record hold was given a session: because that is what the rule now requires — I verified that is the real consequence, not a test-shaped dodge (see B1).
• Session-label games (adversarial):
  • one session id, two models: declaredLabelConflicts voids both rows before supersession, the panel and the reduction, and their withdrawalKey()s never merge (model is in reviewerKey()). Correct.
  • same session, different model, hold: the hold is NOT voided and cannot be withdrawn by the other model's row — fails closed.
  • whitespace / empty / <id> / 5-char session: does not parse, falls back to the bare reviewer key. Fails closed for the two cases the PR names, but see C2.
  • unlabelled approval then sessioned hold (both orders): the hold stands, the earlier unlabelled row is untouched by the labelled seat, and it cannot be overridden. Correct.

Blockers

• B1: P1 — the refusal text and docs/forgejo.md leg 3 name an escape the label rule deletes, and an id-less undeclared hold has no exit at all. Repro (routine diff, 3 comments): #1 REQUEST_CHANGES model: omniroute/deepseek-flash, no session: (any pre-ruling hold) → #2 APPROVED model: … session: sess-<id> (any fleet verdict on the current convention) → #3 APPROVED model: …, no session: (the holder's own undeclared word). At origin/main #3 withdraws #1 and the lock passes — that is the fail-open, correctly closed here. At this head withdrawingVerdict(#1) is undefined and #3 never reaches exactHead: declaredLabelsVoided() voids it as missing: "session". So the new sentence "a hold that declares none is cleared by an undeclared verdict" describes a row the mandatory-label rule has already deleted, and leg 3 of docs/forgejo.md states the same thing. For a hold that also names no blocker id there is no route left at all: holdStillOpen() returns true immediately when blockerIdsIn() is empty (forgejo/landing.ts:2586), so resolved <ID> cannot apply either, and the only exit is pushing a new head. That is a hard strand this PR introduces — the shape merges at main — and it is not in "Residual, recorded not fixed". Fix either way, both small: (a) let a row voided only for missing: "session" still serve as the withdrawer of a same-key undeclared hold — safe now that withdrawalKey() carries the half-labelled rule on its own (a sessioned approval still has a different key, so the hole the PR closes stays closed); or (b) drop the claim from the refusal text and leg 3, state the real route (a new head), and record the strand as residual. This is the same class as D3, which this epic already fixed for leg 4's hint — the hint must match the exit it names.

Concerns

• C1: a session: mentioned in PROSE earlier in the body beats the label line. SESSION_DECL_RE is unanchored and takes the first match anywhere in the body, so As session: sess-aaaaaa-aaaaaa already noted … followed by model: / session: sess-bbbbbb-bbbbbb parses to A, and that row then withdraws session A's hold and occupies A's panel seat. Measured: hold(A) withdrawn by that row, lock passes. Pre-existing parse, unchanged by this diff, and inside the documented "self-declaration read at face value" model — but session is now the positive authority for the withdrawal, so anchoring the label to line start is worth a ticket.
• C2: an unparseable session: degrades to the legacy bare key instead of refusing, and a blank one is worse than blank: session: followed by a newline scrapes the NEXT line's first word as the id (session: + blank line + Reviewed the delta… parses as session Reviewed). Two independent sessions on one template with a blank label then share a "session", sit in one panel group and withdraw each other. Pre-existing, and the same advice as C1: a session: line that does not parse should make the row unlabelled/reported, not silently undeclared.
• C3: session ids are compared case-sensitively, so sess-aaaaaa-aaaaaa and SESS-AAAAAA-AAAAAA are two independent sessions for the dangerous-class panel. Measured: one reviewer can satisfy the two-seat panel on a dangerous diff by changing the case of its session id across two comments. Pre-existing (sessionPanel), outside this diff, but it is a fail-open in the leg that guards dangerous merges and deserves its own ticket.
• C4: the rewritten collapse note now states exactly what I measured (the reviewer-key RECORD keeps the newest; withdrawal and clearing are per declared session over the unreduced stream) — the false "one stance survives for withdrawal" claim is gone and landing-corpus pins the new wording. No action.

Not merging, not editing any reviewer row, and not posting for anyone else. Seat 1's verdict stands on its own.

**Verdict of record — ld-reviewer — head 2315039708de04553905c1c7b7a1eeb0d36e776d: REQUEST_CHANGES** model: deepseek-flash session: sess-2682-seat2-deepseek-4f7c1a Independent review seat 2 of 2 (adversarial, unit streams only — no real PR was merged or held). I read the diff, reproduced the fail-open at the parent, and probed the session-label surface with my own streams. **The core repair is correct and earned**: the half-labelled withdrawal hole is real, it is closed at this head, and every other divergence I could construct fails *closed*. One finding is not a merge-safety hole but is a defect this diff creates: it strands a shape and then names an exit that does not exist. That is a gate-usability defect, and gate guidance is this PR's deliverable. ## What I verified (not asserted — measured) - **Fail-open reproduced at the parent.** At `6dd7b5d` (test-only commit on main): `2 failed | 8 passed (10)` — exactly the two cases in the PR body. At this head: `10 passed (10)`. - **No new fail-open anywhere.** `withdrawingVerdict` narrows; `foreignSessionVerdict` widens only in the direction that keeps findings OPEN. I could not construct a stream that merges at this head and refuses at main. - **Regression:** full suite `31 files / 813 tests passed`; deployed shape `31/31 files loaded and passed, 813/813 tests` on the pinned tree (`typebox 1.3.34`); `tsc --noEmit` clean. Targeted: `landing-bold-negation` (33, #105 negation corpus), `landing-blockers` (131), `landing-corpus` (31), `landing-model-supersede` (26), `landing-hold-forms` (29), `verdict-694-replay` (19), `lock-defects-2682` (18) — 287/287. - **Fixture rewrites are honest.** The three reversals each carry their rationale and the `landing-record` self-withdrawal now pairs with a companion refusal case. Nothing was weakened silently, and the `landing-record` hold was given a `session:` because that is what the rule now requires — I verified that is the real consequence, not a test-shaped dodge (see B1). - **Session-label games (adversarial):** - *one session id, two models*: `declaredLabelConflicts` voids both rows before supersession, the panel and the reduction, and their `withdrawalKey()`s never merge (model is in `reviewerKey()`). Correct. - *same session, different model, hold*: the hold is NOT voided and cannot be withdrawn by the other model's row — fails closed. - *whitespace / empty / `<id>` / 5-char session*: does not parse, falls back to the bare reviewer key. Fails closed for the two cases the PR names, but see C2. - *unlabelled approval then sessioned hold (both orders)*: the hold stands, the earlier unlabelled row is untouched by the labelled seat, and it cannot be overridden. Correct. ## Blockers - B1: P1 — the refusal text and `docs/forgejo.md` leg 3 name an escape the label rule deletes, and an id-less undeclared hold has no exit at all. Repro (routine diff, 3 comments): `#1` REQUEST_CHANGES `model: omniroute/deepseek-flash`, no `session:` (any pre-ruling hold) → `#2` APPROVED `model: …` `session: sess-<id>` (any fleet verdict on the current convention) → `#3` APPROVED `model: …`, no `session:` (the holder's own undeclared word). At `origin/main` `#3` withdraws `#1` and the lock passes — that is the fail-open, correctly closed here. At this head `withdrawingVerdict(#1)` is `undefined` **and `#3` never reaches `exactHead`: `declaredLabelsVoided()` voids it as `missing: "session"`**. So the new sentence "a hold that declares none is cleared by an undeclared verdict" describes a row the mandatory-label rule has already deleted, and leg 3 of `docs/forgejo.md` states the same thing. For a hold that also names no blocker id there is no route left at all: `holdStillOpen()` returns `true` immediately when `blockerIdsIn()` is empty (`forgejo/landing.ts:2586`), so `resolved <ID>` cannot apply either, and the only exit is pushing a new head. That is a hard strand this PR introduces — the shape merges at main — and it is not in "Residual, recorded not fixed". Fix either way, both small: (a) let a row voided *only* for `missing: "session"` still serve as the withdrawer of a same-key **undeclared** hold — safe now that `withdrawalKey()` carries the half-labelled rule on its own (a sessioned approval still has a different key, so the hole the PR closes stays closed); or (b) drop the claim from the refusal text and leg 3, state the real route (a new head), and record the strand as residual. This is the same class as D3, which this epic already fixed for leg 4's hint — the hint must match the exit it names. ## Concerns - C1: a `session:` mentioned in PROSE earlier in the body beats the label line. `SESSION_DECL_RE` is unanchored and takes the first match anywhere in the body, so `As session: sess-aaaaaa-aaaaaa already noted …` followed by `model:` / `session: sess-bbbbbb-bbbbbb` parses to **A**, and that row then withdraws session A's hold and occupies A's panel seat. Measured: hold(A) withdrawn by that row, lock passes. Pre-existing parse, unchanged by this diff, and inside the documented "self-declaration read at face value" model — but `session` is now the positive authority for the withdrawal, so anchoring the label to line start is worth a ticket. - C2: an unparseable `session:` degrades to the legacy bare key instead of refusing, and a blank one is worse than blank: `session:` followed by a newline scrapes the NEXT line's first word as the id (`session:` + blank line + `Reviewed the delta…` parses as session `Reviewed`). Two independent sessions on one template with a blank label then share a "session", sit in one panel group **and** withdraw each other. Pre-existing, and the same advice as C1: a `session:` line that does not parse should make the row unlabelled/reported, not silently undeclared. - C3: session ids are compared case-sensitively, so `sess-aaaaaa-aaaaaa` and `SESS-AAAAAA-AAAAAA` are **two independent sessions** for the dangerous-class panel. Measured: one reviewer can satisfy the two-seat panel on a `dangerous` diff by changing the case of its session id across two comments. Pre-existing (`sessionPanel`), outside this diff, but it is a fail-open in the leg that guards dangerous merges and deserves its own ticket. - C4: the rewritten collapse note now states exactly what I measured (the reviewer-key RECORD keeps the newest; withdrawal and clearing are per declared session over the unreduced stream) — the false "one stance survives for withdrawal" claim is gone and `landing-corpus` pins the new wording. No action. Not merging, not editing any reviewer row, and not posting for anyone else. Seat 1's verdict stands on its own.

ld-reviewer commented 2026-11-06 08:35:46 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 2315039708: APPROVE
model: deepseek-flash
session: 01a1aff4-fd67-757a-b84d-27f5c14b4d51

Gate seat 1 of 2, from primary sources: the diff 77816fa..2315039708de04553905c1c7b7a1eeb0d36e776d, the new
landing-session-collapse suite, the three rewritten fixtures, docs/forgejo.md, and
parent-vs-head behaviour measured on the mgmt VM (no implementer summary was used as a brief).

Blockers

None.

Concerns

• foreignSession() is exported but no longer has a caller anywhere in forgejo/ (grep:
  only its own doc-comment and the new test file's history). Its doc now says it is "NOT the
  predicate the withdrawal is decided on", which is the right warning, but an exported
  both-sides predicate that reads a half-labelled stream as "same session" is exactly the
  shape that failed open here — the next caller to reach for it reintroduces the hole.
  Removing or un-exporting it, or folding the two call sites (withdrawingVerdict,
  foreignSessionVerdict) onto withdrawalKey() only, would close that door.
• The hold refusal's first sentence still reads "from the SAME reviewer key ... AND the same
  declared review session — or a blocker-resolution statement — clears a hold", while the
  next sentence grants that a hold declaring NO session is cleared by an undeclared verdict.
  The pair is correct and ordered, but a reader who stops at line one will think an
  undeclared hold is unclearable.
• Residual recorded in the PR body is accurate and out of this repo: AGENTS.md's
  "when both verdicts declare one" now understates the rule and wants a one-line correction
  in the homelab repo. supersedeUntaggedVerdicts cannot fold a HOLD (it skips
  polarity === "hold"), so its residual really is "a lost seat ⇒ a refusal", as claimed.

What I verified

1. The failing-test evidence reproduces at the parent and passes at the head.
The characterization file is byte-identical on both sides of the fix (git diff --stat 6dd7b5d 2315039708de04553905c1c7b7a1eeb0d36e776d touches no landing-session-collapse file), so the same assertions decide both.
At 6dd7b5d: 2 failed | 8 passed (10) — exactly "a row that declares NO session does not
withdraw a hold that DOES" and "a session-declared approval does not withdraw an undeclared
hold". At the head: 10 passed (10).
It was a merge, not a unit curiosity: feeding both shapes to evaluateLandingLock at the
parent returns ok: true (merge), and at the head both refuse with "1 exact-head hold(s) are
open". So the ticket's reading — a fail-open hole, not a display artifact — is correct.

2. Withdrawal is scoped to key+model+session, both directions. Verified by reading
withdrawalKey()/withdrawingVerdict() and by 10 probes I wrote independently of the
suite, run at BOTH commits: same-session self-correction still withdraws; cross-session does
not; earlier rows never withdraw; declared-hold/undeclared-approval and
undeclared-hold/declared-approval both refuse (these two are the only probes whose outcome
differs from the parent, which is the "only behaviour removed" claim holding); a hold from B
against an approval from A stands and refuses, citing its own id; with three rows
(hold A, approve B, approve A) A's later approval withdraws A's hold while B stays an
independent seat, so the record collapse is display-only for the decision. Leg 4 follows the
same key: a declared hold's B1 survives a foreign approval and clears on its own session's
verdict.

3. Unlabelled verdicts are not orphaned. withdrawalKey() falls back to the bare
reviewer key when a row declares no session, so two undeclared rows still withdraw each other
(probe, plus landing-hold-session's legacy case). No recorded real stream regressed: the
corpora suites (landing-corpus, landing-hold-forms, landing-model-supersede,
landing-fleet-verdict, landing-pr70-reviews, verdict-694-replay) are green and the only
corpus change is the note sentence; the three flipped fixtures are synthetic shapes that
pinned the fail-open, and each flip is a tightening (merge → refusal), not a loosened
assertion.

4. The dangerous panel's session counting is untouched. reviewerKey() is not re-keyed on
session and sessionPanel() is not in the diff: two sessions on one key still count as two
seats (probe, dangerous PR), one session twice still refuses "1 independent review
session(s)", and the untagged-supersession residual still only ever loses a seat.

5. No change to finding closure, holds from non-reviewers, or the census. The diff touches
no findOpenBlockers, no trusted/untrusted split and no census path; the widened
foreignSessionVerdict() only moves leg 4 from "half-labelled ⇒ treated as withdrawn" to
"half-labelled ⇒ findings stay open", i.e. it fails closed in the same direction as the hold
leg, as its comment states.

6. Suite green in the deployed shape, at this head. npm run test:deployed:
31/31 files loaded and passed, 813/813 tests on the pinned tree (typebox 1.3.34);
tsc --noEmit clean; CI on 2315039708 green (run #326/#327, every leg).

**Verdict of record — ld-reviewer — head 2315039708de04553905c1c7b7a1eeb0d36e776d: APPROVE** model: deepseek-flash session: 01a1aff4-fd67-757a-b84d-27f5c14b4d51 Gate seat 1 of 2, from primary sources: the diff `77816fa..2315039708de04553905c1c7b7a1eeb0d36e776d`, the new `landing-session-collapse` suite, the three rewritten fixtures, `docs/forgejo.md`, and parent-vs-head behaviour measured on the mgmt VM (no implementer summary was used as a brief). ## Blockers None. ## Concerns - `foreignSession()` is exported but no longer has a caller anywhere in `forgejo/` (grep: only its own doc-comment and the new test file's history). Its doc now says it is "NOT the predicate the withdrawal is decided on", which is the right warning, but an exported both-sides predicate that reads a half-labelled stream as "same session" is exactly the shape that failed open here — the next caller to reach for it reintroduces the hole. Removing or un-exporting it, or folding the two call sites (`withdrawingVerdict`, `foreignSessionVerdict`) onto `withdrawalKey()` only, would close that door. - The hold refusal's first sentence still reads "from the SAME reviewer key ... AND the same declared review session — or a blocker-resolution statement — clears a hold", while the next sentence grants that a hold declaring NO session is cleared by an undeclared verdict. The pair is correct and ordered, but a reader who stops at line one will think an undeclared hold is unclearable. - Residual recorded in the PR body is accurate and out of this repo: `AGENTS.md`'s "when both verdicts declare one" now understates the rule and wants a one-line correction in the homelab repo. `supersedeUntaggedVerdicts` cannot fold a HOLD (it skips `polarity === "hold"`), so its residual really is "a lost seat ⇒ a refusal", as claimed. ## What I verified **1. The failing-test evidence reproduces at the parent and passes at the head.** The characterization file is byte-identical on both sides of the fix (`git diff --stat 6dd7b5d 2315039708de04553905c1c7b7a1eeb0d36e776d` touches no `landing-session-collapse` file), so the same assertions decide both. At `6dd7b5d`: `2 failed | 8 passed (10)` — exactly "a row that declares NO session does not withdraw a hold that DOES" and "a session-declared approval does not withdraw an undeclared hold". At the head: `10 passed (10)`. It was a merge, not a unit curiosity: feeding both shapes to `evaluateLandingLock` at the parent returns `ok: true` (merge), and at the head both refuse with "1 exact-head hold(s) are open". So the ticket's reading — a fail-open hole, not a display artifact — is correct. **2. Withdrawal is scoped to key+model+session, both directions.** Verified by reading `withdrawalKey()`/`withdrawingVerdict()` and by 10 probes I wrote independently of the suite, run at BOTH commits: same-session self-correction still withdraws; cross-session does not; earlier rows never withdraw; declared-hold/undeclared-approval and undeclared-hold/declared-approval both refuse (these two are the only probes whose outcome differs from the parent, which is the "only behaviour removed" claim holding); a hold from B against an approval from A stands and refuses, citing its own id; with three rows (hold A, approve B, approve A) A's later approval withdraws A's hold while B stays an independent seat, so the record collapse is display-only for the decision. Leg 4 follows the same key: a declared hold's `B1` survives a foreign approval and clears on its own session's verdict. **3. Unlabelled verdicts are not orphaned.** `withdrawalKey()` falls back to the bare reviewer key when a row declares no session, so two undeclared rows still withdraw each other (probe, plus `landing-hold-session`'s legacy case). No recorded real stream regressed: the corpora suites (`landing-corpus`, `landing-hold-forms`, `landing-model-supersede`, `landing-fleet-verdict`, `landing-pr70-reviews`, `verdict-694-replay`) are green and the only corpus change is the note sentence; the three flipped fixtures are synthetic shapes that pinned the fail-open, and each flip is a tightening (merge → refusal), not a loosened assertion. **4. The dangerous panel's session counting is untouched.** `reviewerKey()` is not re-keyed on `session` and `sessionPanel()` is not in the diff: two sessions on one key still count as two seats (probe, dangerous PR), one session twice still refuses "1 independent review session(s)", and the untagged-supersession residual still only ever loses a seat. **5. No change to finding closure, holds from non-reviewers, or the census.** The diff touches no `findOpenBlockers`, no trusted/untrusted split and no census path; the widened `foreignSessionVerdict()` only moves leg 4 from "half-labelled ⇒ treated as withdrawn" to "half-labelled ⇒ findings stay open", i.e. it fails closed in the same direction as the hold leg, as its comment states. **6. Suite green in the deployed shape, at this head.** `npm run test:deployed`: `31/31 files loaded and passed, 813/813 tests` on the pinned tree (typebox 1.3.34); `tsc --noEmit` clean; CI on 2315039708de04553905c1c7b7a1eeb0d36e776d green (run #326/#327, every leg).

pi-landing added 2 commits 2026-11-06 08:51:30 +00:00

HOMELAB-2682: merge origin/main into plane/HOMELAB-2682-collapse 1e01e5f2a5

HOMELAB-2682: fix(forgego): sessioned word withdraws unlabeled holds + session label hardening 7b2539c13a

pi-landing commented 2026-11-06 08:52:55 +00:00

Author

Collaborator

Copy link

Fix-worker report — head 7b2539c13a28ea67bb3b553846ba75e4269b188c (not a verdict; this is the fix worker, not a review seat).

origin/main was merged in (1e01e5f, no rebase, no conflicts) before the fix, so this head carries the #105 negation work too. B1 fixed; C2 and C3 fixed; C1 left as-is per the ruling.

B1 — the strand, and the exit it names

withdrawingVerdict() now has two branches instead of one key comparison:

• a hold that DECLARES a session → withdrawn only by the same reviewer key declaring the SAME session (unchanged, so the half-labelled hole this PR closed stays closed);
• a hold that declares NO session → withdrawn by the same reviewer key's later exact-head verdict whatever that verdict declares.

Why the second branch is the fix and not a re-opening of the hole: the shape had no exit at all. Once any approval declares a session the convention is in force, so the holder's own UNDECLARED verdict is voided as missing: "session" and can never be the withdrawal — the old refusal text named exactly that deleted row. And for an id-less hold holdStillOpen() returns true unconditionally, so resolved <ID> cannot apply either. The only remaining cure was pushing a new head. An undeclared hold names no session, so it cannot be attributed to another seat either; the reviewer key's latest word is the reading left (#694).

I considered the alternative shape (letting a row voided only for missing: "session" act as the withdrawer) and rejected it: it still strands the two-row stream (unlabeled hold + one sessioned approval), because there is no voided row to use — and the exit it relies on is the very verdict shape the label rule voids.

Pinned in landing-hold-session.test.ts (fail at parent 6dd7b5d, pass at this head):

• WITHDRAWS when only the WITHDRAWING row declares a session — the reversal, with its rationale written out;
• an id-less undeclared hold has exactly one exit, and it works — clears on the holder's own later verdict, and with no later verdict the refusal contains "the ONLY clearance there is" and not the deleted "cleared by an undeclared verdict";
• keeps the strand unreachable when the only later row is the voided undeclared one — the exact 3-comment repro (#1 undeclared hold, #2 sessioned approval, #3 voided undeclared approval): exactHead = [#1, #2], #3 reported as unlabelled / missing: "session", withdrawingVerdict(#1) = #2, lock passes.

Refusal text and docs/forgejo.md leg 3 now state this, and the two hold-refusal sentences were reordered so the first one no longer reads as "an undeclared hold is unclearable" (seat 1's concern). The label-voided rows are now named in the hold refusal too, not only in the no-approval refusal — that is where a reviewer's own clearance row disappears.

C2 — a session: label nobody can read is a defect (refusal direction)

declaredSession() reads the value on the same line only ([ \t]*, not \s*), so session: + blank + Reviewed the delta… no longer parses as session Reviewed. A row that writes a session: label with a blank, placeholder (<id>) or prose value is now unpinned evidence: voided as unlabelled with malformed: true, named in the refusal ("declares a session: label with a blank or unparseable value"), not a panel seat, and it withdraws nothing. Holds are never voided, so a hold with a broken label keeps refusing.

Pinned: does not scrape the next line's first word, refuses to count two template seats that both left the label blank, does not let a blank-label row withdraw a hold, keeps a placeholder label out of the grammar. All four fail at the parent (the first fails because it parsed Reviewed), all four pass here — including the cross-withdraw the two-seat panel was exposed to.

C3 — session ids compare case-insensitively

Normalised at every seat of the decision: withdrawalKey(), sessionPanel() (group key — a case flip is one seat), declaredLabelConflicts() (a contradiction cannot hide behind a case flip), and foreignSession(). Pinned: counts a case-flipped panel attempt as ONE session and refuses the dangerous class (dangerous diff, sess-case-0001 + SESS-CASE-0001 → 1 independent review session(s), refused), keys the withdrawal on the normalised id (a case flip is the reviewer correcting itself; a genuinely different session still withdraws nothing), detects a declared-label conflict across a case flip.

C1 — left as-is, deliberately

A session: mentioned in PROSE still beats the label line (SESSION_DECL_RE unanchored), so a body that mentions a session id in prose and declares another on its label line parses to the PROSE one. Not changed here: the rationale is that a misparsed or non-parsing label means "no session declared", which fails closed for the panel (the row cannot serve as a second seat) and for the withdrawal (it cannot clear another session's hold). It is a parse-ordering wart worth its own ticket, not a hole this diff opens. C4 recorded as no-action.

Evidence

• Parent vs head, same test files: the new/changed tests at parent 6dd7b5d — 11 failed | 62 passed (73) across landing-session-collapse, landing-hold-session, landing-record. At this head those three files are green.
• Full suite, this head: 33 files / 852 tests passed (813 at the previous head; the rest of the difference is the merged-in #105 files plus the new pins). tsc --noEmit clean.
• Deployed shape (npm run test:deployed), this head: 33/33 files loaded and passed, 852/852 tests, pinned tree typebox 1.3.34.

Three pre-existing expectations were reversed on purpose and each carries its rationale inline: landing-session-collapse "a session-declared approval does not withdraw an undeclared hold", landing-hold-session "refuses when only the WITHDRAWING row declares a session", landing-record "refuses to read an undeclared hold as the withdrawing session's own". All three are the same shape as B1 and all three reverse to PERMIT — that is the one behavioural widening in this diff, stated here rather than buried: a reviewer whose hold omits session: can have that hold cleared by another session's labelled approval on the same reviewer key. The alternative is a permanent strand, and the declared-session direction stays closed. A companion refusal case was added next to each reversal so the tightening that remains is pinned too.

**Fix-worker report — head `7b2539c13a28ea67bb3b553846ba75e4269b188c`** (not a verdict; this is the fix worker, not a review seat). `origin/main` was merged in (`1e01e5f`, no rebase, no conflicts) before the fix, so this head carries the #105 negation work too. B1 fixed; C2 and C3 fixed; C1 left as-is per the ruling. ## B1 — the strand, and the exit it names `withdrawingVerdict()` now has two branches instead of one key comparison: - a hold that **DECLARES** a session → withdrawn only by the same reviewer key declaring the SAME session (unchanged, so the half-labelled hole this PR closed stays closed); - a hold that declares **NO** session → withdrawn by the same reviewer key's later exact-head verdict **whatever that verdict declares**. Why the second branch is the fix and not a re-opening of the hole: the shape had no exit at all. Once any approval declares a session the convention is in force, so the holder's own UNDECLARED verdict is voided as `missing: "session"` and can never be the withdrawal — the old refusal text named exactly that deleted row. And for an id-less hold `holdStillOpen()` returns `true` unconditionally, so `resolved <ID>` cannot apply either. The only remaining cure was pushing a new head. An undeclared hold names no session, so it cannot be attributed to another seat either; the reviewer key's latest word is the reading left (#694). I considered the alternative shape (letting a row voided *only* for `missing: "session"` act as the withdrawer) and rejected it: it still strands the two-row stream (unlabeled hold + one sessioned approval), because there is no voided row to use — and the exit it relies on is the very verdict shape the label rule voids. **Pinned in `landing-hold-session.test.ts`** (fail at parent `6dd7b5d`, pass at this head): - `WITHDRAWS when only the WITHDRAWING row declares a session` — the reversal, with its rationale written out; - `an id-less undeclared hold has exactly one exit, and it works` — clears on the holder's own later verdict, and with no later verdict the refusal contains "the ONLY clearance there is" and **not** the deleted "cleared by an undeclared verdict"; - `keeps the strand unreachable when the only later row is the voided undeclared one` — the exact 3-comment repro (`#1` undeclared hold, `#2` sessioned approval, `#3` voided undeclared approval): `exactHead` = `[#1, #2]`, `#3` reported as `unlabelled` / `missing: "session"`, `withdrawingVerdict(#1) = #2`, lock passes. Refusal text and `docs/forgejo.md` leg 3 now state this, and the two hold-refusal sentences were reordered so the first one no longer reads as "an undeclared hold is unclearable" (seat 1's concern). The label-voided rows are now named in the hold refusal too, not only in the no-approval refusal — that is where a reviewer's own clearance row disappears. ## C2 — a `session:` label nobody can read is a defect (refusal direction) `declaredSession()` reads the value **on the same line only** (`[ \t]*`, not `\s*`), so `session:` + blank + `Reviewed the delta…` no longer parses as session `Reviewed`. A row that writes a `session:` label with a blank, placeholder (`<id>`) or prose value is now **unpinned evidence**: voided as `unlabelled` with `malformed: true`, named in the refusal ("declares a `session:` label with a blank or unparseable value"), not a panel seat, and it withdraws nothing. Holds are never voided, so a hold with a broken label keeps refusing. Pinned: `does not scrape the next line's first word`, `refuses to count two template seats that both left the label blank`, `does not let a blank-label row withdraw a hold`, `keeps a placeholder label out of the grammar`. All four fail at the parent (the first fails because it parsed `Reviewed`), all four pass here — including the cross-withdraw the two-seat panel was exposed to. ## C3 — session ids compare case-insensitively Normalised at every seat of the decision: `withdrawalKey()`, `sessionPanel()` (group key — a case flip is one seat), `declaredLabelConflicts()` (a contradiction cannot hide behind a case flip), and `foreignSession()`. Pinned: `counts a case-flipped panel attempt as ONE session and refuses the dangerous class` (dangerous diff, `sess-case-0001` + `SESS-CASE-0001` → `1 independent review session(s)`, refused), `keys the withdrawal on the normalised id` (a case flip is the reviewer correcting itself; a genuinely different session still withdraws nothing), `detects a declared-label conflict across a case flip`. ## C1 — left as-is, deliberately A `session:` mentioned in PROSE still beats the label line (`SESSION_DECL_RE` unanchored), so a body that mentions a session id in prose and declares another on its label line parses to the PROSE one. Not changed here: the rationale is that a misparsed or non-parsing label means "no session declared", which fails closed for the panel (the row cannot serve as a second seat) and for the withdrawal (it cannot clear another session's hold). It is a parse-ordering wart worth its own ticket, not a hole this diff opens. C4 recorded as no-action. ## Evidence - **Parent vs head, same test files:** the new/changed tests at parent `6dd7b5d` — `11 failed | 62 passed (73)` across `landing-session-collapse`, `landing-hold-session`, `landing-record`. At this head those three files are green. - **Full suite, this head:** `33 files / 852 tests passed` (813 at the previous head; the rest of the difference is the merged-in #105 files plus the new pins). `tsc --noEmit` clean. - **Deployed shape (`npm run test:deployed`), this head:** `33/33 files loaded and passed, 852/852 tests`, pinned tree `typebox 1.3.34`. Three pre-existing expectations were **reversed on purpose** and each carries its rationale inline: `landing-session-collapse` "a session-declared approval does not withdraw an undeclared hold", `landing-hold-session` "refuses when only the WITHDRAWING row declares a session", `landing-record` "refuses to read an undeclared hold as the withdrawing session's own". All three are the same shape as B1 and all three reverse to PERMIT — that is the one behavioural widening in this diff, stated here rather than buried: a reviewer whose hold omits `session:` can have that hold cleared by another session's labelled approval on the same reviewer key. The alternative is a permanent strand, and the declared-session direction stays closed. A companion refusal case was added next to each reversal so the tightening that remains is pinned too.

ld-reviewer commented 2026-11-06 09:06:15 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 7b2539c13a: APPROVE
model: deepseek-flash
session: sess-2682-r2-seat2-7b2539c

Resolutions

Verified on 7b2539c, synthetic streams only (no PR was touched), tsc --noEmit clean, suite 33 files / 852 tests green.

1. The delta closes the two half-labelled holes. Measured refusals, each of which merged at the pre-delta shape: a hold declaring session: A is NOT withdrawn by a later model:-only approval (refused); a hold declaring no session is NOT withdrawn by an approval pinned at a different commit, nor by a different declared model, nor by a different trusted reviewer identity, nor by an untrusted author. A hold declaring session: A is not withdrawn by session: B (HOMELAB-2661 corpus unchanged and green).
2. B1's strand has its exit, and the exit is session-independent. An unlabeled hold IS withdrawn by the same key's later word whatever that word declares (sessioned and unlabeled both measured, both permit), and an explicit resolution statement clears a sessioned hold in all three label states — well-formed, absent, and defective — with the refusal correctly reading "cleared by a blocker-resolution statement, and a cleared hold is not an approval". So the shape round 1 filed as stranded now has the clearance the refusal text names.
3. C2 is fail-closed where it counts. declaredSession measured over 12 shapes: blank, spaces-only, session: <id>, prose, too-short values and the next-line scrape all return defect: true with no session; tab, run of spaces, backticks, uppercase label and bullet forms all parse. A defect row is VOIDED out of the ledger — measured absent from ledger.exactHead, therefore never an approval, never a withdrawal (a defect-labelled approval does not withdraw an unlabeled hold), and named in both refusals with the reason. As a panel seat it fails closed: one real session plus one defect row is refused as "1 independent review session(s)". The old scrape (session: + newline ⇒ session Reviewed) does not reproduce.
4. C3 applies at every seam the lock decides on. withdrawalKey, foreignSession, sessionPanel grouping and declaredLabelConflicts grouping all normalise; measured: sess-abcdef / SESS-ABCDEF is ONE panel seat (dangerous panel refuses on "1 independent review session(s)") and DOES withdraw on a case flip (routine PR permits). No case-sensitive session comparison remains in landing.ts.
5. Regression beyond the delta. The fail-open I reproduced earlier — an old-head approval that incidentally names the current head sha in its prose — still does not count: it lands in neither exactHead nor the unpinned list and the lock refuses. A second hold does not launder the first (an id-less later hold still refuses), and a cross-session later hold does not withdraw a sessioned hold.

Concerns

• C1 — claim vs behaviour on a HOLD whose session: label is unreadable. VerdictRecord.sessionLabelDefect's note and docs/forgejo.md say such a label "is never read as 'declared no session'" and that "a hold with a broken label keeps refusing". Measured at this head: withdrawingVerdict keys its wide branch on !hold.session, so a REQUEST_CHANGES labelled session: <id> takes the unlabeled branch and IS withdrawn by a later approval from a DIFFERENT session (stream: defect-labelled hold #1 + approval session: sess-b… #2 ⇒ withdrawal = #2, lock permits). It also holds correctly when nothing later is posted, and the behaviour is exactly B1's adjudicated rule for a genuinely unlabeled hold, so this is not a new capability — but the two shipped sentences are false as measured, and a slip to session: <id> is the likeliest real path into that shape.
• C2 — C3 normalisation is not everywhere. forgejo/verdict.ts:326 (bodySession !== m[1]) and :335 (req.session !== bodySession) still compare session ids case-SENSITIVELY, so the composer refuses --session sess-ab… against a body writing session: SESS-AB… as "a second, different session" while the lock now treats the two as one. It refuses and never permits, but docs/forgejo.md leg 3 states the rule globally.
• C3 — forgejo/index.ts:694, the merge tool's own description, still carries the superseded predicate ("… and — when both verdicts declare one — the same session:") and says nothing about the label rule, so inside this one release the model-facing contract and docs/forgejo.md contradict each other for a sessioned hold plus an unlabeled verdict. Same defect class the PR description already flags for AGENTS.md; it lives in this repo and ships with the delta.

Blockers

None.

**Verdict of record — ld-reviewer — head 7b2539c13a28ea67bb3b553846ba75e4269b188c: APPROVE** model: deepseek-flash session: sess-2682-r2-seat2-7b2539c ## Resolutions Verified on 7b2539c, synthetic streams only (no PR was touched), `tsc --noEmit` clean, suite `33 files / 852 tests` green. 1. **The delta closes the two half-labelled holes.** Measured refusals, each of which merged at the pre-delta shape: a hold declaring `session: A` is NOT withdrawn by a later `model:`-only approval (refused); a hold declaring no session is NOT withdrawn by an approval pinned at a different commit, nor by a different declared model, nor by a different trusted reviewer identity, nor by an untrusted author. A hold declaring `session: A` is not withdrawn by `session: B` (HOMELAB-2661 corpus unchanged and green). 2. **B1's strand has its exit, and the exit is session-independent.** An unlabeled hold IS withdrawn by the same key's later word whatever that word declares (sessioned and unlabeled both measured, both permit), and an explicit resolution statement clears a sessioned hold in all three label states — well-formed, absent, and defective — with the refusal correctly reading "cleared by a blocker-resolution statement, and a cleared hold is not an approval". So the shape round 1 filed as stranded now has the clearance the refusal text names. 3. **C2 is fail-closed where it counts.** `declaredSession` measured over 12 shapes: blank, spaces-only, `session: <id>`, prose, too-short values and the next-line scrape all return `defect: true` with no session; tab, run of spaces, backticks, uppercase label and bullet forms all parse. A defect row is VOIDED out of the ledger — measured absent from `ledger.exactHead`, therefore never an approval, never a withdrawal (a defect-labelled approval does not withdraw an unlabeled hold), and named in both refusals with the reason. As a panel seat it fails closed: one real session plus one defect row is refused as "1 independent review session(s)". The old scrape (`session:` + newline ⇒ session `Reviewed`) does not reproduce. 4. **C3 applies at every seam the lock decides on.** `withdrawalKey`, `foreignSession`, `sessionPanel` grouping and `declaredLabelConflicts` grouping all normalise; measured: `sess-abcdef` / `SESS-ABCDEF` is ONE panel seat (dangerous panel refuses on "1 independent review session(s)") and DOES withdraw on a case flip (routine PR permits). No case-sensitive session comparison remains in `landing.ts`. 5. **Regression beyond the delta.** The fail-open I reproduced earlier — an old-head approval that incidentally names the current head sha in its prose — still does not count: it lands in neither `exactHead` nor the unpinned list and the lock refuses. A second hold does not launder the first (an id-less later hold still refuses), and a cross-session later hold does not withdraw a sessioned hold. ## Concerns - C1 — claim vs behaviour on a HOLD whose `session:` label is unreadable. `VerdictRecord.sessionLabelDefect`'s note and `docs/forgejo.md` say such a label "is never read as 'declared no session'" and that "a hold with a broken label keeps refusing". Measured at this head: `withdrawingVerdict` keys its wide branch on `!hold.session`, so a REQUEST_CHANGES labelled `session: <id>` takes the unlabeled branch and IS withdrawn by a later approval from a DIFFERENT session (stream: defect-labelled hold #1 + approval `session: sess-b…` #2 ⇒ withdrawal = #2, lock permits). It also holds correctly when nothing later is posted, and the behaviour is exactly B1's adjudicated rule for a genuinely unlabeled hold, so this is not a new capability — but the two shipped sentences are false as measured, and a slip to `session: <id>` is the likeliest real path into that shape. - C2 — C3 normalisation is not everywhere. `forgejo/verdict.ts:326` (`bodySession !== m[1]`) and `:335` (`req.session !== bodySession`) still compare session ids case-SENSITIVELY, so the composer refuses `--session sess-ab…` against a body writing `session: SESS-AB…` as "a second, different session" while the lock now treats the two as one. It refuses and never permits, but `docs/forgejo.md` leg 3 states the rule globally. - C3 — `forgejo/index.ts:694`, the merge tool's own description, still carries the superseded predicate ("… and — when both verdicts declare one — the same session:") and says nothing about the label rule, so inside this one release the model-facing contract and `docs/forgejo.md` contradict each other for a sessioned hold plus an unlabeled verdict. Same defect class the PR description already flags for AGENTS.md; it lives in this repo and ships with the delta. ## Blockers None.

ld-reviewer commented 2026-11-06 09:06:51 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 7b2539c13a: APPROVE

model: deepseek-flash
session: r2s1-2682-b7f4e1

Independent review seat 1 of 2, ROUND 2 (gate tooling, DeepSeek). Primary sources only: the delta 2315039..7b2539c (with the 1e01e5f main merge it carries), forgejo/landing.ts at head, docs/forgejo.md, the fixture rewrites read against their diff, and my own probes fed to the pure lock. No real PR was merged or held, and the implementer's summary was not used as the brief.

Resolutions

• resolved B1: seat 2's strand is closed and its no-exit state is unreachable. An undeclared hold now yields to the same reviewer key's later word whatever that word declares (withdrawingVerdict's second branch, landing.ts:2408), so #1 undeclared REQUEST_CHANGES with a labelled blocker, #2 a sessioned APPROVE → permits (ok: true, withdrawal resolved to #2). Both directions measured: with no later verdict from that key the hold still refuses, and the refusal now names the route that exists — "that later verdict is the ONLY clearance there is, because resolved <ID> needs an id" — and no longer contains the deleted promise "cleared by an undeclared verdict". The exact 3-row repro (#1 undeclared REQUEST_CHANGES, #2 sessioned APPROVE, #3 undeclared APPROVE) behaves as the fix worker states: #3 voided as missing: "session", exactHead = [#1, #2], withdrawal #2, permits. The half-labelled direction that must stay closed is closed: a hold that DOES declare a session is still not withdrawn by an undeclared approval, nor by a different session's, nor by the same session on a different model.
• resolved C2: a session: label nobody can read is a defect, not a silence. SESSION_DECL_RE reads with [ \t]* (landing.ts:1974), so session: + newline + Reviewed the delta… no longer parses as session Reviewed; declaredSession reports {session: undefined, defect: true} for a blank label, session: <id>, prose, and a too-short value, and the row is voided as unlabelled with malformed: true — not an approval, not a panel seat, and it withdraws nothing. Measured: two blank-label seats on a dangerous diff refuse and the refusal carries "blank or unparseable value"; a blank-label row does not withdraw a sessioned hold; a hold whose label is broken is never voided (it keeps refusing).
• resolved C3: sess-a vs SESS-A is ONE session. Normalised at all three decision points — withdrawalKey (:2357), sessionPanel's group key (:3139), declaredLabelConflicts (:2755) — and I found no session comparison anywhere in landing.ts that skips it. Measured: a case-flipped second seat refuses the dangerous panel with "1 independent review session"; the case flip is still the reviewer's own word and does withdraw its own hold; a declared-label conflict across a case flip still voids both rows; two genuinely distinct sessions still permit.

Blockers

None.

Concerns

• A hold whose session: label is present-but-unreadable is scoped silently as a legacy undeclared hold. sessionLabelDefect is consumed only by declaredLabelsVoided, which skips holds (landing.ts:2710), so the hold never appears in unlabelled and the refusal carries no label-defect line. Measured: an undeclared-session REQUEST_CHANGES with a blank session: and a labelled finding produces a refusal that names the hold and its open finding but says nothing about the malformed label (exactHead row has sessionLabelDefect: true, session: undefined), and that hold is then cleared by a later same-key verdict from a foreign session (ok: true) — where at main the scraped fake session made it effectively unclearable. That scoping follows B1's principle, so it is not a merge-safety hole, but the reviewer whose label was discarded gets no signal. docs/forgejo.md (leg 3, new paragraph) says "the refusal names the row and says why" — the row is named, the why is not. One line emitting sessionLabelDefect hold rows alongside the unlabelled lines would make the doc sentence true.
• The PR description is pre-delta and now understates the rule it ships. It still says "a row that declares a session and a row that does not never withdraw each other … That is the only behaviour removed, it fails closed", and still ends "Do not merge: two gate seats follow" (body last updated 08:52, carries no B1/C2/C3 text). At this head a hold that declares no session IS withdrawn by a sessioned row of the same key — the delta's headline change. docs/forgejo.md, the refusal text and the fixtures are all corrected; the description, which is the first thing a future reader opens, is not. Worth one edit before or at landing.
• Recorded so it is not rediscovered as a bug: declaredSession accepts a value followed by prose on the same line (session: sess-ok-123456 (seat 1 of 2) → sess-ok-123456) and a semicolon-joined same-line declaration (model: x; session: sess-inline-1234 → sess-inline-1234). Both match the stated rule ("the value must be on the same line"), and I found no way to exploit either into a second seat or a withdrawal.

What I measured (reproducible, primary)

1. The delta's own failing cases. Targeted suites at this head: landing-hold-session 20/20, landing-session-collapse 17/17, landing-record 36/36 — 73/73, including the four new C2 cases and the three new C3 cases and every B1 refusal/withdrawal pair.
2. Full suite and deployed shape. vitest run at this head: 33 files / 852 tests passed. scripts/test-in-deployed-shape.sh on the pinned tree (typebox 1.3.34, npm ci from the package lock): "33/33 files loaded and passed, 852/852 tests". tsc --noEmit clean. The 33/852 figures carry the #105 negation work the main merge brought in (31/813 at 2315039), and those suites are green here.
3. The original pins still hold. The round-1 failing pair is intact and now honest: "refuses: a row that declares NO session does not withdraw a hold that DOES" is still a refusal (landing-session-collapse.test.ts:204), and the mirror is reversed on purpose with the rationale and a companion refusal case written out (landing-hold-session.test.ts, landing-record.test.ts). Nothing was deleted silently.
4. #2661 is not re-opened. Cross-session withdrawal refused; same-session self-correction still withdraws; an explicit resolved B1 still clears across sessions (identity-gated, session-independent); a different model is a different reviewer key and withdraws nothing; the legacy owner panel unchanged. #694 supersession intact (untagged row folded onto the same author's model-tagged row, exercised in my probes).
5. Dangerous-panel counting. Two distinct sessions permit with the panel note; one session refuses (whether the single seat comes from a duplicate, a blank label, or a case flip); an untrusted identity's approval is not a seat.
6. Blast radius. The delta touches forgejo/landing.ts, docs/forgejo.md and four test files only — no .forgejo/, no deploy/, no CI definition.

Residual unchanged and still failing closed, as the body records: supersedeUntaggedVerdicts folds an untagged row onto another session's model-tagged row, and it skips holds, so its only reachable effect is a lost panel seat → a refusal.

**Verdict of record — ld-reviewer — head 7b2539c13a28ea67bb3b553846ba75e4269b188c: APPROVE** model: deepseek-flash session: r2s1-2682-b7f4e1 Independent review seat 1 of 2, ROUND 2 (gate tooling, DeepSeek). Primary sources only: the delta `2315039..7b2539c` (with the `1e01e5f` main merge it carries), `forgejo/landing.ts` at head, `docs/forgejo.md`, the fixture rewrites read against their diff, and my own probes fed to the pure lock. No real PR was merged or held, and the implementer's summary was not used as the brief. ## Resolutions - resolved B1: seat 2's strand is closed and its no-exit state is unreachable. An undeclared hold now yields to the same reviewer key's later word whatever that word declares (`withdrawingVerdict`'s second branch, `landing.ts:2408`), so `#1` undeclared REQUEST_CHANGES with a labelled blocker, `#2` a sessioned APPROVE → permits (`ok: true`, withdrawal resolved to `#2`). Both directions measured: with no later verdict from that key the hold still refuses, and the refusal now names the route that exists — "that later verdict is the ONLY clearance there is, because `resolved <ID>` needs an id" — and no longer contains the deleted promise "cleared by an undeclared verdict". The exact 3-row repro (`#1` undeclared REQUEST_CHANGES, `#2` sessioned APPROVE, `#3` undeclared APPROVE) behaves as the fix worker states: `#3` voided as `missing: "session"`, `exactHead` = `[#1, #2]`, withdrawal `#2`, permits. The half-labelled direction that must stay closed is closed: a hold that DOES declare a session is still not withdrawn by an undeclared approval, nor by a different session's, nor by the same session on a different model. - resolved C2: a `session:` label nobody can read is a defect, not a silence. `SESSION_DECL_RE` reads with `[ \t]*` (`landing.ts:1974`), so `session:` + newline + `Reviewed the delta…` no longer parses as session `Reviewed`; `declaredSession` reports `{session: undefined, defect: true}` for a blank label, `session: <id>`, prose, and a too-short value, and the row is voided as `unlabelled` with `malformed: true` — not an approval, not a panel seat, and it withdraws nothing. Measured: two blank-label seats on a dangerous diff refuse and the refusal carries "blank or unparseable value"; a blank-label row does not withdraw a sessioned hold; a hold whose label is broken is never voided (it keeps refusing). - resolved C3: `sess-a` vs `SESS-A` is ONE session. Normalised at all three decision points — `withdrawalKey` (`:2357`), `sessionPanel`'s group key (`:3139`), `declaredLabelConflicts` (`:2755`) — and I found no session comparison anywhere in `landing.ts` that skips it. Measured: a case-flipped second seat refuses the dangerous panel with "1 independent review session"; the case flip is still the reviewer's own word and does withdraw its own hold; a declared-label conflict across a case flip still voids both rows; two genuinely distinct sessions still permit. ## Blockers None. ## Concerns - A hold whose `session:` label is present-but-unreadable is scoped silently as a legacy undeclared hold. `sessionLabelDefect` is consumed only by `declaredLabelsVoided`, which skips holds (`landing.ts:2710`), so the hold never appears in `unlabelled` and the refusal carries no label-defect line. Measured: an undeclared-session REQUEST_CHANGES with a blank `session:` and a labelled finding produces a refusal that names the hold and its open finding but says nothing about the malformed label (`exactHead` row has `sessionLabelDefect: true`, `session: undefined`), and that hold is then cleared by a later same-key verdict from a foreign session (`ok: true`) — where at main the scraped fake session made it effectively unclearable. That scoping follows B1's principle, so it is not a merge-safety hole, but the reviewer whose label was discarded gets no signal. `docs/forgejo.md` (leg 3, new paragraph) says "the refusal names the row and says why" — the row is named, the why is not. One line emitting `sessionLabelDefect` hold rows alongside the `unlabelled` lines would make the doc sentence true. - The PR description is pre-delta and now understates the rule it ships. It still says "a row that declares a session and a row that does not never withdraw each other … That is the only behaviour removed, it fails closed", and still ends "Do not merge: two gate seats follow" (body last updated 08:52, carries no B1/C2/C3 text). At this head a hold that declares no session IS withdrawn by a sessioned row of the same key — the delta's headline change. `docs/forgejo.md`, the refusal text and the fixtures are all corrected; the description, which is the first thing a future reader opens, is not. Worth one edit before or at landing. - Recorded so it is not rediscovered as a bug: `declaredSession` accepts a value followed by prose on the same line (`session: sess-ok-123456 (seat 1 of 2)` → `sess-ok-123456`) and a semicolon-joined same-line declaration (`model: x; session: sess-inline-1234` → `sess-inline-1234`). Both match the stated rule ("the value must be on the same line"), and I found no way to exploit either into a second seat or a withdrawal. ## What I measured (reproducible, primary) 1. **The delta's own failing cases.** Targeted suites at this head: `landing-hold-session` 20/20, `landing-session-collapse` 17/17, `landing-record` 36/36 — 73/73, including the four new C2 cases and the three new C3 cases and every B1 refusal/withdrawal pair. 2. **Full suite and deployed shape.** `vitest run` at this head: 33 files / 852 tests passed. `scripts/test-in-deployed-shape.sh` on the pinned tree (`typebox 1.3.34`, `npm ci` from the package lock): "33/33 files loaded and passed, 852/852 tests". `tsc --noEmit` clean. The 33/852 figures carry the #105 negation work the main merge brought in (31/813 at `2315039`), and those suites are green here. 3. **The original pins still hold.** The round-1 failing pair is intact and now honest: "refuses: a row that declares NO session does not withdraw a hold that DOES" is still a refusal (`landing-session-collapse.test.ts:204`), and the mirror is reversed on purpose with the rationale and a companion refusal case written out (`landing-hold-session.test.ts`, `landing-record.test.ts`). Nothing was deleted silently. 4. **#2661 is not re-opened.** Cross-session withdrawal refused; same-session self-correction still withdraws; an explicit `resolved B1` still clears across sessions (identity-gated, session-independent); a different model is a different reviewer key and withdraws nothing; the legacy owner panel unchanged. `#694` supersession intact (untagged row folded onto the same author's model-tagged row, exercised in my probes). 5. **Dangerous-panel counting.** Two distinct sessions permit with the panel note; one session refuses (whether the single seat comes from a duplicate, a blank label, or a case flip); an untrusted identity's approval is not a seat. 6. **Blast radius.** The delta touches `forgejo/landing.ts`, `docs/forgejo.md` and four test files only — no `.forgejo/`, no `deploy/`, no CI definition. Residual unchanged and still failing closed, as the body records: `supersedeUntaggedVerdicts` folds an untagged row onto another session's model-tagged row, and it skips holds, so its only reachable effect is a lost panel seat → a refusal.

pi-landing merged commit 26b5f150e8 into main 2026-11-06 09:08:50 +00:00

pi-landing referenced this pull request from a commit 2026-11-06 09:08:51 +00:00

Merge pull request 'HOMELAB-2682: fix(forgejo): session-scoped verdict withdrawal (collapse characterized, then repaired)' (#106) from plane/HOMELAB-2682-collapse into main

aaron referenced this pull request from a commit 2026-11-06 11:02:41 +00:00

HOMELAB-2682: fix(forgejo): findings only from ## Blockers, BLOCKER <id>:, or REQUEST_CHANGES — prose and labels never parse

pi-landing referenced this pull request 2026-11-06 11:02:41 +00:00

HOMELAB-2682: fix(forgejo): findings only from ## Blockers, BLOCKER <id>:, or REQUEST_CHANGES — prose and labels never parse #108

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

No description provided.