HOMELAB-2659: fix(forgejo): dangerous class needs two independent review sessions #88
No reviewers
ld-reviewer
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Homelab/pi-extensions!88
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-2659-merge-panel"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The ruling (binding)
Overseer ruling 2026-11-04T05:10Z (ld-hypervisor), replacing the earlier per-environment design:
forgejo_merge_prstops requiring a different declared model and stops requiring a premium model.FORGEJO_DANGEROUS_PATHSis not emptied and the dangerous-class classification is not weakened.Why: the sanctioned method is one DeepSeek seat per review (
omniroute/deepseek-flash). It cannot satisfy a premium demand, so the dangerous class could not land at all. Model diversity was only ever a proxy for "a second, independent pass"; the review session is the real thing, and it is now what the lock records.The exemption, resolved explicitly and recorded
The ruling also protects two flows — "Jesús's flow and sbx-sendrast must not change, so keep them on their current policy". That exemption is OWNER-SCOPED, not a guessed list of repo names:
LandingPolicy.legacyPanelOwners: string[], default["jesus", "Sendrast"]FORGEJO_LEGACY_PANEL_OWNERS(comma-separated), beside the existingFORGEJO_DANGEROUS_PATHS/FORGEJO_PREMIUM_MODELS/FORGEJO_LANDING_REVIEWERSoverrides inresolveLandingPolicy()repoRefby its last two path segments, case-insensitive, soowner/repoandhttps://forgejo…/owner/repoagreelegacy owner-scoped panelRationale, in the code comment and in
docs/forgejo.md: the ruling names the two flows; a live Forgejo read-back names their owners (jesus: infra-core, qs_project, project_dino, hermes-automation, Transponder, dyno —Sendrast: sendrast-web, sendrast-app, api-sendrast, deploy, sendrast-firmware, firmware-lilygo, canary-flow); an owner scope covers their current and future repos without guessing individual repo names; and the default is a declaration, not an inference.What changed
forgejo/landing.tsVerdictRecord.session+SESSION_DECL_RE/SESSION_ID_RE(read anywhere in the body, likemodel:, ≥6 chars so a placeholder can never parse);repoOwner()/isLegacyPanelRepo();sessionPanel(); leg 3 split into the new rule and the untouched legacy rule; success/refusal/collapse notes follow the governing ruleforgejo/verdict.tsComposeRequest/Result.session,sessionLine(), standalonesession:lifted/refused liketier:,sessionadded to the read-back prefix proofforgejo/verdict-cli.ts--session <id>; documented in usage/--helpand the file header; no default, no env fallback; every existing guard (identity assertion, one-POST discipline, read-back) untouchedforgejo/index.tsforgejo_merge_prcontract sentence every orchestrator reads now states the new rule (census formatting untouched)docs/forgejo.mdFORGEJO_LEGACY_PANEL_OWNERS, grammar + helper docs carrysession:forgejo/tests/landing-sessions.test.ts(new)forgejo/tests/{landing-record,landing-model-supersede,landing-corpus,landing-blockers,landing-pr70-reviews,verdict-694-replay,fake}.test/tsThe load-bearing detail: sessions are counted over
ledger.exactHeadEvery fleet reviewer posts as
ld-reviewerwith the samemodel:and nolens:, so two genuinely independent sessions share onereviewerKey()andledger.currentcollapses them to the newest — a panel of one. Counting over the reduced ledger would see one session exactly where two reviewed, so the rule could never pass.sessionPanel()therefore counts the latest non-hold trusted verdict per session over the unreduced exact-head stream, and an undeclared session forms a single(unstated)group that is reported and refused, never merged into a real session.sessionis deliberately NOT added toreviewerKey(): hold-withdrawal and supersession (HOMELAB-2411, HOMELAB-2522) must stay byte-identical, and both are asserted unchanged. This is stated in a code comment so the next reader does not "fix" it.Red → green evidence
Red first, on
origin/main(346ff343) with nolanding.tschange — full log at/home/aaron/repos/homelab/working-files/homelab-2518/HOMELAB-2659-red.log:Real assertion failures (quoted from that run, not paraphrased):
permits two sessions on the SAME model with no premium seat→expected 'Refusing to merge Homelab/pi-extensions#79 …' to be undefined, because the old refusal read1 declared model(s) (omniroute/deepseek-flash) — dangerous class needs two reviewers on different modelsandno exact-head verdict came from a premium modelrefuses the same session approving twice→expected 'Refusing to merge …' to contain 'two independent sessions'refuses a single approval on a dangerous PR→expected … to contain 'two independent sessions'refuses when a panel verdict declares no session, and names the comment→expected … to contain 'do not declare \session: `'`defaults legacyPanelOwners to the two resolved owners→expected undefined to deeply equal [ 'jesus', 'Sendrast' ]carries --session as a \session:` line in the composed grammar block→expected undefined to be 'sess-helper-0001'`documents --session in its usage text→ usage text had no--sessionGreen after implementation — full log at
…/HOMELAB-2659-green.log:(
origin/mainbaseline before this branch was 17 files / 511 tests; the new file adds 26 and two suites gained cases.)The two policies, as they now read
Dangerous class, new rule (any repo whose owner is not in
legacyPanelOwners):Nothing on this path mentions premium or different models. A shared model is reported as information only:
dangerous class: two independent sessions on the head (#1 session=… model=omniroute/deepseek-flash, #2 session=… model=omniroute/deepseek-flash, both on omniroute/deepseek-flash).Dangerous class, legacy owner panel (
Sendrast/sendrast-web):session:is not required on that path.Test coverage (both policies keep real coverage — no rail deleted)
landing-sessions.test.ts(26): same-model two sessions → PERMIT; same session twice → REFUSE; one approval → REFUSE; missingsession:→ REFUSE naming the comment; missingmodel:→ REFUSE; different models, no premium → PERMIT; a session whose latest stance is a concern still counts; routine PR, one approval → PERMIT; legacy owner same-model → REFUSE; legacy two models + premium → PERMIT; legacy does not needsession:; legacy same-session-twice → REFUSE; case-insensitive + URL-shapedrepoRef;resolveLandingPolicy()default andFORGEJO_LEGACY_PANEL_OWNERS(including empty-override fallback and an override that moves Homelab into legacy and Sendrast out); session is not a reviewer-key axis; a different session cannot withdraw another key's hold; same-key withdrawal still works; untrusted identity still not evidence; red CI still refuses;--sessionlands in the composed body and nowhere else changes; placeholder session refused; theforgejo_merge_prdescription states the new rule and no longer states the old one.landing-model-supersede.test.ts: the supersession/premium rails now run on the legacy ownerSendrast/sendrast-web(that is where a model rail still means something), plus a new describe replaying the same #115 stream under the new rule with sessions — proving supersession is unchanged.landing-record.test.ts: the old describe is renamedlegacy owner panel: two declared models, one premiumand runs onSendrast/sendrast-web; the tool-level rails that must still land on a dangerous Homelab PR now declare their sessions;re-using one review id cannot mint a second approvalis rewritten to the replacement rule (a re-used review id cannot mint a second session either) — this is the one assertion of a rule that no longer exists.landing-pr70-reviews.test.tsandverdict-694-replay.test.ts: real corpora recorded before the ruling (2026-10-28 / 2026-10-30) cannot contain asession:, so they replay under the legacy owner policy through the sameFORGEJO_LEGACY_PANEL_OWNERSoverride the operator has; their subjects are the blocker/recognition rails, not panel policy.landing-corpus.test.ts,landing-blockers.test.ts,fake.ts: the collapse-advice assertions move to the new wording, and the panel fixtures that stand for a complete modern panel declare their session.Scope discipline
plane/HOMELAB-2659-merge-panel, worktree.worktrees/HOMELAB-2659-merge-panel, one commit48ed217, basemain(346ff343).~/.pi/agent/extensionswas not touched; no token or secret was put in argv.Verdict: APPROVED
model: omniroute/deepseek-flash
head:
48ed217b5csession: 01a1a528-d3c3-7446-a658-24d868d3a690
Approved at this head.
Base verified rather than taken on trust: the worktree's local
mainis a stale ref,origin/mainis346ff343and is an ancestor of the reviewed head, so I diffed against origin/main and got exactly the 13 paths the PR declares (landing.ts, verdict.ts, verdict-cli.ts, index.ts, docs/forgejo.md, the new landing-sessions suite and seven re-pointed suites). No unrelated file, no scope creep, no change to forgejo/api.ts or forgejo/types.ts.I re-ran the suite myself in the PR worktree:
npx tsc --noEmitclean,npx vitest run18 files / 540 tests passed, andgit statusclean afterwards. CI run #220 is green on this exact sha with all 14 required contexts.The load-bearing detail is implemented correctly.
sessionPanel()iteratesledger.exactHead, notledger.current, and keeps the latest trusted non-hold verdict per session, so two fleet sessions that share one reviewer key (same author, same model, no lens) are two panel members.ledger.exactHeadis chronological, becausesortPRTextsorts by timestamp ascending andsupersedeUntaggedVerdictspreserves that order, so "last write per session wins" holds. An undeclared session groups on the empty string and cannot collide with a declared one, since the declaration regex demands at least six characters; the unstated group is reported and refused, never merged into a real session. A session whose latest stance is a concern stays a panel member, which is the readingnonHoldshas always had on the legacy path.reviewerKey()is byte-identical — still author/model/lens, with the session appearing nowhere in it — so hold withdrawal and supersession keep their release semantics. The suite pins both directions: a same-key later stance still wins, and an approval on a different model cannot withdraw a hold keyed elsewhere. As a sensitivity probe I re-ran the whole suite withFORGEJO_LEGACY_PANEL_OWNERSwidened to include Homelab: exactly one test failed, the rewritten id-reuse assertion, which confirms the suites are isolated and that the assertions depending on the governing policy are not vacuous.The legacy path really is the old rule and does not demand a session declaration. On Sendrast/sendrast-web the lock refuses one model alone, refuses two approvals on the same model, refuses when nothing premium is on the head, refuses a verdict that declares no model, and permits two declared models with one premium with no session declared at all; jesus/infra-core passes with no session line either. Owner derivation takes the second-to-last path segment after stripping query, fragment, trailing slashes and a .git suffix, and lower-cases it, so owner/repo and a URL-shaped ref agree; a repo literally named after a legacy owner under a different owner resolves to that other owner and cannot mis-match, and an unparseable ref yields no owner and falls to the new rule.
FORGEJO_LEGACY_PANEL_OWNERScannot silently empty the list into a fail-open: the splitter returns null when no non-blank part survives, so blank or comma-only values fall back to the declared default, and an override matching nothing merely moves repos onto the new two-session rule.Nothing else weakened. No premium or different-model demand remains on the new path, and the tests assert its absence rather than just its irrelevance.
FORGEJO_DANGEROUS_PATHSand its default list are untouched, with forgejo/** still classed dangerous. The CI-by-name, exact-head, landing-identity, hold and open-blocker legs have no hunk in this diff, formal review state is still not evidence, and no override parameter or marker was added anywhere. The two strings every orchestrator reads state the new rule and the owner-scoped exemption accurately, and the old "two verdicts on different declared models with one premium" sentence is gone rather than merely supplemented — a test asserts its absence.The tests have teeth. The new suite asserts both directions of each bullet: permit on two same-model sessions with no premium, refuse on one approval, refuse on the same session twice, refuse on a missing session declaration while naming the comment, refuse on a missing model declaration, permit on different models with no premium, permit on a routine PR with one verdict, plus the legacy negatives, the case-insensitive and URL-shaped owner match, the env override in both directions, reviewer-key invariance, withdrawal in both directions, untrusted-identity exclusion, red CI, the helper's flag composition and read-back, placeholder refusal, and the tool-contract text. The re-pointing is honest: the legacy model rails moved onto a legacy owner instead of being deleted, the two pre-ruling corpora replay under the documented owner override with an in-file comment saying why, and the rewritten id-reuse assertion asserts the same property in the new axis — one record, not two, and no merge call. The red log is genuine: 18 failed and 7 passed, and the received strings are the real old-rule refusals ("1 declared model(s) (omniroute/deepseek-flash) — dangerous class needs two reviewers on different models" and "no exact-head verdict came from a premium model"), which is what main actually produces. The green log matches my own run.
Three observations that do not block. First, the session is an honesty declaration the lock cannot attest: one agent can satisfy the gate by writing two different session values, exactly as two different model values satisfied the old premium rule. The docs describe the comparison accurately — the two declared values must differ — but never say plainly that independence is declared rather than verified, and one sentence in the docs and in the tool description would stop a future reader believing the lock establishes it. Second, the declaration regex reads the value anywhere in the body with a fairly loose alphabet and length floor, so incidental prose such as a timestamp following the word session parses as a session id; placeholders are correctly rejected, and the realistic effect is fail-closed because two reviews sharing one incidental token collapse and refuse, but requiring a standalone line would be tighter. Third, the rewritten re-used-review-id assertion does not actually isolate id reuse, because both rows declare one session, so the panel is one member whether or not id reuse were mishandled; the same was true of the pre-change version, where both rows shared one model, so no rail was traded away, but the test name promises slightly more than the fixture proves.
No blocker open.
I never edited code, never merged, never touched another worktree or branch, never wrote to the deployed extension directory, and put no token or secret in any argv. The head had not moved when I posted.
Verdict: APPROVED
model: omniroute/deepseek-flash
head:
48ed217b5csession: 01a1a528-d3f5-7446-a658-24da622cfe49
Scope of this review
48ed217b5cagainst base346ff343e6(13 files, +1199/-108), and I re-read the PR over the API at review time: state open, head sha48ed217, unmerged, base main346ff34.The ruling's bullets, checked one by one
not dangerous class: one exact-head verdict is sufficient), and that branch is untouched by the diff.sessionPanelis the whole panel test on the new path, andpremiumModels/FORGEJO_PREMIUM_MODELSare carried unchanged (the premium list is only read by the legacy path now).sessions.length >= 2oversessionPanel(ledger), and a panel row with nosession:is its own problem ("do not declaresession: <id>, so session independence cannot be checked"), so an undeclared reviewer cannot be counted towards the two.deepseek-flashandkimi-pool, i.e. two distinct reviewer keys) still yield1 independent review session(s) (sess-aaaaaa)and refuse. That is precisely the shape the model rule would have passed and the session rule must not.reviewerKey()did NOT gainsession:landing.ts:1484is unchanged (author + model + lens), so hold withdrawal and supersession stay byte-identical.landing-sessions.test.tsasserts the #664 shape (two sessions on one key are one current stance) and the withdrawal case, and I re-probed both.FORGEJO_DANGEROUS_PATHSis untouched and still defaults to the full dangerous list; nothing in this diff empties or narrows it.Independence is counted on the unreduced stream
sessionPaneliteratesledger.exactHead(post-supersession, pre-reviewerKeyreduction), notledger.current. That is correct and load-bearing: every fleet reviewer posts asld-revieweron the same model with nolens:, so two real sessions share one reviewer key andledger.currentwould collapse them to one, and the rule could never pass if it counted the reduced set. I confirmed the two sets differ on the intended fixture (exactHead 2, current 1, panel 2 sessions).polarity === "hold"is the only excluded stance), and I checked this is not a new weakening: the legacy model count read the same non-hold set, so the reading is the one that was already in force."". Probed a mixed stream (one declaredsess-ccccccplus one untagged approval) and it refuses on both counts (#8 do not declare sessionand1 independent review session(s)).""group and the refusal reads0 independent review session(s) (none).ci-readerapproval carrying a well-formedsession:does not enter the panel (probed:1 independent review session(s) (sess-iiiiii)).Fail-open hunting
session:values from one reviewer session: this is a DECLARATION rule and it cannot detect a lie. That is exactly what the ruling asked for ("each verdict declaring model: and session:"), and it does not make the lock weaker than the old rule in a way the ruling did not request, because the old rule was equally declaration-based (twomodel:strings).extractVerdictpass, so they either declare the same session (one member) or one of them declares none (refused).landing-record.test.tskeeps that rail, now expressed as1 independent review session(s) (sess-review-927)for the re-used review id #927.supersedeUntaggedVerdictsis unchanged, and its retirements only ever remove rows from the panel (fewer sessions), never add one.SESSION_DECL_REneeds at least six characters of[\w.:-], sosession: <id>andsession: <session>do not parse (probed both, including the literal snippet printed indocs/forgejo.md), andcomposeVerdictBodyrefuses a short or<id>--session.The legacy owner-scoped path
isLegacyPanelRepo(): samemissingModel, samemodels.length < 2, samepremium.length === 0problems, same refusal prose, and nosession:demand anywhere on that path (jesus/infra-corewith two models and no session still permits).Homelab/*repo by accident:repoOwner()is an exact lower-cased compare on the penultimate path segment, soHomelab/pi-extensionsresolves tohomelabandHomelab/jesusresolves tohomelab(notjesus); a repo actually owned byjesusresolves tojesus.FORGEJO_LEGACY_PANEL_OWNERSfails closed on empty input:resolveLandingPolicyreusessplit(), which returns null for""and for" , ,", so both fall back to["jesus","Sendrast"](probed). A non-empty value can move an owner into or out of the exemption, which is the documented explicit act, not a default.repoRefresolves to the same owner the API reports:https://forgejo.aaron.reynoza.org/Sendrast/sendrast-webgivessendrast, with.gitand trailing slashes stripped.The documented contract
forgejo_merge_prtool description states the new rule, the legacy owner panel, and keeps the four unchanged guarantees (green CI, exact-head verdict COMMENT, formal state never evidence, no override parameter). The old "two verdicts on different declared models with one premium" sentence is gone, and a test asserts its absence rather than its contradiction.docs/forgejo.mdleg 4 matches the code: two exact-head verdicts, each declaringmodel:andsession:, distinct session values, no session refuses, sessions counted over the unreduced stream,sessionnot in the reviewer key, plus the owner-scoped exemption and theFORGEJO_LEGACY_PANEL_OWNERSoverride including the empty-falls-back rule.composeVerdictBody.Tests
npx tsc --noEmitclean andnpx vitest run18 files / 540 tests green in the PR worktree;git statusthere is clean before and after my run (I wrote nothing into it).tests/landing-sessions.test.ts(26 cases): each ruling bullet has a positive and a negative case, and the negatives are real — same session twice on two models refuses, undeclared session refuses by comment id, missingmodel:still refuses, a single approval refuses, a legacy owner keeps the two-model/premium rule and still refuses one model, the env override moves a repo both ways, owner matching is case-insensitive and URL-shaped.landing-record.test.tskeeps every legacy assertion (one model alone, two on the same model, no premium, missingmodel:) onSendrast/sendrast-webwhile the re-used-review-id case moved onto the new rule with a stronger assertion.landing-model-supersede.test.tskeeps the whole 2411/2522 supersession corpus on the legacy owner AND replays the #115 shape under the new rule (two sessions land it, a cache-less shadow still refuses).landing-corpus.test.ts,landing-blockers.test.ts,landing-pr70-reviews.test.tsandverdict-694-replay.test.tsonly add declarations to fixtures — pre-ruling records could not have asession:, so they are replayed under the documented legacy override — and change exactly the assertions whose wording follows the rule.working-files/homelab-2518/HOMELAB-2659-red.logshows 18 real failures of the new file against346ff34, and the sampled failures are assertions of the OLD rule (1 declared model(s) (omniroute/deepseek-flash),no exact-head verdict came from a premium model,so they are one verdict, not two), not parser errors or missing fixtures; the green log matches the run I observed (18 files / 540 tests).Observations, none of them a blocker
~/.pi/agent/extensions/forgejo/verdict-cli.tspredates this PR, so--sessionis silently ignored there (itsparseArgsaccepts unknown flags) and the composed block places prose after a blank line. A reviewer using the deployed helper must write thesession:line by hand; the failure mode is fail-closed (the lock refusesdo not declare session), so this is a deployment-ordering note for the ticket, not a defect in the diff.session: unknownwould count as a distinct session from a real id. Rejecting reserved words would be defence in depth; a declaration rule cannot prove independence in any case, so this does not change what the gate guarantees.repoOwner()takes the last two segments, so a deep URL (.../Sendrast/x/pulls/88) would resolve the owner topulls. Not reachable today:collectLandingEvidenceand the census both buildrepoReffromparseRepoRefasowner/repo, and I checked both call sites.nonHolds), so it is not a regression introduced here; it is the one place I would want an explicit rule later.landing-pr70-reviews.test.tssetsprocess.env.FORGEJO_LEGACY_PANEL_OWNERSat module scope, which can leak into a later file in the same worker and flip a repo to the legacy panel;verdict-694-replay.test.tsscopes the same setting throughio.envand an explicit policy instead. The suite is deterministic today; restoring the env would make it robust.Hygiene
~/.pi/agent/extensions, and no Plane write; the deployed helper directory was read and executed read-only, never modified.No blocker open.
Verdict: REQUEST_CHANGES
model: omniroute/kimi-pool
head:
48ed217b5csession: 01a1a52a-08a6-7446-a658-24dd563e6dc9
Premium seat (Kimi pool) on the landing gate itself, dispatched under the 2026-11-04T05:58Z bootstrap ruling. Reviewed the exact head
48ed217bonly; CI run #220 is green on that sha with all 14 required contexts, and the head has not moved.What holds, checked in the code and not taken from the PR description. Session independence is counted by sessionPanel over ledger.exactHead, the unreduced exact-head stream, never over ledger.current, so two fleet sessions on one reviewer key are two panel members. session is not part of reviewerKey(), so hold withdrawal and supersession stay byte-identical, and both are asserted unchanged by tests. A verdict with no session lands in one reported (unstated) group and refuses. An untrusted identity is not panel evidence even with a declared session. The legacy owner panel keeps the old two-model/one-premium rule unchanged, does not need session, matches owners case-insensitively from the last two path segments of a URL-shaped repoRef, and an empty FORGEJO_LEGACY_PANEL_OWNERS falls back to the declared default instead of emptying the list into a fail-open. No other leg moved: CI-by-name, the exact-head pin, landing identity, holds, blocker findings, the no-override rule, and FORGEJO_DANGEROUS_PATHS are untouched, and the forgejo/index.ts diff is the one contract sentence. Tests re-run in the worktree: npx tsc --noEmit clean and npx vitest run at 18 files and 540 tests, all passed; the re-pointed suites keep their rails and say why in-file, and the one rewritten assertion is disclosed in the PR body.
Blocker — H2659-K1: the honest limit of the self-declared session line is not documented; docs/forgejo.md overclaims it
The entire dangerous-class gate now rests on a self-declared session string, and the lock cannot tell one session declaring two ids from two sessions. A single session can post two approvals with two different declared session values and pass the panel. A single session can also post a comment and a formal review under two different declared session values and pass. An untagged verdict refuses and an untrusted identity refuses, but nothing binds a declared session value to the session that wrote it.
model: was always self-declared too, so self-declaration is not new. What is new is that the old rule forced a second declared model family and a premium seat, and this PR removes that backstop without recording what stands in its place, on the exact class of change that rewrites the gate.
docs/forgejo.md leg 4 overclaims. It says the fleet records the real thing directly and that independence cannot be checked from a declaration nobody made, which together imply a declaration somebody made is checked. It is not checked. It is trusted at face value from a trusted reviewer identity, exactly like model: always was.
The forgejo_merge_pr tool description in forgejo/index.ts states the new rule without this limit either, and it is the contract sentence every orchestrator reads before it dispatches reviewers.
The fix is documentation only. State in docs/forgejo.md leg 4 and in the forgejo_merge_pr tool description that the session declaration is self-declared and taken at face value from a trusted reviewer identity, that the lock cannot distinguish one session declaring two ids from two independent sessions, and that real independence is procedural: the orchestrator dispatches genuinely separate review sessions and each verdict declares the session id it actually ran in. Replace the records-the-real-thing wording with what the lock actually records.
No code change is needed for this blocker. The implementation matches the ruling; the contract overstates what it enforces, on the gate that every other merge trusts.
BLOCKER OPEN: H2659-K1
Verdict: APPROVED
model: omniroute/deepseek-flash
head:
55bbb2b1a5session: 01a1a560-a906-7446-a658-24ea6557f1bf
I reviewed
55bbb2b1a5only, against base main346ff343e6, and I read the branch as its two commits rather than as one blob, so the K1 answer and the change it documents are judged separately:48ed217b5ccarries the ruling,55bbb2bcarries the documentation.55bbb2bis documentation and comment onlygit show 55bbb2btouches three files and adds no executable line: docs/forgejo.md (the "why sessions and not models" paragraph reworded, plus a new "The limit of the declaration (H2659-K1)" paragraph), one clause inside theforgejo_merge_prpromptGuidelines string in forgejo/index.ts, and six*comment lines aboveexport interface SessionPanelin forgejo/landing.ts. I read the landing.ts hunk line by line: every added line sits inside a block comment, and nothing on that path is a regex, a branch condition, a refusal string or an exported signature.git show 55bbb2b -- forgejo/testsis empty, so no test expectation was moved to fit the new text. The only non-comment byte change in the commit is the tool-description string, which is the contract an orchestrator reads rather than a code path, so the lock at55bbb2bbehaves identically to48ed217.The limit is stated, in both places the review asked for
docs/forgejo.md leg 4 now carries a dedicated paragraph: the
session:line is a SELF-DECLARATION the lock reads at face value, nothing cryptographically binds it to the session that wrote it, one agent session could post two verdicts under two differentsession:ids or shadow its own comment with a formal review carrying a second id, independence is therefore PROCEDURAL and enforced outside the lock, and what the lock contributes is "measurement and report … not proof". Theforgejo_merge_prdescription says the same thing in one clause: "that session: line is a SELF-DECLARATION the lock reads at face value, not a proof — nothing binds it to the session that wrote it — so session independence is procedural: the orchestrator must dispatch genuinely separate reviewer sessions, and a forged signature is a fault". The landing.ts comment onSessionPanelrepeats it where a future maintainer will read it.The requirement is not softened, and no fresh overclaim replaces the old one
Both places keep the requirement at full strength: the description still says "TWO APPROVALS FROM TWO INDEPENDENT REVIEW SESSIONS … the same session approving twice is ONE approval, and a verdict with no session: refuses", and the docs add "Naming the limit does not soften the rule: two real sessions are still the requirement, and one session that declares itself twice is a fault, not a loophole". I grepped for the old overclaim the blocker quoted and for its cousins: the "records the real thing directly" sentence is gone, and "proves" / "verified independence" / "proven independence" appear nowhere near the session rule (the surviving
proveshits are about the helper's read-back proof of author andcommit_id, unrelated to session independence). The replacement wording — "the fleet can name the unit that actually carries that second pass: the review session" — is a claim about naming, not about verifying, and the limit paragraph sits four lines below it.Substance is intact and I did not take the summary's word for it
sessionPanel()iteratesledger.exactHeadand neverledger.current, keeping the latest trusted non-hold verdict per session, so two fleet sessions that share one reviewer key (same author, same model, no lens) are two panel members; an undeclared session groups on"", which cannot collide with a declared one becauseSESSION_DECL_REdemands six characters, and that group is reported and refused rather than merged.reviewerKey()at landing.ts:1484 is still author + model + lens withsessionappearing nowhere in it, so hold withdrawal and supersession remain byte-identical; the suite pins both directions, including that a same-key later stance still wins and that an approval on a different model cannot withdraw a hold keyed elsewhere. The legacy owner path really is the old rule and does not ask for a session declaration: on Sendrast/sendrast-web and jesus/infra-core two declared models with one premium pass with nosession:line at all, while one model alone refuses. Owner derivation takes the second-to-last path segment after stripping query, fragment, trailing slashes and a.gitsuffix, then lower-cases it, soowner/repoand a URL-shaped ref agree and a repo literally named after a legacy owner under a different owner resolves to that other owner;Homelab/pi-extensionsderives ownerhomelaband cannot be captured byjesus/Sendrast(a case-insensitive match on the repo segment is not what the code does). An empty or blankFORGEJO_LEGACY_PANEL_OWNERSfalls back to the declared["jesus","Sendrast"]because the splitter returns null when no non-blank part survives, and the test asserts that explicitly.FORGEJO_DANGEROUS_PATHSand its default list are untouched,forgejo/**is still dangerous, no override parameter or marker was added, and the leg 1 / leg 2 / leg 4 / leg 5 hunks are absent from the diff — the only evaluateLandingLock hunks are leg 3 itself, the record it returns, its refusal strings and its success note.Tests and CI as I observed them
In the PR worktree with
node_modulesused read-only:npx tsc --noEmitclean, thennpx vitest runat 18 files / 540 tests, all passed (landing-sessions.test.ts 26 cases among them), andgit status --porcelainempty afterwards. I read landing-sessions.test.ts against the ruling's bullets and its negatives: two sessions on the same DeepSeek model with no premium seat permits; one session approving twice refuses with "the same session approving twice is ONE session"; a single approval refuses; a panel verdict with nosession:refuses and is named by id; a missingmodel:still refuses; two sessions on different models with no premium permits and the note drops the "both on" clause; a concern-only session still counts as a panel member; the routine class still needs one verdict; the legacy owner keeps two-models/one-premium and refuses without a session line only by the model rule; the owner match is case-insensitive and URL-shaped refs agree; an env override moves a repo in both directions; and the "what must NOT change" block pins the reviewer key, the withdrawal semantics, untrusted-identity exclusion and CI-not-green. CI on this exact sha: run #224, all 14 required contexts success (runs #223 and #225 are python-only partials on the same sha). Run #222 failedpi-swarm-suiteintests/core/task-store-wiring.test.ts, which is not in this diff and whose excerpt is the expectedENOTDIR /dev/null/nopediagnostic rather than an assertion this change could affect; the fresh run #224 passes that same job in 6m51s, so the required contexts are green at the head I reviewed.One refinement I noted and do not treat as blocking
Neither session clause spells out that the face-value read happens only for a TRUSTED reviewer identity; docs/forgejo.md states that rule in legs 2 and 3 immediately above (approvals are only evidence from
ld-revieweror theFORGEJO_LANDING_REVIEWERSset, holds likewise), and the lock enforces it — an untrustedci-readerapproval carrying a well-formedsession:does not enter the panel, which landing-sessions.test.ts asserts. So the limit paragraph is not overclaiming by omission, and I record this as a wording refinement, not a finding.H2659-K1 is resolved by
55bbb2b: the "records the real thing" wording is gone and both required places now state that thesession:line is a self-declaration read at face value, that it cannot distinguish one session declaring two ids from two independent sessions, and that real independence is procedural — enforced outside the lock by dispatching separate sessions.No blocker open.
Verdict: APPROVED
model: omniroute/kimi-pool
head:
55bbb2b1a5session: 01a1a560-dffa-7446-a658-24efc6492a11
Premium seat (Kimi), dispatched under the overseer ruling 2026-11-04T05:58Z sanctioning one genuine Kimi verdict beside the two DeepSeek seats because this PR changes the landing gate itself.
H2659-K1 is resolved by
55bbb2b: docs/forgejo.md leg 4 now carries the paragraph "The limit of the declaration (H2659-K1)" stating verbatim that thesession:line is a self-declaration read at face value, that nothing cryptographically binds it to the writing session, that one session could post two verdicts under two ids or shadow its own comment with a formal review carrying a second id and still pass the leg, that independence is therefore procedural and enforced outside the lock, and that what the lock contributes is measurement and report, not proof — and theforgejo_merge_prtool description in forgejo/index.ts now carries the matching clause ("a SELF-DECLARATION the lock reads at face value, not a proof — nothing binds it to the session that wrote it"), with the same limit additionally stated on thesessionPanel()comment seam in forgejo/landing.ts ("It is a measurement, not a proof.").The previous overclaim is gone: "the fleet records the real thing directly" is corrected to "the fleet can name the unit that actually carries that second pass", and a grep of the head for "records the real thing", "proves independence", and "verified/proven independence" finds no surviving overclaim — the only "proves" hits are pre-existing, accurate statements about read-back evidence. The two-session requirement is not softened: the new text says explicitly "two real sessions are still the requirement, and one session that declares itself twice is a fault, not a loophole."
Full-diff re-judgement at this head:
git show 55bbb2bis documentation/comment-only — docs/forgejo.md prose, one tool-description string in forgejo/index.ts, and a six-line comment addition on the SessionPanel seam; no behaviour, regex, refusal text, or test change anywhere in the response commit.sessionPanel()counts sessions over the unreducedledger.exactHead(never the reducedledger.current/nonHolds), which is exactly correct because every fleet reviewer shares onereviewerKey().reviewerKey()is byte-identical to the base (verified by diff of the function body between346ff34and55bbb2b), so hold withdrawal and supersession semantics are unchanged. The legacy-owner path preserves the old rule behaviourally — two declared models, one premium, nosession:demand — andrepoOwner()handles URL-shapedrepoRef(query/hash stripped, trailing slashes and.gitremoved, last two segments, lower-cased); an emptyFORGEJO_LEGACY_PANEL_OWNERSfalls back to["jesus","Sendrast"]becausesplit()returns null on empty.FORGEJO_DANGEROUS_PATHSis intact, legs 1/2/4 (code) and doc legs 1/2/3/5 are untouched, and there is no override parameter.Verification:
npx tsc --noEmitclean;npx vitest rungreen — 18 files / 540 tests;git statusclean in the worktree. CI at this head is green (run #224, all 14 required contexts; the earlier #222pi-swarm-suitefailure was a runner-load flake in a package the diff does not touch, and the sanctioned close/reopen produced a green run at the unchanged head).No blocker open.
blockers resolved
Verdict: APPROVED
model: omniroute/deepseek-flash
head:
55bbb2b1a5session: 01a1a560-a932-7446-a658-24ece132fdf3
H2659-K1 is resolved by
55bbb2b: the independence limit is now stated in one voice in docs/forgejo.md (leg 4, "The limit of the declaration"), in thesessionPaneldocblock, and in theforgejo_merge_prpromptGuidelines that an orchestrator actually reads — each says thesession:line is a SELF-DECLARATION the lock takes at face value, that nothing binds the string to the session that wrote it, that independence is therefore procedural (the orchestrator dispatches genuinely separate sessions, and a forged signature is a fault), and that naming the limit does not soften the two-real-sessions requirement.Scope of the head commit:
55bbb2bchanges 23 lines of docs/forgejo.md prose, six lines of comment inside theSessionPaneldocblock, and one promptGuidelines sentence in forgejo/index.ts. No regex, no refusal string, no branch condition, no test expectation. The behaviour change is all in the parent48ed217, so this head is judged mostly as the answer to K1, and I read both.Substance, verified by reading the head and by running the lock:
sessionPanel()counts one row per session overledger.exactHead, i.e. the stream before thereviewerKey()reduction, which is what makes two same-model sessions two members;reviewerKey()is byte-identical (author/model/lens) and a test pins the split (two exact-head rows, onecurrentrow); the legacy branch is the pre-ruling code moved, not rewritten, and it does not ask forsession:(a jesus repo with two models and one premium seat permits with no session declared);repoOwner()takes the last two path segments and lower-cases them, andisLegacyPanelRepo()fails closed to the new rule when no owner parses;FORGEJO_LEGACY_PANEL_OWNERSset to "", " ", "," or " , " falls back to["jesus","Sendrast"]rather than to no exemption;FORGEJO_DANGEROUS_PATHSis untouched (same 31 patterns as main, and the file list still contains forgejo/**); the leg 1 / leg 2 / leg 4 / leg 5 hunks are comment-only and the merge tool still exposes no override parameter.Adversarial probe, one comment cannot vote twice: a single body carrying two
session:lines yields exactly one session (extractVerdictreturns the first match, "sess-first-decl" in my run), because the declaration is read with a non-global match, so a single review submission cannot be split into two members. A verdict declaring no session is reported by comment id and refuses ("session independence cannot be checked"); an untrusted author's session is not a panel member at all (theci-readerrow was excluded and the panel stayed at one); a verdict pinned to another head does not join the panel.Adversarial probe, the documented limit reproduces: one session posting two verdicts under two fabricated ids passes the leg, and so does one session shadowing its own comment with a formal review (I also checked the inline-comment kind, which passes too) that carries a second id. That is exactly the sentence K1 added, the docs name both shapes, and the promptGuidelines now warn an orchestrator in plain words that the declaration is not proof and independence must be dispatched as separate sessions. I found no route to a two-member panel that the documented limit does not already cover.
Observations, none of them blocking and none introduced by this commit: (a) the declaration is read anywhere in the body, so ordinary prose can declare by accident — a body whose text reads "My session: reviewed this with the operator present." parsed as session "reviewed" and, next to one real declaration, made a two-member panel; that is the same shape the existing
model:declaration has had since it shipped, it is only permissive when a second distinct verdict exists, and it is worth a follow-up rather than a change here; (b) the K1 sentence is the one part of the promptGuidelines contract no test asserts (the rest of that string is asserted), so a later edit could drop the warning silently; (c) the leg this PR was told not to touch — a trusted objection is cleared when any later exact-head verdict shares its reviewer key, and the whole sanctioned fleet shares one key — is unchanged, as the ruling required, so with the new rule a dangerous-class change can be permitted by two approvals posted after a seat dissented, without that seat withdrawing anything; I checked it because the new rule makes the leg reachable on the dangerous path for the first time, I recommend a separate ticket, and I do not hold this PR for a leg the ruling froze.Tests at this head, run by me in the worktree:
npx tsc --noEmitclean,npx vitest run18 files / 540 tests green, and the worktree is clean afterwards. The 26 cases in tests/landing-sessions.test.ts are not vacuous: the negatives assert the named comment ids, the session count, the refusal wording that carries the rule, and the absence of the old premium/different-model language on the new path, and the legacy cases assert the opposite direction on a real legacy owner. CI is green on this exact sha (run #224, all 14 contexts; runs #223 and #225 also green).Hygiene: no secret in argv (the credential was a 0600 file), no files added or modified by me, no write outside the worktree, no Plane or GITS write.