Skip to content

Bound review fan-out: shared agent budget, live ignore_paths, evidence caps - #68

Merged
AbirAbbas merged 5 commits into
mainfrom
fix/review-wide-concurrency-budget
Aug 10, 2026
Merged

Bound review fan-out: shared agent budget, live ignore_paths, evidence caps#68
AbirAbbas merged 5 commits into
mainfrom
fix/review-wide-concurrency-budget

Conversation

@AbirAbbas

@AbirAbbas AbirAbbas commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes #65.

Summary

The Aug 5 OOM crash was caused by the pipeline's peak-concurrency window having no shared limiter: Phase 6 (coverage loop) and Phase 6.7 (consistency-verify) run concurrently, the former with a fresh per-call Semaphore(8) and the latter with none at all, so ~16 opencode subprocesses could be in flight while a 60k-line lockfile-regen diff inflated every prompt. This PR bounds every contributing layer on the pr-af side; the SDK-side buffering fix is Agent-Field/agentfield#903.

Changes

1. Review-wide agent-concurrency budget (feat(budget))

  • One orchestrator-owned semaphore gates every leaf agent invocation across all phases — including phases that run concurrently. Effective cap is min(max_concurrent_agents, max_concurrent_reviewers) so the deprecated reviewer knob still binds.
  • Env-configurable: PR_AF_MAX_CONCURRENT_AGENTS (default 8), honored by the webhook limits path and the review() API.
  • The consistency-verify obligation cap moves from a [:12] literal to PR_AF_MAX_CONSISTENCY_OBLIGATIONS.
  • Slots are held only around the leaf call, never while awaiting children — a cap of 1 serializes instead of deadlocking (covered by the existing sub-review test).
  • build_dimension_pack (which forks grep) moves off the event loop via asyncio.to_thread.

2. ignore_paths was dead config — now applied (fix(intake))

  • The config already listed **/package-lock.json, **/yarn.lock, vendor/**, *.min.js, … but was never consulted. Files matching ignore_paths are now dropped before intake and the diff is rebuilt from the kept patches, so depth resolution, anatomy, meta-selectors, reviewers, and obligation extraction only ever see reviewable files. A lockfile-regen PR now resolves to a quick review of nothing instead of a depth-escalated 16-agent fan-out.
  • ⚠️ Behavior note: the defaults also ignore *.md, *.txt, and .github/** — previously reviewed only because the config was inert. Pass ignore_paths=[] per call to restore that.

3. Evidence-extraction caps (perf(evidence))

  • Repo-wide grep children capped at 8 identifiers per finding (was unbounded).
  • _FILE_CACHE is now byte-bounded (128 MB) rather than entry-count-only, and oversized single files are served uncached.

4. Stale-workspace reaper (feat(workspaces))

  • Clones under PR_AF_WORKDIR were never removed — one checkout per reviewed PR, forever, on the persistent volume. Workspaces idle past PR_AF_WORKSPACE_TTL_DAYS (default 7, <= 0 disables) are now reaped lazily on workspace resolution. The active workspace is never touched; idleness is measured off .git/FETCH_HEAD, which every review's fetch rewrites.

5. Pin agentfield==0.1.126 in the Dockerfile (build(docker))

  • Unpinned >=0.1.84 meant every rebuild silently re-resolved the SDK (the Aug 4 redeploy jumped ≤0.1.117 → 0.1.120 as a side effect of a packaging-only commit). Upgrades are now deliberate one-line bumps.

Validation contract → tests

  • Concurrent leaf agents never exceed the cap, even with review and consistency-verify overlapping → test_shared_budget_caps_leaf_agents_across_concurrent_phases
  • Deprecated max_concurrent_reviewers still binds → test_deprecated_reviewer_knob_still_binds
  • Obligation cap configurable → test_consistency_obligation_cap_is_configurable
  • Env/API knobs plumb through → test_env_knobs_drive_budget_defaults, test_input_override_plumbs_through, test_webhook_limits_include_agent_budget
  • Lockfile-regen PR is filtered before intake and resolves to quicktest_lockfile_regen_is_filtered_before_intake
  • Pattern semantics (**/name, dir/**, basename globs) → test_pattern_semantics
  • Identifier grep fan-out capped; cache byte-bounded; oversized files uncached → tests/test_evidence_caps.py
  • Stale workspaces reaped; active/fresh/TTL-disabled/non-dir cases untouched → tests/test_workspace_reaper.py

Ran locally: full pytest (103 passed) and ruff check src/ scripts/ clean, per-commit. Also verified end-to-end: a real review through a local control plane + this branch's Python node (opencode mocked via the e2e shim, live .ai() gates) — review succeeded under a budget of 3, the fixture's package-lock.json was filtered at intake (1607 diff lines dropped), and measured peak concurrent opencode subprocesses was 2.

Related


🤖 Generated with Claude Code

AbirAbbas and others added 3 commits August 10, 2026 09:01
Phase 6 (coverage loop) and Phase 6.7 (consistency-verify) run concurrently,
but each brought its own limiter — a fresh Semaphore(8) per
_run_parallel_review call, and none at all for the 12 verify_obligation
agents — so ~16 harness subprocesses could be in flight at once and the
node OOM-crashed reviewing a lockfile-regen PR (#65).

One orchestrator-owned semaphore now gates every leaf agent invocation
across all phases. It is sized min(max_concurrent_agents,
max_concurrent_reviewers) so the deprecated reviewer knob still binds, and
is env-configurable via PR_AF_MAX_CONCURRENT_AGENTS. The consistency-verify
obligation cap moves from a literal to PR_AF_MAX_CONSISTENCY_OBLIGATIONS.
Slots are only ever held around the leaf call itself — never while awaiting
children — so low caps serialize instead of deadlocking.

Also moves build_dimension_pack off the event loop (it forks grep
subprocesses synchronously).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ignore_paths was dead config: defined with the right defaults
(package-lock.json, yarn.lock, vendor/**, *.min.js, …), merged from API
input, and never consulted anywhere. The 60k-line lockfile regen that
triggered the crash in #65 flowed straight into every agent prompt, tripped
depth escalation, and fed obligation extraction.

Filter changed_files (and rebuild the diff from the kept patches, in the
same format the GitHub client already uses for oversized PRs) before
intake, so depth resolution, anatomy, meta-selectors, reviewers, and
consistency-verify only ever see reviewable files.

Behavior note: the defaults also ignore *.md, *.txt, and .github/** — those
were previously reviewed only because the config was inert. Callers who
want them reviewed can pass ignore_paths=[] per call.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Evidence extraction forked one repo-wide grep per identifier mentioned in a
finding body, with no cap and 10 findings extracting concurrently — these
child processes stacked on top of the opencode subprocesses during the #65
crash (EAGAIN on thread spawn). Cap it at 8 identifiers per finding.

_FILE_CACHE is process-lifetime and was bounded only by entry count: 2000
large files can pin multiple GB across reviews. Bound it by bytes (128MB),
and serve oversized single files uncached.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AbirAbbas and others added 2 commits August 10, 2026 10:11
Workspaces under PR_AF_WORKDIR were created per reviewed PR and never
removed, so the persistent volume grew without bound (#65). Reap lazily on
workspace resolution: directories idle longer than the TTL (default 7 days,
<= 0 disables) are deleted. The workspace being resolved for the current
review is never touched, and idleness is measured off .git/FETCH_HEAD —
which every review's fetch rewrites — so concurrently active workspaces
are naturally skipped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An unpinned 'agentfield>=0.1.84' means every image rebuild silently
re-resolves the SDK — the Aug 4 redeploy jumped <=0.1.117 -> 0.1.120 as a
side effect of a packaging-only commit (#65). Pin the exact version so SDK
upgrades are deliberate, reviewable bumps.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas

Copy link
Copy Markdown
Contributor Author

Note on the agentfield==0.1.126 pin: this is deliberately the latest stable on PyPI and does not include the SDK-side output-capture bounding (Agent-Field/agentfield#903, merged — currently only in v0.1.127-rc.6, and rc's are not published to PyPI). The crash mechanism itself is fully addressed by this PR's own changes; #903 is defense-in-depth for pathological harness streams. The e2e verification in this PR ran against exactly agentfield 0.1.126, i.e. the pinned combination. When 0.1.127 stable lands on PyPI, bump this pin one line to pick up #903.

@AbirAbbas
AbirAbbas merged commit 8593130 into main Aug 10, 2026
4 checks passed
@AbirAbbas
AbirAbbas deleted the fix/review-wide-concurrency-budget branch August 10, 2026 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OOM on lockfile-regen PRs: Phase 6+6.7 harness fan-out has no shared concurrency budget

1 participant