review: move the whole reviewer roster to Opus 5 - #294
Conversation
🦋 Changeset detectedLatest commit: 470b456 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 |
jwbron
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 |
…ins across REST and GraphQL Both faults from the second trial run (Khan/webapp#41140, run 30416237794). The agent died at turn 1 with `API Error: 400 Model "claude-opus-5" has no AI credits pricing`. claude-opus-5 is in no gh-aw-firewall release's pricing table and no sandbox.agent.version pin can fix that, so the model needs `models.default-ai-credits-pricing` (gh-aw >= v0.83.0), exactly as #294 does for the reviewer roster. The comment this replaces asserted the opposite, reasoning that the firewall was past the release which priced Fable 5; Fable 5 being priced says nothing about Opus 5. Staging reported threadCount: 0 on a PR carrying five reviewer threads. REST spells an App's login `github-actions[bot]` and GraphQL spells the same actor `github-actions`, and staging reads threads over GraphQL but reviews over REST, so one configured spelling cannot match both. Comparison is now suffix-insensitive in staging and in the worklist's ownership guard. The unit tests could not have caught this: every fixture was written in the REST spelling.
…urs the pricing fallback Adding models.default-ai-credits-pricing was necessary but not sufficient: the proxy still 400'd claude-opus-5 for want of pricing. The staged awf-config.json from Khan/webapp#41140 run 30421726630 carried `apiProxy.defaultAiCreditsPricing: {input: 5, output: 25}` and the request was rejected anyway, so the field reaches the config and the compiler's default firewall (v0.27.42) ignores it. #294 verified awf v0.27.27 accepts and maps that field, which is why the reviewer pins it. Autofix inherited the default instead. Pinning to the version with evidence behind it.
…back does not work anywhere Three live runs on Khan/webapp#41140, each isolating one variable: 30416237794 no fallback configured 400 30421726630 fallback + firewall v0.27.42 (compiler default) 400 30422315631 fallback + firewall v0.27.27 (the version #294 names) 400 In both of the last two the staged awf-config.json carried `apiProxy.defaultAiCreditsPricing: {input: 5, output: 25}`, and for the third the job log confirms api-proxy:0.27.27 was the image pulled. v0.27.42 is the newest firewall release, so there is no version left to try. So `models.default-ai-credits-pricing` does not suppress the credits guard for claude-opus-5 on any currently available firewall, contrary to #294's description. Holding the model at claude-opus-4-8, which is what the first successful autofix run used, so the trial can produce the cost and comment data it exists to produce. The frontmatter records the evidence and says to restore the model, its models: block and the sandbox pin together. This blocks #294 the same way: its whole roster would 400.
…stablished, not proven The previous note claimed the pricing fallback "does not work on any firewall release that currently exists". That overstates the evidence. What the three runs show is that it did not work in the configuration tried; the spec documents the mechanism, and config-mapper.ts maps the field in both v0.27.27 and v0.27.42, so version is not the variable and the pin has been dropped. Records the untried combination instead: those runs supplied the fallback AND a models.providers cost entry, and spec 10.7.1 applies the fallback only to a model that cannot be resolved, so the providers entry may be short-circuiting it. Also records that #294's own lock carries no claude-opus-5, so its verified claim is a reading of the spec rather than a run.
|
still experimenting with this. Having some trouble with the gh aw firewall. |
* [jwies/review-dispatch-gate] review: block review submission when the dispatched sub-agent outputs are missing (dispatch-conformance gate) * [jwies/review-dispatch-gate] review: harden the gate's fail-open ordering; sentinel-gated job failure; per-line disclosure match (review feedback) * [jwies/review-dispatch-gate] review: protect the sentinel/pre-gate writes so the rewrite always runs once blocked (re-review feedback) * [jwies/review-dispatch-gate] review: pin keep-list survivors, Step 6 template coupling, and the Conformant summary branch (third-round nits) * [jwies/review-pre-agent-staging] review: move the staging layer into a deterministic pre-agent step (orchestrator slice 1) * [jwies/review-pre-agent-staging] review: fix the added-lines hash for ++ content lines; paginate reviews; shared hunk splitter; feedback hardening * [jwies/review-pre-agent-staging] review: tighten ghGet retry semantics; warn on a missing scoped.diff; eval roster-shrink assertion (re-review feedback) * [jwies/review-stamp-carrier] review: re-review fingerprint falls back to cache memory (the body stamp never survives gh-aw ingest) * [jwies/review-dispatch-gate-local] review: shared lenient sub-agent JSON extraction; the gate reads out-files with the dispatcher's leniency (trial run 29893634730) * [jwies/review-pre-agent-staging-local] review: the staged plan artifact mirrors stampSource (carrier-fix follow-through) * [jwies/review-pre-agent-staging-local] review: retry the secondary-rate-limit 403 (Retry-After) in the staging fetch (re-review feedback) * [jwies/autofix-v1] autofix: opt-in, one-shot fixing of the PR reviewer's feedback Label a PR `autofix: blocking` or `autofix: nits` (they union) and the run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, posts a summary, and removes the label. One run per arming. The deterministic half (workflows/autofix/lib) decides everything before the agent edits anything: scope resolution, review-currency gating, the work list, and the commit trailer. The plan is final; the prompt may execute it or stop, never widen it. Currency is checked per file against the reviewer's own fingerprint stamp, so a PR whose author pushed one unrelated fix after the review still gets its other findings fixed rather than being refused wholesale. The push uses KHAN_ACTIONS_BOT_TOKEN, not GITHUB_TOKEN: GitHub creates no workflow runs for GITHUB_TOKEN-triggered events, and the re-review of the autofix commit is the only verification the fix gets. * [jwies/autofix-v1] autofix: add /autofix as a peer arming surface alongside the label Both surfaces resolve through one function over a shared token vocabulary, so a value cannot mean one thing as a label and another as a command. The axis model now describes tokens rather than labels. The trigger decides and the two never union: a stale `autofix: nits` label must not widen an explicit `/autofix blocking`. A command-armed run removes no labels, since a comment is self-clearing and any label present was not what armed it. The command gate is spelled out in the workflow's `if:` rather than using gh-aw's `slash_command` trigger, whose compiled gate misses a trailing CRLF and silently killed /review in Khan/webapp#40943. * [jwies/autofix-v1] autofix: fix the two faults the first live trial run exposed The bash allowlist used gh-aw's documented `"npx *"` form, which compiles to `Bash(npx)` and matches only a bare `npx` with no arguments, so the plan CLI was never executable and the run hand-simulated it instead. `"npx:*"` compiles to `Bash(npx:*)`, the form gh-aw's own defaults use. The prompt now treats a non-executable CLI as a hard stop. The currency guard collapsed "no reviews" with "reviews exist but carry no fingerprint", so a PR with real blocking feedback was told no feedback had been posted. They are now distinct, and the unstamped state degrades to the per-thread anchor check with a note in the summary rather than refusing. * [jwies/autofix-v1] autofix: work around gh-aw's unbounded PR-branch fetch on large monorepos The safe-outputs job gets a depth-1 shallow clone of refs/pull/N/merge, then push_to_pull_request_branch.cjs:935 fetches the PR branch with no --depth. The branch tip is absent and its history never reaches the shallow boundary, so git walks it all the way back. Against Khan/webapp that ran >14 min and 5.6 GB before being cancelled, which is what killed the first trial push. Depth cannot be bounded from outside: git has no fetch.depth config, no gh-aw option touches the safe-outputs checkout, and no step can be injected into that job. Injecting a partial-clone filter via GIT_CONFIG_* does reach the fetch and removes the bulk: same command, 91 s and ~557 MB, exit 0. Composes with gh-aw's own GIT_CONFIG_* use, which appends rather than overwrites (git_helpers.cjs:64-76). * [jwies/autofix-v1] autofix: move to Opus 5; settle the tagOpt question with a measurement Pin the autofixer at claude-opus-5, deliberately ahead of the reviewer's claude-opus-4-8: the reviewer's pin is tied to its eval calibration, autofix has no such tie, and writing the fix is the harder half of the pair. gh-aw v0.83.4's firewall (0.27.42) already knows Claude 5 pricing, so no sandbox.agent.version or models: override is needed. Do NOT add remote.origin.tagOpt=--no-tags. Measured back to back against Khan/webapp from identical checkouts: 85 s to completion without it, still transferring past 20 minutes with it. Also drop the em dash from the commit-message template, which is how one reached the bot's first real commit. * [jwies/autofix-v1] autofix: self-contained commit messages, exception-driven summary, fewer turns The commit subject must now name the change; `autofix: address reviewer feedback` was identical on every run and made git log useless. The body states the problem as a fact about the code, not a pointer to a thread a later reader cannot see. The summary comment is exception-driven. A clean run is already told by the thread reply, the commit, and the engine's own pushed-commit comment; a fourth notification saying the same thing trains people to ignore the bot. It still always posts a refusal, a no-op, anything left unfixed, or a degraded check. The first run spent 131 turns and $4.61 on a six-line fix while moving only ~93 KB of tool output, so the cost was turns, not payload: 7 turns to make one directory, 3 reading this workflow's own lib source, 5 hand-assembling staged JSON, 3 on --help. The prompt now closes each of those. * [jwies/autofix-v1] autofix: stage deterministically in a pre-agent step lib/stage.ts fetches everything the plan needs and writes it before the agent starts, so staging costs zero assistant turns and fails before any credits are spent. Follows the reviewer's orchestrator slice 1 (#280). Turns are what this workflow costs. The first live run: 131 turns, 460 AIC, ~93 KB of tool output total. 61% of the bill was cache reads at a 40.7:1 read-to-write ratio, so caching was already near-optimal and only the turn count could move. Staging was ~15 of those turns. Also records two findings from #287: gh-aw's ingest strips all HTML comments, so the reviewer's fingerprint never reaches a posted review (making autofix's degraded path the normal one, since cache memory is per-workflow and not reachable from here), and this workflow's own summary marker was being silently deleted too. * [jwies/autofix-v1] autofix: fix the currency guard failing open; one changeset per PR Review feedback on #298, both blocking findings and one suggestion. assessReviewCurrency returned `current` with no stale paths and no degraded note whenever computeHunkSignature produced an empty signature, which happens for an empty diff and for raw get_files patches (no diff --git/---/+++ headers for splitUnifiedDiff to recognise). The guard reported a clean full check having performed none: the one failure direction this module must not have. An unreadable diff now degrades like any other unusable input. buildWorkList never checked who opened a thread, so the reviewer-feedback-only restriction lived solely in prose. A human thread whose first line quotes `**issue (blocking):**` would have become a work item an agent edits code for. Ownership is now enforced where the decision is made. Also collapses six changesets into one, which is what a PR should carry. * [jwies/autofix-v1] autofix: reuse the stack's diff builder; close the review's remaining gaps Rebased onto the orchestrator's staging slice (#280), so the local buildUnifiedDiff copy is gone. The shared one is also more correct: mine emitted `diff --git a/<name> b/<name>` from `filename` alone, wrong for a rename and for an add or delete. A cross-package test now pins that what the reviewer's builder emits is what our currency check can still parse. Remaining #298 feedback: - The head SHA now comes from the job's checkout rather than the API. Comparing an API read against an earlier checkout left a window where a push landed between the two and both reads agreed while the tree was already stale. - Trailer parsing is scoped to the final paragraph. A revert or doc commit quoting `Autofix-Version: 1` parsed as an autofix commit and inflated the cycle count; harmless while it is only reported, load-bearing once a cadence axis caps on it. - Nothing checks the agent's own edits before the push, so the prompt now requires the run to say so rather than let an unverified fix read as a verified one. - `diffText` is documented as what it actually is (the full diff, not the stripped copy) and why that is harmless: stalePaths is consulted only for paths carrying a finding, and findings are not raised on generated files. * [jwies/autofix-v1] autofix: price Opus 5 for the proxy; match bot logins across REST and GraphQL Both faults from the second trial run (Khan/webapp#41140, run 30416237794). The agent died at turn 1 with `API Error: 400 Model "claude-opus-5" has no AI credits pricing`. claude-opus-5 is in no gh-aw-firewall release's pricing table and no sandbox.agent.version pin can fix that, so the model needs `models.default-ai-credits-pricing` (gh-aw >= v0.83.0), exactly as #294 does for the reviewer roster. The comment this replaces asserted the opposite, reasoning that the firewall was past the release which priced Fable 5; Fable 5 being priced says nothing about Opus 5. Staging reported threadCount: 0 on a PR carrying five reviewer threads. REST spells an App's login `github-actions[bot]` and GraphQL spells the same actor `github-actions`, and staging reads threads over GraphQL but reviews over REST, so one configured spelling cannot match both. Comparison is now suffix-insensitive in staging and in the worklist's ownership guard. The unit tests could not have caught this: every fixture was written in the REST spelling. * [jwies/autofix-v1] autofix: pin the firewall to the version that honours the pricing fallback Adding models.default-ai-credits-pricing was necessary but not sufficient: the proxy still 400'd claude-opus-5 for want of pricing. The staged awf-config.json from Khan/webapp#41140 run 30421726630 carried `apiProxy.defaultAiCreditsPricing: {input: 5, output: 25}` and the request was rejected anyway, so the field reaches the config and the compiler's default firewall (v0.27.42) ignores it. #294 verified awf v0.27.27 accepts and maps that field, which is why the reviewer pins it. Autofix inherited the default instead. Pinning to the version with evidence behind it. * [jwies/autofix-v1] autofix: hold at Opus 4.8; the Opus 5 pricing fallback does not work anywhere Three live runs on Khan/webapp#41140, each isolating one variable: 30416237794 no fallback configured 400 30421726630 fallback + firewall v0.27.42 (compiler default) 400 30422315631 fallback + firewall v0.27.27 (the version #294 names) 400 In both of the last two the staged awf-config.json carried `apiProxy.defaultAiCreditsPricing: {input: 5, output: 25}`, and for the third the job log confirms api-proxy:0.27.27 was the image pulled. v0.27.42 is the newest firewall release, so there is no version left to try. So `models.default-ai-credits-pricing` does not suppress the credits guard for claude-opus-5 on any currently available firewall, contrary to #294's description. Holding the model at claude-opus-4-8, which is what the first successful autofix run used, so the trial can produce the cost and comment data it exists to produce. The frontmatter records the evidence and says to restore the model, its models: block and the sandbox pin together. This blocks #294 the same way: its whole roster would 400. * [jwies/autofix-v1] autofix: correct the Opus 5 note; the cause is unestablished, not proven The previous note claimed the pricing fallback "does not work on any firewall release that currently exists". That overstates the evidence. What the three runs show is that it did not work in the configuration tried; the spec documents the mechanism, and config-mapper.ts maps the field in both v0.27.27 and v0.27.42, so version is not the variable and the pin has been dropped. Records the untried combination instead: those runs supplied the fallback AND a models.providers cost entry, and spec 10.7.1 applies the fallback only to a model that cannot be resolved, so the providers entry may be short-circuiting it. Also records that #294's own lock carries no claude-opus-5, so its verified claim is a reading of the spec rather than a run. * [jwies/review-pre-agent-staging] review: don't tell the orchestrator to read the whole staging directory Step 1 said "Read these files" above a list of a dozen staged inputs, most of which only the sub-agents consume. Taken literally that pulls the whole change's diff into the orchestrator's context, which is the waste the same sentence is trying to prevent. Say which two files are the orchestrator's and leave the rest to the steps that use them. * [jwies/autofix-v1] autofix: actually implement the guards the command path claimed Blocking finding on #298. The issue_comment branch of the workflow's `if:` carries neither the fork check nor the skip-ai-review check, because that event has no github.event.pull_request to read them from. Both this file's comment and the README said the checks "move into the plan"; the plan never implemented them, so a command-armed run reached the agent with neither. Staging now records whether the head is a fork, and plan.ts refuses a fork or a skip-ai-review PR before any work is planned. Enforced on every path rather than only the command one: a duplicated guard is cheap and a missing one authorises a code push. Unknown fork status refuses, so a staging gap cannot open the hole back up. Also records why nobody has been able to test /autofix: issue_comment is a repository-level event, so GitHub reads the workflow from the default branch and never from a PR head. Every run of the Khan/webapp#41140 trial was pull_request, and a reviewer's /autofix comment there did nothing. The command surface is only reachable once the workflow is on the consuming repo's default branch. Threads AUTOFIX_BOT_LOGIN through to the worklist's ownership guard, which was re-filtering against the hardcoded default while staging honoured the override. * [jwies/autofix-v1] autofix: lead the description with the command, not the label jeresig on #298: for a single run the /autofix convention reads better than a label. The description named only the label, which both undersold the command surface and made it look like an afterthought. They are peers; the description now says so and leads with the command. * [jwies/autofix-v1] autofix: `skip-ai-review` no longer disarms autofix The label stops the reviewer's next run; it does not withdraw a review already posted, so a labelled PR can still be carrying current findings. The reviewer even suggests the label from inside a review body it just posted, which makes "labelled, with live findings" a state the workflow steers people into rather than a corner case. Reading the label as "no AI may act on this PR" silently swallowed an explicit `autofix:` label from someone with write access, and that opt-in IS the authorisation while autofix only ever runs when a human arms it. Removed from the label path's `if:` (shared source, this repo's install, and the recompiled lock) and from `plan.ts`, where the refusal was justified as "with no review there is nothing to fix" while the guard that actually checks for a review sits ten lines below it and covers exactly that case. Tests pin the new behaviour on both surfaces: a labelled PR with a current review arms, and a labelled PR with no review still refuses on review currency. Revisit when autofix runs automatically rather than only when armed. That is when a push nobody asked for becomes possible, and autofix should get its own opt-out then rather than borrowing the reviewer's. Surfaced by the Khan/webapp#41177 docs trial: suppressing the repo's installed reviewer with the label also disarmed the autofix arm, so no trial could isolate one reviewer and exercise autofix at the same time. * autofix: fail closed when the thread fetch fails Blocking finding on #298. The GraphQL port threw only on `!res.ok`, but GraphQL does not signal failure by status: GitHub answers RATE_LIMITED, node-access failures, and partial field failures with HTTP 200 and an `errors` array. A throttled run therefore read as a successful one, `threadsConnectionOf` found no connection, `collectThreads` treated that as the end of the pages, and staging wrote an empty threads.json for a PR with open reviewer threads. Everything downstream then behaves correctly on false input, which is what makes it worth fixing rather than logging. Reviews come over REST on a separate quota, so the currency check still passes; the plan sees no actionable threads and emits a no-op; the no-op populates labelsToRemove. The label comes off and the author is told there is nothing to fix while the findings it was armed for sit open, with nothing left to re-arm from. That is the fail-open direction this module's contract rules out and the REST paths' own `throw` already rules out for them. Any `errors` entry is fatal, including alongside partial data: fixing whichever threads happened to arrive and then clearing the label is worse than refusing, because it reports success over the ones that did not. A body with no reviewThreads connection is likewise fatal rather than "no threads". Throwing fails the staging step before any AI spend and leaves the label in place. Guarded at both the transport and the reader. Duplicated on purpose, the same reasoning as the fork guard in plan.ts: the check is cheap and its absence clears an arming label on a PR with live findings. The reader is also the layer the unit tests can reach, since the real port is built inside the `require.main` block. The test harness defaulted to `{}` for unsupplied pages, which the old code read as "no threads"; it now defaults to a well-formed empty page, so the fail-closed tests are the only ones exercising the error paths. Verified the three new guards are load-bearing by reverting them: 3 fail, 27 pass. * [jwies/autofix-v1] autofix: the re-review is best-effort verification, and the summary says so The docs claimed the reviewer's re-review is the verification an autofix commit gets, in the indicative, and Step 7 made the summary comment promise it: "The reviewer will re-review this push." On Khan/webapp#41194 that promise was false. Autofix pushed ad8da8d4, the shim posted /review, and the re-review died in gh-aw's "Install AWF binary" step before the model ran. The commit sat unverified for 36 minutes until a human noticed and re-triggered. Nothing on the PR said so. That repo's reviewer is an issue_comment local override, so the run's head SHA is a default-branch merge commit and it never joins the PR's check suite; status-comment is false, so it posts nothing; and gh-aw's own fallback tried to file a failure issue and got "410 Issues has been disabled in this repository". The only trace was the eyes reaction the activation job had already added, which is indistinguishable from "still running". Of that repo's last 100 reviewer runs, 15 of the 53 that started ended in failure, so this is a shape to expect rather than a one-off. The gap is not really autofix's: where the reviewer is push-triggered, as the shared workflow ships it, a failed re-review is already a red X on the commit autofix pushed. What autofix owed the human re-arming loop was the truth, so the fix is prose in the four places that overstated it, plus one line of output. Step 7 now posts exactly one comment on every path, and states that nothing has checked the commit. The quiet-clean-run branch is retracted deliberately, and the section says so and says why: its rationale was that a clean run is already told in three other places, and all three record that something changed while none records that nothing has checked it. It was unreachable anyway, because it required an empty degradedNote and the reviewer's fingerprint stamp is stripped from every posted review. Both mandated lines have to survive the re-review landing, so neither is a conditional instruction: "if no review appears, comment /review" would leave every verified PR permanently carrying an instruction to go and trigger a review. The same objection is why verification state stays out of the commit trailer, where nothing in v1 could ever flip it. No new trigger, no detector, no trailer field. A detector would need a home that runs later than a one-shot run, and the README records the query it would ask so it need not be re-derived.
…, with the provider stop reason Run 30650071285's instrumentation showed the real signature behind three runs of chasing a contract bug: the failing correctness-reviewer returns a length-0 final, on BOTH harnesses, on the security-adjacent corpus cases. "malformed output: output carries no parseable JSON object" is actively misleading for that case, since nothing malformed was emitted; nothing was emitted at all. Both the eval producer and the production contract parser now name it: `empty output: the agent returned no final text`, distinct from the malformed-output message that still covers unparseable prose. Both arms also carry the provider stop reason for the last assistant message (`AgentResult.stopReason`) — the SDK arm from `message.stop_reason`, falling back to the result subtype; the Pi arm from the assistant message's `stopReason`. That is the field that separates the two candidate explanations for an empty final: a refusal, which #294 documents as surfacing "as a missing agent result, not an error", versus a result the harness dropped. The eval appends it to the failure note, so the next run says which. Not asserted here: that these ARE refusals. Length 0 is what was measured; the stop reason is what will say.
The refusal risk this PR accepts is now measured, and one premise here is wrongContext: #311. While benchmarking a different change I instrumented the eval's failure reporting, and it surfaced something directly relevant to this PR.
|
8ca29fd to
138e7a1
Compare
Rebased onto #311, and the pricing question resolved by checkingOpus 5 is still not priced. I checked That is the whole Anthropic section. No I dropped that block in the first pass of the rebase (I re-applied only the pin changes onto main's frontmatter). Restored in acf83a6 with the verification written into the comment. The branch would have failed every run without it, so this was worth the check rather than the guess. One thing that did improve: the State of the rebaseBase is now Resolution principle: keep main's infrastructure evolution, re-apply this PR's pin intent on top, rather than resurrecting frontmatter main has superseded. 22 pins now on Two reconciliations I made in the README, both judgment calls worth a look:
Still owed: the risk sectionThe "Known risk this accepts" section still describes the pre-#311 world — refusals as a lens-only hazard, drift as the detector, and no fallback available. All three have changed, and the third was never quite right (gh-aw exposes no fallbacks parameter, but scripted dispatch owns its runner). That section is an argument in your voice, so I have left it for you rather than rewriting it; the material for the edit is in my earlier comment. |
Firewall pricing: v0.27.43 has Opus 5, but keep the fallback for nowFollowing up on the pricing check. v0.27.43 (newest firewall release, and there is nothing after it) does carry a curated That entry is strictly better than the Two reasons not to reach for it yet:
So the fallback stays. a08f433 records the removal condition in the frontmatter rather than leaving it to be re-derived: drop the The
|
aea99a9 to
bccb375
Compare
|
Rebased onto #314 (AI-credits pricing overlay) now that #315 is on The The two Verified at this tip: frontmatter parses with one |
bccb375 to
67663d6
Compare
|
Removed One thing to be deliberate about before merging: dropping the fallback means this PR now depends on #314's So: #314 can merge whenever (it is inert but harmless under v0.27.42); this PR should wait for gh-aw v0.84.x to go stable. If you would rather not carry that constraint, restoring the fallback is a one-line revert. |
2766e4b to
90e373a
Compare
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
…ck my rebase dropped Re-applying only the pin changes onto main's frontmatter lost the load-bearing half of this PR: the models.default-ai-credits-pricing block. Without it every dispatch 400s before reaching the model, so the branch as I rebased it would have failed every run. Verified rather than assumed: claude-opus-5 is absent from the firewall api-proxy's curated pricing table at v0.27.42, the release gh-aw v0.83.4 defaults to now that this workflow's sandbox.agent.version pin is retired. The table carries claude-opus-4-5 through 4-8 and claude-fable-5 and stops there. The blast radius is worse than when this PR was written: main retired the sandbox pin on the grounds that v0.27.42 prices claude-fable-5, which is true and irrelevant once the roster is on Opus 5. And with all 22 pins on the un-priced model, a missing fallback 400s the orchestrator and every default agent rather than the two opt-in dispatches that #266 cost. The gh-aw >= v0.83.0 floor this field needs is already met (v0.83.4), so the compile-time constraint on consumers stands as documented but this repo clears it.
…s Opus 5, and when to drop the fallback v0.27.43 (newest firewall release, 2026-07-31) adds a curated claude-opus-5 entry at the same rates this fallback carries. The curated entry is strictly better: it bills cache writes, the one component the default-pricing path under-counts. No gh-aw release references v0.27.43 yet, so the fallback stays for now. The comment records the removal condition rather than leaving a future reader to re-derive it, and explicitly rules out pinning sandbox.agent.version forward to reach it early — review.md's own rule is that a version is pinned here only to hold a release BACK.
… models block Two top-level `models:` keys is a duplicate YAML key that resolves silently to whichever comes last, so one of the fallback or the overlay would be dropped with no error. gh aw compile does not catch it: it compiles only .github/workflows, and the duplicate is in the shared source. Insert default-ai-credits-pricing into the overlay's existing block rather than relocating the block, so this PR's diff does not claim the overlay's lines. Stated at 50% of list (2.5/12.5 $/1M) for the same reason the overlay is, so a credit means real spend whichever path prices a request.
… note The pricing overlay this PR stacks on already prices claude-opus-5 under `models.providers`, so `default-ai-credits-pricing` is a second, coarser statement of the same rate: it is $/1M rather than $/token, it does not bill cache writes, and it applies to ANY un-priced model, so a typo'd model id bills at Opus rates instead of failing loudly. Also drop the eval-drift note explaining why the 220 budget was held rather than re-derived for the Opus 5 move; the surrounding derivation already documents the number.
…tignore The changeset still described a default-ai-credits-pricing fallback and a sandbox.agent.version pin that this branch no longer carries, and ran to 1051 words. Rewrite at 308, accurate to the current diff: claude-opus-5 is priced by the models.providers overlay this stacks on, so the gh-aw v0.84.x dependency is a merge gate rather than a consumer compiler floor. Also remove .github/aw/logs/.gitignore, which a gh aw command created and a git add -A swept into an earlier commit here. It is unrelated to the roster move.
6f04b80 to
d3cafce
Compare
…o the root manifest CI was red on this branch before the restack, for two reasons that both live in package.json. The SDK range moved to ^0.3.219 without regenerating pnpm-lock.yaml, so `pnpm install --frozen-lockfile` failed with ERR_PNPM_OUTDATED_LOCKFILE before any job ran a test. The changeset already explains why the range bump alone is insufficient; the lockfile just never followed. zod was also dropped from the root manifest. main carries it, and workflows/review/lib/dispatch-runner.ts imports it, so removing it breaks resolution for root-level runs. Nothing in this branch explains the removal, so restore it.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Changes requested — see inline comments.
1 of 1 prior review thread is still unaddressed as of c451b34:
1 non-blocking thread still open
- question (non-blocking)
.changeset/review-correctness-opus-5.md:7: The changeset documents the refusal hazard and its mitigation — detect the signature on the weekly drift corpus, then "p...
| name: first-principles | ||
| description: A diverse-perspective, advisory-only sanity check on whether the change should exist as written; returns findings as JSON. | ||
| model: claude-fable-5 | ||
| model: claude-opus-5 |
There was a problem hiding this comment.
suggestion (non-blocking): The roster becomes a single-model-family monoculture, and the one deliberately-diverse reviewer moves with it. The description concedes the point itself: first-principles "ran Fable partly to be the one non-Opus reviewer," and the remedy offered — watch whether its findings converge with holistic's over the drift series, then move one back — is a slow, inferential detector for a property that was originally bought upfront by design. The stated rationale for this pin (recall at lower cost) barely applies to an advisory-only agent that never blocks, so the cheapest hedge is also the least costly one to keep.
A sketch, not a committable replacement:
Leave `first-principles` (advisory-only, never blocks, low volume) on `claude-fable-5` — or any non-Opus pin — so one reviewer and the refusal-fallback path remain outside the Opus family, instead of waiting for drift-series convergence to re-derive the diversity this PR removes.
…iscount-overlay' into 294-work
…pricing mechanically Review feedback on #294: - The merge from the pricing-overlay base (#314) dropped the models.default-ai-credits-pricing block for the second time in this branch's history; without it every dispatch 400s on the stable toolchain (claude-opus-5 is not in firewall v0.27.42's curated table). Restored, with the prose reconciled to the merged 50% providers overlay: fallback rates stay at list because in the only window it binds, every other model bills at list too. - New model-pricing.test.ts turns both prose warnings into failing checks: every model pin must have a providers overlay entry, and the fallback must exist while claude-opus-5 is pinned (delete that test with the fallback block once gh-aw defaults to v0.27.43+). - Stale rationale comments updated: correctness-reviewer's Fable 5 justification now states what Opus 5 carries; first-principles records the move; the overlay's engine markers moved to opus-5, with opus-4-8 relabeled as the refusal-fallback target. - Changeset renamed to review-roster-opus-5 (roster-wide scope) and records why security-auth moves with the roster instead of a pre-emptive carve-out. Also merges the base branch (sonnet-4-6 overlay entry).
|
Addressed the review feedback in 470b456 (push pending; the branch name is outside the sandbox push allowlist):
|
Review Guidancegithub-actions (3 files)
Common patterns2 files: Sub-agent model pin changed from -model: claude-opus-4-8
+model: claude-opus-5Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or
|
There was a problem hiding this comment.
4 of 5 prior review threads resolved; 1 still unaddressed as of 470b456:
1 non-blocking thread still open
- suggestion (non-blocking)
workflows/review/review.md:2040: The roster becomes a single-model-family monoculture, and the one deliberately-diverse reviewer moves with it. The descr...
| | `test-adequacy` | `claude-opus-5` | high | Opt-in whole-change reviewer (`enable` in `ROUTING`) | | ||
| | `conventions` | `claude-opus-5` | medium | Opt-in advisory targeted check (`enable` in `ROUTING`) | | ||
| | `documentation` | `claude-opus-5` | medium | Opt-in advisory targeted check (`enable` in `ROUTING`) | | ||
| | `first-principles` | `claude-opus-5` | high | Opt-in advisory-only; reviews the change's justification | |
There was a problem hiding this comment.
question (non-blocking): This PR rewrites the models/effort table (README lines 557–569) to put the entire roster on claude-opus-5, but leaves the immediately-following rationale paragraph (lines 576–588, untouched by the diff) asserting 'Exactly two roles run Fable 5: first-principles ... and correctness-reviewer' and 'Everything else stays on Opus 4.8 deliberately', which the updated table now directly contradicts.
| # MINIMUM COMPILER: gh-aw >= v0.83.0 for `models.default-ai-credits-pricing`. | ||
| # $/1M tokens. `input` and `output` are the only rates the schema accepts, | ||
| # so the cache rates are the proxy's derivations, not ours. | ||
| default-ai-credits-pricing: |
There was a problem hiding this comment.
note (non-blocking): PR description says this PR carries no pricing fallback, but this hunk adds default-ai-credits-pricing — the merge-gate rationale in the description is stale. Checked the added block (lines 346–379) against the released review.md (tag review-v1.11.0 has no models: block at all): the fallback is introduced by this change, and its own comment says its purpose is to price claude-opus-5 on the stable toolchain — precisely the every-dispatch-400s failure the description says only #314's overlay prevents. Not a code defect; the code and its comments are internally consistent — but the description's 'Hard merge gate' section should be updated to reflect that the fallback now bounds that failure mode, and to state which caveats keep the gate alive if it still applies.
Also flagged by completeness, first-principles.
A sketch, not a committable replacement:
Update the PR description to state the fallback is now self-contained and restate what, if anything, still gates merge on #314 (the overlay is now just the 50%-rate refinement, not the thing that makes dispatch work).
| */ | ||
| const pins = [ | ||
| ...new Set( | ||
| [...reviewMd.matchAll(/^\s*model:\s*(claude-[a-z0-9.-]+)\s*$/gm)].map( |
There was a problem hiding this comment.
nitpick (non-blocking): The pin-extraction regex requires the model id to end the line, so a pin with a trailing inline comment silently escapes the coverage gate. Checked all 23 current pin lines in review.md — none carry inline comments today, so this is a latent gap introduced with the new test, not a live failure. Tolerating an optional trailing comment (or asserting the pin count against the number of agent blocks) would keep the gate from resting on that formatting convention.
A sketch, not a committable replacement:
/^\s*model:\s*(claude-[a-z0-9.-]+)\s*(?:#.*)?$/gm
| name: correctness-reviewer | ||
| description: Classifies each changed file's risk and reviews the diff for correctness defects; returns JSON. | ||
| model: claude-fable-5 | ||
| model: claude-opus-5 |
There was a problem hiding this comment.
thought (non-blocking): The 22-pin swap ships on vendor-reported quality ("Opus 5 reports high precision and high recall") where the 2-pin Fable move required a powered A/B first — measure-then-swap was the precedent, this is swap-then-measure. The description says "the powered A/B is the acceptance criterion, not this description", yet nothing in the diff or the stated merge gates makes that A/B a merge precondition — the only hard gate is toolchain pricing. The 2026-07-20 Fable move ran its A/B before moving correctness-reviewer; this change is 11x larger in pins and inverts the evidence order, using live reviews plus revert tripwires as the experiment.
A sketch, not a committable replacement:
Run the powered A/B (at minimum the four listed tripwire rows) on this branch before merge, or state explicitly in the description why swap-first is acceptable this time.
There was a problem hiding this comment.
Approved — no blocking issues found.
5 of 5 prior review threads are still unaddressed as of 470b456:
5 non-blocking threads still open
- question (non-blocking)
workflows/review/README.md:567: This PR rewrites the models/effort table (README lines 557–569) to put the entire roster onclaude-opus-5, but leaves... - nitpick (non-blocking)
workflows/review/lib/model-pricing.test.ts:38: The pin-extraction regex requires the model id to end the line, so a pin with a trailing inline comment silently escapes... - thought (non-blocking)
workflows/review/review.md:1226: The 22-pin swap ships on vendor-reported quality ("Opus 5 reports high precision and high recall") where the 2-pin Fable... - suggestion (non-blocking)
workflows/review/review.md:2040: The roster becomes a single-model-family monoculture, and the one deliberately-diverse reviewer moves with it. The descr... - note (non-blocking)
workflows/review/review.md:377: PR description says this PR carries no pricing fallback, but this hunk addsdefault-ai-credits-pricing— the merge-gat...
|
Powered A/B complete (workflow_dispatch run 31738851830: full live corpus, repeats=2 per arm, baseline = this PR's base branch, so the delta isolates the roster swap from the pricing overlay). The stated acceptance criterion passes:
Per the four revert rows:
Adversarial hard gate PASSED on the candidate arm in every repeat. One row worth an eye at merge review: golden-documentation-stale-and-narrated (verdict) read 1/2 baseline vs 0/2 candidate, while both its specs were caught (the candidate labeled doc-stale-expiry-days blocking 2/2 vs baseline's severity split); a verdict disagreement with full spec recall on an advisory-documentation case is label calibration, not lost detection. |
2 participants
What
Moves the entire reviewer roster (22 pins) to Opus 5:
thread-reconciler,skill-auditor,claim-validator,conventions,documentation,holistic,completeness,test-adequacy, 11 specialist lensesclaude-opus-4-8claude-opus-5correctness-reviewer,first-principlesclaude-fable-5claude-opus-5pattern-triageclaude-sonnet-4-6The eval's judge and match arbiter stay on Haiku 4.5: they are the ruler, not the reviewer.
The basis: Opus 5 reports high precision and high recall at Opus 4.8's per-token price, half Fable's. A straight upgrade for the 4.8 roles; for the two Fable roles it should keep the recall the 2026-07-20 A/B bought (must-catch 82% -> 89%, true misses down 41%) without the +35% premium. The powered A/B is the acceptance criterion, not this description.
Four rows that each revert independently
A roster-wide swap measured in aggregate hides per-role regressions, so each of these reverts on its own evidence:
correctness-reviewer: must-catch recall on the six corpus rows Fable moved in review: move deep-reasoning reviewer roles to Fable 5 #266 (incident-sql-missing-index:dm-default-backfill, the threetrial-retention-deletion:erasure-*specs,golden-retention-fix-push:quota-exceeded-never-true,golden-retention-lifecycle-2:retention-prune-unhandled-rejection).claim-validator: the precision gate, where Fable measurably failed (noise 43% -> 49%, one wrong blocking flag on a clean case). Read noise/unmatched-posted and any wrong blocking flag on the expected-clean cases.security-authlens: refusal exposure (below).Refusals: a known, mitigated hazard that moves with the roster
Fable refused two security-adjacent eval cases outright on shipped v1.9.0 (run 30656579898:
stop_reason: refusal, "violative cyber content", 5,207 tokens so not a context limit; reproduced on two harnesses; intermittent, run 30658862532 cleared the same cases). A refused agent emits nothing, and the review used to proceed a dimension short, silently.Opus 5 does not resolve this (it ships its own refusal behavior); the merged fallback (#315) mitigates it: a refused agent re-dispatches once on
claude-opus-4-8, recorded asfellBackTo, never silent, and with the roster on Opus 5 that fallback target leaves the roster entirely. The drift detector changes character: recall cratering on security cases is now masked by the fallback, and the weekly counters report the per-agent/per-model fallback rate instead: a measurement rather than an inference.first-principlesloses its deliberate cross-family perspective diversity; if its findings converge withholistic's in the drift series, put one reviewer back on a different family.Hard merge gate: gh-aw v0.84.x stable (via #314)
claude-opus-5is absent from the firewall's curated pricing table at v0.27.42, and the proxy 400s an un-priced model before dispatch: every dispatch would fail. This PR carries no pricing fallback of its own; #314's overlay is what prices it, and that overlay is silently dropped below AWF v0.27.43. #314 can merge any time (inert but harmless); this PR must not merge until the overlay is verifiably live in the compiled lock.Eval drift budget held at 220
Per-token parity is not per-run parity: Opus 5 thinks by default and writes longer, now for every agent in a run. Under-sizing costs a red run (
caseAsymmetry) plus a contaminated noise-floor band; re-size only from a measured rate. Like all eval dollar figures the budget is a list-price ceiling, so it fails safe under the 50% discount.Also folded in
Two pre-existing CI breaks on the base: a stale pnpm lockfile (SDK range bump without regeneration) and
zoddropped from the root manifest whilemainstill imports it. Both fixed; CI green, full suite passes (69 files, 1618 tests). The installed reviewer (.github/workflows/review.md+ lock, pinned to a release) is untouched.