Skip to content

review: raise the sub-agent turn cap from 30 to 100 #369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 3 commits into from
Aug 24, 2026
Merged

review: raise the sub-agent turn cap from 30 to 100 #369

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc7d0fa
[jwbron/review-subagent-turn-cap] review: raise the sub-agent turn ca…
jwbron Aug 21, 2026
d3f89cb
review: raise the dispatcher Bash ceiling to 30 minutes alongside the…
jwbron Aug 21, 2026
d30bd86
[jwbron/review-subagent-turn-cap] review: correct the ceiling comment…
jwbron Aug 21, 2026
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
7 changes: 7 additions & 0 deletions .changeset/review-raise-subagent-turn-cap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"review": patch

@github-actions github-actions Bot Aug 21, 2026

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): patch under-states the bump: the structurally identical prior change (CHANGELOG 1.8.0, 82af000, timeout-minutes 20 to 40) shipped under Minor Changes. This repo makes the changeset's semver level an explicit reviewer duty (.github/aw/review/ci-tooling.md: "semver level matches the change; CI can't judge that (see risk-classification.md)", and risk-classification.md's "Reviewer behavior contract" section), and workflows/review/README.md calls semver the behavior contract. This change moves operational limits every consumer inherits, which is more than a patch by this repo's own precedent. I weighed the PR description's argument that nothing succeeding today behaves differently — it's a good reason not to go major, but it doesn't get this down to patch.

A sketch, not a committable replacement:

"review": minor
Lower-confidence observations (1)
  • workflows/review/lib/budgets.ts:13 note (non-blocking): The refreshed sizing sentence updates the minutes half of the "20-minute / $10" envelope but not the dollars half, and the turn-cap raise is the one thing that moves the dollars. (correctness-reviewer)
review details found by correctness-reviewer

---

Raise the sub-agent turn cap from 30 to 100. The correctness pass on large diffs (Khan/actions#295, runs 32422547351 and 32491692754) hit the old cap on two consecutive runs, ending each in error_max_turns and a HOLD_FOR_HUMAN at $11.74-$13.45 of wasted sub-agent spend per run. The turn cap is a loop guard; credit spend, per-finding tool calls, and wall clock are metered separately, so agents that finished under the old cap (8-37 reported turns) are unaffected. The eval producer's cap is bumped in lockstep so trials keep reproducing prod behavior.

Also raise the dispatcher's Bash ceiling from 20 to 30 minutes (`BASH_MAX_TIMEOUT_MS` 1200000 to 1800000, and the prompt's dispatcher `timeout` with it) and the job's `timeout-minutes` from 40 to 50. The dispatch phases are sequential (finders, dedup, claim validation, each sub-agent under the 15-minute cap), so worst case is ~30 minutes; run 32418662895 (Khan/actions#362) was killed mid-claim-validation at the 20-minute line and posted nothing, and longer-running agents under the new turn cap make that more likely, not less.
3 changes: 2 additions & 1 deletion workflows/review/eval/live-producer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ export type ProduceLiveOptions = {
reReviewMode?: ReReviewMode;
};

const DEFAULT_MAX_TURNS = 30;
/** Keep in sync with lib/dispatch.ts so trials reproduce prod behavior. */
const DEFAULT_MAX_TURNS = 100;
const DEFAULT_TIMEOUT_MS = 5 * 60 * 1000;
const DEFAULT_CONCURRENCY = 4;

Expand Down
3 changes: 2 additions & 1 deletion workflows/review/lib/dispatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@ export type AgentRunner = (request: AgentRequest) => Promise<AgentResult>;
const REVIEW_DIR = "/tmp/gh-aw/review";
const OUT_DIR = `${REVIEW_DIR}/out`;

const DEFAULT_MAX_TURNS = 30;
// 100, not 30: #295 held twice on error_max_turns (a loop guard only).
const DEFAULT_MAX_TURNS = 100;
/**
* Per-sub-agent wall-clock cap. 15 minutes, not 5: trial run 29901690493
* killed both default finders (correctness-reviewer, skill-auditor) at
Expand Down
15 changes: 12 additions & 3 deletions workflows/review/review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,22 @@ observability:
# 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.
#
# The ceiling is 30 minutes (with timeout-minutes at 50), not 20 (at 40): the
# dispatch phases are sequential (finder fan-out, dedup, claim validation, each
# sub-agent under the 15-minute per-agent cap), so worst case is ~30 minutes
Comment thread
khan-actions-bot marked this conversation as resolved.
Outdated
# before overhead, and run 32418662895 (Khan/actions#362) was killed mid-claim-
# validation at the 20-minute line, posting nothing. Raising the turn cap to 100
# (this release) lets agents run longer, so the ceiling moves in lockstep; the
Comment thread
khan-actions-bot marked this conversation as resolved.
Outdated
# job cap keeps ~20 minutes of headroom over the dispatcher call so the two
# never collapse into the same kill line.
engine:
id: claude
model: claude-opus-5
env:
BASH_DEFAULT_TIMEOUT_MS: "60000"
BASH_MAX_TIMEOUT_MS: "1200000"
timeout-minutes: 40
BASH_MAX_TIMEOUT_MS: "1800000"

@github-actions github-actions Bot Aug 21, 2026

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): Fix reaches this repo's own reviewer only after a follow-up install bump the description doesn't mention. Confirmed the installed copy at .github/workflows/review.md:198-199 still reads 1200000 / timeout-minutes: 40 and .github/workflows/review.lock.yml:976,1008 matches; .github/workflows/review-pins.test.ts documents that this install is pinned to a release and that "no release automation touches" those pins. That's the normal flow here, not a defect in this diff — but since the stated motivation is Khan/actions' own runs (#295, #362), it's worth saying that the follow-up install bump is what actually stops the holds. No linked Jira/Confluence ticket was referenced in the title, description, or branch, so this is judged against the PR description alone.

review details found by completeness

@github-actions github-actions Bot Aug 21, 2026

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): Nothing couples BASH_MAX_TIMEOUT_MS to the dispatcher timeout the prompt hard-codes 573 lines away at line 769. The coupling is pre-existing (both were 1200000); this change is the second hand-edit of both literals and still adds no backstop, so the drift window stays open. This repo already treats exactly this shape as test-worthy: workflows/review/cache-miss-guard.test.ts ("nothing at compile time can couple them; this test is the coupling") and .github/workflows/review-pins.test.ts ("coupled pins that no release automation touches"). Both literals live in this one file, so a regex assertion in a sibling test covers it.

A sketch, not a committable replacement:

// workflows/review/bash-ceiling.test.ts
import * as fs from "fs";
import {describe, expect, it} from "vitest";

const reviewMd = fs.readFileSync(new URL("./review.md", import.meta.url), "utf-8");

describe("the dispatcher Bash call vs the engine's Bash ceiling", () => {
    it("asks for exactly the timeout the engine allows", () => {
        const ceiling = reviewMd.match(/^\s*BASH_MAX_TIMEOUT_MS:\s*"(\d+)"\s*$/m)?.[1];
        const asked = reviewMd.match(/`timeout` set to\s*\n?\s*`(\d+)`/)?.[1];
        expect(ceiling).toBeDefined();
        expect(asked).toBe(ceiling);
    });
});
review details found by correctness-reviewer | also flagged by test-adequacy

timeout-minutes: 50
Comment thread
khan-actions-bot marked this conversation as resolved.

@github-actions github-actions Bot Aug 21, 2026

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): PR description covers only the turn cap; the timeout bump is undescribed scope. Title and description are entirely about DEFAULT_MAX_TURNS 30→100, and the description explicitly says the change is kept to "a two-line change plus a changeset" and enumerates what was deliberately excluded — but the diff also raises BASH_MAX_TIMEOUT_MS 1200000→1800000, the dispatcher prompt's timeout to 1800000, and timeout-minutes 40→50, justified only in the changeset and the new review.md comment (a distinct incident, run 32418662895 / #362). Worth folding a sentence about the timeout bump into the description so the stated intent matches the diff.

review details found by completeness | also flagged by first-principles: The PR description doesn't cover the timeout half of the diff.


# The awf sandbox stays declared (its api-proxy is what meters AI credits and
# caps a runaway fan-out), but its version now floats with the gh-aw release
Expand Down Expand Up @@ -755,7 +764,7 @@ exactly this sequence:
none. This is the only thread work left to you: what a reply chain concedes
or refutes is a judgment, while fetching and classifying the threads is not.
2. Invoke the dispatcher, once, as a single Bash call with `timeout` set to
`1200000` (it waits for the whole sub-agent fan-out; the engine's Bash
`1800000` (it waits for the whole sub-agent fan-out; the engine's Bash
ceiling is raised for exactly this call):
```
cd gh-aw-review-lib && REVIEW_REPO_ROOT="$GITHUB_WORKSPACE" \
Expand Down
Loading