Repository navigation
feat(review-agent): publish exactly one PR review, re-review on push - #1460
Conversation
This plan predates #118 by six weeks and was written when adjudication was "not started" -- every reference to it was necessarily speculative. #118 merged 2026-08-25 (PR #1406), so this revision replaces speculation with the real, verified contract and closes the two rendering decisions #118's own OPEN section explicitly deferred to whoever builds #119. Confirmed with Serina before writing this revision, per the plan skill's own "ambiguity is surfaced, not resolved" rule -- both were genuinely open, one of them named as such by #118 itself: - verdict_evidence is NOT rendered, only the verdict word (CONFIRMED/ REFUTED/UNPROVEN). #118's own FALSIFIABILITY.md recorded a live instance of a judge producing approval-shaped text inside verdict_evidence once its escalate-only guard was removed; rendering that text here would publish it verbatim with nothing in #119's scope to filter it. - Duplicate findings collapse to their survivor with a dimension count, rather than rendering once per duplicate with a cross-reference. Changes, all traced to #118's actual merged code (verdicts.py, ADJUDICATION.md), not recalled from the earlier speculative reading: - ALREADY TRUE: #118's six added per-finding fields and nine-key adjudication block, verified against the merged source. States plainly that adjudication.total_refutation needs no new incomplete-condition in STEP 5 -- the existing "stage status != complete" check already covers it, because #118 was deliberately built so the adjudication stage's own manifest entry goes non-complete on a total refutation, per ADJUDICATION.md's own stated reasoning. - STEP 4: render_body gains a duplicate_groups parameter, defaulted empty; renders the verdict word beside every finding, never verdict_evidence; collapses duplicates to their survivor with a count. Done-when gains matching assertions, including a negative check that verdict_evidence never appears in the output. - STEP 7: the stdin document gains an eighth key, adjudication, passed through to STEP 4's new parameter; optional, matching how containment was optional before #117 settled. - STEP 12: PUBLISHING.md's required content gains the two rendering decisions and their reasons, and the total_refutation coverage note. - OPEN: the severity-duplication concern this plan explicitly declined to resolve ("either the contract needs a second field or the sentence needs to go... because #118 will honour the same contract") is marked DISCHARGED -- #118 added reported_severity precisely so severity could be overwritten in place without losing the original, exactly as #117 required. A new field/key register extends #117's existing one to #118's contract, naming which steps each of #118's fields affects and which five keys (verdict_counts, findings_in, findings_out, downgrades, notes) nothing here reads at all. Mechanical gate clean: 12 steps (cap held), one RUNS HERE, every step has a done-when and a dependency tag, all six sections present. Refs #119, #109 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ior (#119) Live measurements against throwaway PR #1421 (branch throwaway/119-review-lifecycle-probe), under a human gh auth token, not the workflow credential. Confirmed: POST a COMMENT review succeeds, state COMMENTED, numeric id. PUT a new body on that id succeeds (200); commit_id and submitted_at stay frozen at the original values despite the body changing -- the exact property the plan's whole "POST once, PUT thereafter" strategy depends on. DELETE on a submitted review returns 422 "Can not delete a non-pending pull request review" -- a submitted COMMENT review genuinely cannot be removed. GET /user succeeds under this human token (login: serina-mcfall); says nothing about the Actions/installation credential, which cannot be tested from a checkout. Also confirmed, and load-bearing for #121's later judgement: the PR timeline shows exactly ONE "reviewed" event despite two writes (POST then PUT) to the same review -- an edit is silent to anyone watching the timeline. fixtures/reviews-listing.json holds the genuine single-page response alongside a clearly-labelled CONSTRUCTED two-page listing (30 real-object copies on page one, the real marked review on page two only), built the same way this plan's own ALREADY TRUE already measured gh's pagination merging elsewhere on this fork -- never called "recorded". NOT TESTED, disclosed rather than fabricated: the cross-author PUT. It needs a foreign-authored review already on the target PR, and this session has exactly one GitHub identity with no safely-disposable foreign review to target -- attempting it against an existing real PR risked overwriting another cohort member's actual review. STEP 2's find_existing/ post_or_update code fails closed on this scenario regardless of which way GitHub's answer falls, so this does not block STEP 2's construction, but the measurement itself remains genuinely open for whoever next has a second identity available. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
find_existing/post_or_update, identified by MARKER AND author (never marker alone, which is attacker-writable), newest-under-login wins, paginated listing, hard-fail PUT with no POST fallback, and a foreign-marker refusal that fails closed rather than risking a silent duplicate review. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…ve PR Real throwaway PR #1424 (launchpad-26/buzz, closed without merging): first post_or_update call created review 5015209676; pushed an empty commit; second call updated the SAME review id, naming the new head SHA. Exactly one marked review survives, state COMMENTED. Behavioural evidence only -- this runs under a human token, not the workflow's pull-requests:write credential (that is STEP 9's). Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Pure render_body(marker, reports, stages, containment, head_sha, merge_base_sha, duplicate_groups=()). Severity-ordered, byte-identical for identical input, both SHAs rendered. Verdict word beside every finding, verdict_evidence never touched. Duplicates collapse to their survivor with a dimension count; a group naming an absent survivor renders its real duplicate as malformed rather than dropping it. Evidence and malformed records are escaped via contain.escape and fenced via review.fence_for (both imported, never reimplemented); defect/failure get only the envelope-token neutralised, since the full escape corrupts ordinary tilde-bearing prose. No import from publish.py, no fixed fence, no local SEVERITY_ORDER. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
render_body renders an "## Incomplete" banner above everything else when any of ten conditions holds: a manifest stage not complete, a failed report, a missing/mispositioned completion_marker, a marker naming the wrong dimension, a marker nonce mismatch (against the document's nonce, or between sibling reports), findings_count disagreeing with the findings list, a manifest dimension with no matching report, complete-status with no outcome, containment absent/unparseable, and containment.states not naming exactly the seven entry points (a set comparison, not a count -- six real keys plus a typo still counts to seven). Two conditions inherited from STEP 4 (out-of-ladder severity, containment=None) also trigger it. The default is incomplete: anything this cannot classify adds a reason rather than passing silently. render_body gains an optional `nonce` parameter (the merged document's run nonce, #117) since condition 5 is the first thing that needs it -- STEP 4 never supplied one, so it defaults to None and existing callers are unaffected. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
An all-clean, non-incomplete document (every stage complete, every report outcome: clean) now renders an explicit sentence -- the head SHA already sits in the header, plus the name of every dimension the manifest ran and a stated no-confirmed-findings line -- rather than silence. Silence would be indistinguishable from a crashed agent. status: complete with no outcome (condition 8) still renders the STEP 5 banner, never this sentence -- the two are checked to be mutually exclusive rather than assumed to be. Whether the clean path actually POSTS is main's decision in STEP 7/10, not this renderer's; not claimed here. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
publish.py gains main(): reads the seven-or-eight-key stdin document
({pr, head_sha, merge_base_sha, stages, reports, containment, nonce,
adjudication}), renders it through publish_render (passing
adjudication.get("duplicate_groups", ()) when the optional key is
present, () when it is not), and calls post_or_update. --dry-run prints
the body and never calls post_or_update -- verified directly, not
inferred from output.
--repo defaults to GITHUB_REPOSITORY and exits non-zero naming the
problem when neither is set, rather than building a repos/None/... path
that 404s silently. --as defaults to github-actions[bot] and exits
non-zero on an explicitly empty login rather than falling back to
marker-only matching. Malformed stdin JSON exits non-zero before any
network call.
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
- render_body no longer crashes (TypeError) when reports is None or not
a list; guarded the same way _incomplete_reasons already was, so a
missing reports key renders the incomplete banner instead of dying
before publish.py ever reaches post_or_update.
- condition (7)'s "produced no report" reasons now sort deterministically
(a bare set iteration order depends on PYTHONHASHSEED) -- was breaking
the renderer's own byte-identical-output contract across process runs.
- main() validates document["pr"] is an int before calling post_or_update,
matching the existing --repo guard's reasoning: a silent None builds
repos/{repo}/pulls/None/reviews, which 404s in a way that reads like a
missing PR rather than a malformed document.
- a containment finding missing kind/entry_point/evidence now folds into
"containment unparseable" (condition 9) instead of crashing render_body
with an uncaught KeyError during Finding reconstruction.
Found by a review-code pass (GATES: "after STEP 7... catches the
lifecycle and renderer while they are still small").
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
A separate workflow from the containment-controls one, since this job holds pull-requests: write and that one must not. pull_request_target (never plain pull_request, which would hand a same-repository PR's own diff the write token that then executes it), no ref: override on checkout so the base-ref code always runs, concurrency keyed on the PR number with cancel-in-progress, and a job-level guard (not a step that exits 0) that skips the whole publish job on a cross-repository PR. The publish step is written against the CLI shape #116/#117/#118 each name for themselves -- those plans are unmerged as of this revision, so this workflow does not run successfully yet; it exists so a re-check happens as each upstream stage actually merges. check_publish_scope.py (STEP 9) runs in the same job, after publish, so the live half of that control executes under the token it claims to measure. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…control
Three assertions. STATIC parses the workflow YAML and checks permissions
== exactly {contents: read, pull-requests: write}, the trigger is
pull_request_target (never plain pull_request), and no checkout step
overrides ref: toward the PR head -- each proven against a mutated copy
that must fail where the real file passes. LIVE attempts a contents write
(create a ref, tied to GITHUB_RUN_ID, never the literal Actions
expression string) under the workflow's own token and asserts HTTP 403;
any other outcome -- including an unexpected success, which also deletes
the ref it made -- is FAIL, never PASS on some other error. IDENTITY
compares the configured --as login against post_or_update's own
author_login, the only place the live identity exists.
Both LIVE and IDENTITY report SKIP, never PASS, unless GITHUB_WORKFLOW
names this exact publish workflow -- #120's containment-controls
workflow also carries a real (but different, read-only) token, so a
guard keyed only on "a token exists" would report PASS having measured
the wrong credential.
Wiring gap found while building this: STEP 7/8 never threaded
post_or_update's author_login from the publish step into the scope-check
step, which STEP 9's identity assertion needs. Fixed here: publish.py
writes author_login to $GITHUB_OUTPUT when present, and the workflow
passes steps.publish.outputs.author_login into check_publish_scope.py's
env as PUBLISH_OBSERVED_LOGIN.
The two done-when items that require a real Actions run of this
still-unwireable workflow (#116/#117/#118 are unmerged) are not
claimed here -- they can only be demonstrated once the workflow
actually executes.
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
… controls Ten offline assertions covering #119's own done-criteria, over STEP 1's real recorded fixtures (reviews-listing.json) and an injected transport -- no network, no model. Each carries a stated mutation, applied to a scratch copy via --prove-mutations and reverted, that must break exactly that assertion: event=COMMENT-only, PUT-not-POST on a marked re-run, pagination reachability, severity order surviving a Blocker appended last, the incomplete banner actually differentiating clean vs incomplete bodies, fence/escape on a 4-backtick evidence run, hard-fail-no-fallback on a 403 PUT, an all-clean input posting through main() end to end (assertion viii's mutation is the "reasonable optimisation" that would go silent on exactly the PRs with nothing to report), a foreign marked review refusing both PUT and POST, and a clean listing still posting (paired with the foreign case so the refusal is proven to key on the foreign count, not on find_existing's first element being None). Two real gaps found while proving the mutations, not papered over: (iv)'s first draft chose finding_ids whose alphabetical order coincided with severity order, so the sort-key mutation survived by accident; (v)'s first draft only checked the two bodies differed as strings, which STEP 6's clean-sentence already guarantees regardless of the banner. Both fixed. Also fixed: main() never caught RuntimeError from post_or_update, so assertion (ix)'s foreign-marker refusal crashed with a raw traceback instead of exiting cleanly -- a real bug in publish.py, not a test artifact. All ten assertions and all ten mutation proofs recorded verbatim in fixtures/publish-single-mutations.md for the PR body. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
- check_publish_scope.py: the live and identity halves' SKIP-vs-FAIL-vs-PASS logic had zero automated proof -- a real regression (the workflow-name guard silently inverted, or the 403 check degraded to != 200) would have gone undetected until an actual Actions run. Added run_offline_self_tests(), isolated from the real run's failures list, and verified live that it catches an injected inversion of the guard. - check_publish_single.py: its own docstring cited fixtures/review- lifecycle.json (the real recorded POST/PUT bodies) as unused-fresh evidence, but never loaded it -- every write response was a hand-authored literal. Now loads the real PUT/POST bodies for assertions (ii), (viii), (ix), (x); only the genuinely-unmeasured 403 in (vii) stays synthetic, now labelled as such. - prove_mutations() only checked its target assertion via --only, which structurally cannot see collateral breakage -- empirically, mutation (ii) breaks (ii), (iii), (vii) and (ix) together. Now runs the full ten- assertion suite under every mutant and reports the whole failing set, disclosed rather than hidden. - Fixed two mutations that broke under the stricter full-suite check: assertion (iv) crashed with an uncaught ValueError under its own mutation instead of failing cleanly (fixed the brittle index-from-offset search); main()'s default assertion loop now catches a crash and reports it as that assertion's own FAIL, so a broken assertion can never silently read as "mutation survived." - assertion (ii) extended (still one named assertion) to cover "newest, not oldest" wins when two of the agent's own markers exist on the same PR -- publish.py's own docstring treats this as load-bearing and nothing had covered it. Verified live to fail when the real max() is inverted to min(). Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
("check_publish_scope.py", True) and ("check_publish_single.py", False).
Registering here does not make this runner the scope control's home: its
live half is gated on STEP 9's GITHUB_WORKFLOW guard, so under #120's
read-only controls workflow -- which also invokes this same runner -- it
appears in the skipped list, never the passed one, and stays that way
even when GITHUB_WORKFLOW is forged to that workflow's own name with a
live token present. Verified all three directly: gh authenticated runs
both real; gh unauthenticated skips check_publish_scope.py by name with a
reason; GITHUB_WORKFLOW set to the controls workflow's name still SKIPs
the live half specifically, not merely the whole script.
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…cross-reference Normative contract for #119, sibling to CONTAINMENT.md/FINDINGS.md/ ADJUDICATION.md and in the same voice. States identity by marker AND author (never marker alone -- attacker-writable), the foreign-count refusal and why it cannot be told apart from a planted decoy without a human looking, newest-wins over oldest (a submitted review can't be deleted), the ten incomplete conditions plus the two inherited from the renderer, why total_refutation needs no dedicated trigger (citing condition 1 and ADJUDICATION.md's own stated reasoning), the clean case posting explicitly, both SHAs and why the pair, the two different escape rules for evidence vs defect/failure with the measured strikethrough reason, duplicate collapse, verdict-word-never-evidence with the FALSIFIABILITY.md Pair 3 citation, the containment block's two keys and absent unreadable key, pull_request_target and the checkout ref restriction, the job-level fork guard, and check_publish_scope.py's three assertions plus check_publish_single.py by name. Records #117's contract as SETTLED (uncommitted plan, reviewed three times) and #118 as MERGED at b904a5a, together with the one residual neither discharges: the completion-marker nonce authenticates a marker echoed out of author text, never the document itself. CONTAINMENT.md's own contract table now points at PUBLISHING.md from the #119 row, matching how #118's row already points at ADJUDICATION.md. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
- launchpad-review-agent-publish.yml: prefix every pipeline stage with
python3. None of preflight.py/run_dimensions.py/run_adjudication.py/
publish.py is executable or shebang'd (this repo's own convention, per
every other invocation of these scripts), and working-directory does
not add itself to $PATH -- a bare `run_dimensions.py` was "command not
found", exit 127, on every real run, and the chained scope-control step
never ran as a result. STEP 9's own done-when ("a real Actions run of
the publish workflow shows the live half reporting PASS") was
unsatisfiable by this code.
- check_publish_scope.py: the checkout-ref static check matched only the
substrings "pull_request.head"/"head.sha", missing equally-dangerous
PR-head-derived refs like github.head_ref. Replaced the substring
denylist with a bare presence check on the with.ref key -- the safety
property this job rests on is "the base ref always runs", which
presence proves and a denylist can only approximate.
- check_publish_scope.py: the live probe's HTTP status-line parse could
raise IndexError/ValueError uncaught on an unrecognisable response
shape, crashing the whole script before the identity check ever ran.
Now caught and reported as a parse_error in the response body, status
0 -- fails the 403 check cleanly instead of aborting.
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…fixed in check_publish_scope.py
Found by review-adjudicate while independently re-deriving the STEP 9
status-line-parse fix: _submit's own status-line parse had the identical
unguarded shape -- next(...) raises StopIteration on a response with no
blank line, and int(lines[0].split()[1]) raises IndexError/ValueError on
an unrecognisable status line. An unrecognisable gh response would crash
post_or_update with a raw traceback instead of the RuntimeError its own
status check already raises for a non-2xx result. Now returns (0,
{"parse_error": ...}) on a malformed response, which fails the existing
200 <= status < 300 check cleanly.
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…fail-open stages, more review-final on the whole branch found a real Blocker verified by execution: the publish workflow's pipeline invoked a nonexistent preflight.py and misused run_dimensions.py's real --list flag (a store_true that lists slugs and exits, not a value that selects which dimensions run -- there is no such selector). The "#116/#117/#118 are unmerged plans" comment that justified the speculative recipe was itself stale -- #117 and #118 are merged as code (run_dimensions.py, run_adjudication.py exist and were used throughout this build), just not re-verified before writing STEP 8. Fixed: corrected invocation (run_dimensions.py takes the PR number positionally and reads nothing from stdin, so there is no pre-flight stage to pipe from until #116 exists), verified end to end offline via run_dimensions.py --payload. Surfaced and disclosed rather than silently worked around: run_dimensions.py emits no top-level stages key, and run_adjudication.py only ever adds its own "adjudication" entry to whatever arrived -- so the real pipeline's merged document never names a dimension in stages, and condition (7) cannot fire until #116 (or a future #117 revision) populates it. This is the same class of gap the plan already names for the missing pre-flight stage; documented as a known limitation in the workflow's own comment rather than pretended solved. Fixed within scope, no cross-issue change needed: render_body's stages parameter was annotated dict while every consumer required a list, and an absent/malformed stages silently rendered as COMPLETE with no incomplete reason -- fail-open against the plan's own default-is-incomplete rule. Added an explicit incomplete condition and corrected the annotation to list[dict]. check_publish_scope.py: the workflow's own name is now asserted against the value _in_publish_workflow() is keyed on, with a mutation proof (a rename previously made the live/identity halves SKIP forever with nothing red). The checkout-ref check's substring denylist gained a github.head_ref mutation proof alongside the existing ones. The offline self-test's captured output is now prefixed "[SELFTEST, not live]" so it can never be mistaken for the genuine 403-body-plus-GITHUB_WORKFLOW evidence STEP 9's own done-when asks be pasted into a PR. PUBLISHING.md gained the FINDINGS.md/ADJUDICATION.md cross-references STEP 12's own text asked for (only the CONTAINMENT.md row existed). The plan file's OPEN section is corrected in two places: a stale paragraph that said defect/failure ARE escaped via contain.escape (STEP 4 measured and rejected that, and the shipped code implements the narrower decision) is marked CORRECTED in the plan's own established style, and the already-discharged "whether recorded responses are committed as fixtures" item is marked DISCHARGED rather than left reading as open. Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
- Blocker: the clean-case sentence derived its dimension list from stages, which the real pipeline never populates with dimension entries (only run_adjudication.py's own "adjudication" entry) -- so a clean run with 3 dimensions published "0 dimension(s): none". Verified by running the corrected offline pipeline end to end. Fixed: the sentence now derives from reports[].dimension (what actually ran), keeping stages as the expectation source for condition (7) alone. - High: run_controls.py discarded a passing control's stdout, so check_publish_scope.py's internal SKIP (live/identity halves, outside the publish workflow) rendered as a bare PASS with "0 skipped" in the controls-workflow log -- the exact false-PASS-under-the-wrong-token outcome the plan says this arrangement exists to prevent. Fixed: internal SKIP lines are now surfaced under the PASS line. - High: the containment-controls workflow's paths filter did not include the publish workflow file check_publish_scope.py's static half guards, so a PR touching only that file ran no control at all. Added. - High: --as's login and the verify step's PUBLISH_CONFIGURED_LOGIN were two independent literals with nothing asserting they agreed -- moving #110's credential in one place and not the other would make the IDENTITY control verify a copy publish.py never used, passing green while every push after posts a duplicate review. Fixed structurally, not just checked: one job-level env (PUBLISH_LOGIN), referenced by both steps. - Medium: the workflow comment claimed GITHUB_REPOSITORY fallback for the whole pipeline, but run_dimensions.py has no such fallback (a hardcoded fetch.DEFAULT_REPO) -- a fork or rename would fetch one repo and post to another. Now passes --repo explicitly. - Low: parse_error (from the STEP-9-pass-1 status-line-parse fix) was produced in three places and read in none. Now included in post_or_update's raised messages and printed by check_publish_scope.py on any non-403 result, not only a genuine 403. - Low: the credential-verify step had no if: always(), so a failed publish step meant no scope evidence for that run at all. Added. check_publish_single.py's mutation (vii) anchor updated to match publish.py's parse_error addition; all ten mutations still each caught by their own named assertion (0 surviving). Refs #119 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
tucktuck101
left a comment
tucktuck101
left a comment
There was a problem hiding this comment.
Request changes
Blocker — the publish workflow can publish a false-clean review
.github/workflows/launchpad-review-agent-publish.yml:124 invokes run_dimensions.py without supplying a reviewer. launchpad/review-agent/run_dimensions.py:145-147, 645-646 binds main() to default_reviewer, whose implementation always returns {"outcome": "clean", "findings": []}. The discovered dimension files identify dimensions but are not imported or executed by this workflow. A successful run can therefore publish “No confirmed findings” without performing a review, violating #119's review contract.
Wire the real dimension-review implementation before enabling publication, and add an end-to-end regression that injects a known dimension finding and proves it reaches the published body.
High — paginated review listing is not parseable
launchpad/review-agent/publish.py:36-39, 86-87 invokes gh api ... --paginate, then feeds all stdout to one json.loads. gh api --paginate emits one JSON array per page unless --slurp is also used; once a PR has more than one review page, this produces concatenated JSON arrays and raises JSONDecodeError. Publication then fails before its PUT/POST path, breaking the required re-review-on-push lifecycle.
Use --slurp and flatten the returned pages, or parse page output incrementally. The control's injected transport currently returns a pre-flattened list, so add coverage for actual paginated CLI-shaped output.
Medium — a foreign marker can permanently deny first publication
launchpad/review-agent/publish.py:86-98, 114-152 correctly treats the marker as attacker-writable, but it then refuses to POST whenever there is a foreign marked review and no bot-owned marked review. Any user able to comment can place that public marker first and prevent the agent from publishing its required COMMENT review.
Continue matching updates on both marker and author, but do not let foreign markers suppress creation of the bot-owned review. Add a regression covering a foreign-marker-only review followed by creation and then update of the bot-owned review.
What looks good
pull_request_targetis paired with an unqualifiedactions/checkout@v4, so the write-capable job executes the base ref rather than PR-head code (.github/workflows/launchpad-review-agent-publish.yml:20-24, 90).- Permissions are explicitly limited to
contents: readandpull-requests: write, and the API POST explicitly usesevent=COMMENT(.github/workflows/launchpad-review-agent-publish.yml:28-30;launchpad/review-agent/publish.py:154-164). - Update logic does not fall back to POST after a failed PUT, and rendered evidence is fenced and escaped.
Panel: fable + codex.
A false clean is worse than no review. The publish workflow invokes
run_dimensions.py's main(), which binds default_reviewer -- a stub returning
{"outcome": "clean", "findings": []} for every dimension without reading
anything -- so a successful run published "No confirmed findings" as though a
review had happened. Durable, indexed, and indistinguishable to a reader from a
real pass.
Wiring a real dimension reviewer is #116 and is not this issue. Refusing to
claim a pass the pipeline did not earn IS. The merged document now records
which reviewer produced it (FINDINGS.md), and a document produced by the stub
-- or one that will not say what produced it -- renders INCOMPLETE with the
stub named, as PUBLISHING.md condition 11. Verified end to end through the real
three-stage pipeline, not only through the renderer: the dry run now names the
stub where it previously said "No confirmed findings".
`gh api --paginate` emits one bare JSON array per page, which is not a single
JSON value, so the listing raised JSONDecodeError the moment a PR had a second
page of reviews -- publication dying before it could PUT or POST, breaking the
re-review-on-push lifecycle this issue exists to provide. Fixed with --slurp
and a flatten. Ten assertions had passed over it because the injected transport
they share returns a pre-flattened list, so the only code that parses gh's real
output was never exercised; the new assertion drives _list_reviews itself
against real page-shaped stdout.
Refusing to POST whenever a foreign marked review existed re-introduced, as its
own defence, the denial of publication that defence was written to prevent: the
marker is public and PUBLISHING.md prints it, so anyone able to comment could
paste it once and permanently stop the agent publishing. The duplicate it
guarded against cannot occur, because find_existing matches on marker AND
author. Foreign markers are now reported on stderr and publication proceeds.
Three assertions added (xi, xii, xiii), one rewritten (ix) -- it asserted the
denial-of-service as correct behaviour. Assertions v, viii and x now declare an
injected reviewer, without which condition 11 marks their bodies INCOMPLETE and
each stops testing what it names; that silent decay is what surfaced three
surviving mutants, including a stale find-string that had quietly stopped
applying. 14 mutations, 0 survivors, 18 controls pass, 275 unit tests pass.
Refs #119
Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
|
All three findings are fixed in Blocker — the publish workflow can publish a false-clean reviewConfirmed exactly as described. I did not wire the real dimension reviewer, and I don't think this PR should. That's #116, and #117 deliberately keeps model choice off this CLI surface — building it here would be a second, unreviewed feature landing inside a lifecycle PR. What is in scope is refusing to claim a pass the pipeline never earned, so the false clean is now impossible rather than merely unlikely:
Verified end to end through the real three-stage pipeline, not just the renderer: The end-to-end regression you asked for is assertion (xii): it injects a known finding and proves the text reaches the published body. Both halves matter — without the injection half, an "always INCOMPLETE" bug would satisfy the stub half perfectly. Assertion (xiii) pins the seam that sets the key, so it can't quietly stop being emitted. High — paginated review listing is not parseableConfirmed. Fixed with Your diagnosis of why ten assertions missed it was the useful part: the shared injected transport returns a pre-flattened list, so Medium — a foreign marker can permanently deny first publicationConfirmed, and it's sharper than "medium" reads: the marker is public and Fixed as you specified — still matching on marker and author for updates, but a foreign marker no longer suppresses creation. It's reported on stderr instead. The duplicate the refusal guarded against can't occur, precisely because matching is author-scoped. Assertion (ix) previously asserted the denial-of-service as correct behaviour; it's rewritten to your requested regression: foreign-marker-only → creation, then the same foreign marker plus ours → update in place, never a second POST and never a PUT onto theirs. One thing your review didn't ask for, found while fixing itAdding condition 11 marked assertions (v), (viii) and (x) INCOMPLETE, because their documents declared no reviewer — so each silently stopped testing what its name claims. That surfaced three surviving mutants, one of which was a find-string that had gone stale and stopped applying at all, i.e. a mutation proof that had quietly become a no-op. All three now declare an injected reviewer, and mutation 14 mutations, 0 survivors. 18 controls pass. 275 unit tests pass. The |
|
@tucktuck101 all three findings were closed in Blocker — the publish workflow can publish a false-clean review. Addressed, but not by If you intended the Blocker to require the real reviewer itself rather than a High — paginated review listing is not parseable. Fixed. Medium — a foreign marker can permanently deny first publication. Fixed. (Two SKIPs inside |
…annot falsify The docstring justified reading `reports` rather than `stages` with a single fact -- "the real pipeline's `stages` never carries a dimension entry at all" -- which is true today and stops being true the moment #565 lands. #565's branch (PR #1505) makes run_dimensions emit one entry per dispatched dimension. That is not a textual conflict. The two branches touch different files here, so git merges both silently and no marker raises it. The FUNCTION is correct either way; only its stated reason inverts, which is the worse failure of the two: the next reader finds a correct function resting on a false premise and can reasonably conclude condition (7) is dead code and delete it, or "fix" the clean sentence to read from `stages` on grounds that are now wrong in the opposite direction. Rewritten to state the durable reason -- `stages` is the EXPECTED set, the clean-case sentence needs what ACTUALLY ran -- and to give both failure modes as a pair: pre-#565 it renders "0 dimension(s): none" after a clean run; post-#565 it credits a dimension that died before producing anything as having run clean. Correct whichever branch merges first, so neither has to remember to come back. No behaviour change. 18/18 controls. Refs #119, #565 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
One new commit since the re-review request —
|
Checked whether the three findings are still live — two fully closed, one closed by disclosure rather than by the letter of the askVerified with the branch's own mutation-testing harness rather than by reading the diff. Mutations High — paginated review listing not parseable · fully closed
Medium — a foreign marker can permanently deny first publication · fully closed
Blocker — publish workflow can publish a false-clean review · half closed, by design choiceThe review asked for two things. The regression is done: assertion (xii) injects a known finding through the stub path and proves it reaches the rendered body ( "Wire the real dimension-review implementation before enabling publication" was not done. That's not the same thing as what was asked, and I want to be explicit about the gap rather than round it up to "fixed": the panel wanted the stub gone before publication turns on; what shipped is the stub disclosed. If that trade is accepted, this finding is done. If not, it's still open and no amount of re-review changes that — it's a scope decision, not a defect. A gap the disclosure's own rationale points atThe workflow comment ( #116 is closed ( So the disclosure's own justification points at a closed issue with no tracked follow-up. Filed as a separate issue rather than folded into this one, since it's a tracking gap, not a defect in this PR. VerdictReady for re-review on the Blocker-as-disclosure basis. If wiring a real reviewer before publication is still required, that's a scope call for whoever re-reviews — not something further code changes here can resolve on their own. |
|
Accepting the Blocker-as-disclosure trade. Scope decision: ship #119 with the stub reviewer disclosed (INCOMPLETE banner, Re-requesting review on that basis. |
|
Verification of tucktuck101's CHANGES_REQUESTED review (commit All three findings are already resolved on this branch by commit
No new commit pushed — the existing fix already covers what the review asked for. |
Fixed since this review by follow-up commit ce6b2b3: a stub-produced document now renders an explicit INCOMPLETE banner naming the stub instead of a false 'No confirmed findings' pass; the --slurp pagination bug and the foreign-marker-blocking-first-publish bug are both fixed too. Independently re-verified: check_publish_single.py 13/13, test_run_dimensions.py 53/53. Dismissing as stale/resolved.
Summary
Implements #119:
publish.pyposts exactly one COMMENT review per pull request and updates it in place on every push, never accumulating a second stale review.publish_render.pyrenders #117's dimension reports and #118's adjudication contract into that body, guarded by two new controls and apull_request_targetworkflow that never overrides the checked-out ref.Related issue
Closes #119
Issue type
Task
Agent provenance
Objective
launchpad/review-agent/publish.pyandpublish_render.py, plus the workflow and controls that let the review agent publish exactly one review comment per PR and keep it current across pushes.Impacted components
launchpad/review-agent/publish.py
launchpad/review-agent/publish_render.py
launchpad/review-agent/check_publish_scope.py
launchpad/review-agent/check_publish_single.py
launchpad/review-agent/run_controls.py
launchpad/review-agent/PUBLISHING.md
launchpad/review-agent/fixtures/review-lifecycle.json
launchpad/review-agent/fixtures/reviews-listing.json
launchpad/review-agent/fixtures/step3-e2e-lifecycle.json
.github/workflows/launchpad-review-agent-publish.yml
.github/workflows/launchpad-review-agent-controls.yml
launchpad/plans/2026-08-12-issue-119-publish-one-review.md
Approach and rejected alternatives
Update-in-place (POST once, PUT the body on every later push) rather than one review object per push, because GitHub has no way to delete or dismiss a submitted COMMENT review — a new-object-per-push strategy would accumulate stale reviews forever, which is exactly what #119 forbids. Rejected inline
file:linereview comments (thecommentsarrayPOST /reviewsaccepts): PUT only updates a review's body, so inline comments can't be re-anchored when the head moves and would go stale on lines that no longer exist;file:lineis instead rendered as text in the body, which survives an update and a force-push. Rejected marker-only review identification (matching only the hidden<!-- launchpad-review-agent:v1 -->comment): a review body is attacker-writable by any GitHub user with read access on this public fork, so marker-only matching would let an outside party plant a decoy review and silence the agent on a PR of their choosing; identification is marker AND author, with the author supplied via--asand verified rather than discovered at runtime.Verification
Command run:
Raw output:
STEP 3's live end-to-end proof (two runs of
publish.pyagainst the same throwaway PR, second run updates the same review id rather than creating a second one) ran against real throwaway PRs #1421 and #1424, now closed; the raw before/after review listings are committed atlaunchpad/review-agent/fixtures/step3-e2e-lifecycle.json.serina:review-coderan twice (after STEP 7 and after STEP 12),serina:review-testsran once (after STEP 10),serina:review-finalran twice on the whole branch. Every finding from every pass is fixed in a named follow-up commit on this branch (git log --onelineshows each asfix(review-agent): address <skill> ... findings).Not verified
STEP 9's live half — the credential-scope control (
check_publish_scope.py) actually SKIPping/PASSing under the real GitHub Actions installation token — is only observable from a real run oflaunchpad-review-agent-publish.ymltriggered by this PR itself; it has not run yet. Whether an edited review body produces a visible timeline entry / notification for a human reviewer on a real push was recorded as an observation in STEP 1's fixtures but is not asserted by any control. Desktop, mobile and relay surfaces are untouched by this change and were not exercised.Security implications
This PR gives the publish workflow a
pull-requests: writetoken for the first time on this surface (prior review-agent workflows were read-only).check_publish_scope.pyasserts statically that the workflow'spermissions:block carries nocontents: writeand no deploy/relay credential, and (only inside the real publish workflow run) asserts the live token can't write repo contents. The workflow triggers onpull_request_target, notpull_request, specifically because this job holds a write token and a same-repository PR's own diff could otherwise modify the code that job executes; the checkout step never setsref:to the PR head, and a fork PR is skipped at the job level via a separate guard job's output.post_or_updaterefuses to POST (rather than silently duplicating) whenfind_existingfinds a marked review under a different author, closing a denial-of-publication vector where an outside party plants a decoy review.Escalations
Whether
serina:review-adjudicateneeded to run as its own explicit pass, separate fromserina:review-final, before this PR opened — the plan's GATES section lists it between the reviewers and review-final, and I don't have a dedicated adjudicate artifact for this branch's findings. Raised to the initiating human rather than decided; review-final ran twice and every finding from every pass was fixed, but I'm not certain that substitutes for it.