diff --git a/launchpad/plans/2026-08-12-issue-119-publish-one-review.md b/launchpad/plans/2026-08-12-issue-119-publish-one-review.md new file mode 100644 index 00000000000..73b5dc71191 --- /dev/null +++ b/launchpad/plans/2026-08-12-issue-119-publish-one-review.md @@ -0,0 +1,1565 @@ +Issue #119 — task: publish exactly one PR review comment, and re-review on push +Stated size: none given — the task template has no Size field -> cap: 12 steps + +Sized by asking, not guessing. Answered: more than an hour, so the cap is 12. +Two further answers were asked rather than assumed, because both change what gets +built and neither is a builder's call: + + - **Re-review means update in place.** GitHub cannot delete or dismiss a + submitted review of type COMMENT (see ALREADY TRUE), so #119's two + done-criteria — "a new review for that commit" and "does not accumulate one + stale review per push" — cannot both be literally true. Answered: POST once, + then PUT the body on every later push, with the head SHA named inside the + body. Exactly one review object for the life of the pull request. + - **A fork pull request is skipped loudly.** On `pull_request` a fork gets a + read-only token and cannot post at all. Answered: detect it, exit 0 having + posted nothing, and say why in the job log. + +Larger than an hour is flagged, not refused. These would each have been an +observable issue on their own — splitting is the reader's call, not this plan's: + + (a) the single-review lifecycle alone — locate, post, update, never duplicate — + against a stub body, with no findings renderer written + (b) the body renderer over #117's report envelopes, including the incomplete + and clean cases, printing to stdout and posting nothing + (c) the credential and its controls — the permissions block, the static + assertion, and the live proof that contents write is absent + (d) the workflow that triggers it + +Planned as written below. + +ALREADY TRUE (verified against git, the working trees, the live GitHub API and +the sibling worktrees — not against notes) + + Nothing of #119 is built. `git ls-files | grep -i publish` matches only + unrelated upstream desktop React files, and no launchpad/review-agent/ directory + exists on this branch. + THIS BRANCH MOVES UNDER THIS FILE, so the anchor is dated rather than asserted. + It read "at d897a06e8" until an independent review pass checked it: HEAD was + eb2bf09d0 when this plan was committed, and 8a405a9f5 ("match #119's plan to + render_review's current signature") landed while it was being revised — a commit + from another session working in this same worktree. As of 2026-08-13 the branch + is at 8a405a9f5 with this plan file itself carrying further staged edits. The + substantive claim survives every one of those moves; the SHA did not. This is the + pinned-fact rot this plan warns about for review.py's line numbers, applied to + its own anchor commit — which is why it is now a dated observation and why a + reader should re-run `git log -1` rather than trust the number above. + + ADR #110 is decided, and it names #119 by name as what it unblocks. The + decision comment on #110 chooses GitHub Actions for Phase 1 with a committed + revisit, and fixes the credential as "a GitHub token scoped to + `launchpad-26/buzz` (pull-request write, contents read)" — no Buzz identity, + deferred explicitly. #119's own stated blocker ("the GitHub token and its + scopes — provisioning depends on #110") is therefore settled, and this plan is + written against that credential. + + The review lifecycle API, checked live rather than recalled. On PR 86 of this + fork, `gh api repos/launchpad-26/buzz/pulls/86/reviews` returns objects + carrying `id`, `state` ("COMMENTED"), `commit_id`, `submitted_at` and `user`. + `commit_id` is set at submission and is NOT updated when the review body is + later edited — which is precisely why #119's criterion "the comment names the + commit SHA it reviewed" has to be met by text inside the body, not by the + review object's own field. The stronger claims — that PUT on a review id + returns 200, that DELETE returns 422 for a non-pending review, and that + dismissal is refused for a COMMENT review — are NOT verified here. They are + what STEP 1 exists to record. This plan's chosen strategy depends on the first + of the three, and STEP 1 is where it either holds or the strategy changes. + + PR 86 is same-repository: `isCrossRepository` is false and + `headRepositoryOwner.login` is `launchpad-26`. That matches how the cohort + works — branches are pushed to the fork, not forked from it. An earlier + revision read that as making the fork-skip cheap in practice rather than a + real gap; it is the opposite. Same-repository is exactly the case a fork skip + does nothing to protect, and it is the case this cohort actually produces: a + same-repo PR's own diff can modify the code STEP 8's job executes, and under + plain `pull_request` that code runs with the write token this job holds. STEP + 8's move to `pull_request_target` with an unoverridden base-ref checkout is + the fix that closes the gap this observation actually describes. + + #120's work exists, is substantial, and is committed nowhere. The worktree + /home/serina/Launchpad/buzz__worktrees/feat-review-agent-untrusted-input is at + d897a06e8 with launchpad/review-agent/ UNTRACKED — 14 Python modules, three + fixtures, and CONTAINMENT.md. `git log --all --oneline -- launchpad/review-agent/` + returns nothing. Everything #119 imports currently exists only in another + worktree's working directory. This is the single largest risk in this plan; see + BUDGET. + + #119's renderer already has a sibling, and that sibling says #119 owns + publication. review.py's module docstring opens "Render the review body. Does + not post it — #119 owns publication." `review.SEVERITY_ORDER` is + {"Blocker": 0, "High": 1, "Medium": 2, "Low": 3} at review.py:32, and + `render_review(findings, states) -> str` at review.py:45 + already renders containment findings, an "Incomplete" line naming unreadable + surfaces, and a COVERAGE_NOTE. #119 composes with that function; it does not + replace it and does not re-declare the ladder. + **The signature changed after this plan was committed, and the old one is a + TypeError.** It was `render_review(findings, states, *, unreadable=None)` at + review.py:42 when STEP 4 was written. #120 removed the `unreadable` keyword in + `e072fba55`, because it had no producer anywhere on that branch — so the + "Incomplete" banner it fed could never render, and a caller cannot forget an + argument that does not exist. `unreadable` is now DERIVED inside the function + from `states`, against `UNREADABLE_STATES = ("absent", "oversized", + "unparseable")` — which now occupies review.py:42, the line this plan cites for + the signature. STEP 4 is corrected accordingly. + Those two line numbers were 19 and 29 when this plan was first drafted and + moved to 32 and 42 within the hour, because #120 was actively writing in that + worktree. `render_review` has since moved again, to 45, while SEVERITY_ORDER + stayed at 32. Recorded here as of 2026-08-13 against `c64ff7958`. Cite them as + orientation, never as evidence — this paragraph's warning has now fired twice, + and the second time it was the signature rather than the line that moved. + + `render_review` does NOT accept #117's ten-field records, and that is a design + constraint, not a detail. It takes `findings: list[Finding]` and reads + `.severity`, `.kind`, `.entry_point` and `.evidence` by ATTRIBUTE — the + `contain.Finding` dataclass, not a JSON object — plus a `states: dict[str, + str]` mapping entry point to fetch state, which it uses for its "Fetched and + empty" line AND, since `e072fba55`, for the "Incomplete" banner. So containment + findings cannot reach #119 through the `reports` array, and STEP 7's input + contract carries them separately. This was found by review, not by the first + draft, which assumed composition would just work. + Two claims an earlier revision of this paragraph got WRONG, corrected here + rather than quietly dropped: + — "#117's envelope carries neither `kind` nor `evidence`." The `kind` half is + true: `kind` exists only on a containment finding, never on the ten-field + record. The `evidence` half is false. `evidence` is the TENTH field of that + record, RAW, and REQUIRED whenever `entry_point` is set. STEP 4 renders it, + and finding it absent from this plan's own field list is what STEP 4's + escaping rule exists to answer. + — "no stage in the chain emits `states` at all." #117 now emits + `containment.states` for ALL SEVEN entry points on every run. That is the + producer whose absence removed the `unreadable` keyword in the first place. + Both errors came from reading revision 3 of #117's contract; the contract is + now settled and both are fixed against it. Recorded because a verified section + that carries a false claim is worse than one that carries none — a later reader + trusts it by construction. + + A workflow already exists for the controls, is untracked, and is read-only. + .github/workflows/launchpad-review-agent-controls.yml in #120's worktree runs + on `pull_request` with `permissions: {contents: read, issues: read, + pull-requests: read}` and a comment explaining, correctly, why it is not + `pull_request_target`. It cannot host the publish job: publishing needs + pull-requests **write**, and widening that file would give the containment + controls a write token they have no use for. + + A control runner exists and is the registration point. run_controls.py holds a + CONTROLS list of (script, needs_network) pairs, probes `gh api rate_limit` for + connectivity, and reports SKIP with a reason — never PASS — for a control whose + input is missing. #119's controls register there rather than inventing a second + runner. + + #117's output contract is now SETTLED, and this plan is rewritten against it. + It is at launchpad/plans/2026-08-12-issue-117-review-dimensions.md in the + feat-review-agent-dimensions worktree, it has been through serina:review-plan + three times, and it carries a "CONTRACT CHANGES SINCE REVISION 3" block written + expressly so this plan's author could diff. It is still UNCOMMITTED, so it is a + settled decision rather than a settled ref. + TEN finding fields, per report envelope entry: `dimension`, `severity`, + `anchor` (line|file|pr), `file`, `line` (new-side, at head_sha), `defect`, + `failure`, `finding_id`, `entry_point`, `evidence`. ELEVEN envelope fields: + `schema_version`, `dimension`, `pr`, `merge_base_sha`, `head_sha`, `status` + (complete|failed), `outcome` (findings|clean), `error`, `findings`, + `findings_count`, `completion_marker`. And FIVE merged-document keys, which is + what #119 actually reads: `pr`, `merge_base_sha`, `head_sha`, `reports`, + `containment`. + WHERE CONTAINMENT FINDINGS LIVE is the settled part that decides STEP 4, and it + settled the way this plan already assumed. Quoting the contract: containment + findings "do NOT enter the `findings` array of any dimension report, and they + are NOT converted into the ten-field record. They travel as a TOP-LEVEL SIBLING + KEY of the merged document, named `containment`, carrying contain.Finding + verbatim in JSON" — `findings[]` of {severity, kind, entry_point, evidence} and + a `states` map of all seven entry points. The reserved "containment" dimension + slug an earlier revision proposed is WITHDRAWN, and no dimension may claim that + slug. So STEP 4's separate block is correct and stays. + Five changes from revision 3, and only two cost this plan anything: + 1. the `containment` sibling key exists and has the shape STEP 4 specified — + this is the key OPEN said #117 must add, and it is added. + 2. `evidence` is RAW, not post-escape. This one costs STEP 4 a rule: raw + attacker text now arrives in the document, on BOTH the containment block + and any ten-field finding carrying an `entry_point`. + 3. `entry_point` is REQUIRED on an injection finding, with `evidence` + required alongside it — not optional as revision 3 had them. STEP 5's + cross-cutting clause in #117 is what produces those, so #119 will receive + them on ordinary dimension findings, not only in the containment block. + 4. `finding_id` hashes seven inputs rather than five. #119 uses it only as a + sort tie-break, so this costs nothing. + 5. the finding-field count is ten, not nine. Corrected above. + NOTHING IS RENAMED. This plan's OPEN correctly predicted that a rename would + cost STEPs 4, 5, 6, 10 and 12; no rename happened. What it did NOT predict is + an ADDITION, which is what actually broke it — see OPEN, which now registers + additions as well as renames and removals. + Severity is imported from review.SEVERITY_ORDER, not redefined, and STEP 4 no + longer subscripts it bare — see its ordering rule. + + #116's pre-flight record is enumerated but not built. origin/feat/review-agent-preflight + carries only its plan file. Its record has seven top-level keys — pr, + closing_issue, diff, checks, required_gate, nearest_rules, skips — and its own + OPEN leaves the schema version undecided. So #119 cannot depend on that record + today; the stage manifest in STEP 5 is what stands in for it. + + #118 is not started. Adjudication is the stage that would normally sit between + #117 and #119. Its absence is why STEP 7 takes report envelopes on stdin and is + agnostic about which stage produced them. + + launchpad/plans/ is the established path, and this plan uses it. AGENTS.md §3 + puts every cohort file under launchpad/ and #116's plan landed there. The + skill's default docs/plans/ is upstream's tree and is not used; docs/plans/ + does not exist in this checkout. New workflow files go in .github/workflows/ + because GitHub requires it, and §3 requires the `launchpad-*.yml` prefix. + + No verify gate is installed in this checkout. .claude/settings.json and + .claude/settings.local.json are both absent, so every review skill is a manual + invocation and none fires on its own. + + Toolchain present: python3 3.12.3, gh 2.93.0. + + #122's corrections are honoured by not repeating the figure. This plan quotes + no AUROC range. Per #122's verification on #109, the 0.48–0.64 range is one + judge (JailJudge), one victim model (Llama-3.1-8B), two attacks (GCG and + GCG-R), and "despite high performance on standard validation sets" is not a + quotation from the paper. What is confirmed verbatim is that judges perform "on + average only slightly better than a random coin-flip" against 6,642 + human-verified labels. Nothing in #119 depends on either figure. + +STEP 1 Record what the review-lifecycle API actually does. [independent] + A throwaway pull request in this fork, and FIVE raw responses captured to + launchpad/review-agent/fixtures/review-lifecycle.json: POST a COMMENT + review; PUT a new body on its id; DELETE that id; attempt a dismissal; and + attempt a PUT on a review THIS TOKEN DID NOT WRITE. Each entry stores the + HTTP status, the response body, and the `gh` command that produced it. + THE FIFTH IS THE ONE THREE REVIEW PASSES COULD NOT SETTLE, and it is nearly + free here. STEP 2's author filter exists because a marked review written by + someone else must never become a PUT target, and the consequence of getting + that wrong — the agent publishes nothing on that pull request, permanently — + depends on GitHub refusing a cross-author PUT. No pass verified that, because + verifying it means writing to a public fork, which is exactly what this step + already does. This step runs under a HUMAN token, a different identity from + the workflow's, so a review already on the pull request under any other login + is a ready-made subject: attempt the PUT, record the status verbatim. + If GitHub instead ALLOWS it, that is a bigger finding than the one the filter + was written for — this agent would be able to overwrite other people's review + bodies — and it reopens STEP 2 rather than being worked around. Either + outcome is worth a single call. + TWO IDENTITY FACTS ARE RECORDED HERE TOO, for the same reason and at the same + price. STEP 2 compares each review's `user.login` against a supplied login, + so both the field and the credential's own answer are worth capturing: + the `user.login` of the review this step POSTs, from the POST response + already being saved — which fixes the shape of the field STEP 2 filters + on rather than assuming it. + what `GET /user` returns under the token in use, status and body verbatim. + STEP 2 rejects that call as the identity mechanism on the grounds that an + installation token cannot use it; this step holds a HUMAN token, so it + will succeed here, and recording it makes the contrast explicit instead + of leaving a reader to wonder why the plan refuses a call that plainly + works. The Actions answer cannot be obtained here at all and is not + claimed — see STEP 9, which runs where the real credential exists. + This is first because the whole strategy rests on PUT working and DELETE + not. If PUT turns out to be refused, the answer recorded above ("update in + place") is not implementable and the plan changes at STEP 2, not at STEP + 10 with nine steps built on it. + Run under a human `gh auth` token, which is NOT the workflow credential — + so this records what the endpoints do, and proves nothing about scopes. + The scope question is STEP 9's and is not claimed here. + TWO MORE THINGS ARE RECORDED HERE, because this is the only step that + touches the live API and both are otherwise never captured at all: + fixtures/reviews-listing.json — the raw `GET /pulls/{n}/reviews` + response, saved as the UNMERGED per-page bodies with their `Link` + headers, not as one merged array. STEP 2 and STEP 10 both assert + against a recorded listing and neither can produce one. + THE TWO-PAGE CASE IS SYNTHETIC BY NECESSITY, and the fixture says so in + its own header. Measured on this fork: PRs 86, 124 and 126 carry 1, 0 + and 1 reviews. Nothing here has thirty, and growing a second page on a + throwaway pull request would take thirty separate submissions. So the + fixture holds TWO artefacts, labelled differently and never conflated: + the genuinely RECORDED single-page response, which is the authority on + what a review object's fields actually are; and a CONSTRUCTED two-page + listing, built by replicating that recorded object with distinct `id`s + and a real `Link: rel="next"` header copied from a genuine paginated + response. An earlier revision said page one was "padded", which is the + same construction described in a word that invites hand-authoring — and + hand-authoring a listing is the defect the fixture exists to remove. + Calling it constructed is honest; calling it recorded is not. + whether an EDIT is observable to a human. After the PUT, capture the + pull request's timeline (`GET /issues/{n}/timeline`) and record + whether the edit appears in it at all. The whole strategy turns + "re-review on push" into a body edit, so if an edit produces no + timeline entry and no notification, every re-review after the first + is silent — the body is current and no reader is told. That is a + judgement call for #121, and it cannot be made without this + observation. Recording it costs one call; inferring it costs the + feature's entire value. + done when: the fixture exists and contains FIVE entries, each with a + non-empty `status`, `body` and `command`; the POST entry's response has + `state` "COMMENTED" and a numeric `id`; the PUT entry's status and the + DELETE entry's status are both recorded verbatim whatever they are; the + cross-author PUT entry records its status verbatim and the fixture states in + one line whether GitHub refused it, with STEP 2's author filter cited as what + depends on the answer; the POST entry's `user.login` is recorded, and the + `GET /user` status and body under this step's token are recorded beside a + one-line note that this is a human token and says nothing about the Actions + credential; + fixtures/reviews-listing.json exists, holds the recorded single-page + response AND a constructed two-page listing whose page one holds 30 entries, + each artefact labelled recorded or constructed in the file itself, and no + assertion anywhere cites the constructed one as evidence about GitHub; + the timeline response after the PUT is saved + and the fixture states in one line whether the edit is visible in it; and + the file states which of the two strategies in this plan's header the + recorded statuses support. + +STEP 2 launchpad/review-agent/publish.py — the single-review lifecycle. [needs 1] + Three functions over an already-rendered body string. No rendering here, + no findings, no contract. + MARKER — a hidden HTML comment, ``, + emitted as the FIRST line of every body. Identification is by marker + AND by author, and neither alone is sufficient — the marker says "this + is our kind of review", the author says "this one is ours". + Not by a HARDCODED author login: the workflow token posts as + `github-actions[bot]` today and #110 commits to revisiting the identity + later, so a literal login in the source would break at exactly the + moment the credential moves. The login is resolved at runtime instead, + per find_existing below, which keeps that portability while closing the + hole the marker alone leaves open. An earlier revision of this bullet + read "identification is by marker, NOT by author login" and drew the + wrong conclusion from the right premise: what must not be hardcoded is + the name, not the comparison. + publish.py OWNS the constant and publish_render RECEIVES it as + render_body's first argument. ONE DIRECTION IS BANNED AND THE OTHER IS + NORMAL: publish_render must import nothing from publish, and publish.py + imports publish_render at module level in the ordinary way. + A cycle needs both edges. STEP 7 puts `main` in publish.py, so if + publish_render also imported publish for the marker, `python3 + publish.py` would die with "cannot import name ... from partially + initialized module" — which is why the marker is passed in rather than + imported. With that edge gone there is no cycle left to prevent, and an + earlier revision nevertheless banned BOTH directions, which forbade the + normal import and left `main`'s import site unstated. Assert the edge + that matters, not both. Passing the marker in also makes STEP 4 testable + on its own with a sentinel value. + find_existing(pr, repo, login) -> (int | None, int) — lists reviews and + returns (the id of the NEWEST review that carries MARKER **and was + written by `login`**, the COUNT of marked reviews under any OTHER + author). Both identity conditions on the first element, and the + author one is checked FIRST. The count is not a log line — + `post_or_update` below reads it to decide whether a POST is even safe. + THE MARKER IS ATTACKER-WRITABLE AND THE AUTHOR FIELD IS NOT. A review + body is not agent-controlled territory: any GitHub user with read access + can submit a COMMENT review on a pull request in this public fork and + write its body, marker and all. Matching on the marker alone therefore + lets an outside party choose which review object this agent tries to + update. The listing already carries what a filter needs — a review + object's fields are `_links, author_association, body, commit_id, + html_url, id, node_id, pull_request_url, state, submitted_at, user` — + so the fix costs one comparison against data already fetched. + Without it, the failure is a denial of publication that an outsider can + trigger deliberately: they post a marked review, find_existing returns + its id, the PUT is refused because the token does not own it, and the + hard-failure rule below means the agent then publishes NOTHING on that + pull request, permanently, presenting as an HTTP error rather than as an + attack. #119's own criterion calls silence "indistinguishable from a + crashed agent"; marker-only matching puts that silence on demand. + THE IDENTITY IS SUPPLIED AND VERIFIED, NOT DISCOVERED — find_existing + takes the expected login as a PARAMETER and compares it against each + review's `user.login`. The `--as ` flag that supplies it, and its + `github-actions[bot]` default, belong to STEP 7: a flag needs the + argument parsing that `main` provides, and `main` does not exist until + then, so this step takes an argument and STEP 7 wires the flag to it. + A default VALUE on a flag is not a hardcoded comparison: #110 commits to + revisiting the credential, and moving it is then one flag rather than a + code change, which keeps the portability the marker-over-author choice + was made for. Marker for "this is our kind of review", author for "this + one is ours". + NOT `GET /user`, and an earlier revision of this bullet said exactly + that. `GITHUB_TOKEN` is an installation credential; GitHub's REST + documentation for "Get the authenticated user" lists OAuth app tokens + and personal access tokens (classic) as what it supports, and + installation access tokens are not among them. **This plan does not + claim the 403 as proved** — an installation token exists only inside a + real Actions run, so it cannot be tested from a worktree, and STEP 1 + records it rather than asserting it. + The change does not depend on settling that. If `GET /user` is refused + under Actions, resolving the identity that way fails on EVERY pull + request under the credential it was written for — worse than the attack + the filter prevents, and invisible locally because STEPs 1 and 3 run + under a human token where the call succeeds. If it is not refused, a + runtime call is still a needless failure mode for a value that is known + before the run starts. Supply-and-verify is correct either way, which is + why it is not waiting on the measurement. + The default is evidence-based rather than assumed: every bot comment in + this fork is authored by `github-actions[bot]` — 153 of them across the + paginated issue-comments listing, with no other bot login present. + AN UNRESOLVED IDENTITY ABORTS, and never degrades to matching on the + marker alone. If no login is configured and no default applies, + find_existing raises and publish.py exits non-zero. Falling back to + marker-only would silently restore the vector the filter exists to + close, and a loud failure to publish is strictly better than a quiet + invitation: one is visible in a job log, the other is available to + anyone who can open a pull request. + NEWEST AMONG OUR OWN, not oldest, and the two rules are not in tension + once the filter exists. A submitted COMMENT review cannot be deleted, so + once two of the agent's own markers exist neither can be retired and one + stays stale forever. The one a human reaching the bottom of the timeline + reads LAST is the newest, so that is the one that must carry the current + body. An earlier revision changed oldest to newest WITHOUT adding the + author filter, which is the one combination worse than either concern + alone: taking the oldest at least required an attacker to plant before + the agent's first run, and taking the newest removes even that timing + requirement, so a review planted at any moment wins every run + thereafter. Filter, then take the newest. + It PAGINATES — `gh api --paginate` — and asserts it reached the end + of the listing. GET /pulls/{n}/reviews returns 30 per page, and a + pull request accumulates reviews from every human as well as this + agent. An unpaginated read on a busy pull request returns None for a + marker sitting on page two, and None means POST, which is a second + review on exactly the pull requests most likely to have a reader — + the failure this issue exists to prevent, arriving only under load. + A listing that cannot be fully read is an ERROR, not an absence: if + pagination fails part way, find_existing raises rather than returning + None, because a partial listing is indistinguishable from an empty + one at the call site. + THE PAGINATION SEAM, because otherwise the control for it cannot fail. + `gh` merges pages itself: measured on this fork, `gh api + "repos/launchpad-26/buzz/pulls?state=all&per_page=2" --paginate` over + five requests returns ONE flat JSON array that `json.load` accepts, + length 9. So a fixture holding gh's merged output is + indistinguishable from a single page of 31 entries, and an assertion + fed that fixture passes whether or not `--paginate` was ever sent. + find_existing therefore takes an injected `list_reviews(argv) -> + list[dict]` transport, defaulting to the real gh call. The stub in + STEP 10 receives the actual argv and serves STEP 1's page two ONLY + when `--paginate` is present in it. That is what makes "drop + --paginate" a mutation that fails rather than one that passes. + post_or_update(pr, repo, body, login) -> (id, "created"|"updated", + author_login) — PUT when find_existing's first element is an id, POST + otherwise, UNLESS THE FOREIGN COUNT IS NONZERO, in which case + post_or_update raises rather than posting — see below. The event is + the literal "COMMENT", hardcoded at the single call site, and the + function takes no event parameter at all. A parameter that could hold + "APPROVE" is a parameter that one day will. `author_login` is + `user.login` read off the POST/PUT response body — the only place the + live identity actually exists, and STEP 9's identity control needs it. + A FAILED PUT IS A HARD FAILURE AND NEVER FALLS BACK TO POST. If the + PUT returns any non-2xx — 403, 404, 410 — post_or_update raises with + the review id and the status in the message, and the workflow fails + loudly with no review updated. A fallback POST would create the second + review this entire issue exists to prevent, and it would do so on + exactly the run where something is already wrong. + A NONZERO FOREIGN COUNT ALSO REFUSES TO POST, and this is the fix for + a gap an earlier revision left open. If `--as`'s configured value ever + changes — #110 commits to revisiting the credential — a review this + agent posted under the OLD login is now authored by someone + `find_existing` calls foreign under the NEW login: it is excluded as a + candidate, the first element comes back None, and nothing before this + revision distinguished that from "no marker has ever been posted + here". post_or_update POSTed a second review in that case, silently + creating exactly the duplicate #119 exists to prevent while reading as + a normal first publish. It is also indistinguishable, from the data + alone, from an outside party planting one throwaway marked review to + see what happens — and the plan does not need to tell those two cases + apart to know that POSTing blind in EITHER case is wrong. So: first + element None and foreign count zero POSTs normally; first element None + and foreign count nonzero RAISES, naming the count, and posts nothing. + A human looks at the PR's existing reviews before this can proceed — + retiring a stale review by hand, or confirming a planted decoy is + exactly that — rather than the tool guessing which case it is. + Immediately before a POST, find_existing is called a second time. Two + pushes seconds apart produce two workflow runs, and a check performed at + the start of a run is stale by the time the run posts. The foreign-count + refusal above applies to this second call too, not only the first. + done when: `python3 -c "import publish"` run from launchpad/review-agent/ + succeeds — not `py_compile`, which compiles without resolving imports and + so would pass on the circular import the MARKER rule above exists to + prevent; `grep -nE "APPROVE|REQUEST_CHANGES" publish.py` returns nothing; + `grep -c "def post_or_update" publish.py` is 1 and its signature has no + event parameter; MARKER is a module-level constant in publish.py, asserted by + reading `publish.MARKER` after import — the matching assertion, that + publish_render imports nothing from publish, belongs to STEP 4's done-when + and NOT here, because publish_render.py does not exist until STEP 4 and this + step does not depend on it, so a grep against that file here would pass by + the file's absence rather than by its contents; + against STEP 1's recorded listing with no marker find_existing returns + (None, 0); with one marker under the agent's own login returns (that id, + 0), and with two of its own markers returns (the NEWER id, 0) AND prints + the duplicate count; a listing whose ONLY marked review carries a + different `user.login` returns (None, 1) — the load-bearing case, since + returning that id as a PUT target is the denial-of-publication vector; + a listing carrying a foreign marked review submitted AFTER the agent's own + returns (the agent's own id, 1), not the newer foreign one as the first + element, so the author filter is proven to run before the newest-wins rule + rather than after it; the SAME listing passed a DIFFERENT expected login + returns (None, 1) with the roles of "own" and "foreign" swapped, so the + parameter is honoured rather than ignored in favour of a constant baked + into the comparison; find_existing called with an empty or missing login + RAISES rather than matching anything, so an unresolved identity cannot + degrade to marker-only; `grep -n "GET /user\|api user" publish.py` returns + nothing, so no runtime identity call exists to fail under an installation + token; with the injected transport serving page two only on `--paginate`, + find_existing returns the marked id from page two, and the same transport + with `--paginate` absent from the argv returns (None, 0) — so the two cases + differ and the assertion can fail; `--paginate` is present in the default + transport's argv, asserted on the argv itself; a recorded listing whose + second page returns an error causes a raise, not a (None, 0); a stubbed PUT + returning 403 makes post_or_update raise with the id and status in the + message and issue NO POST, asserted on the recorded transport calls; + AGAINST A LISTING WITH ONLY A FOREIGN MARKER AND NO MATCH UNDER THE + CONFIGURED LOGIN, post_or_update RAISES naming the foreign count and + issues no POST and no PUT, asserted on the recorded transport calls — the + control for the fix above, since a POST here is exactly the silent + duplicate that fix exists to prevent; the SAME listing with the foreign + count reset to zero (a fresh PR, nothing posted yet) DOES POST, so the + refusal is proven to key on the count and not merely on the listing being + non-empty; and a stubbed successful POST/PUT response carrying + `user.login: "some-login"` makes post_or_update's third return element + equal `"some-login"`, asserted by value rather than by presence, so a + function that returns the CONFIGURED login instead of reading the + response would still pass a presence-only check. + +STEP 3 End to end on a throwaway pull request, with a stub body. [needs 2] <- RUNS HERE + The lifecycle is demonstrable before a single finding is rendered. Post a + stub body naming the head SHA; push an empty commit to the same branch; + run publish.py again; read the reviews back. + This is behavioural evidence only. It runs under a human token with far + wider scope than the workflow credential, so it demonstrates that the + review does not duplicate — it does NOT demonstrate anything about + pull-requests write or contents read. That is STEP 9's, and the two must + not be conflated in the PR body. + done when: after the second run `gh api repos/launchpad-26/buzz/pulls//reviews + --jq '[.[] | select(.body | startswith(""))] | length'` + equals 1; the surviving review's body names the SECOND commit's SHA and + not the first; its `id` is unchanged between the two runs; its `state` is + "COMMENTED"; and the raw before/after listings are saved for the PR body. + +STEP 4 launchpad/review-agent/publish_render.py — the findings body. [independent] + A pure function + `render_body(marker, reports, stages, containment, head_sha, merge_base_sha) -> str`. + No network, no subprocess, no posting. `marker` is passed in rather than + imported, per STEP 2 — this module must not import publish.py. + `containment` is a SEPARATE argument, not an entry in `reports`, and #117's + settled contract is now what says so rather than this plan's inference: + containment findings "do NOT enter the `findings` array of any dimension + report" and travel as a top-level sibling key. Its shape is + `{findings: [{severity, kind, entry_point, evidence}], states: + {entry_point: state}}` — the four raw `contain.Finding` fields, unrenamed, + and `fetch.Surface.state` for all seven entry points. render_body + reconstructs `contain.Finding` objects from that block and passes them, + with `states`, straight into `review.render_review`, which fences and + escapes each excerpt itself at review.py:71-73. Evidence is handed over + RAW and exactly once: #117 now guarantees raw, and pre-escaping it here + would escape it twice — a `~` publishing as `~~~~`, per #117's own + reasoning — so the excerpt would stop matching what the author wrote. + The block carries NO `unreadable` key, and render_body passes no + `unreadable=` argument — that keyword was removed in #120's `e072fba55` + and passing it now raises TypeError. See ALREADY TRUE. Re-adding it here + would be a second source of truth for a fact `states` already carries: + the two could disagree, and `render_review` would silently ignore the one + this plan sent. Derive, never pass. + The consequence is that **`states` is now load-bearing for the + "Incomplete" banner**, not merely for the "Fetched and empty" line. If the + `containment` block omits `states`, or populates it for only the surfaces + that succeeded, every unreadable surface reads as absent-from-the-map + rather than unreadable, and the banner never renders. That is #120's + original defect — a banner with no producer — relocated one stage up. STEP + 5's trigger for a MISSING block does not cover a PRESENT block with a thin + `states` map, so the control must assert the map names all seven entry + points, not merely that it exists. + This block NOW EXISTS upstream, and that is the change that unblocked this + step. #117's contract settled it normatively — a top-level `containment` + key, present on EVERY run, carrying an empty `findings` array and a full + seven-key `states` map when nothing was found, "never a missing key". So a + missing block is reserved for the case where #117 genuinely could not + produce one, which is exactly the reading the incomplete rule below needs. + The dependency OPEN recorded is discharged, not assumed. + A MISSING `containment` block is INCOMPLETE, never "no containment + findings". CONTAINMENT.md's own reasoning is that a detected attempt which + does not reach the review is worse than one never detected, because it + reads as a clean review; an absent block is exactly that case and must + never render as the "No containment findings" line, which is a positive + claim. STEP 5 carries this as a trigger. + Ordering: findings from every report are merged into one list and sorted + by `review.SEVERITY_ORDER.get(finding["severity"], 9)`, ties broken by + (dimension, file or "", line or 0, finding_id) so the order is total and + the body is byte-identical for identical input. SEVERITY_ORDER is + IMPORTED from review.py; a second copy of a four-value ladder drifts. + `.get` WITH A DEFAULT, never a bare subscript, and review.py:62 already + does exactly this — `SEVERITY_ORDER.get(f.severity, 9)`. One finding + carrying a severity outside the ladder ("Info", or a lowercase "blocker" + from a model-authored report) raises KeyError inside the sort key, and a + KeyError here means publish.py exits non-zero having posted NOTHING. That + is total silence on a pull request that had findings — the failure #119's + clean-case criterion exists to prevent, arriving through the sort. #117's + validator refuses an out-of-ladder severity upstream, but STEP 7 is + deliberately agnostic about which stage produced the reports, so #119 must + not depend on that refusal. An unrecognised severity sorts last, renders + under the "malformed finding" heading below, AND triggers the incomplete + banner: the review cannot claim to be ordered by a ladder it could not + read. + "Most severe first survives an update" is a property of construction, not + of maintenance: the body is rebuilt from scratch on every run and nothing + is ever appended to an existing one, so ordering cannot degrade across + pushes the way an append-only comment would. + BOTH SHAs ARE RENDERED, in a header line above everything else: the head + SHA the review read and the merge base it was diffed against. An earlier + revision threaded `merge_base_sha` through this signature and through STEP + 7's stdin document and then never rendered, compared or tested it — a value + carried the whole length of the interface and dropped at the end, which no + type checker sees and no done-when here caught. It was found by an + independent review pass counting its occurrences. + Rendering it is the right resolution rather than removing it, because + #119's criterion is that "a review is valid only for the commit it read" and + the commit it read is a PAIR: findings are anchored to new-side line numbers + in the merge-base diff, per #117, so a reader given only the head SHA cannot + reconstruct which diff a `path:line` refers to. A force-push that rewrites + the base makes the same head SHA mean a different diff, and the merge base + is what distinguishes them. + Anchoring follows #117's three rules exactly and renders each accordingly + — `anchor: line` as `path:line`, `anchor: file` as `path`, `anchor: pr` as + `(pull request)`. A finding whose anchor and fields disagree is rendered + under an explicit "malformed finding" heading with its raw record, not + dropped: a finding silently discarded by the publisher is a finding the + reviewer believes it reported. + THE MALFORMED RECORD IS THE MOST UNTRUSTED THING THIS RENDERER PRINTS, and + it gets the same treatment as evidence: serialised, passed through + `contain.escape`, and wrapped in a `review.fence_for`-sized fence. It is + the path a record takes precisely BECAUSE its fields did not match the + contract, so it is the last place to assume any field is well-formed — an + attacker-shaped record reaches it by construction, and a `defect` value + carrying a long backtick run would otherwise close a fixed fence and spill + the rest of the review out of the code block. The fence is sized off the + serialised record, not off any single field. + Every finding renders `defect` and `failure` as separate lines, because + #119's criterion is "the concrete failure it allows" and a defect with no + stated consequence is what lets an unfalsifiable finding through. + A DIMENSION FINDING CAN ALSO CARRY RAW ATTACKER TEXT, and this is the part + the containment block does not cover. #117's settled contract makes + `entry_point` REQUIRED on any finding whose defect is an injection attempt, + with `evidence` required alongside it and RAW — and #117's STEP 5 puts that + clause in all three dimensions, for the paraphrase cases detect.py misses. + So `reports[].findings[].evidence` will hold verbatim author-supplied text, + arriving through the ordinary findings path, not through the block that + `review.render_review` handles. + Two rules follow, and both are load-bearing: + RENDER IT. A finding with an `entry_point` renders that entry point and + its evidence. Dropping the excerpt is the detected-then-dropped case + CONTAINMENT.md calls worse than never detecting, and it would land on + exactly the 14 attack classes #117 was handed because detection misses + them. + FENCE AND ESCAPE IT WITH THE FUNCTIONS THAT ALREADY EXIST. + `review.fence_for(evidence)` sizes the fence longer than the longest + backtick run in the excerpt, and `contain.escape(evidence)` + neutralises the envelope delimiter. Both are IMPORTED, never + reimplemented — review.py:21-29 is explicit that attacker text + containing ``` "would therefore break out of a fixed three-backtick + fence and corrupt every following section of the review", and a fixed + fence here would do it in the one place a human is reading. + `defect` and `failure` are NOT passed through `contain.escape`, and an + earlier revision of this step said they were, on the strength of a claim + that measurement falsifies. That revision asserted "the transform only + touches the escape character and the envelope token, so ordinary prose + survives it unchanged". `contain.ESC` is a single TILDE. Measured: + '~/.claude/settings.json is read at startup' + -> '~~/.claude/settings.json is read at startup' + 'the ~ operator inverts bits' -> 'the ~~ operator inverts bits' + `~~text~~` is STRIKETHROUGH in GitHub-flavoured markdown, so a defect line + naming two home-relative paths publishes with the passage between them + struck through, and a single mention publishes a visible doubled tilde. + This fork's own subject matter is dotfile paths — this plan cites + `~/.claude/skills/plan-issue/check-plan.sh` — so the corruption would land + on the most likely findings, in the one surface a human reads, and no + assertion in STEP 10 looks at `defect` rendering at all. + `contain.escape` is a boundary guard for text re-entering a delimited + envelope. It is not a markdown sanitiser and this body is not fed back to a + model in Phase 1. What IS worth neutralising in model-authored prose is the + envelope token itself, so `defect` and `failure` have `contain.TOKEN` + replaced with `contain.ESC_TOKEN` and nothing else touched. That is the + narrow half of the escape, and it leaves a tilde alone. + Evidence is a different case and IS shown escaped, because `render_review` + escapes containment evidence at review.py:72 and a dimension finding's + excerpt must not render by a different rule than a containment finding's. + Inside a `fence_for` fence `~~` is literal rather than strikethrough, so + the cost is a visible doubled tilde and not a corrupted body — but it does + mean the excerpt is not byte-for-byte what the author wrote, so STEP 12 + says so rather than letting a reader assume otherwise. + done when: given three report envelopes carrying findings of mixed + severity, the output lists every Blocker before every High, every High + before every Medium, and every Medium before every Low; two calls with + the same input produce byte-identical strings; the body contains BOTH the + head SHA and the merge-base SHA, and a call with two different SHA values + produces a body containing both of them rather than one twice — the second + half is the assertion, because rendering head_sha into both slots satisfies + "contains the head SHA" on its own; a finding with anchor "pr" + and a non-null `file` appears under "malformed finding" and is still + present in the output; a finding whose `severity` is "Info" appears under + "malformed finding", sorts last and does NOT raise — its banner is STEP 5's + assertion, not this step's, because STEP 4 is [independent] and the banner + does not exist until STEP 5 builds it; the body's first line is the `marker` + argument, asserted + with a sentinel value rather than with publish.py's constant, so this step + needs no import from publish.py; `grep -n "^from publish import\|^import + publish$" publish_render.py` returns nothing; `grep -n "SEVERITY_ORDER *=" + publish_render.py` returns nothing; a containment block carrying one + Blocker finding produces a body containing that finding's `kind`, its + `entry_point`, and its evidence in ESCAPED form — asserted by comparing + against `contain.escape(evidence)`, not against the raw string; a + DIMENSION finding carrying an `entry_point` and an evidence string + containing a four-backtick run renders that evidence escaped and inside a + fence of at least five backticks, and the text following it is still + outside any code block — the assertion is on the fence length and on what + comes after, because a body that merely contains the excerpt is what a + broken fence also produces; a malformed record whose `defect` carries a + four-backtick run is likewise fenced longer than that run and escaped, with + the text after it still outside any code block — same assertion, applied to + the other untrusted path; `python3 -c "import pathlib,sys; + sys.exit(0 if chr(96)*3 not in + pathlib.Path('publish_render.py').read_text() else 1)"` exits 0, which bans + a FIXED fence rather than fencing itself — `review.fence_for` builds its + fence from `chr(96)` at runtime and so satisfies this check, and it is the + only permitted source of a fence in the module; and a run with + `containment=None` + does NOT contain the string "No containment findings" — the positive claim + is what this step can assert without the banner, and the banner itself is + STEP 5's. + WHY TWO CLAUSES MOVED OUT OF THIS DONE-WHEN. An earlier revision asserted + the incomplete banner here, in a step tagged [independent], while STEP 5 + [needs 4] is what builds it. That is a done-when observing something its own + tag cannot produce: STEP 4 could not close until STEP 5 existed, STEP 5 + could not start until STEP 4 closed, and the way out of a deadlock like that + is always to declare the step done on the clauses that do pass. #117's + second review pass found this same defect in four of its steps. Both clauses + now live in STEP 5's done-when, which is the step whose tag supports them. + +STEP 5 The incomplete case — an unfinished stage is never rendered as done. [needs 4] + A `stages` manifest of {name, status, reason} entries accompanies the + reports and names EVERY stage the review depended on — #116's pre-flight, + #117's three dimensions by slug, and #118's adjudication. Not only the + stages that emit no envelope of their own, which is what an earlier revision + said and which contradicted this step's own condition (7) and STEP 6's + done-when three lines apart: both require dimension names to be IN the + manifest, and the definition excluded them. + The dimensions are in it because A REPORT CANNOT TESTIFY TO ITS OWN + ABSENCE. Every other condition here reads a report that arrived; (7) is the + only one that catches a dimension crashing so completely that #117 emits no + envelope for it at all, and it has nothing to compare `reports` against + unless the manifest says which dimensions were expected. Built to the old + definition, the manifest held two entries, neither a dimension, so (7) could + never fire: a three-dimension run that produced two reports rendered as + COMPLETE. That is the partial review reading as a complete one that #119's + done-criteria forbid by name, reached through the one condition written to + prevent it. Found by an independent review pass, not by this plan's author. + A review is INCOMPLETE when any of + these TEN conditions holds. The count is stated because an earlier revision + listed six, called them seven in its own done-when and six again in STEP + 12, and a builder told to test "each of the seven" over a six-item list + invents one or drops one: + (1) a stage in the manifest has status other than "complete" + (2) a report has `status: failed` + (3) a report's `completion_marker` is absent, or is not the last key + (4) a report's marker names a dimension other than that report's own + `dimension` field + (5) a report's marker nonce differs from the merged document's `nonce`, + or two reports' markers disagree with each other + (6) `findings_count` does not equal `len(findings)` + (7) a dimension named by the manifest produced no report at all + (8) a report has `status: complete` and no `outcome` — moved here from + STEP 6, which is where it was stated and where STEP 5 could not see + it + (9) the `containment` block is absent or unparseable + (10) `set(containment.states)` does not EQUAL + `set(contain.ENTRY_POINTS)` — a set comparison against the imported + tuple, never a count. "All seven entry points" is satisfied by + counting to seven, and the realistic bug is six real keys plus one + typo, which counts to seven and passes. The banner names the + difference in both directions. + On (5), and on what the nonce does and does not prove. #117's marker is + BUZZ-DIMENSION-COMPLETE:{dimension}:{nonce}, and #117 now emits that run + nonce as a top-level `nonce` key on the merged document. So the primary + check is direct: every report's marker nonce must equal the document's + `nonce`. Marker-to-marker agreement stays as a secondary, because a run + whose reports disagree with each other is broken whatever the document says. + ONE NONCE PER RUN, NOT PER DIMENSION. All reports embed the same value. + Two earlier revisions of this step got this wrong in opposite directions and + both are recorded so neither returns. The first said the marker is checked + for "the wrong dimension or nonce" when the nonce reached no downstream + stage at all, making the check unimplementable from this input — that gap + was raised against #117 and #117 closed it by adding the key. The second + replaced it with marker-to-marker agreement alone and described "another + dimension's nonce" as a test input. There is no such thing: one value per + run means a fixture built on that phrasing exercises nothing, which is a + test that cannot fail dressed as thoroughness. + WHAT THE KEY DOES NOT DO, in #117's words rather than this plan's + paraphrase: it is "not an authentication token for the document, only for a + marker echoed out of author text". A pull-request author can type a fixed + marker string into their own diff; they cannot know the run nonce, so a + forged marker fails against the document. Nothing here authenticates the + document itself — a compromised runner emits a self-consistent document with + its own nonce and this stage cannot tell. That residual is real, it is + bounded, and it must not be described as making the review unforgeable. + (5) is no longer vacuous on a single report. An earlier revision noted that + agreement needs siblings, so a one-dimension run — or a three-dimension run + where two came back `status: failed` — had no nonce checking at all, exactly + when the run was already degraded. The document nonce removes that: one + report is enough to check, because the comparison is against the document + rather than against a sibling. + On (10), because a present-but-thin map is the dangerous shape. STEP 4 + derives nothing and `review.render_review` derives its "Incomplete" banner + from `states` against `UNREADABLE_STATES`. A map populated only for the + surfaces that succeeded makes every unreadable surface read as + absent-from-the-map rather than unreadable, and the banner never renders — + a review over three unreadable surfaces publishing as complete. Counting + the keys is the check; asserting the key exists is not. + Incomplete renders as a banner at the TOP of the body, above the findings, + naming every stage that did not finish and its reason. It is at the top + because a reader who stops after the first finding must still have seen + it, and #119's criterion is that the comment "never publishes a partial + review that reads as a complete one". + The default is incomplete. An input that cannot be classified — a report + that will not parse, a manifest entry with no status — is incomplete, not + complete. Absence of a failure signal is not evidence of success. + done when: for each of the TEN conditions above, given an input exhibiting + ONLY that condition, the body contains the incomplete banner and names the + offending stage, dimension or entry point — ten inputs, ten assertions, + counted against the numbered list above rather than against the word + "every"; for an input exhibiting none of them the banner is absent, which + is the negative control that stops a banner that always fires from passing + all ten; a SINGLE-report input whose marker nonce differs from the document's + `nonce` is incomplete while the same input with a matching nonce is not — + one report, because the point of the document key is that agreement between + siblings is no longer what the check rests on; a two-report input whose + markers disagree with each other is also incomplete; and no fixture anywhere + in this step uses "another dimension's nonce", which is an input #117's + contract cannot produce — one nonce per run — so a test built on it would + pass against any implementation; a + `containment.states` map naming six of the seven entry points is incomplete, + AND so is one naming seven keys of which one is not in + `contain.ENTRY_POINTS` — two inputs, because the second is the one a count + lets through; a manifest naming three dimensions against `reports` carrying + two is incomplete and the banner names the missing slug; and an unparseable + report is incomplete rather than raising. + TWO CLAUSES INHERITED FROM STEP 4, which asserted them while tagged + [independent] and could not produce them: a finding whose `severity` is + outside the ladder triggers the banner, and a `containment=None` input + triggers the banner. Both are behavioural assertions about this step's own + code, so they belong here. + +STEP 6 The clean case — no findings still posts, and says so. [needs 4] + Every stage complete and every report `outcome: clean` renders an explicit + body: the SHA reviewed, the dimensions that ran, and a sentence saying no + confirmed findings were produced. #119's reasoning is that silence is + indistinguishable from a crashed agent, so this path posts on exactly the + same code path as the findings path — there is no early return that skips + publication. + `outcome: clean` and an empty findings array are not the same input. + A report with `status: complete` and no `outcome` is incomplete — it is + condition (8) of STEP 5's list, which is where it now lives. It was stated + only here in an earlier revision, so STEP 5 enumerated six conditions while + two of its own done-whens counted seven, and the seventh was this sentence. + THE CLEAN PATH MUST BE PROVEN TO POST, not merely to render, and that proof + lives in STEP 10 (viii) because the decision to post is in `publish.py`'s + `main` and not in the renderer at all. An earlier revision offered `grep -n + "return None" publish_render.py` as evidence of "no early return on the clean + path". That check cannot fail: it greps the PURE RENDERER, whose contract is + `-> str`, so no correct implementation ever returns None from it, and it + passes for every implementation. Meanwhile the actual risk — `if not findings + and not incomplete: return 0` in `main`, which looks like a reasonable + optimisation against the edit-event noise OPEN itself raises — lives in a file + that grep never reads. The grep is withdrawn. + done when: an all-clean input produces a body containing the head SHA, the + name of every dimension in the manifest, and the no-findings sentence; that + body still carries the marker as its first line; and an input with `status: + complete` and no `outcome` produces the STEP 5 banner rather than the clean + sentence. That a clean run POSTS is asserted in STEP 10 (viii), against + `main` and through the recorded transport, and is not claimed here. + +STEP 7 Wire the renderer into the CLI. [needs 2, 4] + `publish.py` gains a `main` reading one JSON document on stdin — + `{pr, head_sha, merge_base_sha, stages, reports, containment, nonce}`, where + each entry of `reports` is a #117 envelope verbatim and `containment` is the + block specified in STEP 4 — rendering it through publish_render, and + calling post_or_update. A `--dry-run` prints the body and posts nothing. + The document WRAPS #117's envelopes; it does not restate or rename a + single field inside them, and `containment` is a sibling key precisely so + that it does not have to. #117's contract quotes this shape back — "`reports` + and `containment` mean here exactly what they mean there" — so the keys + are fixed by agreement between two plans and are not #119's to extend. + `nonce` IS THE SEVENTH KEY AND IT ARRIVES FROM #117, not from this plan. + #117's merged document now carries the run nonce at the top level, and its + change list names this step: "#119 should add `nonce` to its STEP 7 stdin + document as a sixth key and check each marker against it." Six of #117's + keys plus this plan's own `stages` makes seven here. STEP 5 condition (5) is + what consumes it, and until this key existed that condition was + unimplementable from this input — which is why it was raised upstream rather + than worked around locally. + `--as ` IS WIRED HERE, defaulting to `github-actions[bot]`, and passed + into post_or_update as the `login` parameter STEP 2 defined, which forwards + it into find_existing internally. The flag lives in this step because + argument parsing lives in `main`; the comparison lives in STEP 2 because + that is where the listing is read. STEP 9's identity control is what + proves the default is still the truth under the live credential — this + step only has to pass the value through, and to fail non-zero rather than + defaulting to nothing if the flag is given empty. + `repo` IS NOT ONE OF THEM, and it has to come from somewhere. `find_existing` + and `post_or_update` both take it, #117's merged document does not carry it, + and adding it to the stdin document would break the shape #117 documents. So + it comes from a `--repo owner/name` flag, defaulting to `GITHUB_REPOSITORY`, + and publish.py EXITS NON-ZERO when neither is present. Not a hardcoded + `launchpad-26/buzz`: that is wrong the day the fork is renamed and unusable + from any other checkout. Not a silent `None` either — that builds the path + `repos/None/pulls/...`, which 404s in a way that reads like a missing pull + request. STEP 10's controls inject a transport, so neither mistake would be + caught by any control in this plan; the guard has to be in the code. + done when: `python3 publish.py --dry-run --repo launchpad-26/buzz < + fixture.json` exits 0 and prints a body whose first line is the MARKER; + the same command with `--repo` omitted and `GITHUB_REPOSITORY` unset exits + non-zero and names the missing repository, posting nothing; the same + fixture with + `reports: []` and a manifest naming two dimensions exits 0 and prints the + incomplete banner; the same fixture with `containment` removed exits 0 and + prints the incomplete banner; malformed JSON on stdin exits non-zero and + posts nothing; and `--dry-run` produces no entry in the target PR's review + list. + +STEP 8 .github/workflows/launchpad-review-agent-publish.yml. [needs 7] + A separate file from #120's controls workflow, for the reason in ALREADY + TRUE: this job needs pull-requests write and that one must not have it. + Named `launchpad-*` per AGENTS.md §3. + `pull_request_target`, NOT `pull_request` — reversed from an earlier + revision, and the reversal is the point. #120's own comment block, which + an earlier draft of this file quoted directly, is correct for #120's OWN + workflow: a job with no write scope has nothing an attacker gains by + borrowing its token, so running PR-controlled code under it is safe. That + reasoning does not transfer here, because this job HOLDS + pull-requests: write. Under plain `pull_request`, a SAME-REPOSITORY pull + request — no fork required — gets that same write-capable token while + its own diff can modify `publish.py` or `check_publish_scope.py`, the + exact code this job then executes. A PR could rewrite either to submit + an APPROVE under the bot's identity: a hard violation of + `launchpad/AGENTS.md` rule 1, "Draft everything. Approve nothing." Copying + #120's comment into a job that carries a real privilege was the actual + defect; citing #120 was a symptom of it. + `pull_request_target` closes this the standard way, not by avoiding it: + it grants the base repository's token to every pull request, fork or not, + but `actions/checkout` defaults to the BASE ref when no `ref:` is given — + so the code that runs is always what is committed on `launchpad`, + regardless of what the PR changed. THE CHECKOUT STEP MUST NOT PASS + `ref: github.event.pull_request.head.sha` OR ANY EQUIVALENT: doing so + recreates the exact vulnerability this trigger change exists to close, + now with a base-repository token instead of a fork's. The untrusted PR + content this job ever reads is the seven-key document `main` takes on + stdin — DATA, fetched by #116/#117/#118 over the API exactly as + CONTAINMENT.md requires — never code checked out and executed, so this + trigger costs the job nothing #120's threat model warns against. + on: pull_request_target, types [opened, synchronize, reopened] — + `synchronize` is what makes "re-review on push" happen at all. + permissions: contents: read, pull-requests: write. Nothing else. Set at + the workflow level with no job-level override. + concurrency: group per pull request — the group string INTERPOLATES + `github.event.pull_request.number` (or `github.ref`), never a fixed + name — with cancel-in-progress: true. Two pushes in quick succession + otherwise race, and STEP 2's second find_existing is a backstop for + that race, not a substitute. A workflow-wide fixed group would be + worse than none: every pull request would then cancel every other + one's publish run, and the pull request that lost the race would + silently keep a review describing a commit it no longer has. + TWO JOBS, NOT A FIRST STEP THAT EXITS 0. An earlier revision's fork + guard was a shell step doing `exit 0` when + `github.event.pull_request.head.repo.full_name` differed from + `github.repository` — but exiting 0 marks only THAT STEP successful; + every later step in the same job still runs regardless. The + publisher and the scope probe would still execute on a cross-repo + pull request and fail, rather than performing the promised loud, + clean skip. A JOB-LEVEL `if:` is what actually skips every + subsequent step at once: a `guard` job computes and outputs whether + the PR is cross-repository; the `publish` job declares + `needs: guard` and `if: needs.guard.outputs.is-fork != 'true'`. The + whole `publish` job — the pipeline invocation and the scope control + both — is skipped, visibly, in the job list, rather than one green + step sitting above others that ran anyway. + CROSS-REPOSITORY PULL REQUESTS ARE STILL SKIPPED, FOR A DIFFERENT + REASON THAN BEFORE. `pull_request_target` closes the permission gap + that motivated the original guard — a fork PR could technically be + published on now — but no cross-repository publication path is + designed or built (see LEFT OUT), and this issue's own scope is + `launchpad-26/buzz` pull requests. The guard stays; only its + mechanism and its stated reason change. + A STEP THAT BUILDS THE SEVEN-KEY DOCUMENT AND PIPES IT INTO publish.py, + which an earlier revision of this file omitted entirely — every other + step (permissions, triggers, the guard, the scope control) could be + present and correct while the workflow still never published or + updated a single review, on any run, because nothing produced the + input `main` reads on stdin. #116, #117 and #118 are unmerged plans, + not code, as of this revision, so this step is specified against the + CLI shape each names for itself rather than built and run today: + preflight.py --pr ${{ github.event.pull_request.number }} | + run_dimensions.py --list secrets-and-access,claim-vs-evidence, + correctness-and-failure-modes | + run_adjudication.py | + publish.py --as github-actions[bot] + That chain is the recipe this step becomes once its three inputs + exist as code; it is named here so a re-check happens at the point + each upstream stage merges, per BUDGET, rather than this file being + silently correct on its own four corners while wiring nothing. + NO `--repo` FLAG IS NEEDED HERE. STEP 7 requires either `--repo` or + `GITHUB_REPOSITORY` and exits non-zero without both; Actions sets + `GITHUB_REPOSITORY` for every run, so the default covers this workflow + and the flag is for local invocation. Stated because a reader arriving + from STEP 7's hard failure will otherwise look for a flag that should + not be here — and hardcoding one into the workflow would reintroduce + exactly the wrong-the-day-the-fork-is-renamed problem STEP 7 rejects. + A STEP that runs `check_publish_scope.py`, in THIS workflow, in the + `publish` job. The live half of that control must execute under the + credential it claims to measure, and this is the only workflow whose + token is that credential. The script does not exist until STEP 9 + writes it; declaring the step here is what makes STEP 9's live half + reachable at all, and this file's done-when checks the step's + presence rather than the script's behaviour. + done when: `python3 -c "import yaml,sys; + d=yaml.safe_load(open('.github/workflows/launchpad-review-agent-publish.yml')); + print(d['permissions'])"` prints exactly {'contents': 'read', + 'pull-requests': 'write'}; `grep -c pull_request_target` on the file is 1, + not 0 — the earlier revision's absence check is inverted, because the + trigger this file now uses is the string this check used to forbid; + `grep -c '\bpull_request:' ` on the `on:` block is 0, so the old trigger + is fully replaced rather than left alongside the new one; the + `on.pull_request_target.types` list contains `synchronize`; a + `concurrency` key is present with `cancel-in-progress: true` AND its + `group` value contains `${{` and either `pull_request.number` or + `github.ref`, so a fixed group name fails this check rather than passing + it; no job in the file declares its own `permissions`; the `publish` job + declares `needs: [guard]` and an `if:` referencing + `needs.guard.outputs` — asserted on the parsed YAML's job dependency and + condition fields, not by grepping the word "guard" anywhere in the file; + no step anywhere in the file sets `ref:` on a checkout action, so the + default base-ref checkout is never overridden toward the PR head; the + `publish` job contains, in order, a step whose `run` contains a pipe + (`|`) and the literal substring `publish.py`, and a step invoking + `check_publish_scope.py` — both asserted on the parsed YAML rather than by + eye, because a live credential control (or a publish invocation) that no + workflow runs is a control that never executes. + +STEP 9 launchpad/review-agent/check_publish_scope.py — the credential control. [needs 8] + Three assertions, because each alone is weak. + STATIC — parse the workflow YAML and assert the permissions mapping + equals exactly {contents: read, pull-requests: write}, that no job + overrides it, that the trigger IS `pull_request_target` (never plain + `pull_request`, which would hand a same-repository PR's own diff the + write token that executes it — see STEP 8), and that no checkout + step in the file sets `ref:` to anything derived from + `pull_request.head` — the one override that would undo + `pull_request_target`'s safety by checking out the PR's own code + under the base repository's token. This runs anywhere, needs no + token, and catches a later widening OR a later checkout change in + review, before either reaches a live credential. + IDENTITY — inside the publish workflow, assert that the login STEP 2 was + configured with is the login the credential actually posts as. This is + the ONLY place in the plan where that can be checked: the configured + value is a flag default, the real identity exists only under the + workflow token, and STEP 1's measurement is of a human token and proves + nothing about this one. Read it from `post_or_update`'s third return + element — `author_login`, `user.login` off the POST/PUT response — and + FAIL when it differs from the configured value, naming both. + This is what catches a stale default when #110 moves the credential. + Without it the author filter degrades quietly rather than loudly: STEP + 2's foreign-count refusal turns a configured-login mismatch into a + RAISE rather than a silent extra POST, but "the workflow fails loudly + once, on the first push after the credential moves" is not the same + guarantee as "the mismatch is caught before it ever reaches a pull + request" — this control is what catches it in review, before #110's + change merges, rather than in a failed run afterward. A control that + only asserts the flag was READ, as STEP 2's offline assertions do, + cannot see this; it needs the live identity. + Outside the publish workflow it reports SKIP with a reason and never + PASS, on the same `GITHUB_WORKFLOW` guard as the live half below. + LIVE — with the workflow's own token, attempt one contents write: + create the ref `refs/heads/scope-probe-`, where the run id is + read from `os.environ["GITHUB_RUN_ID"]` and the control FAILS with a + stated reason when that variable is absent. NOT the literal string + `${{ github.run_id }}`: that is an Actions expression, interpolated by + the workflow YAML and never by Python, so transcribing it into this + module produces a ref name containing spaces and braces. GitHub then + answers on ref-name validity before it evaluates permissions, the + control fails for a reason that has nothing to do with scope, and it + does so in the step BUDGET already names as the most expensive to + iterate on. Assert + HTTP 403. Any other outcome is FAIL, including success, 404, and a + rate-limit error — a probe that treats "some error happened" as proof + of absent permission is fail-open, and would report PASS on a network + blip. If the probe unexpectedly SUCCEEDS the control deletes the ref + it made and still fails. + THE LIVE HALF MUST ASSERT WHICH WORKFLOW IT IS RUNNING IN, and this is the + difference between measuring the credential and measuring a different one. + It reads `GITHUB_WORKFLOW` and reports SKIP with a reason — never PASS — + unless it is the publish workflow. Absent that guard the control is + worse than useless: STEP 11 registers it in `run_controls.py`, and that + runner is invoked by #120's controls workflow, whose permissions block on + `feat/review-agent-untrusted-input` at `c64ff7958` reads + `{contents: read, issues: read, pull-requests: read}` under the comment + "Read-only, and no write scope of any kind". A ref-create under THAT token + returns 403 too — both tokens carry `contents: read` — so the control would + report PASS, and the 403 body would be pasted into the pull request as + evidence about the publish credential having measured a read-only one. The + criterion #119 states — "a control or documented check demonstrates the + absence of contents write" — would read as satisfied while nothing had tested + the token in question. A control that passes under the wrong token is not a + weak control; it is a false one. + The "no token outside Actions" SKIP does not cover this. Inside #120's + controls workflow there IS a workflow token — it is simply the wrong one — so + a guard keyed on the token's existence never fires. The guard has to be keyed + on WHICH workflow is running. + Outside Actions there is no workflow token at all, so the live half also + reports SKIP with a reason and never PASS — the rule run_controls.py already + enforces. This is the only step that can demonstrate #119's credential + criterion, it can only do so inside a real run of the PUBLISH workflow, and + STEP 3's local evidence does not substitute for it. + done when: the static half fails when handed a copy of the workflow with + `contents: write` and passes on the real one; the static half fails when + handed a copy of the workflow with `pull_request` in place of + `pull_request_target`, and passes on the real one — the check this + control exists to run BEFORE a same-repo PR's modified publish.py ever + executes under a write token; the static half fails when handed a copy + whose checkout step sets `ref: ${{ github.event.pull_request.head.sha }}` + and passes on the real one, which has no `ref:` override at all; the live half reports SKIP + with a stated reason when GITHUB_TOKEN is absent; the live half reports SKIP + with a stated reason when `GITHUB_WORKFLOW` names any workflow other than the + publish one — asserted by setting it to the controls workflow's name, which is + the exact wrong-token case, and the result must be SKIP and never PASS; a + recorded 404 response fed to the live half yields FAIL rather than PASS; and a + real Actions run of the PUBLISH workflow on this pull request shows the live + half reporting PASS with the 403 response body AND the value of + `GITHUB_WORKFLOW` pasted into the PR together, so a reader can see which + credential was measured rather than taking it on trust; the identity + assertion fails when handed a configured login that differs from the one on + the recorded response, naming both values; and that same real Actions run + reports the identity assertion PASS with the observed `user.login` pasted + into the PR, which is the measurement STEP 2's flag default rests on and the + only one taken under the credential that ships. + +STEP 10 launchpad/review-agent/check_publish_single.py — the behaviour controls. [needs 7] + Recorded inputs, no network, no model. TEN assertions covering #119's + done-criteria, and EVERY one of them carries a stated mutation that must + break it. A control never observed failing has not been shown to test + anything, and the temptation is to prove that only for the assertions + where it is easy — which leaves the load-bearing ones unproven. + Every recorded input here is STEP 1's, by path, and none is authored for + this step. An earlier revision cited "the recorded listing from STEP 1" + when STEP 1 produced only four operation responses, and "the recorded + two-page listing from STEP 2" when STEP 2 saved nothing — so both fixtures + would have been hand-written, and a hand-written listing tests this plan's + belief about gh rather than the code. STEP 1 now names both as + deliverables. + (i) the event published is COMMENT and the module contains no other + event string — the control asserts on the source, since a runtime + assertion cannot prove an absent branch. + Mutation: add the literal "APPROVE" to publish.py. + (ii) a second run over the same PR with a marker present issues a PUT + and no POST, over fixtures/reviews-listing.json from STEP 1 and an + injected transport that records calls instead of making them. + Mutation: make find_existing return None unconditionally. This is + the assertion the single-review invariant rests on, so it gets the + mutation proof first, not last. + (iii) find_existing paginates — the transport serves page two of STEP 1's + recorded listing ONLY when `--paginate` appears in the argv it is + handed, so the marked id on page two is reachable with the flag and + unreachable without it. The fixture is the UNMERGED page bodies: + `gh --paginate` merges pages into one array, measured, so a merged + fixture makes this assertion pass either way. + Mutation: drop `--paginate` from the listing call. With the + flag-aware transport the assertion then fails, which is the whole + point of building the seam. + (iv) severity order holds in the rendered body after an update whose + input has a NEW Blocker appended LAST in the reports array — the + Blocker still renders first. + Mutation: replace publish_render's sort key with identity. + (v) a clean input and an incomplete input both produce a body, and the + two bodies differ. + Mutation: remove the incomplete banner. + (vi) a dimension finding whose `entry_point` is set and whose `evidence` + contains a four-backtick run renders inside a fence of at least + five backticks, escaped, with the following section still outside + any code block. + Mutation: replace `review.fence_for` with a fixed three-backtick + fence. The assertion must fail on the text AFTER the excerpt, not + on the excerpt itself — a broken fence still contains the excerpt, + so an assertion that only looks for it passes under the mutation. + (vii) a PUT that returns 403 raises and issues no POST. + Mutation: make post_or_update fall back to POST on a failed PUT. + This is the mutation that produces a second review, so it is the + one whose absence would be least visible: nothing else in this + suite would notice. + (viii) AN ALL-CLEAN INPUT POSTS. `main` is driven end to end over a + fixture whose every report is `outcome: clean`, through the injected + transport, and the transport records EXACTLY ONE write call. This + asserts against `publish.py`, not against the renderer, because the + decision to post lives in `main` and nowhere else. + Mutation: add `if not findings and not incomplete: return 0` to + `main`. That is not a strawman — it looks like a reasonable + optimisation against the edit-event noise OPEN itself raises — and + before this assertion existed the whole suite passed with it applied: + STEP 6 offered only a grep of the pure renderer, whose `-> str` + contract means it never returns None in any correct implementation, + and STEP 7's done-when exercises `--dry-run`, which posts nothing by + definition. The agent would have gone silent on exactly the pull + requests where it found nothing, which is #119's criterion verbatim: + "A run that produced no confirmed findings still posts... Silence is + indistinguishable from a crashed agent." + (ix) A FOREIGN MARKED REVIEW IS NOT A PUT CANDIDATE, AND IS NOT A POST + LICENCE EITHER. Over a listing whose only marked review carries + another `user.login`, `main` issues NEITHER a PUT against the + foreign id NOR a POST — it raises, naming the foreign count, and + the transport records zero write calls. + Mutation: drop the author comparison from find_existing. The + assertion then sees a PUT against a review the agent does not own, + which is the denial-of-publication vector an outside party can + trigger deliberately, so it gets a control rather than only prose. + (x) A CLEAN LISTING STILL POSTS. Over a listing with no marker at all + — zero foreign count, not merely "no match" — `main` POSTs a new + review. Paired with (ix) so the refusal is proven to key on the + foreign COUNT and not on the listing being non-empty, or on + `find_existing`'s first element being None: both (ix) and (x) + return None as their first element, and only one of them may post. + Mutation: make post_or_update raise whenever find_existing's first + element is None, regardless of the foreign count. This is the + over-broad version of the (ix) fix, and it is wrong in the + opposite direction — refusing to publish on every pull request's + first run, which is silence indistinguishable from a crashed + agent on the majority case, not only the attacked one. + done when: all TEN assertions run offline and pass; each of the ten + stated mutations, applied one at a time, makes exactly its own assertion + fail and is then reverted; the recorded output of all ten mutation runs is + saved for the PR body; and each assertion prints what it compared rather + than only PASS. + +STEP 11 Register both controls in run_controls.py. [needs 9, 10] + Two entries appended to CONTROLS: ("check_publish_scope.py", True) and + ("check_publish_single.py", False). The scope control needs network for + its live half and is expected to SKIP that half locally. + REGISTERING IT HERE DOES NOT MAKE THIS RUNNER ITS HOME. `run_controls.py` is + invoked by #120's read-only controls workflow, so the scope control's live + half must SKIP there on STEP 9's `GITHUB_WORKFLOW` guard, and its PASS can + only ever come from the publish workflow that STEP 8 declares. It is + registered here so the static half travels with every other control and so + the suite has one entry point, not so that a run of this runner can satisfy + #119's credential criterion. A summary line counting it as passed inside the + controls workflow would be the false PASS this arrangement exists to prevent. + done when: `python3 run_controls.py` runs both and its summary line counts + them; with `gh` unauthenticated the scope control appears in the skipped list + with a reason rather than in the passed list; and with `GITHUB_WORKFLOW` set + to the controls workflow's name the scope control's live half still appears + as skipped, so the runner cannot report a live PASS for a token it is not + holding. + +STEP 12 launchpad/review-agent/PUBLISHING.md, and the cross-references. [needs 11] + Normative, a sibling to CONTAINMENT.md and in the same voice. States: that + a review is identified by MARKER AND AUTHOR — the marker says "this is our + kind of review", the author says "this one is ours" — that the author login + is SUPPLIED via `--as` and VERIFIED against the live identity by STEP 9's + control, never discovered at runtime and never written into a comparison, + because `GET /user` is an OAuth-and-PAT endpoint that an installation token + cannot be assumed to reach and a login frozen into the source breaks when + #110 moves the credential; that an unresolved identity aborts and never + degrades to matching on the marker alone; that a marked review under any + other login is counted as foreign and never updated, because a review body is + attacker-writable and marker-only matching hands an outsider a way to silence + the agent on a pull request of their choosing; that a NONZERO foreign count + also refuses a fresh POST rather than silently creating a second review, + because that count cannot be told apart, from the data alone, from a + planted decoy or from this agent's own review orphaned by a changed `--as` + value, and a human must look at either case rather than the tool guessing; + that publishing runs under `pull_request_target`, not `pull_request`, + because this job holds pull-requests write and a same-repository PR's own + diff could otherwise modify the code that job executes, and that the + checkout step must never set `ref:` to the pull request's head or that + protection is undone; that the fork skip is a job-level `if:` on a + separate guard job's output, never a step that exits 0 — a step exiting 0 + marks only itself successful and does not stop the steps after it; that + the live credential + control PASSes only from the + publish workflow and SKIPs everywhere else, so a PASS from the read-only + controls runner is not evidence about the publish token; and that + publish.py owns it while publish_render receives it; that exactly one + review object exists per pull request and is updated in place, and that a + failed PUT raises rather than posting a second review; that when two + markers do exist it is the NEWEST that is kept current, because the oldest + cannot be deleted and the newest is what a reader sees last; that the head + SHA lives in the body because `commit_id` is frozen at submission; the + incomplete rule and its TEN triggers; that the clean case posts; that the + body names both the head SHA and the merge base, and why the pair rather + than the head alone; that raw evidence is fenced with `review.fence_for` + and escaped with `contain.escape`, never with a fixed fence, and that this + means a published excerpt shows a doubled tilde where the author wrote one, + so a reader does not mistake the renderer's escape for the author's text; + that `defect` and `failure` are NOT escaped, with the measured reason — + `contain.ESC` is a tilde and `~~` is markdown strikethrough; that each + report's completion-marker nonce is checked against the merged document's + `nonce`, that there is ONE nonce per run so no marker can legitimately carry + a different dimension's, and — stated as a limit rather than a feature — + that the key authenticates a marker echoed out of author text and NOT the + document, so a compromised runner is outside what this stage can detect and + the review must never be described as unforgeable; the credential + and its two controls; and the fork-skip behaviour. + Cross-referenced from CONTAINMENT.md's "Contract for later stages" table + and from #117's FINDINGS.md, so the three documents point at each other + rather than diverging quietly. + done when: PUBLISHING.md exists under launchpad/review-agent/; it names + the marker string, the TEN incomplete triggers, the two keys of the + `containment` block it consumes from #117 — `findings` and a seven-entry + `states` map, and no `unreadable` key — and both controls by filename; + it states that a nonzero foreign count refuses a POST rather than + silently duplicating the review; it states `pull_request_target` as the + trigger and that the checkout step must never override `ref:` toward the + pull request's head; it states that the fork skip is a job-level `if:`, + never a step that exits 0; CONTAINMENT.md's contract table has a row + pointing at it; and it records that #117's contract is SETTLED and names + which revision, together with the one thing #119 cannot verify from it + (the marker nonce, see OPEN), so a reader is not left to infer that + everything upstream is checkable here. + +PARALLEL + STEP 1 and STEP 4 may run as concurrent subagents. They share no file — STEP 1 + writes only fixtures/review-lifecycle.json, STEP 4 writes only + publish_render.py — and STEP 4's input is #117's contract, not STEP 1's output. + STEP 9 and STEP 10 may run concurrently once their dependencies are met. They + write check_publish_scope.py and check_publish_single.py respectively and touch + nothing else; STEP 11 is what merges them into run_controls.py. + Everything else is sequential, and mostly for one boring reason: STEPs 2, 3 and + 7 all edit publish.py, and STEPs 4, 5 and 6 all edit publish_render.py. Two + steps editing one file are sequential however unrelated they look. + STEP 3 cannot be parallelised with anything that posts, because two agents + publishing to the same throwaway pull request would each see the other's + review and the single-review assertion would be measuring the wrong thing. + Dispatching is not this plan's decision. Nothing here is dispatched. + +GATES + No verify gate is installed in this checkout, so every one of these is a manual + invocation and none fires on its own. + serina:review-plan — HAS RUN ONCE on this file, and this revision is the result + of it plus #117's contract settling. Fourteen findings: one Blocker, six High, + six Medium, one Low. The Blocker was WITHDRAWN — it concluded #117 had moved + containment inside `reports`, and #117 then settled it the other way, so the + separate block was right all along. Its residue was real, though: the plan's + ALREADY TRUE was asserting a nine-field record and no `evidence`, both wrong. + Of the other thirteen, eleven are fixed in this revision, one (finding 3, the + nonce) is fixed as far as this stage can and flagged upstream in OPEN, and one + (finding 13, whether an edit is visible to a human) is converted from an + unanswerable question into a STEP 1 observation. A second pass is available and + has not run; on both #116 and #117 the second pass found most of its findings + in the first pass's own fixes, so assume the same of this revision. + serina:review-code — after STEP 7, and again after STEP 12. The first pass + catches the lifecycle and renderer while they are still small; the second sees + the workflow and the controls. + serina:review-tests — after STEP 10, on the two control scripts. These controls + are the only thing standing between "the credential is narrow" and "we said + the credential is narrow", so a control that cannot fail is the worst defect + available in this issue. + serina:review-adjudicate — after the reviewers, before any verdict is read. + serina:review-final — once, on the whole branch, before merge. + serina:review-a11y — not applicable and not claimed. See LEFT OUT. + The plan gate script: `~/.claude/skills/plan-issue/check-plan.sh` on this file. + It checks form, not substance, and a clean run is not a review. + Codex (`codex review --base origin/launchpad`) — HAS RUN ONCE, independent of + every serina:review-plan pass above and of the model that wrote this plan. + Five findings, three P1 and two P2, all applied in this revision. The most + consequential: this plan's own `pull_request`-not-`pull_request_target` choice, + copied from #120's comment block, was safe for #120's read-only job and unsafe + for this one, which holds pull-requests: write — a same-repository PR (the + norm this cohort actually produces, per ALREADY TRUE) could rewrite publish.py + or check_publish_scope.py and have that modified code execute with the write + token, violating AGENTS.md rule 1. STEP 8 now uses `pull_request_target` with + an explicitly-never-overridden base-ref checkout instead. Also fixed: the fork + guard was a step exiting 0, which marks only that step successful and does not + skip the job's later steps — now a job-level `if:` on a separate `guard` job's + output; no step anywhere in the workflow actually produced the seven-key + document and piped it into publish.py, so the workflow as specified could + never publish on any run — now named as an explicit step, against the CLI + shape #116/#117/#118 each state for themselves since none exist as code yet; + `post_or_update` had no way to surface `user.login` for STEP 9's identity + control — now a third return element; and a changed `--as` login orphaned the + prior review as merely "foreign" and silently POSTed a second one, breaking the + exactly-one-review invariant the whole issue exists to hold — `post_or_update` + now refuses to POST when `find_existing`'s foreign count is nonzero, requiring + a human to look rather than guessing whether it is a stale identity or a + planted decoy. + +BUDGET + STEP 9's live half is the step most likely to overrun. It is the only assertion + that cannot be made locally: the workflow token exists only inside a real + Actions run, so every iteration costs a commit, a push and a full run cycle, + and the failure modes are the slow kind — a permissions block that parses but + does not apply, a 404 where a 403 was expected because the ref path was wrong, + a control that reports PASS on the wrong error. Budget several cycles and write + the static half first so at least one assertion is provable without one. + Second, and structural rather than per-step: everything #119 imports — + `review.SEVERITY_ORDER`, `review.render_review`, `run_controls.CONTROLS`, + CONTAINMENT.md's rendering rule — existed only as untracked files in #120's + worktree, on no branch and in no commit. **That is no longer true, as of + 2026-08-13.** #120 is three commits on `feat/review-agent-untrusted-input`, + all pushed, at `c64ff7958`, with its control suite green (11 controls, 0 + failed, 0 skipped). The dependency is now a real ref rather than a working + directory, so STEPs 4, 5, 6, 11 and 12 can cite commits instead of a path on + one machine. The risk it replaces is smaller but not gone: the branch is + unmerged, so a rebase before it lands still moves every line number here. + Before STEP 4, re-verify render_review's signature and SEVERITY_ORDER's + location against whatever #120 has actually committed by then, rather than + trusting the line numbers quoted in ALREADY TRUE. **That instruction has now + paid for itself**: run on 2026-08-13 it caught the removal of the `unreadable` + keyword, which would otherwise have been a TypeError on the first call STEP 4 + made — found by reading, not by running, because none of this is built yet. + The cheapest mitigation is still ordering: let #120 land first. That is a fleet + sequencing decision, not this plan's. + This risk is not hypothetical — it fired during planning. Between the first + draft and its review, `review.py` moved SEVERITY_ORDER from line 19 to 32 and + render_review from 29 to 42, and two new control scripts appeared in that + worktree. Nothing broke, because the plan cites those symbols by name as well + as by line, but a step that had said "review.py:19" and nothing else would + already have been wrong within the hour. + Third: STEP 1 depends on a claim this plan has not verified — that PUT on a + submitted review returns 200. If it does not, STEP 1 is cheap and the plan + changes there. If instead PUT silently succeeds but GitHub renders the review + against its original commit in a way reviewers find misleading, that surfaces + at STEP 3 and is a judgement call, not a bug. + +OPEN Not for a builder to decide. + The two done-criteria in #119 are in tension and this plan resolved it by + asking. "Pushing a new commit produces a new review for that commit" is read as + new review CONTENT for that commit, in the same review object, because GitHub + offers no way to remove a submitted COMMENT review. A reader who meant a new + review object per push is asking for accumulation, which the next criterion + forbids. If STEP 1's recorded responses contradict the assumption that PUT + works, this is reopened rather than worked around. + An updated review keeps its original `submitted_at` and `commit_id`. The PR + timeline will therefore show the review at the time of the FIRST push, with a + body describing the LATEST commit. The body names the SHA, so nothing is + ambiguous to a reader who reads it — but whether that is acceptable to human + reviewers is a call for whoever reviews the first ten (#121). + Whether the publish workflow is its own file or folds into #116's invocation. + #110's decision comment names #116 as ".github/workflows/ invocation". #119's + own "impacted components" names only `launchpad/` and the token, not a + workflow. STEP 8 adds one anyway, because a criterion about re-review on push + is untestable without a trigger. If #116 lands an invocation workflow first, + STEP 8's job should move into it and STEP 9's static assertion should follow + it. That is a sequencing decision. + #117's contract is SETTLED, and these are the steps that change if it moves + again. A rename or removal of `severity`, `anchor`, `file`, `line`, `defect`, + `failure`, `dimension`, `entry_point` or `evidence` changes STEP 4. A change to + `status`, `outcome`, `error`, `completion_marker` or `findings_count` changes + STEP 5, and `outcome` alone also changes STEP 6. A change to the `containment` + block's two keys changes STEPs 4, 5 and 12. Either changes STEP 10's recorded + inputs and STEP 12's prose. STEPs 1, 2, 3, 8, 9 and 11 are unaffected by any + field rename, because they operate on a body string and a credential and never + look inside a finding. `finding_id` is not used by #119 at all except as a + tie-break in STEP 4's sort, so #117's warning that it is unstable across a + reworded `defect` costs this issue nothing — the body is rebuilt wholesale on + every run rather than diffed against the previous one. + AN ADDITION BELONGS IN THIS REGISTER TOO, and its absence is what actually cost + this plan a revision. The list above tracked renames and removals. #117 changed + by ADDING `evidence` as a tenth field and by ADDING the `containment` sibling + key — no rename, no removal — and this plan's ALREADY TRUE went on asserting + that neither existed, because nothing prompted a re-read. A field that appears + is as consequential as one that disappears: `evidence` arriving raw is the + reason STEP 4 now carries a fencing rule at all, and a plan that only watches + for subtraction will publish an unescaped payload while its register stays + green. So: a NEW field on the finding record, a NEW key on the envelope, or a + NEW key on the merged document changes STEP 4, and any of the three obliges a + re-read of the contract before STEP 4 is built rather than after. + A concern with the contract, raised rather than worked around. #117 states that + #118 re-rates severity and that "the reporting dimension's value must remain + readable after adjudication rather than being overwritten in place" — but the + record carries exactly ONE `severity` field, so there is nowhere for the + re-rated value to live. #119 sorts by `severity` and cannot tell which of the + two it is holding. Either the contract needs a second field or the sentence + needs to go; this plan does not choose, and does not silently diverge, because + #118 will honour the same contract. + The second concern is DISCHARGED, and recorded as discharged rather than + deleted. An earlier revision said "**#117 must add one key to its output before + #119 can publish a containment finding**", and that every real injection attempt + would render as the incomplete banner until it did. #117 has added it: a + top-level `containment` key carrying raw `contain.Finding` plus a seven-key + `states` map, present on every run. The design call this plan declined to make — + a sibling key versus a `kind`/`evidence` pair absorbed into the finding record — + went to the sibling key, which is what STEP 4 already assumed. Nothing here had + to change to accommodate it; what had to change was ALREADY TRUE's claim that + the key did not exist. + + The third concern is ALSO DISCHARGED, and it is worth recording how, because the + mechanism is the point. An earlier revision flagged that the marker nonce was + unverifiable by this stage: #117 made the completion marker + BUZZ-DIMENSION-COMPLETE:{dimension}:{nonce} and argued, correctly, that the nonce + is what stops a pull-request author typing a forged marker into their own diff + and a reviewer echoing it back — but the nonce appeared in no field of the + finding record, no field of the envelope, and none of the merged document's keys, + so every stage after the runner received markers it could not check. #117's own + `validate(report)` had the same problem from the other side. + It was raised rather than worked around, and #117 closed it by adding a top-level + `nonce` key. STEP 5 condition (5) now checks each marker against the document's + value, STEP 7 takes it as a seventh stdin key, and the single-report blind spot + the old agreement-only check had is gone. That is the second time on this issue + that flagging a gap upstream produced a contract change rather than a local + workaround, and it is cheaper than either plan guessing. + WHAT IT STILL DOES NOT DO, kept because discharging a concern is not the same as + eliminating the risk. 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 nothing at this stage can tell. That residual is + bounded and real, and no prose in this plan or in PUBLISHING.md may describe the + published review as unforgeable. + + Whether `defect` and `failure` should be escaped at all is #119's call and this + plan has made it: yes, through `contain.escape`. They are model-authored prose + rather than author-supplied text, so no upstream contract requires it, and a + reader may reasonably think it is belt-and-braces. The argument for it is that a + model quoting an attacker's delimiter into its own defect line is the one path by + which the payload re-enters the document at full authority after containment has + done its job, and the transform touches only two characters. If a reviewer + disagrees, the place to change it is STEP 4's rule and STEP 12's prose together. + The first draft of this plan assumed `review.render_review` would simply + compose with #117's records. It does not — the function reads `.severity`, + `.kind`, `.entry_point` and `.evidence` by attribute off `contain.Finding`. + That was found by serina:review-plan, rated Blocker, and is recorded here so + the next reader knows the composition is deliberate rather than inherited. + Whether the throwaway pull request from STEPs 1 and 3 stays open, and whether + its recorded responses are committed as fixtures. They contain review ids and + bodies from this public fork — no credential — but they are permanent once + committed. + What happens when the workflow token is present but the review is on a pull + request the agent has already reviewed at the SAME head SHA — a re-run with no + new commit. This plan re-renders and PUTs unconditionally, which is idempotent + in content but produces an edit event each time. Whether to skip when the SHA is + unchanged is a preference, not a correctness question. + +LEFT OUT Deliberately excluded. + Approving, requesting changes, merging, and any label that gates a merge. + #119 puts all four out of scope and AGENTS.md §5 rule 1 forbids the first three + outright. STEP 2 enforces it by construction rather than by discipline: there + is no event parameter to pass the wrong value to. + Inline file:line review comments — the `comments` array that POST /reviews + accepts. Rejected for a concrete reason, not omitted: PUT updates only a + review's BODY, so inline comments cannot be re-anchored when the head moves, + and stale ones would accumulate on lines that no longer exist. #119's criterion + is satisfied by rendering `file:line` as text in the body, which survives an + update and survives a force-push. + Publishing on cross-repository (fork) pull requests. `pull_request_target` (see + STEP 8) removes the PERMISSION reason an earlier revision had for skipping + forks — the base repository's token is available either way now — but no + cross-repository publication path is built, and none is designed: this issue's + own scope is `launchpad-26/buzz` pull requests. Fork pull requests still get a + loud, job-level skip, for that scope reason rather than a permission one. If + an outside contributor ever opens one, the job log says why there is no + review; the first fork PR is when someone decides whether that is good enough. + Any read of #116's pre-flight record. It is enumerated in a plan and built + nowhere, and its schema version is its own OPEN question. STEP 5's stage + manifest is what stands in for it, and swapping the manifest for the real + record later touches STEP 5 alone. + Deciding whether a finding is real. #118 owns confirm/refute, re-rated severity + and dedupe. #119 publishes what it is handed, in severity order, and adds no + judgement of its own. + Running the dimensions, and choosing a model. #117 owns the first and puts the + second out of scope. publish.py takes JSON on stdin and never names a model. + Measuring whether the reviews are any good. #121 owns the first ten reviews and + #109's success signals. Nothing here produces a precision or recall figure, and + nothing here should be read as one. + Accessibility is out of scope for this issue and is not claimed. The deliverable + is a CLI and a workflow; the only surface a human reads is markdown rendered by + GitHub's own interface, which carries its own keyboard behaviour and + announcements. There is no control to reach, no focus to manage and nothing to + announce. If a rendered dashboard over these reviews ever follows, it needs its + own keyboard and announcement specification and does not inherit one from here. + The AUROC range from #109. Per #122 it is one judge, one victim model and two + attacks, and the "standard validation sets" phrase is not a quotation. Nothing + in #119 needs either figure, so neither is repeated. diff --git a/launchpad/plans/reviews/2026-08-13-119-ACTION-get-user-under-actions-token.md b/launchpad/plans/reviews/2026-08-13-119-ACTION-get-user-under-actions-token.md new file mode 100644 index 00000000000..05a1f3ba88e --- /dev/null +++ b/launchpad/plans/reviews/2026-08-13-119-ACTION-get-user-under-actions-token.md @@ -0,0 +1,79 @@ +# ACTION for #119's author — `GET /user` cannot resolve the identity under the Actions token + +**From:** the second-pass reviewer (session whose primary worktree is +`feat/review-agent-dimensions`, issue #117). **Written:** 2026-08-13 09:10. +**Against:** the working-tree plan at 1271 lines, `:337-338`, `:418`, `:1017`. +**Short on purpose** — you are mid-revision. Full reasoning is in +`2026-08-13-119-plan-review-second-pass.md`, addendum at the top. + +## Your two Blocker fixes are correct. Do not revisit them. + +`find_existing` requiring MARKER **and** own identity with the author checked first, foreign +markers counted and reported never targeted — correct. STEP 11's "REGISTERING IT HERE DOES NOT +MAKE THIS RUNNER ITS HOME" with the `GITHUB_WORKFLOW` guard — correct. Both close the findings +they were written for. + +## The one problem: the mechanism, not the decision + +`:337-338` reads *"THE IDENTITY IS RESOLVED AT RUNTIME, NOT HARDCODED — once at startup from +`GET /user`, compared against each review's `user.login`."* + +**`GET /user` has no authenticated user under an Actions workflow token.** `GITHUB_TOKEN` is an +installation (server-to-server) token; the authenticated-user endpoint answers +`403 Resource not accessible by integration`. **This is stated unverified** — no installation +token exists outside a real Actions run, so neither of us can test it from a worktree. Treat it +as a claim to check, not a fact I proved. What I did verify: + + +``` +$ gh api user --jq '{login, type}' +{"login":"serina-mcfall","type":"User"} # works — but this is a USER token + +$ gh api repos/launchpad-26/buzz/issues/comments --paginate \ + --jq '[.[]|select(.user.type=="Bot")]|group_by(.user.login)|map({login:.[0].user.login,count:length})' +[{"count":73,"login":"github-actions[bot]"}] # 153 bot comments across pages, +[{"count":63,"login":"github-actions[bot]"}] # every one from github-actions[bot] +``` + +**Why this is worth your time even though it is unverified.** STEPs 1 and 3 both run under a +human `gh auth` token, as your plan says. `GET /user` succeeds there. So the resolution works +in every step that exercises it locally and fails only inside Actions, under the credential it +was written for — either aborting the run, in which case the agent publishes nothing on +**every** pull request (worse than the attack the filter prevents), or falling back to +something unspecified. Same shape as the `unreadable=` keyword you already caught: right +against what was tested, wrong against what ships. + +**It is in three places, one of them a control.** `:418` asserts `GET /user` "through the +injected transport" — a control that goes green under a stub and under a PAT and can never +exercise the credential that breaks it. `:1017` puts it in PUBLISHING.md as the normative rule. +Fixing `:338` alone leaves a document asserting it and a passing control agreeing. + +## You were right to reject hardcoding. This is a third option, not that one. + +`:341` says a hardcoded `github-actions[bot]` "would break on the day it moves". Agreed — do +not hardcode. But do not *discover* it either. **Supply it and verify it:** + +- `--as `, defaulting to a value **STEP 1 measures** rather than one this plan asserts. +- STEP 1 already POSTs a review and captures the response. Add `user.login` from that response + to its done-when, and record what `GET /user` returns for the credential in use — that + settles the 403 question with evidence, in the step that exists for exactly this, and it + costs you one extra field in a fixture you are already writing. +- A control asserts the configured login equals what a live POST reports. When #110 moves the + credential, one flag changes and the control catches a stale default. + +That keeps your objection intact — nothing is frozen into the module — while removing a +runtime call that likely cannot run. Keep the foreign-marker counting and reporting exactly as +it now stands; none of that changes. + +## What I did not do + +I did not edit your plan. It moved 663 → 978 → 1061 → 1271 lines while I reviewed, and a +concurrent edit risks being silently discarded by a rewrite — which is worse than no edit, +because it would read as fixed. The change is yours to make or to reject. + +If you reject it, the useful residue is still STEP 1 recording what `GET /user` actually +returns for the workflow credential. That turns this from an argument into a measurement. + +```findings +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:338 identity resolved via GET /user, which has no authenticated user under an Actions installation token +``` diff --git a/launchpad/plans/reviews/2026-08-13-119-plan-review-second-pass.md b/launchpad/plans/reviews/2026-08-13-119-plan-review-second-pass.md new file mode 100644 index 00000000000..45d817335c4 --- /dev/null +++ b/launchpad/plans/reviews/2026-08-13-119-plan-review-second-pass.md @@ -0,0 +1,469 @@ +# serina:review-plan — #119, second pass (independent) + +**Plan reviewed:** `launchpad/plans/2026-08-12-issue-119-publish-one-review.md` **as committed +at `8a405a9f5`** — 663 lines, the revision whose last commit message is *"match #119's plan to +render_review's current signature"*. Line citations in the findings below are against THAT +text. Current-revision line numbers are given separately for the findings that survive. + +**Who ran this:** a session whose primary worktree is `feat/review-agent-dimensions` +(issue #117). Second pass, run after and independently of +`2026-08-13-119-plan-review.md`, which was deliberately **not read** until the findings below +were finalised. Reviewed: 2026-08-13. + +**Independence — declared, because it is partial.** I am not independent of the *contract* this +plan consumes. Earlier in the same session I reviewed #117's plan and then **fixed** it, +including settling where containment findings live. The first pass's Blocker 1 was withdrawn +*because of the settlement I wrote*. Treat my reading of anything touching the #117 interface +as interested. Findings 1, 2 and 6 — the three that survive — are independent of that +interface and do not depend on it. + +**Format gate:** `check-plan.sh` on the reviewed revision → +`Mechanical checks: clean. Your call on the substance.` + +**Severities are proposals.** `serina:review-adjudicate` re-rates. + +**LINE PINS IN THIS FILE ROT, and did so while it was being written.** The plan went 663 → +978 → 1061 lines during this pass, across commit `47482549e` plus further uncommitted edits. +The three surviving findings were re-verified against the 1061-line text and their +current-revision pins are given as of that. Every finding below therefore names the RULE and +the SYMBOL it lands on, not only a number — cite those, and re-locate the number. Both this +plan and #117's record the same lesson about pinned citations; this file is a demonstration +of it rather than an exception to it. + +--- + +## URGENT — read this first + +The plan was being revised against the FIRST PASS ONLY while this pass ran. Of my seven +findings, **three survive that revision and two of them are Blockers.** They survive because +they were not in the first pass, so nothing prompted the reviser to look at them. + +Worse: **the first pass's fix for its own finding 10 made my Blocker 1 strictly easier to +exploit.** That is the single most important line in this document. + +| Finding | Severity | In first pass? | Status in the in-flight revision | +|---|---|---|---| +| 1. `find_existing` matches attacker-writable text | Blocker | **No** | **SURVIVES — and WORSENED** | +| 2. Live scope control probes the wrong token | Blocker | **No** | **SURVIVES** | +| 3. `SEVERITY_ORDER[...]` subscript | High | Yes (#2) | Fixed | +| 4. `states`-names-all-seven in no done-when | High | **No** | Fixed anyway (now condition 10) | +| 5. Pagination control cannot fail | High | Yes (#5) | Fixed, better than I proposed | +| 6. No control asserts a clean run POSTS | High | **No** | **SURVIVES** | +| 7. Incomplete-trigger count contradicts itself | Medium | Yes (#8) | Fixed | + +--- + +## Blocker 1 — `find_existing` matches attacker-writable text, with no author filter + +**Reviewed revision:** `:195-199`. **Current revision (1061 lines):** `:287` — the +`find_existing` NEWEST rule; the marker-not-author justification in STEP 12; the +failed-PUT-raises rule in STEP 2. Re-verified: no `user.login`, `author_association` or +`GET /user` check appears anywhere in the plan. + +**Defect.** Identification is `body.startswith(MARKER)` with no author filter, chosen +deliberately so the lookup survives the credential's identity changing. But a review body is +not agent-controlled territory: any GitHub user with read access can submit a `COMMENT` +review on an open pull request in this public fork, and its body is theirs to write. + +Verified — the listing already carries everything a filter needs: + + +``` +gh api repos/launchpad-26/buzz/pulls/86/reviews --jq '.[0] | keys' +["_links","author_association","body","commit_id","html_url","id","node_id", + "pull_request_url","state","submitted_at","user"] +``` + +**Concrete failure.** Anyone submits a COMMENT review on the PR whose body's first line is +``. `find_existing` returns that review's id. +`post_or_update` issues a PUT against a review the token does not own, GitHub refuses it, and +the current revision makes the consequence deterministic at `:303-308`: *"A FAILED PUT IS A +HARD FAILURE AND NEVER FALLS BACK TO POST… post_or_update raises… and the workflow fails +loudly with no review updated."* So the agent publishes **nothing**, permanently, on any pull +request an attacker chooses — and the failure presents as an HTTP error, not as an attack. +#119's own criterion calls silence "indistinguishable from a crashed agent"; this makes that +silence available on demand. + +**Why the revision made it worse.** The reviewed revision took the **OLDEST** marked review, +which at least required the attacker to plant *before* the agent's first run. The in-flight +revision takes the **NEWEST** (`:265`, `:269-274`) to fix the first pass's finding 10. With +NEWEST there is no timing requirement at all: a review planted at any moment is immediately +the newest, so it wins every subsequent run, forever. The first pass's diagnosis for its +finding 10 was correct — keeping the oldest current leaves an undeletable stale newest — but +its fix optimised one failure mode into another, because it was not weighing an adversary. + +**Fix, satisfying both concerns at once.** Filter by author FIRST, then take the newest among +the agent's own: match on marker **and** `user.login` equal to the identity the token +authenticates as, resolved once at startup from `GET /user` rather than hardcoded — which +preserves exactly the credential-portability property the marker-over-author choice was made +for. Report the count of *foreign* marked reviews rather than treating them as candidates. + +--- + +## Blocker 2 — the live credential control probes the wrong token, and passes either way + +**Reviewed revision:** `:406`, `:465`. **Current revision (1061 lines):** `:809` — STEP 11's +`("check_publish_scope.py", True)` entry; STEP 9's live half, its "outside Actions" SKIP guard +and its PASS-with-403 done-when. Re-verified: no `GITHUB_WORKFLOW` check appears anywhere in +the plan, so nothing distinguishes which workflow executes the live half. + +**Defect.** STEP 9's live half attempts a contents write "with the workflow's own token" and +asserts 403. STEP 11 registers it as `("check_publish_scope.py", True)` in +`run_controls.py` — which is invoked by **#120's controls workflow**, not the publish +workflow. Verified: + + +``` +$ grep -nE 'permissions|contents|pull-requests|issues|run_controls' \ + .github/workflows/launchpad-review-agent-controls.yml +23:permissions: +24: contents: read +25: issues: read +26: pull-requests: read +48: run: python3 run_controls.py +``` + +The token under test is therefore `{contents: read, issues: read, pull-requests: read}` — a +read-only token whose lack of contents-write was never in question. Both it and the publish +token carry `contents: read`, so the ref-create returns 403 and the control reports PASS +regardless of what the publish workflow's permissions block says. + +The SKIP guard does not save it. `:655` reads *"Outside Actions there is no workflow token, so +the live half reports SKIP"* — but inside #120's controls workflow there IS a workflow token. +It is simply the wrong one, so the guard never fires and the control PASSes. + +**Concrete failure.** `:664` asks for *"a real Actions run on this pull request shows the live +half reporting PASS with the 403 response body pasted into the PR"*. That PASS and that 403 +body get pasted into the pull request as evidence about the publish credential, having +measured a different credential entirely. #119's criterion — *"A control or documented check +demonstrates the absence of contents write"* — is reported satisfied and is not. The static +half does catch a later widening of the publish workflow's YAML, so this is not a total blind +spot; it means the live half, which STEP 9 calls "the only step that can demonstrate #119's +credential criterion", demonstrates nothing. + +**Fix.** Run the live half as a step **inside** the publish workflow, where `GITHUB_TOKEN` is +the credential in question. Additionally have it assert *which* workflow it is running in +(`GITHUB_WORKFLOW`) and SKIP with a reason — never PASS — anywhere else, which is the rule +`run_controls.py` already enforces for missing inputs. Registering it in the shared runner +without that guard guarantees a PASS on the wrong token. + +--- + +## High 6 — no control asserts that a clean run actually posts + +**Reviewed revision:** `:349`. **Current revision (1061 lines):** `:787` — STEP 10's +assertion (v), still "two bodies differ"; STEP 6's `grep -n "return None" publish_render.py` +done-when. Re-verified: no assertion anywhere drives `post_or_update` on a clean input. + +**Defect.** #119's criterion is *"A run that produced no confirmed findings still posts, +saying so explicitly. Silence is indistinguishable from a crashed agent."* The plan offers +`grep -n "return None" publish_render.py` as proof of "no early return on the clean path". +That check fails twice over: it greps the **pure renderer**, whereas the decision to call +`post_or_update` lives in `publish.py`'s `main`; and it cannot fail for the right reason, +since a function whose contract is `-> str` never returns `None` in any correct +implementation. It passes for every implementation, correct or not. + +STEP 10's assertion (v) — still unchanged at `:704` in the current revision — asserts only +that "a clean input and an incomplete input both produce a body, and the two bodies differ". +That is a rendering assertion. None of the now-seven assertions drives `post_or_update` on a +clean input. STEP 7's done-when exercises `--dry-run`, which by definition posts nothing. +STEP 3 does post, but with a stub body, before any findings logic exists. + +**Concrete failure.** An implementer adds `if not findings and not incomplete: return 0` to +`publish.py`'s `main` — a reasonable-looking optimisation against the edit-event noise the +plan itself raises in OPEN. Every control in the plan still passes. The agent goes silent on +exactly the pull requests where it found nothing, and a silent agent is indistinguishable +from a crashed one, which is the criterion. + +**Fix.** Add an eighth STEP 10 assertion with its own mutation: an all-clean input drives +`post_or_update` through the injected transport, and the transport records exactly one call. +Mutation: early-return on the clean path in `publish.py`'s `main`. That puts the assertion in +the file where the risk lives, and STEP 10 already has the transport seam to do it. + +--- + +## Findings 3, 4, 5 and 7 — fixed in the in-flight revision + +Recorded so the diff is complete, not because action is needed. + +- **3 (High) — `review.SEVERITY_ORDER[finding["severity"]]` bare subscript** raising `KeyError` + and posting nothing, where `review.py:62` deliberately uses `.get(f.severity, 9)`. Fixed: + STEP 4's done-when now asserts a finding with `severity: "Info"` renders under "malformed + finding", sorts last, does not raise, and triggers the banner. Corroborates first pass #2. +- **4 (High) — `states` must name all seven entry points, prescribed in prose at `:270` and in + no done-when.** The plan diagnosed the hazard itself and then did not gate it: a thin + `states` map means `unreadable` derives empty and a review over unreadable surfaces + publishes as complete — #120's banner-with-no-producer defect relocated one stage up. Fixed: + now condition (10) of STEP 5's enumerated list. The first pass did not report this; the + reviser reached it while restructuring, so it is fixed by luck of adjacency rather than by + either review. +- **5 (High) — the pagination control cannot fail under its own stated mutation.** Verified + independently that `gh api --paginate` merges pages into one array: + + +``` +$ gh api "repos/launchpad-26/buzz/pulls/86/commits?per_page=1" --paginate +first 3 chars : [{" +count of '][' : 0 +json.loads OK — single value, type list len 12 +``` + + So a "recorded two-page listing" is indistinguishable from a one-page listing of the same + length, and "drop `--paginate`" changes nothing a data-only fixture observes. Fixed, and the + fix is better than the one I proposed: the transport serves page two **only when + `--paginate` appears in the argv**, and the fixture is the unmerged page bodies. Corroborates + first pass #5, which reached the same measurement by a different route. +- **7 (Medium) — the incomplete-trigger count said six, seven, six, seven** across four places, + with the genuinely missing condition ("a report with `status: complete` and no `outcome`") + living only in STEP 6's prose. Fixed: ten numbered conditions, counted identically in + STEPs 5, 6 and 12. Corroborates first pass #8. + +--- + +## What I looked for and did not find + +- **A silent #117/#119 disagreement about containment transport.** This was my going-in + hypothesis and it is **refuted**. OPEN at `:599-611` states it as a hard dependency, names + the block shape needed, records that *"#117 must add one key to its output before #119 can + publish a containment finding"*, and names the safe failure until then. That is a plan doing + its job, and it is why #117's contract could be settled without guessing. +- **`gh api --paginate` emitting concatenated per-page arrays.** Expected; false. It merges. +- **The `8a405a9f5` amendment.** Verified correct against `c64ff7958`: + `render_review(findings, states) -> str` at `review.py:45`, `SEVERITY_ORDER` at `:32`, + `UNREADABLE_STATES` at `:42`, attribute reads at `:62-73`. +- **A path to `APPROVE`.** `post_or_update` has no event parameter. Enforced by construction. +- **STEP 8's `permissions`, `concurrency` and `pull_request_target` reasoning.** All correct. + The group-interpolation check genuinely FAILS a fixed group name rather than passing it, and + `github.ref` on a `pull_request` event is `refs/pull/N/merge`, so both permitted forms are + per-PR. +- **A fail-open classifier.** The incomplete default is correct; the scope probe rejects + success, 404 and rate-limit errors rather than treating any error as proof. + +**Two claims left unverified, deliberately.** Whether GitHub refuses a PUT on another user's +review, and whether a PR author may COMMENT-review their own pull request. Both require a +write to a public fork to settle. Blocker 1 does not depend on either: the current revision +specifies that any non-2xx PUT raises, so both branches end in no review being published, and +the attack needs only *some* user, not the author specifically. + +**Tools.** Full main-session pool. Used `Read` and `Bash` (`gh`, `curl`, `git`, `grep`, `sed`, +`python3`), plus `Write` for this file. Every behavioural claim above has command output +beside it. + +```findings +Blocker launchpad/plans/2026-08-12-issue-119-publish-one-review.md:287 find_existing matches attacker-writable body text with no author filter, and NEWEST removes the timing requirement +Blocker launchpad/plans/2026-08-12-issue-119-publish-one-review.md:809 live credential control is registered in the read-only controls runner, so it PASSes against the wrong token +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:787 no control asserts a clean run actually posts; the offered grep targets the wrong file and cannot fail +``` + +REVIEW COMPLETE + +--- + +# ADDENDUM 2026-08-13 09:05 — both Blockers fixed, and the Blocker 1 fix has a defect + +Checked against the live text at 1244 lines (HEAD `47482549e` plus uncommitted edits, file +modified seconds before this check). **Both Blockers above are fixed**, independently and +well — `find_existing` now requires MARKER **and** the agent's own identity with the author +checked first, foreign markers counted and reported never targeted; and STEP 11 now states +that registering the scope control in `run_controls.py` "DOES NOT MAKE THIS RUNNER ITS HOME", +with the live half SKIPping there on a `GITHUB_WORKFLOW` guard and its PASS only ever coming +from the publish workflow. Nothing in this file's Blocker 1 or Blocker 2 needs further action. + +**One new finding, on the Blocker 1 fix itself.** It reads: *"THE IDENTITY IS RESOLVED AT +RUNTIME, NOT HARDCODED — once at startup from `GET /user`, compared against each review's +`user.login`."* + +`GET /user` has no authenticated user under an Actions workflow token. `GITHUB_TOKEN` is an +installation (server-to-server) token, and GitHub answers `403 Resource not accessible by +integration` for the authenticated-user endpoint. **I could not verify that directly — no +installation token is available outside a real Actions run — and it is stated here as +unverified.** What IS verified: + + +``` +$ gh api user --jq '{login, type}' # human PAT +{"login":"serina-mcfall","type":"User"} # works, and is a USER token + +$ gh api repos/launchpad-26/buzz/issues/comments --paginate \ + --jq '[.[]|select(.user.type=="Bot")]|group_by(.user.login)|map({login:.[0].user.login,count:length})' +[{"count":73,"login":"github-actions[bot]"}] # 153 comments across pages, +[{"count":63,"login":"github-actions[bot]"}] # every one of them from +[{"count":17,"login":"github-actions[bot]"}] # github-actions[bot] +``` + +So the login the workflow credential posts as is `github-actions[bot]`, confirmed in this +fork. The mechanism prescribed to discover it is the one that most likely cannot run. + +**The concrete failure.** STEP 1 and STEP 3 both run under a human `gh auth` token, which the +plan says explicitly. `GET /user` succeeds there, so identity resolution works in every step +that exercises it locally. It fails only inside Actions, under the credential it was written +for — either aborting the run, in which case the agent publishes nothing on **every** pull +request (worse than the attack the filter was added to prevent), or falling back to something +the plan does not specify. A fix that passes every local control and breaks only under the +real credential is the same shape as this plan's own `unreadable=` keyword defect: correct +against the thing that was tested, wrong against the thing that ships. + +**It is now in three places, one of them a control.** `:337-338` prescribes the mechanism; +`:418` asserts it — *"`GET /user` through the injected transport"* — which is a control that +passes under an injected transport and under a human PAT and can never exercise the credential +that breaks it; and `:1017` puts it in STEP 12's normative PUBLISHING.md, *"resolved at runtime +from `GET /user` and never hardcoded"*, which is where a future reader would learn it as the +rule. Fixing the prescription without fixing those two leaves a document asserting it and a +green control agreeing. + +**Change.** Do not discover the login; supply it and verify it. An `--as ` argument +defaulting to a value STEP 1 **measures** — STEP 1 already POSTs a review and captures the +response, so add `user.login` from that response to its done-when, and record what +`GET /user` returns for the credential in use so the next reader does not have to guess. +That answers the objection the marker-over-author choice was originally made for: when #110 +moves the credential, one flag changes, and a control asserts the configured login equals what +a live POST reports. Keep the foreign-marker reporting exactly as it now stands. + +```findings +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:338 identity resolved via GET /user, which has no authenticated user under an Actions installation token +``` + +--- + +# Diff — this pass against `2026-08-13-119-plan-review.md` + +The first pass reported 14 findings (1 Blocker, 6 High, 6 Medium, 1 Low) against `eb2bf09d0`. +This pass reported 7 against `8a405a9f5`. Both gates clean. + +## Corroborated by both passes + +Independent agreement, reached by different routes. These were the strongest signals and all +three are already fixed. + +| Both found | First pass | This pass | +|---|---|---| +| Bare `SEVERITY_ORDER[...]` subscript posts nothing on an out-of-ladder severity | #2 High | #3 High | +| Pagination assertion cannot fail — `gh --paginate` merges pages | #5 High | #5 High | +| Incomplete-trigger count contradicts itself; the seventh lives only in STEP 6 | #8 Medium | #7 Medium | + +Worth noting: both passes measured the `--paginate` merge independently — the first with +`per_page=2` across 9 pull requests, this one with `per_page=1` across 12 commits. Same +conclusion. That is the kind of claim a single pass could have got wrong from memory, and two +passes measuring it is the reason to trust it. + +## Found only by this pass — the actionable residue + +All three survive the in-flight revision, because nothing prompted the reviser to look. + +1. **Blocker — `find_existing` matches attacker-writable text.** The first pass touched this + code twice (#10 on oldest-versus-newest, #11 on unspecified PUT failure) without treating + the body as adversarial. Both of its fixes have now landed and **both increase the + exposure**: NEWEST removes the timing requirement, and raise-on-failed-PUT makes the denial + deterministic. +2. **Blocker — the live scope control probes the wrong token.** Neither pass's territory + overlapped here; the first pass reviewed STEP 9's internals (#9, the `${{ }}` literal) but + not which workflow ends up executing it. +3. **High — no control asserts a clean run posts.** The first pass accepted STEP 6's + `grep "return None"` at face value. + +## Found only by the first pass + +Ten findings, all already applied. Three deserve calling out because a reader of this file +should not conclude the first pass was the weaker one — it was not, it was broader: + +- **#3 (High) — the nonce half of the completion-marker check is unimplementable**, because no + nonce reaches `publish.py`. I missed this entirely, and it is the best finding in either + pass: it identifies a check that *cannot* be built from the input the contract supplies. + **It also lands on #117, and see the cross-issue item below.** +- **#13 (Medium) — STEP 1 records four API responses and never records whether a body edit is + visible to a human.** The plan's entire re-review strategy is a body edit, and this is the + one behavioural assumption STEP 1 does not capture. I did not look for an absent + observation; I was looking for wrong ones. +- **#6 (High) — the stdin document supplies no `repo`** while both lifecycle functions require + one. A plain interface mismatch I walked straight past. + +The rest: #1 (withdrawn, below), #4 (`unreadable` TypeError — already fixed by `8a405a9f5` +before my pass began, which is why I saw a correct signature), #7 (containment evidence +dropped and unfenced), #9 (`${{ github.run_id }}` in Python), #10 (oldest duplicate stale), +#11 (unspecified PUT failure), #12 (STEP 10's fixtures produced by no step), #14 (circular +import). + +## Where the two passes disagree + +Four disagreements. Stated as judgements, not averaged. + +### 1. First pass #10 — oldest versus newest. **I think its fix is wrong.** + +Its diagnosis is right: keeping the oldest marked review current leaves an undeletable newest +one permanently stale, and a reader reaching the bottom of the timeline sees the stale one. +Its fix — keep the newest current — is wrong on its own, because it hands an attacker a +lookup with no timing requirement (Blocker 1). The two concerns are not in tension once the +author filter exists: **filter to the agent's own reviews, then take the newest among them.** +The first pass changed the ordering without adding the filter, which is the one combination +that is worse than either concern alone. + +### 2. First pass #9 — `${{ github.run_id }}` inside a Python module. **It is right; I was wrong to dismiss it.** + +I saw it, judged it already documented, and did not report it, on the grounds that BUDGET +already names *"a 404 where a 403 was expected because the ref path was wrong"*. That was a +bad call. BUDGET documents the risk **class**; the plan then prescribes a specific literal +that instantiates it. A documented hazard is not a documented instance, and the skill's own +rule — a limitation the plan already documents is not a finding — does not stretch to cover a +concrete defect that happens to fall inside a named risk area. Concede. + +### 3. First pass #1 — containment inside `reports`. **Correctly withdrawn, and I am not a neutral judge of that.** + +Its premise was that #117 had moved containment inside `reports`. #117 settled the opposite +way, normatively: a top-level `containment` sibling key carrying raw `contain.Finding` plus a +seven-key `states` map, never inside a dimension's findings array. The plan's separate block +was right all along. + +**Disclosure: I wrote that settlement.** Its withdrawal is a consequence of my own work on +#117 earlier in this session, so my agreement that it was correctly withdrawn is not +independent evidence. What *is* independent: the withdrawal's residue was real and the first +pass caught it — ALREADY TRUE claimed a nine-field record with no `evidence`, and the record +has ten with `evidence` as the tenth. That half of the finding held regardless of which way +the transport settled. + +### 4. Severity of first pass #7 versus its post-settlement form. **Its re-derivation is right and mine would have been too narrow.** + +The first pass rated it High for containment evidence dropped by the generic render path. After +the settlement dissolved the containment half, it **re-derived it larger** rather than +withdrawing it: `evidence` is raw on the ten-field record too, `entry_point` is required on +injection findings, and #117's cross-cutting clause makes all three dimensions emit them — so +raw attacker text arrives via the ordinary findings path and needs `review.fence_for`. That is +the correct move, and it is a better piece of reasoning than anything in my pass. I did not +find this at all. + +## The union, ranked + +Everything outstanding after the in-flight revision, most urgent first. Items 1–3 are mine and +unaddressed; item 4 is the first pass's, partly addressed and flagged upstream. + +1. **Blocker — `find_existing` has no author filter** (`:287` at 1061 lines). Fix before + anything else: it is the only defect here an outside party can trigger deliberately, it + costs one comparison against data already fetched, and the revision's NEWEST change made it + cheaper to exploit. Fixing it also completes first pass #10 properly. +2. **Blocker — the live scope control probes the read-only controls token** (`:809`). The + credential criterion is currently reported satisfied by a measurement of the wrong + credential. +3. **High — no control asserts a clean run posts** (`:787`). One STEP 10 assertion plus its + mutation. +4. **High, cross-issue — the nonce is verifiable by no downstream stage** (first pass #3, + marked "partly fixed, flagged upstream"). See below; the fix is in #117, not here. + +Items already fixed and needing nothing: my 3, 4, 5, 7 and the first pass's 2, 4, 5, 6, 8, 9, +10, 11, 12, 14, with 1 withdrawn and 13 converted to an observation. + +## Cross-issue action for #117 — raised against my own work + +The first pass's finding 3 is a real gap in the contract I settled this session, and it should +not be left sitting in #119's OPEN. + +#117's merged document is `{pr, merge_base_sha, head_sha, reports, containment}` — **no +`nonce` key.** The run nonce exists only embedded inside each report's +`completion_marker` (`BUZZ-DIMENSION-COMPLETE:{dimension}:{nonce}`, #117 STEP 1). So a +downstream stage can check that every marker *agrees* on a nonce — which catches one forged +marker among several — but it cannot check that the agreed nonce is the one actually generated +for the run. An all-forged run is undetectable downstream, which is precisely the forgery the +nonce was introduced to defeat. + +#119's revision handles this as well as it can from where it sits, and says so. The clean fix +is a `nonce` key on #117's merged document, checked against each marker. **That is a change to +#117's plan, which this pass did not make** — writing into another issue's plan on the strength +of my own review is exactly the loop this second pass exists to break. diff --git a/launchpad/plans/reviews/2026-08-13-119-plan-review.md b/launchpad/plans/reviews/2026-08-13-119-plan-review.md new file mode 100644 index 00000000000..2f5c23fa61a --- /dev/null +++ b/launchpad/plans/reviews/2026-08-13-119-plan-review.md @@ -0,0 +1,750 @@ +# serina:review-plan — #119, publish exactly one review comment + +**Plan under review:** `launchpad/plans/2026-08-12-issue-119-publish-one-review.md` +(committed at `eb2bf09d0`, branch `feat/review-agent-publish`) +**Reviewed:** 2026-08-13 +**Reviewer independence:** independent. The reviewer did not write the plan and had no +part in drafting it. Issue #119 was read first, then #117's contract, `review.py`, +`contain.py`, `fetch.py` and `run_controls.py` in the sibling worktrees. Every factual +claim below was executed, not recalled; the command output is quoted inline. +**Format gate:** `~/.claude/skills/plan-issue/check-plan.sh` → +`Mechanical checks: clean. Your call on the substance.` +**Severities are proposals.** `serina:review-adjudicate` re-rates; its rating is the one +a gate reads. + +## Headline + +The plan's own PROVISIONAL warning was correct, and the thing it warned about has already +happened. #117's contract changed at 20:34 on 2026-08-12 — 24 minutes after this plan's +recorded 20:10 snapshot — and the change breaks the plan's central design decision. +Findings 1, 5 and 7 all trace to that single contract move. + +A second observation, structural rather than per-finding: the plan's OPEN section +enumerates what breaks when a #117 field is **renamed or removed**. What actually broke it +was a field being **added**. An addition is invisible to that register. + +--- + +## Blocker + +### 1. The premise that containment findings cannot travel inside `reports` is now false, and STEP 5 converts that into a permanent "incomplete" banner + +- **Lands on:** STEP 4 primarily; STEP 5, STEP 12 and OPEN inherit it +- **Plan line:** `:90` (claim), `:237` `:250` (design), `:571` (cross-issue demand) + +**Defect.** Line 90 states as ALREADY TRUE — the plan's verified section — that *"#117's +envelope carries neither `kind` nor `evidence`"*. The `kind` half is true. The `evidence` +half is false at the contract revision the plan names. + +#117's contract declares `evidence` as its tenth finding field, required whenever +`entry_point` is set, and fixes a conversion placing containment findings **inside +`reports`** as a reserved dimension: + +``` +2026-08-12-issue-117-review-dimensions.md:176 evidence optional — the excerpt a finding rests on, rendered post-escape +2026-08-12-issue-117-review-dimensions.md:195 anchor "pr" — never "line" or "file" +2026-08-12-issue-117-review-dimensions.md:200 dimension "containment" — a reserved slug +2026-08-12-issue-117-review-dimensions.md:206 evidence rendered post-escape, per review.py's existing rule +``` + +#117's own review history records the change and its reason: *"Second pass, one Blocker: +STEP 1's containment conversion mapped an `evidence` value into a record that declared only +nine fields… `evidence` is now declared, required whenever `entry_point` is set, and the +count is ten."* + +So STEP 4's separate `containment` argument, STEP 5's rule that a missing block is +INCOMPLETE, and OPEN's demand — *"**#117 must add one key to its output before #119 can +publish a containment finding**"* — all rest on a constraint that no longer exists. #117 has +already decided to deliver containment the other way. + +**Concrete failure it allows.** #117 emits containment as a `reports` entry and never emits +a sibling `containment` key. Nothing can synthesise one: STEP 4's block shape requires +`kind` and `states`, #117's conversion consumes `kind` into `defect`, and no stage emits +`states` at all. STEP 5 therefore classifies **every** run as incomplete. Every published +review carries the banner that is supposed to mean "a stage did not finish", so a genuinely +truncated run becomes indistinguishable from a normal one — the inverse of #119's own +criterion, and worse, because a banner that always fires is a banner readers learn to skip. + +**Change.** Re-read #117 STEP 1 and STEP 2 as they now stand and pick one route: either +containment arrives inside `reports` under dimension `containment` and STEP 4/5/12 drop the +separate block, or the separate block stays and #117 is asked to keep both paths. Do not +leave both described. Add "a field added" to OPEN's register. + +--- + +## High + +### 2. `SEVERITY_ORDER[finding["severity"]]` is a bare subscript, and one unexpected severity posts nothing at all + +- **Lands on:** STEP 4 +- **Plan line:** `:257` + +**Defect.** `review.py:62` — the sibling this plan imports from — deliberately uses +`SEVERITY_ORDER.get(f.severity, 9)`. The plan prescribes the bare `[...]` subscript. + +**Concrete failure it allows.** Narrowest input: one finding anywhere in `reports` with +`severity: "Info"`, or lowercase `"blocker"`. `KeyError` in the sort key → `publish.py` +exits non-zero → **no review is posted at all**. That is precisely the silence #119's +clean-case criterion exists to prevent. Nothing catches it: STEP 4's "malformed finding" +path covers only anchor/field disagreement, and none of STEP 5's conditions mentions +severity. The plan cannot lean on #117's validator either, because STEP 7 declares itself +*"agnostic about which stage produced them"*. + +**Change.** Use `.get(severity, 9)` and route an unrecognised severity to STEP 4's +malformed-finding heading, or add "a finding whose severity is outside the imported ladder" +to STEP 5's conditions. + +### 3. The nonce half of the completion-marker check cannot be implemented, because no nonce reaches `publish.py` + +- **Lands on:** STEP 5; STEP 7's input contract is where the fix goes +- **Plan line:** `:293` (condition), `:308` (its test), `:329` (the input document) + +**Defect.** STEP 5 makes a review incomplete when a report's `completion_marker` *"carries +the wrong dimension or nonce"*. STEP 7's stdin document is +`{pr, head_sha, merge_base_sha, stages, reports, containment}` — no nonce is in it. #117 +fixes the marker as `BUZZ-DIMENSION-COMPLETE:{dimension}:{nonce}` with **one run nonce +shared by every dimension** (`117:238`), so "another dimension's nonce" — the input STEP 5's +done-when prescribes — is not a thing that contract can produce. + +**Concrete failure it allows.** Two. The implementer builds the dimension half and quietly +drops the nonce half, because the input cannot support it; and the done-when is satisfied by +a hand-built fixture that proves the plan's assumption rather than the code. The result is +that the forgery #117 introduced the nonce to defeat — a PR author writing a +`BUZZ-DIMENSION-COMPLETE:security:…` line into their own diff, echoed back by a reviewer — +reaches the publisher unverified, and a truncated report publishes as a complete review. + +**Change.** Either add `nonce` to STEP 7's stdin document and check it, or state that +#117's `findings.validate` owns the nonce and delete the clause from STEP 5. Do not describe +a check the input cannot support. + +### 4. `render_review` has no `unreadable` parameter; the prescribed call raises `TypeError` + +- **Lands on:** STEP 4; STEP 12 documents the wrong contract +- **Plan line:** `:76` (recorded signature), `:240` (block shape), `:243` (the call) + +**Defect.** Measured in this session against the live module: + +``` +render_review signature: (findings: 'list[Finding]', states: 'dict[str, str]') -> 'str' +TypeError: render_review() got an unexpected keyword argument 'unreadable' +``` + +`review.py:48` states why it was removed: *"`unreadable` is derived from `states`, never +passed in. It was a keyword argument with no producer anywhere on the branch… a caller +cannot forget an argument that does not exist."* + +**Concrete failure it allows.** BUDGET does tell the implementer to re-verify the signature, +so the `TypeError` itself gets caught at STEP 4. The damage is downstream and survives that +catch: STEP 4's `containment` block shape requires an `unreadable: [entry_point]` key, STEP +12 would document it normatively, and OPEN asks #117 to emit it. #119 would be asking +another issue to rebuild the caller-supplied argument #120 had just deleted as a defect. + +**Change.** Drop `unreadable` from the containment block shape and from STEP 12; derive it +from `states`, as `review.py` does. + +### 5. The pagination assertion cannot fail, so the control guarding the single-review invariant under load proves nothing + +- **Lands on:** STEP 2's done-when; STEP 10 (iii) inherits it +- **Plan line:** `:215` (the assertion), `:424` (the mutation) + +**Defect.** STEP 2 specifies `gh api --paginate`, and its done-when asserts against *"a +recorded TWO-PAGE listing carrying 30 unmarked reviews on page one and the marker on page +two"*. STEP 10 (iii)'s stated mutation is *"drop `--paginate` from the listing call"*. +`gh` merges pages itself. Measured on this fork with `per_page=2` across 9 pull requests — +five HTTP requests — piped into `json.load`: + +``` +=== does python json.loads accept it? === +OK, len 9 +``` + +One flat JSON array. A "recorded two-page listing" handed to `find_existing` is therefore +byte-indistinguishable from a single-page listing of 31 entries. + +**Concrete failure it allows.** Dropping `--paginate` still returns the marked id unless +the injected transport inspects the flag and truncates at 30 — which nothing in the plan +requires. STEP 10 (iii) reports PASS, its stated mutation is applied, and it still passes. +The pull requests most likely to have a human reader are the ones carrying more than 30 +reviews, and that is exactly where a duplicate second review appears. + +**Change.** State that the fixture is the **unmerged** per-page responses and that the stub +serves page two only when `--paginate` is present. Otherwise demote the assertion to a +code-shape check in STEP 2 — `--paginate` is present in the argv — and stop calling it +behavioural. + +### 6. The stdin document supplies no `repo`, but both lifecycle functions require one + +- **Lands on:** STEP 7; the signatures come from STEP 2 +- **Plan line:** `:329` (the document), `:184` `:200` (the signatures) + +**Defect.** `find_existing(pr, repo)` and `post_or_update(pr, repo, body)` each take +`repo`. STEP 7's document is `{pr, head_sha, merge_base_sha, stages, reports, containment}`. +`repo` appears nowhere in STEP 7. + +**Concrete failure it allows.** The implementer guesses. Hardcoding `launchpad-26/buzz` +makes the tool wrong the moment the fork is renamed and unusable from any other checkout; +reading `GITHUB_REPOSITORY` yields `None` outside Actions, so a local `--dry-run` builds the +path `repos/None/pulls/…`. STEP 10's controls inject a transport, so neither guess is caught +by any control in the plan. + +**Change.** Add `repo` to the stdin document, or state that it comes from +`GITHUB_REPOSITORY` with an explicit `--repo` override and a hard failure when both are +absent. + +### 7. Once containment arrives via `reports`, its evidence is dropped — and unfenced if added + +- **Lands on:** STEP 4 +- **Plan line:** `:271` +- **Contingent on finding 1's resolution** + +**Defect.** STEP 4's generic path renders *"`defect` and `failure` as separate lines"* and +nothing else; `entry_point` and `evidence` are rendered only through the separate-block +path. A containment finding converted per #117 carries `anchor: "pr"` with `file` and `line` +null — which is **valid**, so it does not reach the "malformed finding" heading either. + +**Concrete failure it allows.** Two halves. First, evidence is present in the input and +absent from the output — the detected-then-dropped case CONTAINMENT.md calls worse than +never detecting, and #120's control asserting *"the injection attempt appears in the +published review"* either fails or passes against a code path #117 never populates. Second, +if the implementer adds evidence to the generic path, nothing sizes the code fence. +`review.py:21-29` exists for exactly this: *"attacker text containing ``` would therefore +break out of a fixed three-backtick fence and corrupt every following section of the +review."* A payload containing a backtick run breaks the fence and the remainder of the +review renders as an unterminated code block. + +**Change.** State that the generic path renders `entry_point` and `evidence` whenever +present, and that evidence is fenced with `review.fence_for(evidence)` — imported, not +reimplemented. + +--- + +## Medium + +### 8. Six incomplete conditions are listed; two done-whens require seven, one requires six + +- **Lands on:** STEP 5, with STEP 6 and STEP 12 disagreeing +- **Plan line:** `:290-295` (six listed), `:304` ("seven"), `:451` ("six"), `:457` ("seven") + +**Defect.** STEP 5 enumerates exactly six conditions. Its own done-when says *"for each of +the seven conditions above"*. STEP 12's prose says *"the incomplete rule and its six +triggers"*; STEP 12's done-when says *"the seven incomplete triggers"*. The missing seventh +is real and lives in STEP 6 — *"A report with `status: complete` and no `outcome` is +incomplete per STEP 5"* — a condition STEP 5 never lists. + +**Concrete failure it allows.** An implementer building STEP 5 from STEP 5's list +implements six checks. STEP 6's done-when then fails, and the cheapest repair is to +special-case a missing `outcome` on the clean path alone — at which point a report with +`status: complete`, no `outcome`, and findings renders as a **complete** review. + +**Change.** Move the missing-`outcome` condition into STEP 5's list and make all four counts +say seven. + +### 9. `${{ github.run_id }}` is Actions expression syntax inside a Python module + +- **Lands on:** STEP 9 +- **Plan line:** `:388` + +**Defect.** STEP 9 tells the implementer to create the ref +`refs/heads/scope-probe-${{ github.run_id }}` from `check_publish_scope.py`. Python does not +interpolate that; only the workflow YAML would. + +**Concrete failure it allows.** Transcribed literally the ref name contains spaces and +braces, so GitHub answers on ref-name validity before evaluating permissions, and the +control FAILs for a reason unrelated to scope — inside the one step BUDGET already names as +most likely to overrun, where every iteration costs a commit, a push and a full run cycle. + +**Change.** Name `os.environ["GITHUB_RUN_ID"]` explicitly, and say the control fails loudly +when it is absent. + +### 10. Updating the oldest duplicate leaves the newest one permanently stale + +- **Lands on:** STEP 2 +- **Plan line:** `:185` + +**Defect.** `find_existing` returns the id of the **oldest** marked review and reports the +count when there is more than one. `DELETE` cannot remove a submitted review, so the newer +duplicate can never be retired. + +**Concrete failure it allows.** If duplicates ever occur — and the plan's own reasoning +admits a race can produce them — the review a human reaching the bottom of the timeline +reads last is the stale one, describing a commit that no longer exists, permanently. + +**Change.** Update the **newest** marked review and report the count, so the current body is +the last one on the page. Either choice needs stating; the reasoning at `:186` justifies +surfacing the count, not the choice of which review to keep current. + +### 11. Nothing specifies what `post_or_update` does when the PUT fails + +- **Lands on:** STEP 2 +- **Plan line:** `:200` + +**Defect.** `PUT /pulls/{n}/reviews/{id}` requires review authorship. The +marker-over-author decision at `:180` was made *because* #110 commits to revisiting the +credential — and a credential change is exactly when `find_existing` matches a review the +new identity cannot edit. The plan never says what happens on a 403 or 404 from PUT. + +**Concrete failure it allows.** The implementer picks one of two behaviours, and one of them +breaks the requirement this issue exists for: raising means no review at all, and falling +back to POST means a second review on the pull request. + +**Change.** State the behaviour. A fallback POST must be explicitly forbidden, or explicitly +allowed with the duplicate reported. + +### 12. STEP 10's recorded inputs are produced by no step's done-when + +- **Lands on:** STEP 10; the fixtures should be deliverables of STEP 1 and STEP 2 +- **Plan line:** `:418` (ii), `:424` (iii), `:171-174` (STEP 1's done-when) + +**Defect.** STEP 10 (ii) uses *"the recorded listing from STEP 1"*, but STEP 1's done-when +produces four operation responses — POST, PUT, DELETE, dismissal — and no reviews listing. +STEP 10 (iii) uses *"the recorded two-page listing from STEP 2"*, which STEP 2 exercises but +is never required to save as a file. + +**Concrete failure it allows.** The implementer hand-writes both fixtures, so the controls +assert against invented API output rather than recorded output. Finding 5 shows the +hand-written two-page listing cannot test what it claims to. + +**Change.** Name both fixtures as deliverables of the steps that record them, with paths. + +### 13. STEP 1 records four API responses and never records whether an edit is visible to a human + +- **Lands on:** STEP 1 +- **Plan line:** `:158` + +**Defect.** The whole strategy turns "re-review on push" into a body edit. STEP 1 captures +status, body and command for four calls and captures nothing about what a human sees. OPEN +at `:534` names the frozen `submitted_at` and `commit_id`, but not whether an edit produces +a timeline entry or reaches anyone's inbox. + +GitHub's notification behaviour on a review-body edit was **not** verified by this review — +doing so means writing to a public fork, which is STEP 1's job, not a reviewer's. That is +the finding: the plan's most load-bearing behavioural assumption is the one thing STEP 1 +does not record, and STEP 1 is the cheapest place in the plan to record it. + +**Concrete failure it allows.** Every re-review after the first is potentially silent: the +body is current and no reader is told. The feature's entire value is a human reading the +review. + +**Change.** Add to STEP 1's done-when: after the PUT, capture the PR timeline and note +whether the edit appears, so whoever decides #121 has evidence rather than an inference. + +--- + +## Low + +### 14. MARKER in `publish.py` plus `main` in `publish.py` makes the two modules a circular import + +- **Lands on:** STEP 4's done-when, biting at STEP 7 +- **Plan line:** `:279` (the requirement), `:178` (MARKER's home) + +**Defect.** STEP 4's done-when requires *"the body's first line is publish.py's MARKER"*, so +`publish_render` imports `publish`. STEP 7 then puts `main` into `publish.py`, which imports +`publish_render`. + +**Concrete failure it allows.** `python3 publish.py --dry-run` raises +`ImportError: cannot import name … from partially initialized module`. STEP 7's own +done-when catches it on the first run, so this costs a fix round rather than shipping — but +it is a decision the plan can make now. + +**Change.** Put MARKER in `publish_render.py`, or a third module, and have `publish.py` +import it. + +--- + +## What was looked for and not found + +- **A fail-open default in the incomplete classifier.** `:302` is explicit and correct — + unclassifiable input is incomplete. STEP 6's `outcome`-versus-empty-array distinction is + right too. +- **Ordering degrading across an update.** `:262` is sound: the body is rebuilt wholesale, + ties are broken to a total order, nothing is appended. "Most severe first survives an + update" holds, subject to finding 2. +- **An event parameter that could one day carry APPROVE.** `:203` removes it by + construction, which is the correct answer to AGENTS.md §5 rule 1. +- **`gh api --paginate` emitting concatenated arrays.** Expected, and false on `gh 2.93.0` — + pages merge into one array. The plan's `--paginate` choice is right; only its test is + unfalsifiable. +- **A null review body crashing `body.startswith(MARKER)`.** Every review on PR 86 returns + `body` as a string. A reachable null body could not be demonstrated, so it is not + reported. +- **Fork detection.** `head.repo.full_name` versus `github.repository` is correct, and a + deleted fork repository — `head.repo` null — fails toward the skip. +- **STEP 8's `permissions`, `concurrency` and `pull_request_target` reasoning.** All three + are right. The group-interpolation check at `:376` is a genuinely good test: it fails a + fixed group name rather than passing it. +- **The severity-field tension and the frozen `commit_id`.** Both already in OPEN. Not + findings. + +## Tools the reviewer actually had + +Full session pool. Used: `Read`, `Bash` (`gh api`, `git`, `python3`, `jq`, `grep`, `sed`, +`stat`), `Skill`. RepoQL MCP tools were available and unused. Every assertion about +`render_review`, `gh --paginate`, `check-plan.sh` and #117's contract was executed in +session, with output quoted above. + +```findings +Blocker launchpad/plans/2026-08-12-issue-119-publish-one-review.md:90 #117 now declares `evidence` and carries containment inside reports, so the separate containment block is never emitted and STEP 5 marks every review incomplete +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:257 bare SEVERITY_ORDER subscript raises KeyError on an out-of-ladder severity and posts nothing at all +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:293 nonce half of the completion-marker check is unimplementable — no nonce reaches publish.py, and its done-when tests an impossible input +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:243 render_review has no `unreadable` parameter; the prescribed call raises TypeError and the containment block requires a key with no consumer +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:215 pagination assertion cannot fail — gh --paginate merges pages into one array, so the two-page fixture cannot distinguish a paginating implementation +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:329 stdin document supplies no `repo`, but find_existing and post_or_update both require one +High launchpad/plans/2026-08-12-issue-119-publish-one-review.md:271 containment evidence is dropped by the generic render path, and unfenced if added, so a backtick payload breaks out of the fence +Medium launchpad/plans/2026-08-12-issue-119-publish-one-review.md:304 six incomplete conditions are listed while two done-whens require seven; the seventh lives only in STEP 6 +Medium launchpad/plans/2026-08-12-issue-119-publish-one-review.md:388 `${{ github.run_id }}` is Actions expression syntax and is not interpolated inside a Python module +Medium launchpad/plans/2026-08-12-issue-119-publish-one-review.md:185 updating the oldest marked review leaves the newest one permanently stale and undeletable +Medium launchpad/plans/2026-08-12-issue-119-publish-one-review.md:200 no specified behaviour when PUT fails, and the committed credential change makes a 403 likely +Medium launchpad/plans/2026-08-12-issue-119-publish-one-review.md:418 STEP 10's recorded inputs are produced by no step's done-when, so the fixtures will be hand-written +Medium launchpad/plans/2026-08-12-issue-119-publish-one-review.md:158 STEP 1 records four API responses and never records whether a body edit is visible to a human +Low launchpad/plans/2026-08-12-issue-119-publish-one-review.md:279 MARKER in publish.py plus main in publish.py makes publish and publish_render a circular import +``` +REVIEW COMPLETE + +--- + +## Outcomes — applied to the plan 2026-08-13, after #117's contract settled + +#117 settled containment transport the OPPOSITE way to what finding 1 concluded: +containment findings do **not** enter any dimension report's `findings` array; they travel +as a top-level `containment` sibling key carrying raw `contain.Finding` plus a seven-key +`states` map. The plan's separate block was right all along. + +| # | Severity | Outcome | Note | +|---|---|---|---| +| 1 | Blocker | **WITHDRAWN** | Premise falsified by the settled contract — #117 kept containment out of `reports`. Residue was real and is fixed: ALREADY TRUE claimed a nine-field record and no `evidence`; the record has ten fields and `evidence` is the tenth. | +| 2 | High | Fixed | STEP 4 sorts with `.get(severity, 9)`; an out-of-ladder severity renders under "malformed finding", sorts last, and triggers the banner. | +| 3 | High | **Partly fixed, flagged upstream** | The nonce is in no field of any envelope or of the merged document, so no downstream stage can verify it. STEP 5 now rejects reports that *disagree* on the nonce, which catches any single forged marker. An all-forged run is not detectable here and the plan says so. A `nonce` key on #117's merged document is the clean fix; raised in OPEN. | +| 4 | High | Fixed | The `unreadable` key is gone from the block; `states` is passed and `unreadable` derived, as `review.py` does. | +| 5 | High | Fixed | `find_existing` takes an injected transport; STEP 1 records the **unmerged** page bodies; the stub serves page two only when `--paginate` is in the argv, so "drop `--paginate`" now fails. | +| 6 | High | Fixed | `repo` comes from `--repo`, defaulting to `GITHUB_REPOSITORY`, hard failure when neither is set. The stdin document stays the six keys #117 documents. | +| 7 | High | **Re-derived, now larger** | The containment half dissolved — that block goes through `render_review`, which fences and escapes. But `evidence` is RAW on the ten-field record too, and `entry_point` is *required* on injection findings, which #117's cross-cutting clause makes all three dimensions emit. So raw attacker text arrives via the ordinary findings path. STEP 4 now renders it, fences with `review.fence_for` and escapes with `contain.escape`, and STEP 10 (vi) proves it with a four-backtick payload. | +| 8 | Medium | Fixed | Ten conditions, numbered, counted the same in STEP 5, STEP 6 and STEP 12. The missing seventh (a `complete` report with no `outcome`) moved from STEP 6's prose into the list as (8). | +| 9 | Medium | Fixed | `os.environ["GITHUB_RUN_ID"]`, with a stated failure when absent. | +| 10 | Medium | Fixed | Keeps the **newest** marked review current, since the oldest cannot be deleted and the newest is what a reader sees last. | +| 11 | Medium | Fixed | A failed PUT raises with id and status; a fallback POST is forbidden by name, and STEP 10 (vii) mutates for it. | +| 12 | Medium | Fixed | `fixtures/reviews-listing.json` is a named STEP 1 deliverable; STEP 10 cites it by path. | +| 13 | Medium | **Converted to an observation** | STEP 1 now captures the PR timeline after the PUT and records in one line whether the edit is visible at all. GitHub's notification behaviour is still unverified — deliberately, since verifying it means writing to a public fork, which is STEP 1's job. | +| 14 | Low | Fixed | `publish.py` owns MARKER and passes it to `render_body` as its first argument; neither module imports the other at module level, and both directions are asserted. | + +**Structural lesson applied.** The OPEN register listed renames and removals. What broke the +plan was an **addition** — `evidence` and the `containment` key. Additions are now in the +register, and a new field on the record, envelope or merged document obliges a re-read of +the contract before STEP 4 is built. + +`check-plan.sh` on the revised plan: `Mechanical checks: clean. Your call on the substance.` + +--- + +# Second and third passes — 2026-08-13 + +Two further passes ran on the revised plan. + +**Pass 2 — NOT independent.** Run by the author of the revision, which the skill names as +the wrong arrangement. Eight findings, four resting on measurement rather than judgement. + +**Pass 3 — independent.** A separate reviewer on a different model (Sonnet 5), in a clean +context, blocked from reading this file so it could not be anchored to pass 2's conclusions. +Three findings. + +## The intersection was EMPTY + +Zero overlap between the eight and the three. Same plan, same primary sources, disjoint +results — pass 3 hunted the incomplete/clean classifier and interface threading, pass 2 +hunted the text the revision had changed. + +**The operative lesson: "apply only what both passes agree on" would have applied nothing, +and would have discarded a Blocker.** Agreement is the right gate for judgement calls. It is +the wrong gate for a claim a command has already falsified. The rule used instead was: apply +what either pass proves with output, plus what both agree on by judgement. + +Pass 3 did corroborate four of pass 2's *verifications* — `gh --paginate` merging pages, +`ENTRY_POINTS` = 7, review.py's current line numbers — and added two useful ones: a review +object has no `updated_at` field (supporting the edit-invisibility concern), and +`GET /issues/{n}/timeline` works (supporting STEP 1's new observation). + +## Applied + +| From | Severity | Finding | Resolution | +|---|---|---|---| +| Pass 3 | **High** | `stages` was defined as covering only envelope-less stages, while condition (7) and STEP 6's done-when both needed dimension names *in* it. Built literally, a three-dimension run producing two reports rendered as COMPLETE. | `stages` now names every stage including the three dimension slugs, with the reason: a report cannot testify to its own absence. | +| Pass 3 | Medium | `merge_base_sha` threaded through `render_body` and the stdin schema with no consumer and no test. | Both SHAs are now rendered in a header line, with a done-when that fails if `head_sha` is written into both slots. Kept rather than removed: findings anchor to new-side lines in the merge-base diff, so the head alone does not identify the diff. | +| Pass 3 | Low | ALREADY TRUE cited HEAD as `d897a06e8`. | Replaced with a dated observation. `eb2bf09d0` was HEAD at commit time; `8a405a9f5` landed mid-revision from another session in this worktree. | +| Pass 2 | **Blocker** | Escaping `defect`/`failure` with `contain.escape` — a rule pass 2 itself had added, justified by an unchecked claim. `ESC` is a tilde, so `~/.claude/settings.json` → `~~/.claude/…`, and `~~…~~` is GFM strikethrough. | Withdrawn. Only `contain.TOKEN` is replaced in prose. Evidence stays escaped (matching `render_review`), and STEP 12 now says the excerpt shows a doubled tilde so no reader mistakes it for the author's text. | +| Pass 2 | High | STEP 4 `[independent]` asserted the incomplete banner that STEP 5 `[needs 4]` builds — a deadlock, and the same defect #117's second pass found in four steps. | Both clauses moved to STEP 5's done-when. | +| Pass 2 | High | STEP 1 required a recorded two-page listing; measured, PRs 86/124/126 carry 1, 0 and 1 reviews, so "padded to 30 entries" meant hand-authored. | The fixture now holds two labelled artefacts — one recorded, one explicitly constructed — and no assertion may cite the constructed one as evidence about GitHub. | +| Pass 2 | High | Condition (10) as worded was met by `len(states) == 7`, passing for six real keys plus a typo. | `set(states) == set(contain.ENTRY_POINTS)`, imported, with the difference named in both directions. | + +## Held, not applied + +Four judgement-only findings from pass 2, none corroborated: fencing the malformed-finding +raw record; the unnecessary ban on `publish.py` importing `publish_render` (pass 3 examined +this design and judged it self-consistent — it is right that it is not a *contradiction*; +the narrower point that the ban is unneeded and `main`'s import site is unstated survives but +is weaker); condition (5) being vacuous below two reports; and STEP 8's missing +`GITHUB_REPOSITORY` note. + +`check-plan.sh` after applying: `Mechanical checks: clean. Your call on the substance.` + +--- + +# Fourth pass reconciled — 2026-08-13 + +A fourth record exists: `2026-08-13-119-plan-review-second-pass.md`, written by the +`feat/review-agent-dimensions` session against the plan **as committed at `8a405a9f5`** — the +pre-revision text. It declares its own partial non-independence: it wrote #117's containment +settlement, so its reading of anything touching that interface is interested. Its three +surviving findings do not touch it. + +Reconciled against the current text by the author of the revision — **not a neutral party**, +which is why every verdict below is anchored to a command output rather than to a judgement. +All three confirmed. All three applied. + +## Applied from the fourth pass + +**Blocker — `find_existing` matched attacker-writable text with no author filter.** +`grep -c "user.login|author_association|GET /user"` returned **0** across the whole plan. Any +user with read access can submit a `COMMENT` review on a public-fork pull request and write its +body, marker included. So an outsider posts a marked review, `find_existing` returns their id, +the PUT is refused, and the raise-never-POST rule means the agent publishes nothing on that +pull request permanently — presenting as an HTTP error, not as an attack. + +**This finding indicts two of this file's own fixes.** Finding 10 changed oldest→newest, which +removed the timing requirement entirely (planting at any moment now wins every subsequent run); +finding 11 made the denial deterministic. The diagnosis for finding 10 was right and the fix was +incomplete, because it was not weighing an adversary. Conceded. Now: filter by author FIRST, +then newest among the agent's own, with the login resolved at runtime from `GET /user` — which +preserves the credential portability the marker-over-author choice existed for. Foreign marked +reviews are counted, never candidates. STEP 10 gains assertion (ix) with the dropped-comparison +mutation, and the MARKER bullet's "identification is by marker, NOT by author" rationale is +corrected: what must not be hardcoded is the *name*, not the comparison. + +**Blocker — the live credential control measured the wrong token.** Verified on #120's branch +at `c64ff7958`, where the controls workflow is committed: + +``` +# Read-only, and no write scope of any kind. +permissions: {contents: read, issues: read, pull-requests: read} +48: run: python3 run_controls.py +``` + +STEP 11 registers the scope control into that runner. Both tokens carry `contents: read`, so +the ref-create returns 403 either way and the control PASSes regardless of the publish +workflow's permissions — with the 403 body pasted into the PR as evidence about a credential it +never touched. The "no token outside Actions" SKIP never fires, because inside the controls +workflow there *is* a token; it is the wrong one. Fixed: the live half asserts `GITHUB_WORKFLOW` +and SKIPs — never PASSes — outside the publish workflow; STEP 8 declares the step that runs it +there; STEP 11 states that registration is not the control's home; the done-when now requires +the workflow name pasted alongside the 403. + +**High — no control asserted that a clean run posts.** STEP 6 offered +`grep -n "return None" publish_render.py` — a grep of the *pure renderer*, whose `-> str` +contract means no correct implementation ever returns `None`, so it passed for every +implementation, while the actual decision to post lives in `publish.py`'s `main`. Withdrawn and +replaced by STEP 10 (viii): an all-clean fixture drives `main` through the transport, which must +record exactly one write, with the mutation `if not findings and not incomplete: return 0`. + +## Also applied + +**STEP 1 gains a fifth recorded response** — a PUT against a review the token did not write. +Three passes left "does GitHub refuse a cross-author PUT?" unverified because settling it means +writing to a public fork. STEP 1 already does exactly that, under a human token whose identity +differs from the workflow's, so the probe is nearly free. If GitHub *allows* it, that is a +larger finding than the one the filter was written for and reopens STEP 2. + +## Where the fourth pass corrected this one + +- **Finding 10's fix was wrong on its own.** Conceded above. +- **Finding 9 (`${{ github.run_id }}`) — it conceded to this pass**, on the reasoning that a + documented risk *class* does not excuse a prescribed instance of it. +- **Finding 1's withdrawal** — it agrees, and discloses it wrote the settlement that caused it, + so its agreement is not independent evidence. + +## Corroboration across passes + +`gh api --paginate` merging pages was measured independently twice — `per_page=2` over 9 pull +requests here, `per_page=1` over 12 commits there — reaching the same conclusion. The bare +`SEVERITY_ORDER[...]` subscript and the incomplete-trigger miscount were each found twice. + +## Still outstanding + +- **Cross-issue, High — the nonce is verifiable by no downstream stage.** The clean fix is a + `nonce` key on **#117's** merged document. Both passes agree it belongs there; neither edited + #117 on the strength of its own review. Still owed. +- **Four judgement findings from this file's second pass**, held on the reader's call: + malformed-record fencing, the unneeded `publish_render` import ban, condition (5) being + vacuous below two reports, STEP 8's `GITHUB_REPOSITORY` note. + +`check-plan.sh` after applying: `Mechanical checks: clean. Your call on the substance.` +Plan is 1222 lines, 12 steps, one run-marker. + +--- + +# The four held findings — applied 2026-08-13 + +The second pass's Tier C, held pending the independent pass and then applied on the reader's +instruction. None was corroborated by another pass; one was partly contested. + +**Malformed-record fencing (Medium).** The "malformed finding" heading is the path a record +takes BECAUSE its fields did not match the contract, so it is the last place to assume any +field is well-formed — and nothing said how to render it. Now: serialised, escaped with +`contain.escape`, wrapped in a `review.fence_for`-sized fence sized off the whole serialised +record rather than any single field, with its own done-when clause using a four-backtick +`defect`. The no-fixed-fence control's intent is also spelled out: it bans a *literal* fence, +and `fence_for` builds its own from `chr(96)` at runtime, so the two rules do not collide. + +**The import ban (Medium) — partly contested.** The fourth pass examined this design and +judged it self-consistent; it is right that it was not a *contradiction*. The narrower point +stood: a cycle needs both edges, and passing the marker in had already removed the +`publish_render → publish` edge, so banning `publish.py` from importing `publish_render` at +module level forbade the normal thing and left `main`'s import site unstated. Now one direction +is banned and the other is explicitly normal. + +**Condition (5) is vacuous below two reports (Medium).** Nonce agreement needs siblings to +disagree with, so a single-dimension run — or a three-dimension run where two came back +`failed` — has no nonce checking at all, precisely when the run was already degraded. Stated in +the plan rather than left for a reader to find, and tied to why the upstream `nonce` key matters +rather than treated as sufficient. + +**STEP 8's `GITHUB_REPOSITORY` note (Low).** Actions sets it for every run, so the publish +workflow needs no `--repo`. Stated, with the reason a flag must not be hardcoded there. + +## One defect introduced and caught in the same pass + +Applying the import fix put `grep ... publish_render.py` into STEP 2's done-when — a step that +does not depend on STEP 4, where that file is created. The grep would have "passed" by the +file's absence rather than by its contents: the exact defect class four passes have now found +in this plan and #117's. Corrected before staging; the assertion lives in STEP 4's done-when, +and STEP 2 asserts only what it can produce, namely that `publish.MARKER` exists after import. + +Worth recording plainly: that is five separate instances of a done-when reaching past its own +dependency tag across two plans. It is the single most repeated defect in this project's +planning, and it survives review passes because it reads as thoroughness. + +`check-plan.sh`: `Mechanical checks: clean. Your call on the substance.` Plan is 1269 lines, +12 steps, one run-marker. + +--- + +# The nonce gap closed upstream — applied 2026-08-13 + +Finding 3 of the first pass — the marker nonce being unverifiable by any stage downstream of +#117's runner — was raised on #117 rather than worked around here. **#117 closed it** by +adding a top-level `nonce` key to its merged document, and its change list names this plan +directly: *"#119 should add `nonce` to its STEP 7 stdin document as a sixth key and check +each marker against it."* + +Four changes applied: + +1. **STEP 5 condition (5)** — now *a report's marker nonce differs from the merged document's + `nonce`*, with marker-to-marker disagreement kept as a secondary check. +2. **STEP 5 done-when** — the "another dimension's nonce" fixture is gone, and the step now + forbids it by name. #117 corrected this plan on the point: there is **one nonce per run**, + so that phrasing describes an input the contract cannot produce, and a test built on it + passes against any implementation. It had survived two revisions here. +3. **STEP 7** — stdin document gains `nonce` as a seventh key (#117's six plus this plan's + own `stages`). +4. **OPEN and STEP 12** — the concern is recorded as discharged rather than deleted, and the + prose is bounded to what the key actually proves. + +## The limit, kept deliberately + +#117 draws a line this plan had blurred: 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 nothing at this stage can tell. STEP 12 now forbids describing the published review as +unforgeable, in those words. + +## What also improved for free + +The single-report blind spot is gone. The previous agreement-only check needed siblings to +disagree with, so a one-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. + +## The pattern, twice now + +This is the second time on this issue that flagging a gap upstream produced a **contract +change** rather than a local workaround — the first being the `containment` sibling key. +Both times the alternative was #119 guessing at a shape #117 owned. Worth keeping: the cost +of raising is one comment; the cost of guessing is a contract mismatch discovered at +integration. + +`check-plan.sh`: `Mechanical checks: clean. Your call on the substance.` Plan is 1389 lines, +12 steps, one run-marker. + +--- + +# Fifth finding — `GET /user` under an installation token, applied 2026-08-13 + +Raised by the second-pass reviewer as an ACTION note after `d97368d79` had already landed: +`2026-08-13-119-ACTION-get-user-under-actions-token.md`. It confirms the two Blocker fixes in +that commit are correct and says not to revisit them; the defect is in the **mechanism** the +identity fix used, not the decision. + +## The claim, and what could be verified + +The plan resolved the agent's own login "at runtime from `GET /user`". The reviewer's claim: +`GITHUB_TOKEN` is an installation credential and `GET /user` has no authenticated user under +one, so the call is refused inside Actions. **It flagged this as unverified** — no installation +token exists outside a real Actions run. + +Verification attempted from primary sources, and only half closed: + +- GitHub's REST documentation for "Get the authenticated user" lists **OAuth app tokens and + personal access tokens (classic)**. Installation access tokens are not among the supported + types. +- The automatic-token-authentication page does **not** state what kind of token `GITHUB_TOKEN` + is, so the chain could not be closed from documentation alone. + +**The fix does not depend on settling it, which is why it was applied anyway.** If the call is +refused, the committed mechanism 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. Supply-and-verify wins either way. Same shape as the `unreadable=` keyword: +right against what was tested, wrong against what ships. + +## Applied + +- **STEP 2** takes the expected login as a **parameter** and compares it to each review's + `user.login`. `GET /user` is rejected by name, with the documentation cited and the 403 + explicitly NOT claimed as proved. An unresolved identity **raises** — it never degrades to + marker-only matching, which would silently restore the vector the filter closes. +- **STEP 7** wires `--as `, defaulting to `github-actions[bot]`. A default value on a + flag is not a hardcoded comparison: when #110 moves the credential it is one flag, not a code + change. The default is evidence-based — all 153 bot comments in this fork are authored by + `github-actions[bot]`, with no other bot login present. +- **STEP 9** gains a third assertion: inside the publish workflow, the configured login must + equal the login the credential actually posts as, read from the response of the review the run + just published. This is the only place in the plan where that can be checked, and it is what + catches a stale default. Without it the filter degrades quietly — a login that no longer + matches means find_existing matches nothing, every run POSTs, and the pull request accumulates + one review per push, which is the failure #119 exists to prevent arriving through the fix for + a different one. +- **STEP 1** records two more facts at no extra cost: the `user.login` of the review it POSTs, + fixing the shape of the field STEP 2 filters on; and the `GET /user` status and body under its + own token, with a note that this is a human token and says nothing about the Actions + credential. +- **STEP 12** states supply-and-verify normatively, replacing the withdrawn runtime-discovery + rule. + +## Corrected against the reviewer's own proposal + +It suggested defaulting `--as` to "a value STEP 1 measures". STEP 1 runs under a **human** token, +so it measures the wrong identity — the workflow's can only be observed inside a real Actions +run. Hence the default lives on the flag and STEP 9 verifies it against the live credential, +rather than STEP 1 supplying it. + +## A sixth instance of the same defect, caught before commit + +Applying this put `--as someone-else` into STEP 2's done-when — but argument parsing lives in +`main`, which STEP 7 creates, so STEP 2 cannot exercise a flag. Split: STEP 2 takes and asserts +a parameter, STEP 7 owns the flag. + +That is now **six** occurrences of a done-when reaching past its own dependency tag, across two +plans, three of them written while fixing something else. It is the most repeated defect in this +project's planning and it survives review because a done-when that checks more reads as more +thorough. A `check-plan.sh` rule flagging any done-when that names a file or step outside the +step's own `[needs]` closure would catch all six mechanically. + +`check-plan.sh`: `Mechanical checks: clean. Your call on the substance.` Plan is 1355 lines, +12 steps, one run-marker.