Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-sonnet-low-orchestrator.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"review": minor
---

The orchestrator moves to Claude Sonnet 5 at low reasoning effort (`engine.model: claude-sonnet-5` plus `CLAUDE_CODE_EFFORT_LEVEL: low` in the engine env). With task mode removed the orchestrator's job is transcription plus a handful of small judgments (thread staging, the router tier questions, Steps 7-8), the dispatch-conformance gate turns any transcription drift into a red run rather than a wrong review, and the lifecycle trial measured the orchestrator line at $2.0-3.1/run on Opus, the largest non-reviewer cost; Sonnet at roughly 40% of Opus pricing with a low-effort dial cuts most of it while keeping near-Opus instruction-following. Sub-agents are untouched: they pin their own models, and the dispatcher's runner strips `CLAUDE_CODE_EFFORT_LEVEL` from the sub-agent environment so the reviewer roles can never silently inherit the orchestrator's effort dial. The pinned firewall (gh-aw-firewall v0.27.27) already prices `claude-sonnet-5`; a `models:` entry keeps gh-aw's cost display correct. Live-trial-gated: the seeded lifecycle that validates the scripted-only pin runs this branch as its second arm against an Opus-orchestrator control.
2 changes: 1 addition & 1 deletion workflows/review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ sub-agent models — this table is the human-facing summary:

| Role | Model | Effort | Why |
| --- | --- | --- | --- |
| orchestrator | `claude-opus-4-8` | high | Owns every GitHub/safe-output decision |
| orchestrator | `claude-sonnet-5` | low | Transcribes the plan and stages threads; the gate reds any transcription drift, and the reasoning lives in the sub-agents (the runner strips the orchestrator's effort env from theirs) |
| `pattern-triage` | `claude-sonnet-4-6` | medium | Cheap first-pass triage |
| `thread-reconciler` | `claude-opus-4-8` | medium | Reconciliation |
| `correctness-reviewer` | `claude-fable-5` | high | Whole-change reviewer; bug-finding recall is the load-bearing metric |
Expand Down
22 changes: 22 additions & 0 deletions workflows/review/lib/dispatch-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,27 @@

import type {AgentRequest, AgentResult, AgentRunner} from "./dispatch";

/**
* The environment the sub-agents run under: the sandbox env minus the
* orchestrator's effort dial. The engine sets CLAUDE_CODE_EFFORT_LEVEL for
* the ORCHESTRATOR (Sonnet at low effort), and env vars reach every process
* in the sandbox — without this filter the SDK-spawned reviewers would
* silently inherit low effort too, degrading the reasoning roles the
* orchestrator dial was never meant to touch. Sub-agents keep the harness
* 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.

env: Record<string, string | undefined>,
): Record<string, string> =>
Object.fromEntries(
Object.entries(env).filter(
(entry): entry is [string, string] =>
entry[1] !== undefined &&
entry[0] !== "CLAUDE_CODE_EFFORT_LEVEL",
),
);

/**
* Build the production runner. The SDK and zod are imported lazily here
* (both installed by the scripted-mode `npm ci` pre-agent step); zod is the
Expand Down Expand Up @@ -57,6 +78,7 @@ export const createSdkRunner = async (): Promise<AgentRunner> => {
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?

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.

};
// The structured-final channel (trial suggestion h): an in-process
// MCP tool whose handler runs the same contract parse the collection
Expand Down
13 changes: 13 additions & 0 deletions workflows/review/lib/dispatch-trial-followups.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
type DispatchFs,
} from "./dispatch";
import {computeDiffProvenance} from "./provenance";
import {subAgentEnv} from "./dispatch-runner";

/**
* Post-trial follow-up tests for the scripted dispatcher: the
Expand Down Expand Up @@ -322,3 +323,15 @@ describe("open-thread suppression (trial suggestion g)", () => {
expect(result.claims).toHaveLength(1);
});
});

describe("subAgentEnv (the orchestrator effort dial never reaches sub-agents)", () => {
it("strips CLAUDE_CODE_EFFORT_LEVEL and undefined values, keeps the rest", () => {
expect(
subAgentEnv({
CLAUDE_CODE_EFFORT_LEVEL: "low",
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.

});
});
43 changes: 33 additions & 10 deletions workflows/review/review.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,20 +167,34 @@ observability:
headers:
x-sentry-auth: ${{ secrets.GH_AW_OTEL_SENTRY_AUTHORIZATION }}

# Pin the orchestrator to a specific model version rather than a floating tier alias, so
# the review doesn't silently change behavior when a new Opus ships. If we use Opus, we
# use Opus 4.8. Sub-agents pin their own versions in their frontmatter below.
# Pin the orchestrator to a specific model version rather than a floating tier
# alias, so the review doesn't silently change behavior when a new model ships.
# Sub-agents pin their own versions in their frontmatter below; the reasoning
# stays on their models (Fable/Opus). The orchestrator itself is Sonnet at LOW
# effort: with task mode removed its job is transcription plus a handful of
# small judgments (thread staging, the tier questions, Steps 7-8), the
# dispatch-conformance gate turns any transcription drift into a red run
# rather than a wrong review, and the trial measured the orchestrator line at
# $2-3/run on Opus — the single largest non-reviewer cost. claude-sonnet-5 is
# its own pin (Sonnet 5 has no dated alias).
#
# The `env:` overrides gh-aw's 60s Bash tool timeout defaults (compile-verified:
# these replace the generated values on the engine execution step). Needed by the
# scripted dispatch mode (ROUTING `dispatch scripted`): the orchestrator invokes
# the deterministic dispatcher (lib/dispatch.ts) as ONE blocking Bash call that
# waits for the whole sub-agent fan-out, which takes minutes, not seconds. The
# job-level timeout-minutes still bounds the run.
# CLAUDE_CODE_EFFORT_LEVEL sets the orchestrator's reasoning effort (it
# outranks the CLI flag and settings.json). It reaches the whole sandbox env,
# so the dispatcher's runner (lib/dispatch-runner.ts) strips it from the
# sub-agent environment — reviewer effort must never silently follow the
# orchestrator dial.
#
# The BASH_* env overrides replace gh-aw's 60s Bash tool timeout defaults
# (compile-verified: these replace the generated values on the engine
# execution step). Needed because the orchestrator invokes the deterministic
# dispatcher (lib/dispatch.ts) as ONE blocking Bash call that waits for the
# whole sub-agent fan-out, which takes minutes, not seconds. The job-level
# timeout-minutes still bounds the run.
engine:
id: claude
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.

BASH_DEFAULT_TIMEOUT_MS: "60000"
BASH_MAX_TIMEOUT_MS: "1200000"
timeout-minutes: 20
Expand Down Expand Up @@ -217,6 +231,15 @@ models:
output: 5.0e-05
cache_read: 1.0e-06
cache_write: 1.25e-05
# The orchestrator's model (same caveat: keeps gh-aw's cost display
# correct; the firewall api-proxy's own table, v0.27.27, already
# prices it).
claude-sonnet-5:
cost:
input: 3.0e-06
output: 1.5e-05
cache_read: 3.0e-07
cache_write: 3.75e-06

# The shared review workflow is more than this markdown file: its deterministic
# pieces (the finding schema and validator today; the router, computed verdict, and
Expand Down
Loading