fix(tool-fs): CRLF-safe write diff, opaque meta, doc sync
Address the applied-hunk-diffs review: - CRLF write overwrite emitted bogus every-line-changed hunks: write's `before` was LF-normalized but `after` kept the raw model content, so a CRLF rewrite of an LF file diffed every line. Normalize write's `after` to LF so both sides share the diff basis (edit already did). Regression test proves it fails on the raw-after path. - The tool-private `meta` payload is now typed `unknown` (opaque) at every seam instead of `JsonValue`. This drops the `dsh-tools -> dsh-session` package edge that existed only to name the type, and removes the `FileDiff` index signature that had been widening the type solely for JsonValue-assignability. Serializability is still enforced at runtime by `Session.append`'s isJsonValue check, which was always the real guard. - Sync the docs the new result/meta surface left stale: ToolResultView's diff card + ToolExecutionResult.meta in tools.md/session.md type-equiv blocks, the acp/tools READMEs, and the adding-a-tool cookbook; regenerate the cordis catalog and module graph.
This commit is contained in:
@@ -429,4 +429,4 @@ export function applyLiteralEdit(
|
||||
return { content: content.split(oldNorm).join(newNorm), replacements }
|
||||
}
|
||||
|
||||
export { restoreLineEndings }
|
||||
export { normalizeLineEndings, restoreLineEndings }
|
||||
|
||||
@@ -26,6 +26,7 @@ import type {
|
||||
} from '@deepseek-ai/dsh-fs'
|
||||
import {
|
||||
applyLiteralEdit,
|
||||
normalizeLineEndings,
|
||||
probe,
|
||||
readForEdit,
|
||||
readTextForDiff,
|
||||
@@ -156,7 +157,10 @@ export class LocalFileSystem extends FileSystem {
|
||||
operation: existing ? 'update' : 'create',
|
||||
version: this.versionAfterWrite(after, target),
|
||||
before,
|
||||
after: content,
|
||||
// LF-normalized to share the diff basis with `before` (also LF): a CRLF
|
||||
// overwrite must not read as every line changed. Line-ending restoration
|
||||
// is a storage detail the applied-hunk diff ignores.
|
||||
after: normalizeLineEndings(content),
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -203,11 +203,15 @@ describe('writeText', () => {
|
||||
expect(outcome.after).toBe('new body')
|
||||
})
|
||||
|
||||
it('an overwrite of a CRLF file returns LF-normalized before content', async () => {
|
||||
await writeFile(join(dir, 'a.txt'), 'a\r\nb\r\n')
|
||||
it('an overwrite returns LF-normalized before AND after (a CRLF rewrite is not every-line-changed)', async () => {
|
||||
// The applied-hunk diff bases on `before`/`after`; if `after` kept CRLF while
|
||||
// `before` is LF-normalized, a CRLF rewrite would read as every line changed.
|
||||
// Both sides are LF so only the genuinely-changed line diffs.
|
||||
await writeFile(join(dir, 'a.txt'), 'a\r\nb\r\nc\r\n')
|
||||
const target = await fs.resolve('a.txt')
|
||||
const outcome = await fs.writeText(target, 'a\nB\n')
|
||||
expect(outcome.before).toBe('a\nb\n')
|
||||
const outcome = await fs.writeText(target, 'a\r\nB\r\nc\r\n')
|
||||
expect(outcome.before).toBe('a\nb\nc\n')
|
||||
expect(outcome.after).toBe('a\nB\nc\n')
|
||||
})
|
||||
|
||||
it('an overwrite of a BINARY prior file reports before:null (undiffable), still succeeds', async () => {
|
||||
|
||||
@@ -103,11 +103,11 @@ export interface FsWriteOutcome {
|
||||
version: FsVersion
|
||||
/**
|
||||
* The file's content BEFORE the write, or `null` when the file did not exist
|
||||
* (a create). Raw storage text (LF-normalized by the backend), never a diff —
|
||||
* a consumer computes the result-time contextual diff from `before`/`after`.
|
||||
* (a create). LF-normalized storage text (the diff basis), never a diff — a
|
||||
* consumer computes the result-time contextual diff from `before`/`after`.
|
||||
*/
|
||||
before: string | null
|
||||
/** The file's content AFTER the write (the text that was written). */
|
||||
/** The file's content AFTER the write, LF-normalized to share `before`'s diff basis. */
|
||||
after: string
|
||||
}
|
||||
|
||||
|
||||
@@ -14,17 +14,17 @@
|
||||
|
||||
import { structuredPatch } from 'diff'
|
||||
import type { FileDiff } from '@deepseek-ai/dsh-tools'
|
||||
import type { JsonValue } from '@deepseek-ai/dsh-session'
|
||||
|
||||
/** Context lines shown on each side of an applied hunk (matches claude-agent-acp). */
|
||||
export const DIFF_CONTEXT = 3
|
||||
|
||||
/**
|
||||
* The `write`/`edit` tools' private `tool/result` `meta` payload: the applied
|
||||
* contextual-diff hunks. A {@link JsonValue} (persisted with the session log, so
|
||||
* `presentResult` reproduces the diff card on replay). The producing tool owns
|
||||
* this shape; the bridge only sees the opaque `meta` and the tool narrows it back
|
||||
* via {@link diffsFromMeta}.
|
||||
* contextual-diff hunks. Attached opaquely (as `unknown`) on the tool result and
|
||||
* persisted with the session log — it must be JSON-serializable (the session
|
||||
* validates this at `append`), so `presentResult` reproduces the diff card on
|
||||
* replay. The producing tool owns this shape; the bridge only sees the opaque
|
||||
* `meta` and the tool narrows it back via {@link diffsFromMeta}.
|
||||
*/
|
||||
export type FsDiffMeta = { diffs: FileDiff[] }
|
||||
|
||||
@@ -68,9 +68,9 @@ export function computeHunkDiffs(path: string, before: string, after: string): F
|
||||
}
|
||||
|
||||
/** Whether `value` is a valid {@link FileDiff} (defensive narrowing from opaque `meta`). */
|
||||
function isFileDiff(value: JsonValue): value is FileDiff & JsonValue {
|
||||
function isFileDiff(value: unknown): value is FileDiff {
|
||||
if (typeof value !== 'object' || value === null || Array.isArray(value)) return false
|
||||
const { path, oldText, newText } = value
|
||||
const { path, oldText, newText } = value as Record<string, unknown>
|
||||
return typeof path === 'string'
|
||||
&& (oldText === null || typeof oldText === 'string')
|
||||
&& typeof newText === 'string'
|
||||
@@ -83,9 +83,9 @@ function isFileDiff(value: JsonValue): value is FileDiff & JsonValue {
|
||||
* it validates defensively rather than trusting the payload — a bad `meta` yields
|
||||
* no diff card (the generic result rendering) instead of a thrown presenter.
|
||||
*/
|
||||
export function diffsFromMeta(meta: JsonValue | undefined): FileDiff[] | undefined {
|
||||
export function diffsFromMeta(meta: unknown): FileDiff[] | undefined {
|
||||
if (typeof meta !== 'object' || meta === null || Array.isArray(meta)) return undefined
|
||||
const diffs = meta.diffs
|
||||
const diffs = (meta as Record<string, unknown>).diffs
|
||||
if (!Array.isArray(diffs) || diffs.length === 0 || !diffs.every(isFileDiff)) return undefined
|
||||
return diffs
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user