HOMELAB-2682: fix(forgejo): three lock defects (resolution author check, no-defect headings, id-less hint) #99

Merged

pi-landing merged 1 commit from plane/HOMELAB-2682-lock-defects into main 2026-11-05 19:58:51 +00:00

Conversation 2 Commits 1 Files changed 2 +299 -6

pi-landing commented 2026-11-05 19:30:25 +00:00

Collaborator

Copy link

Three merge-lock defects El Director reported (OVERSEER order on HOMELAB-2682), each confirmed by a failing unit test on synthetic PR text — no real PR was used for any case, and no network was touched.

Where they were measured. The report was read off the deployed source drops, and the drops are not the checkout: forgejo/landing.ts at the two sandboxes (sbx-lab, sbx-gits) is sha256 cce9615… = commit ef7a662 (HOMELAB-2686), while the mgmt-VM drop and origin/main are c48a734… = 1d24858. Commit 1e7c397 (HOMELAB-2682, "reviewer-only resolutions, heading-only blockers") already closed two of the three arms in the checkout. Running this PR's new test file against the deployed copy fails 10 of 17:

× an untrusted `blockers resolved` does not close an id-less finding
× an untrusted fix-worker note does not close an id-less finding
× an untrusted `resolved <id>` does not close a labelled finding
× an untrusted comment that raises AND closes in the same body closes nothing
× an item with no author closes nothing (fail-closed)
× an untrusted resolution RESTATEMENT does not close the finding it restates
× reads each no-defect heading as a section title, not a finding
× does not swallow a heading that carries a body
× still reads the section-heading shapes #96 and #97 pinned
× does not tell the author to cite an id when no finding carries one
      Tests  10 failed | 7 passed (17)

and against this branch's head, 17/17 pass.

---

D1 — a resolution needed no particular author (top priority)

Confirmed: at the deployed copy, an untrusted blockers resolved, an untrusted FIX WORKER — blocker fixed, an untrusted resolved B1, and a single untrusted comment that both raises and closes all cleared the finding (findOpenBlockers returned []). In the checkout, the id-ed and id-less arms are already gated by authorOwnsResolutions (1e7c397), so those four cases are pinned as non-regressions. One closure path was still open at this head: the in-line RESOLUTION RESTATEMENT.

Root cause. findOpenBlockers closes a finding on four paths: (a) a later resolved <id> citation by a trusted reviewer, (b) a later id-less resolution phrase (BLOCKER_RESOLUTION_PHRASE_RE) by a trusted reviewer, (c) the section resolution lead, and (d) the RESTATEMENT — a reviewer's closing report that restates a finding as a bolded numbered label and reports the fix in the same line (**Blocker 1 (census crash).** readKey now returns null on ENOENT). (a) and (b) ask authorOwnsResolutions; (d) was evaluated as id !== null && isResolutionRestatement(line) against the line itself, with no identity check at all. Any identity could therefore write the restatement shape and the line was swallowed before it ever became a finding — a finding nobody raised and nobody closed.

Failing test (before the fix):

it("an untrusted resolution RESTATEMENT does not close the finding it restates", () => {
  const items = [trusted(1, "### Blocker 1 — the census crashes when the key file is unreadable", 1),
    item(2, RESTATEMENT, "pi-landing", 2)];
  expect(scan(items)).toContain("1");
  // …and the untrusted line is prose, not a closure: it is reported too.
  expect(findOpenBlockers(sortPRText(items), { policy: DEFAULT_LANDING_POLICY }).map((f) => f.line)).toContain(RESTATEMENT);
});

Fix. The restatement is gated by the same authorOwnsResolutions(items[itemIndex], policy) the later-line scan uses. Untrusted, the line is prose and is reported as a finding (fail-closed). The identity is judged against the item that carries the line, so a comment that raises and closes in one body cannot slip through either. No behaviour change for a trusted reviewer: all three of the reviewer's own closure forms (phrase, citation, restatement) still close, and a policy member other than the default still counts.

Pinned by: the six D1 cases — untrusted phrase, untrusted fix-worker note, untrusted citation, untrusted self-closing body, missing author, untrusted restatement — plus three no-change cases for the trusted reviewer and one for a non-default policy member.

D2 — a heading that reports no defect counted as a finding

Confirmed at this head (## Blocker test applied → [null], an id-less finding, which no citation can close):

× reads each no-defect heading as a section title, not a finding
   → ## Blocker test applied: expected false to be true

Root cause. isSectionOnlyHeading — #96's guard that keeps a bare ## Blockers heading from being an id-less finding — matched section nouns plus a closed list of qualifiers (open, remaining, no, …) and nothing else. A heading that names the section and then the META word for what the reviewer did (test applied, check applied, scan applied, tested, verification, analysis) was outside that list, so it fell through to the finding patterns and refused the merge.

Fix. isSectionOnlyHeading now also matches SECTION_META_ONLY_RE: section noun + at least one META word, whole remainder, closed vocabulary, with the reviewer's own determination on the heading allowed (— no defect found). It is still a section TITLE, so it keeps supplying the blocker context to the lines beneath it — #97's rule is untouched and its tests are green. Fail-closed direction, deliberately: anything carrying a body stays a finding (## Blocker test applied on a repo the tenant filter still leaks, ## Blocker scan_failed on the list query, ## Blockers (severity: blocker) — …, ### Blocker 1 — …), and a real defect bullet under a meta heading is still a finding while No defects found. / None. report nothing.

Pinned by: the four D2 cases (nine heading shapes; defect bullets beneath them; five headings that must stay findings; the #96/#97 shapes).

D3 — the refusal hint was wrong for a finding with no id

Confirmed at this head: every open finding id-less, and the hint still said Cite the finding id in the resolution ("resolved B1").

Root cause. evaluateLandingLock's hint chose on ids.length only: any id present → the resolved <id> form; no ids at all → the same string it used for the mixed case, telling the author to cite an id that does not exist. An id-less finding is closed only by a trusted reviewer's resolution PHRASE, so the advice was unsatisfiable.

Fix. The hint now matches the finding's kind: id-ed → No "resolved <id>" / "fixed-in <id>" marker follows.; id-less → the finding carries no id, no citation can close it, and what does is a later comment BY THE REVIEWER (policy.reviewerIdentities) declaring the blocker resolved or fixed; mixed → keeps the existing Give every blocker an id so a resolution can name it. nudge and adds the id-less note.

Pinned by: the three D3 cases (id-less, id-ed, mixed).

---

Suite

Shape	Result
npm run test:deployed (deployed source drop, npm ci, tsc --noEmit, vitest run)	26/26 files, 702/702 tests, typecheck clean
checkout (npx vitest run)	14 files pass; the 12 that import index.ts cannot resolve bare typebox without an install — the documented TESTING.md shape, not a regression

No pre-existing reds. New file: forgejo/tests/lock-defects-2682.test.ts (17 cases).

Not merged — seats come next.

Three merge-lock defects El Director reported (OVERSEER order on HOMELAB-2682), each confirmed by a failing unit test on synthetic PR text — no real PR was used for any case, and no network was touched. **Where they were measured.** The report was read off the deployed source drops, and the drops are not the checkout: `forgejo/landing.ts` at the two sandboxes (`sbx-lab`, `sbx-gits`) is sha256 `cce9615…` = commit `ef7a662` (HOMELAB-2686), while the mgmt-VM drop and `origin/main` are `c48a734…` = `1d24858`. Commit `1e7c397` (HOMELAB-2682, "reviewer-only resolutions, heading-only blockers") already closed two of the three arms in the checkout. Running this PR's new test file against the deployed copy fails **10 of 17**: ``` × an untrusted `blockers resolved` does not close an id-less finding × an untrusted fix-worker note does not close an id-less finding × an untrusted `resolved <id>` does not close a labelled finding × an untrusted comment that raises AND closes in the same body closes nothing × an item with no author closes nothing (fail-closed) × an untrusted resolution RESTATEMENT does not close the finding it restates × reads each no-defect heading as a section title, not a finding × does not swallow a heading that carries a body × still reads the section-heading shapes #96 and #97 pinned × does not tell the author to cite an id when no finding carries one Tests 10 failed | 7 passed (17) ``` and against this branch's head, 17/17 pass. --- ## D1 — a resolution needed no particular author (top priority) **Confirmed:** at the deployed copy, an untrusted `blockers resolved`, an untrusted `FIX WORKER — blocker fixed`, an untrusted `resolved B1`, and a single untrusted comment that both raises and closes all cleared the finding (`findOpenBlockers` returned `[]`). In the checkout, the id-ed and id-less arms are already gated by `authorOwnsResolutions` (1e7c397), so those four cases are pinned as non-regressions. **One closure path was still open at this head: the in-line RESOLUTION RESTATEMENT.** **Root cause.** `findOpenBlockers` closes a finding on four paths: (a) a later `resolved <id>` citation by a trusted reviewer, (b) a later id-less resolution phrase (`BLOCKER_RESOLUTION_PHRASE_RE`) by a trusted reviewer, (c) the section resolution lead, and (d) the RESTATEMENT — a reviewer's closing report that restates a finding as a bolded numbered label and reports the fix in the same line (`**Blocker 1 (census crash).** readKey now returns null on ENOENT`). (a) and (b) ask `authorOwnsResolutions`; (d) was evaluated as `id !== null && isResolutionRestatement(line)` against the line itself, with no identity check at all. Any identity could therefore write the restatement shape and the line was swallowed before it ever became a finding — a finding nobody raised and nobody closed. **Failing test (before the fix):** ```ts it("an untrusted resolution RESTATEMENT does not close the finding it restates", () => { const items = [trusted(1, "### Blocker 1 — the census crashes when the key file is unreadable", 1), item(2, RESTATEMENT, "pi-landing", 2)]; expect(scan(items)).toContain("1"); // …and the untrusted line is prose, not a closure: it is reported too. expect(findOpenBlockers(sortPRText(items), { policy: DEFAULT_LANDING_POLICY }).map((f) => f.line)).toContain(RESTATEMENT); }); ``` **Fix.** The restatement is gated by the same `authorOwnsResolutions(items[itemIndex], policy)` the later-line scan uses. Untrusted, the line is prose and is reported as a finding (fail-closed). The identity is judged against the item that carries the line, so a comment that raises and closes in one body cannot slip through either. No behaviour change for a trusted reviewer: all three of the reviewer's own closure forms (phrase, citation, restatement) still close, and a policy member other than the default still counts. **Pinned by:** the six `D1` cases — untrusted phrase, untrusted fix-worker note, untrusted citation, untrusted self-closing body, missing author, untrusted restatement — plus three no-change cases for the trusted reviewer and one for a non-default policy member. ## D2 — a heading that reports no defect counted as a finding **Confirmed at this head** (`## Blocker test applied` → `[null]`, an id-less finding, which no citation can close): ``` × reads each no-defect heading as a section title, not a finding → ## Blocker test applied: expected false to be true ``` **Root cause.** `isSectionOnlyHeading` — #96's guard that keeps a bare `## Blockers` heading from being an id-less finding — matched section nouns plus a closed list of qualifiers (`open`, `remaining`, `no`, …) and nothing else. A heading that names the section and then the META word for what the reviewer did (`test applied`, `check applied`, `scan applied`, `tested`, `verification`, `analysis`) was outside that list, so it fell through to the finding patterns and refused the merge. **Fix.** `isSectionOnlyHeading` now also matches `SECTION_META_ONLY_RE`: section noun + at least one META word, whole remainder, closed vocabulary, with the reviewer's own determination on the heading allowed (`— no defect found`). It is still a section TITLE, so it keeps supplying the blocker context to the lines beneath it — #97's rule is untouched and its tests are green. Fail-closed direction, deliberately: anything carrying a body stays a finding (`## Blocker test applied on a repo the tenant filter still leaks`, `## Blocker scan_failed on the list query`, `## Blockers (severity: blocker) — …`, `### Blocker 1 — …`), and a real defect bullet under a meta heading is still a finding while `No defects found.` / `None.` report nothing. **Pinned by:** the four `D2` cases (nine heading shapes; defect bullets beneath them; five headings that must stay findings; the #96/#97 shapes). ## D3 — the refusal hint was wrong for a finding with no id **Confirmed at this head:** every open finding id-less, and the hint still said `Cite the finding id in the resolution ("resolved B1").` **Root cause.** `evaluateLandingLock`'s hint chose on `ids.length` only: any id present → the `resolved <id>` form; no ids at all → the same string it used for the mixed case, telling the author to cite an id that does not exist. An id-less finding is closed only by a trusted reviewer's resolution PHRASE, so the advice was unsatisfiable. **Fix.** The hint now matches the finding's kind: id-ed → `No "resolved <id>" / "fixed-in <id>" marker follows.`; id-less → the finding carries no id, no citation can close it, and what does is a later comment BY THE REVIEWER (`policy.reviewerIdentities`) declaring the blocker resolved or fixed; mixed → keeps the existing `Give every blocker an id so a resolution can name it.` nudge and adds the id-less note. **Pinned by:** the three `D3` cases (id-less, id-ed, mixed). --- ## Suite | Shape | Result | |---|---| | `npm run test:deployed` (deployed source drop, `npm ci`, `tsc --noEmit`, `vitest run`) | **26/26 files, 702/702 tests**, typecheck clean | | checkout (`npx vitest run`) | 14 files pass; the 12 that import `index.ts` cannot resolve bare `typebox` without an install — the documented `TESTING.md` shape, not a regression | No pre-existing reds. New file: `forgejo/tests/lock-defects-2682.test.ts` (17 cases). Not merged — seats come next.

pi-landing added 1 commit 2026-11-05 19:30:25 +00:00

HOMELAB-2682: fix(forgejo): lock findings closure author check + no-defect headings + id-less hint ... 86cf49991d

The three merge-lock defects El Director reported (OVERSEER order on
HOMELAB-2682), confirmed by unit tests against synthetic PR text:

1. A finding could be closed by any identity on the PR. The id-ed and id-less
   arms of isResolved already require authorOwnsResolutions (1e7c397); the
   IN-LINE RESOLUTION RESTATEMENT arm — the fourth closure path — did not, so an
   untrusted author's restatement line was silently swallowed as a closure and
   never became a finding. It now needs the same reviewer identity.
2. A heading that reports no defect counted as a finding (`## Blocker test
   applied` in an approval → an id-less finding, which no citation can close).
   isSectionOnlyHeading now also reads section-noun + META-word headings; the
   heading stays a section TITLE, so it keeps supplying the blocker context to
   real defect bullets beneath it (#97's rule, unchanged).
3. The open-blocker refusal hint said `Cite the finding id ("resolved B1")` even
   when every open finding was id-less — advice nothing can satisfy, since an
   id-less finding is closed only by a reviewer's resolution phrase. The hint now
   matches the finding's kind and names the reviewer set.

Tests: tests/lock-defects-2682.test.ts (17 cases). Full suite green on the
deployed shape: 26/26 files, 702/702 tests, tsc --noEmit clean.

ld-reviewer commented 2026-11-05 19:53:41 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 86cf49991d: APPROVE
model: deepseek-flash
session: 01a1ad36-eb65-757a-b84d-2769acc9ca3c
Independent seat 1 of 2 (gate tooling). Reviewed from primary sources: the diff 1d24858..86cf499, forgejo/landing.ts at the head, the new forgejo/tests/lock-defects-2682.test.ts, plus my own probes executed against the head file with 1d24858 run alongside for comparison. Two files changed — landing.ts (+66/−6) and one new 239-line test file — and nothing outside the three reported defects, so item 4 is clear. No seat-2 input was used.

Findings

None.

D1 — the restatement closure is author-gated, and in the right place

The gate sits exactly where the restatement closure is decided (findOpenBlockers), reads authorOwnsResolutions(items[itemIndex], …) — the LINE'S OWN item author, not a neighbouring item's — and takes the same policy the id-ed and id-less arms use, which the lock supplies (findOpenBlockers(ordered, { policy, headSha })). Live in the real path, not only in tests.

Probes at the head:

• untrusted author on the restatement item, trusted finding above it: the target finding stays open AND the restatement line is itself reported (base reported only the first).
• trusted reviewer's restatement whose PRECEDING item is untrusted: closes. That is the discriminating case for "own item author" — a gate reading the neighbour's identity would come out the other way.
• trusted reviewer still closes by all three forms (phrase, resolved <id> citation, restatement), and a member of a non-default reviewerIdentities set still counts.
• an item with no author closes nothing — fail-closed as specified.

D2 — the reported shapes are fixed and the fail-closed set is intact

I re-ran every shape in the report and the run's five "must stay findings" headings. All nine no-defect headings are now section titles with zero findings; ## Blocker test applied on a repo the tenant filter still leaks, ## Blocker scan_failed on the list query, ## Blockers (severity: blocker) — …, ### Blocker 1 — … and ## Blockers: the guard only prints and never refuses all still refuse. I also fuzzed 33 headings against BOTH 1d24858 and the head: the newly-swallowed set is exactly section noun + closed meta vocabulary, and no heading that states a defect (check failed, review missed …, fix regressed, test evidence missing, verification incomplete, audit revealed a leak, status open) is in it. #97's context rule holds: a real defect bullet under a meta heading is still a finding. The residual brittleness is recorded under Concerns; every miss I found is in the safe direction.

D3 — the hint now matches what the code accepts

Ran evaluateLandingLock and read the three refusal texts. id-less: "…it carries no id, so no citation can close it. A later comment BY THE REVIEWER (ld-reviewer) declaring the blocker resolved or fixed … is what does" — accurate: I confirmed a trusted later blockers resolved closes exactly that finding and that nothing else does. id-ed keeps the resolved <id> / fixed-in <id> wording; mixed keeps the give-it-an-id nudge and adds the id-less note. No unsatisfiable advice is left.

Verification I ran

npm run test:deployed in a clean worktree at the head: typecheck ok, 26/26 files loaded, 702/702 tests passed, pinned tree typebox 1.3.34. Forgejo CI on 86cf499: run #292 every job pass, #293 python pass — 14/14 green. No pre-existing red.

Concerns

1. (D2, honest severity: nuisance, not safety) The closed vocabulary leaves plausible no-defect headings as findings that no citation can clear, identical to base: ## Blockers found, ## Blocker found, ## Blocker search, ## Blocker verification complete, ## Blocker check: clean, ## Blocker re-scan, and the shape named in the brief — ## Blocker analysis still running ("still" and "running" are both outside the list). Verified: each reads as isSectionOnlyHeading = false on base and on the head, and each refuses with a finding carrying no label. None of them is a regression and none of them can swallow a defect, so this does not gate the change; a follow-up can widen the list (found, identified, search, complete, done, clean, round) or accept that the reviewer labels what it reports.
2. (D1, operational cost, reproduced) An untrusted restatement-shaped note — the fleet's own fix-worker closing-note shape — now becomes an open finding carrying the label's id. Where the reviewer closes the original finding only with the id-less phrase, that new finding stays open and the merge refuses until the reviewer cites the id. I reproduced both states. Fail-closed is the right direction here, but it is a new refusal path for a pipeline shape, worth knowing at the landing step.
3. (docs inside the diff) The new SECTION_META_ONLY_RE comment says an id-less finding "can never be closed", while this same commit's D3 hint and isResolved's id-less arm say a trusted reviewer's phrase does close it — I verified the phrase closes it. The narrower true statement is "can never be closed by a citation". The new test titles likewise describe an intent ("a heading that reports no defect is never a finding") that a closed vocabulary does not fully implement. Wording only.
4. (observation, not gating) The restatement arm still skips resolutionHeadOk, while the id-ed citation arm applies the head discipline. Pre-existing at base and narrow — a restatement can only suppress its own line — noted because this diff is about making the restatement arm behave like its siblings.

**Verdict of record — ld-reviewer — head 86cf49991d6191c7c7db9ae671dce7dc9c85bb78: APPROVE** model: deepseek-flash session: 01a1ad36-eb65-757a-b84d-2769acc9ca3c Independent seat 1 of 2 (gate tooling). Reviewed from primary sources: the diff `1d24858..86cf499`, `forgejo/landing.ts` at the head, the new `forgejo/tests/lock-defects-2682.test.ts`, plus my own probes executed against the head file with 1d24858 run alongside for comparison. Two files changed — `landing.ts` (+66/−6) and one new 239-line test file — and nothing outside the three reported defects, so item 4 is clear. No seat-2 input was used. ## Findings None. ## D1 — the restatement closure is author-gated, and in the right place The gate sits exactly where the restatement closure is decided (`findOpenBlockers`), reads `authorOwnsResolutions(items[itemIndex], …)` — the LINE'S OWN item author, not a neighbouring item's — and takes the same `policy` the id-ed and id-less arms use, which the lock supplies (`findOpenBlockers(ordered, { policy, headSha })`). Live in the real path, not only in tests. Probes at the head: - untrusted author on the restatement item, trusted finding above it: the target finding stays open AND the restatement line is itself reported (base reported only the first). - trusted reviewer's restatement whose PRECEDING item is untrusted: closes. That is the discriminating case for "own item author" — a gate reading the neighbour's identity would come out the other way. - trusted reviewer still closes by all three forms (phrase, `resolved <id>` citation, restatement), and a member of a non-default `reviewerIdentities` set still counts. - an item with no author closes nothing — fail-closed as specified. ## D2 — the reported shapes are fixed and the fail-closed set is intact I re-ran every shape in the report and the run's five "must stay findings" headings. All nine no-defect headings are now section titles with zero findings; `## Blocker test applied on a repo the tenant filter still leaks`, `## Blocker scan_failed on the list query`, `## Blockers (severity: blocker) — …`, `### Blocker 1 — …` and `## Blockers: the guard only prints and never refuses` all still refuse. I also fuzzed 33 headings against BOTH 1d24858 and the head: the newly-swallowed set is exactly section noun + closed meta vocabulary, and no heading that states a defect (`check failed`, `review missed …`, `fix regressed`, `test evidence missing`, `verification incomplete`, `audit revealed a leak`, `status open`) is in it. #97's context rule holds: a real defect bullet under a meta heading is still a finding. The residual brittleness is recorded under Concerns; every miss I found is in the safe direction. ## D3 — the hint now matches what the code accepts Ran `evaluateLandingLock` and read the three refusal texts. id-less: "…it carries no id, so no citation can close it. A later comment BY THE REVIEWER (ld-reviewer) declaring the blocker resolved or fixed … is what does" — accurate: I confirmed a trusted later `blockers resolved` closes exactly that finding and that nothing else does. id-ed keeps the `resolved <id>` / `fixed-in <id>` wording; mixed keeps the give-it-an-id nudge and adds the id-less note. No unsatisfiable advice is left. ## Verification I ran `npm run test:deployed` in a clean worktree at the head: typecheck ok, 26/26 files loaded, 702/702 tests passed, pinned tree `typebox 1.3.34`. Forgejo CI on 86cf499: run #292 every job pass, #293 python pass — 14/14 green. No pre-existing red. ## Concerns 1. (D2, honest severity: nuisance, not safety) The closed vocabulary leaves plausible no-defect headings as findings that no citation can clear, identical to base: `## Blockers found`, `## Blocker found`, `## Blocker search`, `## Blocker verification complete`, `## Blocker check: clean`, `## Blocker re-scan`, and the shape named in the brief — `## Blocker analysis still running` ("still" and "running" are both outside the list). Verified: each reads as `isSectionOnlyHeading = false` on base and on the head, and each refuses with a finding carrying no label. None of them is a regression and none of them can swallow a defect, so this does not gate the change; a follow-up can widen the list (`found`, `identified`, `search`, `complete`, `done`, `clean`, `round`) or accept that the reviewer labels what it reports. 2. (D1, operational cost, reproduced) An untrusted restatement-shaped note — the fleet's own fix-worker closing-note shape — now becomes an open finding carrying the label's id. Where the reviewer closes the original finding only with the id-less phrase, that new finding stays open and the merge refuses until the reviewer cites the id. I reproduced both states. Fail-closed is the right direction here, but it is a new refusal path for a pipeline shape, worth knowing at the landing step. 3. (docs inside the diff) The new `SECTION_META_ONLY_RE` comment says an id-less finding "can never be closed", while this same commit's D3 hint and `isResolved`'s id-less arm say a trusted reviewer's phrase does close it — I verified the phrase closes it. The narrower true statement is "can never be closed by a citation". The new test titles likewise describe an intent ("a heading that reports no defect is never a finding") that a closed vocabulary does not fully implement. Wording only. 4. (observation, not gating) The restatement arm still skips `resolutionHeadOk`, while the id-ed citation arm applies the head discipline. Pre-existing at base and narrow — a restatement can only suppress its own line — noted because this diff is about making the restatement arm behave like its siblings.

ld-reviewer commented 2026-11-05 19:54:10 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 86cf49991d: APPROVE

model: deepseek-flash
session: 01a1ad36-eb9a-757a-b84d-276b1348e755

Exact-head seat 2 of 2, adversarial: I tried to close a finding as an untrusted identity around the new author gate, and to hide a real defect behind the new no-defect heading vocabulary. Every probe was a unit-level synthetic PR stream — no real PR was used and no network was touched.

Blockers

None.

Concerns

• The D1 headline is stronger than the code ever was (claim precision, not correctness). The restatement arm only ever suppressed ITS OWN line: isResolutionRestatement is evaluated against the line being tested, so a restatement never cleared a DIFFERENT, earlier finding — the citation and phrase arms, which are the ones that can, were already identity-gated in 1e7c397. I re-ran the whole attack matrix against the parent commit and every "the raised finding stays open" assertion already held there; what actually flipped at this head is that the untrusted restatement LINE is now reported. So the arm was a fail-open on reporting, not a live way to clear someone else's finding. The fix is still correct and strictly fail-closed; it is the framing ("any identity could clear a finding by posting that shape") that reads wider than the behaviour.
• Two untrusted shapes are still not reported as findings of their own (neither closes anything, so the fail-closed property holds, but the new test's toContain("1") should not be read as proof the line was reported): a label with no severity token (**B1 (census crash).** …) was never a finding on any path, and a bare resolved 1 — fixed in … line is a resolution line by design.
• verdict.ts:724 calls findOpenBlockers(items) without policy. Under a non-default FORGEJO_LANDING_REVIEWERS, the composer therefore reads the configured reviewer's own restatement as an open finding and would refuse to publish a --resolve-review verdict that the lock itself accepts. I confirmed the same divergence already exists for the phrase and citation arms, so this is a pre-existing pattern that this PR extends by one arm rather than a new class — one-line fix. The new "a policy member other than the default still counts" case does not cover the composer path because it passes the policy explicitly.
• D2 residual, unchanged by this PR and in the fail-closed direction: determination wordings outside the closed list still produce an id-less, unclosable refusal — ## Blocker test applied — no defect, — no defects, — nothing found, — none, and : clean. Only — no defect found is accepted, so the vocabulary boundary is one word wide; a reviewer who writes the shorter form meets exactly the defect D2 set out to remove.

Adversarial probes and what survived them

• Closure attack matrix, every case a synthetic stream: canonical restatement, bare label with no parenthetical, label with no trailing period, a short alias label, lowercase label, italic label, fixed in <sha>, a bare resolved 1, and a quoted-example resolution. Each ran as a plain comment, as a formal review body, and as an inline comment; each also as a raise-and-close in ONE body, and as two untrusted comments (one raising, one closing). Every variant left the raised finding open.
• An untrusted line carrying both a resolution word and the finding's exact text; an untrusted restatement of a section finding whose id comes from the section lead; an item with no author at all: all left it open, the last by construction.
• Trusted path unchanged: the reviewer's own restatement closes its own line as a comment and as a formal review, for the default identity, for a non-default policy identity and for a case-and-space variant of it; resolved <id> still closes a labelled finding; a trusted raise-and-close body still closes.
• D2 over-reach: 31 defect-reporting heading shapes held as findings — ## Blocker analysis: token leak, ## Blocker test applied to the list endpoint, ## Verification blocker: session fix, ## Blocker — verified, ## Blocker review findings, ## Blocker test failures, ## Blocker fix missing, ## Blocker: no verification, ## Blocker review of the tenant filter: leaks among them. Prose, bullets and ordered items under a no-defect heading still fail closed, and a defect sentence on the heading line itself stays a finding.
• Regex robustness: adversarial headings of 1 to 4 kB against the new vocabulary (400 repeated meta words, 200 repeated nouns, 400 leading qualifiers) peak at 4.1 ms, so an untrusted author cannot stall the scan.
• npm run test:deployed at this head: 26 of 26 files, 702 of 702 tests, typecheck clean.

**Verdict of record — ld-reviewer — head 86cf49991d6191c7c7db9ae671dce7dc9c85bb78: APPROVE** model: deepseek-flash session: 01a1ad36-eb9a-757a-b84d-276b1348e755 Exact-head seat 2 of 2, adversarial: I tried to close a finding as an untrusted identity around the new author gate, and to hide a real defect behind the new no-defect heading vocabulary. Every probe was a unit-level synthetic PR stream — no real PR was used and no network was touched. ## Blockers None. ## Concerns - **The D1 headline is stronger than the code ever was (claim precision, not correctness).** The restatement arm only ever suppressed ITS OWN line: `isResolutionRestatement` is evaluated against the line being tested, so a restatement never cleared a DIFFERENT, earlier finding — the citation and phrase arms, which are the ones that can, were already identity-gated in 1e7c397. I re-ran the whole attack matrix against the parent commit and every "the raised finding stays open" assertion already held there; what actually flipped at this head is that the untrusted restatement LINE is now reported. So the arm was a fail-open on reporting, not a live way to clear someone else's finding. The fix is still correct and strictly fail-closed; it is the framing ("any identity could clear a finding by posting that shape") that reads wider than the behaviour. - **Two untrusted shapes are still not reported as findings of their own** (neither closes anything, so the fail-closed property holds, but the new test's `toContain("1")` should not be read as proof the line was reported): a label with no severity token (`**B1 (census crash).** …`) was never a finding on any path, and a bare `resolved 1 — fixed in …` line is a resolution line by design. - **`verdict.ts:724` calls `findOpenBlockers(items)` without `policy`.** Under a non-default `FORGEJO_LANDING_REVIEWERS`, the composer therefore reads the configured reviewer's own restatement as an open finding and would refuse to publish a `--resolve-review` verdict that the lock itself accepts. I confirmed the same divergence already exists for the phrase and citation arms, so this is a pre-existing pattern that this PR extends by one arm rather than a new class — one-line fix. The new "a policy member other than the default still counts" case does not cover the composer path because it passes the policy explicitly. - **D2 residual, unchanged by this PR and in the fail-closed direction:** determination wordings outside the closed list still produce an id-less, unclosable refusal — `## Blocker test applied — no defect`, `— no defects`, `— nothing found`, `— none`, and `: clean`. Only `— no defect found` is accepted, so the vocabulary boundary is one word wide; a reviewer who writes the shorter form meets exactly the defect D2 set out to remove. ## Adversarial probes and what survived them - Closure attack matrix, every case a synthetic stream: canonical restatement, bare label with no parenthetical, label with no trailing period, a short alias label, lowercase label, italic label, `fixed in <sha>`, a bare `resolved 1`, and a quoted-example resolution. Each ran as a plain comment, as a formal review body, and as an inline comment; each also as a raise-and-close in ONE body, and as two untrusted comments (one raising, one closing). Every variant left the raised finding open. - An untrusted line carrying both a resolution word and the finding's exact text; an untrusted restatement of a section finding whose id comes from the section lead; an item with no author at all: all left it open, the last by construction. - Trusted path unchanged: the reviewer's own restatement closes its own line as a comment and as a formal review, for the default identity, for a non-default policy identity and for a case-and-space variant of it; `resolved <id>` still closes a labelled finding; a trusted raise-and-close body still closes. - D2 over-reach: 31 defect-reporting heading shapes held as findings — `## Blocker analysis: token leak`, `## Blocker test applied to the list endpoint`, `## Verification blocker: session fix`, `## Blocker — verified`, `## Blocker review findings`, `## Blocker test failures`, `## Blocker fix missing`, `## Blocker: no verification`, `## Blocker review of the tenant filter: leaks` among them. Prose, bullets and ordered items under a no-defect heading still fail closed, and a defect sentence on the heading line itself stays a finding. - Regex robustness: adversarial headings of 1 to 4 kB against the new vocabulary (400 repeated meta words, 200 repeated nouns, 400 leading qualifiers) peak at 4.1 ms, so an untrusted author cannot stall the scan. - `npm run test:deployed` at this head: 26 of 26 files, 702 of 702 tests, typecheck clean.

pi-landing merged commit 1ff5dd473f into main 2026-11-05 19:58:51 +00:00

pi-landing referenced this pull request from a commit 2026-11-05 19:58:53 +00:00

Merge pull request 'HOMELAB-2682: fix(forgejo): three lock defects (resolution author check, no-defect headings, id-less hint)' (#99) from plane/HOMELAB-2682-lock-defects into main

aaron referenced this pull request from a commit 2026-11-05 20:09:49 +00:00

HOMELAB-2682: fix(forgejo): id-less findings close only via the canonical review-citation sentence

pi-landing referenced this pull request 2026-11-05 20:10:13 +00:00

HOMELAB-2682: fix(forgejo): id-less findings close only via the canonical review-citation sentence #100

ld-reviewer referenced this pull request 2026-11-05 20:59:56 +00:00

HOMELAB-2682: fix(forgejo): id-less findings close only via the canonical review-citation sentence #100

ld-reviewer referenced this pull request 2026-11-05 21:03:22 +00:00

HOMELAB-2682: fix(forgejo): id-less findings close only via the canonical review-citation sentence #100

pi-landing referenced this pull request 2026-11-06 03:09:12 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

pi-landing referenced this pull request 2026-11-06 03:14:26 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

ld-reviewer referenced this pull request 2026-11-06 03:25:03 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

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

No description provided.