review: dispatch-conformance gate at the submit chokepoint (deterministic orchestrator, slice 0) - #279
Conversation
🦋 Changeset detectedLatest commit: db17815 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 |
|
Scoping note for the next slice (staging becomes pre-workflow steps; round-three step 5 / orchestrator doc slice 1), recorded here instead of expanding this PR: What moves. The staging work the orchestrator currently performs by invoking CLIs mid-conversation becomes deterministic steps that run before the agent starts, so it wakes with files on disk: the prior-reviews fetch, the rereview-mode CLI (plan + Mechanism, informed by this PR's investigation. What this slice buys the gate. Today the gate trusts the orchestrator to have invoked the router and rereview-mode CLIs honestly ( Inherited ledger this migration owns (deferred to it from the 07-13 bot-feedback pass): #244's post-submit accountability-splice check and executed-vs-decided resolve accounting, #245's code-side shape enforcement for out-of-lane handoffs and cross-source same-line dedup, #246's flip-gate code chokepoint, #247's extraction-verify strengthening (disciplines extraction becomes a pre-step, its verify becomes code). Rider on the same track, eval-gated separately with a powered run because it is recall-affecting: the |
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 |
|
Stack guide (bottom-up, one slice per PR, each stacked on the previous): #279 slice 0 (this PR, the dispatch-conformance gate) → #280 slice 1 (staging becomes a pre-agent step) → #282 slice 2 (script-driven dispatch behind the ROUTING |
This comment has been minimized.
This comment has been minimized.
a8f80b4 to
d9604c2
Compare
| echo "::error title=dispatch-conformance gate::submission blocked; failing the job" | ||
| exit 1 | ||
| fi | ||
| echo "::warning title=dispatch-conformance gate::gate could not run (infra failure; review not blocked)" |
There was a problem hiding this comment.
nitpick (non-blocking): This gate could not run (infra failure; review not blocked) warning also fires when the gate did block. If the sentinel write throws but the queue rewrite succeeds (the degrade-to-detect path), runDispatchGateCli still exits 1 with no sentinel file, so both clauses are false — the gate ran and the queue was stripped, so nothing posts, yet the operator's only log line says the review was not blocked. Consider distinguishing the case (e.g. also test for dispatch-gate.json) or rewording to gate exited non-zero without a violation sentinel.
Lower-confidence observations (4)
workflows/review/lib/agent-json.ts:45— the 200-span cap inbalancedSpanscounts balanced-but-unparseable spans, so a final message quoting a large brace-rich code block before an unfenced trailing JSON payload could exhaust the cap before reaching the payload; consider capping failed parse attempts, or scanning from the end since the payload is terminal.workflows/review/lib/agent-json.ts:116— fenced blocks are tried (last-first) before balanced spans, so an earlier fenced example ahead of the real bare payload wins; viatriageEmptiedReviewa quoted empty-reviewFilesexample could waive rule 1.workflows/review/lib/rereview-mode.ts:385— the cache carrier hard-rejects any verdict exceptAPPROVE/REQUEST_CHANGES, while the body-stamp regex accepts any token; a comment-only arm recordingCOMMENTcould never anchor via the only working carrier and would re-planfullforever.workflows/review/lib/rereview-mode.ts:42— since no HTML-comment stamp has ever survived gh-aw sanitization, a sanitizer-surviving encoding could restore the durable body carrier rather than a stripped format plus an evictable cache fallback (eviction degrades safely tofull, so this is cost, not correctness). If visible body noise rules that out, worth noting in the header.
| be evicted, the review body cannot.) | ||
| only; both sides of that comparison are Step 1's own added-lines hash.) | ||
| - `stampHunks`: copy **verbatim** from `rereview-plan.json`'s `stampHunks` field (the | ||
| plan CLI wrote it in Step 3). This, with `verdict` and `wasDraft`, is the divergence |
There was a problem hiding this comment.
suggestion (non-blocking): Step 9 lists stampHunks, wasDraft, and reviewedHunks as pinned (backticked) cache keys, but the verdict is only described in prose (The verdict and whether a risks/patterns comment was posted this run) with no key name or allowed values. stampFromCacheMemory requires exactly verdict: "APPROVE" | "REQUEST_CHANGES", so a run that records the verdict under a different key — or as a COMMENT event — makes the cache fallback return null and every re-review re-plans full, the silent inertness this changeset exists to prevent. Consider pinning it like its neighbors:
| plan CLI wrote it in Step 3). This, with `verdict` and `wasDraft`, is the divergence | |
| - `verdict`: the submitted review event, recorded as exactly `APPROVE` or `REQUEST_CHANGES` |
| * thread resolutions, the risks/patterns comment, reviewer requests, and any | ||
| * type this list has never seen) is stripped: default-deny. | ||
| */ | ||
| export const KEEP_ITEM_TYPES: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
suggestion (non-blocking): On a block, KEEP_ITEM_TYPES strips the verdict, every inline comment, and the risks comment, so the author gets a red run with zero review signal even though the findings were only unvalidated, not shown to be wrong. Did you weigh rewriting the submission to a COMMENT-event review (with a conspicuous non-conformance banner and blocking labels demoted) — preserving the findings while removing verdict authority and still going red? The module doc states the suppression trade but not this alternative; if total suppression is deliberate, a line naming that choice would help the next reader.
d9604c2 to
1d9d558
Compare
Review Guidancegithub-actions (4 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.
3 of 3 prior review threads are still unaddressed as of 1d9d558:
3 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-gate.ts:409: On a block,KEEP_ITEM_TYPESstrips the verdict, every inline comment, and the risks comment, so the author gets a red... - suggestion (non-blocking)
workflows/review/review.md:1659: Step 9 listsstampHunks,wasDraft, andreviewedHunksas pinned (backticked) cache keys, but the verdict is only de... - nitpick (non-blocking)
workflows/review/review.md:271: Thisgate could not run (infra failure; review not blocked)warning also fires when the gate did block. If the senti...
| schemaVersion: STAMP_SCHEMA_VERSION, | ||
| depth: "full", | ||
| verdict: record.verdict, | ||
| anchorDraft: record.wasDraft, |
There was a problem hiding this comment.
note (non-blocking): Step 9's wasDraft (the run's draft status) is mapped onto anchorDraft in stampFromCacheMemory, but the body-stamp regime it replaces carried plan.stampAnchorDraft — the prior anchor's draft flag at fast/flip-gated depth, not the current run's. The mismatch only ever makes the reconstructed anchor stricter (an extra full review via the ready-for-review-anchor guard), so it's cost, not lost coverage — but it contradicts the comment's claim that the cache fields carry the same information as the stamp. Consider copying stampAnchorDraft verbatim in Step 9 and preferring it over wasDraft, falling back to wasDraft for older records.
Lower-confidence observation (1)
workflows/review/lib/dispatch-gate.ts:194—disclosesSkippedDimensionrequires the phrase and dimension alias to co-occur on one physical line; a model-authored disclosure note hard-wrapped mid-phrase would false-block a conforming, disclosed run. Matching withinNote:-delimited segments (or a ~200-char window) would tolerate wrapping.
| "review": patch | ||
| --- | ||
|
|
||
| review: the re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest |
There was a problem hiding this comment.
nitpick (non-blocking): This changeset body opens with a review: prefix, unlike every other changeset in the repo — its sibling .changeset/review-dispatch-gate.md opens "The dispatch-conformance gate: ...". The prefix duplicates the "review" scope already declared in the frontmatter and renders straight into the published CHANGELOG. Consider dropping it and capitalizing: The re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest.
… dispatched sub-agent outputs are missing (dispatch-conformance gate)
…ring; sentinel-gated job failure; per-line disclosure match (review feedback)
…ites so the rewrite always runs once blocked (re-review feedback)
…template coupling, and the Conformant summary branch (third-round nits)
… to cache memory (the body stamp never survives gh-aw ingest)
…SON extraction; the gate reads out-files with the dispatcher's leniency (trial run 29893634730)
1d9d558 to
db17815
Compare
There was a problem hiding this comment.
Approved — no blocking issues found.
5 of 5 prior review threads are still unaddressed as of db17815:
5 non-blocking threads still open
- nitpick (non-blocking)
.changeset/stamp-carrier-cache-memory.md:5: This changeset body opens with areview:prefix, unlike every other changeset in the repo — its sibling `.changeset/re... - suggestion (non-blocking)
workflows/review/lib/dispatch-gate.ts:409: On a block,KEEP_ITEM_TYPESstrips the verdict, every inline comment, and the risks comment, so the author gets a red... - note (non-blocking)
workflows/review/lib/rereview-mode.ts:426: Step 9'swasDraft(the run's draft status) is mapped ontoanchorDraftinstampFromCacheMemory, but the body-stamp... - suggestion (non-blocking)
workflows/review/review.md:1659: Step 9 listsstampHunks,wasDraft, andreviewedHunksas pinned (backticked) cache keys, but the verdict is only de... - nitpick (non-blocking)
workflows/review/review.md:271: Thisgate could not run (infra failure; review not blocked)warning also fires when the gate did block. If the senti...
Plan-of-record: The deterministic orchestrator (slice 0; it absorbs round three's step 5, promoted 07-21 from cost bet to integrity fix). This PR is that first slice: a deterministic dispatch-conformance gate at the submit chokepoint, so a review verdict cannot be submitted unless the sub-agent outputs it is supposed to summarize actually exist.
The motivating evidence (two production transcripts)
The violation: Khan/webapp#40992, run 29865480728 (2026-07-21, the v1.7.0 acceptance trial). Verified from the run artifacts, not from memory:
out/contains a single self-authoredorchestrator-findings.jsonwhoseprocessfield reads"streamlined direct review (3-file, 151-line self-contained Go change)". No router ran, norouting.json, norereview-plan.json, nocorrectness-reviewer.json, no claim validation.issue (blocking)) and a REQUEST_CHANGES whose entire body isChanges requested — see inline comments.No skipped-dimension notes, no fingerprint stamp, no disclosure of any kind.The contrast: #272, run 29763410312 (2026-07-20). Same workflow version family, small PR, Low-tier budget: the transcript dispatches
pattern-triage,correctness-reviewer,skill-auditor, and the five enabled whole-change reviewers;out/carries one<agent>.jsonper dispatch plusrereview-plan.json; and the one gap (no validator output) is disclosed in the submitted body, verbatim:Note: claim validation not assessed this run (claim-validator output unavailable).So the failure is stochastic non-conformance of unknown frequency. Its consequences: model changes to sub-agents (the v1.7.0 Fable correctness-reviewer swap) silently do not run on such reviews, the claim-validator precision gate is bypassed, and a blocking verdict reaches the PR with none of the machinery the A/Bs price. Prompt rules cannot fix it: an orchestrator that skips dispatch also skips prompt-mandated self-checks. Code is the only lever (same family as v1.6.1's non-empty-body rule, #262).
The enforcement point, settled empirically
The design fork was whether
submit_pull_request_reviewexecutes inside the agent job (detect-after-the-fact fallback) or in a separate code job (blockable). From the lock file and the real runs' artifacts:/tmp/gh-aw/agent_output.json, which uploads inside theagentartifact; the separatesafe_outputsjob downloads that artifact andsafe_output_handler_manager.cjsmakes the actual GitHub API calls.safe_outputsjob, but it does supportpost-steps:in the agent job, and (verified by compiling this exact frontmatter) they land after "Ingest agent output" and before "Upload agent artifacts", i.e. after the queue is final and before it ships. Failing the agent job alone would NOT block anything:safe_outputsruns whenever the agent job is not skipped, which is why the gate rewrites the queue rather than merely failing.So the gate (
lib/dispatch-gate.ts, invoked from the newpost-steps:inreview.md) runs on the agent runner with both the final queue and the real/tmp/gh-aw/review/staging in view. On violation it:upload_artifactand the non-posting diagnostics survive, so the evidence artifact still lands),agent_output.pre-gate.jsonand writesdispatch-gate.jsonbeside it (both ride theagentartifact; the report doubles as the violation-rate instrument the plan doc's Q3 asks for), andThe submission is blocked, not detected: the safe_outputs job executes the rewritten queue and has nothing to post.
if: always()on the step because safe_outputs also executes queues from partially-failed agent jobs.The rules (per re-review depth, from
rereview-plan.json; missing plan defaults tofull)full/scoped/flip-gated:out/correctness-reviewer.jsonmust exist (Step 3 requires even a failed dispatch to stage an error note, and the shed ranking never sheds a default). One waiver, proven by staged output:pattern-triagereturned an emptyreviewFiles. An existing-but-unparseable file needs theoutput unavailabledisclosure note.fastdispatches no finding producers, so no correctness requirement there.out/claim-validator.json, or the disclosed skipped-dimension note (both review: budget and shed tuning from the 07-10 production sheds #258 wordings match: planned shed under hard-ceiling pressure, or output unavailable).full/scoped): every name inrouting.json'senabledReviewers/lensesToSpawnwith noout/<name>.jsonneeds itsnot assessed this runnote in the submitted body. Dispatched-below-planned with no disclosure is a violation.Deliberately not enforced, to keep the false-positive rate at zero:
thread-reconcilerandskill-auditorexistence (the conforming #272 run dispatched no reconciler; it had no prior threads),pattern-triageitself, and "the router ran" as an independent rule (a routerless run is already caught by rule 1). Fail-open only for the gate's own bugs (loud::warning, review unblocked); a detected violation never passes silently.Verification
pnpm test --run workflows/review(878 tests),pnpm typecheck, and eslint on the changed files are green./tmp/gh-awtree with a queued REQUEST_CHANGES + blocking comment and anout/missingcorrectness-reviewer.jsonexits 1 withcorrectness-missingandvalidator-missing-with-findings, rewrites the queue down to theupload_artifactitem, and writes both forensics files; the same tree with conforming staging exits 0 with the queue byte-identical.review.mdfrontmatter compiles clean undergh aw compile(gh-aw v0.81.6), with the gate step landing exactly between the staging upload and the agent-artifacts upload.What remains prompt-trusted after this gate
out/files (the prompt body deliberately does not mention the gate, to avoid teaching that move). Script-driven dispatch (slice 2) closes this structurally.plausibledowngrades were applied, that the verdict matches the labels: still prompt-executed until slice 3's decision chokepoints.skill-auditor/thread-reconcilerdispatch, and honestrereview-plan.json/routing.jsonstaging (the plan/routing files the gate reads are themselves CLI-written, but the orchestrator invoking those CLIs is prompt-trusted until slice 1 moves staging into pre-workflow steps).Nothing is enabled consumer-side; the gate ships with the next release like any other
workflows/reviewchange (the installed copy and lock bump at release time, per the #276 flow). Rollout note for consumers when it lands: a violated run flips from a green run with a normal-looking review to a red run that posts nothing, which is the intended loud failure mode; the gate report in theagentartifact says exactly why, and gives the violation rate for free (plan doc Q3).