fix(#1177): size the review budget from the pipeline floor, materialise the PR head once - #1178
Conversation
|
🤖 Review · Commit: |
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 5:31 PM UTC · Completed 6:13 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
|
/fs-fix-stop |
|
Fix agent disabled for this PR. Remove the |
The per-iteration 20-minute budget killed large-PR reviews (50+ files, ~5.7k diff lines) -- both iterations ran to exactly 20m0s with zero output. The orchestrator inlined the full source of every changed file into each sub-agent prompt, exhausting the context window and time budget before dispatch completed. Changes: - harness/review.yaml: raise timeout_minutes from 20 to 45. Set validation_loop.max_iterations to 1 -- a retry of a timed-out review from scratch is pure waste; the validation loop exists for schema failures, not for running out of time. - skills/pr-review/SKILL.md: for large PRs (>20 files or >5000 lines), omit source_files from context packages entirely -- sub-agents receive only per-file diffs and use their Read tool on demand. Default Sonnet-tier read-only dimensions (intent-coherence, style-conventions, docs-currency, cross-repo-contracts) to the Explore child type for a lighter footprint. Note: pre-commit hooks were not run. pre-commit could not complete (infrastructure failure -- network-restricted sandbox), and hooks were run directly where possible (check-yaml, trailing-whitespace, lint-agent-docs). shellcheck/actionlint/gitleaks/pinact do not apply to .yaml/.md files. Closes #1177
769a314 to
7026930
Compare
waynesun09
changed the title
|
🤖 Finished Review · ✅ Success · Started 3:13 PM UTC · Completed 3:34 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.13 |
|
Risk Assessment: elevated (3/5) DetailsElevated risk preserved from prior assessment: 8 protected paths and a large blast radius continue to dominate, with churn activity now at 150 commits from 14 authors in 90 days, partially offset by bot authorship, zero security exposure, no dependency or CI changes, and a well-scoped fix traceable to issue #1177. Previous runRisk Assessment: elevated (3/5) DetailsElevated risk due to large blast radius across 8 protected skill and harness paths that have accumulated 10 distinct contributors and 58+ commits in 90 days, partially offset by bot authorship, zero security exposure, no dependency changes, and a well-scoped bug fix clearly traceable to issue #1177. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7026930 to
8d0b6e1
Compare
|
🤖 Review · Commit: |
Fleet data (2026-08-18 → 09-05, 280 `dispatch / Review` jobs across fullsend-ai/fullsend, fullsend-ai/agents and nonflux/integration-service) shows the review pipeline has a ~13–18 minute floor that does not depend on PR size: a 51-line PR and a 5 700-line PR both died at 20m0s with the challenger pass still running. Raise timeout_minutes to 45 and mirror it into the sandbox as TIMEOUT_SECONDS (the code/fix convention) so the skill can budget its own tail. max_iterations drops to 1 as an interim: the runner cannot tell a timeout from a schema failure (fullsend-ai/fullsend#7042), so every second iteration in the window (49 runs) was a from-scratch replay of a killed review — zero schema failures — and only 11 finished. Restore 2 with feedback_mode: append once the runner distinguishes the two. The review eval's case timeout moves with the harness but stays sized to the functional-tests job (1800/1700 like eval/fix): a case past 30 min fails the eval, not the job. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
8d0b6e1 to
41a5fcb
Compare
|
🤖 Review · Commit: |
…d sub-agents the diff by path Where the minutes go (two local runs on a 29-file PR, and the fleet transcripts): a risk-assessment pre-pass that ran alone for 2–3 minutes before anything else was dispatched, ~3 minutes streaming seven prompts that each carried the diff and every changed file, dimension sub-agents (4–13 minutes, correctness the long pole), then a 2.5–6 minute challenger that is what the kill landed on in every small-PR failure. - Time budget section keyed on TIMEOUT_SECONDS: start time persisted to /sandbox/workspace/agent-start, `test`-based checks with a 120 s buffer, skip the challenger under 600 s (a low finding says so), and write a `failure` result with `reason: time-budget` under 240 s with dimensions still outstanding — the post-script posts that notice, a kill posts nothing. `time-budget` joins the schema's reason enum with a passing and a rejecting schema test. - The risk-assessment prompt is composed in 3c-2 and dispatched in the same message as the dimension sub-agents; nothing in step 4 consumes its output. - Step 4 names `model` from each persona's frontmatter and drops the first cut's `subagent_type: "Explore"` — a search persona whose own description excludes review; the dimensions were never the bottleneck. - The first cut omitted source files for large PRs and told sub-agents to Read from disk, which is the base branch. Instead the orchestrator materialises every changed file at HEAD_SHA once, in parallel, under /sandbox/workspace/pr-head/ with a manifest beside it (ok / too-large / binary / failed / removed / unsafe), writes the diff to /sandbox/workspace/pr-diff.txt (per-file patches from the forge for large PRs — never `git diff` against the base checkout), and context packages carry paths, not contents. Anything not `ok` stays explicitly unverifiable at the PR head; the meta-prompt, challenger and agent workspace text say the same thing, and sub-agents page the diff. - The forge snippets are written for the sandbox's guards: tirith 0.4.0 blocks `[ ]`, nested `$( )`, `xargs … sh -c`, subshell groups and `curl --header "PRIVATE-TOKEN"`, and Claude Code's dangerous-rm check refuses `rm -f "$VAR/…"` — so `test`, two-step arithmetic, simple backgrounded commands, marker files, and a `curl -K` config file under /tmp scrubbed in its own call. The forge payload is saved to disk once because shell variables do not survive between Bash calls. Every embedded snippet scans `allow` with the pinned tirith; the GitHub one is exercised end to end (29/29 files, 404 → failed, PNG → binary, `-n` → failed, removed and unsafe paths listed). Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
41a5fcb to
99ff315
Compare
|
🤖 Review · Commit: |
Where a review's minutes go, what the PR sees when the budget runs out (the challenger-skip finding, the time-budget failure notice, or — after a kill — nothing), how to raise the budget in a base overlay with both keys moving together, and why max_iterations is 1 for now. The timeline is a real local run of this harness on a 29-file PR. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Keep the decision and the issue reference in each comment; the measurements that justified them live in the PR. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
99ff315 to
b47bda3
Compare
|
🤖 Finished Review · ✅ Success · Started 4:38 PM UTC · Completed 5:19 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.03 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approved after takeover: budget sized from the measured pipeline floor (280 fleet review jobs), one interim iteration citing fullsend#7042, budgeted tail with challenger skip, PR head materialised once and passed by path, validated on fullsend#7038 (branch harness 21.3/25.5 min passed vs main killed 2x20m), three review rounds incl. Codex gpt-6-astra and Grok, CI green on b47bda3.
Outdated: all four threads (all low: two acknowledgements, one duplicate scope note answered on the earlier thread, one cosmetic table separator deferred to the next docs change) are answered and resolved; PR is approved and queued at b47bda3.
|
🤖 Finished Retro · ✅ Success · Started 5:37 PM UTC · Completed 5:49 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.59 |
|
PR #1178 fixed the review agent's 20-minute per-iteration timeout that killed reviews regardless of PR size (issue #1177). The code agent produced a basic fix (+21/−30 lines, 2 files); the human (waynesun09) substantially reworked it into a comprehensive solution (+393/−184 lines, 12 files) with time budget checkpoints, PR head materialisation, and sandbox-aware shell snippets. Only the timeout bump survived from the code agent's output — its other changes (omitting source files for sub-agents to Read from disk, which gives the base branch not the PR head; assigning the Explore sub-agent type, whose description explicitly excludes review) were correctly identified as wrong and replaced. Review quality: high. The agent posted 12 findings across two rounds. The most impactful — a severity mismatch ( Cost: $17.16 for the two successful reviews ($8.13 + $9.03). The initial review (33900756308) failed after 2 × 20-minute iterations — the very timeout the PR was fixing — wasting ~$16–18 in compute. Three subsequent reviews were cancelled during rapid human iteration (33975380024 ran 22 min, 33976534702 ran 29 min, 33978155264 ran 2 min). Estimated total spend including wasted runs: $40–50. Evidence for existing open issues (no new proposals needed):
|
1 participant
Closes #1177.
What the data says
The issue framed this as a large-PR problem. Fleet data (280
dispatch / Reviewjobs across fullsend-ai/fullsend, fullsend-ai/agents and nonflux/integration-service, 2026-08-18 → 09-05) says the review pipeline has a ~15–18 minute floor that does not depend on PR size, and the 20-minute budget sat on that floor: a 51-line PR (#1178's own review) and a 5 700-line PR both died at20m0swith the challenger pass still running.Successful runs only for the percentiles (job minutes ≈ agent minutes + 3.5); failures counted separately:
Every one of the 49 second iterations followed a
20m0skill — zero schema failures in the window — and the 11 that "succeeded" were from-scratch replays that happened to land under the cap. Where the minutes go, from the runner's transcript echo of #1178's own failed review (run 33900756308, 51 lines): 0–5 min context, 5.5–8 risk pre-pass sequential (with a Sonnet alias 404 and re-dispatch inside it), 9–10.5 dispatching four dimensions, 11.5–15.5 dimensions complete, 16.5 challenger dispatched, 20.0 killed, iteration 2 restarted from zero.What changes
timeout_minutes20 → 45TIMEOUT_SECONDS(the code/fix convention)max_iterations2 → 1pr-reviewskill)failureresult withreason: time-budgetwith < 240 s left and dimensions still running — the post-script posts that notice, a kill posts nothingmodelfrom each persona's frontmatter stated explicitly;subagent_type: "Explore"dropped (Explore is a search persona whose own description excludes review; the dimensions were never the bottleneck — 3c-1's haiku triage keeps it, deliberately)HEAD_SHAonce, in parallel, under/sandbox/workspace/pr-head/(outside the read-only checkout) with a MANIFEST; context packages carry paths, not contents; anything notokstays explicitly unverifiableeval/review/eval.yamldocs/review.mdWhat stays in this repo vs fullsend
Everything above is agents-side and works with today's runner. What the runner still cannot do is filed as fullsend-ai/fullsend#7042: treat a timed-out validation-loop iteration as a timeout (not a validation failure that retries from scratch), export the budget/deadline into the sandbox (which would replace the static
TIMEOUT_SECONDSmirror), and send a wrap-up signal before the kill. When that lands,max_iterationsgoes back to 2 withfeedback_mode: append.Testing
Local runs of this harness with
fullsend run review --forge github --no-post-scripton fullsend-ai/fullsend#7038 (29 files, +2336/−139), target repo cloned atmain, Claude Code 2.1.258, Opus 4.6 orchestrator:request-changes, 13 findings, validation passedxargs … sh -csnippet blocked by tirith; orchestrator improvised the fetch;[ -ntime check blockedrequest-changes, 7 findings, validation passedtest-dialect snippet;rm -f "$PR_HEAD/$f"refused by the dangerous-rm check → orchestrator dropped thermand completed; time checks at 7.0 min (Elapsed: 394s, Remaining: 2186s), 19.2 min (Elapsed: 1130s, Remaining: 1450s), 23.2, 24.0origin/main@ b506757 (20 min / 2 iterations)validation failed after 2 iteration(s), nothing posted (runner writes no metrics on that path)20m0swith the challenger running; iteration 2 was a from-scratch replayBoth branch runs would have died at
20m0swith the challenger in flight — and the baseline run on the same PR did exactly that, twice. The 7 (C) / 6 (C2) sub-agent dispatches were a single assistant message each (verified in the transcript), the risk pre-pass among them; the orchestrator's computed remaining time matched the runner ticker inside the 120 s buffer.Materialise step: 29/29 files in 3 s inside the sandbox (C2 transcript:
pr-head: 29 of 29 files in 3s), 1 s from a laptop; final snippet exercised locally against the same PR — 29/29ok, a 404 path →failed, a PNG →binary,removedfiles excluded — and scannedallowwith tirith 0.4.0 both locally and viaopenshell sandbox execin a live sandbox.skills/pr-review/github/SKILL.md"Materialise PR head files",skills/pr-review/gitlab/SKILL.md"Materialise MR head files", the Time budget check inskills/pr-review/SKILL.md) scansallowwith the sandbox's pinned scanner (tirith 0.4.0 check --json --non-interactive --shell posix, HOME isolated so no personal policy applies), and the materialise snippet also viaopenshell sandbox execin a live review sandbox. The pre-existing## PR data fetching/## MR data fetchingblocks scanblockonorigin/maintoo (skills: documented shell snippets are blocked by the sandbox command scanner (time check, GitLab MR-head fetch) #1190). The first cut of the GitHub snippet usedxargs … sh -cand was blocked in the sandbox (analysis_incomplete), which is how the constraint was found.eval-results-reviewartifact): case 002 ran the materialise block as written (pr-head: 3 of 3 files ok in 0s); case 004 ran a copy with the&/waitand the timing lines dropped (pr-head: 1 of 1 files ok); every time check printed a sane remainder (REMAINING: 2486s/2245son 001,2458s/2154son 002,2281s/2231son 004). Cases 001 and 003 (two- and one-file PRs) improvised a shorter fetch instead; 003's carried[ … ]and a one-lineif …; then …=$(( … )); fiand was blocked twice by the scanner before a python fallback wrote the file. Step 2b now says to run the block as written even for a one-file PR. The one-lineifis a dialect fact the earlier bisect missed:$(( ))insideif …; then …; fion a single line blocks, the same statement across lines passes — the shipped time check is multi-line.info-vs-lowwording, the roster row's lost domain text and trailing padding, Title Case table headers — fixed in 9566ccb / eefc212 — and two scope notes (parallel risk dispatch, fetch-all on small PRs) answered without change. All resolved.Known limits
tirith 0.4.0 blocks
[ … ]/[[ … ]]tests,$( )nested inside$(( )), and anycurl --header "PRIVATE-TOKEN: …"— the snippets here usetest, two-step arithmetic and acurl -Kconfig file instead. The pre-existing snippets elsewhere (code-implementation / fix-review time checks, the GitLab file fetch) are affected too: skills: documented shell snippets are blocked by the sandbox command scanner (time check, GitLab MR-head fetch) #1190; the scanner rule itself: sandbox hooks: tirith 0.4.0 blocks POSIX[ … ]tests and$( )inside$(( ))as analysis_incomplete (HIGH), breaking documented agent snippets fullsend#7043.The 240 s checkpoint only fires when the orchestrator regains control (a sub-agent returning); a single sub-agent that hangs past the deadline still ends in a kill. A deadline independent of worker completion needs the runner-side signal in run: a timed-out validation-loop iteration must not retry from scratch; export the budget and send a wrap-up signal fullsend#7042.
Paths containing URL-special characters (space,
#,?,%) are fetched unencoded on GitHub — the contents API does not decode%2F, so encoding would break every path — and land asfailed(unverifiable) rather than wrong. Sixteen fetches in flight can trip GitHub's secondary rate limit on very large PRs; those files also land asfailed, never asok.The GitLab snippet is scanner-clean and
bash -nclean but was not executed against a GitLab instance.The orchestrator may still hand-roll a fetch on a one-file PR despite step 2b; a blocked improvisation costs a retry, not the review (case 003 recovered on its third attempt).
Sub-agent
tools:frontmatter is documentation, not enforcement — the persona travels as prompt text — soreadonly_repo, OPA and the meta-prompt remain the actual controls; unchanged by this PR.TIMEOUT_SECONDSis a static mirror oftimeout_minutes; an overlay that changes one must change both until the runner exports the budget (run: a timed-out validation-loop iteration must not retry from scratch; export the budget and send a wrap-up signal fullsend#7042).Review rounds (findings consumed locally, nothing posted): round 1 claude-researcher + Codex gpt-6-astra, round 2 claude-researcher + Grok 4.6. Fixed from them:
time-budgetmissing from the schema enum; bookkeeping files inside the materialised tree;AGENT_STARTnot surviving between Bash calls; an unfetched file reportedok;echoeating-n/-efilenames; abodysuppressing the post-script's NOT-reviewed notice; the eval timeout inert withoutEVAL_TIMEOUTand then too large for the job; per-file diffs fromgit diffagainst the base checkout; withheld challenger findings dropped; Read's 2000-line window; stale pre-pass wording. Not taken: percent-encoding GitHub contents paths (%2F404s),subagent_typeas required (13 live dispatches without it on the fleet's Claude Code 2.1.258).CI functional-tests (review eval, 4 fixture cases) on the first pushed head 7026930: 3 of 4 cases failed the
max_turnsjudge (37–58 turns vs 6–10 on main) because the skill said to dispatch the risk-assessment call "in the foreground" and the orchestrator obeyed by passingrun_in_background: falseon every sub-agent — which serialises the whole run and also defeats the mid-wait time checks. Main's orchestrator leaves it unset (the Agent tool is asynchronous by default and delivers completions as notifications). Fixed in the skill text (second run: 8 / 7 / 5 turns on cases 001–003). Case 004 still counted 45 against its declared 40: its five sub-agents returned inline (Claude Code backgrounds only long-running sub-agents) and their own tool turns — now Reads of the PR-head tree instead of pasted contents — are counted into the parent; itsmax_turnsannotation moves to 80 with that rationale. The eval's 3FAIL (exit 1)cases are the pre-existing fixture self-review 422 (Review Can not request changes on your own pull request), identical on main's own runs.make lint(skillsaw, context-budget ratchet on SKILL.md at 15,709 tokens),scripts/validate-output-schema-test.sh, and pre-commit on every commit (pre-commit run --from-ref <parent> --to-ref <sha>per commit).The
max_turnsjudge measures Claude Code's scheduling as much as the skill: the same case lands at 5–8 turns when sub-agents are backgrounded and 37–58 when they return inline and their tool turns are counted into the parent. Read a redmax_turnson a review case against the transcript before moving an annotation.Rollout
The fleet runs
fullsend_ai_ref: v0, andv0points at1056025(2026-09-03) — nothing merged since then, including #1181's Sonnet alias pin and this PR, reaches a fleet review untilv0is re-cut. TheSonnet … not availablere-dispatch visible in the timelines above is that gap.