DeepSeek/deepseek-harness
1
0
Fork 0
mirror of https://github.com/deepseek-ai/deepseek-harness synced 2026-08-15 21:04:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(acp): server crashed on connect — drop export default, read optional service cwd-independently

Browse Source 
Two independent bugs made the ACP server crash the moment an editor (Zed)
connected, despite 178 green unit tests at 100% coverage:

1. `session/new` threw `cannot get property "agents" without inject`. Root
   cause: a stray `export default apply` made the cordis Loader's
   `unwrapExports` (`exports.default ?? exports`) collapse the module to the
   bare `apply` function, discarding the sibling `inject`/`name`/`Config`
   named exports. The plugin fiber was built with empty `inject`, so every
   `ctx.<service>` read in `apply` threw at load. Fix: remove the default
   export so the Loader uses the namespace.

2. `session/load` threw `cannot get property "sessionPersistence" without
   inject`. `AgentLoop.resume` read `this.ctx.sessionPersistence` (a service
   it deliberately does NOT inject); the property proxy's ancestor-only fiber
   walk fails through the bridge's traceable shadow. Fix: read it via
   `this.ctx.get('sessionPersistence', false)`, the topology-independent
   global-store lookup.

Why the suite missed both: every test mounted the plugin by hand
(`ctx.plugin({name,inject,apply})`), bypassing `unwrapExports` entirely, and
the only test driving these RPCs was key-gated (skipped in CI). Added a no-key
`session/new` e2e that boots the real example through the real Loader — it
fails loudly on bug #1 without an API key. Set `TSX_TSCONFIG_PATH` in the e2e
spawn so the subprocess resolves workspace `paths` from a temp cwd (it was
silently falling back to a stale built `lib/`).

Docs: post-mortem 0001; AGENTS.md "line coverage is not behavior coverage" +
with-key/smoke-test philosophy; packages/AGENTS.md plugin-export-shape and
ctx.get rules; dsh-code-review SKILL checks.
This commit is contained in:
Tianyi Cui
2026-06-18 03:12:37 +08:00
parent a9d5a5ba68
commit 6d37b6c33d
10 changed files with 221 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.agents/skills/dsh-code-review/SKILL.md
View File

@@ -20,8 +20,9 @@ This is a where-to-look map, not a rules list. The rules live in the docs below
1. **Docs in sync?** If the PR changes a config key, default, error code, wire field, or event name, did it update the package README + module/JSDoc in the same diff? Stale docs are the most common miss — `pnpm run doc-sync` only gates compilable `ts` blocks and the event-taxonomy table, so prose drift (config keys, defaults, error codes, wire fields) has no gate and is on the reviewer to catch.
2. **HMR-safety test present?** Any new registry/registration needs a test that disposes the contributing fiber and asserts cleanup. Its absence is a blocking gap.
3. **Gates green?** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints. Don't re-review what a gate already enforces — trust the gate, spend attention on what gates can't check (intent, contracts, doc sync).
4. **e2e verifies the world, not the agent's self-report.** For real-API tests, confirm the assertion re-runs the command/checks the file externally — a keyword probe lets a cheating agent pass (see AGENTS.md e2e bullet).
5. **Seam discipline.** New swappable capability? Check it's split per ADR 0009 (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type.
4. **e2e verifies the world, not the agent's self-report.** For real-API tests, confirm the assertion re-runs the command/checks the file externally — a keyword probe lets a cheating agent pass (see AGENTS.md e2e bullet). For a behavior change to the agent's real flows, a no-key/mock test alone is usually insufficient: a with-key e2e (especially a smoke test that boots the real example and checks the world) is cheap here and catches "green units, broken product" — encourage it rather than treating real-API tests as expensive (see AGENTS.md § Secrets / .env).
5. **Plugin export shape + real-loader coverage.** A new/changed `cordis.yml`-loaded plugin: is it a function/namespace plugin (`name`/`inject`/`Config`/`apply` named exports) with NO `export default`? A stray default export makes the Loader's `unwrapExports` drop `inject` and the plugin crashes at load with `cannot get property … without inject` — invisible to hand-built `ctx.plugin({...})` tests and to line coverage. Confirm there's a test driving it through the REAL loader path (the no-key subprocess e2e for ACP is the model). And any opportunistic read of a service NOT in `static inject` should use `ctx.get(name, false)`, not `ctx.<name>` (the property proxy throws through a foreign shadow). See [packages/AGENTS.md](../../../packages/AGENTS.md), AGENTS.md § Defensive patterns, and [docs/postmortem/0001](../../../docs/postmortem/0001-acp-default-export-drops-inject.md).
6. **Seam discipline.** New swappable capability? Check it's split per ADR 0009 (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type.
## How to respond

7
AGENTS.md
View File

@@ -47,6 +47,8 @@ docs/ architecture.md — the design doc. module-graph.md — generated
adr/ — decision records (the
why behind vendoring, event-sourcing, the schema DSL, …).
rfc/ — proposals for substantial future work.
postmortem/ — incident write-ups: a bug that escaped to a
user/merge/release, why the safety nets missed it, the guardrails added.
cookbook/ — step-by-step guides: adding a package, a tool,
an LLM adapter.
scripts/ repo maintenance scripts (vendor-manifest guard, publint runner).
@@ -97,6 +99,8 @@ DEEPSEEK_BASE_URL=https://… # optional; defaults to the public API
cordis.yml configs reference env vars with the `!!js` tag: `apiKey: !!js process.env.DEEPSEEK_API_KEY`. Never commit real credentials; CI has no secrets and e2e suites must self-skip without them.
**Lean on with-key e2e tests — we are DeepSeek and model inference is cheap.** A no-key test (mock adapter, or an operation that never reaches the model) is great for determinism and CI, but it can only prove the plumbing, not that the agent actually *works* against a real model. Do not ration real-API tests to save tokens: write many of them, cover the real flows (a real prompt that writes a file, a multi-turn conversation, tool use, cancellation mid-stream), and run them frequently while developing — locally and whenever you have a key in the environment. **Especially smoke tests**: a cheap with-key smoke test that boots the real example, sends one real prompt, and checks the world (a file on disk, a non-empty assistant turn) catches whole classes of "green unit tests, broken product" failures that mocks structurally cannot — the very gap that let the ACP inject bug ship (see [docs/postmortem/0001](docs/postmortem/0001-acp-default-export-drops-inject.md)). The self-skip rule is ONLY so CI (which has no secrets) stays green and so a contributor without a key isn't blocked — it is not a signal that real-API tests are expensive or second-class. When in doubt, add the with-key test AND run it.
Dev/test/demo run **unbuilt** via tsx + the `paths` map in the root `tsconfig.json` (`vitest` resolves through `tsconfig.test.json`). Building is only needed for publishing/consumption outside the repo — with one exception: `pnpm run lint`'s type-aware rules resolve vendor packages through their built declarations (`tsconfig.typecheck.json` → `vendor/*/lib`), so run `pnpm run typecheck` once after a fresh clone (CI does the same) or lint reports unresolved-type `no-unsafe-*` errors.
## Conventions
@@ -116,7 +120,7 @@ Dev/test/demo run **unbuilt** via tsx + the `paths` map in the root `tsconfig.js
- **Symmetry is usually more correct**: when two related values play parallel roles (a test fixture and its expected output, a request shape and its response shape, a buggy input and the test that checks the fix), give them parallel form — both named consts, or both inline, not one each way. Asymmetry is a smell that usually points at a missed extraction.
- **Merging PRs**: always merge with a **merge commit** (`gh pr merge --merge`), never squash or rebase. The per-PR commit history is intentional — review-fix commits, regression-test commits, and the reasoning in each message are part of the record — and squashing flattens it away.
- **TODO markers**: use `FIXME`/`TODO`/`XXX` to flag known issues by urgency — see [docs/development.md](docs/development.md) for the semantics of each.
- **Tests**: vitest, colocated under `packages/<name>/tests/*.spec.ts`. Every registry needs an HMR-safety test (dispose the contributing fiber, assert cleanup). **Excessive tests are welcome** — when in doubt, write the test; err on the side of covering edge cases, error paths, event ordering, and concurrency races even if they seem unlikely. Review findings get regression tests (see `packages/agent-loop/tests/review-fixes.spec.ts`).
- **Tests**: vitest, colocated under `packages/<name>/tests/*.spec.ts`. Every registry needs an HMR-safety test (dispose the contributing fiber, assert cleanup). **Excessive tests are welcome** — when in doubt, write the test; err on the side of covering edge cases, error paths, event ordering, and concurrency races even if they seem unlikely. Review findings get regression tests (see `packages/agent-loop/tests/review-fixes.spec.ts`). The same generosity applies to **real-API (with-key) e2e tests — inference is cheap here (we are DeepSeek), so do not ration them**: cover the agent's real flows (a real prompt that writes a file, multi-turn, tool use, cancellation) and run them frequently while developing, especially cheap **smoke tests** that boot the real example and check the world. A green mock/no-key suite proves the plumbing, not the product — the with-key smoke test is what catches "green units, broken product". See § Secrets / .env for the with-key policy and why self-skip is a CI accommodation, not a verdict that real-API tests are expensive.
## Defensive patterns (hard-won)
@@ -129,6 +133,7 @@ Each bullet is a bug class that bit us; the rule prevents the reoccurrence.
- **Contain callback exceptions at the boundary.** A user-supplied listener (`onTaskDone`, event handlers) that throws must not reject the promise it runs inside or starve the listeners after it. Wrap the dispatch loop in try/catch and log; never let one bad subscriber break core lifecycle.
- **Never hand untrusted/model output the ambient environment or predictable paths.** Spawned commands get a scrubbed env (drop `*KEY*`/`*SECRET*`/ `*TOKEN*`) so the harness's own credentials can't leak into output, `env`, or spill files. Temp/spill files use a private (0700) dir, random names, and exclusive owner-only (`'wx'`, `0o600`) opens — predictable world-readable paths invite symlink races and disclosure.
- **e2e tests own their resources.** Real-API/integration tests must create the harness in the test and dispose it in `afterEach` (even on failure/retry/timeout), so a flaky run doesn't leak processes or contexts. Shared fixtures live in a plain `tests/harness.ts` module, NOT another `*.e2e.ts` file — importing a spec file re-registers its `describe` and duplicates real API calls. Verify the WORLD, not the agent's self-report: re-run the command/check externally and assert files are byte-identical where they should be unchanged (a keyword probe lets a cheating agent pass).
- **Line coverage is not behavior coverage; test the REAL entry path, not a synthetic stand-in.** 100% per-file coverage and a green suite are necessary, not sufficient — they prove lines ran, not that the feature works the way it ships. A plugin shipped via `cordis.yml` is loaded by the cordis Loader, which calls `Loader.unwrapExports` (`exports.default ?? exports`) and then constructs a fiber from the module's `inject`/`name`/`Config` namespace exports. A test that mounts the plugin by hand-building `ctx.plugin({ name, inject, apply })` (or even `ctx.plugin(NamespaceImport)`) BYPASSES `unwrapExports` entirely, so it cannot catch a broken export shape. This bit us hard: a stray `export default apply` made `unwrapExports` collapse the module to the bare function, dropping `inject` — so every service read threw `cannot get property … without inject` the instant a real editor connected, while 178 hand-mounted tests stayed green. The guard is at least one test that drives the plugin through its REAL load path (a subprocess booting the example via the Loader, or the Loader API directly), exercising the headline operations end-to-end. It runs WITHOUT a key when the operation doesn't call the model (`session/new`/`session/load` reach the factory but never the LLM), so there is no excuse to skip it. Corollary: when an `*.e2e.ts` spawns the example from a temp cwd, set `TSX_TSCONFIG_PATH` to the repo-root tsconfig — the unbuilt `paths` map is found by searching UP from cwd, so a temp cwd outside the repo silently falls back to built `lib/`, which both hides source changes and only "works" when a stale build happens to exist.
- **Tag spelling and EOF hygiene.** cordis.yml interpolates env via the `!!js` tag (js-yaml resolves custom tags under `tag:yaml.org,2002:js`), not `!js` — keep code, comments, and docs consistent. Files end with exactly one trailing newline; `git diff --check` (a pre-push gate) rejects new blank lines at EOF.
## Type Safety and Documentation

111
docs/postmortem/0001-acp-default-export-drops-inject.md Normal file
View File

@@ -0,0 +1,111 @@
# Post-mortem 0001: ACP server crashed on connect — `export default` dropped the plugin's `inject`
Status: resolved (fix in PR #41 `feat/acp-2-bridge`)
## Executive summary
One stray line — `export default apply` at the bottom of the ACP plugin — made the ACP server crash the moment any editor connected, because the cordis Loader unwraps a default export and threw away the plugin's `inject` declaration along with it. A second, independent bug (an optional service read that fails through Cordis's traceable-shadow proxy) crashed `session/load` for a different reason. Both shipped green: 178 unit tests at 100% line coverage never caught either, because every test mounted the plugin by hand instead of through the real loader, and the only test that drove the failing requests was skipped in CI. The fixes are one-line each; the durable lesson is that **line coverage proved the code ran, not that the feature worked the way it ships** — so we added a no-key end-to-end test that boots the real example through the real loader, plus AGENTS.md rules on plugin export shape and optional-service access.
## Summary
The ACP server (`examples/acp-agent`, `@deepseek-ai/dsh-acp`) crashed the instant a real editor (Zed) connected: the first `session/new` request returned `Internal error: cannot get property "agents" without inject`, and `session/load` returned the same for `sessionPersistence`. The bridge was completely non-functional in production despite 178 green unit tests and 100% line coverage. Two independent bugs were hiding behind the same error string, and the test suite missed both for the same reason: every test mounted the plugin through a path that did not exercise how it actually loads or how its services actually resolve.
## Impact
The ACP server could not create or load a single session — the two RPCs an editor calls first. Anyone wiring the agent into Zed got an immediate hard failure. No data loss (nothing persisted before the crash); the cost was entirely "the feature does not work" plus the debugging time to find out why, twice.
## Timeline
- The bridge (RFC 010) landed with a full unit suite (codec, in-memory transport, property-based protocol-shape, failure paths, HMR), a key-gated real-API e2e, and a no-key stdout-purity e2e. All green, 100% coverage.
- A real Zed session immediately failed on `session/new` with `cannot get property "agents" without inject`.
- Investigation initially pursued a Cordis "traceable/shadow" theory (plausible, and the mechanism is real — see Bug #2), then instrumented the actual fiber walk in vendored `reflect.ts` and ran the real subprocess. The trace showed the throw at `apply()` line 179 *at plugin load time*, on the ROOT fiber with no shadow — falsifying the shadow theory for `session/new`.
- Root cause #1 found: a stray `export default apply`. Removing it fixed `session/new`.
- Removing it then exposed Bug #2: `session/load` still threw on `sessionPersistence` — a genuinely distinct mechanism (the shadow walk), confirmed by isolating the fix and re-running the real subprocess.
## Root cause #1 — `export default apply` drops the plugin's `inject` (broke `session/new`)
`packages/acp/src/index.ts` is a *namespace plugin*: it exports `name`, `inject`, `Config`, and `apply` as separate named exports — the same shape as every other plugin in the repo (`invariants`, `llm-deepseek`, `tool-bash`, `stdio-chat`, …). But it *also* ended with one extra line no other plugin had:
```ts ignore-check
export const name = 'acp'
export const inject = ['agents', 'sessions', 'sessionPersistence']
export function apply(ctx: Context, config: AcpConfig): void { /* … */ }
// …
export default apply // ← the bug
```
When a plugin is loaded from `cordis.yml`, the cordis Loader normalizes the imported module through `Loader.unwrapExports` (`vendor/loader/src/index.ts`):
```ts ignore-check
unwrapExports(exports: any) {
if (isNullable(exports)) return exports
exports = exports.default ?? exports // ← prefers `.default`
if (!exports.__esModule) return exports
return exports.default ?? exports
}
```
With a default export present, `exports.default ?? exports` resolves to the **bare `apply` function**. A bare function has no `inject`, no `name`, no `Config` properties — those lived as *sibling* named exports on the module namespace, and unwrapping to `.default` threw the namespace away. The Loader then built the plugin's fiber from an empty `inject`.
Consequently `apply` ran in a fiber with **no injected services**. The very first line, `const agents = ctx.agents`, walked the fiber tree (ROOT → Include → Loader → ROOT) and, finding `agents` in no fiber's store and reaching the root fiber (`runtime === null`), threw `cannot get property "agents" without inject`. The crash was at *load time*, not in a later request handler — the request just happened to be what triggered the load in the failing trace.
**Fix:** delete `export default apply`. The Loader then uses the module namespace, honors `inject`/`name`/`Config`, and `apply` runs inside a fiber that actually grants the declared services.
## Root cause #2 — optional service read trips the inject guard through a traceable shadow (broke `session/load`)
With #1 fixed, `session/new` worked but `session/load` still threw `cannot get property "sessionPersistence" without inject`. This one *is* the Cordis traceable/shadow mechanism, and it is worth understanding precisely.
`session/load` calls `agents.resume(...)`, which delegates to `AgentLoop.resume()`, which read `this.ctx.sessionPersistence`. `AgentLoop`'s `static inject` deliberately does NOT include `sessionPersistence` — injecting it would make non-persistent demos pend forever waiting for a backend that never loads. The service is provided by a separate sibling plugin/fiber and read opportunistically.
Service access in Cordis goes through a context proxy (`vendor/cordis/src/reflect.ts`). When a service method is invoked through a *traceable proxy* obtained from a foreign fiber (here: the bridge fiber calls `ctx.agents.resume`, and the registry hands back `this.factory` — the `AgentLoop` — re-wrapped as a fresh traceable proxy bound to the caller), `createShadowMethod` (`vendor/cordis/src/utils.ts`) rebinds `this` to a *shadow* object whose `ctx` carries `[symbols.shadow]` pointing at `AgentLoop`'s own construction context. Inside `resume`, then, `this.ctx.sessionPersistence` resolves with the proxy handler starting its fiber walk from the shadow's fiber:
```ts ignore-check
// reflect.ts get handler
let fiber = (ctx[symbols.shadow] as Context ?? ctx).fiber // ← starts at AgentLoop's fiber
while (true) {
const impl = fiber.store?.[prop]
if (impl) return getTraceable(ctx, impl.value)
if (prop in fiber.inject) { /* inactive-context error */ }
if (!fiber.runtime) throw error // ← reached root, throw
if (fiber.parent[symbols.isolate][prop] !== key) throw error
fiber = fiber.parent.fiber // ← ancestor-only
}
```
The walk is **ancestor-only**. `sessionPersistence` is in neither `AgentLoop`'s fiber store (not in its `static inject`) nor any ancestor on the way to root (it lives on a *sibling* branch), so the walk reaches the root fiber and throws.
Why didn't the in-memory `AgentLoop` resume tests catch this? Because they call `ctx.agents.resume(...)` directly from test code — *outside any plugin fiber*. There, `ctx.fiber.runtime` is `null`, so the proxy handler takes an early bypass:
```ts ignore-check
if (!ctx.fiber.runtime) return ctx.reflect.get(prop, false) // ← direct global-store lookup, no fiber walk
```
`ctx.reflect.get(name, false)` is a direct lookup in the global service store keyed by the isolate symbol — it ignores fiber topology entirely and finds the service. So from a top-level test the read works; from inside a real plugin fiber, reached via a shadow, it throws. The bridge is exactly the latter.
**Fix:** read the optional service the same fiber-independent way the bypass does — `this.ctx.get('sessionPersistence', false)` — instead of `this.ctx.sessionPersistence`. `get(name, false)` performs the direct global-store lookup (the `false` skips the active-state check, since the backend lives on another fiber), so resume resolves the backend regardless of which fiber or shadow the call arrives through. The other reads in the resume path (`this.ctx.sessions`, `this.ctx.agents`) are fine — those *are* in `AgentLoop`'s `static inject`, so they sit in its fiber store and the ancestor walk finds them immediately.
## Why every test missed it (the real failure)
Both bugs share one root process gap: **no test exercised the plugin through its real load path or its real call topology.**
- The in-memory harness mounts the bridge by hand-building a plugin object: `ctx.plugin({ name, inject, apply })`. That supplies `inject` manually, so it can never reproduce Bug #1 — `unwrapExports` is called only by the *Loader*, never by `ctx.plugin`. Even `ctx.plugin(NamespaceImport)` would not have caught it.
- The same harness mounts everything flat on one root context, so an `AgentLoop` resume reached from it either runs top-level (the `!runtime` bypass) or through a shadow whose origin still resolves on root — masking Bug #2's ancestor-walk failure.
- The only no-key e2e sent `initialize` and checked stdout purity. `initialize` never reaches the factory, so it sailed past both bugs.
- The only test that drove `session/new`/`session/load` was key-gated, so CI (no key) skipped it — and locally it "passed" only because a stale built `lib/` (with the old code) happened to satisfy module resolution.
100% line coverage was satisfied the whole time. Coverage proves lines *ran*; it says nothing about whether the feature works *the way it ships*.
## Guardrails added
- **Removed `export default apply`** (`packages/acp/src/index.ts`) — the Bug #1 fix.
- **`AgentLoop.resume` reads `this.ctx.get('sessionPersistence', false)`** (`packages/agent-loop/src/index.ts`) — the Bug #2 fix, with a comment explaining the shadow-walk trap.
- **No-key `session/new` e2e over real stdio** (`examples/acp-agent/tests/acp.e2e.ts`): boots the example as a subprocess through the real Loader and asserts `session/new` resolves. This fails loudly on Bug #1 with no API key. Verified it fails when `export default apply` is restored.
- **`TSX_TSCONFIG_PATH` in the e2e spawn**: the subprocess runs from a temp cwd, where tsx cannot find the repo-root tsconfig `paths` map by searching upward — so dsh-* imports silently fell back to built `lib/`. Pointing tsx at the repo tsconfig makes resolution cwd-independent and ensures the test runs *source*, not a possibly-stale build.
- **AGENTS.md defensive pattern**: "Line coverage is not behavior coverage; test the REAL entry path, not a synthetic stand-in" — codifies the lesson for every future plugin.
## Lessons
- A namespace plugin and a default export are mutually exclusive under the cordis Loader. Pick the namespace form (`name`/`inject`/`Config`/`apply`) and do not add `export default` — `unwrapExports` will discard the namespace.
- For a service a plugin reads opportunistically but does NOT declare in `static inject`, use `ctx.get(name, false)`, never `ctx.<name>`. The property proxy resolves by an ancestor-only fiber walk that fails through a foreign shadow; `get(…, false)` is the topology-independent lookup.
- A test that constructs a plugin by hand cannot validate how the plugin loads. At least one test must drive the real Loader/export path end-to-end. When the headline operation does not call the model, that test needs no API key — so it belongs in CI, not behind a key gate.
- Trust the trace, not the theory. The elegant shadow explanation was real but was the *second* bug; the *first* was a one-line export mistake that a fiber-walk `console.error` found in minutes after hours of plausible-but-wrong reasoning.

13
docs/postmortem/README.md Normal file
View File

@@ -0,0 +1,13 @@
# Post-mortems
Incident write-ups: a bug reached a place it shouldn't have (a real user, a merged PR, a release), and the interesting part is *why our process let it through*, not just the one-line fix.
A post-mortem is NOT an [ADR](../adr/README.md) (which records a deliberate decision and its rejected alternatives) and NOT an [RFC](../rfc/README.md) (which proposes future work). It is a backward-looking record of a failure: what broke, the mechanism, why every safety net missed it, and the concrete guardrails added so the same class of bug fails loudly next time.
Write one when a bug is **subtle** (the mechanism is non-obvious and a careful engineer would re-derive it the hard way), **systemic** (the reason it escaped is a gap in tests/tooling/conventions, not a one-off typo), and **costly to rediscover** (it cost real debugging time, and would cost it again). Link the guardrails (tests, AGENTS.md rules, ADRs) the post-mortem motivated.
Every post-mortem opens with an **Executive summary**: one short paragraph a busy reader can absorb in thirty seconds — what broke, the root cause in plain terms, why it escaped, and the durable lesson — before the detailed Summary / Timeline / Root cause / Guardrails sections that follow.
| # | Title |
|---|---|
| [0001](0001-acp-default-export-drops-inject.md) | ACP server crashed on connect: `export default` dropped the plugin's `inject` |

43
examples/acp-agent/tests/acp.e2e.ts
View File

@@ -32,6 +32,17 @@ const startScript = fileURLToPath(new URL('../start.ts', import.meta.url))
// `--import tsx` would not resolve from node_modules. import.meta.resolve gives
// the worktree's tsx regardless of the child's cwd.
const tsxLoader = fileURLToPath(import.meta.resolve('tsx'))
// Absolute path to the repo-root tsconfig. Dev/test/demo run UNBUILT: the
// `@deepseek-ai/dsh-*` workspace imports resolve through the `paths` map in the
// root tsconfig (tsx reads it), NOT through built `lib/` output. But tsx finds
// that tsconfig by searching UP from the child's cwd — and the child's cwd is a
// temp workdir OUTSIDE the repo, so the search misses and the dsh-* imports fail
// (the child dies before writing a byte). Point tsx at the repo tsconfig
// explicitly via TSX_TSCONFIG_PATH so resolution is cwd-independent. (Without
// this the suite only passed by accident when a stale built `lib/` happened to
// exist — exactly the contamination that masked the inject bug this suite now
// guards.) The repo root is four levels up from this file (examples/acp-agent/tests).
const repoTsconfig = fileURLToPath(new URL('../../../tsconfig.json', import.meta.url))
interface Spawned {
child: ChildProcessWithoutNullStreams
@@ -40,11 +51,11 @@ interface Spawned {
stderr: string[]
}
function spawnAcpAgent(cwd: string): Spawned {
function spawnAcpAgent(cwd: string, env: NodeJS.ProcessEnv = process.env): Spawned {
const child = spawn(
process.execPath,
['--import', tsxLoader, startScript],
{ cwd, env: { ...process.env }, stdio: ['pipe', 'pipe', 'pipe'] },
{ cwd, env: { ...env, TSX_TSCONFIG_PATH: repoTsconfig }, stdio: ['pipe', 'pipe', 'pipe'] },
)
const stderr: string[] = []
child.stderr.setEncoding('utf8')
@@ -82,7 +93,7 @@ afterEach(async () => {
workdir = undefined
})
describe('acp-agent stdout purity (no key required)', () => {
describe('acp-agent over real stdio (no key required)', () => {
it('emits only framed JSON-RPC on stdout', async () => {
workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-'))
// Collect raw stdout bytes directly (bypass the SDK framing) to inspect.
@@ -91,7 +102,7 @@ describe('acp-agent stdout purity (no key required)', () => {
// which this purity test never triggers). So this runs WITHOUT real creds.
const child = spawn(process.execPath, ['--import', tsxLoader, startScript], {
cwd: workdir,
env: { ...process.env, DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' },
env: { ...process.env, DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot', TSX_TSCONFIG_PATH: repoTsconfig },
stdio: ['pipe', 'pipe', 'pipe'],
})
const out: string[] = []
@@ -114,6 +125,30 @@ describe('acp-agent stdout purity (no key required)', () => {
expect(() => JSON.parse(line) as unknown).not.toThrow()
}
}, 30_000)
it('session/new succeeds over real stdio (no model call)', async () => {
// REGRESSION GUARD (this exact RPC crashed a real Zed session with
// "cannot get property \"agents\" without inject"): `session/new` drives the
// full bridge → `ctx.agents.create({sessionId, meta:{cwd}})` → AgentLoop →
// registry/persistence path, ALL of which run from the JSON-RPC read loop
// OUTSIDE the bridge plugin's injection scope. A lazy `ctx.<service>` read
// on that path throws and the RPC fails with an Internal error — yet the
// call never touches the model, so this reproduces WITHOUT a key. The
// key-gated prompt test below never caught it (it needs real creds); the
// initialize-only purity test never caught it (initialize does not reach
// the factory). This closes that gap: boot the real subprocess and create a
// session, asserting the RPC RESOLVES (not rejects with an inject error).
workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-'))
// A dummy key lets the deepseek adapter boot (it only checks presence, not
// validity, at apply time); no model call is made, so the key is never used.
spawned = spawnAcpAgent(workdir, { ...process.env, DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' })
const { client } = spawned
await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
const { sessionId } = await client.newSession({ cwd: workdir, mcpServers: [] })
expect(typeof sessionId).toBe('string')
expect(sessionId.length).toBeGreaterThan(0)
}, 60_000)
})
describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over ACP', () => {

6
packages/AGENTS.md
View File

@@ -5,10 +5,12 @@ This directory contains all `@deepseek-ai/dsh-*` harness packages. When editing
- **Effect-based registrations**: every contribution (tool, section, adapter, agent, event listener) goes through `ctx.effect()` / `ctx.on()`, and `register()` methods return disposers. Never use bare arrays or manual cleanup.
- **Declaration merging**: services declare their ctx key in `declare module 'cordis' { interface Context { } }` and their events in `interface Events`. Merge-extensible maps (`ContentBlockMap`, `MessageSourceMap`, `FinishReasonMap`, `TurnTriggerMap`, `TurnEndReasonMap`, `SessionEventMap`) are how plugins add new variants.
- **Waterfall semantics**: `ctx.waterfall` listeners receive `(...args, next)`; call `next()` to delegate, or return without it to short-circuit (veto). Never call `next()` after returning.
- **Tests**: vitest in `packages/<name>/tests/*.spec.ts`. Every registry needs an HMR-safety test (register a plugin, dispose its fiber, assert cleanup). Err on the side of more tests — edge cases, error paths, event ordering, races.
- **Plugin export shape — namespace OR default, never both.** A *service* package exports the service class as `export default` (the Loader instantiates it). A *function/namespace* plugin exports `name` / `inject` / `Config` / `apply` as separate named exports and **must NOT add `export default`** — the cordis Loader's `unwrapExports` does `exports.default ?? exports`, so a stray default export collapses the module to the bare `apply` function and silently discards the `inject`/`name`/`Config` namespace, leaving the plugin with no injected services (it then throws `cannot get property … without inject` at load). See [docs/postmortem/0001](../docs/postmortem/0001-acp-default-export-drops-inject.md).
- **Read an optional (non-injected) service via `ctx.get(name, false)`, not `ctx.<name>`.** For a service a plugin reads opportunistically but deliberately leaves out of `static inject` (e.g. `AgentLoop` reading `sessionPersistence`), the `ctx.<name>` property proxy resolves by an ancestor-only fiber walk that throws when the call arrives through a foreign traceable shadow (the service lives on a sibling fiber). `ctx.get(name, false)` is the topology-independent global-store lookup (`false` skips the active-state check). Services that ARE in `static inject` resolve fine via `ctx.<name>`. See [docs/postmortem/0001](../docs/postmortem/0001-acp-default-export-drops-inject.md).
- **Tests**: vitest in `packages/<name>/tests/*.spec.ts`. Every registry needs an HMR-safety test (register a plugin, dispose its fiber, assert cleanup). Err on the side of more tests — edge cases, error paths, event ordering, races. A plugin shipped via `cordis.yml` also needs at least one test that drives it through the REAL Loader/export path (hand-built `ctx.plugin({...})` mounts bypass `unwrapExports` and cannot catch a broken export shape) — see AGENTS.md § Defensive patterns "Line coverage is not behavior coverage". Real-API (with-key) e2e tests are cheap here (we are DeepSeek) and welcome — write many, especially smoke tests; see AGENTS.md § Secrets / .env.
Naming notes:
- Files `src/index.ts` export the service default + all public types
- A *service* `src/index.ts` exports the service class as `export default` + all public types; a *function/namespace plugin* `src/index.ts` exports `name`/`inject`/`Config`/`apply` as named exports and NO default (see the plugin-export-shape rule above)
- `src/types.ts` contain only types — no runtime code
- Tests live at package level under `tests/`, not `src/__tests__/`
- A package's README and module/JSDoc comments are part of the change: when you alter behavior (config keys, defaults, error codes, wire fields), update them in the same commit. CI runs `pnpm run doc-sync`, which typechecks fenced `ts` blocks in `packages/*/README.md` and verifies the event-taxonomy table — but it does NOT cover this file or prose drift (config keys, defaults, error codes), so those stay on the author.

23
packages/acp/src/index.ts
View File

@@ -169,6 +169,17 @@ export function apply(ctx: Context, config: AcpConfig): void {
const agentName = config.agentName ?? 'deepseek-harness-acp'
const agentVersion = config.agentVersion ?? '0.0.1'
// Capture the injected services NOW, during apply(), while we are inside this
// plugin's fiber (where `inject` grants access). The ACP method handlers run
// LATER, from the AgentSideConnection's JSON-RPC read loop — a context that is
// NOT this fiber's injection scope — so reading `ctx.agents` / `ctx.logger` /
// `ctx.sessionPersistence` lazily inside a handler throws "cannot get property
// … without inject". Resolving the references here and closing over them keeps
// the handlers working regardless of which fiber later invokes them.
const agents = ctx.agents
const sessionPersistence = ctx.sessionPersistence
const logger = ctx.logger
// Single live session for the MVP. RFC 011 turns this into maps keyed by
// sessionId plus an agent→sessionId reverse map for the permission gate.
let record: SessionRecord | undefined
@@ -223,7 +234,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
failure (closed pipe), which the in-memory test transport never induces;
the swallow is a defensive best-effort guard like the loop's emit traps */
void Promise.resolve(conn.sessionUpdate(notification)).catch((error: unknown) => {
ctx.logger.warn(`acp: session/update failed: ${String(error)}`)
logger.warn(`acp: session/update failed: ${String(error)}`)
})
}
@@ -374,7 +385,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
}
validateWorkspaceParams(params)
const sessionId = randomUUID()
const agent = ctx.agents.create({
const agent = agents.create({
agentId: sessionId,
sessionId,
meta: { cwd: params.cwd },
@@ -407,13 +418,13 @@ export function apply(ctx: Context, config: AcpConfig): void {
// launched in workspace B: it would replay A's history while tools run
// in B. (If the id is unknown to `list()`, fall through to resume,
// which rejects with the backend's not-found error.)
const meta = (await ctx.sessionPersistence.list()).find(m => m.id === params.sessionId)
const meta = (await sessionPersistence.list()).find(m => m.id === params.sessionId)
if (meta?.cwd !== undefined && meta.cwd !== process.cwd()) {
throw invalidParams(
`session was created in ${meta.cwd}, but the server's launch directory is ${process.cwd()}; honoring a different cwd is not yet supported — launch the server in the session's workspace`,
)
}
const agent = await ctx.agents.resume({
const agent = await agents.resume({
agentId: params.sessionId,
resumeSessionId: params.sessionId,
agentOptions: agentOptions(config),
@@ -577,7 +588,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
mid-run), and there is nothing else to act on once the connection is gone —
the swallow mirrors notify(). */
void conn.closed.then(quiesce).catch((error: unknown) => {
ctx.logger.warn(`acp: connection-close teardown failed: ${String(error)}`)
logger.warn(`acp: connection-close teardown failed: ${String(error)}`)
})
/* v8 ignore stop */
@@ -733,5 +744,3 @@ function toolResultContent(blocks: ContentBlock[]): { type: 'content'; content:
}
return out
}
export default apply

2
packages/acp/tests/dispose.spec.ts
View File

@@ -38,7 +38,7 @@ describe('acp bridge — disposal & HMR safety', () => {
// stay up and the transport is still live. A late session/new must hit the
// `closed` guard and reject — NOT create an agent the disposed bridge can no
// longer stream or settle. Verify the world: no agent appeared.
const harness = await makeBridgeHarness({ storageDir, script: [], childFiber: true })
const harness = await makeBridgeHarness({ storageDir, script: [] })
await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
const before = harness.ctx.agents.list().length
await harness.acpFiber.dispose() // tear down ONLY the bridge

31
packages/acp/tests/harness.ts
View File

@@ -146,8 +146,6 @@ export async function makeBridgeHarness(options: {
script?: (StreamChunk[] | 'hang')[]
config?: Partial<AcpConfig>
storageDir: string
/** Mount the bridge in a disposable child fiber (for the ACP-only-HMR test). */
childFiber?: boolean
} = { storageDir: '' }): Promise<BridgeHarness> {
const adapter = new MockAdapter(options.script ?? [])
@@ -220,21 +218,20 @@ export async function makeBridgeHarness(options: {
// override means "no model at all".
const cfg: AcpConfig = { stream: agentStream, ...options.config }
if (!(options.config && 'model' in options.config)) cfg.model = 'mock'
// By default apply the bridge directly on the root ctx (services ungated). For
// the ACP-only-HMR test, `childFiber: true` mounts it in a CHILD fiber instead
// so the test can dispose JUST the bridge while the rest of the harness stays
// up — its disposer (`harness.acpFiber.dispose()`) tears down only the
// bridge's listeners/effect. (Child-fiber service tracing gates the async
// persistence path, so the load-replay tests use the default direct mount.)
if (options.childFiber) {
harness.acpFiber = await ctx.plugin({
name: 'acp-test',
inject: ['agents', 'sessions', 'sessionPersistence'],
apply: (inner: Context) => { AcpPlugin.apply(inner, cfg) },
})
} else {
AcpPlugin.apply(ctx, cfg)
}
// Mount the bridge the way production does: as a cordis PLUGIN (via
// `ctx.plugin` with the real `inject`), NOT `AcpPlugin.apply(ctx, cfg)`
// directly on the root ctx. The plugin fiber is the faithful reproduction —
// the bridge's `apply` runs inside the fiber's injection scope, and its ACP
// handlers later run from the JSON-RPC read loop OUTSIDE that scope, exactly
// as under the example's cordis.yml. (Mounting directly on root made every
// service an ungated property and hid the "cannot get property … without
// inject" failure that bit a real Zed session.) `harness.acpFiber.dispose()`
// tears down JUST the bridge (its listeners + effect) for the HMR test.
harness.acpFiber = await ctx.plugin({
name: 'acp-test',
inject: ['agents', 'sessions', 'sessionPersistence'],
apply: (inner: Context) => { AcpPlugin.apply(inner, cfg) },
})
harness.client = new ClientSideConnection(makeClient, clientStream)
return harness

20
packages/agent-loop/src/index.ts
View File

@@ -152,12 +152,20 @@ export class AgentLoop extends Service implements AgentFactory {
* by the time this runs the service exists.
*/
async resume(options: ResumeAgentOptions): Promise<Agent> {
const persistence = this.ctx.sessionPersistence
// `sessionPersistence` is declaration-merged onto Context as non-optional,
// but the service is only present when a backend plugin is loaded — and
// AgentLoop deliberately does NOT inject it (that would pend non-persistent
// demos forever). So the runtime value can be undefined; the type cannot.
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
// Read the service through `ctx.get(name, false)` — a direct global-store
// lookup keyed by the isolate symbol — NOT `this.ctx.sessionPersistence`.
// AgentLoop deliberately does NOT inject `sessionPersistence` (injecting it
// would pend non-persistent demos forever). The property proxy resolves a
// service by walking the current fiber's parent chain; from AgentLoop's own
// fiber (which lacks the inject) that walk never reaches the sibling backend
// fiber and throws "cannot get property … without inject". Worse, when the
// call arrives via a traceable shadow (e.g. the ACP bridge child fiber →
// `ctx.agents.resume()` → `this.factory.resume()`), the walk starts at the
// SHADOW's root fiber and fails the same way. `ctx.get(…, false)` sidesteps
// the fiber walk entirely (the same bypass the proxy itself takes when
// `!ctx.fiber.runtime`), so resume works from any caller fiber. `false`
// skips the ACTIVE-state check, since the backend lives on another fiber.
const persistence = this.ctx.get('sessionPersistence', false)
if (persistence === undefined) {
throw new Error('cannot resume: session persistence is not configured (load a dsh-session-persistence backend)')
}