mirror of
https://github.com/deepseek-ai/deepseek-harness
synced 2026-08-15 21:04:50 +00:00
fix(fs-local): reject unsafe text observations
This commit is contained in:
@@ -121,21 +121,22 @@ The root plugin registers the full suite by composing the per-tool registration
|
||||
|
||||
## Migration plan
|
||||
|
||||
This RFC starts from `origin/master`, where no filesystem tool package exists yet. The final implementation should add the new three-package topology directly:
|
||||
This RFC starts from `origin/master`, where no filesystem tool package exists yet. The landed implementation adds the new three-package topology directly:
|
||||
|
||||
1. Add `packages/fs/fs` with the `ctx.fs` abstract service and vocabulary types.
|
||||
2. Add `packages/fs/fs-local` with the local backend implementation and backend-level tests.
|
||||
3. Add `packages/fs/tool-fs` with the model-facing `read`, `write`, and `edit` tools over `ctx.fs`.
|
||||
4. Wire examples by loading a `ctx.fs` provider first (`dsh-fs-local`), then the consumer (`dsh-tool-fs` or one of its subpath plugins).
|
||||
5. Update `docs/architecture.md`, `packages/README.md`, package READMEs, build/typecheck config, and aggregate maintenance scripts such as `scripts/publint-all.ts`.
|
||||
4. Update `docs/architecture.md`, `packages/README.md`, package READMEs, build/typecheck config, and aggregate maintenance scripts such as `scripts/publint-all.ts`.
|
||||
|
||||
This first pass does not add a separate `@deepseek-ai/dsh-file-context` package. The file-state store lives behind `ctx.fs` so root and subpath `tool-fs` plugins share the same read-before-write/edit policy automatically.
|
||||
|
||||
Example leaf configs stay bash-only in this landing. Wiring `examples/coding-agent` or `examples/acp-agent` to `dsh-fs-local` + `dsh-tool-fs` changes the model prompt, visible tool schemas, and ACP snapshot transcript, so it should land as a follow-up UX/example change with prompt and snapshot updates in the same PR.
|
||||
|
||||
If this work is split into multiple PRs, they should follow the seam order:
|
||||
|
||||
1. Interface PR: `dsh-fs` only, with service registration and contract tests.
|
||||
2. Implementation PR: `dsh-fs-local`, with real filesystem behavior tests.
|
||||
3. Consumer PR: `dsh-tool-fs`, examples, docs, and integration tests.
|
||||
3. Consumer PR: `dsh-tool-fs`, docs, and integration tests; example wiring follows in a separate prompt/snapshot PR.
|
||||
|
||||
The earlier combined package name `@deepseek-ai/dsh-fs-tools` should not become part of the new public surface.
|
||||
|
||||
@@ -159,7 +160,7 @@ Beyond the happy/sad paths above, `dsh-fs-local` tests must cover the defensive-
|
||||
|
||||
Integration tests should load `dsh-fs-local` plus `dsh-tool-fs` and execute `read`, `write`, and `edit` through `ctx.tools.execute()` to prove the three packages work together without bypassing the tool registry. They must verify the world, not the tool's self-report: after a `write`/`edit`, read the file back from disk and assert byte-identical content (and that untouched files are unchanged), rather than trusting the returned `ContentBlock[]`. Each integration/e2e test owns its resources — create the harness in the test, run against a per-test temporary directory, and dispose the harness and remove the directory in `afterEach` even on failure or timeout.
|
||||
|
||||
Repo gates for the implementation include the focused vitest suites, `yarn typecheck`, `yarn test:coverage` for runtime code, and build/publint coverage after adding package entrypoints.
|
||||
Repo gates for the implementation include the focused vitest suites, `pnpm run typecheck`, `pnpm run test:coverage` for runtime code, and build/publint coverage after adding package entrypoints.
|
||||
|
||||
## Risks
|
||||
|
||||
|
||||
@@ -11,8 +11,8 @@ await ctx.plugin(LocalFileSystem, { cwd: process.cwd() })
|
||||
|
||||
## Behavior
|
||||
|
||||
- **`resolve(path)`** — relative paths resolve from `config.cwd` (default `process.cwd()`). The `targetKey` is the file's `realpath`, so two input paths reaching the same file through symlinks share one identity, and writes/edits land on the link target (preserving the link). A not-yet-existing path keeps its absolute path as the key so creates still get a stable identity. `displayPath` is the absolute (un-resolved) path.
|
||||
- **`readPage`** — UTF-8 only. A fast path (`readFile`) handles files under `FAST_PATH_MAX_SIZE` (10 MB); larger files stream with a capped line buffer so a newline-free giant file can't exhaust memory. NUL-byte samples are rejected (`FS_NOT_TEXT`). Output is bounded to `READ_LIMIT` (2000) lines, `READ_MAX_BYTES` (50 KB), and `READ_MAX_LINE_LENGTH` (2000) chars per line. The `version` is `mtimeMs:size`.
|
||||
- **`resolve(path)`** — relative paths resolve from `config.cwd` (default `process.cwd()`). The `targetKey` is the file's `realpath`, so two input paths reaching the same file through symlinks share one identity, and writes/edits land on the link target (preserving the link). A not-yet-existing path uses the realpathed parent directory plus basename when the parent exists; only an unresolvable parent falls back to the absolute path. `displayPath` is the absolute (un-resolved) path.
|
||||
- **`readPage`** — UTF-8 only. A fast path (`readFile`) handles files under `FAST_PATH_MAX_SIZE` (10 MB); larger files stream with a capped line buffer so a newline-free giant file can't exhaust memory. Invalid UTF-8 and NUL-byte samples are rejected (`FS_NOT_TEXT`). Output is bounded to `READ_LIMIT` (2000) lines, `READ_MAX_BYTES` (50 KB), and `READ_MAX_LINE_LENGTH` (2000) chars per line; hitting any bound records a `partial` view. The `version` is `mtimeMs:size`.
|
||||
- **`createOrReplace`** — atomic: writes to a temp file opened exclusively (`wx`, `0o600`) inside a randomly-named private staging dir (`0o700`) next to the target, fsyncs, then renames over the target. An existing file's mode is preserved, while new files default to `0o600`. Honors the `FsExpectation`: an `observed` write must match the recorded version (else `FS_STALE_VERSION`); a `partial` write onto an existing file is rejected (`FS_PARTIAL_OBSERVATION`); an `unobserved` write onto an existing file is rejected (`FS_NOT_OBSERVED`).
|
||||
- **`applyEdit`** — atomic literal read-modify-write over the same primitive. Verifies the expected version, LF-normalizes for matching, restores the file's dominant CRLF/LF style, and rejects empty `oldString` / zero matches (`FS_EDIT_NOT_FOUND`) or ambiguous multi-matches without `replace_all` (`FS_AMBIGUOUS_EDIT`).
|
||||
|
||||
|
||||
@@ -6,8 +6,8 @@
|
||||
* The reader uses two code paths so a single huge line can never balloon
|
||||
* memory: a **fast path** (`readFile` + in-memory split) for files under
|
||||
* {@link FAST_PATH_MAX_SIZE}, and a **streaming path** (manual newline scan
|
||||
* with a capped line buffer) for larger files. Both reject NUL-byte binary
|
||||
* samples and keep only the requested page in memory.
|
||||
* with a capped line buffer) for larger files. Both reject invalid UTF-8 and
|
||||
* NUL-byte binary samples, and keep only the requested page in memory.
|
||||
*
|
||||
* Writes are atomic: content goes to a temp file opened exclusively (`wx`,
|
||||
* `0o600`, so a pre-existing path can never be clobbered and write-in-progress
|
||||
@@ -23,6 +23,7 @@ import { createReadStream } from 'node:fs'
|
||||
import { chmod, mkdir, open, readFile, realpath, rename, rm, stat } from 'node:fs/promises'
|
||||
import type { Stats } from 'node:fs'
|
||||
import { basename, dirname, join, resolve } from 'node:path'
|
||||
import { TextDecoder } from 'node:util'
|
||||
import { FsError } from '@deepseek-ai/dsh-fs'
|
||||
import type { FsReadRequest, FsTextLine, FsView } from '@deepseek-ai/dsh-fs'
|
||||
|
||||
@@ -41,7 +42,6 @@ export const FAST_PATH_MAX_SIZE = 10 * 1024 * 1024
|
||||
const READ_MAX_BYTES_LABEL = `${READ_MAX_BYTES / 1024} KB`
|
||||
const READ_MAX_LINE_SUFFIX = `... (line truncated to ${READ_MAX_LINE_LENGTH} chars)`
|
||||
const BINARY_SAMPLE_BYTES = 8192
|
||||
const NUL_CHAR = String.fromCharCode(0)
|
||||
const LINE_BUFFER_CAP = READ_MAX_LINE_LENGTH + 1
|
||||
|
||||
/**
|
||||
@@ -145,17 +145,18 @@ interface PageAccumulator {
|
||||
totalLines: number
|
||||
outputBytes: number
|
||||
truncatedByBytes: boolean
|
||||
truncatedByLine: boolean
|
||||
done: boolean
|
||||
}
|
||||
|
||||
function newAccumulator(): PageAccumulator {
|
||||
return { lines: [], totalLines: 0, outputBytes: 0, truncatedByBytes: false, done: false }
|
||||
return { lines: [], totalLines: 0, outputBytes: 0, truncatedByBytes: false, truncatedByLine: false, done: false }
|
||||
}
|
||||
|
||||
function truncateReadLine(line: string): string {
|
||||
function truncateReadLine(line: string): { text: string; truncated: boolean } {
|
||||
return line.length > READ_MAX_LINE_LENGTH
|
||||
? `${line.substring(0, READ_MAX_LINE_LENGTH)}${READ_MAX_LINE_SUFFIX}`
|
||||
: line
|
||||
? { text: `${line.substring(0, READ_MAX_LINE_LENGTH)}${READ_MAX_LINE_SUFFIX}`, truncated: true }
|
||||
: { text: line, truncated: false }
|
||||
}
|
||||
|
||||
function lineByteSize(line: string, currentLineCount: number): number {
|
||||
@@ -166,7 +167,8 @@ function consumeLine(acc: PageAccumulator, rawLine: string, request: FsReadReque
|
||||
acc.totalLines += 1
|
||||
if (acc.totalLines < request.offset || acc.lines.length >= request.limit) return
|
||||
|
||||
const text = truncateReadLine(rawLine)
|
||||
const { text, truncated } = truncateReadLine(rawLine)
|
||||
if (truncated) acc.truncatedByLine = true
|
||||
const bytes = lineByteSize(text, acc.lines.length)
|
||||
if (acc.outputBytes + bytes > READ_MAX_BYTES) {
|
||||
acc.truncatedByBytes = true
|
||||
@@ -195,13 +197,41 @@ function buildResult(acc: PageAccumulator, request: FsReadRequest, version: stri
|
||||
throw new FsError(`offset ${request.offset} is out of range for "${displayPath}" (${acc.totalLines} lines)`, 'FS_NOT_FOUND')
|
||||
}
|
||||
const endLine = acc.lines.at(-1)?.number ?? Math.max(0, request.offset - 1)
|
||||
const view: FsView = request.offset === 1 && !acc.truncatedByBytes && endLine >= acc.totalLines ? 'full' : 'partial'
|
||||
const view: FsView = request.offset === 1 && !acc.truncatedByBytes && !acc.truncatedByLine && endLine >= acc.totalLines ? 'full' : 'partial'
|
||||
return { lines: acc.lines, totalLines: acc.totalLines, truncatedByBytes: acc.truncatedByBytes, view, version }
|
||||
}
|
||||
|
||||
function notTextError(verb: 'read' | 'edit', displayPath: string): FsError {
|
||||
return new FsError(`cannot ${verb} "${displayPath}": invalid UTF-8 text`, 'FS_NOT_TEXT')
|
||||
}
|
||||
|
||||
function decodeUtf8(buffer: Uint8Array, verb: 'read' | 'edit', displayPath: string): string {
|
||||
try {
|
||||
return new TextDecoder('utf-8', { fatal: true }).decode(buffer)
|
||||
} catch (error: unknown) {
|
||||
if (error instanceof TypeError) throw notTextError(verb, displayPath)
|
||||
throw error
|
||||
}
|
||||
}
|
||||
|
||||
function decodeUtf8Stream(
|
||||
decoder: TextDecoder,
|
||||
chunk: Uint8Array | undefined,
|
||||
verb: 'read' | 'edit',
|
||||
displayPath: string,
|
||||
): string {
|
||||
try {
|
||||
return chunk ? decoder.decode(chunk, { stream: true }) : decoder.decode()
|
||||
} catch (error: unknown) {
|
||||
if (error instanceof TypeError) throw notTextError(verb, displayPath)
|
||||
throw error
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a bounded UTF-8 text-file page. Rejects non-regular files and NUL-byte
|
||||
* binary samples; dispatches to the fast or streaming path by file size.
|
||||
* Read a bounded UTF-8 text-file page. Rejects non-regular files, invalid
|
||||
* UTF-8, and NUL-byte binary samples; dispatches to the fast or streaming path
|
||||
* by file size.
|
||||
*/
|
||||
export async function readTextPage(
|
||||
target: LocalTarget,
|
||||
@@ -240,7 +270,7 @@ async function readTextPageFast(
|
||||
throw new FsError(`cannot read "${target.displayPath}": binary file`, 'FS_NOT_TEXT')
|
||||
}
|
||||
|
||||
const text = raw.toString('utf8')
|
||||
const text = decodeUtf8(raw, 'read', target.displayPath)
|
||||
const acc = newAccumulator()
|
||||
let startPos = 0
|
||||
let newlinePos: number
|
||||
@@ -261,10 +291,11 @@ async function readTextPageStreaming(
|
||||
version: string,
|
||||
signal?: AbortSignal,
|
||||
): Promise<ReadPageResult> {
|
||||
const stream = createReadStream(target.targetKey, { encoding: 'utf8', ...signal ? { signal } : {} })
|
||||
const stream = createReadStream(target.targetKey, signal ? { signal } : {})
|
||||
const acc = newAccumulator()
|
||||
let lineBuffer = ''
|
||||
let firstChunk = true
|
||||
let sampledBytes = 0
|
||||
const decoder = new TextDecoder('utf-8', { fatal: true })
|
||||
|
||||
function appendToLineBuffer(segment: string): void {
|
||||
if (lineBuffer.length >= LINE_BUFFER_CAP) return
|
||||
@@ -277,24 +308,36 @@ async function readTextPageStreaming(
|
||||
lineBuffer = ''
|
||||
}
|
||||
|
||||
try {
|
||||
for await (const chunk of stream as AsyncIterable<string>) {
|
||||
if (firstChunk) {
|
||||
firstChunk = false
|
||||
if (chunk.slice(0, BINARY_SAMPLE_BYTES).includes(NUL_CHAR)) {
|
||||
throw new FsError(`cannot read "${target.displayPath}": binary file`, 'FS_NOT_TEXT')
|
||||
}
|
||||
}
|
||||
let startPos = 0
|
||||
let newlinePos: number
|
||||
while ((newlinePos = chunk.indexOf('\n', startPos)) !== -1) {
|
||||
appendToLineBuffer(chunk.slice(startPos, newlinePos))
|
||||
flushLine()
|
||||
startPos = newlinePos + 1
|
||||
if (acc.done) return buildResult(acc, request, version, target.displayPath)
|
||||
}
|
||||
appendToLineBuffer(chunk.slice(startPos))
|
||||
function scanBinarySample(chunk: Buffer): void {
|
||||
if (sampledBytes >= BINARY_SAMPLE_BYTES) return
|
||||
const sample = chunk.subarray(0, Math.min(chunk.length, BINARY_SAMPLE_BYTES - sampledBytes))
|
||||
if (sample.includes(0)) {
|
||||
throw new FsError(`cannot read "${target.displayPath}": binary file`, 'FS_NOT_TEXT')
|
||||
}
|
||||
sampledBytes += sample.length
|
||||
}
|
||||
|
||||
function consumeChunk(chunk: string): ReadPageResult | undefined {
|
||||
let startPos = 0
|
||||
let newlinePos: number
|
||||
while ((newlinePos = chunk.indexOf('\n', startPos)) !== -1) {
|
||||
appendToLineBuffer(chunk.slice(startPos, newlinePos))
|
||||
flushLine()
|
||||
startPos = newlinePos + 1
|
||||
if (acc.done) return buildResult(acc, request, version, target.displayPath)
|
||||
}
|
||||
appendToLineBuffer(chunk.slice(startPos))
|
||||
return undefined
|
||||
}
|
||||
|
||||
try {
|
||||
for await (const chunk of stream as AsyncIterable<Buffer>) {
|
||||
scanBinarySample(chunk)
|
||||
const result = consumeChunk(decodeUtf8Stream(decoder, chunk, 'read', target.displayPath))
|
||||
if (result) return result
|
||||
}
|
||||
const finalResult = consumeChunk(decodeUtf8Stream(decoder, undefined, 'read', target.displayPath))
|
||||
if (finalResult) return finalResult
|
||||
} catch (error: unknown) {
|
||||
/* v8 ignore next 4 -- mid-stream errors need an abort/IO fault racing the loop; pre-abort is caught by throwIfAborted. */
|
||||
if (isAbortError(error)) throw new FsError('read aborted', 'FS_ABORTED')
|
||||
@@ -435,7 +478,7 @@ export async function readForEdit(
|
||||
const buffer = await readFile(absolutePath, signal ? { signal } : {})
|
||||
throwIfAborted(signal, 'edit')
|
||||
if (buffer.includes(0)) throw new FsError(`cannot edit "${displayPath}": binary file`, 'FS_NOT_TEXT')
|
||||
const raw = buffer.toString('utf8')
|
||||
const raw = decodeUtf8(buffer, 'edit', displayPath)
|
||||
return { content: normalizeLineEndings(raw), lineEndings: detectLineEndings(raw) }
|
||||
}
|
||||
|
||||
|
||||
@@ -9,7 +9,7 @@ import { mkdtemp, readFile, rm, stat, symlink, writeFile, unlink } from 'node:fs
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { Context } from 'cordis'
|
||||
import { LocalFileSystem } from '@deepseek-ai/dsh-fs-local'
|
||||
import { LocalFileSystem, probe } from '@deepseek-ai/dsh-fs-local'
|
||||
import type { FsExecContext } from '@deepseek-ai/dsh-fs'
|
||||
|
||||
let dir: string
|
||||
@@ -89,6 +89,19 @@ describe('read → write → edit lifecycle', () => {
|
||||
expect(outcome.view).toBe('partial')
|
||||
})
|
||||
|
||||
it('records an over-long-line read as partial, so write/edit stay blocked', async () => {
|
||||
await writeFile(join(dir, 'long.txt'), 'x'.repeat(3000))
|
||||
const owner = exec()
|
||||
const target = await fs.resolve('long.txt')
|
||||
const outcome = await fs.read(target, READ_ALL, owner)
|
||||
|
||||
expect(outcome.view).toBe('partial')
|
||||
await expect(fs.write(target, 'new', owner)).rejects.toMatchObject({ code: 'FS_PARTIAL_OBSERVATION' })
|
||||
await expect(
|
||||
fs.edit(target, { oldString: 'x', newString: 'y', replaceAll: false }, owner),
|
||||
).rejects.toMatchObject({ code: 'FS_PARTIAL_OBSERVATION' })
|
||||
})
|
||||
|
||||
it('allows a follow-up edit without re-reading (write/edit refresh state)', async () => {
|
||||
await writeFile(join(dir, 'a.txt'), 'a b')
|
||||
const owner = exec()
|
||||
@@ -142,6 +155,22 @@ describe('read-before-write policy', () => {
|
||||
await expect(fs.edit(target, { oldString: 'old', newString: 'new', replaceAll: false }, exec()))
|
||||
.rejects.toMatchObject({ code: 'FS_NOT_OBSERVED' })
|
||||
})
|
||||
|
||||
it('rejects invalid UTF-8 reads and edits without rewriting the file', async () => {
|
||||
const path = join(dir, 'invalid-utf8.txt')
|
||||
const bytes = Buffer.from([0x68, 0xff, 0x69])
|
||||
await writeFile(path, bytes)
|
||||
const owner = exec()
|
||||
const target = await fs.resolve('invalid-utf8.txt')
|
||||
|
||||
await expect(fs.read(target, READ_ALL, owner)).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
const existing = await probe(target.targetKey)
|
||||
if (!existing) throw new Error('expected invalid UTF-8 fixture to exist')
|
||||
await expect(
|
||||
fs.applyEdit(target, { oldString: 'h', newString: 'H', replaceAll: false }, { version: existing.version }),
|
||||
).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
expect(await readFile(path)).toEqual(bytes)
|
||||
})
|
||||
})
|
||||
|
||||
describe('stale-version guard + concurrency (defensive class B)', () => {
|
||||
|
||||
@@ -102,6 +102,7 @@ describe('readTextPage', () => {
|
||||
await writeFile(file, 'x'.repeat(3000))
|
||||
const result = await readTextPage(localTarget(file), READ_ALL)
|
||||
expect(result.lines[0]?.text).toContain('... (line truncated to 2000 chars)')
|
||||
expect(result.view).toBe('partial')
|
||||
})
|
||||
|
||||
it('caps output bytes and reports truncatedByBytes', async () => {
|
||||
@@ -141,6 +142,12 @@ describe('readTextPage', () => {
|
||||
await expect(readTextPage(localTarget(file), READ_ALL)).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
})
|
||||
|
||||
it('rejects invalid UTF-8 bytes (fast path)', async () => {
|
||||
const file = join(dir, 'invalid-utf8.txt')
|
||||
await writeFile(file, Buffer.from([0x68, 0xff, 0x69]))
|
||||
await expect(readTextPage(localTarget(file), READ_ALL)).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
})
|
||||
|
||||
it('rejects a missing file and a directory', async () => {
|
||||
await expect(readTextPage(localTarget(join(dir, 'nope')), READ_ALL)).rejects.toMatchObject({ code: 'FS_NOT_FOUND' })
|
||||
await expect(readTextPage(localTarget(dir), READ_ALL)).rejects.toMatchObject({ code: 'FS_NOT_REGULAR_FILE' })
|
||||
@@ -181,6 +188,13 @@ describe('readTextPage', () => {
|
||||
await writeFile(file, 'z'.repeat(5000))
|
||||
const result = await readTextPage(localTarget(file), READ_ALL, undefined, stream)
|
||||
expect(result.lines[0]?.text).toContain('... (line truncated to 2000 chars)')
|
||||
expect(result.view).toBe('partial')
|
||||
})
|
||||
|
||||
it('rejects invalid UTF-8 bytes on the streaming path', async () => {
|
||||
const file = join(dir, 'invalid-utf8.txt')
|
||||
await writeFile(file, Buffer.from([0x68, 0xff, 0x69]))
|
||||
await expect(readTextPage(localTarget(file), READ_ALL, undefined, stream)).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
})
|
||||
|
||||
it('honors abort on the streaming path', async () => {
|
||||
@@ -338,6 +352,12 @@ describe('readForEdit + restoreLineEndings', () => {
|
||||
await expect(readForEdit(file, file)).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
})
|
||||
|
||||
it('rejects invalid UTF-8 bytes', async () => {
|
||||
const file = join(dir, 'invalid-utf8.txt')
|
||||
await writeFile(file, Buffer.from([0x68, 0xff, 0x69]))
|
||||
await expect(readForEdit(file, file)).rejects.toMatchObject({ code: 'FS_NOT_TEXT' })
|
||||
})
|
||||
|
||||
it('passes a live (non-aborted) signal through the read', async () => {
|
||||
const file = join(dir, 'a.txt')
|
||||
await writeFile(file, 'one\ntwo')
|
||||
|
||||
Reference in New Issue
Block a user