Codex review of the vocabulary relocation found two doc-accuracy issues:
- presentation.ts's JSDoc used {@link ToolDefinition...}, which the
TypeScript language service cannot resolve because presentation.ts
deliberately does not import index.ts (that would create the cycle the
split avoids). Demote those three to plain `ToolDefinition` code text;
same-file and imported @links (TerminalResultView, ContentBlock) stay.
- docs/core-data-structures/tools.md's source header listed only index.ts
and schema.ts; add presentation.ts, which now owns the presentation
vocabulary the page documents.
The tool render-intent vocabulary (ToolCallView/ToolResultView + members,
FileLocation, FileDiff, ToolCallKind) is the UI-facing surface of
dsh-tools; it lived inline in index.ts alongside the registry and
execution core. Move it to its own presentation.ts module so index.ts is
the registry + execute waterfall and the presentation vocabulary is a
separate, one-directional dependency.
presentation.ts owns ONLY render-intent types and references none of the
execution types; index.ts imports the view types for ToolDefinition's
presentCall/presentResult signatures (clean acyclic index -> presentation).
The opaque `meta` presentation channel (ToolExecuteReturn, ToolResult,
ToolExecutionResult) is execution plumbing and stays in index.ts.
Public surface unchanged: index.ts re-exports the vocabulary, so consumers
(tool-fs/tool-bash/tool-web/tool-todo, the ACP bridge) keep importing from
@deepseek-ai/dsh-tools with zero churn. No producer/bridge/test edits; a
pure internal relocation with no observable-output change (snapshot goldens
untouched).
The result-time diff card sent view.title raw, so a completed edit/write
of an absolute in-workspace path flipped the card header back from the
relativized `Edit src/b.ts` to the absolute path — the pending card
relativizes, the result did not, and tool_call_update.title replaces the
header. Apply displayTitle to the result diff arm using the diff path,
mirroring the call-side card. Regression test proven red on the unfixed
arm.
Also record the overwrite diff-basis pre-read as a bounded follow-up
(TODO(overwrite-diff-bound) + RFC non-goal): overwriting a large file
reads the whole prior text into memory for a UI-only diff.
master's web_search/web_fetch tools were authored against the old
ToolCallPresentation bag; the render-intent union replaces it with a
card-tagged discriminated union. Both are simple generic cards, so they
declare card:'generic' explicitly.
Three comments still implied the result diff is always an applied hunk or
that write returns undefined on no-hunk: the toolResultUpdate JSDoc (a diff
result "emits the applied-hunk blocks, which replace the call-time snippet"),
the empty-diffs test comment ("an empty write returns undefined" — write now
falls back to a whole-file diff), and diffsFromMeta's JSDoc ("a bad meta
yields no diff card" — only true for edit; write falls back to a whole-file
diff). Each now states the write whole-file fallback. Regenerate the catalog.
Codex's stale-prose pass found seven more spots still describing the
result diff as ALWAYS an applied contextual hunk, or a create/binary
overwrite as rendering "only the call-time card": the DiffCallView JSDoc
and the acp bridge diff-arm comment, the FsWriteOutcome.before and
readTextForDiff JSDoc, and three RFC lines. All now say: the result diff
is the applied change — a contextual hunk when there is a before-image,
else a whole-file diff (create / undiffable binary) — and a successful
mutation always returns the result diff so the model-facing text can't
clobber it. Regenerate the cordis catalog (source line shift).
The writeText comment still said a null `before` (a create or an
undiffable binary file) means "a consumer renders no result-time diff,
only the call-time whole-file card." That is stale since write's
presentResult renders a whole-file diff for a null before-image. Correct
it: a null `before` gives no contextual-hunk basis, so the consumer falls
back to a whole-file diff — the tool still renders a result diff card, not
the raw result text.
The acp README's presentResult card list still described the `diff` result
as always "the APPLIED hunks computed from before/after". Qualify it like
the other surfaces: typically the applied hunks, or a whole-file diff for a
create, and a successful mutation always returns it so the result text
can't clobber the diff.
The write-diff-card fix made write's presentResult return an args-derived
whole-file diff (oldText:null) for a create / unchanged overwrite, but the
DiffResultView contract and its mirrored docs still said `diffs` is ALWAYS
the applied contextual hunks computed from before/after. Correct the type
JSDoc, the write execute-side comment, and the four mirrored surfaces
(tools.md, tools README, acp-feature-support, adding-a-tool cookbook) to
say: typically the applied hunks, or a whole-file diff when there is no
before-image (a create) — and that a mutation returns the diff result even
when it duplicates the call-time card, since a tool_call_update.content
replace would otherwise clobber the diff with the model-facing text.
Regenerate the cordis catalog (source line shift).
A Write CREATE rendered its completed tool_call_update as the model-facing
result TEXT (`<path>…</path>…Created file`), which — because an ACP
tool_call_update.content REPLACES the call's content — clobbered the
new-file diff the pending call installed. So Zed showed the diff, then
replaced it with raw XML-ish text; only overwrite/edit looked right
(their result re-sends a diff).
write's presentResult now ALWAYS returns a diff card for a successful
write: the applied contextual hunk from `meta` when there is one
(overwrite), else an args-derived whole-file diff (`oldText: null`) for a
create or an unchanged-content overwrite. This matches claude-agent-acp,
where the create diff rides on the update and no result text replaces it.
An error still falls through to generic rendering so its message shows.
edit is unchanged (it always has a hunk; no whole-file fallback).
Re-recorded fs-write / fs-write-overwrite goldens; the create's completed
update is now a {type:'diff'} block, not the XML result text.
Same stale enumeration as the presenter-note fix, in the ToolPresenter
JSDoc: it said the tool/result event "carries only { callId, content,
isError }". The event also carries error and meta; the load-bearing fact
is that it omits the tool name/args (why the presenter remembers them per
callId). State that instead of an exhaustive list that drifts.
The callId→args map note said the tool/result event "carries only
{ callId, content, isError }" — an exhaustive field list that drifts as
the event grows (it also carries error, and now meta). State the load-
bearing fact instead: the event omits the tool name/args, which is why
the bridge remembers them per callId.
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.
fs write/edit now emit a result-time contextual-diff tool_call_update
(the applied hunk with ±3 context lines, one hunk per replace_all site),
matching what claude-agent-acp sends and what makes an editor render the
change in place. The call-time snippet diff stays; the result hunk
supersedes it (ACP content-replace).
Mechanism:
- A persisted tool-private `meta` channel: execute may return
`{ content, meta }`; `meta` (JsonValue) rides on the tool/result event
and is handed back to presentResult, so the diff reproduces on replay
(event-sourced). JsonValue is now exported from dsh-session.
- The backend returns raw before/after text (storage facts) on
FsWriteOutcome/FsEditOutcome; the tool computes the hunk via the npm
`diff` package's structuredPatch. A create has no before → no result
diff; a failed/aborted mutation carries no meta.
- ToolResultView gains a DiffResultView; the bridge's result-side switch
renders it as {type:'diff'} content blocks.
RFC: docs/rfc/implemented/architecture/2026-07-02-result-time-applied-hunk-diffs.md
(justifies the npm `diff` runtime dep over vendoring and the meta channel);
the render-intent-union RFC's Non-goal is updated to record this shipped.
All fs snapshot goldens re-recorded; edit/overwrite gain the contextual
result diff, create/read/policy-reject unchanged in structure.
The numResults (exa) and searchRecency (perplexity) conditional spreads
in apply() were only exercised on their absent side, leaving the 100%
per-file branch gate red. Add plugin-registration tests that pass those
config fields and assert they reach the request body.
The Exa and Perplexity providers hard-coded request parameters that
deployments should control while defaults are still unsettled. Exa gains
searchType, numResults, and highlightsPerResult; Perplexity gains
maxTokens (it previously sent none) and an optional searchRecency. Each
follows the deepseek provider's shape: a defaulted Config field, a
DEFAULT_* constant, and a positive-integer status() check for numeric
limits. The call-level maxResults still flows through WebSearchRequest
and wins over the configured default, keeping the seam layering intact.
Addresses tianyicui's "make everything configurable" review comment.
The closed WebErrorCode union leaked fetch-transport details (redirect,
too-large, content-type) into the seam's shared vocabulary and made web
the only seam with a closed error-code union. Drop it and let WebError
carry an open code: string like LlmError/SubagentError; document the
codes grouped by owner (seam-neutral vs dsh-web-fetch-local transport).
Addresses tianyicui's leaky-abstraction review comment on WebErrorCode.
Address the render-intent-union review:
- toolResultUpdate branched on `if (card === 'terminal')` with a generic
fallthrough; ToolResultView is a closed union, so make it an exhaustive
`switch (view.card)` ending in assertNever (matching the call-side
renderer and the § Conventions closed-union rule). Adding a result card
later now fails to compile at the switch. Regression test: a rogue result
card throws.
- displayTitle's `rel.startsWith('..')` guard mis-rejected an in-workspace
target whose relative form merely begins with the chars `..` (e.g.
`..cache/x`, a real sibling name), leaving its title absolute. Test for a
`..` SEGMENT (`..` alone or `..<sep>…`) so such paths relativize, matching
claude-agent-acp's `cwd + sep` prefix check. Regression test added.
Replace the "bag of optional fields" tool-presentation types
(ToolCallPresentation / ToolResultPresentation / ToolTerminal) with a
card-tagged discriminated union — the standing FIXME(tool-presentation).
A tool declares one render intent per call/result and the ACP bridge
switches on `card`:
ToolCallView = generic | terminal | diff
ToolResultView = generic | terminal
The `diff` card is new: fs write/edit now emit an ACP {type:'diff'}
content block (an editor's inline diff), which the old shapes could not
express. The bridge also relativizes a file card's title against the
session cwd (mirroring claude-agent-acp's toDisplayPath) while keeping
locations/diff paths raw, and derives the no-capability fenced console
fallback from a terminal result's output. read gains the window-in-title
(`Read foo.txt (5 - 8)`) and an always-set location line, matching the
reference adapter field-for-field.
Migrates all three producer families (tool-fs, tool-bash, tool-todo) and
the sole consumer (the ACP bridge) together — the source does not compile
piecewise. Adds snapshot coverage for the terminal _meta path (a new
capability-advertising scenario) and re-records the fs goldens to show the
diff cards. Applied-hunk (result-time, context-line) diffs need a new
result/event shape and are a follow-up.
RFC: docs/rfc/implemented/architecture/2026-07-02-tool-render-intent-union.md
Behavioral gaps from the coverage audit (line coverage was already 100%; these
close BEHAVIOR gaps):
- fs-local: service-level writeText/editText pre-abort → FS_ABORTED (file
unchanged); concurrent guarded-write race and mixed write-vs-edit race (one
wins, one FS_STALE_VERSION, locks released); edit→edit version refresh at the
provider; the replaceIfVersion post-write version matches a fresh stat. fsio:
a mid-stream abort → FS_ABORTED (previously only pre-abort was covered).
- fs-policy: the agent-without-session owner rung ({agent:{}} → no owner →
createIfAbsent / FS_NOT_OBSERVED); fs/write-intent first-wins (symmetric to
the existing edit-intent test).
- tool-fs: abort-through-the-tool for read/write/edit (isError FS_ABORTED, file
unchanged); a deterministic tool-tier concurrent-edit race via a shared read;
the throwing-fs/observed contract (a throwing listener surfaces as isError but
the mutation already hit disk); the replace_all edit message; parseReadArgs
rejects fractional/NaN offset and zero/negative limit.
- dsh-fs: FsError chains a cause through ErrorOptions.
New with-key e2e (packages/fs/tool-fs/tests/fs-tools.e2e.ts, self-skips without
DEEPSEEK_API_KEY): a real model drives the real read/write/edit tools to create
→ read → edit a file, verified on disk; a second test proves a relative path
resolves against the per-session cwd (factory meta.cwd) not config.cwd. Booted
via a plain tests/harness.ts. Added dsh-agent-loop + dsh-llm-deepseek devDeps.
Codex convergence findings on the delegate-and-fold fix (code path verified
correct, prose only):
- The hook-bridges RFC claimed a downstream `block` "carries the bridge context
too" for BOTH seams. True for `tools/post-execute` (PostToolDecision.block has
an additionalContext field) but false for `agent/prompt-submit`
(PromptDecision.block is `{kind,reason}` with no context field). The code is
already correct — a blocked prompt drops the context, which is right since the
prompt never reaches the model. Reworded the RFC to state the per-seam
difference accurately.
- Two test comments narrated "Before the fix…", which the current-state-only
doc rule forbids. Reworded to describe the behavior, not its history.
- Documented on concatContext (both bridges) why the merged block carries a
single source: a HookContext holds one MessageSource and the seam cannot
represent mixed provenance; rendering distinguishes only by source.kind, so a
downstream plugin's text stays framed as plugin context.
The fs tools rendered as generic cards (title = tool name, raw file content) in
an ACP editor. Give them tool-owned presentation like bash/subagent have:
- read → title "Read <path>", kind read, offset/limit as rawInput
- write → title "Write <path>", kind edit
- edit → title "Edit <path>", kind edit, a clipped old→new rawInput summary
Add a provider-neutral `locations: { path, line? }[]` to ToolCallPresentation —
the files a call reads/modifies — so a capable editor can follow along / jump to
the file (read carries its offset as the line). The ACP bridge forwards it onto
the wire `tool_call` (ResolvedCallPresentation + call() + the tool_call build in
streamSessionEventUpdate). This flips the `locations` cell in the ACP feature
matrix to supported. The SDK already carries `tool_call.locations`
(ToolCallLocation `{ path, line? }`), so no ACP types leak into dsh-tools.
presentResult is intentionally omitted: it only receives `{ content, isError }`,
not the write/edit outcome, so titling by create-vs-overwrite or replacement
count would mean parsing the model-facing text — the static title stays.
Tests: pure presentCall assertions for all three tools incl. locations and the
edit rawInput clip; a bridge test drives the REAL fs tools through ToolPresenter
and asserts locations reaches the wire tool_call (proven to fail without the
forwarding line). New withFs harness option + dsh-fs devDeps on dsh-acp.
Address review on the hook-bridges PR — two composability/compatibility bugs
in both the CC and Codex bridges:
1. A hook that only attaches additionalContext (no block/deny) returned
`allow`/`accept` WITHOUT calling next(), short-circuiting every later
agent/prompt-submit / tools/post-execute listener. A policy/sandbox plugin
registered after the bridge never saw the prompt. Now the context-only path
delegates via next() and folds its context onto the downstream decision
(concatContext): a downstream block/deny still wins and carries the bridge
context; a downstream allow/accept keeps its own content rewrite and gains
the context. Only a real hook deny/block short-circuits.
2. CLAUDE_PROJECT_DIR was empty in the default ACP wiring (no projectDir
configured), breaking common unmodified hooks that reference
$CLAUDE_PROJECT_DIR. It now defaults per-run to the agent's session
workspace (the same cwd the hook runs in); an explicit config.projectDir
still wins.
Regression tests per bridge: a later listener blocks a prompt a context-only
hook allowed; both contexts survive when the downstream also adds one; the
default CLAUDE_PROJECT_DIR reaches the hook. Each proven red on the pre-fix
code.
The ACP bridge gives each session its own workspace (SessionHeader.cwd), and
dsh-tool-bash already resolves a bash workdir against it. But ctx.fs.resolve(path)
took no caller context and dsh-fs-local resolved every relative path against a
fixed config.cwd (process.cwd() at plugin load) — so in the ACP demo `write
foo.txt` and `bash cat foo.txt` hit different directories the moment an editor
opens any project other than the server's launch dir.
Thread the session cwd into resolution, mirroring dsh-tool-bash: widen
FileSystem.resolve to resolve(path, opts?: { cwd?: string }); dsh-fs-local bases
a relative path on opts.cwd ?? config.cwd (absolute paths ignore it); the
read/write/edit tools derive it via a shared sessionCwd(exec) helper
(exec.agent?.session.header.cwd). The provider stays free of dsh-agent/dsh-session
— the tool projects exec → cwd and hands over a plain string, per the
explicit-at-seams convention. Backward compatible (the arg is optional).
Tests: fs-local resolve(path,{cwd}) bases relative on the passed cwd / ignores it
for absolute; tool integration writes/reads/edits in a session cwd != config.cwd
and verifies the file on disk (proven to fail on the pre-fix no-cwd path). Fakes
that stood in a bare {session:{}} now carry a header so sessionCwd doesn't throw.
RFC in docs/rfc/implemented/architecture/2026-07-02-fs-per-session-cwd.md.
Address review on the hook-protocol PR: the event-scope guard only rejected a
`hookSpecificOutput` block whose `hookEventName` NAMED a different event than
the firing one. A block with NO `hookEventName` slipped through and applied its
event-scoped permission fields to whatever event was firing. Under the keyed
Claude Code schema (where `hookEventName` is part of the block) a missing
discriminator is as malformed as a mismatched one — a Stop/UserPromptSubmit
hook emitting a bare `{ permissionDecision: 'deny' }` could deny the current
point.
Drop the `eventName !== undefined` clause so the guard fires on both a
mismatch and an omission when the caller passes `expectedEventName`; the
opt-out (no expectedEventName) still applies a discriminator-less block as-is.
Flipped the test that pinned the old behavior (it documented an artifact, not
a contract) and proved the corrected one red on the old guard.
Address review on the interception-seams PR: PromptDecision.reason is
documented as the durable record of why a prompt was blocked, but the loop
only surfaced it via the fully-blocked batch's `rejected` turn/end. In a MIXED
batch — one queued prompt blocked, another allowed — the turn does not end
`rejected`, so the blocked prompt and its reason vanished from the session log
entirely.
Add a `prompt/blocked` SessionEventMap variant (content + source + reason),
appended in the open turn at the veto point in place of the user/message the
prompt would have become. It is a non-surface, turn-enclosed event (like
todo/write): it never reaches deriveMessages(). The fully-blocked batch still
also ends `rejected` for boundary balance + ACP settlement. Regression test
drives a mixed batch and asserts the blocked prompt is recorded while the
allowed one runs — proven red without the append.
Tianyi Cui
Dudu-0223
Yichen Jiang