HOMELAB-2522: fix(forgejo): supersede duplicate untagged formal verdicts #80
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!80
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-2522-formal-supersession"
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?
Closes HOMELAB-2522 (epic HOMELAB-2518, split from HOMELAB-2502 / GITS-433).
Problem
supersedeUntaggedVerdictsonly looked FORWARD. Homelab/ghostty-in-the-shell#115 (GITS-430)recorded the shape it misses: comment #19269 carries
model: omniroute/kimi-pool(premium,exact head
3bab7f7) and 18 s later formal review #1601 restated it in prose —— with no
## Verdict:block and nomodel:line. Read as a second, untagged verdict it kept itsown key (
ld-reviewer/unstated) and refused a complete two-model/one-premium panel for a model itsown author had already declared. Nothing an agent may do could cure it: comments cannot be edited,
and posting a verdict on a reviewer's behalf is forbidden.
Contract implemented (from the ticket)
An untagged verdict now yields also to a model-tagged exact-head verdict it NAMES
(
comment #N,review N, bare#N) — independent of order, because the citation decides, notrecency. Rails, unchanged:
tier: premiumrow survives unless itscounterpart is premium);
never yields to the approval it cites), same id namespace (a review id and a comment id are
different records), model-tagged, and not this row itself;
from the same author supersedes" rule would violate the existing HOMELAB-2411 test
the latest verdict of a key without a model still refuses (no fail-open)— there, tagged #17450followed by uncited untagged #17460 is the reviewer's latest word. Existing supersession tests are
unchanged.
The collapse is reported in the leg-3 refusal and in the passing landing note
(
formal review #1601 declares no model but is superseded by the same author's named, earlier comment #19269 (model: omniroute/kimi-pool) at this head — one record, not two), so a passing panel names therow that yielded.
Red first (fresh
origin/mainfe7058b)vitest run tests/landing-model-supersede.test.ts tests/landing-record.test.ts→ 4 failed |47 passed, both positives per test file red for the recorded reason:
Negative cases added and green before the change: hold that names its tagged source; untagged review
naming no duplicate; citation to a remark; citation to another author's verdict; citation to a
verdict on another head; concern citing an approval; untagged
tier: premiumshadow citing aworker-tier verdict.
Green
vitest run(full forgejo suite): 13 files / 362 tests passed.tsc --noEmit: clean.landing-record.test.tsassert the tool outcome:forgejo_merge_prpasseswith the two declared models and one merge POST, and
forgejo_landing_censusreportsverdicts: ["#19267 omniroute/deepseek-flash", "#19269 omniroute/kimi-pool (premium)"]with#1601collapsed (not a third verdict, not an ignored identity).Real-data replay (read-only GETs against Homelab/ghostty-in-the-shell#115)
Evaluated the LIVE stream through
evaluateLandingLock:ok: false,verdict comment(s) #1601 do not declare model: <id>— the recorded #115 refusal reproduced.ok: true, approvals#1600 omniroute/deepseek-flash+#19269 omniroute/kimi-pool (premium), with the #1601 collapse reported.ok: true, unchanged panel; #1601 thenyields forward to #19277 exactly as HOMELAB-2411 intends.
Files
forgejo/landing.ts,forgejo/tests/landing-model-supersede.test.ts,forgejo/tests/landing-record.test.tsonly.docs/forgejo.mddeliberately untouched: open PR #78 changes that file. The ticket'sdocumentation row ("formal reviews should still repeat the verdict/model/head grammar; no comment
edits, no proxy verdicts") is written into the
supersedeUntaggedVerdicts/citedTaggedVerdictdocstrings here and needs a doc sentence once #78 lands. Collision pre-check: #78
(
docs/forgejo.md,forgejo/api.ts,forgejo/index.ts,forgejo/tests/comment-identity-hardening.test.ts,forgejo/verdict-cli.ts), #79 (plane/**), #68 (pi-swarm-suite/**) — no overlap with this diff.No self-review, no verdicts, no merge, no Done: the panel reviews this head normally.
Verdict: APPROVED
model: omniroute/deepseek-flash
head:
7e87ca8a01Fresh independent general review of the exact head (re-fetched immediately before submitting: PR #80
head.sha=7e87ca8a01a18173971a867cab00e790d581cb2d, branch tip unchanged by fetch,mergeable: true). Base note:origin/mainadvancedfe7058b8->4be7c075(HOMELAB-2554 / #79,plane/**+docs/plane.md). No rebase needed:git diff fe7058b8 4be7c075 -- forgejo/is empty, so the PR diff is the intended 3 files with no revert of #79 in the merge.Scope (enumerated, not assumed)
Exactly 3 files, +462/-23:
forgejo/landing.ts,forgejo/tests/landing-model-supersede.test.ts,forgejo/tests/landing-record.test.ts. Nothing else; no debug leftovers, no skipped/only tests.What I re-ran myself (not the worker's logs)
vitest run tests/landing-model-supersede.test.ts tests/landing-record.test.ts-> 2 files / 51 tests passed, exit 0.vitest run-> 13 files / 362 tests passed, exit 0;tsc --noEmit-> exit 0. PR #74 blocker-fix regressions (H2521-C1..C10, K1..K6, R1..R5 intests/landing-blockers.test.ts) are inside those 362 and the file is not in the change set.forgejo/tree to/tmp, overwrotelanding.tswithgit show fe7058b8:forgejo/landing.ts, reran the PR's two test files against MAIN's implementation -> 4 failed | 47 passed (51), exit 1, and the exact recorded #115 refusal:verdict comment(s) #1601 do not declare model: <id>, so the second-reviewer rule cannot be checked. All seven negative cases stay green on main.7e87ca8a01: run #184, 14/14 jobs success.Contract legs verified (probes of my own against the head)
**APPROVED — head …**grammar) -> panel passes on#19267 deepseek-flash+#19269 kimi-pool (premium), collapse named in the note.REQUEST_CHANGESnaming its tagged source still refuses onhold(s) are open(early returnv.polarity === "hold"untouched).tier: premiumshadow citing a worker-tier verdict survives; premium->premium collapse leaves the premium leg standing.#1601keeps its key and keeps refusing.modeland no reviewer key, so its collapse can only resolve themissingModeltaint. Probed: one tagged model + collapsed shadow still refuses "needs two reviewers on different models"; no premium + collapsed shadow still refuses "no exact-head verdict came from a premium model".models.length < 2/premium.length === 0are byte-identical to main.### Blocker — X/BLOCKER OPEN: Xinside a collapsed row still refuses at leg 4 —findOpenBlockersreadsinput.items, not the ledger.supersessionLines()feeds BOTH the leg-3 refusal and the passing note.Findings (non-blocking)
H2522-D1 (non-blocker, precision/claim drift). The documented "same id namespace" rail is not enforced.
citedVerdictIdspushes a kind-qualified entry and then a second bare{id}from the fallback regex, whose[^\w#&]matches the space before#1600; so!c.kind || c.kind === u.itemKindcan never reject, and thekindfield is dead weight. Probe: a review whose body sayscomment #1600, where 1600 is a review (tagged, same author/head/stance), collapses onto it. Not a fail-open for the reasons above — a real model-tagged same-author/same-head/same-stance/premium-safe verdict is still required and no panel number moves — but the PR body's andcitedTaggedVerdict's docstring's claim that "a review id and a comment id are different records" is not what the code does. Fix: skip the bare match when the#followscomment(s)/review(s), or delete the qualifier extraction and document the bare-#rule as the real contract.H2522-D2 (non-blocker, audit surface).
forgejo_merge_pr's passing note and the leg-3 refusal name the yielded row, butforgejo_landing_censusexposes onlyverdicts/ignored/collapsed, so an operator scanning the census cannot see that #1601 yielded.H2522-D3 (informational, owner follow-up). Ticket scope row 4 (
docs/forgejo.mdgrammar sentence) is delivered aslanding.tsdocstrings only. I verified the stated reason live: open PR #78's branch does changedocs/forgejo.md, so the collision is real and the deferral is justified — it needs a follow-up doc PR, which should state D1's actual behaviour. Owner note, not this PR's defect: #78's own description claims it does not touch that file; that text is stale.No open blocker findings. The dangerous-class panel still needs a second verdict on a different model with one premium — that second verdict is not mine to give and I have not touched any prior verdict. No comment edited, no verdict posted on anyone's behalf, no merge.
7e87ca8a011d2f7bc794Rebase onto current
origin/main— new head1d2f7bc794bd394c4531e40c7e75390051387241Not a verdict. No review state, no hold, no approval. Posted by the rebase worker so formal reviews re-pin to the current head.
Rebase
origin/main=9747346db937a0f27ff2717f80daaead74fef8b6(merge of #78).7e87ca8→ new head1d2f7bc794bd394c4531e40c7e75390051387241, single commit, parent is9747346d.docs/forgejo.md,forgejo/api.ts,forgejo/index.ts,forgejo/tests/comment-identity-hardening.test.ts,forgejo/verdict-cli.ts; #79 touchedplane/**; this diff touches none of them.--force-with-lease=…:7e87ca8a01a18173971a867cab00e790d581cb2d); result+ 7e87ca8...1d2f7bc (forced update).Diff unchanged in shape — still exactly three files
docs/forgejo.mdstill untouched, so the follow-up doc sentence noted in the description remains a separate step after #78 lands.Verification on the new head (in worktree
/home/aaron/repos/pi-extensions-2522)vitest run tests/landing-model-supersede.test.ts tests/landing-record.test.ts→ 2 files / 51 tests passed (20 + 31).vitest run→ 14 files / 385 tests passed. (Description's "13 files / 362 tests" was pre-rebase; the extra file is #78'scomment-identity-hardening.test.tsnow onmain.)tsc --noEmit→ clean (exit 0).1d2f7bc: green.Branch left stable and untouched for fresh formal reviews.
D1 fix round (review §7 D1) — NOT a verdict.
Head:
65fe570c11ad42382758bff225bd1c66021a66b4(1d2f7bc..65fe570, one push, remote ref verified).Scope: D1 only. Files:
forgejo/landing.ts,forgejo/tests/landing-model-supersede.test.ts. D2 (census index) and docs untouched — HOMELAB-2566 is editingforgejo/index.tsanddocs/forgejo.md.Defect:
citedVerdictIdsemitted a kind-qualified entry AND a bare{id}fallback for the same#N, so!c.kind || c.kind === u.itemKindcould never reject anything — an untagged review body sayingcomment #1600collapsed onto review 1600.Fix: the
#offsets consumed by acomments?/reviews?qualifier are recorded and skipped in the bare#Npass. A qualifier binds its namespace; unqualified#19269stays namespace-agnostic (recorded #115 shape). Author/head/stance/premium rails, holds, blockers and #115 replay unchanged.Red-first (tests written before the fix):
Two sibling rails (qualified
review #1600collapse, bare#1600acceptance) are green pre- and post-fix.Green: targeted 23 passed; full forgejo suite 14 files / 388 passed exit 0;
npx tsc --noEmit(forgejo/) exit 0.Evidence:
working-files/homelab-2518/HOMELAB-2522-d1-red.log,-d1-green-forgejo.log,-d1-green-full.log,-d1-typecheck.log,HOMELAB-2522-worker.md.D2 fix round (worker note, non-verdict) — head
a9b2e5e48790d6c2e84c6ea1d8ed4d888cbb37d4Scope: D2 of the general review (
HOMELAB-2522-review-deepseek§7) only. Noreview verdict, no self-review, no merge, no Done; D1 rail and D3 doc untouched.
Defect.
supersedeUntaggedVerdictscomputesledger.modelSupersededand boththe passing note and the leg-3 refusal name the yielded row, but
LandingLockResult.recordnever carried it — soforgejo_landing_censusrenderedverdicts/ignored/collapsedonly and the #115#1601collapse was invisiblein the queue.
Change (3 files, +145/−1, all in the allowed set).
forgejo/landing.ts: record gains optionalmodelSuperseded: ModelSupersessionNote[]— untagged
id/kind/author/polarityandby {id,kind,model}— populated atall four record sites. Context, never evidence:
approvals,holds,collapsed,untrustedand exact-head logic are unchanged.forgejo/index.ts: the census row carries the structured notes and the renderedline prints
YIELDED review #1601 ld-reviewer→comment #19269 omniroute/kimi-pool.The type is imported from
./landing.js, soapi.tsneeded no re-export.forgejo/tests/landing-record.test.ts: red-first route-fake census test (row +rendered line, and the collapse is still not a verdict, not an ignored identity,
not a reviewer-key collapse), plus a refusing record with an open hold that
carries the collapse while the hold still refuses.
Evidence.
65fe570c:expected undefined to deeply equal [ { id: 1601, kind: 'review', … } ]— 1 failed / 31 skipped.landing-record.test.ts33 passed; full forgejo suite 14 files / 390 passed (exit 0);npx tsc --noEmitexit 0.Logs:
working-files/homelab-2518/HOMELAB-2522-d2-{red,green-forgejo,typecheck}.log.Verdicts remain the reviewers' to post.
Verdict: APPROVED
model: omniroute/deepseek-flash
head:
a9b2e5e487Fresh GENERAL review, dangerous class, entire diff against
origin/main9747346. This head is a new head since the earlier APPROVED (review1657,pinned to
7e87ca8a): it carries the rebase onto #78, the D1 namespace fix(
65fe570) and the D2 census fix (a9b2e5e), so all three are in scope here.Head/base, re-fetched live. PR head =
a9b2e5e48790d6c2e84c6ea1d8ed4d888cbb37d4= dispatched head = worktree HEAD (clean). Base =
9747346…= currentorigin/main— nothing to rebase. Mergeable true. Diff is exactly 4 files,672+/24−:
forgejo/landing.ts,forgejo/index.ts,forgejo/tests/landing-model-supersede.test.ts,forgejo/tests/landing-record.test.ts.Independent runs (PR worktree, read-only): targeted
landing-model-supersede+landing-record→ 2 files / 56 tests passed;full suite → 14 files / 390 passed;
tsc --noEmit→ exit 0. PR #74'slanding-blockers.test.ts(79 tests) and #78'scomment-identity-hardening.test.ts(23 tests) are inside those 390 and pass —no blocker regression, no revert of the merge this branch sits on. CI on the
exact head, re-enumerated myself: run #190, 14/14 jobs success.
Red-first, re-verified by me (tarred
forgejo/to/tmp, swapped oldlanding.tsin): againstorigin/main'slanding.ts, HEAD's tests give8 failed / 48 passed — the positives only, every negative inert. Against
pre-D1
1d2f7bc: exactly the one cross-kind test fails. Against pre-D265fe570: exactly the two census/record-carrying tests fail. Worker claims hold.Contract legs, each checked with my own probes, not the PR's tests:
comment #1600no longercollapses onto REVIEW 1600;
comment#1600with no space is still bound, so thebare fallback does not leak.
#idstays namespace-agnostic (the recorded #115 shape) and stillcollapses; a qualifier that matches the kind still collapses.
REQUEST_CHANGESciting the tagged approval keeps itskey and refuses.
tier: premiumshadow citing a worker-tier source doesnot collapse and stays in
approvals.still refuses
1 declared model(s) … needs two reviewers on different models.both named); the census carries
row.modelSupersededplusYIELDED review #1601 ld-reviewer→comment #19269 omniroute/kimi-pool(D2),and my probes confirm the collapse rides the hold refusal and the
incomplete-panel refusal too, while adding no approval, hold or model.
Structural invariant re-read in the code: an untagged row carries no
modelandno reviewer key, so a collapse can only clear the
missingModeltaint — itcannot add a model, cannot add premium (
u.premium || !v.premium), cannot drop ahold (early return), cannot hide a blocker (leg 4 reads
input.items). Panelarithmetic is byte-identical to main.
No blocker. Non-blocking findings:
H2522-D4— thecitedVerdictIdsdocstring lists`comments 19269`(no#) as a supported form; the regex requires the literal#, so that shapeyields no citation at all (
citedVerdictIds("details in comments 19269") === [])and the row keeps refusing. Fail-closed, but this docstring is the only
documentation of the grammar now, so it should not advertise a dead form.
H2522-D5— "a qualifier BINDS the id it introduces" is not absolute. Thebinding needs whitespace between qualifier and
#, so the naturalreview: #1600(colon) or a decorated**comment** #1600falls back to thenamespace-agnostic bare id. Still fail-closed for evidence (the collapse needs a
same-author / same-head / same-stance / premium-safe tagged record and moves no
panel number), so this is a docstring-accuracy defect, not a fail-open: allow an
optional
:/ markdown emphasis in the qualified regex, or soften the sentence.Observations, no action needed for this PR:
modelSuperseded:sits at 8 spacesinside a 4-space record literal in
landing.ts(cosmetic); thechangedFiles === undefinedrefusal returns no record at all (pre-existing onmain, so the census cannot carry the collapse there — D2 is complete for every
record that exists);
forgejo/index.tsis outside the ticket'sfiles_may_touchand was added by the D2 finding — I checked and no open PRtouches it now (#81 moved to
docs/forgejo.md,verdict*.ts), so no collision.H2522-D3carried, still owner follow-up: the ticket's docs row lives only inlanding.tsdocstrings. The original reason (#78 helddocs/forgejo.md) hasmerged away, but PR #81 / HOMELAB-2566 now holds that file, so the deferral
remains justified and must be re-checked after #81 lands; the follow-up sentence
should state D4/D5's actual behaviour.
Dangerous class: this is one of the two required verdicts. The panel still needs
a second exact-head verdict on a different declared model, one of them premium —
this review is
omniroute/deepseek-flashand is not premium.Verdict: APPROVED
model: omniroute/k3.1-pro
head:
a9b2e5e487tier: premium
Fresh independent premium review of the entire four-file diff against
9747346db937a0f27ff2717f80daaead74fef8b6(+672/-24). Read HOMELAB-2522, the worker's final D1/D2 report, prior review findings, and PR comments. Live head was re-fetched before submission.Independent verification on the exact head:
tsc --noEmitclean.D1 now binds qualified citations to comment/review namespaces while bare ids remain agnostic. D2 exposes
modelSupersededin census rows and rendered output without promoting it to evidence. Holds, blockers, exact-head checks, two-model arithmetic, and premium rails remain fail-closed. No open blocker findings.Non-blocking documentation nit: the docstring's
comments 19269and PR description'sreview Nforms omit#, but the qualified parser requirescomment #N/review #N. Direct probes confirm the no-#forms are inert (fail-closed); align wording or parser in follow-up.No code edits, comment edits/deletions, proxy verdicts, or merge.