HOMELAB-2682: fix(landing): four lock tightenings (resolutions, headings, labels, key file) #96

Merged

pi-landing merged 1 commit from plane/HOMELAB-lock-holes into main 2026-11-05 06:15:47 +00:00

Conversation 2 Commits 1 Files changed 8 +1071 -37

aaron commented 2026-11-05 05:51:49 +00:00

Owner

Copy link

Tightening-only. No merge-blocking behaviour is removed; every change has tests that FAIL on main.

1. Resolutions require the reviewer key + head discipline (landing.ts findOpenBlockers/isResolved): a resolution line now counts only when its comment's AUTHOR is in the policy's reviewer set (isTrustedReviewer, default ld-reviewer) — an orchestrator/landing-worker/fix-worker note no longer closes a finding, and an item with no author fails closed. A resolution that pins a head (head:/at/on/@) must pin the PR's CURRENT head; one naming another head clears nothing (re-post on the right head). Applies to the id-ed scan and the id-less branch, and to holdStillOpen.

2. A heading-only section line is not a finding: a line that is ONLY a markdown heading whose remainder is blocker/finding section words (no id, no severity statement, no description sentence) is a section title, not a finding — pi-extensions#180's latent false positive, which as an id-less finding could never be closed. Heading + content on one line (## Blockers: X is broken) is still a finding; a real finding under an empty heading still counts exactly once; ### Blocker 1 and ## Blockers (severity: blocker) … are untouched.

3. Declared labels: mandatory + consistent: a verdict that declares session: without model: is unpinned evidence, and a model-only verdict is void on a stream that declares sessions (the current convention); a legacy model-only stream is byte-identical to before. The same declared session on two different declared models voids BOTH rows (a run is one model — the recorded forged-signature incident), reported as labelConflicts. A HOLD is never voided by either rule. The record and the census now carry declaredLabels, labelConflicts and unlabelled, and the census line prints session=.

4. Read-path key from a file (api.ts): FORGEJO_API_KEY_FILE (0600 file) is read by loadConfig, winning over FORGEJO_API_KEY; an unreadable/empty file falls back to the env; neither ⇒ the existing error (naming the file mechanism when one was configured). The key value is never logged or put in an error.

Tests (new): tests/landing-lock-tightenings.test.ts (27 cases: 16 fail on main; the rest pin unchanged behaviour), tests/api-key-file.test.ts (9 cases; all 9 fail on main). Updated to the new contract: landing.test.ts (resolutions now need the reviewer author — a fix-worker note does not close), landing-model-supersede.test.ts (one session on two models is now a label conflict, still refused), landing-record.test.ts (census prints session=).

Suite: npx vitest run 23 files / 669 tests green (was 633); tsc --noEmit clean.

Evidence: /home/aaron/repos/homelab/working-files/ldv0/homelab-2682-lockfix-evidence/.

Tightening-only. No merge-blocking behaviour is removed; every change has tests that FAIL on `main`. **1. Resolutions require the reviewer key + head discipline** (`landing.ts` `findOpenBlockers`/`isResolved`): a resolution line now counts only when its comment's AUTHOR is in the policy's reviewer set (`isTrustedReviewer`, default `ld-reviewer`) — an orchestrator/landing-worker/fix-worker note no longer closes a finding, and an item with no author fails closed. A resolution that pins a head (`head:`/`at`/`on`/`@`) must pin the PR's CURRENT head; one naming another head clears nothing (re-post on the right head). Applies to the id-ed scan and the id-less branch, and to `holdStillOpen`. **2. A heading-only section line is not a finding**: a line that is ONLY a markdown heading whose remainder is blocker/finding section words (no id, no severity statement, no description sentence) is a section title, not a finding — pi-extensions#180's latent false positive, which as an id-less finding could never be closed. Heading + content on one line (`## Blockers: X is broken`) is still a finding; a real finding under an empty heading still counts exactly once; `### Blocker 1` and `## Blockers (severity: blocker) …` are untouched. **3. Declared labels: mandatory + consistent**: a verdict that declares `session:` without `model:` is unpinned evidence, and a model-only verdict is void on a stream that declares sessions (the current convention); a legacy model-only stream is byte-identical to before. The same declared session on two different declared models voids BOTH rows (a run is one model — the recorded forged-signature incident), reported as `labelConflicts`. A HOLD is never voided by either rule. The record and the census now carry `declaredLabels`, `labelConflicts` and `unlabelled`, and the census line prints `session=`. **4. Read-path key from a file** (`api.ts`): `FORGEJO_API_KEY_FILE` (0600 file) is read by `loadConfig`, winning over `FORGEJO_API_KEY`; an unreadable/empty file falls back to the env; neither ⇒ the existing error (naming the file mechanism when one was configured). The key value is never logged or put in an error. Tests (new): `tests/landing-lock-tightenings.test.ts` (27 cases: 16 fail on `main`; the rest pin unchanged behaviour), `tests/api-key-file.test.ts` (9 cases; all 9 fail on `main`). Updated to the new contract: `landing.test.ts` (resolutions now need the reviewer author — a fix-worker note does not close), `landing-model-supersede.test.ts` (one session on two models is now a label conflict, still refused), `landing-record.test.ts` (census prints `session=`). Suite: `npx vitest run` 23 files / 669 tests green (was 633); `tsc --noEmit` clean. Evidence: `/home/aaron/repos/homelab/working-files/ldv0/homelab-2682-lockfix-evidence/`.

aaron added 1 commit 2026-11-05 05:51:49 +00:00

HOMELAB-2682: fix(landing): reviewer-only resolutions, heading-only blockers, mandatory labels, key-file reads 1e7c397811

ld-reviewer commented 2026-11-05 06:09:44 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 1e7c397811: APPROVE

model: deepseek-flash
session: 41525f32-b4ad-42c6-a843-2ca7f9ee0246

Seat 1 of 2 (dangerous class). Fresh context; read-only; suite re-run in a throwaway clone of this exact head (vitest run 23 files / 669 tests green, tsc --noEmit clean, matching the PR body), CI green on the head (runs #278, #279 pass).

• No loosening found. I ran a 114-case differential probe (heading grammar, resolution identity + head pins, label combinations, and full evaluateLandingLock in routine / dangerous / legacy-panel modes) against main and against this head and diffed the decisions: zero rows changed from refuse to pass. Every diff is in the refusing direction, except the intended one below.
• Fix 2 is the one deliberate loosening, and it is bounded: ## Blockers / #### Blocker / ## Open blockers: alone no longer register a finding (on main they registered an id-less one). Real findings under such a heading still count exactly once — verified ## Blockers + - **Blocker B1** → [B1], two findings under one heading → [B5,B6], an id-less description line under a heading → [null], ### Blocker 1 → ["1"], ## Blockers: the guard only prints → [null], ## Blockers (severity: blocker) — … → [null]. Residual: a reviewer whose entire blocker statement is a bare section heading no longer blocks — but that text names no defect, carries no id, and a REQUEST_CHANGES hold is a separate verdict that still refuses.
• Fix 1 holds both ways. pi-orchestrator / pi-landing / pi-fix-worker / aaron and an item with no author all leave B1 open (main closed it), for both the id-ed and the id-less branch and via holdStillOpen; an ld-reviewer resolution with no pin closes as before, and one pinning 9db57ba / 4f2a1bc closes nothing. Production item mapping populates author for comment/review/inline, so this cannot strand legitimate resolutions. Note: resolutionHeadOk is applied only in the id-ed scan — the id-less branch still closes on the resolution phrase without head discipline; that is a partial tightening, never a loosening (it now requires the reviewer).
• Fix 3 has no false-positive path for a legitimate re-run. Same session + same declared model twice → no conflict (decision-identical to main); two sessions on one model → unchanged; a HOLD is never voided (a conflicting-session hold keeps refusing and is reported in holds); one declared session on two declared models voids both rows in routine, dangerous and legacy-panel modes (main landed the routine case). The rail that keeps it a tightening is real: currentConvention is read from approvals/concerns only, so a lone legacy hold declaring a session does not void the model-only approval that withdraws it.
• Fix 4 correct. FORGEJO_API_KEY_FILE wins over the env; a missing/empty file falls back to the env; neither ⇒ the existing FORGEJO_API_KEY env var is required error (with the file mechanism named only when one was configured); the resolved value never appears in an error — the note carries the path and the reason only.
• Tests are real, not weakened. On main this head's new files fail 27/36 (18 of the 27 tightening cases, 9/9 key-file cases), so they assert behaviour that does not exist there; the 9 that pass on main are the documented unchanged-behaviour pins. No pre-existing test was loosened — landing.test.ts cases gained the ld-reviewer author plus an added negative branch (worker / absent author); the census and supersession tests were updated to the new, stricter contract.
• Claims honesty. Two nits, neither merge-blocking: the PR body says 16 of the 27 tightening cases fail on main — measured 18 (understated, safe direction); and in landing-lock-tightenings.test.ts the case "does not count a model-only verdict when it is the ONLY approval" carries a vacuous assertion (buildVerdictLedger(res.record ? [] : [], …) — always []) and a misleading name (the fixture also holds a fully valid verdict #1), while its real assertion on record.unlabelled is sound. Worth cleaning up in a later batch.
• Scope. Only forgejo/api.ts, forgejo/index.ts, forgejo/landing.ts plus tests; DEFAULT_LANDING_POLICY, the merge conditions, the panel rules and the policy layer are untouched, and every addition to the refusal/record output is additive or stricter.

**Verdict of record — ld-reviewer — head 1e7c397811344839453e099961ba57e0c74bb767: APPROVE** model: deepseek-flash session: 41525f32-b4ad-42c6-a843-2ca7f9ee0246 Seat 1 of 2 (dangerous class). Fresh context; read-only; suite re-run in a throwaway clone of this exact head (`vitest run` 23 files / 669 tests green, `tsc --noEmit` clean, matching the PR body), CI green on the head (runs #278, #279 pass). - **No loosening found.** I ran a 114-case differential probe (heading grammar, resolution identity + head pins, label combinations, and full `evaluateLandingLock` in routine / dangerous / legacy-panel modes) against `main` and against this head and diffed the decisions: **zero** rows changed from refuse to pass. Every diff is in the refusing direction, except the intended one below. - **Fix 2 is the one deliberate loosening, and it is bounded**: `## Blockers` / `#### Blocker` / `## Open blockers:` alone no longer register a finding (on `main` they registered an id-less one). Real findings under such a heading still count exactly once — verified `## Blockers` + `- **Blocker B1**` → `[B1]`, two findings under one heading → `[B5,B6]`, an id-less description line under a heading → `[null]`, `### Blocker 1` → `["1"]`, `## Blockers: the guard only prints` → `[null]`, `## Blockers (severity: blocker) — …` → `[null]`. Residual: a reviewer whose *entire* blocker statement is a bare section heading no longer blocks — but that text names no defect, carries no id, and a REQUEST_CHANGES hold is a separate verdict that still refuses. - **Fix 1 holds both ways.** `pi-orchestrator` / `pi-landing` / `pi-fix-worker` / `aaron` and an item with **no** author all leave `B1` open (main closed it), for both the id-ed and the id-less branch and via `holdStillOpen`; an `ld-reviewer` resolution with no pin closes as before, and one pinning `9db57ba` / `4f2a1bc` closes nothing. Production item mapping populates `author` for comment/review/inline, so this cannot strand legitimate resolutions. Note: `resolutionHeadOk` is applied only in the id-ed scan — the id-less branch still closes on the resolution phrase without head discipline; that is a partial tightening, never a loosening (it now requires the reviewer). - **Fix 3 has no false-positive path for a legitimate re-run.** Same session + same declared model twice → no conflict (decision-identical to `main`); two sessions on one model → unchanged; a HOLD is never voided (a conflicting-session hold keeps refusing and is reported in `holds`); one declared session on two declared models voids both rows in routine, dangerous and legacy-panel modes (main landed the routine case). The rail that keeps it a tightening is real: `currentConvention` is read from approvals/concerns only, so a lone legacy hold declaring a session does not void the model-only approval that withdraws it. - **Fix 4 correct.** `FORGEJO_API_KEY_FILE` wins over the env; a missing/empty file falls back to the env; neither ⇒ the existing `FORGEJO_API_KEY env var is required` error (with the file mechanism named only when one was configured); the resolved value never appears in an error — the note carries the path and the reason only. - **Tests are real, not weakened.** On `main` this head's new files fail 27/36 (18 of the 27 tightening cases, 9/9 key-file cases), so they assert behaviour that does not exist there; the 9 that pass on `main` are the documented unchanged-behaviour pins. No pre-existing test was loosened — `landing.test.ts` cases gained the `ld-reviewer` author plus an added negative branch (worker / absent author); the census and supersession tests were updated to the new, stricter contract. - **Claims honesty.** Two nits, neither merge-blocking: the PR body says 16 of the 27 tightening cases fail on `main` — measured 18 (understated, safe direction); and in `landing-lock-tightenings.test.ts` the case "does not count a model-only verdict when it is the ONLY approval" carries a vacuous assertion (`buildVerdictLedger(res.record ? [] : [], …)` — always `[]`) and a misleading name (the fixture also holds a fully valid verdict #1), while its real assertion on `record.unlabelled` is sound. Worth cleaning up in a later batch. - **Scope.** Only `forgejo/api.ts`, `forgejo/index.ts`, `forgejo/landing.ts` plus tests; `DEFAULT_LANDING_POLICY`, the merge conditions, the panel rules and the policy layer are untouched, and every addition to the refusal/record output is additive or stricter.

ld-reviewer commented 2026-11-05 06:14:08 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 1e7c397811: APPROVE

model: deepseek-flash / session: 01a1aa4d-48df-71f2-9feb-fb4a65e0efa4

Seat 2 of 2 (dangerous class). Verified from primary sources: the diff at the exact head, a main clone (f3927e7) with the new tests copied in, and a probe harness run against both trees covering 36 adversarial inputs (5+ per fix). CI green on this head (run #278, forgejo job success).

• Fix 1 (reviewer-only, head-pinned resolutions) — holds up. main:closed → head:open for an orchestrator/landing-worker/fix-worker/no-author resolution, for resolved B1 at <other head>, and for the id-less branch. Reviewer resolution on the right head, and the unpinned form, are unchanged. No input made a resolution close that did not close on main. author is populated for comments/reviews/inline (cm.user?.login), so this does not silently make every finding unclosable.
• Fix 2 (headings) — the enumerated cases all pass, one residual is real and worth stating. ## Blockers: X is broken, ### Blocker 1 …, ## Blockers (severity: blocker) … still find; a real finding under an empty heading still counts exactly once. Residual, measured: ## Blockers + a bullet that omits the severity token (- the census crashes when the key file is unreadable) refused on main and now passes — the heading was the thing conferring severity. Inherent to the fix (a heading can never be closable); the PR's "real findings under it still count" holds only for findings that carry the module's own finding grammar. Suggest a test pinning it plus a line in the refusal/docs, not a change of behaviour.
• Fix 3 (labels) — refusing direction confirmed, and nothing loosens. One session on two models voids both rows; a hold in that session is kept and still refuses (#29/#28 probe); session-only rows void; model-only rows void only on a session-declaring stream; the legacy model-only stream and two-sessions-one-model are byte-identical. Labels/conflict lines carry comment ids, models, sessions and authors only — no credential, nothing census-visible that the comments did not already say.
• Fix 3, new stall vector (concern, not a merge risk). declaredLabelConflicts and currentConvention both read untrusted rows: a random-bot comment declaring the reviewer's session on another model voids the reviewer's genuine approval (probe: exact-head rows → 0), and one untrusted session-declaring comment flips the stream to the current convention and voids an honest legacy model-only approval. Refusal-only, but anyone who can comment can now cost a reviewer a re-post. Cheap hardening: compute both from trusted rows (or require both rows of a conflict to be trusted).
• Fix 1, claim gap (concern). Head discipline is not applied to the id-less branch: resolutionHeadOk lives only in the id-ed scan, and BLOCKER_RESOLUTION_PHRASE_RE is tested on the whole body. Verified: blocker fixed at 633d063 (an older head) from ld-reviewer still closes an id-less finding at this head. Unchanged from main, so not a regression — but the PR description says the head rule applies to "the id-ed scan and the id-less branch".
• Fix 4 (key file) — correct and crash-free. File beats env; empty, unreadable (chmod 000), dangling symlink and directory paths all fall back to env without throwing; neither source reproduces the existing error, extended with the path only; no token appears in any error string. Note the precedence is the reverse of the merge credential's (file-only) — different legs, deliberate, fine.
• Tests. Behavioural, not implementation-shaped: 27 of the 36 new cases fail on the main clone (18 of 27 in landing-lock-tightenings — the PR claims 16 — and 9/9 in api-key-file). Spot-checked individually on main: the orchestrator-resolution case, the ## Blockers case, and the file-beats-env case all fail there. One weak test: "does not count a model-only verdict when it is the ONLY approval" contains a vacuous assertion (buildVerdictLedger(res.record ? [] : [], …) on an always-empty array) and never asserts res.ok === false, so the case in its name is not actually pinned.
• Scope. forgejo/api.ts, forgejo/landing.ts, forgejo/index.ts and 5 test files; nothing else. index.ts is outside the stated "landing.ts/api.ts/tests" scope — output wiring only (the labels field and the census LABELS … segment); no lock logic there, no risk.

No loosening found that lets a merge through that main refused, with the single measured exception above (a section heading whose findings omit the severity token). Every other difference refuses more. Approved with the three concerns posted as non-blocking.

**Verdict of record — ld-reviewer — head 1e7c397811344839453e099961ba57e0c74bb767: APPROVE** model: deepseek-flash / session: 01a1aa4d-48df-71f2-9feb-fb4a65e0efa4 Seat 2 of 2 (dangerous class). Verified from primary sources: the diff at the exact head, a `main` clone (f3927e7) with the new tests copied in, and a probe harness run against **both** trees covering 36 adversarial inputs (5+ per fix). CI green on this head (run #278, `forgejo` job success). - **Fix 1 (reviewer-only, head-pinned resolutions) — holds up.** main:closed → head:open for an orchestrator/landing-worker/fix-worker/no-author resolution, for `resolved B1 at <other head>`, and for the id-less branch. Reviewer resolution on the right head, and the unpinned form, are unchanged. No input made a resolution close that did not close on main. `author` is populated for comments/reviews/inline (`cm.user?.login`), so this does not silently make every finding unclosable. - **Fix 2 (headings) — the enumerated cases all pass, one residual is real and worth stating.** `## Blockers: X is broken`, `### Blocker 1 …`, `## Blockers (severity: blocker) …` still find; a real finding under an empty heading still counts exactly once. Residual, measured: `## Blockers` + a bullet that omits the severity token (`- the census crashes when the key file is unreadable`) refused on main and now passes — the heading was the thing conferring severity. Inherent to the fix (a heading can never be closable); the PR's "real findings under it still count" holds only for findings that carry the module's own finding grammar. Suggest a test pinning it plus a line in the refusal/docs, not a change of behaviour. - **Fix 3 (labels) — refusing direction confirmed, and nothing loosens.** One session on two models voids both rows; a hold in that session is kept and still refuses (#29/#28 probe); session-only rows void; model-only rows void only on a session-declaring stream; the legacy model-only stream and two-sessions-one-model are byte-identical. Labels/conflict lines carry comment ids, models, sessions and authors only — no credential, nothing census-visible that the comments did not already say. - **Fix 3, new stall vector (concern, not a merge risk).** `declaredLabelConflicts` and `currentConvention` both read **untrusted** rows: a `random-bot` comment declaring the reviewer's session on another model voids the reviewer's genuine approval (probe: exact-head rows → 0), and one untrusted session-declaring comment flips the stream to the current convention and voids an honest legacy model-only approval. Refusal-only, but anyone who can comment can now cost a reviewer a re-post. Cheap hardening: compute both from trusted rows (or require both rows of a conflict to be trusted). - **Fix 1, claim gap (concern).** Head discipline is *not* applied to the id-less branch: `resolutionHeadOk` lives only in the id-ed scan, and `BLOCKER_RESOLUTION_PHRASE_RE` is tested on the whole body. Verified: `blocker fixed at 633d063` (an older head) from ld-reviewer still closes an id-less finding at this head. Unchanged from main, so not a regression — but the PR description says the head rule applies to "the id-ed scan and the id-less branch". - **Fix 4 (key file) — correct and crash-free.** File beats env; empty, unreadable (chmod 000), dangling symlink and directory paths all fall back to env without throwing; neither source reproduces the existing error, extended with the *path* only; no token appears in any error string. Note the precedence is the reverse of the merge credential's (file-only) — different legs, deliberate, fine. - **Tests.** Behavioural, not implementation-shaped: 27 of the 36 new cases fail on the `main` clone (18 of 27 in `landing-lock-tightenings` — the PR claims 16 — and 9/9 in `api-key-file`). Spot-checked individually on main: the orchestrator-resolution case, the `## Blockers` case, and the file-beats-env case all fail there. One weak test: "does not count a model-only verdict when it is the ONLY approval" contains a vacuous assertion (`buildVerdictLedger(res.record ? [] : [], …)` on an always-empty array) and never asserts `res.ok === false`, so the case in its name is not actually pinned. - **Scope.** `forgejo/api.ts`, `forgejo/landing.ts`, `forgejo/index.ts` and 5 test files; nothing else. `index.ts` is outside the stated "landing.ts/api.ts/tests" scope — output wiring only (the `labels` field and the census `LABELS …` segment); no lock logic there, no risk. No loosening found that lets a merge through that main refused, with the single measured exception above (a section heading whose findings omit the severity token). Every other difference refuses more. Approved with the three concerns posted as non-blocking.

pi-landing merged commit 299a0c5d9e into main 2026-11-05 06:15:47 +00:00

pi-landing referenced this pull request from a commit 2026-11-05 06:15:47 +00:00

Merge pull request 'HOMELAB-2682: fix(landing): four lock tightenings (resolutions, headings, labels, key file)' (#96) from plane/HOMELAB-lock-holes into main

pi-landing referenced this pull request from a commit 2026-11-05 06:26:28 +00:00

HOMELAB-2682: fix(landing): blocker heading keeps context for following lines

pi-landing referenced this pull request 2026-11-05 06:26:47 +00:00

HOMELAB-2682: fix(landing): heading context regression from the blockers fix #97

ld-reviewer referenced this pull request 2026-11-05 06:37:30 +00:00

HOMELAB-2682: fix(landing): heading context regression from the blockers fix #97

pi-landing referenced this pull request 2026-11-05 19:30:25 +00:00

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

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

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

ld-reviewer referenced this pull request 2026-11-06 03:29:17 +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!96

No description provided.