From 66e56bd395bd27ebfa1090068a442963562505cb Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 12:49:59 +0800 Subject: [PATCH] fix review findings: stale coding-agent README, export-shape guards, overclaim wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of PR #88 found three issues in the example-app extraction: A1 — examples/coding-agent/README.md's plugin table still listed the OLD direct-wired leaf entries (agent-loop, session-persistence, src/stdio-chat.ts — the whole src/ dir is gone). Rewrite it to the four real leaf entries the current cordis.yml loads (hmr, llm-deepseek, bash, stdio-agent), noting that tool-bash/persistence/agent/loop now live inside the agent-core + stdio-agent bundles. A2 — the three new app/spine packages (agent-core, stdio-agent, acp-agent) export NO `inject`, so a stray `export default apply` would let unwrapExports collapse the module and silently DROP name/Config WITHOUT crashing — the real-load-path smokes would stay green. agent-core is never Loader-unwrapped at all. Add an explicit export-shape guard per package: assert no `default` export and that the real Loader.unwrapExports leaves name/Config/apply intact. Verified each fails when `export default apply` is added. B — soften "structurally unreachable / cannot wire a stdout logger" overclaims in the acp-agent/agent-core READMEs and the implemented RFC: a leaf CAN still add a sibling logger entry; the accurate claim is the app omits one so the default leaf has nothing to get wrong. Keep the safety directive (never add a stdout logger to an ACP leaf). --- ...2026-06-20-extract-example-app-packages.md | 4 ++-- examples/acp-agent/README.md | 4 ++-- examples/coding-agent/README.md | 11 +++++----- packages/core/agent-core/README.md | 2 +- .../core/agent-core/tests/agent-core.spec.ts | 22 +++++++++++++++++++ packages/ui/acp-agent/README.md | 2 +- packages/ui/acp-agent/tests/acp-agent.spec.ts | 21 ++++++++++++++++++ .../ui/stdio-agent/tests/stdio-agent.spec.ts | 21 ++++++++++++++++++ 8 files changed, 76 insertions(+), 11 deletions(-) diff --git a/docs/rfc/implemented/architecture/2026-06-20-extract-example-app-packages.md b/docs/rfc/implemented/architecture/2026-06-20-extract-example-app-packages.md index eba2d9501b..6ab2c6a15f 100644 --- a/docs/rfc/implemented/architecture/2026-06-20-extract-example-app-packages.md +++ b/docs/rfc/implemented/architecture/2026-06-20-extract-example-app-packages.md @@ -13,7 +13,7 @@ The deeper problem was a **coupled front-door cluster** that lived at the leaf w Each example is now **mostly an invocation of an app package**, splitting the wiring along the existing [interface / implementation / consumer seam](2026-06-13-capability-seams.md): the **app package owns the composition**, the leaf `cordis.yml` owns only the **swappable choices** (which LLM adapter, which bash executor, model, prompt, persistence root). - **`@deepseek-ai/dsh-agent-core`** ([packages/core/agent-core](../../../../packages/core/agent-core)) — a Cordis bundle plugin for the providerless, executor-less, UI-less spine: `timer` + `llm` + sessions + system-prompt + tools + agents + invariants + `tool-bash` + `agent-loop`, mounted as child plugins inside its `apply(ctx)` via `ctx.plugin(...)`. This is the old `base-core.yml` **minus** `bash-local`, **plus** `timer` and the loop, as code instead of a YAML include. The bundle **forwards** `agent-loop`'s `agents` list as its own config (`export const Config = AgentLoop.Config`, default `[]`, the existing `AgentLoop.Config` shape in [packages/core/agent-loop/src/index.ts](../../../../packages/core/agent-loop/src/index.ts)) — so each app supplies its own pre-created agents. This is precisely the reason the old `base-core.yml` gave for keeping `agent-loop` *out* of the shared core ("the examples disagree — stdio needs a pre-created `main`, acp needs none"); forwarding the config dissolves that objection — the loop is shared, the agents list is per-app. The bundle children register into the root service store, so a leaf-mounted sibling (the adapter, the executor) sees them exactly as a nested `plugin-include` subtree's services were seen before. -- **`@deepseek-ai/dsh-stdio-agent`** ([packages/ui/stdio-agent](../../../../packages/ui/stdio-agent)) and **`@deepseek-ai/dsh-acp-agent`** ([packages/ui/acp-agent](../../../../packages/ui/acp-agent)) — app packages, each consuming `dsh-agent-core` and **baking in its coupled front-door cluster**: stdio = `ui-stdio` + console logger + a pre-created `main`; acp = the `acp` bridge + JSONL persistence + **no stdout logger** + no pre-created agents. The coupling becomes structurally unreachable from the leaf. They land under the existing `ui` group alongside `acp`, so no new package group (and no `tsconfig`/`packages/README` group plumbing) was needed. +- **`@deepseek-ai/dsh-stdio-agent`** ([packages/ui/stdio-agent](../../../../packages/ui/stdio-agent)) and **`@deepseek-ai/dsh-acp-agent`** ([packages/ui/acp-agent](../../../../packages/ui/acp-agent)) — app packages, each consuming `dsh-agent-core` and **baking in its coupled front-door cluster**: stdio = `ui-stdio` + console logger + a pre-created `main`; acp = the `acp` bridge + JSONL persistence + **no stdout logger** + no pre-created agents. The leaf no longer carries the cluster, so it has no logger entry to copy wrong by default — the common stdout-purity mistake loses its foothold. (A leaf can still *add* a sibling logger entry — a package cannot forbid what a leaf author writes — so the rule "never add a stdout logger to an ACP leaf" stays documented at the leaf; what changed is that the default leaf has nothing to get wrong.) They land under the existing `ui` group alongside `acp`, so no new package group (and no `tsconfig`/`packages/README` group plumbing) was needed. - **`start.ts` is gone.** Each app package exposes a `bin` (`dsh-stdio-agent` / `dsh-acp-agent`); the `demo:*` scripts invoke it (e.g. `dsh-stdio-agent ./cordis.yml`). The Loader-boot tail, `.env` loading, snapshot-mode selection, and stdin-dispose lifecycle moved into that bin, owned by the app. The `bin.ts` files are coverage-excluded (a self-executing CLI entry, like the old `start.ts`) and driven by the keyless Loader-path tests. - **Each leaf `cordis.yml` collapses** to backends + config: the LLM adapter (`llm-deepseek` with apiKey/models, or `llm-replay`), the bash executor (`bash-local`), `hmr` for the stdio demos (see the amendment below), and one app entry carrying the app's config (model, system prompt, persistence root — surfaced as the app package's own `Config`, which routes each value to wherever the app wires it: stdio onto its pre-created agent, acp onto the bridge plugin). - **echo-agent folds onto `dsh-stdio-agent`**, swapping the LLM backend to the local `mock-llm` and adding the local `echo-tool` (plus `bash-local`, which the spine's `tool-bash` injects) at the leaf — the clean demonstration of "swap the backend, keep the app". `mock-llm.ts` / `echo-tool.ts` stay as example-local teaching plugins. @@ -28,7 +28,7 @@ The proposal listed `hmr` among the stdio app's baked-in front-door cluster. Val 1. `@cordisjs/plugin-hmr` is a Loader-only, subprocess-only dev plugin — its constructor throws without `node --expose-internals` + a live `loader` service, so it can only run in the real `demo:*`/bin subprocess, never in the in-process unit/coverage tier. 2. The in-process test tier (vitest) cannot even *import* the vendored `hmr` module (its class-decorator `@Inject` form fails under Vite's transform), so a package whose `apply` statically imported it could never satisfy the per-file 100% coverage gate on its headline function. -Crucially, `hmr` is **not** a stdout-purity footgun the way the console logger is — a stray `hmr` in the ACP config would not corrupt the JSON-RPC frames — so leaving it at the leaf costs none of the safety the coupling argument is about. The **logger** (the real coupling) stays baked in: the stdio app has it, the ACP app structurally cannot. +Crucially, `hmr` is **not** a stdout-purity footgun the way the console logger is — a stray `hmr` in the ACP config would not corrupt the JSON-RPC frames — so leaving it at the leaf costs none of the safety the coupling argument is about. The **logger** (the real coupling) stays baked in: the stdio app includes it, the ACP app omits it. ## Why not keep the wiring in shared YAML includes? diff --git a/examples/acp-agent/README.md b/examples/acp-agent/README.md index 3f65a2f354..1df36715ec 100644 --- a/examples/acp-agent/README.md +++ b/examples/acp-agent/README.md @@ -6,11 +6,11 @@ The DeepSeek Harness coding agent exposed as an **Agent Client Protocol (ACP)** pnpm run demo:acp # needs DEEPSEEK_API_KEY (repo-root .env or env) ``` -This example is just a leaf `cordis.yml`: it loads the [`@deepseek-ai/dsh-acp-agent`](../../packages/ui/acp-agent) app (which bundles the [`@deepseek-ai/dsh-agent-core`](../../packages/core/agent-core) spine, JSONL session persistence, and the `@deepseek-ai/dsh-acp` bridge — with **no pre-created agents**, since ACP `session/new` creates them on demand) plus the two swappable backends (`llm-deepseek`, `bash-local`). The app package bakes in the no-stdout-logger cluster, so the stdout-purity guarantee is a property of the artifact, not a leaf convention. +This example is just a leaf `cordis.yml`: it loads the [`@deepseek-ai/dsh-acp-agent`](../../packages/ui/acp-agent) app (which bundles the [`@deepseek-ai/dsh-agent-core`](../../packages/core/agent-core) spine, JSONL session persistence, and the `@deepseek-ai/dsh-acp` bridge — with **no pre-created agents**, since ACP `session/new` creates them on demand) plus the two swappable backends (`llm-deepseek`, `bash-local`). The app package bakes in the no-stdout-logger cluster, so a leaf has no logger entry to get wrong by default — keeping stdout pure for JSON-RPC. ## stdout is the protocol -This example loads **no stdout logger** — `stdout` carries the JSON-RPC frames, and any other write corrupts them. `@deepseek-ai/dsh-acp-agent` contains no logger entry, so the footgun is structurally unreachable from this leaf. Use a stderr exporter if you need logs. +This example loads **no stdout logger** — `stdout` carries the JSON-RPC frames, and any other write corrupts them. `@deepseek-ai/dsh-acp-agent` includes no logger entry, so this leaf has none to get wrong by default; do not add one (use a stderr exporter if you need logs). ## Zed configuration diff --git a/examples/coding-agent/README.md b/examples/coding-agent/README.md index 44445adc5d..7585129382 100644 --- a/examples/coding-agent/README.md +++ b/examples/coding-agent/README.md @@ -32,15 +32,16 @@ RESUME_SESSION_ID= pnpm run demo:coding The id is wired through `cordis.yml` (`resumeSessionId: !!js process.env.RESUME_SESSION_ID`); unset, the agent starts a new session. A missing/unreadable id is non-fatal — it logs a warning and starts no `main` agent. -## What each plugin demonstrates +## What each leaf entry demonstrates + +This example is a thin leaf `cordis.yml`: it picks the swappable backends and loads one app package. The spine (sessions, system-prompt, tools, agents, invariants, `agent-loop`) and the front-door cluster (console logger, JSONL persistence, readline UI, the pre-created `main` agent) all live inside the [`@deepseek-ai/dsh-stdio-agent`](../../packages/ui/stdio-agent) app and the [`@deepseek-ai/dsh-agent-core`](../../packages/core/agent-core) bundle it loads — so the leaf has only four entries: | Entry | Demonstrates | |---|---| +| `hmr` (`@cordisjs/plugin-hmr`) | the dev/demo edit-reload loop — a **leaf** entry (not baked into the app) because it is Loader-only and needs `node --expose-internals`, which `demo:coding` passes | | `llm-deepseek` | real `LlmAdapter` via config (`!!js process.env.…` secrets); swap one line to `@deepseek-ai/dsh-llm-pi-ai` for the library-backed twin | -| `bash` (`dsh-bash-local`) + `tool-bash` | the executor seam + tool schemas as separate plugins | -| `agent-loop` | agent created from config with a coding system prompt | -| `session-persistence` (`dsh-session-persistence-jsonl`) | durable JSONL persistence (`root: ./.sessions`): append-only event log per session, crash-safe atomic writes — the shared backend, no per-example file | -| `src/stdio-chat.ts` | UI as a plugin; copied from echo-agent with reasoning-dimming and an exit-on-idle close handler for piped stdin. Example-local on purpose — extract a shared UI package when a third example needs it | +| `bash` (`dsh-bash-local`) | the executor implementation — the swappable half of the bash seam. The model-facing `bash`/`bash_output`/`bash_kill` tool schemas (`tool-bash`) come from `agent-core`, so only the executor is a leaf choice | +| `stdio-agent` (`@deepseek-ai/dsh-stdio-agent`) | the app bundle: the agent-core spine + console logger + JSONL persistence + readline UI + a pre-created `main` agent. Its config carries the model, system prompt, `persistenceRoot` (`./.sessions`), and `resumeSessionId` — so persistence and the agent are configured here, not wired as separate leaf plugins | ## End-to-end tests (`pnpm run test:e2e`, key-gated) diff --git a/packages/core/agent-core/README.md b/packages/core/agent-core/README.md index 1357a6ce8a..28a3592ac6 100644 --- a/packages/core/agent-core/README.md +++ b/packages/core/agent-core/README.md @@ -41,4 +41,4 @@ The bundle FORWARDS `agent-loop`'s `agents` list as its own (default `[]`), so e ## Why a code bundle, not a shared YAML include -A YAML include can dedupe the config, but it cannot OWN a `bin`, and it can only *describe* the front-door coupling in a comment and trust each leaf to obey. Moving the spine into a package, and the front-door cluster into the app packages, turns "the ACP app never logs to stdout" from a prose warning into a property of the artifact. Services register in the root store keyed by their isolate symbol, so a child loaded here is visible to the bundle's siblings (the leaf's adapter and executor) exactly as a nested `plugin-include` subtree's services were — cordis gates every read on `inject`, never on load order. +A YAML include can dedupe the config, but it cannot OWN a `bin`, and it can only *describe* the front-door coupling in a comment and trust each leaf to obey. Moving the spine into a package, and the front-door cluster into the app packages, means the default leaf for an ACP server has no logger entry to copy wrong — "the ACP app never logs to stdout" stops being a prose warning a leaf must remember and becomes the app package's default shape (a leaf can still add a sibling logger, so the rule stays documented — but it has nothing to get wrong by default). Services register in the root store keyed by their isolate symbol, so a child loaded here is visible to the bundle's siblings (the leaf's adapter and executor) exactly as a nested `plugin-include` subtree's services were — cordis gates every read on `inject`, never on load order. diff --git a/packages/core/agent-core/tests/agent-core.spec.ts b/packages/core/agent-core/tests/agent-core.spec.ts index f42f9b399d..67f5d88532 100644 --- a/packages/core/agent-core/tests/agent-core.spec.ts +++ b/packages/core/agent-core/tests/agent-core.spec.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import { Context } from 'cordis' +import Loader from '@cordisjs/plugin-loader' import * as agentCore from '../src/index.ts' import { AgentId } from '@deepseek-ai/dsh-agent' @@ -54,4 +55,25 @@ describe('dsh-agent-core bundle', () => { expect(agentCore.Config).toBeDefined() expect(agentCore.name).toBe('agent-core') }) + + it('has the namespace-plugin export shape (no stray default) so the Loader keeps name/Config/apply', () => { + // Postmortem 0001 guard: a stray `export default apply` makes the Loader's + // `unwrapExports` (`exports.default ?? exports`) collapse the module to the + // bare `apply` function, DROPPING the named `name`/`Config`. This package has + // no `inject` export (it mounts children that carry their own), so that + // collapse would NOT crash at load — the plugin would boot but silently lose + // its config schema. This bundle is also never Loader-unwrapped by any smoke + // (the apps import it directly; the mount test namespace-mounts it), so this + // is its ONLY export-shape guard. Assert directly AND through the real + // `unwrapExports` so adding `export default` to src/index.ts fails here. + expect('default' in agentCore).toBe(false) + expect(typeof agentCore.apply).toBe('function') + + const loader = Object.create(Loader.prototype) as Loader + const unwrapped = loader.unwrapExports(agentCore) as Record + expect(unwrapped).toBe(agentCore) + expect(unwrapped.name).toBe('agent-core') + expect(unwrapped.Config).toBeDefined() + expect(typeof unwrapped.apply).toBe('function') + }) }) diff --git a/packages/ui/acp-agent/README.md b/packages/ui/acp-agent/README.md index 38894f3146..d48ecaa2b3 100644 --- a/packages/ui/acp-agent/README.md +++ b/packages/ui/acp-agent/README.md @@ -16,7 +16,7 @@ stdout is the ACP JSON-RPC channel, so the cluster is defined as much by what it | ~~console logger~~ | **omitted** — it writes to stdout and would corrupt the protocol frames ([the stdout-purity footgun](../acp/README.md)) | | ~~`hmr`~~ | **omitted** — the editor owns the subprocess | -Because there is no logger entry in the package, the footgun is **structurally unreachable from the leaf**: a leaf author cannot wire a stdout logger into the ACP config, because the leaf only picks backends, not the front door. +Because the package wires no logger entry, an ACP leaf has **nothing to get wrong by default**: it only picks backends, so the common mistake — copying a console-logger entry from the stdio config — has no place here. (A leaf author technically *can* still add `@cordisjs/plugin-logger-console` as a sibling entry; the package can't forbid that. So the rule stands: never add a stdout logger to an ACP leaf — stdout is the JSON-RPC channel. Use a stderr exporter if you need logs.) ## Config diff --git a/packages/ui/acp-agent/tests/acp-agent.spec.ts b/packages/ui/acp-agent/tests/acp-agent.spec.ts index 227b4c67b9..7a02837fca 100644 --- a/packages/ui/acp-agent/tests/acp-agent.spec.ts +++ b/packages/ui/acp-agent/tests/acp-agent.spec.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import { Context } from 'cordis' +import Loader from '@cordisjs/plugin-loader' import * as acpAgent from '../src/index.ts' /** @@ -49,4 +50,24 @@ describe('dsh-acp-agent composition', () => { expect(acpAgent.name).toBe('acp-agent') expect(acpAgent.Config).toBeDefined() }) + + it('has the namespace-plugin export shape (no stray default) so the Loader keeps name/Config/apply', () => { + // Postmortem 0001 guard: a stray `export default apply` makes the Loader's + // `unwrapExports` (`exports.default ?? exports`) collapse the module to the + // bare `apply` function, DROPPING the named `name`/`Config`. This package has + // no `inject` export, so that collapse would NOT crash at load (the keyless + // bin smoke would still answer `initialize`) — it would silently lose its + // config schema. So guard the shape directly here: assert no `default` + // export, and that the real `unwrapExports` leaves `name`/`Config`/`apply` + // intact. Adding `export default` to src/index.ts fails this test. + expect('default' in acpAgent).toBe(false) + expect(typeof acpAgent.apply).toBe('function') + + const loader = Object.create(Loader.prototype) as Loader + const unwrapped = loader.unwrapExports(acpAgent) as Record + expect(unwrapped).toBe(acpAgent) + expect(unwrapped.name).toBe('acp-agent') + expect(unwrapped.Config).toBeDefined() + expect(typeof unwrapped.apply).toBe('function') + }) }) diff --git a/packages/ui/stdio-agent/tests/stdio-agent.spec.ts b/packages/ui/stdio-agent/tests/stdio-agent.spec.ts index ab095bda66..f72de0a1da 100644 --- a/packages/ui/stdio-agent/tests/stdio-agent.spec.ts +++ b/packages/ui/stdio-agent/tests/stdio-agent.spec.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest' import { Context } from 'cordis' +import Loader from '@cordisjs/plugin-loader' import { AgentId } from '@deepseek-ai/dsh-agent' import * as stdioAgent from '../src/index.ts' @@ -68,4 +69,24 @@ describe('dsh-stdio-agent app', () => { expect(stdioAgent.name).toBe('stdio-agent') expect(stdioAgent.Config).toBeDefined() }) + + it('has the namespace-plugin export shape (no stray default) so the Loader keeps name/Config/apply', () => { + // Postmortem 0001 guard: a stray `export default apply` makes the Loader's + // `unwrapExports` (`exports.default ?? exports`) collapse the module to the + // bare `apply` function, DROPPING the named `name`/`Config`. This package has + // no `inject` export, so that collapse would NOT crash at load (the keyless + // echo smoke would still boot the tree) — it would silently lose its config + // schema. So guard the shape directly here: assert no `default` export, and + // that the real `unwrapExports` leaves `name`/`Config`/`apply` intact. Adding + // `export default` to src/index.ts fails this test. + expect('default' in stdioAgent).toBe(false) + expect(typeof stdioAgent.apply).toBe('function') + + const loader = Object.create(Loader.prototype) as Loader + const unwrapped = loader.unwrapExports(stdioAgent) as Record + expect(unwrapped).toBe(stdioAgent) + expect(unwrapped.name).toBe('stdio-agent') + expect(unwrapped.Config).toBeDefined() + expect(typeof unwrapped.apply).toBe('function') + }) })