perf(tools): bound foreground filesystem work and cancellation - #288
Conversation
|
/hermes review |
Hermes Review BotConfidence: 5 Engine: SummaryBounds foreground filesystem operations ( Confidence Score: 5/5Every changed file, boundary condition, and edge case was inspected and traced end-to-end against parent/subagent security models, concurrency scopes, and test contracts. 📁 Important Files Changed
FindingsNo findings. Sequence DiagramsequenceDiagram
autonumber
actor Agent as Agent Turn
participant Scope as ForegroundReadScope
participant Tool as Parent Tool (Grep/Glob)
participant Worker as Matcher Worker
participant FS as Filesystem
Agent->>Scope: execute(params, signal)
Scope->>Tool: run(operationSignal)
Tool->>Worker: spawn & initialize worker
Worker-->>Tool: ready (seeds / ack)
loop Traversal & Content Scan
Tool->>FS: open / opendir (bufferSize: 1)
FS-->>Tool: entries / file chunk
Tool->>Worker: match(lines) / step(request)
Worker-->>Tool: matched indices / next tasks
end
Tool->>Worker: terminate()
Worker-->>Tool: exit
Tool-->>Scope: formatted bounded result
Scope-->>Agent: textResult(matches + warnings)
[]
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new parent glob traversal drops results for valid brace patterns that mix absolute and relative alternatives, so native fs.glob compatibility is incomplete.
Reviewed changes I reviewed the bounded foreground file-tool implementation and its worker-backed cancellation and glob traversal changes at b37b4b2.
- Bounded filesystem work adds capped descriptor reads, directory enumeration, aggregate grep input, match counts, and output.
- Worker-backed matching moves JavaScript regex matching off the Electron main thread and terminates workers on cancellation and deadlines.
- Native glob traversal replaces direct
fs.globiteration with bounded segment-state traversal and differential tests.
GPT Luna ｜ 𝕏
There was a problem hiding this comment.
Important
This PR has a glob result regression and foreground filesystem calls that can outlive cancellation or deadlines.
Reviewed changes This review covers bounded parent filesystem reads and scans, worker-isolated grep/glob matching, compatibility handling, and the new tests and performance evidence.
- Bounded filesystem tools.
read_filenow reads only a capped descriptor prefix;list_dir,glob, andgrepcap enumeration, grep bytes, results, elapsed work, and output. - Isolated search matching. Grep and glob use disposable workers, while glob traversal uses minimatch segments and host-side workspace checks.
- Compatibility and evidence. The PR adds behavioral tests, benchmark receipts, an Electron smoke harness, and the pinned
minimatchdependency.
GPT Luna ｜ 𝕏
|
/hermes review |
|
/hermes review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed the follow-up changes since the prior Pullfrog review at b37b4b2.
- Corrected mixed-root glob expansion. Derived each brace-expanded alternative's root independently and added native differential and workspace-confinement coverage.
- Bounded cancellation and pending I/O. Added foreground operation ownership across
read_file,list_dir,glob, andgrep, retaining admission until I/O and cleanup settle while callers and workers respond to cancellation and deadlines.
GPT Luna ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed the Windows glob-root correction added since the prior Pullfrog review at b1bc608.
- Normalized Windows glob roots. Counted slash-normalized minimatch root segments so drive and UNC alternatives skip the parsed prefix correctly, with Windows-semantics worker fixtures for drive, UNC, and mixed-root patterns.
GPT Luna ｜ 𝕏
There was a problem hiding this comment.
Important
The final offset fix still mis-seeds a valid UNC glob whose pattern is exactly a share root without a trailing slash. This can return a duplicated share component rather than the requested root.
Reviewed changes Re-reviewed the final Windows normalized glob-root offset fix and behavioral fixtures against the full PR at exact head b28f681e863ecdccabd7aba551b8cc79c71f6535.
- Normalized Windows root offsets. Replaced host-separator counting with normalized slash counting and added production-worker fixtures for drive, UNC, and mixed-root patterns.
GPT Luna ｜ 𝕏
|
/hermes review |
|
/hermes review |
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes Re-reviewed the Electron smoke runner, CI registration, and refreshed evidence added since the prior Pullfrog review at b28f681.
- Registered the Electron smoke runner. Added an isolated pinned-Electron package command as a required Desktop verification step, with CI-policy coverage.
- Refreshed the runtime evidence. Updated the benchmark and smoke receipts; one now records an author-local absolute workspace path.
GPT Luna ｜ 𝕏
|
/hermes review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed the two commits added since the prior Pullfrog review at 73f8664.
- Fixed bare UNC share-root matching. Consumed the complete Windows share root and added worker fixtures for bare roots with and without a trailing slash, including mixed relative arms.
- Normalized Electron smoke receipts. Replaced the synthetic workspace's absolute prefix with
<workspace>in emitted glob fixtures while retaining real paths in runtime assertions.
Focused native glob and Windows worker tests and npm run type-check passed. Electron smoke could not run in this Linux environment because the SUID sandbox helper was not configured.
GPT Luna ｜ 𝕏
|
/hermes review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed the commit added since the prior Pullfrog review at 5059c78.
- Isolated the recovery-stream gate. Held the exact stream-consumer endpoint and exercised the unrelated progress stream first, preventing it from consuming the test gate.
- Preserved simulator diagnostics. Added an
.xcresultoutput and seven-day failure artifact, with CI-policy coverage for the configuration. - Added mode-specific failure context. Included upload-lifecycle mode names in assertion messages without changing the assertions or timeouts.
The CI policy suite passed (47 tests). iOS simulator XCTest could not be run in this Linux review environment.
GPT Luna ｜ 𝕏
|
/hermes review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed the PR at 0beec58, including the commits added since the prior Pullfrog review at 30b042b.
- Joined foreground cleanup in the Electron smoke. Added observation and joining of the underlying operations after caller cancellation or deadline, and retained late cleanup errors so sequential samples do not race the four-operation admission limit.
- Integrated current
main. Mergedmainat137ce6bd1and retained the combined CI/test registration and audit notes.
The focused coding-tool and generation-runtime suites passed (79 tests), as did type-check, smoke-script lint, and CI policy (47 tests). The Electron smoke bundled successfully but could not launch in this Linux environment because its SUID sandbox helper is not configured; Electron exited with SIGTRAP.
GPT Luna ｜ 𝕏
|
/hermes review |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ No critical issues — one minor documentation suggestion.
Reviewed changes Re-reviewed the merge and cleanup commits added after the previously checked head ad38615.
- Merged current
main. Integratedmainat4d0d982. - Removed PR-specific troubleshooting notes. Dropped 47 lines from
.papercuts/troubleshooting.md; foreground-tool behavior is unchanged.
ℹ️ The worker lifecycle test rationale was dropped
The new commit removes the only durable note that a live idle worker's MessagePort may be absent from _getActiveHandles(). The actual-termination test remains, but retaining why that oracle matters would help prevent future tests from mistaking an unreported port for a terminated worker.
Technical details
# Preserve the worker-lifecycle test oracle rationale
## Affected sites
- `.papercuts/troubleshooting.md` — commit `e980e599` removes the note that a live idle worker's `MessagePort` may be absent from Node's `_getActiveHandles()`.
- `main/services/coding-tools.test.ts:2480-2503` — the lifecycle test awaits actual `Worker.terminate()` promises but does not explain why active-handle inspection is insufficient.
## Required outcome
- Retain this reusable testing pitfall in durable project guidance or next to the lifecycle test.GPT Luna ｜ 𝕏
2 participants
Foreground
read_filepreviously read an entire file before returning a 200 KB prefix, whilegrepignored cancellation and could run JavaScript regexes on Electron's main thread.list_dirand no-match glob searches could enumerate entire trees despite output limits.This change bounds descriptor reads, directory enumeration, aggregate grep bytes, matching time and returned output. Cancellation and search deadlines settle callers even while filesystem I/O is pending. Four bounded operation owners retain admission until original I/O and cleanup settle; workers terminate immediately on abort and preserve native JavaScript regex behavior. Issued kernel syscalls cannot be cancelled, so stalled I/O retains its slot. Glob uses pinned minimatch segment parsing with Node-compatible traversal, preserving safe symlinks, mixed absolute/relative brace-expanded prefixes, Windows drive/UNC roots and glob-dependent
..paths. Parent hidden-file and secret policies remain distinct from subagent policy.Validation:
Tiny grep calls pay roughly 17 ms of worker startup versus sub-millisecond baseline calls in the quiet synthetic run. Full fixtures, raw before/after receipts, compatibility decisions and reproduction commands are in
docs/performance/foreground-file-tools-2026-09-28/.CI follow-up: reuse the exact stream-consumer gate correction from #278 / #280. The test now deliberately runs unrelated progress while the intended stream endpoint is held; reverting the exact endpoint to
/eventsreproduces a progress-observer timeout. All 208 iOS chat tests pass on the explicitly selected iPhone 17 Pro / iOS 27 simulator after the correction. Simulator results are regression evidence, not physical-device acceptance.The single permitted retry of CI run 36480769658 failed the separate
testCompletedUploadsRemainOwnedThroughRemovalAndExplicitCleanuptest. That failure remains unreproduced and unresolved: the unchanged local class also passed 208/208. This patch adds per-mode failure labels and preserves failed hosted XCTest result bundles for seven days, with CI policy coverage, while keeping all ownership assertions and timeouts. Both original failures and the later, separate simulator launch errors are recorded in the coordinating audit's papercuts.Integrated main through
ea65d03c3by ordinary merges, preserving troubleshooting records and both parents' test commands. The workspace bulk metadata FIFO budget and foreground-operation owners remain separate; independent integration review found no nested admission or shared release path. Workspace/Remote validation passed 43 tests with one existing platform skip after building its native helper. Integration validation passed 56 coding-tool/generation tests, 47 CI-policy tests, type-check, advisor source-drift coverage, the Electron bundle build, and 214 iOS chat tests. The Electron smoke now joins original owned operations between cancellation/deadline samples and retains late cleanup failures, matching the production contract where caller cancellation precedes disposal. It passes alongside local build/type-check activity with zero worker ports; a temporary rejection after real worker termination correctly fails the cleanup assertion. Independent Astra medium review cleared the integration and final smoke correction.