Skip to content

review: orchestrator on Sonnet 5 at low effort - #290

Closed
jwbron wants to merge 1 commit into
jwies/review-scripted-onlyfrom
jwies/review-sonnet-low-orchestrator
Closed

review: orchestrator on Sonnet 5 at low effort#290
jwbron wants to merge 1 commit into
jwies/review-scripted-onlyfrom
jwies/review-sonnet-low-orchestrator

Conversation

@jwbron

@jwbron jwbron commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

The orchestrator moves to Claude Sonnet 5 at low reasoning effort. Stacked on #289 (task mode removed) over #288 and the rest of the stack; top of the stack.

Why this is safe to try now

With #289, the orchestrator's whole job is: stage the review threads, answer the router's tier questions, invoke three CLIs, transcribe the submission plan into safe-output calls, decide Steps 7-8, and upload the artifact. The reasoning lives in the sub-agents, which pin their own models (Fable/Opus) and are untouched here. The two guardrails that bound a weaker orchestrator: the dispatch-conformance gate compares every queued output against the staged plan and turns transcription drift into a red run rather than a wrong review, and the deterministic cache writer refuses to record anything the plan and queue do not corroborate.

Why bother

The lifecycle trial measured the scripted orchestrator's residual at $2.0-3.1/run (of $5.9-7.4 total), the single largest non-reviewer cost line. Sonnet 5 is 40% of Opus pricing per token, low effort cuts turn overhead further, and #289's prompt diet (~780 lines) shrinks the per-turn cache-read line on top.

Mechanics

  • engine.model: claude-sonnet-5 (Sonnet 5 has no dated alias; the bare id is the pin), plus CLAUDE_CODE_EFFORT_LEVEL: "low" in the engine env — the documented effort mechanism for headless runs, outranking flags and settings.
  • Sub-agent shield: engine env reaches every process in the sandbox, so the dispatcher's runner now spawns sub-agents with CLAUDE_CODE_EFFORT_LEVEL stripped from their environment (subAgentEnv in dispatch-runner.ts, unit-tested). Reviewer effort must never silently follow the orchestrator dial.
  • The pinned firewall (gh-aw-firewall v0.27.27) already prices claude-sonnet-5 in its api-proxy credits table (verified against the tag), so orchestrator calls are metered, not rejected; a models: frontmatter entry keeps gh-aw's cost display correct, same pattern as the Fable entry.
  • README per-role table updated.

Gating

Live-trial-gated, per the standing rule: the next seeded webapp lifecycle runs two arms on identical pushes — control pinned to #289's branch (Opus orchestrator), treatment pinned to this branch — so one round prices the demotion and validates the scripted-only pin plus the #288 features at the same time. Watch items for the trial: red-run (gate-block) rate, thread-staging fidelity (reconciler inputs), Step 7 comment quality, and the early-exit judgment.

Verification

1253 tests green, typecheck and eslint clean, frontmatter compiles clean under gh-aw v0.81.6 (compiled lock verified to carry agent_model: claude-sonnet-5 and the effort env on the engine step).

… 5 at low effort

With task mode removed the orchestrator transcribes the plan and makes a
handful of small judgments; the gate reds any transcription drift. The
trial measured the orchestrator line at $2.0-3.1/run on Opus, the largest
non-reviewer cost. engine.model moves to claude-sonnet-5 with
CLAUDE_CODE_EFFORT_LEVEL=low in the engine env; the dispatcher's runner
strips that variable from the sub-agent environment so reviewer roles
never inherit the orchestrator dial. The pinned firewall (v0.27.27)
already prices claude-sonnet-5; a models: entry keeps the cost display
correct. Live-trial-gated as the treatment arm of the next seeded
lifecycle.
@changeset-bot

changeset-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: e0f9a3b

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, jaredly and somewhatabstract and removed request for a team July 22, 2026 16:54
@github-actions

Copy link
Copy Markdown
Contributor

Review Guidance

github-actions (2 files)
File Reason
dispatch-runner.ts The shared reviewer's sub-agent runner; the new env filter changes how every sub-agent is spawned on each consuming PR, so a bug here silently alters reviewer behavior fleet-wide.
review.md The shared workflow frontmatter; this moves the orchestrator model and effort for every consuming repo, so the model/cost/env surface should be verified before merge.
Excluded from review (2 files)

Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:

  • .changeset/review-sonnet-low-orchestrator.md — formatting-only
  • workflows/review/README.md — formatting-only

@khan-actions-bot
khan-actions-bot requested a review from a team July 22, 2026 17:14
REVIEW_REPO_ROOT: "/work",
EMPTY: undefined,
}),
).toEqual({REVIEW_REPO_ROOT: "/work"});

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.

note (non-blocking): toEqual ignores properties whose value is undefined, so the undefined-stripping half of this assertion is vacuous — removing the entry[1] !== undefined clause in subAgentEnv would leave this test green. Use toStrictEqual to actually guard it.

Suggested change
).toEqual({REVIEW_REPO_ROOT: "/work"});
).toStrictEqual({REVIEW_REPO_ROOT: "/work"});
Low-confidence (1)
  • .changeset/review-sonnet-low-orchestrator.md:5 — Prose says Sonnet is ~40% of Opus pricing, but the added cost-table numbers ($3/$15 per M) are ~20% of Opus list ($15/$75); the table values are correct, the prose figure is imprecise.

* default (their definitions' effort annotations remain the human-facing
* table in the README until gh-aw grows a per-agent effort field).
*/
export const subAgentEnv = (

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): This strips exactly one key, so any future orchestrator-only engine-env dial (or a model bump with a different default effort) would pass through to sub-agents by default. Since scripted dispatch owns sub-agent spawns, consider inverting to a pass-through allow-list plus an explicit per-role effort, so future engine-env additions are opt-in rather than leak-by-default.

model: claude-opus-4-8
model: claude-sonnet-5
env:
CLAUDE_CODE_EFFORT_LEVEL: "low"

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): The Opus→Sonnet swap delivers the dominant cost cut on its own; the low effort dial is what introduces the sandbox-wide env propagation and the subAgentEnv shield. Since the trial arm bundles model+effort, it won't isolate the marginal low-vs-default saving — if trial telemetry can split orchestrator thinking-token cost, worth confirming the low dial pays for its added mechanism.

allowedTools,
permissionMode: "bypassPermissions",
abortController: abort,
env: subAgentEnv(process.env),

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 shield assumes the SDK treats options.env as a full replacement rather than a merge over process.env; since the package is npm-ci'd at run time, this isn't checkable where the unit tests run. Would it be worth having a sub-agent echo whether CLAUDE_CODE_EFFORT_LEVEL is present in its environment during the gating trial, to make the shield an observed property rather than an SDK-version assumption?

allowedTools,
permissionMode: "bypassPermissions",
abortController: abort,
env: subAgentEnv(process.env),

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.

note (non-blocking): The unit test covers the pure subAgentEnv helper, but nothing exercises createSdkRunner actually passing env: subAgentEnv(process.env) to the SDK — the wiring that does the real work is verified only by inspection. This matches the module's no-SDK-in-unit-tests design, so it's a convention-aligned gap rather than a demand.

@github-actions

Copy link
Copy Markdown
Contributor

Review live A/B

Baseline: origin/jwies/review-scripted-only (review.md 0c10a3fa3e14); candidate: working tree (review.md 17c38ae48677).

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

Metric Baseline Candidate Delta
Must-catch recall 100% 100% +0%
Verdict agreement 100% 89% -11%
Noise (unmatched posted) 59% 55% -4%
Clean false flags 0 0
Judge mean quality 0.90 0.90 -0.00
Cost $8.61 $9.78
Wall clock 1052s 1149s
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.

Agent failures

  • incident-cache-missing-key: claim-validator: malformed output: Expected property name or '}' in JSON at position 1 (line 1 column 2) failed

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

jwbron commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Round-1 trial report: Sonnet-low orchestrator vs the prior lifecycle arms

Seeded memory-expiration content, identical to the prior lifecycle's round 1 (20cee294 + fee783fa). Arm under test: this PR's pin (scripted-only pipeline, task mode removed, Sonnet 5 orchestrator at low effort), on webapp #41029, run 29943085279. Baselines: S0 = webapp#41010 r1 (task mode + gate, Opus orchestrator), S4 = webapp#41013 r1 (scripted, Opus orchestrator, pin 7170b49). Lifecycle rounds 2-3 are deliberately not run yet.

1. Headline

S0 r1 (#41010) S4 r1 (#41013) sonnet-low r1 (#41029)
Verdict REQUEST_CHANGES REQUEST_CHANGES REQUEST_CHANGES
Inline comments 5 5 9
Reported credits (agent_usage, the prior report's metric) 710.8 593.5 421.4
Proxy-metered credits (what the cap enforces) 1,054.5 1,076.6 1,062.4
Orchestrator share est. ~33% $1.99 SDK / 14 turns $2.55 SDK / 26 turns / 153.1 proxy credits
Agent wall clock 13.9 min 12.4 min 11.8 min
Correctness dimension delivered voided ("output unavailable" note) delivered (3 blocking findings)
Dispatch-conformance gate pass pass pass (all 9 roster agents dispatched, no sheds)
Cache record model-written model-written code-written, fingerprints verbatim

Two cost accountings diverge sharply and both are real: by the metric the prior tables used, sonnet-low is 29% under S4 and 41% under S0; by the proxy meter that enforces the cap, all three runs are within 2% of each other (1,050-1,077), because sub-agent spend dominates and is constant. The proxy series also explains the superseded first attempt: this content has NEVER fit the default 1000 cap on any arm; the prior trial ran at max-ai-credits: 2500 and my first attempt failed to replicate that, dying at 1,016 after emission ($10, counted honestly; superseded PR #41023 closed).

2. Defect by defect

Seed / trap S0 r1 S4 r1 sonnet-low r1
AddDate months-for-days (blocking) caught, issue (blocking) caught via first-principles (correctness was voided), pr-level anchor caught by correctness, issue (blocking) at expiration.go:38, correct anchor, with suggested patch
Missing deletion-path test (blocking) caught, todo (blocking) caught via skill-auditor out-of-lane caught by correctness, todo (blocking) at expiration_test.go:15
Unbounded expiration read (real non-seed finding, the f9cb6a1 lens teaching) flagged (suggestion) flagged (KeysOnly variant, suggestion) flagged blocking by correctness, validator downgraded to suggestion (non-blocking) (plausible, not confirmed): the three-state gate working as intended
DeleteMulti-chunking trap (must NOT flag) silent silent silent

Recall on the seeds is 2/2 on every arm. The load-bearing difference: on the prior S4 pin both seeds survived only because OTHER reviewers happened to cover for the voided correctness dimension; on this pin the correctness reviewer itself delivered them, with better anchors and a committable suggestion.

3. Defect 13, answered (the "correctness output unavailable" question)

#41013's round 1 posted with Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable): the chronic contract-drift class voided the dimension even on the final r1 run, not just r2/r3 as the prior report's caveat emphasized. On this pin the correctness reviewer drifted in shape AGAIN (a ReportFindings-style object: summary/anchor/category, no top-level subject/path), and the pipeline absorbed it: the near-miss salvage plus the structured-final submit_result channel accepted the payload, and all three findings landed with correct labels and anchors. No separate S4 re-run is needed: this run is the fixed pipeline live, and since sub-agents are untouched by the orchestrator swap, the correctness recovery is attributable to the dispatch-layer fixes, not the model change.

4. Quality and noise

  • Duplication regressed, in a specific way. 9 comments vs 5, and the missing-test seed was posted FOUR times (correctness todo at test:15, skill-auditor question at test:58, test-adequacy todo at expiration.go:62, first-principles thought at :38). The review: hand off out-of-lane observations instead of dropping them #245 cross-source merge only fires within a 2-line same-path window, and these copies sit across two files, so exactly one merge note posted. Prior rounds' duplicates happened to share anchors; these did not. Ledger candidate: a path-relaxed same-defect merge for cross-file duplicates (the open-thread suppression already trusts the same similarity floor without a line window).
  • The remaining 3 non-seed comments (holistic lifecycle question, first-principles wrong-layer question and native-TTL suggestion) have close analogues in both baselines' comment sets; they are the roster's character, not orchestrator noise.
  • Transcription fidelity was clean: the tool sequence is exactly the protocol (one dispatcher call, one plan call, one early-exit check, one thread fetch, 9 comment emissions, one submit, one cache-record call), the gate found the queue plan-conformant, and the body carries no invented text.
  • One visible blemish: the threat-detection banner. The posted review opens with gh-aw's "[!CAUTION] agentic threat detected" block. Root cause found: the engine env (Sonnet + CLAUDE_CODE_EFFORT_LEVEL: low) also applies to gh-aw's threat-detection job, and the low-effort detection agent stopped emitting the required THREAT_DETECTION_RESULT: line, so warn mode prepended the banner ("results could not be parsed"). This needs a fix on this branch before any further rounds: scope the effort env away from the detection job (or pin detection's model/effort explicitly).

5. First live evidence on the #288 features

  • Structured finals / salvage: correctness delivered through a drifted shape (above). Observability gap found: dispatch-result.json (with perAgent.structuredFinal, retries, and per-agent cost) is not in the uploaded artifact, so whether the tool or the text fallback carried it is not distinguishable post-hoc; add it to the Step 9 artifact set.
  • Deterministic cache record: first live write. pr-41029.json carries the staged diffFingerprint/reviewedHunks/stampHunks verbatim, the queued verdict, and wasDraft: false; no hand-serialization happened at all (the prompt now forbids it).
  • Open-thread suppression: not exercisable in round 1 (no threads); rounds 2-3 are its test.

6. Orchestrator economics, honestly

Sonnet-low did NOT cheapen the orchestrator line by the naive 40%: turn count nearly doubled (26 vs 14), so SDK-estimated orchestrator cost rose ($2.55 vs $1.99) while proxy credits landed around 153 vs an estimated ~100 for the Opus orchestrator. Low effort takes smaller steps; the per-token discount paid for the extra steps. The big reported-metric win (421 vs 593) appears to come mostly from engine-side accounting of the cheaper model against the same cache traffic. Worth one arm at medium effort before concluding; the turn inflation may also shrink as the retired prompt text ages out of the pin (this run already carried the 780-line-smaller prompt).

7. Issues opened by this round (ledger)

  1. Threat-detection env leak (banner on the posted review): fix on this branch before rounds 2-3.
  2. Cross-file duplicate posting of one defect (4 copies of the missing-test seed): dedup window extension candidate.
  3. dispatch-result.json missing from the run artifact: add to Step 9 uploads.
  4. The review-trial skill does not record the 2500-credit trial cap; my first attempt died at the default 1000 (superseded, ~1,016 credits spent). Skill doc update.
  5. Measurement: agent_usage vs proxy-meter credits diverge ~2.5x; the cap enforces the proxy meter. Any future cap tuning should be reasoned in proxy units.

8. Status and spend

Round 1 complete and scored. Lifecycle paused before the re-review rounds per the operator. Spend so far (proxy units): 1,016 superseded + 1,062 scored, roughly $21 of the approved ~$45+margin envelope. Next steps when unpaused: fix the detection-env leak, push 06cc4448 (bad fix) and 60e7457b (good fix) at the same boundaries as #41013, score the lifecycle features, then export the corpus skeletons and clean up.

@jwbron

jwbron commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Closing unmerged after the round-1 trial (report above): the outcome matches the operator's prior. The demotion is mechanically sound (clean transcription, gate-conformant queue, correct code-written cache record), but the orchestrator line did not actually get cheaper where it counts: low effort roughly doubled the turn count (26 vs 14), so SDK-estimated orchestrator cost rose ($2.55 vs $1.99) and proxy-metered credits landed around 153 vs ~100 for the Opus orchestrator, while the headline "421 vs 593" delta was mostly engine-side accounting of the cheaper model against the same cache traffic, not reduced spend. The swap also surfaced a real integration hazard: the engine env (model + CLAUDE_CODE_EFFORT_LEVEL) compiles into gh-aw's threat-detection job too, whose low-effort run stopped emitting the THREAT_DETECTION_RESULT marker, so every posted review wore a spurious CAUTION banner.

What survives this PR's closure, since it ran on the stack below rather than on this change: the correctness dimension delivered through a drifted contract shape (the #288 structured finals plus salvage, resolving the defect-13 class that voided #41013's rounds), the deterministic cache record's first live write, and the scripted-only pipeline of #289. The trial ledger items land with the stack: cross-file duplicate posting of one defect (dedup window), dispatch-result.json missing from the run artifact, the 2500-credit trial cap missing from the review-trial skill doc, and the detection-env scoping hazard for any future engine-env change.

If the orchestrator model question is revisited, the two measured leads are an effort: medium arm (turn inflation, not token price, is what erased the margin) and scoping any effort env strictly to the agent job. Trial PRs and branches in webapp are closed and deleted; transcripts and artifacts are with the operator.

@jwbron

jwbron commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Correction to one line of the round-1 report above: the AddDate catch was described as arriving "with better anchors and a committable suggestion", but no committable suggestion actually posted. The reviewer authored the one-line fix, in its drifted ReportFindings-style output under suggested_patch (see out/correctness-reviewer.json in the run 29943085279 artifact), and fromLabelShape read only the contract's suggestion key, so the salvage kept the finding but dropped the fix; the posted comment on Khan/webapp#41029 carried no suggestion fence. Fixed in 6795934 on #282 (the salvage now accepts suggested_patch when suggestion is absent); the trial defect fixes were distributed to the PRs that introduce the code they touch, so this one lives with the dispatcher's contract parsing; see the dated hardening section of #282's description.

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.

1 participant