Review follow-ups on the bash owner-token PR:
- packages/acp/README.md still described task isolation in object-identity terms
("records each background task's owning agent", "a different agent"). Rewrite
to the session-token model: ownership is by `session.header.id`, stored on the
executor's task, so a different Agent object on the same session may access it
and ownership survives a tool-bash HMR reload.
- The reviewer flagged that the notice routes by `session.header.id` while the
registry only enforces unique `agent.id`, so a programmatic caller could
register two agents sharing a session token and mis-route a notice (not
reachable via ACP). Rather than bolt a session-id invariant onto the generic
registry, add a proposed RFC (2026-06-20-unify-agent-and-session-id) to remove
the precondition by construction — an agent IS its session, one id — with a
full risks discussion (forecloses multi-session-actor / fork futures, makes the
config resume-or-create policy load-bearing, migration churn). The actual
unification ships as its own Codex-converged PR. Cross-linked from the
agent-lifecycle RFC's seam-precondition note.
- Reframe the tool-bash module-doc ownership paragraph to current-state (per the
new AGENTS.md doc convention): contrast storing the token on the executor vs
in the plugin as a standing rationale, not as "closing the old gap".
A reviewer found that window 2 (a cancel from a synchronous agent/status('running')
listener) had the same early-whenIdle() race that window 1 already guards: it
unconditionally `setStatus('idle')` + continue, which settles `whenIdle()`
waiters — so if the running listener cancels AND queues replacement work, the
waiter resolves while the replacement is still queued-and-unrun (the next
iteration runs it later, but the caller already observed quiescence).
Mirror window 1: after clearing the marker, only `setStatus('idle')` when
nothing new is queued; otherwise fall through to run the queued replacement
(status is already `running`), so `whenIdle()` resolves on that turn's
running→idle. Regression test reproduces the reviewer's interleaving (running
listener cancels A, sends B; whenIdle() resolves only after B ran).
Also syncs the cancellation contract in the two ACP RFCs that describe the live
behavior: `session/cancel` is the queue-aware `agent.cancel()` (drops an
about-to-start turn), not the old best-effort `agent.abort()` pre-step
limitation.
Delete the `taskOwner: Map<string, Agent>` entirely — it served two roles
(access control AND holding a live Agent for completion notices), both now
stateless:
- Access control: `bash_output`/`bash_kill` compare `ctx.bash.ownerOf(id)` to
the caller's token (`exec.agent?.session.header.id`) with `!== undefined`
semantics (an empty-string token is still a real owner). The owner is stamped
at spawn via `resolve({ …, owner })`. Ownership now lives on the task in the
executor, so it SURVIVES a tool-bash HMR reload — closing the old
XXX(tool-bash-owner-hmr) gap.
- Completion notice: `onTaskDone` reads `ctx.bash.ownerOf(task.id)` and finds
the live agent by scanning `ctx.get('agents')?.list()` for a matching
`session.header.id` (read via `ctx.get` — the listener runs on the bash
fiber, a foreign fiber, where the `ctx.agents` proxy would throw). No
registry / owner gone → drop the notice cleanly.
Token is `session.header.id` (NOT `session.id`): every other subsystem keys off
the header id, and the test fakes populate only `session.header.id`, so reading
`session.id` would make every fake unowned and pass the isolation tests for the
wrong reason.
Tests give A and B DISTINCT real session tokens (a same-token-different-Agent
case is now ALLOWED — identity no longer matters); the HMR test inverts to
assert ownership SURVIVES a tool-bash reload; a new test covers the
owner-agent-gone-before-completion drop. Migrates the agent-lifecycle RFC
proposed->implemented (recording all three seams + the session-id-uniqueness
precondition) and updates the tool-bash README + the now-implemented RFC's
cross-links.
Background-task ownership needs a stable home that survives a consumer HMR
reload. Add an optional `owner?: string` to `BashExecRequest` and a
required-but-nullable `owner: string | undefined` to the resolved
`BashExecSpec` (mirroring how `workdir`/`timeoutMs` are required on the spec —
a forgotten owner is a visible `undefined`, never a silently-absent property
that yields an unowned, cross-session-readable task). `resolve()` carries it
through.
Expose the stored token via a new `BashExecutor.ownerOf(id): string |
undefined` seam (ONE read path — not also on the public `BashTask`). The
executor stores and returns the token verbatim and NEVER interprets it: the
access POLICY lives in the consumer (`dsh-tool-bash`). `bash-local` stores
`owner` on its `TrackedTask` and implements `ownerOf`; unknown-id and
known-but-ownerless both read as `undefined`. Because ownership lives on the
task in the executor (disposed with the `dsh-bash` fiber), it survives a
`tool-bash` HMR reload.
Updates the StubExecutor seam test and the bash/bash-local READMEs.
The bridge now holds each session's `AgentHandle` disposer in its
`SessionRecord` and runs it on teardown (client disconnect or fiber dispose)
instead of the old `abort()` + `whenIdle()` drain that left agents
registered. A bare client disconnect now leaves NO registered agent and NO
session-store entry — not an idled-but-still-registered one. The queue-aware
`cancel()` inside the disposer also closes the former pre-step best-effort
window (a turn about to start is dropped), so teardown reaches true
quiescence.
The `session/load`-races-teardown leak is fixed: if the bridge closed while
`resume()` was pending, the just-resumed handle is disposed before throwing,
so it leaves no orphan (it has no SessionRecord, so quiesce() never sees it).
Tests: the disconnect test now asserts (through the SAME memoized teardown)
that the agent is unregistered AND its session removed; a durability test
re-loads the persisted log after dispose and asserts the closing turn/end is
on disk (guards the teardown-order contract); a sibling-isolation test proves
one handle's dispose() leaves other agents untouched. Docs: agent /
agent-loop / acp READMEs, architecture.md, and the stale in-code quiesce()
ownership comment updated to the per-agent disposal model; the now-resolved
TODO(rfc010-agent-disposal) / TODO(rfc010-cancel-prestep) teardown notes
removed.
The JSONL and SQLite backends were byte-identical (or same-algorithm) for ALL
of their write-path orchestration — the four maps (states/buffers/chains/inits),
installWritePath, initFor, onCreated's four adoption cases, flush, drain,
serialize, adopt/adoptLivePrefix, assertVersion, and the create/append/load/
has/delete skeletons. Only the storage primitives (write bytes vs INSERT rows)
differed, so every fix landed twice.
Extract that orchestration into a PersistenceCoordinator in the seam package.
Each backend composes one (new PersistenceCoordinator(ctx, this)), implements a
small PersistenceBackend hook interface (loadStored, loadLive, appendBatch,
commitRepair, deleteStored, list, optional close), and delegates its six public
service methods to it. Composition, not inheritance — a backend exposes only the
hooks, can't reach the coordinator's private state, and the public
SessionPersistence API is unchanged so a third-party backend may still implement
it directly.
The crash-repair torn-tail token is OPAQUE: the coordinator computes the
synthetic closers (it owns interruptedTurnClosers) but only tests
`tornMarker !== undefined` and round-trips it to commitRepair, never inspecting
it (JSONL = byte offset, SQLite = seq). loadStored vs loadLive stay distinct so
HMR adoption is cwd-scoped (a same-id log at a different cwd is a collision, not
a resume). appendBatch carries meta so lazy-materialize + first-batch commit
atomically (no separate materialize hook).
Tests: the duplicated orchestration tests (adoption, HMR, collision,
dispose-drain, crash-tail) move into one runCoordinatorContract suite run once
per backend (memory + jsonl + sqlite) via hook fixtures; per-backend specs keep
only storage mechanics. A through-coordinator torn-tail test per real backend
keeps the commitRepair-with-marker branch covered under the 100% gate.
Net -112 lines (the dedup outweighs the new coordinator + shared suite); 100%
coverage; backends shrank ~1200 lines of duplicated churn. Migrates the
write-coordinator RFC proposed -> implemented.
Codex's converge pass on PR A flagged three now-false references the deletion
left behind:
- the proposed write-coordinator RFC still listed an "update summary" backend
hook and "sidecar behavior" in its test focus;
- the JSONL README's format-version note still said a format change needs a
"version bump + migration" (contradicting the no-migration pre-release stance);
- a stale "sidecar pathing" comment in findLog's cwd-recovery branch.
All three corrected to current truth.
Rename the concrete Agent class to make its ReAct-style reasoning loop
explicit in the name. Package name, default-export plugin (`AgentLoop`),
and the `ctx.agentLoop` service key are unchanged.
Records the verified design before implementing: keep dsh-bash agent-side
execution and render Zed's terminal tool-call card via the `_meta` convention
(terminal_info/terminal_output/terminal_exit), capability-gated on
clientCapabilities._meta.terminal_output, with the ```console text block as the
no-capability fallback. Rejects the spec's client-side terminal/create path (it
would bypass dsh-bash's sandbox/env-scrub/ownership/cwd). Studied
claude-agent-acp, codex-acp, and Zed's renderer to ground the wire contract.
Live streaming and command classification are noted as separate follow-ups.
- bash presentCall title is now "description — command" (e.g. "List files in
src — ls -la src"). An execute-kind ACP card HIDES rawInput (Zed renders it
only for non-terminal tools), so the command must ride in the always-visible
title to be seen — matching how claude-agent-acp/codex-acp title execute
tools. The command stays in rawInput too for non-execute UIs that show it.
- Rework the acp tool-call presentation tests (turns + load replay) to drive the
REAL dsh-tool-bash + dsh-bash-local via a new makeBridgeHarness({ withBash })
option, running an actual `echo` — instead of an inline fake bash tool. The
mock MODEL still scripts the call (deterministic, no key), but the tool and
executor are real, so the test verifies the shipping presentCall/presentResult.
- AGENTS.md: add the principle "prefer the REAL implementation over a mock/
stand-in in tests" (mock only the expensive/non-deterministic boundary).
- RFC (proposed): the ACP terminal sub-protocol + command classification — the
capability-gated rich rendering (live cwd-header terminal card, classify a
`cat` as a read / `grep` as a search) that the reference adapters do; the
fenced ```console text block stays the no-capability baseline. Studied
codex-acp, claude-agent-acp, and Zed's renderer to ground it.
- Fix two root-AGENTS.md cross-links that the depth bump left pointing at the
new docs/AGENTS.md instead of the root file they cite (capability-seams,
optional-code-mode). These resolved on disk so verify-md-links passed — the
gate checks existence, not which file you meant; corrected to ../../../.
- Broaden verify-md-links scope to .agents/skills/**/*.md: this PR rewrote the
dsh-code-review skill's links into the RFC tree, but the skill dir was outside
the gate, so a broken skill link would have passed silently.
- Percent-decode the path component before the existence check, so a valid
encoded relative target (My%20File.md) is not falsely reported broken; a
malformed escape (%zz) is reported broken rather than crashing the gate.
- Drop the merged property-testing RFC's "nightly CI job 100x" claim: that line
came from the original proposal, not the accepted decision, and CI has only
push/pull_request triggers — note it as possible future work instead.
doc-sync (incl. verify-md-links over 58 files), doc-typecheck, lint pass.
Collapse docs/adr/ and docs/rfc/ into a single docs/rfc/ with proposed/,
implemented/, and rejected/ subfolders. Every file is renamed to
yyyy-mm-dd-topic-title.md, where the date is when the topic was first
proposed (from git history). ADRs and RFCs that covered exactly the same
topic are merged (property-based testing, session persistence); the
umbrella RFC 005 stays split across its three implemented decisions, and
RFC 006's deferred part-3 (API extractor reports) splits into its own
proposed RFC. All cross-references become machine-checkable relative
links instead of bare "ADR NNNN" / "RFC NNN" prose.
Add a verify-md-links doc-sync gate (scripts/verify-md-links.ts) that
checks every relative Markdown cross-link resolves, wired into doc-sync
alongside verify-md-wrap. This makes the reorganization self-verifying:
the same change that rewrote ~forty inter-doc links adds the check that
proves none dangle. Document the cross-link convention in a new
docs/AGENTS.md and record the gate as an implemented RFC.
doc-sync, typecheck, lint, and the full test suite (667) all pass.
Tianyi Cui