Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesBounded subprocess acquisition
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
WalkthroughBefore this change, subprocess-backed tools such as grep, ripgrep, and DiscoveredTool collected child-process output without hard limits: stdout/stderr could grow unbounded, truncation was not surfaced consistently, and cancellation did not reliably terminate the whole process tree. After this PR, output acquisition is bounded by semantic budgets and retention state, ripgrep results are parsed with incremental UTF-8 framing and truncation-aware formatting, DiscoveredTool enforces abort-aware process-tree termination while exposing truncation metadata, and a shared pure TypeScript fallback path plus utilities for line framing, subprocess settlement, and native ripgrep resolution make the behavior consistent across tools. Release NotesBug Fixes
New Features
Refactor
Tests
Documentation
Changes
Magnitude🎯 3 (L) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tools/src/tools/grep.ts (1)
217-235: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
maxFiles - filesWithMatches.sizecan go negative across directories.The loop guards
maxResultsat line 218, somaxResults - allMatches.lengthstays positive. There is no equivalent guard for files.filesWithMatchesaccumulates across every directory, so after the first directory it can already holdmaxFilesentries or more, and later iterations pass a zero or negativemaxFilestoperformGrepSearch.Two failures follow:
tryRetainGrepMatchinpackages/tools/src/tools/grep/search-strategies.tsevaluatesstate.filesSeen.size > state.limits.maxFilesas true on the first match and setsearlyStopped, so every remaining directory is reported as incomplete.applyLimitscallsArray.from(filesWithMatches.entries()).slice(0, maxFiles). A negative second argument makesslicecount from the end, so it returns the wrong subset of files instead of an empty list.🐛 Proposed fix
for (const searchDir of searchDirectories) { if (allMatches.length >= maxResults) { wasLimited = true; break; } + const remainingFiles = maxFiles - filesWithMatches.size; + if (remainingFiles <= 0) { + wasLimited = true; + break; + } const matches = await performGrepSearch( { pattern: this.params.pattern, path: searchDir, include: this.params.include, signal: combinedSignal, maxResults: maxResults - allMatches.length, - maxFiles: maxFiles - filesWithMatches.size, + maxFiles: remainingFiles, maxPerFile, semanticBudget: aggregateBudget, }, this.fileExclusions, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/grep.ts` around lines 217 - 235, Prevent the per-directory grep request from receiving a negative file limit: update the loop around performGrepSearch to stop or handle the search when filesWithMatches.size has reached maxFiles, and ensure the remaining maxFiles value passed to performGrepSearch is never negative. Preserve the existing maxResults limiting behavior and ensure downstream applyLimits receives a non-negative limit.
🧹 Nitpick comments (23)
packages/tools/src/utils/lineFramer.ts (2)
90-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
segmentLen === 0test.
spaceis never negative, sosegmentLen <= spacealready coverssegmentLen === 0. The extra term adds no behavior.♻️ Proposed refactor
- if (segmentLen === 0 || segmentLen <= space) { + if (segmentLen <= space) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/lineFramer.ts` around lines 90 - 97, In the line-framing logic, simplify the conditional after calculating segmentLen by removing the redundant segmentLen === 0 disjunct and retain the existing segmentLen <= space behavior and processing.
26-29: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse one
TextDecoderinstance.
decodeFatalbuilds a newTextDecoderfor every emitted line. In the ripgrep path (packages/tools/src/tools/ripGrep.tslines 226-243) this runs once per match line. A module-level decoder is safe here because each call decodes a complete buffer without{ stream: true }, so no state carries over.♻️ Proposed refactor
-function decodeFatal(bytes: Uint8Array): string { - const decoder = new TextDecoder('utf-8', { fatal: true }); - return decoder.decode(bytes); -} +const fatalDecoder = new TextDecoder('utf-8', { fatal: true }); + +function decodeFatal(bytes: Uint8Array): string { + return fatalDecoder.decode(bytes); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/lineFramer.ts` around lines 26 - 29, Move the fatal UTF-8 TextDecoder construction out of decodeFatal and reuse a single module-level decoder instance. Keep decodeFatal decoding each complete Uint8Array without streaming so behavior and error handling remain unchanged.packages/tools/src/utils/lineFramer.test.ts (1)
184-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for
reset().
BoundedLineFramer.reset()is public and clearslength,discarding, anddroppedLine. No test exercises it. A test that drops a line, callsreset(), then assertswasLineDropped === falseand that framing restarts cleanly would close this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/lineFramer.test.ts` around lines 184 - 218, Add a test for BoundedLineFramer.reset() that first drops an invalid line, calls reset(), verifies wasLineDropped is false, and confirms subsequent valid input is framed and emitted normally from a clean state.packages/tools/src/utils/processTermination.ts (2)
136-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
gracePeriodMsat the resolution boundary.
gracePeriodMsis accepted without validation. A negative orNaNvalue makeswaitForGroupGoneandwaitForChildExitskip the grace window, so termination escalates toSIGKILLimmediately and silently. Reject non-finite and non-positive values instead.🛡️ Proposed fix
const signal = options?.signal ?? defaultSignal; - const gracePeriod = options?.gracePeriodMs ?? DEFAULT_TERMINATION_GRACE_MS; + const gracePeriod = options?.gracePeriodMs ?? DEFAULT_TERMINATION_GRACE_MS; + if (!Number.isFinite(gracePeriod) || gracePeriod <= 0) { + throw new RangeError( + `gracePeriodMs must be a finite number greater than zero, got: ${String(gracePeriod)}`, + ); + }Based on learnings: when resolving shell or task timeouts in
packages/tools/src, validate configured values at the resolution boundary and throw on invalid values rather than coercing them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/processTermination.ts` around lines 136 - 137, Validate options?.gracePeriodMs when resolving gracePeriod in the termination utility, rejecting values that are non-finite or non-positive by throwing an error; continue using DEFAULT_TERMINATION_GRACE_MS when the option is absent.Source: Learnings
115-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoalescing ignores the options of later callers.
activeTerminationskeys only on theChildProcess. If a second caller passes a differentgracePeriodMsor a differentownsProcessGroup, it silently receives the outcome computed with the first caller's options. Inpackages/tools/src/tools/tool-registry.tslines 180-205 both call sites use identical options, so no defect exists today. Add a short comment that states the coalescing contract, so future callers do not assume per-call options apply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/processTermination.ts` around lines 115 - 124, Add a brief comment immediately above the activeTerminations lookup in the termination coalescing flow, documenting that concurrent calls for the same ChildProcess share the first call’s doTerminate options, including gracePeriodMs and ownsProcessGroup. Do not change the existing coalescing behavior.packages/tools/src/tools/tool-registry.ts (1)
284-331: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTruncation is reported only in the text, not in structured metadata.
buildChildProcessResultappendstruncationNoticeto the content strings.ToolResultsupports an optionalmetadatafield. Consumers that need a machine-readable truncation signal must parse prose. Copyacquisition.metadataintoToolResult.metadataso callers can detect truncation without string matching.♻️ Proposed refactor
return { llmContent, returnDisplay: llmContent, + metadata: { acquisition: acquisition.metadata }, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/tool-registry.ts` around lines 284 - 331, Update buildChildProcessResult to include acquisition.metadata in the metadata field of every returned ToolResult, including both the execution-error and successful branches. Preserve the existing content formatting while exposing the structured truncation information directly to callers.packages/tools/src/utils/subprocessSettle.ts (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the catch path with the settled flag.
The catch block sets
settlement.settled = trueand callsrejectwithout checking whether another path already settled. With a native promise the extrarejectis a no-op, so no defect exists today. If a caller ever supplies arejectwith side effects, such as logging or metrics, the failure is reported twice. A guard makes the ownership rule explicit.♻️ Proposed refactor
} catch (err) { + if (settlement.settled) return; settlement.settled = true; abortSignal.removeEventListener('abort', abortHandlerRef.handler); reject(err instanceof Error ? err : new Error(String(err))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/subprocessSettle.ts` around lines 50 - 54, Update the catch path in subprocess settlement to check settlement.settled before performing cleanup, marking the settlement, and invoking reject. Return without action when another path has already settled, while preserving the existing error normalization for the first rejection.packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.ts (3)
455-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe conditional assertion weakens this test.
The test passes for both an error result and a success result. It cannot detect a regression that flips the outcome. Make the script deterministic, then assert one expected outcome.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.ts` around lines 455 - 464, Update the test around the result assertions to make the invoked script produce a deterministic error outcome, then assert that result.error is defined and validate its message and exit-code content directly. Remove the conditional success branch so the test fails if the outcome unexpectedly changes.
470-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the collector tests to an acquisition test file.
These two tests exercise
BoundedCombinedCollectordirectly and do not involveDiscoveredTool. They belong beside the otherpackages/tools/src/acquisitiontests, so the discovered-tool file stays focused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.ts` around lines 470 - 499, Move the “retains bounded head and tail from one huge chunk” and “retains all bytes when chunk fits within budget” tests out of the discovered-tool test suite into the existing acquisition test file alongside the other BoundedCombinedCollector tests. Preserve their assertions and setup unchanged, and keep the discovered-tool file focused on DiscoveredTool behavior.
113-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exported omission notice in the bounded-acquisition tests.
Import
DEFAULT_OMISSION_NOTICEfrom../acquisition/index.jsand replace all five hardcoded'truncated'assertions withtoContain(DEFAULT_OMISSION_NOTICE).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.ts` around lines 113 - 115, Update the bounded-acquisition tests to import DEFAULT_OMISSION_NOTICE from ../acquisition/index.js, then replace all five hardcoded 'truncated' assertions with checks for DEFAULT_OMISSION_NOTICE.packages/tools/src/utils/subprocessSettle.test.ts (1)
67-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the remaining termination outcomes.
The suite covers only
outcome: 'failure'. Two branches stay untested:outcome: 'timeout', which must also reject, and a benign outcome such as'graceful'or'no_target', which must still runaction(). Two short tests close the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/subprocessSettle.test.ts` around lines 67 - 81, Add tests alongside the existing termination-failure case in the subprocess settlement suite, covering a timeout result that rejects with the termination-failure error and a benign graceful or no_target result that executes action(). Use createSettleFn and the existing spies/context setup, asserting both rejection behavior and that the action is or is not called as appropriate.packages/tools/src/tools/ripGrep.ts (2)
46-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the exported budget constants instead of redefining them.
packages/tools/src/tools/grep/search-strategies.tsalready exportsMATCH_OVERHEAD_BYTES(line 242),HARD_RETAINED_MATCH_CAP(line 243), andcreateAggregateSemanticBudget(line 245). This file redefines the same values and function. The two copies can drift.Move the shared definitions into one module and import them here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/ripGrep.ts` around lines 46 - 59, Remove the local MATCH_OVERHEAD_BYTES, HARD_RETAINED_MATCH_CAP, and createAggregateSemanticBudget definitions from ripGrep.ts, and import the exported symbols from search-strategies.ts instead. Keep RipgrepSemanticBudget usage compatible with the shared createAggregateSemanticBudget implementation.
378-390: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttach
errorlisteners to the child streams.
stopProcesskills the process tree while stdout and stderr are still piped. A killed pipe can emit anerrorevent onchild.stdoutorchild.stderr. A streamerrorevent without a listener throws and is not caught by thechild.on('error')handler.Add no-op or logging listeners so a terminated read pipe cannot produce an unhandled error.
♻️ Proposed change
child.stderr.on('data', (chunk: Buffer) => { state.collector.append(chunk, 'stderr'); }); + child.stdout.on('error', () => {}); + child.stderr.on('error', () => {});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/ripGrep.ts` around lines 378 - 390, Update the child process stream setup near processRipgrepStdoutChunk to attach error listeners to both child.stdout and child.stderr, using no-op or appropriate logging handlers. Keep the existing data handling and child.on('error') rejection behavior unchanged, ensuring errors from terminated pipes cannot remain unhandled.packages/tools/src/utils/ripgrepPathResolver.ts (1)
97-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGate the
PATHEXTlogic onisWindows.
findInPathreadsPATHEXTand builds the extension list before it consultsisWindows. On POSIX,PATHEXTis normally unset, sorawExtsbecomes['', '.EXE']and the function probesdir/rg.EXEin everyPATHentry. That adds needlessstatSynccalls, and on a case-insensitive macOS volume it can match an unrelatedrg.exe.Build the extension list only when
isWindowsistrue.♻️ Proposed change
- const pathEnv = process.env.PATH ?? ''; - const pathExt = process.env.PATHEXT ?? ''; - const rawExts = pathExt - ? ['', ...pathExt.split(path.delimiter).filter((e) => e.length > 0)] - : ['', '.EXE']; + const pathEnv = process.env.PATH ?? ''; + const pathExt = process.env.PATHEXT ?? ''; + const rawExts = !isWindows + ? [''] + : pathExt + ? ['', ...pathExt.split(path.delimiter).filter((e) => e.length > 0)] + : ['', '.EXE'];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/utils/ripgrepPathResolver.ts` around lines 97 - 112, Update findInPath so PATHEXT is read and extension candidates are constructed only when isWindows is true; on POSIX, use only the bare binary name and avoid adding the .EXE fallback. Preserve the existing Windows extension deduplication behavior.packages/tools/src/tools/grep/search-strategies.ts (1)
474-483: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRaw stdout truncation marks complete searches as incomplete.
budgetTruncatedusesacquisition.metadata.truncated, which reports that theBoundedCombinedCollectordropped raw bytes past the 4 MiB budget. Parsing is incremental, so a search can retain every match it needs and still settruncatedbecause the raw stdout exceeded the collector budget. The result is then reported as "results may be incomplete" even whenmaxResults,maxFiles, andmaxPerFilewere all respected and the producer exited normally.The collector is only needed for stderr diagnostics and byte accounting on this path. Consider deriving incompleteness from the semantic state alone (
earlyStopped,budgetExhausted,lineDropped), or appending stdout bytes to a stderr-only collector so stdout volume no longer degrades the reported exactness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/grep/search-strategies.ts` around lines 474 - 483, The result assembly around state.collector.getResult() incorrectly treats raw stdout truncation as semantic incompleteness. Update budgetTruncated to derive only from semantic state—earlyStopped, budgetExhausted, and lineDropped—while preserving acquisition for stderr diagnostics and byte accounting.packages/tools/src/tools/grep.ts (2)
808-827: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
validateFinitePositiveinstead of duplicating the numeric rules.This loop repeats the exact checks in
validateFinitePositiveat lines 56-65, including the error text. The two copies can diverge. They already differ in one respect: this loop rejects values above the hard cap, whilevalidateFinitePositiveclamps them. Extract a single predicate that both callers share and keep the reject-versus-clamp difference explicit at each call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/grep.ts` around lines 808 - 827, The validation loop in the grep tool duplicates the numeric checks from validateFinitePositive. Extract or reuse a shared finite-positive-integer predicate for both call sites, while keeping the loop’s hard-cap behavior as rejection and validateFinitePositive’s behavior as clamping explicit at their respective call sites; preserve the existing error text.
206-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
totalObservedCountis computed and plumbed but never used.
collectDirectoryMatchesaccumulatestotalObservedCount,executeDirectorySearchdestructures it at line 581, andbuildDirectorySearchResultaccepts it at line 417. No code reads the value. The accumulation logic at lines 245-251 is therefore dead.Either surface the value in the incomplete branch of
formatMatchOutput(for example "at least N matches observed"), or remove the field and the parameter. The lower bound carries real information for the incomplete case, so using it is the better option.Also applies to: 273-279, 417-417
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/grep.ts` around lines 206 - 215, Use totalObservedCount in the incomplete-result path of formatMatchOutput to communicate the observed lower-bound match count, preserving the existing exact-count output for complete searches. Keep the field accumulation in collectDirectoryMatches, destructuring in executeDirectorySearch, and parameter in buildDirectorySearchResult, updating the output formatting so incomplete results clearly indicate “at least” the observed count.packages/tools/src/tools/grep/types.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider defining
SemanticBudgetintypes.ts.
types.tsis the leaf contract module, but it now imports fromsearch-strategies.js, which itself importsSearchResultsandSearchOptionsfromtypes.js. The cycle is type-only, so it is erased at build time and is not a runtime defect. Moving theSemanticBudgetinterface intotypes.tsand re-exporting it fromsearch-strategies.tsremoves the cycle and keeps the dependency direction one-way.♻️ Proposed change
In
packages/tools/src/tools/grep/types.ts:-import type { SemanticBudget } from './search-strategies.js'; +export interface SemanticBudget { + remainingBytes: number; + remainingObjects: number; +}In
packages/tools/src/tools/grep/search-strategies.ts:-export interface SemanticBudget { - remainingBytes: number; - remainingObjects: number; -} +export type { SemanticBudget } from './types.js';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/grep/types.ts` at line 1, Move the SemanticBudget interface definition from search-strategies.ts into the leaf contract module types.ts, then re-export SemanticBudget from search-strategies.ts so existing consumers retain the same import path while types.ts no longer imports search-strategies.ts.packages/tools/src/__tests__/grep-ripgrep-bounded-acquisition.test.ts (2)
293-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe abort test depends on a 10 ms race.
The test aborts 10 ms after
executeGrepstarts and then requires the output to match/cancel|abort/i. Ifgit grepcompletes the 20,000 matching lines before the abort fires, the tool returns results and the assertion fails. Fast machines and warm page cache make this reachable.Make the cancellation deterministic. Abort as soon as the first output arrives, or use a signal that is already aborted at a known point in the pipeline, instead of a fixed wall-clock delay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-bounded-acquisition.test.ts` around lines 293 - 324, Make the abort behavior in the executeGrep test deterministic by removing the fixed 10 ms timeout race. Trigger controller.abort() when the first subprocess output is observed, or provide an already-aborted signal at a reliable pipeline point, while preserving assertions that cancellation is returned without invoking the system-grep or JavaScript fallback paths.
623-623: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion always passes.
result.error !== undefined || textevaluates totruewhenevererroris set, and to the string otherwise.toBeTruthytherefore cannot fail unlesstextis empty. The same line is repeated at lines 636, 649, and 662. Assert the intended condition directly.♻️ Proposed change
- expect(result.error !== undefined || text).toBeTruthy(); + expect(text.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-bounded-acquisition.test.ts` at line 623, Update the repeated assertions in the bounded-acquisition tests to assert the intended result directly instead of relying on the always-truthy `result.error !== undefined || text` expression. Apply the same correction to all four occurrences, preserving validation that either an error is present or text was returned.packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts (3)
18-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared test helpers.
createTempDir,initGitRepo,gitAdd,createToolHost,executeGrep, andexecuteRipgrepare duplicated almost verbatim inpackages/tools/src/__tests__/grep-ripgrep-bounded-acquisition.test.ts(lines 18-113). The only differences are the default temp-directory prefix and the optionalsignalparameter onexecuteGrep. Move them into a shared test helper module so the tool host contract stays in one place whenIToolHostchanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts` around lines 18 - 110, Extract createTempDir, initGitRepo, gitAdd, createToolHost, executeGrep, and executeRipgrep into a shared test-helper module reused by both remediation and bounded-acquisition tests. Preserve each test’s temp-directory prefix and executeGrep optional signal behavior through configurable parameters, and remove the duplicated helper implementations while keeping the existing helper contracts and behavior unchanged.
314-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test does not verify that later spawns are skipped.
The test is named "does not spawn ripgrep for later directories after budget exhaustion", but it only inspects
llmContent. Both a correct implementation and one that spawns ripgrep for all six directories produce the same incomplete wording, so the assertions pass either way.Count the spawns to verify the stated behavior. Record the search directories that ripgrep actually receives, or assert on an observable side effect such as elapsed time or a per-directory marker, so a regression that keeps spawning fails the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts` around lines 314 - 346, Strengthen the test around executeRipgrep so it observes which directories actually trigger ripgrep after the search budget is exhausted. Instrument or spy on the ripgrep spawn boundary, record each received directory, and assert that later directories are not spawned while preserving the existing incomplete-result assertions.
195-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe rollback test cannot fail.
The test is named "git grep failure restores budget for system grep", but it never forces a
git grepfailure.initGitRepoandgitAddrun, sogit grepsucceeds and the fallback path is never taken.The two assertions are
budget.remainingBytes <= initialBytesandbudget.remainingObjects <= initialObjects. The budget only ever decreases, so both hold unconditionally, including ifrestoreBudgetwere deleted fromperformGrepSearch.Force the git strategy to return
null(for example, use a non-git directory soisGitRepositoryis false, or use anincludepattern with brace expansion sohasBracePatternshort-circuits it), then assert the exact remaining byte count against the value a single system-grep pass consumes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts` around lines 195 - 234, The test around “git grep failure restores budget for system grep” does not exercise the fallback or verify rollback. Update the setup to force the git strategy to return null, then assert budget.remainingBytes equals the expected post-system-grep value based on one pass (and likewise verify the exact object count if applicable), rather than only asserting values are less than or equal to their initial amounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/src/tools/grep/javascriptFallback.ts`:
- Around line 92-101: The JavaScript fallback must consume the shared semantic
budget. In packages/tools/src/tools/grep/javascriptFallback.ts:92-101, add a
SemanticBudget parameter to javascriptGrepFallback and apply
tryRetainGrepMatch’s per-match byte and object accounting, stopping and marking
results incomplete when exhausted; in
packages/tools/src/tools/grep/search-strategies.ts:885-894, pass the shared
semanticBudget into the fallback call.
- Around line 115-151: Update the loop around processFallbackFile in
javascriptFallback.ts to track a resultsLimitHit flag when breaking because
maxResults has been reached, then include that flag in incomplete and wasLimited
alongside filesLimitHit. In
packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts lines
149-170, consider adding a direct javascriptGrepFallback assertion that a
maxResults break sets incomplete to true.
In `@packages/tools/src/tools/ripGrep.ts`:
- Around line 282-291: Derive the result object's budgetTruncated field solely
from the semantic state.budgetExhausted flag, removing
acquisition.metadata.truncated from that status while retaining collector
acquisition for stderr text. Update the multi-root stop logic around
formatDirectoryResults to stop only for semantic budget exhaustion, so raw
collector byte truncation—including stderr-only overflow—does not mark results
incomplete or prevent later roots from being searched.
In `@packages/tools/src/tools/tool-registry.ts`:
- Around line 196-205: Update awaitProcessSettlement to report whether
settlement occurred via the STREAM_DRAIN_TIMEOUT_MS drain timer, then use that
signal in the termination flow around terminationPromise to call
terminateProcessTree for owned detached process groups whenever the drain timer
fired, even if child.exitCode is already set. Preserve the existing termination
behavior for leaders that remain alive and propagate the resulting
terminationOutcome.
In `@packages/tools/src/utils/processTermination.test.ts`:
- Around line 39-66: Gate the two graceful-exit tests in
packages/tools/src/utils/processTermination.test.ts (lines 39-66) with
it.skipIf(process.platform === 'win32'). Also gate all three coalescing tests
that call spawnSleeper in packages/tools/src/utils/processTermination.test.ts
(lines 255-319) using the same platform condition.
In `@packages/tools/src/utils/processTermination.ts`:
- Around line 246-305: Update terminateWindowsTree’s onClose handler to map
taskkill exit code 128 to the no_target outcome, while preserving graceful for
code 0 and failure for all other codes. Add or update coverage for this race
case so the returned ProcessTerminationResult reports no_target when taskkill
exits with 128.
---
Outside diff comments:
In `@packages/tools/src/tools/grep.ts`:
- Around line 217-235: Prevent the per-directory grep request from receiving a
negative file limit: update the loop around performGrepSearch to stop or handle
the search when filesWithMatches.size has reached maxFiles, and ensure the
remaining maxFiles value passed to performGrepSearch is never negative. Preserve
the existing maxResults limiting behavior and ensure downstream applyLimits
receives a non-negative limit.
---
Nitpick comments:
In `@packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.ts`:
- Around line 455-464: Update the test around the result assertions to make the
invoked script produce a deterministic error outcome, then assert that
result.error is defined and validate its message and exit-code content directly.
Remove the conditional success branch so the test fails if the outcome
unexpectedly changes.
- Around line 470-499: Move the “retains bounded head and tail from one huge
chunk” and “retains all bytes when chunk fits within budget” tests out of the
discovered-tool test suite into the existing acquisition test file alongside the
other BoundedCombinedCollector tests. Preserve their assertions and setup
unchanged, and keep the discovered-tool file focused on DiscoveredTool behavior.
- Around line 113-115: Update the bounded-acquisition tests to import
DEFAULT_OMISSION_NOTICE from ../acquisition/index.js, then replace all five
hardcoded 'truncated' assertions with checks for DEFAULT_OMISSION_NOTICE.
In `@packages/tools/src/__tests__/grep-ripgrep-bounded-acquisition.test.ts`:
- Around line 293-324: Make the abort behavior in the executeGrep test
deterministic by removing the fixed 10 ms timeout race. Trigger
controller.abort() when the first subprocess output is observed, or provide an
already-aborted signal at a reliable pipeline point, while preserving assertions
that cancellation is returned without invoking the system-grep or JavaScript
fallback paths.
- Line 623: Update the repeated assertions in the bounded-acquisition tests to
assert the intended result directly instead of relying on the always-truthy
`result.error !== undefined || text` expression. Apply the same correction to
all four occurrences, preserving validation that either an error is present or
text was returned.
In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts`:
- Around line 18-110: Extract createTempDir, initGitRepo, gitAdd,
createToolHost, executeGrep, and executeRipgrep into a shared test-helper module
reused by both remediation and bounded-acquisition tests. Preserve each test’s
temp-directory prefix and executeGrep optional signal behavior through
configurable parameters, and remove the duplicated helper implementations while
keeping the existing helper contracts and behavior unchanged.
- Around line 314-346: Strengthen the test around executeRipgrep so it observes
which directories actually trigger ripgrep after the search budget is exhausted.
Instrument or spy on the ripgrep spawn boundary, record each received directory,
and assert that later directories are not spawned while preserving the existing
incomplete-result assertions.
- Around line 195-234: The test around “git grep failure restores budget for
system grep” does not exercise the fallback or verify rollback. Update the setup
to force the git strategy to return null, then assert budget.remainingBytes
equals the expected post-system-grep value based on one pass (and likewise
verify the exact object count if applicable), rather than only asserting values
are less than or equal to their initial amounts.
In `@packages/tools/src/tools/grep.ts`:
- Around line 808-827: The validation loop in the grep tool duplicates the
numeric checks from validateFinitePositive. Extract or reuse a shared
finite-positive-integer predicate for both call sites, while keeping the loop’s
hard-cap behavior as rejection and validateFinitePositive’s behavior as clamping
explicit at their respective call sites; preserve the existing error text.
- Around line 206-215: Use totalObservedCount in the incomplete-result path of
formatMatchOutput to communicate the observed lower-bound match count,
preserving the existing exact-count output for complete searches. Keep the field
accumulation in collectDirectoryMatches, destructuring in
executeDirectorySearch, and parameter in buildDirectorySearchResult, updating
the output formatting so incomplete results clearly indicate “at least” the
observed count.
In `@packages/tools/src/tools/grep/search-strategies.ts`:
- Around line 474-483: The result assembly around state.collector.getResult()
incorrectly treats raw stdout truncation as semantic incompleteness. Update
budgetTruncated to derive only from semantic state—earlyStopped,
budgetExhausted, and lineDropped—while preserving acquisition for stderr
diagnostics and byte accounting.
In `@packages/tools/src/tools/grep/types.ts`:
- Line 1: Move the SemanticBudget interface definition from search-strategies.ts
into the leaf contract module types.ts, then re-export SemanticBudget from
search-strategies.ts so existing consumers retain the same import path while
types.ts no longer imports search-strategies.ts.
In `@packages/tools/src/tools/ripGrep.ts`:
- Around line 46-59: Remove the local MATCH_OVERHEAD_BYTES,
HARD_RETAINED_MATCH_CAP, and createAggregateSemanticBudget definitions from
ripGrep.ts, and import the exported symbols from search-strategies.ts instead.
Keep RipgrepSemanticBudget usage compatible with the shared
createAggregateSemanticBudget implementation.
- Around line 378-390: Update the child process stream setup near
processRipgrepStdoutChunk to attach error listeners to both child.stdout and
child.stderr, using no-op or appropriate logging handlers. Keep the existing
data handling and child.on('error') rejection behavior unchanged, ensuring
errors from terminated pipes cannot remain unhandled.
In `@packages/tools/src/tools/tool-registry.ts`:
- Around line 284-331: Update buildChildProcessResult to include
acquisition.metadata in the metadata field of every returned ToolResult,
including both the execution-error and successful branches. Preserve the
existing content formatting while exposing the structured truncation information
directly to callers.
In `@packages/tools/src/utils/lineFramer.test.ts`:
- Around line 184-218: Add a test for BoundedLineFramer.reset() that first drops
an invalid line, calls reset(), verifies wasLineDropped is false, and confirms
subsequent valid input is framed and emitted normally from a clean state.
In `@packages/tools/src/utils/lineFramer.ts`:
- Around line 90-97: In the line-framing logic, simplify the conditional after
calculating segmentLen by removing the redundant segmentLen === 0 disjunct and
retain the existing segmentLen <= space behavior and processing.
- Around line 26-29: Move the fatal UTF-8 TextDecoder construction out of
decodeFatal and reuse a single module-level decoder instance. Keep decodeFatal
decoding each complete Uint8Array without streaming so behavior and error
handling remain unchanged.
In `@packages/tools/src/utils/processTermination.ts`:
- Around line 136-137: Validate options?.gracePeriodMs when resolving
gracePeriod in the termination utility, rejecting values that are non-finite or
non-positive by throwing an error; continue using DEFAULT_TERMINATION_GRACE_MS
when the option is absent.
- Around line 115-124: Add a brief comment immediately above the
activeTerminations lookup in the termination coalescing flow, documenting that
concurrent calls for the same ChildProcess share the first call’s doTerminate
options, including gracePeriodMs and ownsProcessGroup. Do not change the
existing coalescing behavior.
In `@packages/tools/src/utils/ripgrepPathResolver.ts`:
- Around line 97-112: Update findInPath so PATHEXT is read and extension
candidates are constructed only when isWindows is true; on POSIX, use only the
bare binary name and avoid adding the .EXE fallback. Preserve the existing
Windows extension deduplication behavior.
In `@packages/tools/src/utils/subprocessSettle.test.ts`:
- Around line 67-81: Add tests alongside the existing termination-failure case
in the subprocess settlement suite, covering a timeout result that rejects with
the termination-failure error and a benign graceful or no_target result that
executes action(). Use createSettleFn and the existing spies/context setup,
asserting both rejection behavior and that the action is or is not called as
appropriate.
In `@packages/tools/src/utils/subprocessSettle.ts`:
- Around line 50-54: Update the catch path in subprocess settlement to check
settlement.settled before performing cleanup, marking the settlement, and
invoking reject. Return without action when another path has already settled,
while preserving the existing error normalization for the first rejection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7ad45f8-81cc-4903-8bdb-f852dad83755
⛔ Files ignored due to path filters (1)
project-plans/issue3203/PLAN.mdis excluded by!project-plans/**
📒 Files selected for processing (19)
packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.tspackages/tools/src/__tests__/grep-ripgrep-bounded-acquisition.test.tspackages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.tspackages/tools/src/index.tspackages/tools/src/tools/grep.tspackages/tools/src/tools/grep/javascriptFallback.tspackages/tools/src/tools/grep/ripgrepParse.tspackages/tools/src/tools/grep/search-strategies.tspackages/tools/src/tools/grep/types.tspackages/tools/src/tools/ripGrep.tspackages/tools/src/tools/tool-registry.tspackages/tools/src/utils/lineFramer.test.tspackages/tools/src/utils/lineFramer.tspackages/tools/src/utils/processTermination.test.tspackages/tools/src/utils/processTermination.tspackages/tools/src/utils/ripgrepPathResolver.test.tspackages/tools/src/utils/ripgrepPathResolver.tspackages/tools/src/utils/subprocessSettle.test.tspackages/tools/src/utils/subprocessSettle.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts (1)
202-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
remainingObjectsassertion couples the test to the selected strategy.Line 247 asserts
budget.remainingObjectsis exactly90. The value holds only if the strategy that succeeds retains exactly 10 matches and the failed git-grep consumption is fully rolled back. If a host resolves agrepthat emits a different match count, or the JavaScript fallback runs with different file discovery, the assertion becomes brittle for a reason unrelated to rollback. Consider asserting the rollback property directly, for example thatremainingObjectsequals100 - result.results.length.♻️ Proposed assertion change
- expect(budget.remainingObjects).toBe(90); + expect(budget.remainingObjects).toBe(100 - result.results.length);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts` around lines 202 - 256, Update the remainingObjects assertion in the performGrepSearch test to derive the expected value from result.results.length, asserting it equals the initial budget of 100 minus the number of returned results. Keep the remainingBytes and result completeness assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts`:
- Around line 202-256: Update the remainingObjects assertion in the
performGrepSearch test to derive the expected value from result.results.length,
asserting it equals the initial budget of 100 minus the number of returned
results. Keep the remainingBytes and result completeness assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89e5333a-d276-493f-b813-2580132476bd
📒 Files selected for processing (13)
packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.tspackages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.tspackages/tools/src/__tests__/grep-ripgrep-raw-truncation.test.tspackages/tools/src/tools/grep.tspackages/tools/src/tools/grep/grepBudget.tspackages/tools/src/tools/grep/javascriptFallback.tspackages/tools/src/tools/grep/ripgrepParse.tspackages/tools/src/tools/grep/search-strategies.tspackages/tools/src/tools/grep/types.tspackages/tools/src/tools/ripGrep.tspackages/tools/src/tools/tool-registry.tspackages/tools/src/utils/processTermination.test.tspackages/tools/src/utils/processTermination.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/tools/src/tools/grep/types.ts
- packages/tools/src/utils/processTermination.test.ts
- packages/tools/src/tools/tool-registry.ts
- packages/tools/src/utils/processTermination.ts
- packages/tools/src/tools/grep.ts
|
Final bounded-acquisition remediation is pushed as commit 319498f. Correctness coverage in this commit:
Verification on the pushed change:
A concurrent build/typecheck attempt raced over generated ide-integration declarations; the required sequential reruns both passed. Open Code Review covered all 11 changed files including tests. Its actionable test-cleanup and assertion findings were addressed before commit; the suggestion to catch an impossible internal termination rejection was not adopted because terminateProcessTree's contract resolves ProcessTerminationResult outcomes and this codebase prefers fixing contract violations rather than swallowing them. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/tools/src/__tests__/grep-ephemeral-precedence.test.ts (1)
152-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the hard-cap assertion observable.
The fixture has only 10 matches. A regression that uses
500_000directly instead of applying the hard cap still passes this test.Assert the resolved limit through a test seam, or use input that distinguishes the hard cap from the configured value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ephemeral-precedence.test.ts` around lines 152 - 161, The test “hard-cap: ephemeral above cap does not crash and stays bounded” does not distinguish applying MAX_RESULTS_HARD_CAP from using the configured value because the fixture contains only 10 matches. Update this test to use a fixture or observable test seam that verifies the resolved limit is capped at 100,000, while preserving the no-error assertion.packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts (1)
593-624: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that later roots are not searched after the cap is filled.
Both tests only assert incomplete output. That output is also valid if the tool searches the later root and observes its extra match. Add a stable per-root invocation observation and assert that the later root is not started.
packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts#L593-L624: assert that grep does not start a search forrootB.packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts#L661-L692: assert that ripgrep does not start a search forrootB.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts` around lines 593 - 624, The cap tests only validate incomplete output, not that later roots are skipped. In packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts lines 593-624, add stable per-root invocation tracking around executeGrep and assert grep never starts searching rootB; apply the equivalent observation and assertion to the ripgrep test at lines 661-692. Preserve the existing output assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tools/src/__tests__/grep-ephemeral-precedence.test.ts`:
- Around line 152-161: The test “hard-cap: ephemeral above cap does not crash
and stays bounded” does not distinguish applying MAX_RESULTS_HARD_CAP from using
the configured value because the fixture contains only 10 matches. Update this
test to use a fixture or observable test seam that verifies the resolved limit
is capped at 100,000, while preserving the no-error assertion.
In `@packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts`:
- Around line 593-624: The cap tests only validate incomplete output, not that
later roots are skipped. In
packages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.ts lines
593-624, add stable per-root invocation tracking around executeGrep and assert
grep never starts searching rootB; apply the equivalent observation and
assertion to the ripgrep test at lines 661-692. Preserve the existing output
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf10e386-daa3-4e4c-a6fc-a93e4c089338
📒 Files selected for processing (10)
packages/tools/src/__tests__/discovered-tool-bounded-acquisition.test.tspackages/tools/src/__tests__/grep-ephemeral-precedence.test.tspackages/tools/src/__tests__/grep-ripgrep-issue3203-remediation.test.tspackages/tools/src/__tests__/grep-ripgrep-raw-truncation.test.tspackages/tools/src/tools/grep.tspackages/tools/src/tools/grep/search-strategies.tspackages/tools/src/tools/ripGrep.tspackages/tools/src/tools/tool-registry.tspackages/tools/src/utils/ripgrepPathResolver.test.tspackages/tools/src/utils/ripgrepPathResolver.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/tools/src/utils/ripgrepPathResolver.test.ts
- packages/tools/src/utils/ripgrepPathResolver.ts
- packages/tools/src/tools/tool-registry.ts
- packages/tools/src/tools/grep.ts
- packages/tools/src/tools/ripGrep.ts
- packages/tools/src/tools/grep/search-strategies.ts
|
CI note: the current Lint (GitHub Actions) failure occurred before lint execution while downloading ShellCheck from github.com. curl exited 60 because the hosted runner observed a self-signed certificate. The job checked out merge commit 5463c1e and did not inspect any PR source. I will rerun the failed job after the workflow completes. |
|
Final CI status: all required checks now pass on commit 319498f. The failed GitHub Actions lint setup was rerun after the workflow completed and passed, confirming the earlier ShellCheck certificate error was transient runner infrastructure. The Windows installed-command behavioral smoke also passed. CodeRabbit, LLxprt review, CodeQL, all lint/test/build smoke jobs, Linux E2E in both sandbox modes, and Windows coverage are green. There are no actionable review threads, and the local branch remains clean and synchronized with origin/issue3203. |
1 participant
TLDR
Bounds subprocess output while it is being acquired by discovered tools, grep, and ripgrep so large producer streams cannot exhaust memory before result limiting runs. Partial output is now labeled explicitly, search subprocesses stop only after omission is proven, and cancellation terminates owned process trees with bounded escalation.
Dive Deeper
This change reuses the public acquisition primitives introduced by #3200 and keeps process lifecycle policy local to packages/tools.
The implementation includes behavioral coverage for huge and tiny chunks, split multibyte sequences, invalid UTF-8, lone CR and CRLF boundaries, exact and exceeded limits, aggregate multi-root budgets, fallback budget rollback, stderr-only truncation, process-group descendants, Windows taskkill outcomes, and settlement races.
Reviewer Test Plan
Run the complete tools suite:
Run static verification:
Exercise search behavior with a synthetic workspace containing:
Exercise a discovered tool that writes far beyond the output budget to both stdout and stderr. Confirm retained output is bounded and all result surfaces identify truncation consistently.
Cancel a discovered tool whose child ignores TERM and retains inherited pipes. Confirm the invocation settles and no owned descendants survive.
Local verification on macOS:
Testing Matrix
Linked issues / bugs
Fixes #3203
Parent: #3202
Depends on the acquisition contract delivered by #3200.
Summary by CodeRabbit