docs(process): implementation plan for publishing one review comment (#119) - #167
Merged
Conversation
Plans #119 against the contract #117 defined, at the revision that existed when this was written: exactly one comment per pull request, updated in place on push rather than appended, with findings ordered most severe first across updates. Recorded as provisional on purpose — #117's plan had not been through review-plan when this was planned, so a renamed or dropped contract field changes the steps this plan names. Verified with the plan checker (plan-issue/check-plan.sh): mechanical checks clean. The stamp was touched rather than earned because this worktree holds a markdown plan and no suite the gate's TEST_PATTERN recognises. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
#120 removed render_review's `unreadable` keyword in e072fba — it had no producer anywhere on that branch, so the "Incomplete" banner it fed could never render. The parameter is now derived inside the function from `states`. This plan was committed against the old signature, and STEP 4 prescribed passing `unreadable` straight into `review.render_review`, which raises TypeError. The call, the `containment` block shape and the ALREADY TRUE citation are corrected; the old signature is kept as history rather than deleted. Two consequences recorded rather than quietly fixed: - `states` is now load-bearing for the "Incomplete" banner, not just the "Fetched and empty" line. A present-but-thin `states` map re-creates #120's original defect one stage up, and STEP 5's MISSING-block trigger does not cover it, so the control must assert all seven entry points are named. - BUDGET said #120 exists "on no branch and in no commit". It is now three pushed commits at c64ff79 with a green control suite. Its own instruction to re-verify the signature before STEP 4 is what caught this. check-plan.sh: mechanical checks clean. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Adds the serina:review-plan report for #119 and the plan revision that answers it. The reviewer was independent — it did not draft the plan, and it executed every factual claim rather than recalling it. Fourteen findings: one Blocker, six High, six Medium, one Low. The Blocker was WITHDRAWN, and how it failed is worth keeping. It concluded #117 had moved containment findings inside `reports`, which would have made this plan's separate `containment` block wrong; #117 then settled the contract the other way, so the separate block was right all along. Its residue was real though — the plan's ALREADY TRUE asserted a nine-field record and no `evidence` field, and both were false. Of the remaining thirteen, eleven are fixed in this revision. Finding 3 (no nonce reaches publish.py, so half the completion-marker check cannot be implemented) is fixed as far as this stage can and flagged upstream in OPEN. Finding 13 (whether an edit is visible to a human) is converted from an unanswerable question into a STEP 1 observation. Finding 4 is the `render_review` TypeError already corrected in 8a405a9, found here independently by a reviewer that had not seen that work. Two routes to the same defect is the reason the separate pass is worth its cost. The report's structural observation is the one to carry forward, because it generalises past this plan: OPEN enumerated what breaks when a #117 field is RENAMED or REMOVED, and what actually broke it was a field being ADDED. An addition is invisible to that register. A second review pass is available and has not run. On both #116 and #117 the second pass found most of its findings inside the first pass's own fixes, so assume the same of this revision rather than treating one clean round as evidence. check-plan.sh: mechanical checks clean. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
The plan for publishing one review comment has been through serina:review-plan four times and is now revised against all of it. Two passes were run by the author of the revision under review and say so in their own reports; one was an independent reviewer on a different model, blocked from reading the earlier findings; one came from the #117 session. Two Blockers were found only by the later passes: - find_existing matched on the marker alone, and a review body is attacker-writable — any user with read access can submit a marked COMMENT review on a public-fork pull request. The agent would then try to PUT a review it does not own, fail, and publish nothing on that pull request permanently. Identification is now marker AND author, with the login resolved at runtime from GET /user so the credential can still move without a code change. - The live credential control was registered into #120's read-only controls runner, so it would have reported PASS against a token that was never the one under test. It now asserts GITHUB_WORKFLOW and PASSes only from the publish workflow, which STEP 8 declares. One Blocker was self-inflicted and disproved by measurement: an earlier revision escaped defect and failure through contain.escape, on the claim that ordinary prose survives the transform. contain.ESC is a tilde, so a path such as ~/.claude/settings.json publishes as markdown strikethrough. Withdrawn. One finding was withdrawn because #117's contract settled the opposite way to what the first pass assumed; its residue was real and is fixed. Still owed upstream: the run nonce is verifiable by no stage downstream of #117's runner, because it appears in no field of the merged document. The fix belongs in #117, not as a workaround here. The reconciliation of all four passes is recorded beside the plan. Verification: check-plan.sh clean on the plan file. No test covers a markdown document, so the verify-gate stamp was authorised by the repository owner for a documentation-only change rather than earned by a test run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
The identity fix in d97368d was right about the decision and wrong about the mechanism. find_existing resolved the agent's own login "at runtime from GET /user". GitHub's REST documentation for "Get the authenticated user" lists OAuth app tokens and personal access tokens (classic); installation access tokens are not among them, and GITHUB_TOKEN is an installation credential. The 403 is NOT claimed as proved — an installation token exists only inside a real Actions run, so it cannot be tested from a worktree, and STEP 1 now records what the endpoint actually returns rather than the plan asserting it. The change does not depend on settling that. If the call is refused, resolving the identity that way fails on every pull request under the credential it was written for, and invisibly, because STEPs 1 and 3 run under a human token where GET /user succeeds. If it is not refused, a runtime call is still a needless failure mode for a value known before the run starts. So the login is supplied and verified rather than discovered: - STEP 2 takes the expected login as a parameter. An unresolved identity raises; it never degrades to matching on the marker alone, which would restore the vector the author filter closes. - STEP 7 wires --as <login>, defaulting to github-actions[bot]. A default value on a flag is not a hardcoded comparison: moving the credential under #110 is one flag rather than a code change. Every bot comment in this fork is authored by github-actions[bot], with no other bot login present. - STEP 9 gains an identity assertion, inside the publish workflow, that the configured login equals the login the credential actually posts as. That is the only place the real identity exists, and it is what catches a stale default — without it the filter degrades quietly into one review per push. - STEP 12 states supply-and-verify normatively. Raised by the #117 session as an ACTION note after d97368d had landed. Its own proposal defaulted the flag to a value STEP 1 measures; STEP 1 holds a human token, so it measures the wrong identity, and the default is verified against the live credential in STEP 9 instead. Verification: check-plan.sh clean on the plan file. No test covers a markdown document, so the verify-gate stamp was authorised by the repository owner for a documentation-only change rather than earned by a test run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
#117 closed the gap this plan raised against it. The completion marker is BUZZ-DIMENSION-COMPLETE:{dimension}:{nonce}, and until now the nonce reached no stage downstream of #117's runner — not in the finding record, not in the envelope, not in the merged document — so a marker could not be checked against anything. It was flagged upstream rather than worked around, and #117 added a top-level nonce key, naming this plan in its change list. Four changes: - STEP 5 condition (5) now compares each report's marker nonce against the merged document's nonce. Marker-to-marker disagreement stays as a secondary check, because reports that disagree with each other are broken regardless. - STEP 5's done-when drops the "another dimension's nonce" fixture and now forbids the phrasing by name. #117 corrected this plan on the point: there is ONE nonce per run, so that describes an input the contract cannot produce, and a test built on it passes against any implementation. It had survived two revisions here as a test that cannot fail. - STEP 7 takes nonce as a seventh stdin key — #117's six plus this plan's own stages manifest. - OPEN records the concern as discharged rather than deleting it, and STEP 12 carries the limit into PUBLISHING.md. The limit is kept deliberately. In #117's words the key is "not an authentication token for the document, only for a marker echoed out of author text". A forged marker fails against the document nonce; a compromised runner emits a self-consistent document with its own nonce and this stage cannot tell. STEP 12 now forbids describing the published review as unforgeable. One blind spot closes for free: the old agreement-only check needed siblings, so a single-dimension run — or a three-dimension run where two failed — had no nonce checking at all, precisely when the run was already degraded. Comparing against the document works with one report. Second time on this issue that raising a gap upstream produced a contract change rather than a local workaround, after the containment sibling key. Both times the alternative was guessing at a shape #117 owns. Verification: check-plan.sh clean on the plan file. No test covers a markdown document, so the verify-gate stamp was authorised by the repository owner for a documentation-only change rather than earned by a test run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Two records authored by the session working on #117, not by this one. They are committed here because three commits already on this branch cite them by name, so the pushed history was referencing files that were not in it. - 2026-08-13-119-plan-review-second-pass.md — an independent second pass on #119's plan, run from the #117 worktree. Seven findings, three of which survived the revision that was in flight while it ran, two of those Blockers: find_existing matching attacker-writable body text with no author filter, and the live credential control registered into the read-only controls runner so it would PASS against a token that was never under test. Both are fixed on this branch. Its addendum records that, and adds the GET /user finding. - 2026-08-13-119-ACTION-get-user-under-actions-token.md — the short form of that addendum, written so the finding could be acted on without reading the whole record. It is the reason 26aa16f exists. Both were read in full before committing. The second-pass record grew from 403 to 469 lines after its first reading, so the added addendum was read before this commit rather than assumed unchanged. Worth keeping for the record: that pass declares its own partial non-independence — it wrote #117's containment settlement, which is what caused the first pass's Blocker 1 to be withdrawn — and it concedes one of its own findings was wrongly dismissed. Both disclosures are in the file. Verification: no test covers a markdown document, so the verify-gate stamp was authorised by the repository owner for a documentation-only change rather than earned by a test run. These files are committed as authored; nothing in them was edited by this session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
An independent Codex review (codex review --base origin/launchpad) found three P1s and two P2s in this plan, all confirmed real by reading the plan's own text before fixing anything: 1. STEP 8's workflow used pull_request, copied from #120's own comment block explaining why that trigger is safe there -- but #120's job is read-only and this one holds pull-requests: write. Under pull_request, a SAME-REPOSITORY pull request (the norm this cohort actually produces, per ALREADY TRUE) gets that write token while its own diff can modify publish.py or check_publish_scope.py, the exact code the job then executes -- a path to an APPROVE under the bot's identity, violating AGENTS.md rule 1. Switched to pull_request_target with an explicitly-never-overridden base-ref checkout, so the code that runs is always what launchpad has committed regardless of what the PR changed. 2. The fork guard was a step exiting 0, which marks only that step successful -- every later step in the job still runs regardless. Restructured as a job-level `if:` gating a separate `publish` job on a `guard` job's output, which actually skips every subsequent step at once. 3. No step in the workflow ever produced the seven-key document and piped it into publish.py -- every other step (permissions, triggers, the guard, the scope control) could be correct while the workflow still never published on any run. Named as an explicit step, specified against the CLI shape #116/#117/#118 each state for themselves since none exist as code yet. 4. post_or_update had no way to surface user.login for STEP 9's identity control. Now returns it as a third element, read off the POST/PUT response. 5. A changed --as login orphaned this agent's own prior review as merely "foreign" and silently POSTed a second one, breaking the exactly-one-review invariant. find_existing now returns a (id, foreign_count) pair; post_or_update refuses to POST when the foreign count is nonzero, since that count cannot be told apart, from the data alone, from a planted decoy -- a human looks at either case rather than the tool guessing. check-plan.sh remains clean. No code exists on this branch to earn a verification stamp; touched per verify-gate's own instructions for a no-suite project. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
serina-mcfall
marked this pull request as ready for review
August 14, 2026 05:52
This was referenced Aug 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the implementation plan for publishing exactly one PR review comment, plus the record of two independent
serina:review-planpasses over it and a targeted ACTION note. The plan settles identification (a hidden marker plus the agent's own author identity, never marker alone), the credential-scope control's workflow guard, and a control asserting a clean run still posts. No part of the plan is built: this PR is the plan and its review, nothing else.Related issue
Refs #119
This is deliberately
Refsand notCloses. #119's done-criteria requirepublish.py, the credential control and the workflow itself to exist and be exercised; none of that exists here. The body becomesCloses #119when STEPs 2 through 12 land on this branch.Issue type
Task
Agent provenance
Objective
A 12-step implementation plan for #119 at
launchpad/plans/2026-08-12-issue-119-publish-one-review.md, with identification, the credential-scope control, and the clean-run-posts guarantee settled in enough detail for an implementer to build against without renegotiating.Impacted components
launchpad/plans/2026-08-12-issue-119-publish-one-review.md
launchpad/plans/reviews/2026-08-13-119-plan-review.md
launchpad/plans/reviews/2026-08-13-119-plan-review-second-pass.md
launchpad/plans/reviews/2026-08-13-119-ACTION-get-user-under-actions-token.md
Approach and rejected alternatives
The design question that decided the rest was how to identify "our own" review among every review GitHub returns for a pull request.
find_existingrequires a review to carry both a hidden marker comment AND to have been authored by the agent's own configured login, author checked first — never marker alone.Rejected: marker-only matching, which an earlier revision used. A review body is not agent-controlled territory — any GitHub user with read access can submit a marked COMMENT review on a public fork's pull request. Marker-only matching lets an outside party choose which review object the agent tries to update: they plant a marked review,
find_existingreturns its id, the PUT is refused because the token doesn't own it, and the plan's own hard-failure rule (never fall back to POST on a failed PUT) means the agent then publishes nothing on that PR, permanently — a denial-of-publication vector an outsider can trigger deliberately, presenting as an HTTP error rather than an attack.Also rejected: resolving the agent's own identity at runtime via
GET /user. GitHub's REST documentation for "Get the authenticated user" lists OAuth app tokens and personal access tokens as what it supports; installation access tokens (whatGITHUB_TOKENis under Actions) are not among them. The 403 is not claimed as proved here — it can't be tested from a worktree — but the change doesn't depend on settling it: if the call is refused under Actions, the agent fails on every pull request, invisibly, because the steps that exercise it locally run under a human token where the call succeeds. Replaced with supply-and-verify:--as <login>defaults to a value STEP 1 measures rather than one the plan asserts, and a live control (STEP 9) asserts the configured login equals what the credential actually posts as — catching a stale default the day #110 moves the credential.Also rejected: letting the live credential-scope control run wherever
run_controls.pyruns. #120's controls workflow shares that runner and holds a read-only token ({contents: read, issues: read, pull-requests: read}). A scope probe attempting acontents: writeref-create returns 403 under BOTH tokens, so without a workflow-identity guard the control would report PASS having measured the wrong credential entirely — a false PASS pasted into a PR as evidence about a token it never touched. The live half now assertsGITHUB_WORKFLOWand SKIPs, never PASSes, outside the actual publish workflow.Verification
Command run:
Raw output:
Not verified
No code exists on this branch —
launchpad/review-agent/publish.py,check_publish_scope.pyandcheck_publish_single.pyare all plan text, not files.check-plan.shverifies the plan's mechanical shape (sections, step cap, dependency tags); it does not and cannot verify that the design is correct, only that a builder has enough to start. The 403-under-an-installation-token claim underlying the identity redesign is explicitly unverified in the plan itself (STEP 1 records whatGET /useractually returns rather than asserting it), since no installation token is reachable from a worktree.Security implications
This plan's central concern IS security: a review body is attacker-writable, so identification by marker alone hands any outside contributor a way to silence the agent's publication on a pull request of their choosing. The author-filter-first design, the never-fall-back-to-POST rule on a failed PUT, and the workflow-identity guard on the live credential control all exist to close variants of that one vector. Nothing here is built yet, so nothing here is exposed yet.
Escalations
Two cross-issue items, both already discharged upstream rather than left as blocking risk: #117's merged document previously carried the run nonce only inside each report's completion marker, with no way for this stage (or #118) to check it against the value actually generated for the run — #117 has since added a top-level
noncekey (verified:git show 733f48088:...) closing this. Separately, #117 previously left where containment findings live unsettled between a converted record and a sibling key; #117 settled on the sibling key, which is what this plan already assumed.Two items are recorded as genuinely open rather than decided here, per the plan's own OPEN section: whether the publish workflow is its own file or folds into #116's invocation (a sequencing call, not a correctness one), and whether
#118's severity re-rating needs a second field since the current contract carries exactly oneseverityvalue for both the original and re-rated ratings.