review: move to gh-aw v0.83.4; retire the firewall pin and restore sub-agent retries - #296
Conversation
🦋 Changeset detectedLatest commit: 1a00c9f 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 |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
fd85215 to
fe5a75d
Compare
There was a problem hiding this comment.
Approved — no blocking issues found.
2 of 2 prior review threads are still unaddressed as of fe5a75d:
2 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-runner.ts:77: SUBAGENT_MAX_RETRIES = "2" hardcodes a copy of the SDK's own default. If the SDK's default retry count ever changes, thi... - suggestion (non-blocking)
workflows/review/review.md:197: Document the gh-aw >= v0.83.x floor this pin removal now requires. Removing sandbox.agent.version means a consumer that...
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 |
There was a problem hiding this comment.
Approved — no blocking issues found.
2 of 2 prior review threads are still unaddressed as of a56d5ee:
2 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-runner.ts:77:SUBAGENT_MAX_RETRIES = "2"hardcodes a copy of the SDK's own default (the comment calls it "the SDK default"). If the... - suggestion (non-blocking)
workflows/review/review.md:197: Document the gh-aw >= v0.83.x floor this pin removal now requires. Removingsandbox.agent.versionmeans a consumer tha...
There was a problem hiding this comment.
1 of 2 prior review threads resolved; 1 still unaddressed as of 35b95cf:
1 non-blocking thread still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-runner.ts:77:SUBAGENT_MAX_RETRIES = "2"hardcodes a copy of the SDK's own default (the comment calls it "the SDK default"). If the...
Note: correctness not assessed this run (correctness-reviewer output unavailable).
| # which prices claude-fable-5 and pins each container by digest, so both are | ||
| # retired: keeping the pin would freeze the firewall at the old floor (and give | ||
| # up those digests) while gh-aw moves on. Re-pin a version here only to hold a | ||
| # firewall release BACK, never to move one forward. Before pinning any sub-agent |
There was a problem hiding this comment.
suggestion (non-blocking): This "hold a release BACK, never move one forward" guidance omits the version floor that makes a hold-back pin dangerous. The changeset documents that pinning sandbox.agent.version below v0.27.42 on gh-aw >= v0.83 reproduces the all-403 awmg-mcpg gateway breakage this PR fixes, but that warning lives only in the changeset while this comment is the enduring in-repo doc. Consider stating the floor here too, e.g. "never below the topology floor (>= v0.27.42 on gh-aw >= v0.83 — older firewalls cannot reach the awmg-mcpg bridge gateway)."
35b95cf to
af566c3
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approved — no blocking issues found.
2 of 2 prior review threads are still unaddressed as of af566c3:
2 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-runner.ts:77:SUBAGENT_MAX_RETRIES = "2"hardcodes a copy of the SDK's own default (the comment calls it "the SDK default"). If the... - suggestion (non-blocking)
workflows/review/review.md:200: This "hold a release BACK, never move one forward" guidance omits the version floor that makes a hold-back pin dangerous...
af566c3 to
cdafd50
Compare
cdafd50 to
04faf38
Compare
There was a problem hiding this comment.
Approved — no blocking issues found.
2 of 2 prior review threads are still unaddressed as of 04faf38:
2 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-runner.ts:77:SUBAGENT_MAX_RETRIES = "2"hardcodes a copy of the SDK's own default (the comment calls it "the SDK default"). If the... - suggestion (non-blocking)
workflows/review/review.md:200: This "hold a release BACK, never move one forward" guidance omits the version floor that makes a hold-back pin dangerous...
04faf38 to
1b176dd
Compare
1b176dd to
b322abd
Compare
b322abd to
ed3becc
Compare
|
Risk & pattern guidance — advisory; points at where a human skim pays off, not a blocking review. Reviewed 8 changed files (verdict: approve). Higher-risk files worth a closer look: Owning team:
Common pattern
Excluded from review
|
There was a problem hiding this comment.
Approved — no blocking issues found.
1 of 2 prior review threads resolved; 1 still unaddressed as of ed3becc:
1 non-blocking thread still open
- suggestion (non-blocking)
workflows/review/review.md:200: This "hold a release BACK, never move one forward" guidance omits the version floor that makes a hold-back pin dangerous...
…all pin and restore sub-agent retries Recompiles this repo's installed reviewer with gh-aw v0.83.4 (from v0.81.6) and drops the two frontmatter workarounds the new default firewall makes obsolete. The toolchain bump carries two execution-model changes: the awf sandbox now runs rootless (no sudo) and the MCP gateway moves to a bridge network the sandbox attaches to. The compiled invariants the reviewer depends on were re-verified under v0.83.4 on a scratch consumer layout: the dispatch-conformance gate still lands between "Ingest agent output" and "Upload agent artifacts", the engine.env Bash-timeout override still replaces the generated 60s defaults, inline sub-agents still extract to .claude/agents, the safe-output handler set and upload_artifact allowed-paths are unchanged, and no step was dropped. The firewall version pin (v0.27.27) and the claude-fable-5 `models:` pricing override both existed only to get an un-priced model past the api-proxy's 400; v0.83.4 defaults to firewall v0.27.42, which prices it and pins each container by digest, so the pin now only holds the firewall at the old floor. v0.83 also sets ANTHROPIC_MAX_RETRIES=0 on the engine step so terminal errors reach the harness that owns retry/backoff for the orchestrator process. The dispatcher's sub-agents run inside that process without such a wrapper, so dispatch-runner.ts now restores the SDK default for the sub-agent subprocesses alone, spreading process.env because that SDK option replaces rather than merges. Also opts out of gh-aw's newly generated agentics-maintenance.yml rather than adopting a daily scheduled cleanup workflow as a side effect of a version bump.
…copy too; it mismatches the v0.83.4 lock The v0.83.4 lock reaches the MCP gateway over a bridge network (MCP_GATEWAY_DOMAIN: awmg-mcpg, network.isolation, network.topologyAttach). Firewall v0.27.27 implements none of those keys: they are absent from its awf-resolved-config.json, and its squid allowlist carries .host.docker.internal with no route to awmg-mcpg. Observed across two runs of this PR. With the pin (run 30290472047): 3 TCP_DENIED 403s on POST awmg-mcpg:8080/mcp/github and /mcp/safeoutputs, i.e. a gateway path the lock expects to work being rejected; MCP itself still functioned over the path that bypasses squid (10 tools/call), and the run failed on the pre-existing 20-minute step timeout that three earlier v0.81.6 runs on other branches also hit (#285 raises it). Without the pin (run 30292838824, firewall v0.27.42): zero firewall denials, 16 tools/call including the safe-output posts, execute step 18m31s, review posted. The shared source dropped the pin in the previous commit; this drops it from this repo's installed copy as a marked LOCAL OVERRIDE, since the install stays on review-v1.7.0 until its next bump and would otherwise keep running a config its own firewall silently discards. The gh-aw default (v0.27.42) implements the topology keys, pins each container by digest, and prices claude-fable-5, which is what makes the models: block redundant.
ed3becc to
1a00c9f
Compare
Retargets the PR from the jwies/review-pre-agent-staging staging branch to main, which now carries that branch's work squash-merged (#280, #282, #283, #284, #288, #296) plus the later slices this branch never received. The head branch carried pre-squash copies of the review stack, so every conflict was a stale duplicate rather than a real divergence. All seven resolve to main: - .github/aw/actions-lock.json: main's superset. The autofix compile at v0.83.4 had pruned the setup-cli entry; main is at v0.83.4 too now, so the version skew the PR description flagged between autofix.lock.yml (0.83.4) and review.lock.yml (0.81.6) is gone. - lib/dispatch-gate.ts, lib/dispatch-gate.test.ts, lib/stage-pr.ts, lib/stage-pr.test.ts: add/add against main's merged versions. - review.md, README.md: main supersedes the head copies (timeout 40, the scripted-dispatch SDK install, code-owned disciplines extraction, the retired firewall pin, the lens payload seam). No autofix commit ever touched workflows/review/, so nothing is lost taking main wholesale there: the merged review stack is byte-identical to main and the whole merge reduces to main plus the autofix package.
2 participants
Moves the reviewer onto gh-aw v0.83.4 (from v0.81.6) and retires the two frontmatter workarounds the new default firewall makes obsolete. Stacked on #288; #289 is restacked on top of this.
Why this is its own PR
#289 graduates scripted dispatch to the only mode, and the next step after it (replacing the LLM orchestrator with a behavior-defined engine, which only exists in gh-aw >= 0.82) needs this bump. Landing the bump separately keeps the bisect honest: if the reviewer misbehaves after this, the cause is the toolchain, not the pipeline change on top of it.
The bump is not cosmetic. Two execution-model changes ride along:
sudo -E awf …becomesawf …, and the binary installs with--rootless. The--proxy-logs-dir/--audit-dirflags move into the awf config'sloggingblock, and--enable-host-access --allow-host-ports 80,443,8080is gone.MCP_GATEWAY_DOMAINbecomesawmg-mcpg, and the sandbox config gains"isolation": truewith"topologyAttach": ["awmg-mcpg"].Neither can be verified by compiling. The live run of this workflow on this PR is the acceptance test (
pull_requestruns the workflow from the PR head, so this PR reviews itself with the new lock).What changed
Toolchain (recompiled
review.lock.yml+actions-lock.json). Setup action v0.81.6 → v0.83.4, MCP gateway v0.3.30 → v0.4.6, GitHub MCP server v1.4.0 → v1.7.0, Claude Code CLI 2.1.191 → 2.1.220,actions/cachev5.0.5 → v6.1.0,actions/checkoutv7.0.0 → v7.0.1,actions/setup-nodev6.4.0 → v7.0.0. New steps: an activation-job OAuth-token check, automatic GitHub-MCP lockdown-mode determination (sink-visibility), and a safe-outputs manifest download in the conclusion job. The agent job now also surfaceshttp_400_response_errorandinvocation_cap_exceededoutputs, which is exactly the class of failure the fable pricing incident produced.The firewall pin and the
models:pricing override are removed from the shared frontmatter and from this repo's installed copy (the latter as a markedKHAN/ACTIONS LOCAL OVERRIDE, since the install stays onreview-v1.7.0until its next bump). This turned out to be required, not a cleanup — see the live finding below. On its own terms the removal is also right: both existed only because claude-fable-5 was absent from the AI-credits pricing table of the firewall gh-aw <= 0.81.x defaulted to (v0.27.11), and the api-proxy rejects an un-priced model with a 400; the pin held v0.27.27, the release that added Claude 5 pricing. v0.83.4 defaults to v0.27.42, which prices claude-fable-5 (verified incontainers/api-proxy/ai-credits-pricing.jsat that tag) and pins every firewall container by digest.sandbox.agent.id: awfstays declared, since the api-proxy is what meters AI credits and caps a runaway fan-out.Live finding: the pin and a v0.83.4 lock disagree about how to reach the MCP gateway. v0.83.4 compiles the agent to reach it over a bridge network (
MCP_GATEWAY_DOMAIN: awmg-mcpg,network.isolation,network.topologyAttach). Firewall v0.27.27 implements none of those keys — they are absent from itsawf-resolved-config.json— and its squid allowlist consequently carries.host.docker.internalwith no route toawmg-mcpg. Both runs of this PR measure the difference:TCP_DENIEDonPOST awmg-mcpg:8080/mcp/githuband/mcp/safeoutputstools/callcreate_pull_request_review_comment,submit_pull_request_review,upload_artifactSo the pin does not disable MCP outright — traffic over the path that bypasses squid still worked — but it does have the sandbox rejecting a gateway path its own lock expects to use, on a firewall that silently discarded the topology config it was handed. That pairing should not ship, hence the removal in both the shared source and this repo's installed copy.
The timeout is pre-existing, not a bump regression. Three earlier runs on v0.81.6 (
30279356940,30026567946,29952468664, three different branches) failed at the same 20-minute ceiling, and #285 is the open fix that raises it to 40. Even the clean run above finished with only 90 seconds of margin, so that fix is still the load-bearing one for large diffs.Sub-agent retries are restored in code (
dispatch-runner.ts). v0.83 setsANTHROPIC_MAX_RETRIES=0on the engine step, deliberately: it wants a terminal error (403ai_credits_limit_exceeded) to reach the harness that owns retry/backoff for 429/529. That harness wraps the orchestrator process only. The dispatcher's sub-agents are spawned inside it with no such wrapper, so they would have inherited 0 and turned any transient overload into a shed lens on every scripted run. The runner now passes the SDK'senvoption withANTHROPIC_MAX_RETRIESback at the SDK default for the sub-agent subprocesses alone, spreadingprocess.envfirst because that option replaces the subprocess environment rather than merging it. Two tests cover both halves.agentics-maintenance.ymlis declined via.github/workflows/aw.json({"maintenance": false}). v0.83.4 generates a 633-line daily-cron workflow that closes "expired" issues/PRs/discussions and prunes cache-memory entries withactions: write. The cache pruning looks benign (it keeps the newest entry per group), but adopting a scheduled agentic workflow as a side effect of a version bump is not this PR's business. Adopt it deliberately if wanted.Verification
gh aw compileon this repo's installed reviewer: 0 errors, 0 warnings.gh aw compileon the shared source (workflows/review/review.md, which is what consumers get) in a scratch consumer layout: 0 errors. The compiled invariants the pipeline depends on were re-checked in that output:engine.envBash-timeout override still replaces the generated 60s defaults on the engine step (BASH_MAX_TIMEOUT_MS: 1200000, which the blocking dispatcher call needs);.claude/agents, the directorydispatch.tsreads;upload_artifactallowed-paths(out/**) are byte-identical;AWF_NETWORK_ISOLATION=1in the agent env, sub-agents dispatched and returning tool results); the api-proxy meters every model, includingclaude-fable-5, with no 400 (run 30292838824: 1219 credits / $12.19 across opus-4-8, sonnet-4-6 and fable-5), which confirms token steering and the fable-pricing premise behind the pin removal; MCP works end to end through the new bridge-network gateway (16tools/call, zero firewall denials); and the reviewer posted a real review, so the safe-output path survives the new networking too.pnpm run lintcannot run in a worktree nested under the main checkout (eslint loads both.eslintrc.jsfiles and rejects the duplicate prettier plugin, and it ignores paths containing a dot-directory); the changed files were checked with prettier directly and CI lints from a normal checkout.Security review of the compile diff
Required by v0.83.4's safe-update mode, which prompts for one on every compile that touches secrets or actions.
COPILOT_GITHUB_TOKEN, referenced by exactly one new gh-aw-authored step ("Check for OAuth tokens",check_oauth_tokens.sh) alongside the two GitHub tokens this workflow already passes. We do not set that secret; it resolves empty and the step is a token-shape check, not a network call. Compilation of this repo's reviewer reported no unapproved secret changes.actions/*andgithub/gh-aw-actions/setup, resolved by the compiler from the release tags shown above; no new action publisher appears.ghcr.io/github/*, and the firewall trio moves from a bare0.27.27tag to0.27.42with per-container digests, which is stricter than what it replaces.patchdiff.githubusercontent.com(a gh-aw default) and the awf config gainsisolation: true; no domain we rely on was removed.Follow-ups (not in this PR)
claude-opus-5appears in neither the curated pricing table nor the bundledmodels.dev.catalog.jsonat firewall v0.27.42. Worth confirming against a live dispatch before that PR lands, since the failure mode is a per-dispatch 400, not a compile error.engine.modelfield is deprecated in v0.83 in favour of a top-levelmodel:; the compiler warns 6 times per compile. Mechanical, but it changes model resolution for consumers, so it deserves its own change.