fix(fs-search): address the second-round #1119 review
- inline the collect() identity wrapper now that both streams use the seam's diagnostic-tail shape - resolve the packaged rg path lazily at the first call (memoized): @vscode/ripgrep resolves its platform package at module evaluation, so a static import turned a missing/corrupt platform package into a Loader composition failure instead of the documented per-call SEARCH_FAILED - classify synchronous spawn-creation throws (a NUL in argv, an abort racing the pre-check, a rejected resolution) into SEARCH_FAILED / SEARCH_ABORTED instead of leaking raw errors - correct the stderrMaxBytes contract: the stderr excerpt is embedded in SEARCH_* error messages, not hidden from the model - export virtualManifest and pin its three acceptance paths (prefix hit, pnpm-11 truncated-name content-scan fallback, both miss) with fixture unit tests Tests: rg-path.spec.ts (resolution failure + memoized rejection), tools.spec.ts spawn-creation classification, notices spec virtualManifest.
This commit is contained in:
@@ -55,6 +55,7 @@ export {
|
||||
SEARCH_TIMEOUT_MS,
|
||||
SearchError,
|
||||
previewLine,
|
||||
resolveRgPath,
|
||||
runRipgrep,
|
||||
toWorkdirRelative,
|
||||
trySaveFormattedResult,
|
||||
@@ -83,7 +84,7 @@ export interface Config {
|
||||
rawOutputMaxBytes?: number
|
||||
/** Terminate-escalation grace period (ms) for one search process, handed to the subprocess seam. */
|
||||
graceMs?: number
|
||||
/** Max bytes retained for one search's stderr diagnostic tail (never surfaced to the model). */
|
||||
/** Max bytes retained for one search's stderr tail; the excerpt is embedded in `SEARCH_*` error messages, never shown on success. */
|
||||
stderrMaxBytes?: number
|
||||
/** Cooperative tool-call timeout budget (ms) on both tools, enforced by `@deepseek-ai/dsh-timeout-policy` through `exec.signal`. */
|
||||
timeoutMs?: number
|
||||
|
||||
@@ -21,11 +21,10 @@
|
||||
|
||||
import { isAbsolute, relative, sep } from 'node:path'
|
||||
import type { Context } from 'cordis'
|
||||
import { rgPath } from '@vscode/ripgrep'
|
||||
import { HarnessError } from '@deepseek-ai/dsh-llm'
|
||||
import { ItemRetainer, TextRetainer } from '@deepseek-ai/dsh-retention'
|
||||
import type { RetainedItems } from '@deepseek-ai/dsh-retention'
|
||||
import type { SubprocessCollect, SubprocessOutcome, SubprocessOutputRead, SubprocessSpawnSpec } from '@deepseek-ai/dsh-subprocess'
|
||||
import type { SubprocessHandle, SubprocessOutcome, SubprocessOutputRead, SubprocessSpawnSpec } from '@deepseek-ai/dsh-subprocess'
|
||||
import type { SaveTextSpill, SpillRef } from '@deepseek-ai/dsh-spill'
|
||||
import type { ToolExecution } from '@deepseek-ai/dsh-tools'
|
||||
|
||||
@@ -154,6 +153,26 @@ function completeStdout(toolName: string, stdout: SubprocessOutputRead, rawOutpu
|
||||
)
|
||||
}
|
||||
|
||||
let rgPathPromise: Promise<string> | undefined
|
||||
|
||||
/**
|
||||
* The packaged ripgrep binary path, resolved lazily once per process.
|
||||
*
|
||||
* `@vscode/ripgrep` resolves its platform package (`@vscode/ripgrep-<platform>
|
||||
* -<arch>`) at module evaluation, so a static import would turn a missing or
|
||||
* corrupt platform package (`pnpm install --omit=optional`, partial install)
|
||||
* into a failure of the whole Loader composition. Resolving at the call
|
||||
* boundary keeps that failure at the first search call as `SEARCH_FAILED` —
|
||||
* the package's documented no-load-time-probe contract.
|
||||
*
|
||||
* @returns the packaged binary's absolute path; the memoized promise rejects
|
||||
* when the platform package cannot be resolved.
|
||||
*/
|
||||
export function resolveRgPath(): Promise<string> {
|
||||
rgPathPromise ??= import('@vscode/ripgrep').then(module => module.rgPath)
|
||||
return rgPathPromise
|
||||
}
|
||||
|
||||
/**
|
||||
* Run the packaged ripgrep binary with a plain argv vector and return its
|
||||
* complete raw stdout. The working directory is the calling agent's session
|
||||
@@ -173,9 +192,12 @@ function completeStdout(toolName: string, stdout: SubprocessOutputRead, rawOutpu
|
||||
* success with zero results (`noMatches`), anything else throws a
|
||||
* {@link SearchError} (abort/timeout → `SEARCH_ABORTED`, invalid pattern →
|
||||
* `SEARCH_INVALID_PATTERN`, the rest → `SEARCH_FAILED` /
|
||||
* `SEARCH_RAW_OUTPUT_OVERFLOW`). A spawn REJECTION — the seam's
|
||||
* infrastructure failures — is translated into `SEARCH_FAILED` with the
|
||||
* original as `cause`; a pre-aborted signal becomes `SEARCH_ABORTED`.
|
||||
* `SEARCH_RAW_OUTPUT_OVERFLOW`). Both launch-time failure domains are
|
||||
* classified: a synchronous throw at spawn CREATION (a NUL in argv, an abort
|
||||
* racing the pre-check, a rejected `@vscode/ripgrep` resolution) and a
|
||||
* rejection of `handle.done` (the seam's infrastructure failures) both become
|
||||
* `SEARCH_FAILED` with the original as `cause` — an abort already observed by
|
||||
* creation time becomes `SEARCH_ABORTED` instead.
|
||||
*
|
||||
* @param ctx - the plugin context; execution uses its `subprocess` service.
|
||||
* @param exec - the tool-execution context; supplies the session cwd and the abort signal.
|
||||
@@ -200,19 +222,31 @@ export async function runRipgrep(
|
||||
}
|
||||
const cwd = exec.agent?.session.header.cwd
|
||||
const workdir = cwd ?? process.cwd()
|
||||
const collect = (maxBytes: number): SubprocessCollect =>
|
||||
({ maxBytes })
|
||||
const handle = ctx.subprocess.spawn({
|
||||
argv: [rgPath, '--no-config', ...argv],
|
||||
cwd: workdir,
|
||||
stdio: {
|
||||
stdin: 'ignore',
|
||||
stdout: collect(rawOutputMaxBytes),
|
||||
stderr: collect(stderrMaxBytes),
|
||||
},
|
||||
graceMs,
|
||||
signal: exec.signal,
|
||||
} satisfies SubprocessSpawnSpec)
|
||||
let handle: SubprocessHandle
|
||||
try {
|
||||
handle = ctx.subprocess.spawn({
|
||||
argv: [await resolveRgPath(), '--no-config', ...argv],
|
||||
cwd: workdir,
|
||||
stdio: {
|
||||
stdin: 'ignore',
|
||||
stdout: { maxBytes: rawOutputMaxBytes },
|
||||
stderr: { maxBytes: stderrMaxBytes },
|
||||
},
|
||||
graceMs,
|
||||
signal: exec.signal,
|
||||
} satisfies SubprocessSpawnSpec)
|
||||
} catch (error: unknown) {
|
||||
// Node's spawn() throws synchronously for a NUL in argv, and the local
|
||||
// impl can throw synchronously when the signal aborts between the check
|
||||
// above and this call (or when the platform-package resolution rejects).
|
||||
// The static narrowing that proves this re-check "always false" cannot
|
||||
// see AbortSignal state changes.
|
||||
// oxlint-disable-next-line typescript/no-unnecessary-condition
|
||||
if (exec.signal.aborted) {
|
||||
throw new SearchError(`${toolName} was aborted before completion (tool timeout or caller cancellation)`, 'SEARCH_ABORTED')
|
||||
}
|
||||
throw new SearchError(`${toolName} could not start its search command (ripgrep launch failed)`, 'SEARCH_FAILED', { cause: error })
|
||||
}
|
||||
let outcome: SubprocessOutcome
|
||||
try {
|
||||
outcome = await handle.done
|
||||
|
||||
37
packages/fs/tool-fs-search/tests/rg-path.spec.ts
Normal file
37
packages/fs/tool-fs-search/tests/rg-path.spec.ts
Normal file
@@ -0,0 +1,37 @@
|
||||
/**
|
||||
* Failure-path tests for the lazy packaged-ripgrep resolution. The success
|
||||
* path (the real `@vscode/ripgrep` module) is exercised throughout
|
||||
* tools.spec.ts; here the module is mocked to throw at evaluation, proving a
|
||||
* missing or corrupt platform package (`--omit=optional`, partial install)
|
||||
* surfaces as a per-call `SEARCH_FAILED` — not a composition-load failure.
|
||||
*/
|
||||
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { Context } from 'cordis'
|
||||
import { CallId } from '@deepseek-ai/dsh-llm'
|
||||
import type { ToolExecution } from '@deepseek-ai/dsh-tools'
|
||||
import { resolveRgPath, runRipgrep } from '@deepseek-ai/dsh-tool-fs-search'
|
||||
|
||||
// Any access to the mocked module's surface throws — the shape a missing
|
||||
// platform package produces at module evaluation.
|
||||
vi.mock('@vscode/ripgrep', () => new Proxy({}, {
|
||||
get() {
|
||||
throw new Error('platform package @vscode/ripgrep-win32-x64 is not installed')
|
||||
},
|
||||
}))
|
||||
|
||||
describe('lazy packaged-ripgrep resolution', () => {
|
||||
it('fails the first search call with SEARCH_FAILED instead of failing module load', async () => {
|
||||
// The resolution rejects before any spawn, so no subprocess service is needed.
|
||||
const controller = new AbortController()
|
||||
const exec = { signal: controller.signal, name: 'glob', callId: CallId('missing-platform-package') } as unknown as ToolExecution
|
||||
|
||||
await expect(runRipgrep(new Context(), exec, 'glob', ['--files'], 1_000_000, 3_000, 64 * 1024))
|
||||
.rejects.toMatchObject({ name: 'SearchError', code: 'SEARCH_FAILED' })
|
||||
})
|
||||
|
||||
it('keeps failing every subsequent call (the resolution is memoized)', async () => {
|
||||
await expect(resolveRgPath()).rejects.toThrow(/platform package/)
|
||||
await expect(resolveRgPath()).rejects.toThrow(/platform package/)
|
||||
})
|
||||
})
|
||||
@@ -32,6 +32,7 @@ import {
|
||||
presentGrepCall,
|
||||
presentGrepResult,
|
||||
previewLine,
|
||||
resolveRgPath,
|
||||
runRipgrep,
|
||||
sampleAcrossTopLevel,
|
||||
toWorkdirRelative,
|
||||
@@ -468,6 +469,48 @@ describe('workdir derivation and signal forwarding', () => {
|
||||
expect(text(result)).toContain('could not start')
|
||||
})
|
||||
|
||||
it('classifies a synchronous spawn-creation throw as SEARCH_FAILED', async () => {
|
||||
// Node's spawn() throws synchronously for a NUL in argv, and the local
|
||||
// impl can throw synchronously for other invalid specs. Creation-time
|
||||
// failures must join the error vocabulary instead of escaping raw.
|
||||
const { ctx, subprocess } = await setup()
|
||||
subprocess.handler = () => { throw new Error('spawn ERR_INVALID_ARG_VALUE') }
|
||||
|
||||
const result = await call(ctx, 'grep', { pattern: 'x' })
|
||||
|
||||
expect(result.isError).toBe(true)
|
||||
expect(result.error).toMatchObject({ info: { name: 'SearchError', code: 'SEARCH_FAILED' } })
|
||||
expect(text(result)).toContain('could not start')
|
||||
})
|
||||
|
||||
it('classifies a synchronous spawn-creation throw after an abort as SEARCH_ABORTED', async () => {
|
||||
// The local impl can throw synchronously when the signal aborts between
|
||||
// the pre-spawn check and the spawn call; no process was launched, so the
|
||||
// abort is the reportable cause.
|
||||
const { ctx, subprocess } = await setup()
|
||||
const controller = new AbortController()
|
||||
subprocess.handler = () => {
|
||||
controller.abort('timeout')
|
||||
throw new Error('aborted during spawn')
|
||||
}
|
||||
|
||||
const result = await call(ctx, 'glob', { pattern: '*' }, { signal: controller.signal })
|
||||
|
||||
expect(result.isError).toBe(true)
|
||||
expect(result.error).toMatchObject({ info: { name: 'SearchError', code: 'SEARCH_ABORTED' } })
|
||||
expect(text(result)).toContain('aborted before completion')
|
||||
})
|
||||
|
||||
it('resolves the packaged ripgrep path lazily, once per process', async () => {
|
||||
// The module must not touch @vscode/ripgrep at load (a missing platform
|
||||
// package would otherwise fail the whole composition), and repeated
|
||||
// resolution reuses the first result. The resolution-failure path is
|
||||
// pinned separately in rg-path.spec.ts.
|
||||
await setup()
|
||||
expect(await resolveRgPath()).toBe(rgPath)
|
||||
expect(resolveRgPath()).toBe(resolveRgPath())
|
||||
})
|
||||
|
||||
it('rejects when the subprocess implementation drops a requested collect stream', async () => {
|
||||
const { ctx, subprocess } = await setup()
|
||||
subprocess.dropReaders = true
|
||||
|
||||
Reference in New Issue
Block a user