HOMELAB-2529: fix(plane): assert comment identity before posting #77

Merged

aaron merged 1 commit from plane/HOMELAB-2529-comment-identity into main 2026-11-03 01:58:23 +00:00

Conversation 3 Commits 1 Files changed 5 +727 -8

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

Owner

Copy link

What

plane_comment and the sidebar's add-comment posted with whatever token was in PLANE_API_KEY and never asked the API whose token that was. Authorship is not decoration on this path: the pipeline writes verification records (HOMELAB-1679) and stage evidence to tickets, so a record attributed to an unknown user is a record nobody can rely on. Same structural gap HOMELAB-2527 fixed for Forgejo, on the Plane side. No live misattribution was ever demonstrated — characterization and the guard are the whole change.

Red-first (pre-fix, fake API, no network)

All three refusal cases POSTed anyway against the unmodified tool:

❯ tests/_red-probe.test.ts (3 tests | 3 failed)
  × declared identity mismatch must refuse with ZERO writes
    → promise resolved "{ content: [ { …(2) } ], …(1) }" instead of rejecting
  × unreadable identity must refuse with ZERO writes
  × a verification record with no declared identity must refuse with ZERO writes

The probe was deleted after capture; plane/tests/comment-identity.test.ts encodes the same cases plus the matching-identity post with authorship read-back.

Supported Plane API facts (v1.3.1 = deployed pin, read-only probes)

• Current-user read is GET /api/v1/users/me/ — API-root scoped. The workspace-scoped form /api/v1/workspaces/<slug>/users/me/ answers 404 on v1.3.1 (measured live), so this call cannot use apiBase(); request() gained an explicit-base sibling.
• Response is UserLiteSerializer: id, first_name, last_name, email, avatar, avatar_url, display_name. Plane has no login field, so an expectation may name the id, email, display name, or First Last.
• Author is server-assigned: IssueCommentCreateSerializer marks actor read-only, the view saves actor=request.user. comment_html is re-serialized through lxml on write, so a byte-exact body comparison would be unsound — actor is what a read-back can trust.

Guard

assertCommentIdentity (plane/api.ts) reads the current user and refuses before any POST, never throwing:

• identity unreadable (users/me fails or answers without an id);
• identity contradicts a declared expectation (expected_identity arg, else PLANE_EXPECTED_IDENTITY);
• a verification record (HOMELAB-1679 marker) with no declared identity at all.

postVerifiedComment requires a branded proof minted by the assertion (a hand-rolled { ok: true } cannot reach the POST), then reads the comment back by id and requires the stored actor to equal the token's user id; a mismatch is reported loudly with the comment id, because at that point the write already happened.

Ordinary comments still post without a declaration — the identity is always read and reported, never guessed — because Plane's Done gate consumes the verification marker rather than the author, unlike the Forgejo landing lock.

Both comment write paths (tool + sidebar) go through the pair. No historical comment is edited or deleted; no protected database touched; no live comment write was performed during development.

Verification

• plane suite 36/36 (17 new in plane/tests/comment-identity.test.ts), tsc --noEmit clean.
• Collision check before editing: open PRs #74 (forgejo docs/landing), #76 (forgejo merge), #68 (pi-swarm-suite) touch no plane/** or docs/plane.md.
• Files: plane/api.ts, plane/index.ts, plane/tests/fake.ts, plane/tests/comment-identity.test.ts, docs/plane.md.

## What `plane_comment` and the sidebar's add-comment posted with whatever token was in `PLANE_API_KEY` and never asked the API whose token that was. Authorship is not decoration on this path: the pipeline writes verification records (HOMELAB-1679) and stage evidence to tickets, so a record attributed to an unknown user is a record nobody can rely on. Same structural gap HOMELAB-2527 fixed for Forgejo, on the Plane side. **No live misattribution was ever demonstrated** — characterization and the guard are the whole change. ## Red-first (pre-fix, fake API, no network) All three refusal cases POSTed anyway against the unmodified tool: ``` ❯ tests/_red-probe.test.ts (3 tests | 3 failed) × declared identity mismatch must refuse with ZERO writes → promise resolved "{ content: [ { …(2) } ], …(1) }" instead of rejecting × unreadable identity must refuse with ZERO writes × a verification record with no declared identity must refuse with ZERO writes ``` The probe was deleted after capture; `plane/tests/comment-identity.test.ts` encodes the same cases plus the matching-identity post with authorship read-back. ## Supported Plane API facts (v1.3.1 = deployed pin, read-only probes) - Current-user read is **`GET /api/v1/users/me/` — API-root scoped**. The workspace-scoped form `/api/v1/workspaces/<slug>/users/me/` answers `404` on v1.3.1 (measured live), so this call cannot use `apiBase()`; `request()` gained an explicit-base sibling. - Response is `UserLiteSerializer`: `id, first_name, last_name, email, avatar, avatar_url, display_name`. Plane has **no login field**, so an expectation may name the id, email, display name, or `First Last`. - Author is server-assigned: `IssueCommentCreateSerializer` marks `actor` read-only, the view saves `actor=request.user`. `comment_html` is re-serialized through lxml on write, so a byte-exact body comparison would be unsound — `actor` is what a read-back can trust. ## Guard `assertCommentIdentity` (plane/api.ts) reads the current user and refuses **before any POST**, never throwing: - identity unreadable (`users/me` fails or answers without an id); - identity contradicts a declared expectation (`expected_identity` arg, else `PLANE_EXPECTED_IDENTITY`); - a **verification record** (HOMELAB-1679 marker) with no declared identity at all. `postVerifiedComment` requires a branded proof minted by the assertion (a hand-rolled `{ ok: true }` cannot reach the POST), then reads the comment back by id and requires the stored `actor` to equal the token's user id; a mismatch is reported loudly with the comment id, because at that point the write already happened. Ordinary comments still post without a declaration — the identity is always **read and reported, never guessed** — because Plane's Done gate consumes the verification marker rather than the author, unlike the Forgejo landing lock. Both comment write paths (tool + sidebar) go through the pair. No historical comment is edited or deleted; no protected database touched; no live comment write was performed during development. ## Verification - `plane` suite **36/36** (17 new in `plane/tests/comment-identity.test.ts`), `tsc --noEmit` clean. - Collision check before editing: open PRs #74 (forgejo docs/landing), #76 (forgejo merge), #68 (pi-swarm-suite) touch no `plane/**` or `docs/plane.md`. - Files: `plane/api.ts`, `plane/index.ts`, `plane/tests/fake.ts`, `plane/tests/comment-identity.test.ts`, `docs/plane.md`.

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

HOMELAB-2529: fix(plane): assert comment identity before posting ... 7c8d7a633b

`plane_comment` (and the sidebar's add-comment) posted with whatever token was
in PLANE_API_KEY and never asked the API whose token that was. Authorship is
not decoration here: the pipeline writes verification records (HOMELAB-1679)
and stage evidence to tickets, so a record attributed to an unknown user is a
record nobody can rely on. This is the same structural gap HOMELAB-2527 fixed
for Forgejo, on the Plane side; no live misattribution was ever demonstrated.

Measured red-first, against the unmodified tool (fake API, no network), all
three cases POSTed anyway:

  x declared identity mismatch must refuse with ZERO writes
  x unreadable identity must refuse with ZERO writes
  x a verification record with no declared identity must refuse

Supported Plane API facts this rests on (v1.3.1, the deployed pin):

- The current-user read is `GET /api/v1/users/me/`, API-ROOT scoped. The
  workspace-scoped form `/api/v1/workspaces/<slug>/users/me/` answers 404 on
  v1.3.1, so `request()` gained an explicit-base sibling and the call does not
  use `apiBase()`. Response is UserLiteSerializer —
  `id, first_name, last_name, email, avatar, avatar_url, display_name`; Plane
  has no login field, hence the expectation may name id/email/display name.
- The author is server-assigned: IssueCommentCreateSerializer marks `actor`
  read-only and the view saves `actor=request.user`, so `actor` is what a
  read-back can trust. `comment_html` is re-serialized through lxml on write,
  so a byte-exact body comparison would be unsound and is not used.

The guard (plane/api.ts): `assertCommentIdentity` reads the current user and
refuses BEFORE any POST — never throws — when the identity is unreadable, when
it contradicts a declared expectation (`expected_identity` argument, else
PLANE_EXPECTED_IDENTITY), or when a verification record carries no declared
identity at all. `postVerifiedComment` requires a branded proof minted by the
assertion (a hand-rolled `{ ok: true }` cannot reach the POST), then reads the
comment back by id and requires the stored `actor` to be the token's user;
a mismatch is reported loudly with the comment id, because the write already
happened. Ordinary comments keep posting without a declaration — the identity
is always read and reported, never guessed — since Plane's Done gate consumes
the verification marker rather than the author, unlike the Forgejo lock.

Both comment write paths (tool and sidebar) go through the pair. No historical
comment is edited or deleted; no protected database is touched. Tests:
36/36 (17 new), tsc clean.

aaron commented 2026-11-03 00:44:02 +00:00

Author

Owner

Copy link

CI green on head 7c8d7a633b (run #174): all 14 jobs pass, including plane (1m28s) and pr-title (8s).

Local verification on the same tree:

• plane suite 36/36 (19 closure + 17 new identity tests)
• tsc --noEmit clean (strict)
• red-first capture against the pre-fix tool is quoted in the description and on HOMELAB-2529

Ready for independent review. No self-review, verdicts, or merge from the author.

CI green on head `7c8d7a633b` (run #174): all 14 jobs pass, including `plane` (1m28s) and `pr-title` (8s). Local verification on the same tree: - `plane` suite 36/36 (19 closure + 17 new identity tests) - `tsc --noEmit` clean (`strict`) - red-first capture against the pre-fix tool is quoted in the description and on HOMELAB-2529 Ready for independent review. No self-review, verdicts, or merge from the author.

ld-reviewer commented 2026-11-03 00:57:00 +00:00

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/deepseek-flash
head: 7c8d7a633b

GENERAL review — HOMELAB-2529 / pi-extensions #77

Verdict: APPROVED. Model omniroute/deepseek-flash, general reviewer, fresh context.
Reviewed exact head 7c8d7a633b1fd07a720dd4f4c4e56cf48f7b2a12 (re-fetched live; unchanged).
CI #174 at that sha: 14/14 jobs pass, including plane. No blockers.

Contract

HOMELAB-2529 asked for a read-only characterization first, then — only if confirmed — an
expected-identity assertion before comment writes, fake-API red-first, zero writes on
mismatch/unreadable identity, supported Plane APIs only. The diff is plane/api.ts,
plane/index.ts, plane/tests/fake.ts, plane/tests/comment-identity.test.ts and
docs/plane.md (+727/−8), inside the ticket's declared file set.

Independently reproduced

• Suite re-run in the worker worktree at this head: plane 36/36 (17 new, 19 closure);
  tsc --noEmit clean.
• Live read-only Plane GETs (no writes, key never in argv): /api/v1/users/me/ → 200;
  the workspace-scoped twin → 404, so apiBase() genuinely cannot carry this call and the
  new apiRoot() is the necessary base rather than a stylistic choice.
• GET .../comments/<id>/ → 200 returning actor and comment_html, so the read-back
  leg rests on a real endpoint, not an assumption. Every listed row has actor == created_by.
• The defect premise is re-derived from source, not accepted on report: origin/main's
  plane/index.ts called createComment directly on both write paths with no identity read
  anywhere on the path.

Attack surface, item by item

• API-root handling — requestAt() takes the base explicitly and request() delegates
  with apiBase(), so every pre-existing call site composes a byte-identical URL (same
  trailing-slash rule, same params handling). Minimal and correct.
• expected_identity / env semantics — explicit argument beats PLANE_EXPECTED_IDENTITY,
  blank collapses to "no expectation" so an empty declaration cannot become either
  match-nobody or match-anybody; matching covers id, email, display name and First Last
  case-insensitively, empty never matches. Plane has no login field, so that is the right key
  set.
• Verification marker declaration — an undeclared verification body is refused, and the
  predicate is the very hasVerificationMarker the HOMELAB-1679 Done gate consumes, so guard
  and gate cannot disagree about what evidence is. This is a faithful mirror of the merged
  Forgejo leg from HOMELAB-2527, not a novel rule invented on this PR.
• Zero writes on mismatch/unreadable — the assertion precedes createComment on both
  paths; the tests pin zero comment POSTs, and for unreadable identity zero calls to any
  comment path at all. Refusals are returned rather than thrown, so the reason reaches the
  caller intact.
• Branded proof — module-private symbol; postVerifiedComment demands both ok === true
  and the symbol, and a hand-rolled object is rejected with zero fetch calls.
• Actor read-back — stored actor must equal the token's user id; a mismatch throws
  naming the comment id and stating the write already happened, which is the honest failure
  mode for a post-write discovery.
• HTML sanitization — no byte-exact body comparison anywhere, only marker presence. That
  is the correct call given Plane re-serializes comment_html on write.
• Sidebar add-comment — routed through the same assertion and read-back, refusal surfacing
  as Comment failed: …. No unguarded second write path remains; createComment is no longer
  imported by index.ts.
• Caller compatibility — request and createComment signatures preserved, tool
  details still carries the stored comment with ldAttribution additive, prompt guideline
  additive. No historical comment edited or deleted, no protected data touched.

Non-blocking observations

1. A failure of the read-back GET itself propagates the raw Plane API GET … error instead of
   the "the write may have landed — reconcile" wording the other two post-write legs use, so
   an agent could retry and duplicate. The endpoint answers 200 today, so the exposure is
   narrow, but wrapping it would make the trio consistent.
2. Attribute-form marker survival is unproven: a verification body carrying only
   data-ld-verification (no heading) could pass the pre-POST check and then throw after a
   successful write, and re-posting would fail identically. The canonical
   <h2>Verification</h2> form is demonstrably preserved (observed in stored rows); the
   attribute form is not observed anywhere sampled. The docs do not overclaim it and the
   guideline leads with the heading, so this is a sharp edge worth remembering rather than a
   defect on this PR.
3. Fleet-visible consequence, for the owner rather than this PR: verification comments now
   require a declared identity and PLANE_EXPECTED_IDENTITY is not set in this environment,
   so agents must pass expected_identity per call. That is the ticket's intent and the
   refusal explains itself, but .pi/skills/homelab-ticket does not yet mention it — the first
   agent to hit the refusal should not read it as a Plane outage.
4. The branded proof does not bind the body it was minted for, so a caller that asserts
   without body and then posts verification HTML would bypass the undeclared-evidence leg.
   Both in-repo write paths pass body, so this is hardening rather than a hole in the
   shipped path.

Nothing above gates landing. The change is the minimal correct guard for the confirmed
structural gap, its evidence is reproducible, and the refusals are all on the safe side of the
write.

## Verdict: APPROVED model: omniroute/deepseek-flash head: 7c8d7a633b1fd07a720dd4f4c4e56cf48f7b2a12 ## GENERAL review — HOMELAB-2529 / pi-extensions #77 Verdict: **APPROVED**. Model `omniroute/deepseek-flash`, general reviewer, fresh context. Reviewed exact head `7c8d7a633b1fd07a720dd4f4c4e56cf48f7b2a12` (re-fetched live; unchanged). CI #174 at that sha: 14/14 jobs pass, including `plane`. No blockers. ### Contract HOMELAB-2529 asked for a read-only characterization first, then — only if confirmed — an expected-identity assertion before comment writes, fake-API red-first, zero writes on mismatch/unreadable identity, supported Plane APIs only. The diff is `plane/api.ts`, `plane/index.ts`, `plane/tests/fake.ts`, `plane/tests/comment-identity.test.ts` and `docs/plane.md` (+727/−8), inside the ticket's declared file set. ### Independently reproduced - Suite re-run in the worker worktree at this head: `plane` 36/36 (17 new, 19 closure); `tsc --noEmit` clean. - Live read-only Plane GETs (no writes, key never in argv): `/api/v1/users/me/` → 200; the workspace-scoped twin → 404, so `apiBase()` genuinely cannot carry this call and the new `apiRoot()` is the necessary base rather than a stylistic choice. - `GET .../comments/<id>/` → 200 returning `actor` **and** `comment_html`, so the read-back leg rests on a real endpoint, not an assumption. Every listed row has `actor == created_by`. - The defect premise is re-derived from source, not accepted on report: `origin/main`'s `plane/index.ts` called `createComment` directly on both write paths with no identity read anywhere on the path. ### Attack surface, item by item - **API-root handling** — `requestAt()` takes the base explicitly and `request()` delegates with `apiBase()`, so every pre-existing call site composes a byte-identical URL (same trailing-slash rule, same params handling). Minimal and correct. - **expected_identity / env semantics** — explicit argument beats `PLANE_EXPECTED_IDENTITY`, blank collapses to "no expectation" so an empty declaration cannot become either match-nobody or match-anybody; matching covers id, email, display name and `First Last` case-insensitively, empty never matches. Plane has no login field, so that is the right key set. - **Verification marker declaration** — an undeclared verification body is refused, and the predicate is the very `hasVerificationMarker` the HOMELAB-1679 Done gate consumes, so guard and gate cannot disagree about what evidence is. This is a faithful mirror of the merged Forgejo leg from HOMELAB-2527, not a novel rule invented on this PR. - **Zero writes on mismatch/unreadable** — the assertion precedes `createComment` on both paths; the tests pin zero comment POSTs, and for unreadable identity zero calls to any comment path at all. Refusals are returned rather than thrown, so the reason reaches the caller intact. - **Branded proof** — module-private symbol; `postVerifiedComment` demands both `ok === true` and the symbol, and a hand-rolled object is rejected with zero fetch calls. - **Actor read-back** — stored `actor` must equal the token's user id; a mismatch throws naming the comment id and stating the write already happened, which is the honest failure mode for a post-write discovery. - **HTML sanitization** — no byte-exact body comparison anywhere, only marker presence. That is the correct call given Plane re-serializes `comment_html` on write. - **Sidebar add-comment** — routed through the same assertion and read-back, refusal surfacing as `Comment failed: …`. No unguarded second write path remains; `createComment` is no longer imported by `index.ts`. - **Caller compatibility** — `request` and `createComment` signatures preserved, tool `details` still carries the stored comment with `ldAttribution` additive, prompt guideline additive. No historical comment edited or deleted, no protected data touched. ### Non-blocking observations 1. A failure of the read-back GET itself propagates the raw `Plane API GET …` error instead of the "the write may have landed — reconcile" wording the other two post-write legs use, so an agent could retry and duplicate. The endpoint answers 200 today, so the exposure is narrow, but wrapping it would make the trio consistent. 2. Attribute-form marker survival is unproven: a verification body carrying only `data-ld-verification` (no heading) could pass the pre-POST check and then throw after a successful write, and re-posting would fail identically. The canonical `<h2>Verification</h2>` form is demonstrably preserved (observed in stored rows); the attribute form is not observed anywhere sampled. The docs do not overclaim it and the guideline leads with the heading, so this is a sharp edge worth remembering rather than a defect on this PR. 3. Fleet-visible consequence, for the owner rather than this PR: verification comments now require a declared identity and `PLANE_EXPECTED_IDENTITY` is not set in this environment, so agents must pass `expected_identity` per call. That is the ticket's intent and the refusal explains itself, but `.pi/skills/homelab-ticket` does not yet mention it — the first agent to hit the refusal should not read it as a Plane outage. 4. The branded proof does not bind the body it was minted for, so a caller that asserts without `body` and then posts verification HTML would bypass the undeclared-evidence leg. Both in-repo write paths pass `body`, so this is hardening rather than a hole in the shipped path. Nothing above gates landing. The change is the minimal correct guard for the confirmed structural gap, its evidence is reproducible, and the refusals are all on the safe side of the write.

ld-reviewer commented 2026-11-03 01:45:14 +00:00

Collaborator

Copy link

Verdict: APPROVED

model: omniroute/k3.1-pro
head: 7c8d7a633b

tier: premium

I independently reviewed the complete Plane-scoped change at the exact live head and approve it. No open blockers.

The API-root current-user read is correctly separated from workspace requests, and existing request and comment-call signatures remain compatible. Explicit expectations override the environment, blank declarations do not match, and verification-marked comments require a declared identity. Both the tool and sidebar writes assert identity before posting. Mismatch, unreadable identity, and undeclared verification therefore perform zero comment writes. The private proof brand prevents a hand-rolled assertion from reaching the posting helper.

The post-write check reads the stored comment and requires its actor to equal the current-user id. It does not compare sanitized HTML byte-for-byte; it checks only that the verification marker survives, using the same marker predicate as the closure gate. Tool results retain the stored comment and add attribution details.

I reran the Plane suite independently: 36 of 36 tests passed, including all 17 new identity tests, and strict TypeScript checking was clean. CI run 174 is green in all 14 jobs at this head.

Non-blocking: if the read-back request itself fails after a successful POST, the raw GET error does not explicitly warn that the comment may already have landed. Missing ids, actor mismatches, and marker loss do provide that warning. Clearer reconciliation wording for transport failure would be a useful follow-up, but this does not bypass the required pre-write guard.

## Verdict: APPROVED model: omniroute/k3.1-pro head: 7c8d7a633b1fd07a720dd4f4c4e56cf48f7b2a12 tier: premium I independently reviewed the complete Plane-scoped change at the exact live head and approve it. No open blockers. The API-root current-user read is correctly separated from workspace requests, and existing request and comment-call signatures remain compatible. Explicit expectations override the environment, blank declarations do not match, and verification-marked comments require a declared identity. Both the tool and sidebar writes assert identity before posting. Mismatch, unreadable identity, and undeclared verification therefore perform zero comment writes. The private proof brand prevents a hand-rolled assertion from reaching the posting helper. The post-write check reads the stored comment and requires its actor to equal the current-user id. It does not compare sanitized HTML byte-for-byte; it checks only that the verification marker survives, using the same marker predicate as the closure gate. Tool results retain the stored comment and add attribution details. I reran the Plane suite independently: 36 of 36 tests passed, including all 17 new identity tests, and strict TypeScript checking was clean. CI run 174 is green in all 14 jobs at this head. Non-blocking: if the read-back request itself fails after a successful POST, the raw GET error does not explicitly warn that the comment may already have landed. Missing ids, actor mismatches, and marker loss do provide that warning. Clearer reconciliation wording for transport failure would be a useful follow-up, but this does not bypass the required pre-write guard.

aaron merged commit f910731d9b into main 2026-11-03 01:58:23 +00:00

aaron referenced this pull request from a commit 2026-11-03 01:58:26 +00:00

Merge pull request 'HOMELAB-2529: fix(plane): assert comment identity before posting' (#77) from plane/HOMELAB-2529-comment-identity into main

aaron referenced this pull request from a commit 2026-11-03 02:39:05 +00:00

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

aaron referenced this pull request 2026-11-03 02:39:12 +00:00

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

aaron referenced this pull request 2026-11-03 02:49:57 +00:00

HOMELAB-2521: fix(forgejo): parse blocker heading ids and resolutions #74

ld-reviewer referenced this pull request 2026-11-03 02:56:49 +00:00

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

aaron referenced this pull request from a commit 2026-11-03 03:26:30 +00:00

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

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

No description provided.