Skip to content

review: remove task mode; scripted dispatch is the only mode - #289

Merged
jwbron merged 9 commits into
mainfrom
jwies/review-scripted-only
Jul 30, 2026
Merged

review: remove task mode; scripted dispatch is the only mode#289
jwbron merged 9 commits into
mainfrom
jwies/review-scripted-only

Conversation

@jwbron

@jwbron jwbron commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Graduates the deterministic pipeline: scripted dispatch becomes the only mode, task mode is removed. Stacked on #296 (the gh-aw v0.83.4 bump) over #288/#284/#283/#282/#280/#279. Plan-of-record: The deterministic orchestrator (this is the slice-4 end state shipping as the pipeline rather than the probe).

Why now

The scripted path was live-trial-gated, and the lifecycle trial was the named acceptance instrument: S4 (scripted) matched S0's verdicts on all three rounds of the seeded lifecycle (webapp#41010 vs #41013), ran 8-17% cheaper on every round, and posted a tighter, deduplicated comment set, with round 3 matching production #40996's behavior. Keeping two dispatch modes past that point means maintaining, evaling, and prompting two pipelines while all the reliability machinery (the plan-match gate, structured finals, code-owned cache record) only fully protects one of them.

What changes

  • review.md: the orchestrator prompt drops ~780 lines of turn-by-turn protocol. Phases 1-3 (triage, fan-out, contracts, normalization, provenance gate, scope filter, validation) are deleted; the scripted pipeline IS Step 3, with the thread-staging spec inlined (it previously lived in Phase 2). Steps 4-6 remain as short stubs naming the plan CLI as their owner, so step numbering and cross-references stay stable. The task-mode Step 9 record spec and the budget-guardrail choreography are deleted (the dispatcher's roster cap, the investigation-cap CLI, and the sub-agent timeout already own the budget in code). Step 7 now has exactly one signature path: the staged risks-patterns-key.txt.
  • ROUTING: the dispatch dial is retired. A leftover dispatch line warns and is ignored (a task or unknown value gets stronger retired-dial wording); routing.json always carries dispatchMode: "scripted" for the artifact and counters.
  • Frontmatter: the Agent SDK npm ci pre-agent step is unconditional.
  • The gate keeps both rule families; the task-mode-only rules are now dead weight to prune in a follow-up once this settles.

The smaller prompt also cuts the orchestrator's per-turn cache-read line on every model, and it is what makes a smaller orchestrator model realistic (the stacked follow-up PR).

Consumer impact and rollback

Consumers adopt the flip at their next release bump; nothing changes until then. Rollback is release-level pinning (the tag's review.md pins its own lib ref, so pinning back restores task mode wholesale). One caveat carried forward from the trial: rounds 2-3 ran with the correctness dimension voided by contract drift, and #288's structured finals (which address that class) have not yet run live. The next seeded lifecycle on this pin, planned as the same trial that prices the Sonnet-low orchestrator, doubles as that validation — and, since the restack below, must also be the run that first exercises this pipeline on the gh-aw v0.83.4 runtime (see the coverage gap named there).

Verification

1252 tests green (dial tests rewritten for the retired dial; the gate's note-template sync guard repointed from the deleted Step 6 text to the code templates in dispatch.ts), typecheck and eslint clean, frontmatter compiles clean (originally verified on gh-aw v0.81.6; re-verified on v0.83.4 with the restack below).

Rebased on the redistributed stack (2026-07-22)

The post-trial defect fixes and degradation-audit hardening were distributed into the PRs that introduce the code they touch (#282, #284, #288: see each PR's dated section; evidence in #290 and run 29943085279), and this branch was rebuilt on the result; its own diff against #288 is unchanged. After the rebuild this branch is green end to end: 1277 tests, typecheck and eslint clean, and gh aw compile passes with 0 errors on this branch's review.md via a scratch consumer layout.

Restacked on the gh-aw v0.83.4 bump (2026-07-27)

This branch now sits on #296, which moves the reviewer from gh-aw v0.81.6 to v0.83.4 and retires the firewall pin and the models: pricing block that release makes obsolete. The bump goes underneath rather than on top because the next step after this PR (replacing the LLM orchestrator with a behavior-defined engine) needs gh-aw >= 0.82, and because keeping the toolchain change separate keeps the bisect honest if the reviewer misbehaves.

Two things in #296 matter for the pipeline this PR makes mandatory:

  • The compiled invariants were re-verified on v0.83.4 against this branch's review.md in a scratch consumer layout: the dispatch-conformance gate still compiles between "Ingest agent output" and "Upload agent artifacts", the engine.env Bash-timeout override still replaces the generated 60s defaults (BASH_MAX_TIMEOUT_MS: 1200000, which the blocking dispatcher call needs), inline sub-agents still extract to .claude/agents (what dispatch.ts reads), the unconditional npm ci pre-agent step lands, and the safe-output handler set plus upload_artifact allowed-paths are unchanged. 0 errors, 0 warnings.
  • Sub-agent SDK retries are restored in review: move to gh-aw v0.83.4; retire the firewall pin and restore sub-agent retries #296 (dispatch-runner.ts). v0.83 sets ANTHROPIC_MAX_RETRIES=0 on the engine step, deliberately, so terminal errors reach the harness that owns retry/backoff for the orchestrator process. The dispatcher's sub-agents run inside that process with no such wrapper, so on this PR's now-mandatory scripted path a transient overload would have shed a lens on every run instead of retrying.

Rebase was clean; this branch's own diff is unchanged (9 files). 1279 tests green (the two new ones are #296's), typecheck clean.

Coverage gap the restack opens, and what closes it. The trial evidence above (S4 vs S0, three rounds) was collected on gh-aw v0.81.6, i.e. on the old sandbox; nothing has yet run this pipeline on the rootless-awf runtime. Neither CI signal closes that:

  • the live agent job on review: move to gh-aw v0.83.4; retire the firewall pin and restore sub-agent retries #296 exercises the new runtime but not this pipeline, because this repo's installed reviewer is still pinned to review-v1.7.0 (task mode, no gate, no dispatcher). It has since passed on the new runtime (run 30292838824: rootless awf, 16 MCP tools/call through the bridge-network gateway, zero firewall denials, review posted), which retires the runtime risk for the task-mode reviewer and leaves the dispatcher's share of it open;
  • the eval A/B exercises this pipeline but not the new runtime, because live-ab.ts runs on a plain ubuntu-latest runner outside gh-aw entirely (no awf, no MCP gateway).

So the untested intersection was scripted dispatch × the new runtime, and the specific risks were runtime-shaped rather than judgment-shaped: the dispatcher's single blocking Bash call surviving rootless awf, sub-agents reaching the api-proxy over the new bridge network (and still being metered by it), and the fan-out not shedding lenses now that SDK retries are set explicitly rather than inherited.

That gap is now closed by a live seeded run. Round 1 of the trial3c lifecycle was replicated against this branch's head on the v0.83.4 runtime (arm PR Khan/webapp#41118, scaffolding Khan/webapp#41117, run 30301235749), reusing the memory-expiration seed and the #41013 arm shape:

check result
blocking dispatch call under rootless awf completed; execute step 9m43s (prior arm r1: 12.4 min)
sub-agents over the bridge-network proxy 9 dispatched, every one metered ($8.14 recorded per-agent)
sheds from the ANTHROPIC_MAX_RETRIES change 0 shed, 0 timed out, 0 retried
conformance gate passed, in its compiled slot
safe-output path REQUEST_CHANGES posted, 7 inline comments
cost 1021 credits / $10.21 (prior arm r1: 593 / $5.94)

Verdict matches the prior arm's round 1, and so does recall on the seeds: the TTL-unit defect and the untested delete path both caught, the missing composite index missed exactly as #41013 missed it at r1. Rounds 2-3 (re-review depth, thread resolution, the cache-memory carrier on the new runtime) were not run.

One defect the run surfaced, not caused by this PR. Four sources flagged the same TTL defect on the same line (expiration.go:38: correctness-reviewer, completeness, holistic, first-principles) and cross-source dedup merged none of them, so three blocking comments plus a note posted for one defect; the run's only merge was a different cluster at :62. Replaying describesSameDefect over the run's claim texts clears only 2 of the 6 pairs, i.e. dedup.ts's floors (MIN_JACCARD 0.2 / MIN_OVERLAP 0.35 / MIN_SHARED_BIGRAMS 4, calibrated on one earlier run's wording) are fragile to how each lens phrases the same finding. dedup.ts is pure text arithmetic with no runtime dependency and it lands in #282, so this is neither a bump regression nor a regression in this PR; it is a tuning follow-up against #282.

@changeset-bot

changeset-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 34bedb5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
review Minor

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

@khan-actions-bot
khan-actions-bot requested review from a team, jeresig and kevinb-khan and removed request for a team July 22, 2026 16:51
Comment thread workflows/review/review.md Outdated
Comment thread workflows/review/review.md Outdated
Comment thread workflows/review/review.md Outdated
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review live A/B

Baseline: origin/main (review.md ccece9056ede); candidate: working tree (review.md 8170b094e2e3).

Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).

Metric Baseline Candidate Delta
Must-catch recall 100% 100% +0%
Verdict agreement 89% 89% +0%
Noise (unmatched posted) 59% 53% -6%
Clean false flags 0 0
Judge mean quality 0.90 0.90 +0.01
Cost $8.87 $8.35
Wall clock 1187s 940s
Cases run / skipped 9 / 0 9 / 0
Misses found-but-dropped 0 0
Findings anchor-snapped 0 0

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 --repeats to resolve smaller effects.

jwbron added a commit that referenced this pull request Jul 22, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 3 prior review threads are resolved.

Comment thread workflows/review/lib/submission.ts
* rejects safe outputs past that number, and a plan the engine cannot fully
* emit is a conformance-gate red after full spend.
*/
export const MAX_INLINE_COMMENTS = 20;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): MAX_INLINE_COMMENTS declares it MUST match the frontmatter create-pull-request-review-comment: max:, but the pairing is unenforced — no test references the constant or asserts the frontmatter value. This PR itself establishes the coupling-test pattern (dispatch-gate-hardening.test.ts couples the gate matcher to dispatch.ts templates). A one-assertion test would close it:

// parse review.md frontmatter, then:
expect(frontmatterMax).toBe(MAX_INLINE_COMMENTS);

Without it, a one-sided edit reds the conformance gate after full model spend.

export type DispatchMode = typeof DISPATCH_MODES[number];

export const DEFAULT_DISPATCH_MODE: DispatchMode = "task";
export const DEFAULT_DISPATCH_MODE: DispatchMode = "scripted";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): This flips DEFAULT_DISPATCH_MODE from task to scripted and removes task mode — a change to the reviewer's runtime default — but the release ships as review: minor, while the review-workflow-contract README states "a release that changes the reviewer's behavior bumps the major version." Is the minor bump intentional (the trial matched task mode's verdicts, so arguably verdict-equivalent), or should this be a major?

"review": minor
---

Scripted dispatch becomes the only mode: task mode is removed. The lifecycle trial (webapp#41010 vs #41013) was the acceptance instrument the scripted probe was gated on; it matched task mode's verdicts on all three rounds while running 8-17% cheaper with a tighter, deduplicated comment set, so the migration plan's slice-4 shape is now the pipeline, not the probe. The orchestrator prompt drops roughly 780 lines of turn-by-turn protocol (Phases 1-3, the task-mode Steps 4-6 and Step 9 record spec, and the budget-guardrail choreography whose shed arithmetic the dispatcher already owns); Steps 4-6 remain as short stubs naming the plan CLI as their owner so cross-references stay stable, and the thread-staging spec moves inline into the Step 3 pipeline. The ROUTING `dispatch` dial is retired: a leftover line warns and is ignored (`routing.json` always carries `dispatchMode: "scripted"`), and the Agent SDK install pre-step is unconditional. Consumers adopt the flip with their next release bump; rollback is release-level pinning. The next seeded lifecycle trial on this pin doubles as the live validation of the structured-final, open-thread-suppression, and deterministic-cache-write follow-ups, which have not yet run live together.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): The changeset notes the acceptance evidence is partial (rounds 2-3 ran with correctness voided by contract drift, and #288's structured-final/suppression/cache-write follow-ups have not run live together), yet this release both deletes the task-mode fallback and carries those follow-ups' first live run. If the next seeded trial reds, release-level rollback reverts the mode removal and the unvalidated follow-ups together — confounding which regressed. Was a one-release sequencing (live-validate the follow-ups while task mode still exists as an escape hatch, then remove it) considered?

if (collapsed.length > 0) {
const section = [
"<details>",
`<summary>Lower-confidence observations (${collapsed.length})</summary>`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (non-blocking): Cap-overflow claims (including blocking ones beyond the 20-inline cap) render under the literal heading "Lower-confidence observations". The verdict floor is correct — a collapsed blocking claim still blocks — but the two collapse causes (cap overflow vs. sub-medium confidence) are now conflated in code, so an author scanning for what blocks could reasonably treat that section as ignorable. Consider a neutral title (e.g. "Additional findings (N)") or marking collapsed blocking entries with their label.

@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — no blocking issues found.

5 of 5 prior review threads are still unaddressed as of 567c459:

5 non-blocking threads still open

jwbron added a commit that referenced this pull request Jul 22, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 567c459 to a1ec669 Compare July 22, 2026 19:47
jwbron added a commit that referenced this pull request Jul 27, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from a1ec669 to 550b09f Compare July 27, 2026 17:43
@jwbron
jwbron changed the base branch from jwies/review-trial-followups to jwies/gh-aw-v0.83.4 July 27, 2026 17:53
@khan-actions-bot
khan-actions-bot requested a review from a team July 27, 2026 17:54
jwbron added a commit that referenced this pull request Jul 27, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 550b09f to 4bbccbe Compare July 27, 2026 18:14
jwbron added a commit that referenced this pull request Jul 27, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 4bbccbe to fc82b0d Compare July 27, 2026 18:38
Comment thread workflows/review/README.md Outdated
Comment on lines +212 to +213
(`lib/dispatch.ts`); the `dispatch` dial is retired (task mode was removed
after the lifecycle trial held; a leftover `dispatch` line warns and is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to refer to something that no longer exists here...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Dropped the dial entirely from the docs: the how-Step-3-runs prose moved into "How it works" (it stopped being a ROUTING directive when the dial went away), and the README no longer names task mode. Also swept the leftovers in code comments and in the consumer-facing ROUTING warning for a stale dispatch line, which was explaining the retirement in terms of a mode nobody can see anymore. 48f5393

jwbron added a commit that referenced this pull request Jul 29, 2026
…rom the docs (#289 feedback)

The ROUTING directive list documented a dial that no longer exists. Move
the how-Step-3-runs prose into How it works (it is not a directive) and
stop naming the removed task mode in the README, the code comments, and
the consumer-facing ROUTING warning.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 8513fc0 to 9f11e6f Compare July 29, 2026 19:44
jwbron added a commit that referenced this pull request Jul 30, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
jwbron added a commit that referenced this pull request Jul 30, 2026
…rom the docs (#289 feedback)

The ROUTING directive list documented a dial that no longer exists. Move
the how-Step-3-runs prose into How it works (it is not a directive) and
stop naming the removed task mode in the README, the code comments, and
the consumer-facing ROUTING warning.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 5845bad to 341b42b Compare July 30, 2026 17:39
jwbron added a commit that referenced this pull request Jul 30, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
jwbron added a commit that referenced this pull request Jul 30, 2026
…rom the docs (#289 feedback)

The ROUTING directive list documented a dial that no longer exists. Move
the how-Step-3-runs prose into How it works (it is not a directive) and
stop naming the removed task mode in the README, the code comments, and
the consumer-facing ROUTING warning.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 341b42b to b83a21d Compare July 30, 2026 17:47
jwbron added a commit that referenced this pull request Jul 30, 2026
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
jwbron added a commit that referenced this pull request Jul 30, 2026
…rom the docs (#289 feedback)

The ROUTING directive list documented a dial that no longer exists. Move
the how-Step-3-runs prose into How it works (it is not a directive) and
stop naming the removed task mode in the README, the code comments, and
the consumer-facing ROUTING warning.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from b83a21d to 4c2b23a Compare July 30, 2026 17:51
@github-actions

Copy link
Copy Markdown
Contributor

Risks & patterns — re-review at 4c2b23a

Overall risk: Moderate. This is a subtractive change — removing task-mode dispatch and making scripted the only mode — landed alongside a review: minor changeset. The logic touched (dispatch gate, submission/skip predicate, routing-config graduation) is the reviewer's own control plane, so a regression here degrades the reviewer silently rather than loudly.

Higher-risk surfaces reviewed:

  • routing-config.tsDISPATCH_MODES trimmed to ['scripted'], DEFAULT_DISPATCH_MODE flipped, ROUTING parser now warns-and-ignores a leftover dispatch: line.
  • submission.ts — code-owned posting bar + skipSubmission predicate (reads the prior stamp's verdict).
  • dispatch-gate.ts — conformance gate accepts/derives skipSubmission.
  • review.md — orchestrator prose realigned to scripted-only.

Cross-cutting patterns:

  1. Task-mode retirement (logic): routing-config.ts, dispatch.test.ts, router-dispatch-mode.test.ts — modes list trimmed, default flipped, warn-and-ignore path added.
  2. Task-mode text/reference cleanup (non-logic): cache-record.ts, cache-record.test.ts, rereview-mode.ts — comments/JSDoc/reason strings only.

Standing concerns (not blocking, all pre-existing open threads): the stamp-vs-live-state skip under dismiss-stale-approvals (submission.ts:547), unsurfaced retired-dial routingConfig.warnings + the now-stale surface-warnings instruction (review.md:480 / routing-config.ts:230), and the minor-vs-major bump question. See the 9 open threads carried forward in the review body.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ APPROVE — re-review at 4c2b23a (full depth).

Ran all reviewer lenses over the current diff (14 files; the substantive change is the removal of task-mode dispatch and the flip of DEFAULT_DISPATCH_MODE to scripted). The new commits on this branch since the prior approval are refactors — splitting submission.test.ts under the line budget, folding the NOTIFIED signature into the code-owned guidance key, making the redundant-approval skip a code-owned predicate. No new defects surfaced: every concern the lenses raised re-derives one of the 9 review threads already open from earlier runs, and none rises to a confirmed merge-blocker.

Two lenses proposed escalating an existing open thread to blocking; both stay non-blocking:

  • submission.ts:547 — the redundant-approval skip reads the stamped verdict rather than live review state (the dismiss-stale-approvals interaction). Already tracked by an open thread, and rereview-mode.ts documents reading the prior verdict from the stamp "whatever the review state" as intended, so this is design, not a bug.
  • routing-config.ts:230 / review.md:480 — retired-dial routingConfig.warnings have no render path and the surface-warnings instruction is now stale. Real but latent (no effect when warnings is empty, as in this run) and already an open thread.

No threads were resolved by this change — all 9 remain the author's to address (none blocking).

9 of 9 prior review threads are still unaddressed as of 4c2b23a:

9 non-blocking threads still open

Base automatically changed from jwies/gh-aw-v0.83.4 to main July 30, 2026 18:36
jwbron added 9 commits July 30, 2026 11:38
… 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.

Two tests, neither of which injects a signature: a repo with a
`.github/NOTIFIED` gets the component in its staged key without
`notified.json` being staged first, and changing only the NOTIFIED rules
moves the key. The first fails against the old read-from-file version. A
repo without the file still stages the pre-feature key.
…tch is the only mode

The lifecycle trial (webapp#41010 vs #41013) was the acceptance instrument
the scripted probe was gated on: matched verdicts on all three rounds,
8-17% cheaper, tighter comment set. The orchestrator prompt drops ~780
lines of turn-by-turn protocol (Phases 1-3, task-mode Steps 4-6, the
Step 9 record spec, the budget-guardrail choreography); Steps 4-6 stay as
stubs naming the plan CLI as their owner, and the thread-staging spec
moves inline into the Step 3 pipeline. The ROUTING dispatch dial is
retired (leftover lines warn and are ignored; routing.json always carries
dispatchMode scripted) and the SDK install pre-step is unconditional.

Verification: 1252 tests green, typecheck and eslint clean, frontmatter
compiles clean under gh-aw v0.81.6.
- Step 1 list item de-garbled: the run-start removal had fused two list
  markers and duplicated the pr-context.json fragment in the shipped
  orchestrator prompt.
- The task-mode Step 5 posting bar is now code in submission.ts: claims
  rank blocking first then confidence descending, at most 20 post
  inline (MAX_INLINE_COMMENTS, pinned to the frontmatter's
  create-pull-request-review-comment max so the engine can always emit
  the full plan), sub-medium-confidence (< 0.5) non-blocking claims
  never post inline, and everything else folds into a single collapsed
  details block riding the top-ranked comment (or the review body when
  nothing posts inline). The verdict still computes from ALL claims, so
  a collapsed blocking claim blocks. Step 5's stub names the code-owned
  bar.
- Step 9's opening code fence unglued from the sentence so the block
  renders as a fence in CommonMark.
…rom the docs (#289 feedback)

The ROUTING directive list documented a dial that no longer exists. Move
the how-Step-3-runs prose into How it works (it is not a directive) and
stop naming the removed task mode in the README, the code comments, and
the consumer-facing ROUTING warning.
…he max-lines budget

Absorbing the trial follow-ups put this file at 1094 lines against the
1000-line cap. Move the three follow-up suites (risks/patterns key
staging, the inline posting bar, the suppression verdict floor) into
submission-trial-followups.test.ts with local fixture copies, the same
split dispatch-gate-hardening.test.ts uses. Same 41 tests.
… the code-owned guidance key

main's NOTIFIED support makes the Review Guidance comment cover risks,
patterns, AND notifications, and it added `notified.json`'s signature to
the Step 7 idempotency key the orchestrator used to compose by hand. This
branch removed that hand composition in favour of the plan CLI's staged
key, which did not know about the notified set: a run where only the
NOTIFIED matches changed would compute an unchanged key and never re-post
the new mentions. Fold the signature into computeRisksPatternsKey so both
sides of the repost decision see it.
…eam explanation

Slice 4 replaced the credentials-based description of the emission seam
with the real reason (the queue needs no credentials; the agent sandbox
mounts ${RUNNER_TEMP}/gh-aw read-only). Rebasing this branch's task-mode
removal re-introduced the older wording; restore the correction.
… one code-owned predicate

The prompt described the skip (APPROVE, no comments, no Note: lines, no
accountability section) while the conformance gate required the body to
equal the bare approve line. The new collapsed low-confidence `<details>`
section rides in the body and is neither a Note: line nor an accountability
section, so an APPROVE plan carrying only that section satisfied the prompt
and red-flagged at the gate: the orchestrator queued nothing, the gate
raised submission-plan-mismatch after full spend, and both the approval and
the observations were withheld on every later run until an inline or
blocking finding appeared.

The plan CLI now computes `skipSubmission` (APPROVE, zero comments, bare
approve body modulo the ingest sanitizer, and a prior APPROVE stamp) and
both the prompt and the gate read that one field. The prior-APPROVE term
also makes the gate stricter in the right direction: a first approval can
no longer be silently skipped.
…t file duplicates

The submission CLI's NOTIFIED tests moved into their own
`submission-notified.test.ts` downstack, so the copies this branch's split
carried are now duplicates of that file. Remove them here; the split keeps
the three describes it was written to move.
@jwbron
jwbron force-pushed the jwies/review-scripted-only branch from 4c2b23a to 34bedb5 Compare July 30, 2026 18:38
@jwbron
jwbron merged commit 237d540 into main Jul 30, 2026
6 checks passed
@jwbron
jwbron deleted the jwies/review-scripted-only branch July 30, 2026 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants