review: the submission plan; Steps 4-6 as code in scripted mode (orchestrator slice 4) - #284
Conversation
🦋 Changeset detectedLatest commit: ca877da The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
357db2e to
bef1acd
Compare
bef1acd to
3b4809b
Compare
3b4809b to
02c6187
Compare
02c6187 to
ec302ed
Compare
ec302ed to
e6fceba
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
9c5894c to
c0cc85a
Compare
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
Post-fix side-by-side lifecycle trial: consolidated cost report and analysisFull three-round lifecycle on the seeded 1. Cost table
Every run far under the 2500-credit hard cap (max 891.6). Round 3 matched production #40996's final round on both arms: APPROVE plus resolution of every blocking thread, with a "3 of 6 prior review threads resolved" recap on S4. Superseded S4 round-1 attempts (fix-iteration cost, counted honestly): run 29901690493 (654.2 cr, posted with both default finders dead: the 5-minute sub-agent timeout) and run 29903306596 (708.4 cr, gate-blocked red run, posted nothing: rule-7 unicode false positive). With these, the trial's total spend was ~5.8k credits across 8 runs. Reference points:
S4 ran 8-17% cheaper than S0 on every round (r1: 593 vs 711; r2: 724 vs 789; r3: 744 vs 892) and posted a tighter, deduplicated comment set. 2. Orchestrator vs sub-agent decompositionS4 (exact: orchestrator from the agent-stdio
S0 (share estimate from streamed usage): method: group agent-stdio assistant messages by
The scripted orchestrator's residual is $2.0-3.1/run against S0's ~$7-9-equivalent share; the delta is the deleted dispatch loop. (Prior round's task-mode orchestrator read 11.2M cache tokens vs scripted's 1.6M.) 3. Re-review economics (#287's cache-memory carrier)First live verification, both arms, both re-review rounds: The carrier works, but under webapp's 4. Defect ledger (this trial series), each with fix and run evidence
5. Improvement suggestions(a) Delete the orchestrator's MCP transcription. The $2.0-3.1 scripted-orchestrator residual is mostly reading 6. End state (nothing cleaned up beyond the superseded-PR closes; operator decides)
|
There was a problem hiding this comment.
Changes requested — see inline comments.
4 of 4 prior review threads are still unaddressed as of 738154b:
- issue (blocking)
workflows/review/lib/dispatch-gate.ts:590: Rule 7's no-submission skip is broader than the shape review.md documents. This branch permits queueing nothing whenever... - issue (blocking)
workflows/review/lib/submission.ts:245: Scripted mode drops theskipLinesinvariant.runSubmissionCliplans an inline comment from every claim carrying a `p...
2 non-blocking threads still open
- note (non-blocking)
workflows/review/lib/dispatch-gate.ts:normalizeBodyfolds every URL to(url), so rule 7's plan-vs-queued comparison is blind to which link appears. The... - suggestion (non-blocking)
workflows/review/lib/dispatch-gate.ts:551: The zero-width / combining-grapheme-joiner strip arms ofnormalizeBody(lines 547-548) are untested — the only typogra...
…ipLines, the no-submission skip, URL folding) Four threads from the reviewer's CHANGES_REQUESTED on #284, plus the two non-blocking notes it carried. Blocking: - `runSubmissionCli` now applies Step 5's defer-to-open-human-threads rule. It drops any validated claim whose (path, line) is in the reconciler's `skipLines` before composing the inline comments AND before computing the verdict, so a deferred claim neither posts into the middle of a human conversation nor drives REQUEST_CHANGES. Scripted mode had shed the invariant entirely: review.md told the orchestrator `skipLines` was "already reflected in the plan" (it was not), and rule 7 forbids the orchestrator from dropping the comment itself, so the filter had to live in code or nowhere. Malformed entries are ignored, degrading to posting. - Rule 7's no-submission skip now checks the plan BODY, not just the event and comment count. review.md's redundant-approval skip requires the body to be exactly the bare comment-less-approve line; the old branch let an APPROVE carrying a mandatory "Note: <lens> not assessed this run" disclosure be dropped on the floor and still pass green, withholding both the disclosure and the approval. Non-blocking: - `normalizeBody` folds https URLs and the domain-redaction token to a host-bearing placeholder instead of wildcarding every URL, so the sanitizer's own rewrites still pass while a link splice that swaps the host blocks. Path and query stay wildcarded; the residual is documented at the fold. Moved to its own module (`lib/sanitizer-normalize.ts`), which also brings dispatch-gate.ts back under the 1000-line cap. - The zero-width / CGJ strip arms are now covered: a plan body carrying them against a queued copy the sanitizer stripped must not mismatch. - The emission-seam note in submission.ts (and the README) attributed the seam to credentials never entering the sandbox. That is not the constraint: queueing is an uncredentialed JSONL append, and what blocks code is the agent sandbox's read-only mount of ${RUNNER_TEMP}/gh-aw (confirmed in the compiled lock: `awf --mount ...gh-aw:...:ro`, while the safeoutputs MCP container gets safeoutputs/ rw). Recorded accurately, including that deleting it may be cheaper than "upstream-only". Six new vitest cases; all four behavior changes verified by mutation.
97acf80 to
4acf4e0
Compare
| ); | ||
| const planBody = | ||
| typeof planStaged.body === "string" ? planStaged.body : ""; | ||
| if ( |
There was a problem hiding this comment.
suggestion (non-blocking): The no-submission skip gates only on the plan shape (bare comment-less APPROVE), not the prior-review-APPROVED half review.md requires. priorReviews is already an input to evaluateDispatchConformance, but the skip branch never consults it — so on a first run with no prior review, an orchestrator that queues nothing passes green and an earned approval is silently withheld. Consider requiring the latest github-actions[bot] review to already be APPROVED before allowing the skip.
Lower-confidence (1)
workflows/review/lib/submission.ts:24— thought: the gate already rewrites the queue on violation (dispatch-gate.ts:853), so a repo-controlled queue-substitution step could remove the compare-and-normalize surface (and the per-version sanitizer-audit tax) entirely; the docstring itself concedes the seam is likely cheaper to delete.
| const stamp = | ||
| findLatestStamp(priors) ?? | ||
| stampFromCacheMemory(readCacheMemoryRecord(fs)); | ||
| if (stamp !== null && stamp.verdict === "REQUEST_CHANGES") { |
There was a problem hiding this comment.
suggestion (non-blocking): Only the positive arm of the reduced-depth flip floor is tested — both flip tests set a prior REQUEST_CHANGES stamp and assert REQUEST_CHANGES. The stamp.verdict === "REQUEST_CHANGES" guard here is the only thing holding the floor at 0 over a prior APPROVE (or absent stamp), and its negative arm is unprotected: a regression to if (stamp !== null) would flip a reduced-depth re-review to REQUEST_CHANGES silently and every existing test would still pass. Add a case: reduced depth over a prior APPROVE stamp with a kept blocking thread asserts APPROVE.
| // one same-host deep link for another passes rule 7; the splice | ||
| // check covers the prose and the link's host, not which page on | ||
| // that host is cited. | ||
| .replace(/https:\/\/([a-z0-9.-]+)\S*/g, "<url:$1>") |
There was a problem hiding this comment.
suggestion (non-blocking): normalizeBody folds https URLs with \S* (greedy to whitespace), but gh-aw's sanitizer path class is [^\s,] — it stops at commas. A claim discussion with a URL immediately followed by , (or a markdown link whose trailing )/. the two sides split differently) normalizes differently on the plan vs queued sides, so rule 7 false-blocks a byte-faithful submission. Mirror the sanitizer's [^\s,] boundary instead of \S*, and pin tests for URL+comma, URL+), and a markdown link to a redacted host.
| .replace(/[\u2018\u2019\u201a\u201b]/g, "'") | ||
| .replace(/[\u201c\u201d\u201e\u201f]/g, '"') | ||
| .replace(/[\u2012\u2013\u2014\u2015]/g, "-") | ||
| .toLowerCase() |
There was a problem hiding this comment.
note (non-blocking): .toLowerCase() and the global backtick strip here don't correspond to any gh-aw sanitizer transform (NFKC doesn't case-fold ASCII, and the sanitizer has no backtick strip). Because every fold is symmetric, rule 7's "matches the plan verbatim" guarantee actually tolerates case, backtick, and whitespace changes the orchestrator may freely make. Worth justifying these two folds in the doc (or dropping them) so the binding's real looseness is legible.
| body: renderClaimComment(claim), | ||
| }); | ||
| } else { | ||
| prLevelLines.push(`**${claim.label}:** ${claim.discussion}`); |
There was a problem hiding this comment.
nitpick (non-blocking): The body fold for a file/PR-level claim emits only **label:** discussion, dropping claim.path, any rule_quote, and any suggestion that renderClaimComment would render inline. A file-anchored finding then posts naming no file and without its supporting rule quote. Consider routing the fold through renderClaimComment (or at least prefixing the path).
| } | ||
| } | ||
| } | ||
| if (planStaged !== undefined && submit !== undefined) { |
There was a problem hiding this comment.
note (non-blocking): Rule 7 binds the queued event, body, and comments to the plan but never compares queued resolves against planStaged.resolve. Rule 6 catches a dropped resolve only as a deficit note, not a block — asymmetric with how event/body/comments are made binding. Likely the benign deficit direction, but worth naming since the rule advertises making the queued outputs match the plan.
4acf4e0 to
7e46c1d
Compare
… as code in scripted mode (orchestrator slice 4)
…shapes; URL-tolerant normalization; HOLD guard; renderer parity (re-review feedback)
…he cache-memory stamp; rule 6 reads the reconciler out-file leniently
…TML comments; the flip floor anchors on the cache-memory stamp (trial run 29893634730)
…viewedShare (was a dead field); blank-quote-line and tripwire-note coverage (re-review feedback)
…viewedShare; the hardening test's fixture encoded the dead field (re-review feedback)
…or plausibly drop-in payloads A claim's suggestion was wrapped verbatim in a suggestion fence, which GitHub offers as a one-click committable replacement of the anchored line. Trial run 29897276810 posted an English sentence (Khan/webapp#41009 comment r3628128268) and a 30-line test function (r3628128224) that way; committing either would have written prose or a misplaced function into the file. renderClaimComment now emits the committable fence only when the payload is plausibly drop-in: at most 8 lines, at least one line carrying a code signal (a call, statement punctuation, an assignment/arrow operator, a comment marker, or code indentation), and no line reading as an English sentence (six-plus words, three quarters of them plain). Anything else renders as a plain fenced block introduced as a sketch. Deterministic and tested against both of the run's bad payloads and its two legitimate drop-ins.
… prose (suggestion-fence guard follow-up)
Run 29901690493 posted two English sentences as committable suggestion
fences because the guard's code-signal check vetoed the prose check: a
sentence embedding a call ('Use ctx.Time().Now().AddDate(0, 0,
-MemoryTTLDays), and add a test that ...') or a semicolon slipped through.
looksLikeProse now scores the line on its plain-word ratio regardless of
code tokens; a drop-in payload must still carry a code signal AND read as
no line of prose. Both live misses are pinned as fixtures.
…e gate's sanitizer-tolerant normalization Trial run 29903306596 went red on rule 7 with a jq-verbatim emission: the orchestrator piped each planned comment unmodified into the safe-outputs queue, but gh-aw's ingest sanitizer folded a reviewer-authored ellipsis to three ASCII dots, and the normalized comparison treated that one character as an accountability splice. normalizeBody now folds the same typographic family (ellipsis, curly quotes and apostrophes, dash variants, no-break space) to ASCII on both sides before comparing. Pinned with a fixture that round-trips a fancy-punctuation claim through the fold.
…nicode pipeline in the gate normalization (NFKC + zero-width strip) gh-aw's hardenUnicodeText (sanitize_content_core.cjs) NFKC-normalizes and strips zero-width characters before content is queued, which is what folded run 29903306596's ellipsis; the hand-rolled ellipsis rule covered that one character but not the rest of NFKC (superscripts, ligatures, full-width forms). normalizeBody now applies NFKC and the same zero-width strip on both sides, keeping the curly-quote and dash folds NFKC does not perform.
…oiner out of the zero-width class (no-misleading-character-class)
… transforms in the plan-match normalization
An audit of gh-aw v0.81.6's ingest sanitizer against rule 7's
normalizeBody found unabsorbed transforms that false-block a
byte-faithful submission (the same class as the two already fixed:
comment stripping, NFKC). The deployed URL policy is allowed-only and
runs inside code regions, so any cited non-allowlisted https URL (MDN,
StackOverflow) comes back "(host/redacted)" and any non-https or
blocked scheme "(redacted)" while the plan side normalized to <url>:
a guaranteed mismatch on the most ordinary review prose. Also absorbed:
neutralizeTemplateDelimiters' escaping backslashes (unbackticked
${{ }} / {{ }} / {% %} in prose, likely in this repo's reviews), the
bidi-control strip, and the C0/DEL control strip. All folds apply to
both sides, and every URL form and redaction token maps to one
placeholder, so splice detection loses nothing it could previously
see.
…dening in the submission-plan changeset
…ipLines, the no-submission skip, URL folding) Four threads from the reviewer's CHANGES_REQUESTED on #284, plus the two non-blocking notes it carried. Blocking: - `runSubmissionCli` now applies Step 5's defer-to-open-human-threads rule. It drops any validated claim whose (path, line) is in the reconciler's `skipLines` before composing the inline comments AND before computing the verdict, so a deferred claim neither posts into the middle of a human conversation nor drives REQUEST_CHANGES. Scripted mode had shed the invariant entirely: review.md told the orchestrator `skipLines` was "already reflected in the plan" (it was not), and rule 7 forbids the orchestrator from dropping the comment itself, so the filter had to live in code or nowhere. Malformed entries are ignored, degrading to posting. - Rule 7's no-submission skip now checks the plan BODY, not just the event and comment count. review.md's redundant-approval skip requires the body to be exactly the bare comment-less-approve line; the old branch let an APPROVE carrying a mandatory "Note: <lens> not assessed this run" disclosure be dropped on the floor and still pass green, withholding both the disclosure and the approval. Non-blocking: - `normalizeBody` folds https URLs and the domain-redaction token to a host-bearing placeholder instead of wildcarding every URL, so the sanitizer's own rewrites still pass while a link splice that swaps the host blocks. Path and query stay wildcarded; the residual is documented at the fold. Moved to its own module (`lib/sanitizer-normalize.ts`), which also brings dispatch-gate.ts back under the 1000-line cap. - The zero-width / CGJ strip arms are now covered: a plan body carrying them against a queued copy the sanitizer stripped must not mismatch. - The emission-seam note in submission.ts (and the README) attributed the seam to credentials never entering the sandbox. That is not the constraint: queueing is an uncredentialed JSONL append, and what blocks code is the agent sandbox's read-only mount of ${RUNNER_TEMP}/gh-aw (confirmed in the compiled lock: `awf --mount ...gh-aw:...:ro`, while the safeoutputs MCP container gets safeoutputs/ rw). Recorded accurately, including that deleting it may be cheaper than "upstream-only". Six new vitest cases; all four behavior changes verified by mutation.
7e46c1d to
43b63b0
Compare
There was a problem hiding this comment.
Changes requested — see inline comments.
6 of 6 prior review threads are still unaddressed as of 43b63b0:
6 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-gate.ts:560: The no-submission skip gates only on the plan shape (bare comment-less APPROVE), not the prior-review-APPROVED half `rev... - note (non-blocking)
workflows/review/lib/dispatch-gate.ts:579: Rule 7 binds the queued event, body, and comments to the plan but never compares queued resolves against `planStaged.res... - note (non-blocking)
workflows/review/lib/sanitizer-normalize.ts:51:.toLowerCase()and the global backtick strip here don't correspond to any gh-aw sanitizer transform (NFKC doesn't case... - suggestion (non-blocking)
workflows/review/lib/sanitizer-normalize.ts:69:normalizeBodyfolds https URLs with\S*(greedy to whitespace), but gh-aw's sanitizer path class is[^\s,]— it st... - suggestion (non-blocking)
workflows/review/lib/submission.ts:285: Only the positive arm of the reduced-depth flip floor is tested — both flip tests set a prior REQUEST_CHANGES stamp and... - nitpick (non-blocking)
workflows/review/lib/submission.ts:302: The body fold for a file/PR-level claim emits only**label:** discussion, droppingclaim.path, anyrule_quote, and...
Note: holistic not assessed this run (shed under the high-tier run budget).
Note: completeness not assessed this run (shed under the high-tier run budget).
Note: test-adequacy not assessed this run (shed under the high-tier run budget).
Note: first-principles not assessed this run (shed under the high-tier run budget).
Note: conventions not assessed this run (shed under the high-tier run budget).
…se against a null staged plan
A `submission-plan.json` containing literal JSON `null` fail-opened the
entire dispatch gate. `readJsonIfPresent` passes `JSON.parse("null")`
through unchanged, and `planStaged` was guarded only by `!== undefined`, so
the property reads threw a TypeError. That throw escapes
`evaluateDispatchConformance` to the CLI entry catch, which exits 0 with the
queue untouched, so the failure bypassed all seven rules rather than just
rule 7.
Guard on `typeof === "object" && !== null`, matching the `priorReviews`
parse that was already hardened against exactly this shape.
Two tests: a null plan must not throw and must still let the other rules
fire, and a well-formed plan must still bind the queue, so the guard cannot
become a bypass. The first fails without the fix with the exact TypeError.
Review Guidancegithub-actions (3 files)
Excluded from review (3 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
There was a problem hiding this comment.
1 of 7 prior review threads resolved; 6 still unaddressed as of ca877da:
6 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-gate.ts:574: The no-submission skip gates only on the plan shape (bare comment-less APPROVE), not the prior-review-APPROVED half `rev... - note (non-blocking)
workflows/review/lib/dispatch-gate.ts:593: Rule 7 binds the queued event, body, and comments to the plan but never compares queued resolves against `planStaged.res... - note (non-blocking)
workflows/review/lib/sanitizer-normalize.ts:51:.toLowerCase()and the global backtick strip here don't correspond to any gh-aw sanitizer transform (NFKC doesn't case... - suggestion (non-blocking)
workflows/review/lib/sanitizer-normalize.ts:69:normalizeBodyfolds https URLs with\S*(greedy to whitespace), but gh-aw's sanitizer path class is[^\s,]— it st... - suggestion (non-blocking)
workflows/review/lib/submission.ts:285: Only the positive arm of the reduced-depth flip floor is tested — both flip tests set a prior REQUEST_CHANGES stamp and... - nitpick (non-blocking)
workflows/review/lib/submission.ts:302: The body fold for a file/PR-level claim emits only**label:** discussion, droppingclaim.path, anyrule_quote, and...
| // catch, which exits 0 with the queue untouched: not a rule-7 failure but | ||
| // a fail-open of ALL SEVEN rules. The sibling `priorReviews` parse was | ||
| // hardened against exactly this shape; this one has to match it. | ||
| const planStaged = |
There was a problem hiding this comment.
suggestion (non-blocking): A present-but-malformed submission-plan.json (literal null, a scalar, or an object with no bindable event/body/comments) folds to planStaged === undefined, so rule 7 is skipped silently and the gate still reports conformant — unlike the routing.json parse (dispatch-gate.ts:777-781), which records a present but unparseable note. Run forensics then cannot distinguish plan matched from plan staged but never bound. Consider pushing a note when the plan file exists but parses to undefined/null, mirroring the routing.json precedent. Non-blocking: the null fold is the right fail-open direction and an agent that can rewrite the plan can also delete it, so this is observability, not a new bypass.
| .filter( | ||
| ( | ||
| comment, | ||
| ): comment is {path: string; line: number; body: string} => |
There was a problem hiding this comment.
nitpick (non-blocking): The plan-comment filter predicate narrows to {path: string; line: number; body: string} but only checks that path and body are strings, never line. It fails closed (a non-numeric line yields a planned key that can never match a queued key, so the run blocks rather than falsely passing), so there is no live defect — the writer always stages numeric lines. But it falls short of the file own defensive-parse convention; adding typeof (comment as {line?: unknown}).line === "number" to the predicate makes the asserted type actually checked and drops a malformed entry the same way a bad path/body already is.
…ad suppression, deterministic cache record (#288) * [jwies/review-decision-chokepoints-local] review: rule 5 anchors on the cache-memory stamp; rule 6 reads the reconciler out-file leniently * [jwies/review-trial-followups] review: post-trial follow-ups; structured sub-agent finals, open-thread suppression, deterministic cache record The three highest-value in-repo suggestions from the lifecycle trial's consolidated report (suggestions h, g, and b on #284): - Structured sub-agent finals: every scripted dispatch exposes an in-process submit_result MCP tool whose input is validated against the agent's exact output contract at the tool boundary; a drifted shape is rejected back to the model in-session with the precise contract error (defect 13: three drift shapes in five runs, two voided correctness dimensions). Free-text finals remain the fallback. - Open-thread suppression at dispatch: a candidate describing a defect an open bot thread already tracks posts no duplicate (same path plus the #245 similarity floor; reconciler-resolved threads exempt); a suppressed blocking candidate still floors the verdict at REQUEST_CHANGES. - Deterministic Step 9 cache write: in scripted mode the orchestrator invokes lib/cache-record.ts once after emitting; the fingerprint-carrier fields are copied verbatim from staged files and corroborated against the safe-output queue. risksPatternsKey becomes code-owned end to end. Verification: 1254 tests green, typecheck and eslint clean, frontmatter compiles clean under gh-aw v0.81.6 (gate step ordering verified against the compiled lock; the cache writer cannot be a post-step because the compiled agent job uploads cache-memory before post-steps run). * [jwies/review-trial-followups] review: address #288 review feedback - computeRisksPatternsKey: the risk-tier filter matched the literal "moderate" while the triage contract emits Medium/High; every medium-risk file was silently dropped from the signature, so Step 7 could not detect a guidance change confined to medium-risk files. Accept medium (moderate stays tolerated) and pin the contract vocabulary in the fixtures. - Open-thread suppression floor: record the matched thread opener's blocking-ness (threadBlocking) and floor the verdict only when both the suppressed candidate and the thread opener are blocking. A false-positive blocking candidate matching an open non-blocking thread can no longer force REQUEST_CHANGES with no validation and no visible blocking comment. - risks-patterns-key.txt now stages at full depth only. Step 7 skips every reduced depth including scoped, and a scoped run's triage sees only the scoped subset; staging that narrower signature could collapse the standing full-run guidance. - The cache-record CLI emits ::warning on corroboration/staging refusals (a systematic refusal permanently stales the fingerprint); the benign no-ops (task mode, gate-blocked) stay quiet. - readQueue parses the safe-output JSONL per line, so one truncated tail line no longer discards the whole queue. - dispatch-runner gets direct tests via a mocked Agent SDK: the submit_result accept/reject handler, the structured final beating the free-text final, and the salvage of an accepted payload after the session dies. * [jwies/review-trial-followups] review: enforce bot-authored openers on open-thread suppression in code threads.json staging is prompt-executed (review.md asks the orchestrator for the unresolved github-actions[bot] threads; stage-pr deliberately does not stage threads yet), and the dispatcher read the opener body without ever reading its author, so a mis-staged human thread on the same path could silently suppress a bot candidate before validation; its free-text opener would also read as non-blocking and skip the verdict floor. The OpenThread build moves to openThreadsFromStaged (dedup.ts, beside its consumers) and admits only threads whose opening comment the bot authored, the same code-level stance stage-pr already takes for prior-reviews.json. Fails closed: a thread without a bot opener never suppresses, worst case a duplicate comment. Eval staging (live-stage.ts) already writes bot-authored openers. * [jwies/review-trial-followups] review: record the suppression hardening in the followups changeset * [jwies/review-trial-followups] review: address the reviewer feedback on the trial follow-ups Three items from the bot review on #288. 1. `threadOpenerIsBlocking` matched a hand-rolled `\(blocking\)`, which cannot match `issue (blocking, best-practice)`, a real BLOCKING_LABELS entry. A suppressed re-confirmation of such a thread recorded `threadBlocking: false`, the both-blocking floor in submission.ts never fired, and the run could flip to APPROVE over a still-open, re-confirmed blocking objection: exactly the invariant the suppression adds. The opener's label is now parsed off the template and classified by `isBlockingLabel`, the taxonomy's own rule (spacing inside the parens normalized first). 2. Open-thread suppression depended on staged threads being unresolved, but only review.md prose enforced that, the same gap the bot-authorship hardening closed. `get_review_comments` returns `is_resolved` per thread, so review.md now stages it as `resolved` and `openThreadsFromStaged` admits only threads that carry an explicit false. Fails closed like the author check: worst case is a duplicate comment. 3. The cache-record writer trusted the plan alone when no safe-output queue was readable, citing the dispatch gate. The gate runs on the post-ingest agent_output.json, after cache-memory is committed, so it cannot protect the record: a run whose emission silently failed recorded "review posted" with the current fingerprints, and the next run stamped against an unreviewed diff. An unreadable queue is now a refuse-with-warn, except the one shape that legitimately queues nothing (the Step 6 redundant-approval skip), keeping the failure pointed at a fuller next review. Suite green (1278 tests), typecheck and lint clean. * [jwies/review-trial-followups] review: fold the NOTIFIED signature into the staged guidance key review.md's Step 7 defines the canonical risks/patterns signature to include `notified.json`'s signature and treats the NOTIFIED match set as an independent repost trigger, but `computeRisksPatternsKey` built the key from risks, patterns, exclusions and owners only. Because the key is staged at Step 4, a NOTIFIED-only change produced an identical key on both sides of the Step 7 compare: the guidance comment never reposted and a newly subscribed `.github/NOTIFIED` @team was silently never mentioned, with the notified-less key persisting as the baseline. * [jwies/review-trial-followups] review: compute the NOTIFIED signature in the submission CLI The NOTIFIED fold was dead at runtime in scripted mode. `runSubmissionCli` read `notified.json`, but the notified CLI does not run until Step 7 and nothing stages that file earlier, so `signature` was always undefined and `computeRisksPatternsKey` dropped the `notified:` component on every real run. A `.github/NOTIFIED`-only change then staged a key identical to the prior run's, Step 7 read the guidance as unchanged, and a newly-subscribed team was never mentioned: exactly the failure the fold was added to fix. The existing test passed only because it injected the signature directly. Compute it here instead of depending on step ordering. `notified.ts`'s `runCli` is pure over staged inputs the pre-agent step writes as hard prerequisites (`files.json`, `full.diff`), so it is safe to call this early, and it stages `notified.json` as a side effect, which makes Step 7's own invocation idempotent rather than first-of-its-kind. The tests go in a new `submission-notified.test.ts` rather than another block in `submission.test.ts`, which sits within ten lines of the 1000-line max-lines budget and went over when they were added there. None of them injects a signature, which is the point: one asserts the component lands without `notified.json` staged first, one asserts a NOTIFIED-only change moves the key (the assertion that fails against the read-from-file version), and one asserts a repo without the file still stages the pre-feature key.
Retargets the PR from the jwies/review-pre-agent-staging staging branch to main, which now carries that branch's work squash-merged (#280, #282, #283, #284, #288, #296) plus the later slices this branch never received. The head branch carried pre-squash copies of the review stack, so every conflict was a stale duplicate rather than a real divergence. All seven resolve to main: - .github/aw/actions-lock.json: main's superset. The autofix compile at v0.83.4 had pruned the setup-cli entry; main is at v0.83.4 too now, so the version skew the PR description flagged between autofix.lock.yml (0.83.4) and review.lock.yml (0.81.6) is gone. - lib/dispatch-gate.ts, lib/dispatch-gate.test.ts, lib/stage-pr.ts, lib/stage-pr.test.ts: add/add against main's merged versions. - review.md, README.md: main supersedes the head copies (timeout 40, the scripted-dispatch SDK install, code-owned disciplines extraction, the retired firewall pin, the lens payload seam). No autofix commit ever touched workflows/review/, so nothing is lost taking main wholesale there: the merged review stack is byte-identical to main and the whole merge reduces to main plus the autofix package.
2 participants
Plan-of-record: The deterministic orchestrator, slice 4 (the end state, shipped as the production probe). Stacked on #283 over #282, #280, #279. Top of the stack.
What
In scripted dispatch mode (#282's ROUTING dial), Steps 4-6 become code.
lib/submission.tsruns once after the dispatcher and stagessubmission-plan.json:computeVerdictover the validated claims' post-validation labels (REQUEST_CHANGES iff a blocking label posts), plus the reduced-depth flip floor: at flip-gated/fast depth over a prior REQUEST_CHANGES stamp (parsed fromprior-reviews.json),rereview.json'skeptBlockingCountfloors the verdict. HOLD_FOR_HUMAN is unreachable here by construction (the dispatcher surfaces unavailable dimensions as note lines instead), so the plan is always a submittable event.renderReviewBody: empty-body APPROVE with comments, the fixed REQUEST_CHANGES line), the code-rendered accountability section spliced verbatim, the dispatcher's code-rendered note lines, the depth/tripwire notes, and the fingerprint stamp as the final line.The orchestrator's remaining role in this mode is emitting safe outputs that match the plan, and the gate now makes that binding (rule 7,
submission-plan-mismatch): when a plan is staged and a submission is queued, the queued event, body, and inline comments must match the plan under a sanitizer-tolerant normalization (case, whitespace, backticks — gh-aw's ingest sanitizer may neutralize mentions). Any splice, omission, or "improvement" strips the queue and fails the run. This is #244's accountability-splice check as code, and it closes the last slice-3 residual: the model can no longer compose, only transcribe. The redundant-approval skip still applies (no queued submission, no rule).What this makes the run, end to end (scripted mode)
stage (pre-agent, #280) → dispatch/validate (one CLI, #282) → plan (this PR) → emit (model, gate-checked) → gate (#279/#283) → safe_outputs job. The orchestrator's model turns are: the early-exit check, thread staging, the optional tier-question call, three CLI invocations, and MCP transcription of the plan. That is the no-post runner's shape in production, bounded by the ~$2 sub-agent floor the cost anatomy names.
The recorded upstream-only item (plan doc Q1): the safe-output emission seam. gh-aw queues safe outputs through the engine's MCP tools whose credentials never enter the firewall sandbox, so code cannot write the queue directly; deleting the orchestrator entirely (zero model turns) requires an upstream gh-aw change (a custom-engine or code-emitted safe-output path). Until then the emission is model work that the plan-match rule reduces to typing.
Verification
32 vitest cases in
lib/submission.test.ts: comment rendering (labels, rule quotes, suggestions, drop-in vs sketch), verdict composition (blocking claim, non-blocking-only, comment-less approve, the flip floor with a real stamp and kept blocking thread, including the depth note), pr-level folding, the dispatcher-not-run error, the human-threadskipLinesfilter (comments and verdict, plus malformed entries), the gate plan-match rule in all four directions (match passes; spliced body, flipped event, and dropped comment each block), the no-submission shapes (bare approve skips; a disclosure-carrying approve and a dropped REQUEST_CHANGES both block), and the sanitizer-tolerance suite (stamp strip, typographic fold, URL/scheme redaction, template escaping, zero-width/CGJ strip, cross-host link splice). Full suite green (1158 tests), typecheck clean, eslint clean, frontmatter compiles clean under gh-aw v0.81.6.Same gating stance as #282: nothing changes in task mode; scripted mode (dispatch + plan together) is live-trial-gated per the review-trial protocol before any consumer flips the ROUTING line. Cost measurement per the plan doc's stance: before/after per-run dollars and wall clock on the probe repo, plus a drift-corpus pass for recall neutrality.
Sanitizer-audit hardening (2026-07-22, added before human review)
An audit of gh-aw v0.81.6's ingest sanitizer against rule 7's
normalizeBody(this PR's plan-match rule) found unabsorbed transforms that false-block a byte-faithful submission, the same class as the comment-strip and NFKC fixes already in the rule. Top vector: the deployed URL policy is allowed-only and runs inside code regions, so any cited non-allowlisted https URL (MDN, StackOverflow) comes back(host/redacted)against the plan's<url>placeholder. The normalization (now its own module,lib/sanitizer-normalize.ts) folds https URLs and the domain-redaction token to a host-bearing placeholder, so the sanitizer's rewrites pass while a link splice that swaps the host still blocks (path and query stay wildcarded, documented at the fold); it also absorbs the template-delimiter escaping (unbackticked${{ }}in prose), bidi-control and C0/DEL strips; every fold applies to both sides, so splice detection loses nothing. Documented-not-absorbed residuals (each needs a pathological body and fails red, not silent): HTML entity decode, XML tag conversion, percent-decode side effect, homoglyph folds, 65k truncation, markdown link titles, tilde fences.Review follow-up (2026-07-29)
Addressing #284 (review) (2 blocking, 4 non-blocking):
skipLines(blocking).runSubmissionClinow applies Step 5's defer-to-open-human-threads rule: any validated claim anchored on a line in the reconciler'sskipLinesis dropped before the inline comments are composed and beforecomputeVerdictruns, so it neither posts into the middle of a human conversation nor drives REQUEST_CHANGES. Scripted mode had shed the invariant entirely, and since rule 7 forbids the orchestrator from dropping the comment itself, the filter had to live in code or nowhere.Note: <lens> not assessed this rundisclosure could be dropped on the floor and still pass green, withholding both the disclosure and the approval.normalizeBodyfolds https URLs and the(host/redacted)token to a host-bearing placeholder instead of wildcarding every URL, so a cross-host link splice now blocks. Path and query remain wildcarded; that residual is stated at the fold.awf --mount "${RUNNER_TEMP}/gh-aw:${RUNNER_TEMP}/gh-aw:ro"(onlysafeoutputs/upload-artifactsis rw) while the MCP gateway container gets-v "${RUNNER_TEMP}/gh-aw/safeoutputs:...:rw". Queueing is an uncredentialed JSONL append that this CLI simply cannot write. The note insubmission.tsand the README now say so, including that deleting the seam may be cheaper than "upstream-only" (a mount change, or a post-agent host step of the kind the gate already uses to rewrite the queue). Untested; recorded, not claimed.normalizeBodymoved tolib/sanitizer-normalize.ts, which also bringsdispatch-gate.tsback under the repo's 1000-line cap. All four behavior changes were verified by mutation (reverting each makes its test fail).