HOMELAB-2527: fix(forgejo): assert comment identity before posting #75

Merged
aaron merged 1 commit from plane/HOMELAB-2527-comment-identity into main 2026-11-02 23:31:45 +00:00
Owner

Closes HOMELAB-2527 (split from HOMELAB-2510 / GITS-312). Authorship of a Forgejo comment is landing-lock evidence (HOMELAB-2346); a write must be attributed to the identity it claims BEFORE it happens.

The two measured defects

  1. forgejo_comment_on_pr (forgejo/index.ts → api.ts commentOnPR) posted with whatever token was in FORGEJO_API_KEY. It never issued GET /user, and a caller had no way to state which identity the write must come from.
  2. verdict-cli.ts --post read GET /user only as a default author (args.author ?? await loginOf(...)), so an explicit --author ld-reviewer short-circuited the only identity read: the body was composed and lock-evaluated as ld-reviewer while the token could be aaron. The post-write read-back compared the stored body, never the stored author. The 403 regression test makes this concrete — with --author supplied, no GET /user was issued at all and the POST still happened.

The guard

forgejo/api.ts:

  • assertWriteIdentity(cfg, { expected, body }) reads GET /user with the same token and compares it against an explicitly configured expected identity — the tool's expected_identity parameter, FORGEJO_EXPECTED_IDENTITY in the env, or the helper's --author. No universal hardcoded reviewer: aaron, workers and ld-reviewer all legitimately post comments, so the expectation is configuration, not a constant.
  • Refused before any write: a mismatch; an unreadable identity (GET /user fails, or answers with no login); and a landing-lock verdict body with no declared identity at all (a verdict whose authorship is undeclared is a record nobody can attribute).
  • postVerifiedComment() requires a branded proof minted by the assertion, so a hand-rolled { ok: true } cannot reach the POST. It then reads the comment back and verifies the body and the author (sameLogin compares case-insensitively, because Forgejo echoes stored case).
  • Refusals name expected/actual logins only. Tokens are never printed.

verdict-cli.ts: GET /user on every run; a mismatched --author refuses (dry run too); --post requires a readable identity; after the POST the stored author is verified against the token's identity and a mismatch fails loudly with "do not repost under another identity without a ruling".

No historical comment is edited or deleted anywhere in this change — the guard is pre-POST only, and no code path here issues PATCH/DELETE (asserted in the tests: every refusal proves zero mutating requests, not just zero comment POSTs).

Red first

$ cd forgejo && npx vitest run tests/comment-identity.test.ts
 Test Files  1 failed (1)
      Tests  11 failed | 3 passed (14)
  RED: declared ld-reviewer identity with an aaron token -> POST happened (+1)
  RED: FORGEJO_EXPECTED_IDENTITY path                     -> POST happened (+1)
  RED: unreadable identity (GET /user 403)                -> POST happened (+1)
  RED: GET /user with no login                            -> POST happened (+1)
  RED: --author ld-reviewer with an aaron token           -> POST happened (+1)
  RED: --post with unreadable identity + --author         -> POST happened (+1)

Verdict-body rule added after the first guard, red first as well (1 failed), then implemented.

Green

$ cd forgejo && npx vitest run tests/comment-identity.test.ts
 Test Files  1 passed (1)   Tests 17 passed (17)

$ cd forgejo && npx vitest run
 Test Files  12 passed (12)  Tests 265 passed (265)   # 248 pre-existing + 17 new

$ cd forgejo && npx tsc --noEmit   # clean

Baseline on the same commit with the new test file present, before implementation: 11 failed | 251 passed (262) — every pre-existing test was already green, so the red is this guard only.

The suite covers: mismatch/unreadable/verdict-without-declaration → zero writes and a refusal naming both logins; matching identity → exactly one POST, GET /user strictly before it, and read-back authorship; ordinary comment under a sanctioned non-reviewer identity → still posts and records its real author.

Scope

  • Touched: forgejo/api.ts, forgejo/index.ts, forgejo/verdict-cli.ts, forgejo/tests/comment-identity.test.ts.
  • Not touched: forgejo/landing.ts, any landing test, CI required-context code, merge behaviour (no landing-lock logic is changed or re-evaluated here).
  • docs/forgejo.md is omitted: open PR #74 (HOMELAB-2521) holds that file. The new env var and parameter are documented in the tool description and the test-file header; a doc line can follow after #74 lands.
  • plane/ is untouched: the same class of defect is plausible there (plane_comment posts under whatever PLANE_API_KEY is set), but it is not demonstrated on this ticket, so it does not belong in this PR. Flagging to the orchestrator for an owner ticket rather than widening scope.
  • No live comment/review write was performed; every test runs against a fake API route table with no network.
Closes HOMELAB-2527 (split from HOMELAB-2510 / GITS-312). Authorship of a Forgejo comment is landing-lock evidence (HOMELAB-2346); a write must be attributed to the identity it claims BEFORE it happens. ## The two measured defects 1. **`forgejo_comment_on_pr`** (forgejo/index.ts → api.ts `commentOnPR`) posted with whatever token was in `FORGEJO_API_KEY`. It never issued `GET /user`, and a caller had no way to state which identity the write must come from. 2. **`verdict-cli.ts --post`** read `GET /user` only as a *default* author (`args.author ?? await loginOf(...)`), so an explicit `--author ld-reviewer` **short-circuited the only identity read**: the body was composed and lock-evaluated as `ld-reviewer` while the token could be `aaron`. The post-write read-back compared the stored **body**, never the stored **author**. The 403 regression test makes this concrete — with `--author` supplied, no `GET /user` was issued at all and the POST still happened. ## The guard `forgejo/api.ts`: - `assertWriteIdentity(cfg, { expected, body })` reads `GET /user` with the same token and compares it against an **explicitly configured** expected identity — the tool's `expected_identity` parameter, `FORGEJO_EXPECTED_IDENTITY` in the env, or the helper's `--author`. **No universal hardcoded reviewer**: `aaron`, workers and `ld-reviewer` all legitimately post comments, so the expectation is configuration, not a constant. - Refused **before any write**: a mismatch; an unreadable identity (`GET /user` fails, or answers with no login); and a landing-lock **verdict** body with no declared identity at all (a verdict whose authorship is undeclared is a record nobody can attribute). - `postVerifiedComment()` requires a **branded proof** minted by the assertion, so a hand-rolled `{ ok: true }` cannot reach the POST. It then reads the comment back and verifies the body **and** the author (`sameLogin` compares case-insensitively, because Forgejo echoes stored case). - Refusals name expected/actual logins only. Tokens are never printed. `verdict-cli.ts`: `GET /user` on every run; a mismatched `--author` refuses (dry run too); `--post` requires a readable identity; after the POST the stored author is verified against the token's identity and a mismatch fails loudly with "do not repost under another identity without a ruling". **No historical comment is edited or deleted anywhere in this change** — the guard is pre-POST only, and no code path here issues PATCH/DELETE (asserted in the tests: every refusal proves **zero** mutating requests, not just zero comment POSTs). ## Red first ``` $ cd forgejo && npx vitest run tests/comment-identity.test.ts Test Files 1 failed (1) Tests 11 failed | 3 passed (14) RED: declared ld-reviewer identity with an aaron token -> POST happened (+1) RED: FORGEJO_EXPECTED_IDENTITY path -> POST happened (+1) RED: unreadable identity (GET /user 403) -> POST happened (+1) RED: GET /user with no login -> POST happened (+1) RED: --author ld-reviewer with an aaron token -> POST happened (+1) RED: --post with unreadable identity + --author -> POST happened (+1) ``` Verdict-body rule added after the first guard, red first as well (`1 failed`), then implemented. ## Green ``` $ cd forgejo && npx vitest run tests/comment-identity.test.ts Test Files 1 passed (1) Tests 17 passed (17) $ cd forgejo && npx vitest run Test Files 12 passed (12) Tests 265 passed (265) # 248 pre-existing + 17 new $ cd forgejo && npx tsc --noEmit # clean ``` Baseline on the same commit with the new test file present, before implementation: `11 failed | 251 passed (262)` — every pre-existing test was already green, so the red is this guard only. The suite covers: mismatch/unreadable/verdict-without-declaration → zero writes and a refusal naming both logins; matching identity → exactly one POST, `GET /user` strictly **before** it, and read-back authorship; ordinary comment under a sanctioned non-reviewer identity → still posts and records its real author. ## Scope - Touched: `forgejo/api.ts`, `forgejo/index.ts`, `forgejo/verdict-cli.ts`, `forgejo/tests/comment-identity.test.ts`. - **Not touched**: `forgejo/landing.ts`, any landing test, CI required-context code, merge behaviour (no landing-lock logic is changed or re-evaluated here). - `docs/forgejo.md` is **omitted**: open PR #74 (HOMELAB-2521) holds that file. The new env var and parameter are documented in the tool description and the test-file header; a doc line can follow after #74 lands. - `plane/` is untouched: the same class of defect is plausible there (`plane_comment` posts under whatever `PLANE_API_KEY` is set), but it is not demonstrated on this ticket, so it does not belong in this PR. Flagging to the orchestrator for an owner ticket rather than widening scope. - No live comment/review write was performed; every test runs against a fake API route table with no network.
HOMELAB-2527: fix(forgejo): assert comment identity before posting
All checks were successful
CI / pr-title (pull_request) Successful in 1s
CI / forgejo-ci (pull_request) Successful in 47s
CI / pi-otel-telemetry (pull_request) Successful in 57s
CI / plane (pull_request) Successful in 1m12s
CI / forgejo (pull_request) Successful in 1m15s
CI / skill-args (pull_request) Successful in 36s
CI / mcp-bridge (pull_request) Successful in 29s
CI / nats-intercom (pull_request) Successful in 30s
CI / ask-user-question (pull_request) Successful in 41s
CI / redact (pull_request) Successful in 30s
CI / refocus (pull_request) Successful in 53s
CI / todo (pull_request) Successful in 54s
CI / pi-friction (pull_request) Successful in 43s
CI / pi-swarm-suite (pull_request) Successful in 3m22s
c682d40359
A Forgejo comment's AUTHOR is landing-lock evidence (HOMELAB-2346), but both
comment write paths posted with whatever token was in FORGEJO_API_KEY and
neither asked the API whose token that was. The harness once posted a verdict
body as `aaron` and honestly reposted it as `ld-reviewer` (GITS-312).

Two concrete defects, both measured red-first through fake-API tests:

- `forgejo_comment_on_pr` never issued GET /user and had no way for a caller to
  state the identity the write must be attributed to.
- `verdict-cli.ts --post` read GET /user only as a DEFAULT author
  (`args.author ?? await loginOf(...)`), so an explicit `--author ld-reviewer`
  short-circuited the only identity read; the post-write read-back compared the
  stored body and never the stored author. The 403 regression case shows it
  plainly: with `--author` set, no GET /user was issued at all.

The guard (forgejo/api.ts): `assertWriteIdentity` reads GET /user with the same
token and compares it against an EXPLICITLY configured expected identity —
`expected_identity` on the tool, `FORGEJO_EXPECTED_IDENTITY` in the env, or the
helper's `--author`. There is no universal hardcoded reviewer: `aaron`, workers
and `ld-reviewer` all legitimately post comments, so the expectation is
configuration. What is refused before any write:

- an identity that differs from the declared expectation;
- an identity that cannot be read (GET /user fails or answers without a login);
- a landing-lock VERDICT body 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
and verifies both the body and the author. The verdict helper does the same: it
reads GET /user on every run, refuses a mismatched `--author` in dry run too,
requires a readable identity for `--post`, and fails loudly if the stored author
is not the token's identity.

No historical comment is edited or deleted anywhere in this change; the guard is
pre-POST only. `forgejo/landing.ts`, the landing tests and every merge behaviour
are untouched, and `docs/forgejo.md` is skipped because open PR #74 holds it.

Verification: forgejo suite 265/265 (17 new identity tests), tsc clean.
Collaborator

Verdict: APPROVED

model: omniroute/deepseek-flash
head: c682d40359

Independent GENERAL review — fresh context, no edits, no merge, no other PR, no formal review submitted. Exact head re-fetched live immediately before this post and compared to the sha in the header above: unchanged. Poster identity proved first (GET /user = ld-reviewer, id 47) via the sanctioned reviewer token; no credential on argv, none printed.

Re-measured rather than read from the worker's logs:

  • red-first: base 6518158 api/index/verdict-cli plus this PR's own test file → tests/comment-identity.test.ts fails 14/17 (3 pass); at this head it passes 17/17. The 11-failure log is the earlier 14-test revision of the same file, and the shapes it names reproduce.
  • full suite: base 248/248, head 265/265, 12 files — additive only (+17), no pre-existing test edited or deleted.
  • tsc --noEmit clean.
  • CI on this exact sha: 14/14 success (forgejo, forgejo-ci, plane, pi-swarm-suite, redact, pr-title, …).

Security contract, attacked with a fake API and a driver outside the repo:

  • GET /user precedes the write on both paths. Tool order is GET /user → POST comment → GET comment read-back, exactly one POST; helper order is collectState (all GETs) → GET /user → one POST → read-back.
  • The expectation is configuration, not a constant: FORGEJO_EXPECTED_IDENTITY, the tool's expected_identity and the helper's --author each drive it, and a matching aaron token posts an ordinary comment.
  • Refusals are pre-write and total: mismatch, GET /user 403, GET /user 200 with no login, and a verdict body with no expectation each produce zero mutating requests of any verb (POST/PUT/PATCH/DELETE), not merely zero comment POSTs; the text names expected and actual logins and never the token.
  • The branded proof holds: postVerifiedComment called with a hand-rolled {ok:true, actual}, with a forged symbol-keyed object, or with undefined throws and writes nothing; only a minted assertion reaches the POST.
  • Success path: one POST, stored body compared verbatim, stored author compared to the token identity; a server-side misattribution is a loud failure after the POST on both paths, with no repost.
  • Helper regression closed: with --author supplied, GET /user is now issued anyway; a mismatched --author refuses even in a dry run; --post with an unreadable identity refuses; a 403 keeps its own exit code.
  • api.ts now imports isReviewVerdict from landing.ts; landing.ts imports nothing, so no cycle. extractVerdict gates on readVerdictStance, which itself requires isReviewVerdict, so the bodies the lock can count are a subset of the bodies the tool's verdict rule intercepts. A verdict declared as aaron's own login posts but is non-evidence under the trusted-identity filter, so declaring an identity cannot manufacture a counted approval.

OBSERVATIONS, hardening and surface items only (none of these can make the lock count a verdict it should not; all reproduced, recorded in my working note):

  • O1, narrow fail-open: an explicit empty expected_identity wins over FORGEJO_EXPECTED_IDENTITY, so expected_identity: "" silently disables a process-wide expectation for non-verdict comments (reproduced: env ld-reviewer, token aaron, ordinary body, empty explicit → posted). Verdict-shaped bodies are still refused. One-line fix: treat "" as unset or refuse it, plus a test.
  • O2, silent degradation: a failed read-back is swallowed (.catch(() => null)) and the author then falls back to the POST echo while the body comparison is skipped (reproduced with a 404 on the read-back: success reported, row never read). The identity is then asserted, not proven — consider failing loudly.
  • O3: commentOnPR is still exported and unguarded; postVerifiedComment is its only in-repo caller, so this is compatibility-preserving, but a deprecation note would close a future bypass.
  • O4: the forgejo_comment_on_pr details shape changed from the comment row to a flat refusal/success record; nothing in the repo consumes it. docs/forgejo.md does not mention expected_identity (deliberately deferred behind PR #74) and the operational note that a token without read:user can no longer comment at all is worth recording.
  • O5: verdict-cli keeps its own sameLogin copy rather than importing the api.ts one — duplication, not a defect.

Scope: exactly the four expected files (+796/−9); landing.ts, verdict.ts, merge behaviour, docs and plane/ untouched. Worktree note: /home/aaron/repos/pi-extensions-2527 shipped with an empty forgejo/node_modules, so I re-installed dependencies in a throwaway archive of this head under /tmp and measured there; nothing in the repo was modified.

I am approving the diff: the two measured defects are closed on both write paths at this exact head, and the items above are hardening and surface polish. forgejo is dangerous class, so this is the general lens only: a premium second opinion on a different declared model is required before this lands.

## Verdict: APPROVED model: omniroute/deepseek-flash head: c682d4035935bf0e6978e00828be25c4d847b719 Independent GENERAL review — fresh context, no edits, no merge, no other PR, no formal review submitted. Exact head re-fetched live immediately before this post and compared to the sha in the header above: unchanged. Poster identity proved first (`GET /user` = ld-reviewer, id 47) via the sanctioned reviewer token; no credential on argv, none printed. Re-measured rather than read from the worker's logs: - red-first: base `6518158` api/index/verdict-cli plus this PR's own test file → tests/comment-identity.test.ts fails 14/17 (3 pass); at this head it passes 17/17. The 11-failure log is the earlier 14-test revision of the same file, and the shapes it names reproduce. - full suite: base 248/248, head 265/265, 12 files — additive only (+17), no pre-existing test edited or deleted. - `tsc --noEmit` clean. - CI on this exact sha: 14/14 success (forgejo, forgejo-ci, plane, pi-swarm-suite, redact, pr-title, …). Security contract, attacked with a fake API and a driver outside the repo: - GET /user precedes the write on both paths. Tool order is GET /user → POST comment → GET comment read-back, exactly one POST; helper order is collectState (all GETs) → GET /user → one POST → read-back. - The expectation is configuration, not a constant: FORGEJO_EXPECTED_IDENTITY, the tool's expected_identity and the helper's --author each drive it, and a matching aaron token posts an ordinary comment. - Refusals are pre-write and total: mismatch, GET /user 403, GET /user 200 with no login, and a verdict body with no expectation each produce zero mutating requests of any verb (POST/PUT/PATCH/DELETE), not merely zero comment POSTs; the text names expected and actual logins and never the token. - The branded proof holds: postVerifiedComment called with a hand-rolled `{ok:true, actual}`, with a forged symbol-keyed object, or with undefined throws and writes nothing; only a minted assertion reaches the POST. - Success path: one POST, stored body compared verbatim, stored author compared to the token identity; a server-side misattribution is a loud failure after the POST on both paths, with no repost. - Helper regression closed: with --author supplied, GET /user is now issued anyway; a mismatched --author refuses even in a dry run; --post with an unreadable identity refuses; a 403 keeps its own exit code. - api.ts now imports isReviewVerdict from landing.ts; landing.ts imports nothing, so no cycle. extractVerdict gates on readVerdictStance, which itself requires isReviewVerdict, so the bodies the lock can count are a subset of the bodies the tool's verdict rule intercepts. A verdict declared as aaron's own login posts but is non-evidence under the trusted-identity filter, so declaring an identity cannot manufacture a counted approval. OBSERVATIONS, hardening and surface items only (none of these can make the lock count a verdict it should not; all reproduced, recorded in my working note): - O1, narrow fail-open: an explicit empty `expected_identity` wins over FORGEJO_EXPECTED_IDENTITY, so `expected_identity: ""` silently disables a process-wide expectation for non-verdict comments (reproduced: env ld-reviewer, token aaron, ordinary body, empty explicit → posted). Verdict-shaped bodies are still refused. One-line fix: treat "" as unset or refuse it, plus a test. - O2, silent degradation: a failed read-back is swallowed (`.catch(() => null)`) and the author then falls back to the POST echo while the body comparison is skipped (reproduced with a 404 on the read-back: success reported, row never read). The identity is then asserted, not proven — consider failing loudly. - O3: `commentOnPR` is still exported and unguarded; postVerifiedComment is its only in-repo caller, so this is compatibility-preserving, but a deprecation note would close a future bypass. - O4: the `forgejo_comment_on_pr` details shape changed from the comment row to a flat refusal/success record; nothing in the repo consumes it. docs/forgejo.md does not mention expected_identity (deliberately deferred behind PR #74) and the operational note that a token without read:user can no longer comment at all is worth recording. - O5: verdict-cli keeps its own sameLogin copy rather than importing the api.ts one — duplication, not a defect. Scope: exactly the four expected files (+796/−9); landing.ts, verdict.ts, merge behaviour, docs and plane/ untouched. Worktree note: `/home/aaron/repos/pi-extensions-2527` shipped with an empty `forgejo/node_modules`, so I re-installed dependencies in a throwaway archive of this head under /tmp and measured there; nothing in the repo was modified. I am approving the diff: the two measured defects are closed on both write paths at this exact head, and the items above are hardening and surface polish. forgejo is dangerous class, so this is the general lens only: a premium second opinion on a different declared model is required before this lands.
Collaborator

Verdict: APPROVED

model: omniroute/kimi-pool
head: c682d40359

Premium (kimi-pool) exact-head review of c682d40359. All work
claims reproduce independently: targeted 17/17, full forgejo suite 265/265, tsc clean, CI #165
14/14 green at this head. Scope is exactly the four declared files; landing.ts is untouched and
the new api.ts -> landing.js import is acyclic.

The security contract holds on both write surfaces. The expected identity is configuration
(explicit argument, FORGEJO_EXPECTED_IDENTITY, or --author) with no universal hardcoded
reviewer. GET /user is read unconditionally before any write in both the tool and the verdict
helper; the old --author short-circuit is regression-pinned closed. Mismatch, unreadable
identity, and a verdict body with no declared identity all refuse before any mutation, and every
refusal test proves zero POST/PUT/PATCH/DELETE requests, not merely zero comment POSTs. The
branded proof is a module-private symbol checked at the POST boundary, so a hand-rolled
{ ok: true } cannot reach the write. On success there is exactly one POST followed by a read-back
that verifies the stored body verbatim and the stored author case-insensitively against the
asserted identity; a post-write authorship mismatch fails loudly with an explicit do-not-repost
instruction. No route can post under aaron while declaring ld-reviewer, no branded-proof forgery
path exists, and existing callers keep working: ordinary comments under any readable sanctioned
identity still post and now report the real author.

Deliberate behaviour changes, both consistent with the ticket contract: the tool details shape
is now a flat record carrying the read-back author, and a token whose identity is unreadable
(no read:user scope) can no longer post any comment. docs/forgejo.md documentation is
disclosed-deferred behind PR #74, and the worker's plane/ anomaly flag is a correct scope call
for an owner ticket.

No findings, no blockers. Approved for landing at this exact head.

## Verdict: APPROVED model: omniroute/kimi-pool head: c682d4035935bf0e6978e00828be25c4d847b719 Premium (kimi-pool) exact-head review of c682d4035935bf0e6978e00828be25c4d847b719. All work claims reproduce independently: targeted 17/17, full forgejo suite 265/265, tsc clean, CI #165 14/14 green at this head. Scope is exactly the four declared files; landing.ts is untouched and the new api.ts -> landing.js import is acyclic. The security contract holds on both write surfaces. The expected identity is configuration (explicit argument, FORGEJO_EXPECTED_IDENTITY, or --author) with no universal hardcoded reviewer. GET /user is read unconditionally before any write in both the tool and the verdict helper; the old --author short-circuit is regression-pinned closed. Mismatch, unreadable identity, and a verdict body with no declared identity all refuse before any mutation, and every refusal test proves zero POST/PUT/PATCH/DELETE requests, not merely zero comment POSTs. The branded proof is a module-private symbol checked at the POST boundary, so a hand-rolled { ok: true } cannot reach the write. On success there is exactly one POST followed by a read-back that verifies the stored body verbatim and the stored author case-insensitively against the asserted identity; a post-write authorship mismatch fails loudly with an explicit do-not-repost instruction. No route can post under aaron while declaring ld-reviewer, no branded-proof forgery path exists, and existing callers keep working: ordinary comments under any readable sanctioned identity still post and now report the real author. Deliberate behaviour changes, both consistent with the ticket contract: the tool details shape is now a flat record carrying the read-back author, and a token whose identity is unreadable (no read:user scope) can no longer post any comment. docs/forgejo.md documentation is disclosed-deferred behind PR #74, and the worker's plane/ anomaly flag is a correct scope call for an owner ticket. No findings, no blockers. Approved for landing at this exact head.
aaron merged commit fd410c3ce8 into main 2026-11-02 23:31:45 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
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!75
No description provided.