HOMELAB-2554: fix(plane): harden comment verification evidence #79

Merged

aaron merged 1 commit from plane/HOMELAB-2554-comment-identity-hardening into main 2026-11-03 04:11:46 +00:00

Conversation 5 Commits 1 Files changed 4 +530 -30

aaron commented 2026-11-03 02:39:12 +00:00

Owner

Copy link

Closes the four non-blocking review notes from #77 (HOMELAB-2529).

N1 — a read-back failure is not "nothing happened"

After a successful POST, a failing read-back GET (5xx / network / timeout) no longer
propagates the raw transport error. It says the comment MAY EXIST, names the
comment id, and forbids a blind retry. A POST answered without an id is treated the
same way (may have landed, attribution unverifiable).

N2 — verification marker form

verificationMarkerForm() classifies heading | attribute | null. Only the
<h2>Verification</h2> heading form is observed surviving Plane's write-time lxml
re-serialization, so an attribute-only body (data-ld-verification with no heading)
is refused before any read or write — zero writes — with a message telling the
caller to re-send under a heading. Read-side tolerance for the attribute form is
unchanged (older comments still count). The marker-stripped-after-write error now
says the write HAPPENED and must not be re-posted (same body sanitizes identically),
instead of inviting a duplicate.

N3 — expected_identity contract

docs/plane.md and the plane_comment prompt guidelines state the contract and that
PLANE_EXPECTED_IDENTITY is usually unset by design — a refusal naming it is the
guard working, not a Plane outage; nothing was written, so re-send with the correct
expected_identity.

N4 — the proof binds the posted body

commentBodyHash() (sha256) is recorded on a successful assertion; postVerifiedComment
refuses a body that differs, and refuses an unbound assertion posting a
verification record. Ordinary (non-evidence) comments from an unbound assertion still post.

Red-first

plane/tests/comment-hardening.test.ts (fake API, no network): 10 of 14 failed on
the pre-fix source (f910731) — red log in working-files/homelab-2518/HOMELAB-2554-red.log.

Green (this head)

• npx vitest run tests/comment-hardening.test.ts → 14/14 passed
• npx vitest run (full plane suite) → 50/50 passed (3 files)
• npx tsc --noEmit → clean

Files: plane/api.ts, plane/index.ts, plane/tests/comment-hardening.test.ts,
docs/plane.md. No live skill/extension state touched.

Not self-reviewed, not merged — for independent review.

Closes the four non-blocking review notes from #77 (HOMELAB-2529). ## N1 — a read-back failure is not "nothing happened" After a successful POST, a failing read-back GET (5xx / network / timeout) no longer propagates the raw transport error. It says the comment **MAY EXIST**, names the comment id, and forbids a blind retry. A POST answered without an id is treated the same way (may have landed, attribution unverifiable). ## N2 — verification marker form `verificationMarkerForm()` classifies `heading | attribute | null`. Only the `<h2>Verification</h2>` heading form is observed surviving Plane's write-time lxml re-serialization, so an attribute-only body (`data-ld-verification` with no heading) is refused **before** any read or write — zero writes — with a message telling the caller to re-send under a heading. Read-side tolerance for the attribute form is unchanged (older comments still count). The marker-stripped-after-write error now says the write HAPPENED and must not be re-posted (same body sanitizes identically), instead of inviting a duplicate. ## N3 — expected_identity contract `docs/plane.md` and the `plane_comment` prompt guidelines state the contract and that `PLANE_EXPECTED_IDENTITY` is usually unset **by design** — a refusal naming it is the guard working, not a Plane outage; nothing was written, so re-send with the correct `expected_identity`. ## N4 — the proof binds the posted body `commentBodyHash()` (sha256) is recorded on a successful assertion; `postVerifiedComment` refuses a body that differs, and refuses an **unbound** assertion posting a verification record. Ordinary (non-evidence) comments from an unbound assertion still post. ## Red-first `plane/tests/comment-hardening.test.ts` (fake API, no network): **10 of 14 failed** on the pre-fix source (f910731) — red log in `working-files/homelab-2518/HOMELAB-2554-red.log`. ## Green (this head) - `npx vitest run tests/comment-hardening.test.ts` → 14/14 passed - `npx vitest run` (full plane suite) → 50/50 passed (3 files) - `npx tsc --noEmit` → clean Files: `plane/api.ts`, `plane/index.ts`, `plane/tests/comment-hardening.test.ts`, `docs/plane.md`. No live skill/extension state touched. Not self-reviewed, not merged — for independent review.

aaron added 1 commit 2026-11-03 02:39:12 +00:00

HOMELAB-2554: fix(plane): harden comment verification evidence ... ebef6f4c5e

Close the four non-blocking review notes from PR #77 (HOMELAB-2529):

- A read-back GET failure after a successful POST no longer propagates the
  raw transport error. It says the comment MAY EXIST, names the id, and
  forbids a blind retry (same for a POST answered without an id).
- Verification marker form is classified: only the `## Verification`
  heading is observed surviving Plane's write-time HTML sanitization, so an
  attribute-only (data-ld-verification) body is refused BEFORE the write with
  zero writes instead of landing un-marked and unrepairable. Read-side
  tolerance for the attribute form is unchanged, and the Done-gate refusal no
  longer advertises it as a write form.
- The minted proof binds the asserted body by sha256: a different posted
  body, or an unbound assertion posting a verification record, refuses with
  zero writes. Ordinary comments from an unbound assertion still post.
- docs/plane.md and the tool guidance state the expected_identity contract and
  that PLANE_EXPECTED_IDENTITY is normally unset by design - a refusal naming
  it is the guard, not a Plane outage.

Red-first: plane/tests/comment-hardening.test.ts, 10 of 14 failing on the
pre-fix source. Plane suite 50/50, tsc clean.

ld-reviewer commented 2026-11-03 02:56:49 +00:00

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: ebef6f4c5e

Independent GENERAL review, fresh context. Reviewed the exact live head ebef6f4c5eefc1641a636afbaa6d4800b3d01ec0 (re-fetched: the branch ref still resolves to that sha, forgejo/main still f910731, branch exactly 1 ahead). No edits, no merge, no subagents, no other PR. CI run #179 at this head is green in all 14 jobs, including plane. Read-only re-run in the worker worktree: plane suite 50/50 (17 identity, 14 hardening, 19 closure) and tsc --noEmit exit 0. I corroborated the red-first claim from source rather than from the report: git show f910731:plane/api.ts has no verificationMarkerForm, no commentBodyHash, no bodyHash, no MAY EXIST, and still says "answered without an id — the write may have landed" and "re-post the evidence", so the new tests genuinely fail on pre-fix source. The diff touches exactly the ticket's declared file set (plane/api.ts, plane/index.ts, plane/tests/comment-hardening.test.ts, docs/plane.md) and no live skill or extension state.

The four accepted #77 notes, attacked individually.

N1, read-back transport and missing id. postVerifiedComment now wraps the read-back GET and, on 5xx, a thrown network error, or a POST answered without an id, states that the comment MAY EXIST, names the comment id where one exists, and forbids a blind retry in words ("do NOT retry this call"), routing recovery through id-based reconciliation instead of a second POST. The tests pin exactly one comment POST in all three cases, so the message cannot come from a path that also retried. This is the note correctly closed.

N2, marker form. verificationMarkerForm() returns heading, attribute, or null, and tests the heading form first, so a body carrying both is treated as the form Plane is observed to preserve. Attribute-only is refused inside assertCommentIdentity before getCurrentUser and before any comment call; the test asserts zero /users/me/ and zero /comments requests, so the refusal is decidable from the body alone with zero writes. Read-side hasVerificationMarker delegates to the same function, so the HOMELAB-1679 Done gate still accepts already-stored attribute-only comments, guard and gate cannot disagree about what evidence is, and no existing ticket becomes retroactively unclosable. The marker-stripped-after-write error now says the write HAPPENED and must not be retried, which removes the duplicate invitation; one POST is pinned there too.

N3, expected_identity guidance. The env table, the contract section, and the tool's prompt guidelines all state that the variable is usually unset by design and that a refusal naming it is the guard working rather than a Plane outage, and the parameter description no longer implies the env is the source. The test reads the tool's own declaration, so drift breaks it.

N4, sha256 proof binds the body. The hash is computed in assertCommentIdentity from the body it was given and carried on the branded proof; postVerifiedComment recomputes it and refuses a mismatch, and refuses an unbound assertion posting a verification record while still allowing it for an ordinary comment, which is pinned as caller compatibility. Both real write paths pass the same variable to the assertion and to the POST (params.comment_html for plane_comment, the sidebar's own html), so there is neither a false refusal nor a hash race, and createComment is no longer reachable except through postVerifiedComment, so no marker-bypass write path remains.

Other classes checked. Duplicate comments: one POST pinned per error class. Unverified success: success requires the stored actor to equal the token's user id and nothing reports success on a read it did not perform. Marker bypass: both write paths are guarded identically and the sidebar is not a second unguarded door. Caller compatibility: the only cross-extension import of this module is pi-otel-telemetry, and only for PROJECT_MAP; forgejo/api.ts is a separate parallel implementation and is unaffected. The moved VERIFICATION_HTML_HEADING_RE is referenced only at call time from a hoisted function declaration, so there is no TDZ exposure, which tsc and the suite agree on.

Four notes recorded for follow-up, none of which is a defect this change introduces. First, if createComment itself throws (timeout or 5xx on the POST), the raw transport error still propagates without MAY-EXIST wording, which is the same duplicate hazard now closed for the read-back leg; that leg predates this PR and sits outside the four notes, so it deserves its own ticket rather than being charged to this one. Second, an ordinary comment whose prose merely mentions the attribute name and carries no heading is now refused on write even with a declared identity; the refusal writes nothing and tells the caller how to recover, and the old code already special-cased that string, so this narrows behaviour rather than creating a new class. Third, "only an unintended duplicate of that exact id may be removed" is imprecise wording, since a real duplicate would carry a different id; the direction is conservative (do not delete), so it is safe. Fourth, the read-back does not assert the returned row's id equals the requested id, though the actor check would catch a wrong row in practice.

No open blockers. The change is a faithful, tested narrowing of the comment guard with no bypass and no caller break.

## Verdict: APPROVED model: omniroute/deepseek-flash head: ebef6f4c5eefc1641a636afbaa6d4800b3d01ec0 Independent GENERAL review, fresh context. Reviewed the exact live head `ebef6f4c5eefc1641a636afbaa6d4800b3d01ec0` (re-fetched: the branch ref still resolves to that sha, `forgejo/main` still `f910731`, branch exactly 1 ahead). No edits, no merge, no subagents, no other PR. CI run #179 at this head is green in all 14 jobs, including `plane`. Read-only re-run in the worker worktree: plane suite 50/50 (17 identity, 14 hardening, 19 closure) and `tsc --noEmit` exit 0. I corroborated the red-first claim from source rather than from the report: `git show f910731:plane/api.ts` has no `verificationMarkerForm`, no `commentBodyHash`, no `bodyHash`, no `MAY EXIST`, and still says "answered without an id — the write may have landed" and "re-post the evidence", so the new tests genuinely fail on pre-fix source. The diff touches exactly the ticket's declared file set (`plane/api.ts`, `plane/index.ts`, `plane/tests/comment-hardening.test.ts`, `docs/plane.md`) and no live skill or extension state. The four accepted #77 notes, attacked individually. N1, read-back transport and missing id. `postVerifiedComment` now wraps the read-back GET and, on 5xx, a thrown network error, or a POST answered without an id, states that the comment MAY EXIST, names the comment id where one exists, and forbids a blind retry in words ("do NOT retry this call"), routing recovery through id-based reconciliation instead of a second POST. The tests pin exactly one comment POST in all three cases, so the message cannot come from a path that also retried. This is the note correctly closed. N2, marker form. `verificationMarkerForm()` returns heading, attribute, or null, and tests the heading form first, so a body carrying both is treated as the form Plane is observed to preserve. Attribute-only is refused inside `assertCommentIdentity` before `getCurrentUser` and before any comment call; the test asserts zero `/users/me/` and zero `/comments` requests, so the refusal is decidable from the body alone with zero writes. Read-side `hasVerificationMarker` delegates to the same function, so the HOMELAB-1679 Done gate still accepts already-stored attribute-only comments, guard and gate cannot disagree about what evidence is, and no existing ticket becomes retroactively unclosable. The marker-stripped-after-write error now says the write HAPPENED and must not be retried, which removes the duplicate invitation; one POST is pinned there too. N3, expected_identity guidance. The env table, the contract section, and the tool's prompt guidelines all state that the variable is usually unset by design and that a refusal naming it is the guard working rather than a Plane outage, and the parameter description no longer implies the env is the source. The test reads the tool's own declaration, so drift breaks it. N4, sha256 proof binds the body. The hash is computed in `assertCommentIdentity` from the body it was given and carried on the branded proof; `postVerifiedComment` recomputes it and refuses a mismatch, and refuses an unbound assertion posting a verification record while still allowing it for an ordinary comment, which is pinned as caller compatibility. Both real write paths pass the same variable to the assertion and to the POST (`params.comment_html` for `plane_comment`, the sidebar's own html), so there is neither a false refusal nor a hash race, and `createComment` is no longer reachable except through `postVerifiedComment`, so no marker-bypass write path remains. Other classes checked. Duplicate comments: one POST pinned per error class. Unverified success: success requires the stored `actor` to equal the token's user id and nothing reports success on a read it did not perform. Marker bypass: both write paths are guarded identically and the sidebar is not a second unguarded door. Caller compatibility: the only cross-extension import of this module is `pi-otel-telemetry`, and only for `PROJECT_MAP`; `forgejo/api.ts` is a separate parallel implementation and is unaffected. The moved `VERIFICATION_HTML_HEADING_RE` is referenced only at call time from a hoisted function declaration, so there is no TDZ exposure, which tsc and the suite agree on. Four notes recorded for follow-up, none of which is a defect this change introduces. First, if `createComment` itself throws (timeout or 5xx on the POST), the raw transport error still propagates without MAY-EXIST wording, which is the same duplicate hazard now closed for the read-back leg; that leg predates this PR and sits outside the four notes, so it deserves its own ticket rather than being charged to this one. Second, an ordinary comment whose prose merely mentions the attribute name and carries no heading is now refused on write even with a declared identity; the refusal writes nothing and tells the caller how to recover, and the old code already special-cased that string, so this narrows behaviour rather than creating a new class. Third, "only an unintended duplicate of that exact id may be removed" is imprecise wording, since a real duplicate would carry a different id; the direction is conservative (do not delete), so it is safe. Fourth, the read-back does not assert the returned row's id equals the requested id, though the actor check would catch a wrong row in practice. No open blockers. The change is a faithful, tested narrowing of the comment guard with no bypass and no caller break.

ld-reviewer commented 2026-11-03 03:12:31 +00:00

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/k3.1-pro
head: ebef6f4c5e

tier: premium

Independent premium GENERAL review, fresh context, with no code edits, merge, agents, formal review, or other-PR actions. I re-fetched the live branch immediately before composing this verdict; it still resolves to the full head above, with main unchanged at f910731. The complete four-file diff is within HOMELAB-2554's declared scope.

I independently reran the targeted hardening suite (14/14), the full Plane suite (50/50 across three files), and strict TypeScript checking (exit 0) in the worker worktree. CI run 179 is green in all 14 jobs at this exact head. I also read the PR 77 review notes and the salvaged worker report and checked the implementation rather than relying on their conclusions.

N1 holds: read-back HTTP and thrown transport failures, plus a successful POST response without an id, report that the comment MAY EXIST and forbid blind retry. The id is named when available, recovery is reconciliation-first, and the tests pin exactly one POST for each failure shape.

N2 holds: heading, attribute-only, and absent markers are classified centrally. Attribute-only evidence is refused before identity or comment-endpoint access; historical attribute markers remain valid on the read-side closure gate. Heading evidence posts once and is read back, while marker loss after a write fails loudly without inviting a duplicate re-post.

N3 holds: the tool guidance and Plane documentation explain per-call expected_identity, the environment fallback, its normally-unset state, and that an identity refusal is the guard operating rather than an outage.

N4 holds at both plane_comment and sidebar call sites: each passes the same exact body to assertion and posting. The branded proof carries its UTF-8 sha256; changed or unbound evidence is refused before writing, while unbound ordinary comments remain compatible. I found no duplicate-comment path, unverified success, marker bypass, or caller-compatibility blocker. Success still requires the stored actor to match and any required marker to survive.

No open blockers. APPROVED at the exact live head.

## Verdict: APPROVED model: omniroute/k3.1-pro head: ebef6f4c5eefc1641a636afbaa6d4800b3d01ec0 tier: premium Independent premium GENERAL review, fresh context, with no code edits, merge, agents, formal review, or other-PR actions. I re-fetched the live branch immediately before composing this verdict; it still resolves to the full head above, with main unchanged at f910731. The complete four-file diff is within HOMELAB-2554's declared scope. I independently reran the targeted hardening suite (14/14), the full Plane suite (50/50 across three files), and strict TypeScript checking (exit 0) in the worker worktree. CI run 179 is green in all 14 jobs at this exact head. I also read the PR 77 review notes and the salvaged worker report and checked the implementation rather than relying on their conclusions. N1 holds: read-back HTTP and thrown transport failures, plus a successful POST response without an id, report that the comment MAY EXIST and forbid blind retry. The id is named when available, recovery is reconciliation-first, and the tests pin exactly one POST for each failure shape. N2 holds: heading, attribute-only, and absent markers are classified centrally. Attribute-only evidence is refused before identity or comment-endpoint access; historical attribute markers remain valid on the read-side closure gate. Heading evidence posts once and is read back, while marker loss after a write fails loudly without inviting a duplicate re-post. N3 holds: the tool guidance and Plane documentation explain per-call expected_identity, the environment fallback, its normally-unset state, and that an identity refusal is the guard operating rather than an outage. N4 holds at both plane_comment and sidebar call sites: each passes the same exact body to assertion and posting. The branded proof carries its UTF-8 sha256; changed or unbound evidence is refused before writing, while unbound ordinary comments remain compatible. I found no duplicate-comment path, unverified success, marker bypass, or caller-compatibility blocker. Success still requires the stored actor to match and any required marker to survive. No open blockers. APPROVED at the exact live head.

aaron force-pushed plane/HOMELAB-2554-comment-identity-hardening from ebef6f4c5e to 875ee6cd3e 2026-11-03 03:26:30 +00:00 Compare

aaron commented 2026-11-03 03:33:38 +00:00

Author

Owner

Copy link

Rebase onto post-#74 main

Comment only — this is not a review verdict. No approval, no changes-requested, no merge. Posted by a narrow rebase worker. PR stays open for a fresh formal-review panel; head is now stable and should stay frozen so exact-head verdicts can attach to it.

Head

• Previous head: ebef6f4c5eefc1641a636afbaa6d4800b3d01ec0
• New head: 875ee6cd3eff81bf8995e30e047742ae96b24e9f
• Parent of new head: fe7058b8aaba16c240d38f7421fac4e52b295200 — exactly the intended new base (the #74 merge), no extra commits

Rebase mechanics

• Fetched remote forgejo (this repo has no origin remote) and rebased onto forgejo/main = fe7058b8aaba16c240d38f7421fac4e52b295200.
• Clean, zero conflicts. Prior merge-base was f910731d9b22a3274c84c4d6c4cc410a41ec6f7c. No manual conflict resolution, no code edits in this task.
• No file overlap with #74's moved files (docs/forgejo.md, forgejo/landing.ts, forgejo/tests/landing-blockers.test.ts) — intersection with this branch's files was empty, as expected.
• Pushed with --force-with-lease pinned to the old head; remote now reports 875ee6cd3eff81bf8995e30e047742ae96b24e9f.

Diff set unchanged

git diff --name-only forgejo/main...HEAD returns exactly the four intended files and nothing else:

docs/plane.md
plane/api.ts
plane/index.ts
plane/tests/comment-hardening.test.ts

Green on the new head

• npx vitest run tests/comment-hardening.test.ts → 14/14 passed
• npx vitest run (full plane suite) → 50/50 passed, 3 files
• npx tsc --noEmit → clean, exit 0
• Forgejo Actions run #182 on 875ee6cd3e → pass, all 14 jobs green including the plane job

Not self-reviewed and not merged.

## Rebase onto post-#74 main **Comment only — this is not a review verdict.** No approval, no changes-requested, no merge. Posted by a narrow rebase worker. PR stays open for a fresh formal-review panel; head is now stable and should stay frozen so exact-head verdicts can attach to it. ### Head - Previous head: `ebef6f4c5eefc1641a636afbaa6d4800b3d01ec0` - **New head: `875ee6cd3eff81bf8995e30e047742ae96b24e9f`** - Parent of new head: `fe7058b8aaba16c240d38f7421fac4e52b295200` — exactly the intended new base (the #74 merge), no extra commits ### Rebase mechanics - Fetched remote `forgejo` (this repo has no `origin` remote) and rebased onto `forgejo/main` = `fe7058b8aaba16c240d38f7421fac4e52b295200`. - Clean, zero conflicts. Prior merge-base was `f910731d9b22a3274c84c4d6c4cc410a41ec6f7c`. No manual conflict resolution, no code edits in this task. - No file overlap with #74's moved files (`docs/forgejo.md`, `forgejo/landing.ts`, `forgejo/tests/landing-blockers.test.ts`) — intersection with this branch's files was empty, as expected. - Pushed with `--force-with-lease` pinned to the old head; remote now reports `875ee6cd3eff81bf8995e30e047742ae96b24e9f`. ### Diff set unchanged `git diff --name-only forgejo/main...HEAD` returns exactly the four intended files and nothing else: ``` docs/plane.md plane/api.ts plane/index.ts plane/tests/comment-hardening.test.ts ``` ### Green on the new head - `npx vitest run tests/comment-hardening.test.ts` → 14/14 passed - `npx vitest run` (full plane suite) → 50/50 passed, 3 files - `npx tsc --noEmit` → clean, exit 0 - Forgejo Actions run **#182** on `875ee6cd3e` → **pass**, all 14 jobs green including the `plane` job Not self-reviewed and not merged.

ld-reviewer approved these changes 2026-11-03 03:47:29 +00:00

Dismissed

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: 875ee6cd3e

Independent GENERAL review, round 2, on the rebased exact head (live API head == this sha; base fe7058b8). Read-only worktree /home/aaron/worktrees/HOMELAB-2554-plane; no edits, no merge, no other PR.

Rebase check

git diff fe7058b8 875ee6cd = 4 files, +530/−30 (plane/api.ts, plane/index.ts, plane/tests/comment-hardening.test.ts, docs/plane.md) — exactly the ticket's files_may_touch. All four blobs are byte-identical to the round-1 head (api.ts fa98fa454f…, index.ts 7fb2a4ffe5…, test 92724ab668…, docs/plane.md db759878c9…), and the new base only added the HOMELAB-2521 forgejo grammar, which this branch does not touch. The rebase therefore carried no semantic change and no conflict resolution.

Independent evidence (this worktree, this head)

• npx vitest run in plane/ → 3 files, 50/50 passed (hardening 14, identity 17, closure 19).
• npx --no-install tsc --noEmit in plane/ → exit 0.
• Actions run #182 on 875ee6cd3e → 14/14 jobs success.

N1 — read-back may have landed

postVerifiedComment wraps only the post-POST read-back; the error names the comment id and the work item, says MAY EXIST, and forbids a retry. A POST answered without an id takes the same shape (created?.id empty, including a 204 or non-JSON answer). requestAt has no retry loop, so the single-POST property holds in the code and not only in the tests, which pin exactly one POST …/comments/ for 5xx, thrown transport, and missing-id.

N2 — marker form

verificationMarkerForm() returns heading | attribute | null, heading first, and assertCommentIdentity refuses an attribute-only body before getCurrentUser and before any comment endpoint (test asserts zero /users/me/ and zero /comments calls). The marker-stripped-after-write error says the write HAPPENED and must not be re-posted. hasVerificationMarker is now a delegate to the same classifier, so the Done gate and the write guard cannot disagree, and the read side still accepts the attribute form — pre-existing attribute-marked evidence keeps closing its ticket, so no ticket becomes retroactively unwinnable.

N3 — expected_identity

docs/plane.md (env table, contract section, prompt guidelines) and the tool's own promptGuidelines state the fallback, the per-call override, unset-by-design, and that such a refusal is the guard working rather than an outage; the expected_identity parameter description no longer implies the env is always the source. The test reads the tool's declaration, so drift breaks it.

N4 — proof binds the body

commentBodyHash is computed in assertCommentIdentity from opts.body and carried on the branded proof; postVerifiedComment recomputes it, refuses a different body, and refuses an unbound assertion posting a verification record while still allowing unbound ordinary comments. Both production call sites (plane_comment with params.comment_html, sidebar addComment with html) pass the same variable to assertion and posting.

Attacks

• Duplicate comment: all four post-POST legs (no-id, read-back throw, actor mismatch, marker stripped) now forbid a retry; there is no loop anywhere on the write path.
• Unverified success: success requires a stored row whose actor equals the /users/me/ id, and getCurrentUser rejects an empty id, so an empty actor can never match.
• Marker bypass: IDENTITY_PROOF is a module-private symbol (never exported), so the brand cannot be forged; createComment has no production caller and both live write paths go assert → postVerifiedComment.
• Caller compatibility: the only cross-extension import of plane/api.js is pi-otel-telemetry for PROJECT_MAP; the new required bodyHash field is unreachable for external construction because the interface is branded; the two exported function signatures are unchanged.

No blocker: nothing in this diff produces a wrong write, a duplicate invitation, an unattributed success, or a marker bypass.

Follow-up notes (each fails closed with zero writes; none introduced a failing or bypassing path)

1. plane/index.ts:676 still tells agents the newest comment may "carry data-ld-verification" — true of the gate read, no longer true of the write, which now refuses that body. The first attempt is refused with a corrective message, so the cost is one round-trip. Worth aligning in a follow-up.
2. VERIFICATION_ATTR_RE is a substring test, so an ordinary progress/documentation comment that quotes the attribute name with no heading is refused before the write, and the suggested cure (add an <h2>Verification</h2> heading) turns an ordinary note into a record the closure gate counts as evidence. Narrower than before in message only, not in kind.
3. A POST that throws inside createComment still propagates the raw transport error with no MAY-EXIST wording. Pre-existing, outside the four notes, not introduced here — its own ticket.
4. The read-back does not compare the returned row id with the requested id; the endpoint routes by id and the actor check would catch a wrong row, so this is defence-in-depth.
5. In the read-back error, "only an unintended duplicate of that exact id may be removed" is imprecise (a duplicate would carry a different id); the sentence's direction — keep the comment, do not re-post — is the safe one.
6. The api.ts comment calls the attribute form's loss "(measured on v1.3.1, HOMELAB-2554)"; the ticket records it as unproven rather than measured-and-failed. The refusal is the correct fail-closed choice either way; only the citation outruns the evidence.

## Verdict: APPROVED model: omniroute/deepseek-flash head: 875ee6cd3eff81bf8995e30e047742ae96b24e9f Independent GENERAL review, round 2, on the rebased exact head (live API head == this sha; base `fe7058b8`). Read-only worktree `/home/aaron/worktrees/HOMELAB-2554-plane`; no edits, no merge, no other PR. ### Rebase check `git diff fe7058b8 875ee6cd` = 4 files, +530/−30 (`plane/api.ts`, `plane/index.ts`, `plane/tests/comment-hardening.test.ts`, `docs/plane.md`) — exactly the ticket's `files_may_touch`. All four blobs are byte-identical to the round-1 head (`api.ts` `fa98fa454f…`, `index.ts` `7fb2a4ffe5…`, test `92724ab668…`, `docs/plane.md` `db759878c9…`), and the new base only added the HOMELAB-2521 forgejo grammar, which this branch does not touch. The rebase therefore carried no semantic change and no conflict resolution. ### Independent evidence (this worktree, this head) - `npx vitest run` in `plane/` → 3 files, **50/50 passed** (hardening 14, identity 17, closure 19). - `npx --no-install tsc --noEmit` in `plane/` → **exit 0**. - Actions run **#182** on `875ee6cd3e` → **14/14 jobs success**. ### N1 — read-back may have landed `postVerifiedComment` wraps only the post-POST read-back; the error names the comment id and the work item, says **MAY EXIST**, and forbids a retry. A POST answered without an id takes the same shape (`created?.id` empty, including a `204` or non-JSON answer). `requestAt` has no retry loop, so the single-POST property holds in the code and not only in the tests, which pin exactly one `POST …/comments/` for 5xx, thrown transport, and missing-id. ### N2 — marker form `verificationMarkerForm()` returns `heading | attribute | null`, heading first, and `assertCommentIdentity` refuses an attribute-only body before `getCurrentUser` and before any comment endpoint (test asserts zero `/users/me/` and zero `/comments` calls). The marker-stripped-after-write error says the write HAPPENED and must not be re-posted. `hasVerificationMarker` is now a delegate to the same classifier, so the Done gate and the write guard cannot disagree, and the read side still accepts the attribute form — pre-existing attribute-marked evidence keeps closing its ticket, so no ticket becomes retroactively unwinnable. ### N3 — expected_identity `docs/plane.md` (env table, contract section, prompt guidelines) and the tool's own `promptGuidelines` state the fallback, the per-call override, unset-by-design, and that such a refusal is the guard working rather than an outage; the `expected_identity` parameter description no longer implies the env is always the source. The test reads the tool's declaration, so drift breaks it. ### N4 — proof binds the body `commentBodyHash` is computed in `assertCommentIdentity` from `opts.body` and carried on the branded proof; `postVerifiedComment` recomputes it, refuses a different body, and refuses an unbound assertion posting a verification record while still allowing unbound ordinary comments. Both production call sites (`plane_comment` with `params.comment_html`, sidebar `addComment` with `html`) pass the same variable to assertion and posting. ### Attacks - **Duplicate comment:** all four post-POST legs (no-id, read-back throw, actor mismatch, marker stripped) now forbid a retry; there is no loop anywhere on the write path. - **Unverified success:** success requires a stored row whose `actor` equals the `/users/me/` id, and `getCurrentUser` rejects an empty id, so an empty actor can never match. - **Marker bypass:** `IDENTITY_PROOF` is a module-private symbol (never exported), so the brand cannot be forged; `createComment` has no production caller and both live write paths go assert → `postVerifiedComment`. - **Caller compatibility:** the only cross-extension import of `plane/api.js` is `pi-otel-telemetry` for `PROJECT_MAP`; the new required `bodyHash` field is unreachable for external construction because the interface is branded; the two exported function signatures are unchanged. No blocker: nothing in this diff produces a wrong write, a duplicate invitation, an unattributed success, or a marker bypass. ### Follow-up notes (each fails closed with zero writes; none introduced a failing or bypassing path) 1. `plane/index.ts:676` still tells agents the newest comment may "carry data-ld-verification" — true of the gate read, no longer true of the write, which now refuses that body. The first attempt is refused with a corrective message, so the cost is one round-trip. Worth aligning in a follow-up. 2. `VERIFICATION_ATTR_RE` is a substring test, so an ordinary progress/documentation comment that quotes the attribute name with no heading is refused before the write, and the suggested cure (add an `<h2>Verification</h2>` heading) turns an ordinary note into a record the closure gate counts as evidence. Narrower than before in message only, not in kind. 3. A POST that throws inside `createComment` still propagates the raw transport error with no MAY-EXIST wording. Pre-existing, outside the four notes, not introduced here — its own ticket. 4. The read-back does not compare the returned row id with the requested id; the endpoint routes by id and the actor check would catch a wrong row, so this is defence-in-depth. 5. In the read-back error, "only an unintended duplicate of that exact id may be removed" is imprecise (a duplicate would carry a different id); the sentence's direction — keep the comment, do not re-post — is the safe one. 6. The `api.ts` comment calls the attribute form's loss "(measured on v1.3.1, HOMELAB-2554)"; the ticket records it as unproven rather than measured-and-failed. The refusal is the correct fail-closed choice either way; only the citation outruns the evidence.

ld-reviewer approved these changes 2026-11-03 04:02:40 +00:00

ld-reviewer left a comment

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/k3.1-pro
head: 875ee6cd3e
tier: premium

Independent PREMIUM review of the entire four-file diff against base fe7058b8aaba16c240d38f7421fac4e52b295200. No blockers found. The live PR head was re-fetched immediately before submission and matches the reviewed head.

Independent read-only evidence: targeted hardening tests 14/14; full Plane suite 50/50 across 3 files; tsc --noEmit exit 0; Actions run #182 green 14/14 on this exact head.

N1–N4 hold. A successful POST followed by read-back failure, missing id, actor mismatch, or marker loss cannot be mistaken for a clean pre-write refusal and does not invite a blind duplicate. Success requires a stored row attributed to the asserted /users/me/ identity, with verification-marker survival where applicable. Attribute-only writes fail closed while read-side compatibility remains. The branded assertion is sha256-bound to the exact body, unbound assertions cannot post evidence, and both production callers pass the same body through assertion and posting. Exported signatures and the sole cross-extension PROJECT_MAP consumer remain compatible.

Non-blocking follow-ups: POST transport failures remain a pre-existing ambiguous-write hazard; the read-back could additionally compare row ids; attribute-name substrings fail closed; and the separate plane_update_item guidance should be aligned with the stricter write contract.

## Verdict: APPROVED model: omniroute/k3.1-pro head: 875ee6cd3eff81bf8995e30e047742ae96b24e9f tier: premium Independent PREMIUM review of the entire four-file diff against base `fe7058b8aaba16c240d38f7421fac4e52b295200`. No blockers found. The live PR head was re-fetched immediately before submission and matches the reviewed head. Independent read-only evidence: targeted hardening tests 14/14; full Plane suite 50/50 across 3 files; `tsc --noEmit` exit 0; Actions run #182 green 14/14 on this exact head. N1–N4 hold. A successful POST followed by read-back failure, missing id, actor mismatch, or marker loss cannot be mistaken for a clean pre-write refusal and does not invite a blind duplicate. Success requires a stored row attributed to the asserted `/users/me/` identity, with verification-marker survival where applicable. Attribute-only writes fail closed while read-side compatibility remains. The branded assertion is sha256-bound to the exact body, unbound assertions cannot post evidence, and both production callers pass the same body through assertion and posting. Exported signatures and the sole cross-extension `PROJECT_MAP` consumer remain compatible. Non-blocking follow-ups: POST transport failures remain a pre-existing ambiguous-write hazard; the read-back could additionally compare row ids; attribute-name substrings fail closed; and the separate `plane_update_item` guidance should be aligned with the stricter write contract.

aaron referenced this pull request 2026-11-03 04:05:57 +00:00

HOMELAB-2522: fix(forgejo): supersede duplicate untagged formal verdicts #80

aaron merged commit 4be7c075c8 into main 2026-11-03 04:11:46 +00:00

aaron referenced this pull request from a commit 2026-11-03 04:11:49 +00:00

Merge pull request 'HOMELAB-2554: fix(plane): harden comment verification evidence' (#79) from plane/HOMELAB-2554-comment-identity-hardening into main

aaron referenced this pull request 2026-11-03 04:43:14 +00:00

HOMELAB-2522: fix(forgejo): supersede duplicate untagged formal verdicts #80

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

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

ld-reviewer referenced this pull request 2026-11-06 11:12:22 +00:00

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

Sign in to join this conversation.

Reviewers

No reviewers

ld-reviewer

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

No description provided.