Review follow-up: the blanket 10s default correctly targets worker-thread
cold-start races (starting, first-script-line, async child-registration
messages — genuinely CPU-bound under CI contention), but four waits assert
something different — that the HOST reacted PROMPTLY to an event that
already happened (a settled result, an observed worker death). Those had no
cold-start left to wait on, so the generous default just widened the window
a real regression could hide in.
Verified by injecting a 6s delay into the settle-reap's abort call: the
un-overridden helper's test still passed in ~6s. The same mutation now fails
in ~1s with the explicit 1000ms override restored on all four sites (the
abort-on-settle test's own assertion, the two worker-death cancel/dispose
reap checks, and the dispose-ack-race check). The other 12 waits keep the
10s default — they run BEFORE a result is awaited, waiting on the worker to
actually start rather than on a host reaction. Doc comment corrected to
describe the split instead of claiming every wait is a cold-start race.
The spec's 16 vi.waitFor sites used the 1s default timeout to wait for
worker-thread startup and child registration — CPU-bound work that blows
past 1s on a contended runner. The CI coverage lane (4 vitest workers plus
suites that spawn real subprocesses) hit this 3 times across 4 recent PR
runs, each a different subset of the cancellation/worker-death tests, each
green on rerun.
Every wait now goes through a shared helper with a 10s bound, and the file
sets a 30s test timeout to make room for it. The one deliberately tight
wait keeps its 800ms bound through the helper's override — it proves the
host (not the wedged worker's later loop turn) delivered the cancel, so a
generous bound would erase what it tests. No behavior under test changed.
The empty-env hardening wiped the one variable the UNBUILT worker's
loader depends on: tsx finds a tsconfig by searching up from the
worker's cwd, and a parent running outside the repo (the ACP snapshot
harness pins the repo tsconfig through TSX_TSCONFIG_PATH exactly
because its child cwd is a temp dir) lost the dsh-* paths map - the
worker then resolved workspace imports to unbuilt lib/ bundles and died
on CI with ERR_MODULE_NOT_FOUND (green locally only because stale built
lib/ masked the wrong resolution).
Forward exactly that variable when the parent carries it - loader
plumbing, not a secret; the built shape stays fully empty - and pin the
whole contract with an escape-based test: the worker env is exactly
{TSX_TSCONFIG_PATH}, the credential canary still never crosses.
The documented vm escape reaches process, and the worker inherited the
harness's env - so a buggy or prompt-injected script could read and
exfiltrate ambient credentials (DEEPSEEK_API_KEY et al.) without
touching a single file (ds-review-bot finding on #233).
Spawn with env: {} and a hermetic execArgv on both runtime shapes, the
same stance as dsh-code-runtime-worker and stronger than the scrubbed
env the defensive-patterns rule requires for spawned commands (a shell
needs PATH; this worker needs nothing). Ambient-channel hardening only:
an escapee keeps the process-wide privileges the trust premise already
admits - the genuine sandbox remains an engine swap.
agent-end was worker-authored only, so a start already forwarded to
observers lost its paired end whenever the worker could no longer speak
- the grace force-settle terminating a wedged script, or an unexpected
worker death - stranding progress consumers with agents that never
finish (ds-review-bot finding on #233).
The host now keeps a ledger of forwarded starts and funnels every
agent-end through one gate: worker-reported ends pair (and clear) their
entry, and both termination paths drain the remainder as synthesized
'cancelled' ends BEFORE the run settles, so ends always precede
workflow/end. A real settlement racing the force-settle loses to the
synthesized cancellation - the same first-wins override onResult applies
to the run's own result.
Systematic trace through Zed (crates/agent_ui thread_view.rs +
crates/acp_thread): kind:execute routes a tool call onto the
terminal-card layout, whose header (render_collapsible_command) has NO
disclosure toggle, whose body content renders only when is_open — a
flag only a real terminal entity can ever set — and which suppresses
the Raw Input view outright. Every prior attempt (rawInput, pending
content, completed content) targeted slots that layout structurally
never renders; the one slot it always shows is the TITLE, which said
"Run code". codex-acp confirms the idiom: execute cards are titled
with the command itself.
presentCall now titles the card with the program (rawInput kept as the
canonical input slot); presentResult omits the title — an update
replaces only provided fields, so the program header persists — and
carries the captured output as content. Goldens re-recorded; the unit
test pins title-carries-program on both frames.
The sandbox execute wrapper JSON round-tripped the return and blindly cast it
to ToolExecuteReturn. A JSON-valid but wrong-shape return — a bare string,
{ content: 'ok' }, blocks without a type tag — sailed through: the registry
spreads result.content, so { content: 'ok' } became ['o','k'], passed the
session log's isJsonValue gate, and the DeepSeek serializer then flattened it
to '(no output)' — silent corruption of the next model request and every
replay, instead of a contained tool error.
The round-tripped value is now shape-checked against the two ToolExecuteReturn
forms (array of content blocks, or { content: blocks, meta? }); block checks
are structural only (plain object + string type tag) because the ContentBlock
union is merge-extensible. A wrong shape — and the formerly cryptic
forgot-return/bare-string cases — fails that one call with a teaching error
echoing a truncated preview of what was returned and the two valid forms.
New specs pin the object-form pass-through (meta included), six rejection
shapes, and the preview truncation; per-file 100% coverage holds.
The previous commit put the fenced program only on the pending card —
but an ACP tool_call_update's content REPLACES the card content (Zed
truncates to the new list, crates/acp_thread update_fields), so the
code vanished the moment the run completed and was effectively never
visible. presentResult now re-carries the fenced program before the
captured output via a shared fencedProgram helper; the completed card
body is program + output, rendered by Zed as syntax-highlighted
markdown behind the card disclosure. Goldens re-recorded (filtered
this time: DSH_SNAPSHOT=record vitest -u -t mode-turn); unit test pins
the two-block result content.
The generated program rode only rawInput — the detail/expanded slot
many ACP clients never open — so the code a run executed was invisible
in the UI stream. presentCall now also carries it as a fenced ts block
in the card's content, which the bridge already forwards as tool_call
content. The two code-mode snapshot goldens are re-recorded live and
replay green; the presentation unit test pins the fenced block.
The workflow tool's wire schema changed (required meta object
parameter; body-only script). Request-header content is pinned by
exactly ONE scenario (text-turn) and scrubbed to {{system}}/{{tools}}
tokens everywhere else, so the schema change re-records exactly two
fixtures:
- text-turn — the pinned header itself (the one committed copy of the
tool schemas; every other scenario's live header is asserted equal to
this pin by the uniformity guard).
- workflow-run — its recorded interaction used the removed call shape
(meta embedded in the script), which the engine now rejects; the
authored prompt is updated to dictate the new shape (meta as a
parameter, body-only script) and the scenario re-recorded to a clean
single call.
Every other fixture stays byte-identical and replays green against the
new pin. Known pre-existing exception: fs-policy-reject's recording
carries a GNU-only sed -i fallback that fails BSD/macOS replay — kept
out of this change deliberately (the snapshot CI lane is ubuntu).
P1 review finding: extractMeta timed only the literal's vm evaluation;
materializing the RESULT then read properties ordinarily on the HOST
stack, so a meta literal smuggling a getter (get name() { while(true){} })
could wedge the host outside any timeout — defeating the exact spin
isolation the worker thread exists for.
Rather than harden the evaluator (descriptor walks, AST validation),
delete the mechanism: the workflow's identity now reaches the seam as a
plain JSON field (WorkflowStartRequest.meta), carried by the tool as a
schema-validated `meta` object parameter the model fills directly. The
engine only shape-validates data (validateMeta, every violation named)
and pre-parses the body; the scanner, the vm evaluation, and the
host-side materialization are gone, and with them the hole. A body
still opening with a Claude Code-style `export const meta` statement
gets a pointed SCRIPT_PARSE message (the likeliest authoring slip; a
CC script's body stays drop-in, only its meta header moves into the
parameter). syncTimeoutMs now governs exactly one thing: the initial
synchronous slice inside the worker.
The RFC's decision section is rewritten in place (implemented-RFC
rule); the embedded-meta format moves to alternatives-considered with
the hole as the reason. Tool description, presentation (title now reads
meta.name directly — the textual sniff is gone), seam vocabulary docs,
and catalogs follow.
The regression's script spun immediately after calling agent(), but the
agent() continuation (which posts the child-start RPC) only runs on a
microtask tick — the spin seized the worker's loop before the post, so
the host never saw a child inside the waitFor window. A few await-null
yields before the spin let the RPC out; the posted message needs no
further worker-loop turns to reach the host, so the wedge still holds
for the Cancel message the test is about.
Pure mechanical rename now that the package's internals are the
worker-thread engine: directory, package name, spec/e2e filenames,
module tags and logger prefixes, tsconfig/knip/run-gates/AGENTS.md
references, example cordis.yml plugin ids, doc links; catalogs
regenerated and the lockfile refreshed.
