perf(agent): concurrent read-only tool batches via capability gate (PR6) - #715
Conversation
Upgrade the parallel tool planner from SideEffectRead probes to the PR5 CapabilitiesOf contract: EffectReadOnly + ThreadSafe + auto-allowed, with resource-key conflict boundaries so same-path reads stay sequential. Mark audited pure reads ThreadSafe (read_file, read_minified_file, list_directory, glob, grep, skill, web_fetch). Glob/grep use scopedScan directory keys (nil for workspace-wide "." so scans do not false-conflict). Tests cover the capability gate, key conflicts, extendParallelRun windows, and catalog ThreadSafe audit for concurrent-safe reads.
WalkthroughChangesParallel read-ahead execution now relies on capability metadata, thread-safety declarations, decoded permissions, and resource-key conflicts. Tool declarations identify safe concurrent reads, while the agent computes batch boundaries that preserve ordering and avoid conflicting resources. Parallel read scheduling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentLoop
participant ParallelPlanner
participant ToolRegistry
participant ReadTools
AgentLoop->>ParallelPlanner: Select consecutive tool calls
ParallelPlanner->>ToolRegistry: Read capabilities and decode arguments
ToolRegistry-->>ParallelPlanner: Thread-safety and resource keys
ParallelPlanner->>ReadTools: Execute non-conflicting read calls concurrently
ReadTools-->>AgentLoop: Read results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-review at 7bce9ad
Substantive review of PR #715 (gnanam's PR6 — concurrent read-only tool batches via capability gate). Scope is just the PR6 commit (12 files, +285/-27); the PR's stacked diff is much larger because the branch is based on a pre-#704 / pre-#606 / pre-#712 main, but those three changes are already on main. Verdict: approve, ready to merge.
What's good
- The capability gate is the right contract.
parallelSafeToolCallchecksEffect == EffectReadOnly+ThreadSafe == true+effectivePermission == PermissionAllow+ no resource-key conflict with earlier calls in the window. That's the four conditions for safe concurrency and not one more. The fail-closed default (Caps == Unknown→ sequential) is the right call — a tool that hasn't been audited can't claim thread-safety by silence. - Same-path reads stay sequential.
resourceKeysConflictwith non-empty key sets sharing any element is the right conflict primitive.TestExtendParallelRunResourceKeyBoundaryproveskeyed_readon[a, b, a]extends from 0 to 2 (stop before the secondfile:a). Keyless tools don't false-conflict becauseThreadSafeis the eligibility gate and keys only refine same-resource collisions. That distinction matters:globandgrepusescopedScanResourceKeyswhich returnsnilfor workspace-wide scans so a missing path arg doesn't force every concurrent scan to serialize. - Read-after-write ordering is preserved.
extendParallelRunruns per-consecutive-run, not per-batch. A mutating call always breaks the run.TestRunParallelReadsNeverSpanMutatingCallproves this with a shared log: the write starts only after the first read batch's max-end, and the second read batch starts only after the write's end. This is the invariant that makes the optimization safe. - Results stay in original call order. The
precomputedToolResultslice is indexedindex-start, soresults[0]is the call atstart, regardless of completion order.TestRunExecutesConsecutiveReadsConcurrentlyasserts bothOnToolResultordering and provider-message ordering — the two places ordering can drift. - The
callbackMutexis the subtle bit that's right. A sandbox preflight can demand a permission prompt even for an auto-allowed read, andOnPermissionevent handlers append to shared session-recording state without their own locking. Without the mutex, two batched reads under a granted extra-root would race. The defer-after-lock pattern around bothOnPermissionRequestandOnPermissionis the right shape. maxParallelReadTools = 8is a reasonable bound. A turn rarely needs more than 3-4 parallel reads; 8 is a backstop against pathological model outputs without throttling normal cases.decodeCallArgsfail-closed prevents malformed JSON from entering the parallel path. A model that emits brokenArgumentsfalls back to the serial loop rather than panicking inside the goroutine. Good defensive default.NormalizeResourcePathis pure. NoEvalSymlinks/Statso it cannot cause side effects or panic on missing paths. Rejects URL-shaped values (those useendpoint:keys). Case-insensitive on Windows, separator-normalized. ThejoinUnderResourceCwdhelper forapply_patchis the right fix for "same file under different cwd" key collisions.- The 7 read tools marked
ThreadSafeare the right set.read_file,read_minified_file,list_directory,glob,grep,skill,web_fetchare all pure reads with no shared mutable state (FileTracker is mutex-guarded, scans are read-only filesystem walks, web_fetch is a network GET). No tool that was previously safe-but-unmarked would now be excluded; the gate is strictly additive. - Tests cover the right axes. The five new tests gate the capability classification (
TestParallelSafeToolCall), the conflict primitive (TestResourceKeysConflict), the run-extension logic with same-path and keyless cases (TestExtendParallelRunResourceKeyBoundary), the end-to-end concurrency with ordering (TestRunExecutesConsecutiveReadsConcurrently), and the read-after-write ordering (TestRunParallelReadsNeverSpanMutatingCall). Plus the catalogThreadSafeaudit incapabilities_test.go. No test overlaps another; the contract is the union of their assertions.
Minor notes (not blockers)
- The
EffectiveEnd()method onkeyedProbeTooland theprobeToolwrapper in the test file are a bit dense, but they're test scaffolding and the tests pass. The pattern is reusable for future capability-classification tests. - The
OnPermissionRequestandOnPermissionmutex wrap could be slightly more efficient if the callbacks were already mutex-safe (e.g. some front-ends serialize their own), but the defer-after-lock overhead is one mutex acquire per call — negligible against a 60ms read.
One operational note for kevin
The PR's stacked diff is large (2000+ lines) because the branch is based on a pre-#704 / pre-#606 / pre-#712 main. The actual PR6 commit is +285/-27 across 12 files; the rest is aimlapi and other unmerged branches stacked in. If kevin merges via the GitHub button it'll be a rebase-and-merge onto the current main, which should land cleanly. If a merge commit is used, the headline diff will look bigger than the PR6 scope.
Verdict
Approve. This is the right shape for PR6. The gate is the right contract, the conflict primitive is the right granularity, the read-after-write ordering is preserved, results stay in order, and the tests gate every claim. The branch is stale (stacked on pre-#704 / pre-#606 / pre-#712), but the actual PR6 commit is clean. Ready to merge once kevin signs off.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve. I read each of the seven ThreadSafe=true Runs against the actual implementations: read_file/read_minified_file only touch the mutex-guarded FileTracker (same-path calls serialize via file: keys anyway), glob/grep/list_directory are independent FS walks with no shared cache or index, skill is a pure disk read of the skill file (no execution, no registry state), and web_fetch shallow-copies the shared http.Client per call with a fresh Transport (stdlib client is concurrent-safe).
The gate is fail-closed — unknown tools, malformed args, non-ReadOnly, non-ThreadSafe, and non-auto-allowed all stay sequential, and normalizeCapabilities force-clears ThreadSafe for any non-ReadOnly effect so a mis-marked mutator can't slip through later. go build/vet and go test -race ./internal/agent/ ./internal/tools/ pass clean, read-after-write ordering holds (a mutator breaks the run, so a read after a write starts a fresh window), and the scope is tight — only the 7 marks + the gate/planner + tests, no unrelated tool semantics touched.
3 participants
Summary
PR6 of the performance program: run consecutive capability-safe read-only tool calls concurrently, using the PR5 effect metadata contract merged in #705.
Effect == ReadOnlyandThreadSafeand auto-allowed (PermissionAllow) and no resource-key conflict with earlier calls in the same windowextendParallelRun+resourceKeysConflict(empty keys never conflict; ThreadSafe is the safety gate)executeParallelReadBatch(≥2 calls), results consumed in original orderread_file,read_minified_file,list_directory,glob,grep,skill,web_fetchweb_search,tool_search,lsp_navigate, anything that would prompt, unknown toolsscopedScanResourceKeys—nilfor./empty (no false workspace-wide conflicts); otherwisedirectory:…. Same-pathread_filestill serializes viafile:keysDepends on #705 (already on
main). Independent of turn-bench / #712.Behavior / safety
web_fetchis ThreadSafe but still permission-gated (network prompt stays sequential until auto-allowed)Test plan
make buildmake lint+ golangci unused/ineffassign/staticcheck on agent+toolsgo test -race ./internal/agent/ ./internal/tools/Stack / ownership
Performance program: PR5 → PR6 → PR11. This is PR6 (concurrent read-only batches).
Summary by CodeRabbit
Performance
Reliability
Tests