diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..2c5a14b --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,35 @@ +# Types and tests, on every push and every pull request. +# +# There is no build step here — tsx runs TypeScript directly — so nothing checks types +# unless something asks. `yarn typecheck` is that ask, and it carries the compiler flags +# in place of the tsconfig.json this repository deliberately does not have. +name: CI + +on: + push: + branches: [main] + pull_request: + +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-node@v7 + with: + # The active LTS. Node 26 takes that title at the end of October 2026; this is + # pinned rather than `lts/*` so the jump is a commit somebody made on purpose. + node-version: '24' + + # Yarn 4, pinned by the packageManager field; Corepack resolves that version itself. + # Deliberately before the install and without setup-node's cache, which reaches for + # the Yarn 1 shim on the runner and asks it about a Berry cache folder. + - run: corepack enable + + # --immutable: a lockfile that would have to change is a failure, not a fixup. + - run: yarn install --immutable + + - run: yarn typecheck + + - run: yarn test diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml new file mode 100644 index 0000000..12bff86 --- /dev/null +++ b/.github/workflows/docs.yml @@ -0,0 +1,49 @@ +# Publishes docs/ to GitHub Pages. +# +# The site is the Markdown already in the repository, rendered by the same Jekyll that +# Pages runs by itself — no generator, no second copy of the documentation, and nothing +# to keep in step with it. Requires Pages to be set to "GitHub Actions" in the repository +# settings; the workflow cannot turn that on for you. +name: Docs + +on: + push: + branches: [main] + paths: + - 'docs/**' + - '.github/workflows/docs.yml' + workflow_dispatch: + +permissions: + contents: read + pages: write + id-token: write + +# One deployment at a time, and a queued one waits rather than replacing what is live. +concurrency: + group: pages + cancel-in-progress: false + +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + # These three move together: the artifact format they agree on is what ties a build + # to the deployment that consumes it, so bumping one alone is how Pages jobs break. + - uses: actions/configure-pages@v6 + - uses: actions/jekyll-build-pages@v1 + with: + source: ./docs + destination: ./_site + - uses: actions/upload-pages-artifact@v5 + + deploy: + needs: build + runs-on: ubuntu-latest + environment: + name: github-pages + url: ${{ steps.deployment.outputs.page_url }} + steps: + - id: deployment + uses: actions/deploy-pages@v5 diff --git a/AGENTS.md b/AGENTS.md index 0b90ee8..cf9b948 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,6 +22,7 @@ yarn raw # the same protocol over bare curl, no SDK yarn tap # wire-tap proxy :41242 → :41241 yarn mcp # MCP endpoint on :41243 fronting the A2A agent, for a calling agent yarn service # install and drive the whole thing as a background service +yarn typecheck # tsc --noEmit with the flags that stand in for tsconfig.json ``` All three servers listen on the same port and serve the same protocol, so `client`, `raw` and @@ -116,6 +117,23 @@ End-to-end verification is unchanged and still worth running: `yarn client` agai checking the cycle `SUBMITTED → INPUT_REQUIRED → WORKING → artifact → COMPLETED`. Full write-up: [docs/testing.md](docs/testing.md). +## Continuous integration + +`.github/workflows/ci.yml` runs `yarn typecheck` and `yarn test` on every push and every pull +request. The typecheck is there because there is no build step: `tsx` transpiles and throws the +types away, so nothing checks them unless something asks, and the flags that would live in a +`tsconfig.json` live in that script instead. It is expected to pass — the tree is at zero +diagnostics — so a new one is a regression rather than a number to compare against. + +`.github/workflows/docs.yml` publishes `docs/` to GitHub Pages through the same Jekyll that Pages +runs anyway, so the site is the Markdown in this repository rather than a second copy of it. It +needs Pages set to "GitHub Actions" in the repository settings; the workflow cannot turn that on +by itself. + +Corepack is enabled before the install, because that is what resolves the Yarn version pinned in +`packageManager`. `setup-node`'s cache is deliberately not used: it reaches for the Yarn 1 shim on +the runner and asks it about a Berry cache folder, which is global here anyway. + ## Architecture Each server is an entry point that reads the environment, builds an Agent Card and binds a port; @@ -422,6 +440,14 @@ On the ACP side, verified the same way: - `ClientSideConnection` is deprecated in SDK 1.4.0; the current API is `acp.client({name}).onRequest(...).connect(stream)`. +## Changelog + +`CHANGELOG.md` at the root, Keep a Changelog format. A change to behaviour, configuration or the +protocol surface gets an entry in the same commit that makes it — one sentence on what changed, +and a second on why when the reason is not obvious from the first. Refactors, comments and +documentation edits do not. While a version is unreleased the entry goes under its heading; +once it has gone out, open an `## [Unreleased]` above it rather than editing what shipped. + ## Longer prose `docs/` carries the human-facing documentation: architecture, both protocols, the permission @@ -430,7 +456,17 @@ working guide, not the explanation. ## Source of truth -The specification outranks both the SDK and this file: . -The SDK implements v1.0.0; when behavior disagrees, check the spec rather than guessing from the +Three protocols meet here, and each has a specification that outranks both its SDK and this file. +The versions are the ones this repository actually speaks — read off the handshakes, not off the +package names: + +| Protocol | Specification | What we speak | +|---|---|---| +| **A2A** | | v1.0.0, via `@a2a-js/sdk` 1.2.0 | +| **ACP** | | `PROTOCOL_VERSION` 1, via `@agentclientprotocol/sdk` 1.4.0 | +| **MCP** | | `2025-06-18`, the version our `initialize` answers with, via `@modelcontextprotocol/server` 2.0.0 | + +For A2A in particular: when behavior disagrees, check the spec rather than guessing from the bundle — `dist` also ships the v0.3 compatibility layer, and its constants are easy to mistake for -the current ones. +the current ones. The same caution applies to ACP, where the SDK's own version (1.4.0) and the +protocol's (1) are different numbers that both appear in the handshake. diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..3f90ff5 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,97 @@ +# Changelog + +Notable changes, newest first. The format follows +[Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and the numbers follow +[Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +Entries say what changed and, where the reason is not obvious from the change, why — this +repository exists to be read, and a list of verbs is no use to whoever comes next. Findings +referenced as `F01`, `F09` and so on are from [docs/repository-audit.md](docs/repository-audit.md), +which records them in full along with the evidence. + +## [0.1.0] — 2026-09-21 + +The first version worth a number, and an honest one: tasks live in an `InMemoryTaskStore`, the +A2A side runs `UserBuilder.noAuthentication`, and the permission classifier is a placeholder for +an external channel — a human, a policy, or a supervising agent. + +### Added + +- **An A2A v1.0 server and client** (`src/agent.ts`, `src/client.ts`) — Agent Card discovery, + streaming, `INPUT_REQUIRED` and the artifact cycle, with a placeholder executor (the "Revisor") + that computes text statistics and nothing else. +- **The same protocol over bare curl** (`src/raw.sh`) and **a wire-tap** (`src/proxy.ts`) that + prints raw JSON-RPC and SSE frames, because the SDK surfaces parsed objects and what you need to + see is the wire. +- **The ACP bridge** (`src/acp/`) — the same A2A server with its executor replaced by a real + coding agent addressed over stdin/stdout: one adapter process per `contextId`, a permission + classifier that answers instead of the calling agent, and two rotating JSON Lines logs. +- **The MCP bridge** (`src/mcp/`) — the A2A agent published to a calling agent as four tools over + Streamable HTTP, bearer-guarded, with progress ticks derived from upstream liveness. + `a2a_ask` takes a `taskId` as well as a `contextId`, which is the only way to answer an + `INPUT_REQUIRED` question rather than open a second task beside it, and every call reports the + task and context it reached so an interrupted one can still be recovered (`F09`, `F10`). +- **Deadlines on everything that does not stream.** The silence gate covers `a2a_ask`; + `MCP_DISCOVERY_TIMEOUT_MS` (20 s) bounds the shared agent handshake and + `MCP_REQUEST_TIMEOUT_MS` (30 s) bounds `a2a_task`, `a2a_cancel` and `a2a_card`. Discovery is + bounded separately because it is shared: a caller's abort ends only its own wait, while the + deadline ends the handshake for everyone (`F11`). +- **`bin/ambassyctl`** — both processes installed and driven as background services, LaunchAgents + on macOS and `systemd --user` units on Linux. +- **`docs/`** — architecture, both protocols, the permission model, configuration, logging, + testing, troubleshooting, and the repository audit, published to GitHub Pages by + `.github/workflows/docs.yml` through the same Jekyll that Pages runs anyway. +- **A test suite** — `yarn test` runs 288 vitest cases over the paths that are hard to provoke by + hand: a cancel that arrives before the session exists, a log that rotates mid-record, a stream + that stops before its task reaches a state. Nothing is mocked at the module level; every + substitution goes through an argument, so the seams stay visible in the production code. + [docs/testing.md](docs/testing.md) says what they reach and what they leave alone. +- **`yarn typecheck` and CI.** There is no build step, so nothing checked types unless asked; the + script carries the compiler flags in place of the `tsconfig.json` this repository deliberately + lacks, and `.github/workflows/ci.yml` runs it with the tests on every push and pull request. + +### Fixed + +Found by the repository audit and closed before this version went out. + +- **Artifacts aggregate instead of concatenating (`F12`).** Two replacements of one artifact, + `old` then `new`, produced `oldnew`, and artifacts that were never the same artifact merged. + They are now keyed by `artifactId`, honour `append`, are seeded from the task snapshot, and keep + every data part. +- **A stream that loses its ending is an error (`F13`).** One that died mid-turn came back as a + tidy result saying the agent "finished in `TASK_STATE_WORKING`". A turn now reports an outcome — + `task`, `message`, `interrupted` or `truncated` — as well as a state. +- **The agent's last word survives (`F14`).** A refusal from the ACP bridge travels in a status + message and nowhere else, and it was dropped for everything but `INPUT_REQUIRED`. It is kept now + and reported by `a2a_ask` and `a2a_task`; `a2a_cancel` renders like a read rather than discarding + what the task had already produced. +- **Leaving plan mode is not a refusal.** The `switch_mode` rule denied Claude's `ExitPlanMode`, + which the adapter turns into an interrupt and reports as a cancellation — so a task that planned + before acting would have died as it was ready to start. The destination is in the option ids, + never in `rawInput`, and is read from an explicit table of both adapters' effects. +- **A deadline that is not a number falls back.** `Number(process.env.…)` of a typo is `NaN`, and + `AbortSignal.timeout(NaN)` throws rather than ignoring it, which took out every call the + deadline was meant to protect. +- **`..notes` is a filename (`F22`).** The containment check refused any relative path beginning + with two dots, which is a climb out only when the next character is a separator. +- **17 type errors in running code.** Found by the new type check: stream payload fields read as + though a protobuf message field were ever required, an exhausted union asked for its `$case`, + adapters declared with three pipes and spawned with two. +- **`ACP_CWD` must be a directory**, not merely a path that exists. + +### Security + +- **A conversation's sandbox is not named after the caller (`F01`).** The A2A `contextId` is text + the caller chooses, and joining it onto `.acp-sandboxes/` made `contextId: ".."` resolve to the + repository root — returned as `owned: true`, the flag that lets the classifier pass `execute` + and `fetch`. Directories come from `mkdtemp`, which fails unless the directory is new, with an + explicit context-to-directory map and a containment check on reuse. +- **A session that cannot be supervised is not used (`F08`).** When the supervised mode was + unavailable, the bridge logged the miss and carried on in whatever mode the adapter chose — for + both backends, one where it approves its own tool calls. It now fails the session and disposes + the half-started adapter. `switch_mode` is its own classifier case, decided on the option the + adapter offered rather than on who owns the directory. +- **The wire-tap binds loopback (`F02`).** It called `listen(PORT)` with no host, which opened a + wider door than the unauthenticated agent it fronts. It reads `HOST` now, like the two servers. + +[0.1.0]: https://github.com/Flopsstuff/ambassy/commits/main diff --git a/README.md b/README.md index e3c6b43..5cbbc8d 100644 --- a/README.md +++ b/README.md @@ -187,7 +187,15 @@ The official samples worth reading first — the SDK's own README points at them `authentication` (Bearer/JWT via Passport), `verify-signing` (signed cards, JWS + JWKS), and `extensions` (protocol extensions). -Specification: https://a2a-protocol.org/v1.0.0/specification/ +Specifications, one per protocol this repository speaks: + +- A2A v1.0.0 — +- ACP — +- MCP `2025-06-18` — + +## Changes + +What changed and why, release by release: [CHANGELOG.md](CHANGELOG.md). ## License diff --git a/docs/README.md b/docs/README.md index 305c42d..ec9d918 100644 --- a/docs/README.md +++ b/docs/README.md @@ -5,13 +5,15 @@ A2A interface, but answered by a real coding agent over **ACP** instead of by a Three protocols meet here, and the first two are easy to confuse because both are JSON-RPC: -- **A2A** (Agent2Agent) is horizontal — one agent delegates a *task* to another over HTTP. The - task is addressable, has a lifecycle, and survives the call that created it. -- **ACP** (Agent Client Protocol) is what an editor speaks to a coding agent over stdin/stdout. - There the roles are reversed: the agent is a child process, and we are the client. - -- **MCP** (Model Context Protocol) points the other way: it is how a calling agent reaches tools, - and it is what publishes this A2A agent to Claude Code as something callable. +- **[A2A](https://a2a-protocol.org/v1.0.0/specification/)** (Agent2Agent) is horizontal — one + agent delegates a *task* to another over HTTP. The task is addressable, has a lifecycle, and + survives the call that created it. +- **[ACP](https://agentclientprotocol.com/protocol/overview)** (Agent Client Protocol) is what an + editor speaks to a coding agent over stdin/stdout. There the roles are reversed: the agent is a + child process, and we are the client. +- **[MCP](https://modelcontextprotocol.io/specification/2025-06-18)** (Model Context Protocol) + points the other way: it is how a calling agent reaches tools, and it is what publishes this A2A + agent to Claude Code as something callable. The bridge sits between them. Everything in these pages was checked against running code, not inferred from specifications — where behaviour and documentation disagreed, the observed diff --git a/docs/_config.yml b/docs/_config.yml new file mode 100644 index 0000000..e199537 --- /dev/null +++ b/docs/_config.yml @@ -0,0 +1,13 @@ +# Jekyll's share of publishing docs/ — deliberately four settings and no site to maintain. +title: Ambassy +description: A learning sandbox for the A2A protocol, and a bridge from it to a real coding agent. +theme: jekyll-theme-primer + +# The documents link to each other as files — [permissions](permissions.md) — because they +# are read in the repository as often as on the web. This rewrites those links to the +# rendered pages, so one set of links works in both places. +plugins: + - jekyll-relative-links +relative_links: + enabled: true + collections: false diff --git a/docs/acp-bridge.md b/docs/acp-bridge.md index 5809503..bc52dbf 100644 --- a/docs/acp-bridge.md +++ b/docs/acp-bridge.md @@ -1,5 +1,9 @@ # The ACP bridge +Specification: . The SDK is +`@agentclientprotocol/sdk` 1.4.0, and the protocol version it negotiates is `1` — two different +numbers that both travel in the same handshake, so it is worth knowing which is which. + `src/acp/agent.ts` is the A2A server from `src/agent.ts` with the placeholder executor replaced by one that forwards the task to a real coding agent — `AcpExecutor`, in `src/acp/executor.ts`. The backend is chosen by the launch command: diff --git a/docs/mcp-bridge.md b/docs/mcp-bridge.md index f807d34..b3d99cc 100644 --- a/docs/mcp-bridge.md +++ b/docs/mcp-bridge.md @@ -1,5 +1,9 @@ # The MCP bridge +Specification: — the version this +bridge answers `initialize` with. The SDK is `@modelcontextprotocol/server` 2.0.0, over +Streamable HTTP. + A third protocol joins the two others, and it points the other way. A2A and ACP are both about reaching *down* to an agent that does work. MCP is how a calling agent — Claude Code, here — reaches *out* to tools. This bridge publishes the A2A agent as four such tools, so calling it diff --git a/docs/repository-audit.md b/docs/repository-audit.md index 403b652..0064619 100644 --- a/docs/repository-audit.md +++ b/docs/repository-audit.md @@ -35,6 +35,22 @@ cancellation routing; it does not establish behavior of the actual model adapter The MCP HTTP transport, actual adapter authentication, model execution, and a dependency vulnerability scan were not exercised. +## What has been done since + +The findings are being answered in the code, not edited out of this document: the record of +what was found stays as it was written, and each entry carries a **Status** line saying where +it was answered. As of `109fb67`: + +| | Findings | +|---|---| +| Closed | F01, F02, F08, F09, F10, F11, F12, F13, F14, F22 | +| Partly | F18 (the two MCP deadlines), F21 (typecheck and CI, not the smoke exit codes) | +| Open | F03–F07, F15, F16, F17, F19, F20, F23 | + +The open ones are not a leftover pile. F04–F07 are process and task ownership, which the +implementation order below deliberately groups: they share the adapter registry, and fixing +one of them alone moves the bug rather than removing it. + Priority meanings: **P0** = isolation boundary bypass; **P1** = fix before routine bridge use; **P2** = correctness or reliability work; **P3** = smaller edge case. Evidence is marked **reproduced** or **source review**. Source locations refer to the reviewed commit. @@ -43,6 +59,8 @@ Priority meanings: **P0** = isolation boundary bypass; **P1** = fix before routi ### F01 — P0: Client-controlled context IDs become trusted filesystem roots +**Status:** Closed by `3ddd7d4`. Directories come from `mkdtemp` under the sandbox parent, with an explicit context-to-directory map and a containment check on reuse; nothing is derived from the context id but the label on the name. + **Location:** [src/acp/client.ts](../src/acp/client.ts), `boundaryFor`, lines 284–301. **Evidence:** reproduced, plus live verification that the A2A SDK accepts `contextId: ".."`. @@ -64,6 +82,8 @@ command or write a file outside its disposable fixture. ### F02 — P1: The unauthenticated A2A server binds beyond localhost +**Status:** Closed by `3ddd7d4`. The two A2A servers had already gained `HOST` by the time this was acted on; the wire-tap binds it now as well, and `TARGET_HOST` names what it forwards to. + **Location:** [src/acp/agent.ts](../src/acp/agent.ts), lines 609–623; [src/agent.ts](../src/agent.ts), lines 248–260; [src/proxy.ts](../src/proxy.ts), line 58. **Evidence:** source review. @@ -83,6 +103,8 @@ Verify the actual bound address, not just the startup message. ### F03 — P1: Copying the supplied environment template breaks startup +**Status:** Open. + **Location:** [.env.example](../.env.example), lines 20 and 32; [src/acp/agent.ts](../src/acp/agent.ts), lines 51–62; [src/mcp/server.ts](../src/mcp/server.ts), lines 40–68. **Evidence:** reproduced. @@ -100,6 +122,8 @@ and a valid public URL. Test this in isolation without overwriting the operator' ### F04 — P1: Concurrent acquisition creates two adapters for one context +**Status:** Open. + **Location:** [src/acp/client.ts](../src/acp/client.ts), `acquire`, lines 305–364. **Evidence:** reproduced with two simultaneous acquisitions and a fake ACP adapter. @@ -117,6 +141,8 @@ A failed acquisition can be retried, and shutdown leaves no pending or hidden ch ### F05 — P1: Failed startup leaks adapter processes; startup has no deadline +**Status:** Open. The one piece taken early: `acquire` disposes a half-started adapter, because F08 cannot be honoured without it. + **Location:** [src/acp/client.ts](../src/acp/client.ts), lines 127–135, 250–281, 314–364. **Evidence:** failed acquisition reproduced; missing deadlines found by source review. @@ -136,6 +162,8 @@ also completes when spawn fails. ### F06 — P1: Canceling a queued task cancels the task currently running +**Status:** Open. + **Location:** [src/acp/agent.ts](../src/acp/agent.ts), lines 143–161, 199–241; [src/acp/client.ts](../src/acp/client.ts), lines 193–214. **Evidence:** reproduced with controlled turns. @@ -153,6 +181,8 @@ Also cover cancellation during acquisition and a cancel racing normal completion ### F07 — P1: Canceling an INPUT_REQUIRED task hangs +**Status:** Open. + **Location:** [src/agent.ts](../src/agent.ts), lines 114–118, 137–153; [src/acp/agent.ts](../src/acp/agent.ts), lines 143–161, 202–218, 257–263. **Evidence:** live Revisor reproduction and ACP executor bookkeeping fixture. @@ -175,6 +205,8 @@ executors and cancellation during startup with empty input. ### F08 — P1: Missing supervised modes silently retain an unsafe default +**Status:** Closed by `3ddd7d4`. A supervised mode that cannot be established fails the session and takes the adapter with it, and `switch_mode` is decided on the option the adapter offered — which is where both adapters put the destination, `rawInput` holding only the plan. + **Location:** [src/acp/client.ts](../src/acp/client.ts), `superviseMode`, lines 367–385. **Evidence:** reproduced with an adapter offering only `auto`. @@ -192,6 +224,8 @@ Known supported modes continue to work. ### F09 — P1: MCP cannot resume a task that needs more input +**Status:** Closed by `109fb67`. `taskId` runs through the tool schema, the pool and the outgoing message; the result names the task to reply into, and omitting it still opens new work in the same context. + **Location:** [src/mcp/a2a.ts](../src/mcp/a2a.ts), lines 71–79, 119–143; [src/mcp/tools.ts](../src/mcp/tools.ts), lines 54–60, 99–105. **Evidence:** live reproduction. @@ -210,6 +244,8 @@ task, and eventually releases its open-task reference. See the specification's ### F10 — P1: The advertised recovery path loses the task handle on errors +**Status:** Closed by `109fb67`. Identity travels with the first progress notification, with a typed `TurnError` and with the silence timeout; a caller that hung up is recorded as `mcp.ask.abandoned`. + **Location:** [src/mcp/a2a.ts](../src/mcp/a2a.ts), `AskUpdate` and `ask`; [src/mcp/tools.ts](../src/mcp/tools.ts), lines 138–145, 155–158. **Evidence:** source review. @@ -229,6 +265,8 @@ recovery path. A silence timeout includes the known agent, task ID, and context ### F11 — P1: Discovery bypasses the MCP abort gate +**Status:** Closed by `109fb67`. `MCP_DISCOVERY_TIMEOUT_MS` bounds the shared handshake and `MCP_REQUEST_TIMEOUT_MS` the three request/response tools; a caller's abort ends only its own wait. + **Location:** [src/mcp/a2a.ts](../src/mcp/a2a.ts), lines 103–109, 124–125, 184–205; [src/mcp/tools.ts](../src/mcp/tools.ts), lines 115–145. **Evidence:** reproduced with stalled HTTP discovery. @@ -246,6 +284,8 @@ tool call indefinitely. Verify the same behavior for each of the four tools and ### F12 — P2: MCP corrupts replacement artifacts and ignores snapshot content +**Status:** Closed by `109fb67`. Artifacts aggregate by `artifactId` honouring `append`, seeded from the task snapshot, with every data part kept. + **Location:** [src/mcp/a2a.ts](../src/mcp/a2a.ts), lines 150–170. **Evidence:** reproduced with an injected SDK stream. @@ -264,6 +304,8 @@ multiple data parts, and a direct Message response. ### F13 — P2: An incomplete stream is rendered as a successful tool result +**Status:** Closed by `109fb67`. A turn reports an outcome as well as a state, and `truncated` — a stream that stopped before any ending — is an error carrying the recovery identity. + **Location:** [src/mcp/a2a.ts](../src/mcp/a2a.ts), lines 146–181; [src/mcp/tools.ts](../src/mcp/tools.ts), lines 66–69. **Evidence:** reproduced with an injected stream. @@ -280,6 +322,8 @@ remain supported, and interrupted states tell the caller what action is needed. ### F14 — P2: Final failure reasons and recovery data disappear in MCP results +**Status:** Closed by `109fb67`. The last status message is retained and reported by `a2a_ask` and `a2a_task`, and `a2a_cancel` renders like a read rather than dropping what the task produced. + **Location:** [src/mcp/a2a.ts](../src/mcp/a2a.ts), lines 155–162; [src/mcp/tools.ts](../src/mcp/tools.ts), lines 46–83. **Evidence:** source review. @@ -297,6 +341,8 @@ its text, structured result, and final status explanation. ### F15 — P2: Agent Cards advertise part labels instead of media types +**Status:** Open. + **Location:** [src/agent.ts](../src/agent.ts), lines 230–240; [src/acp/agent.ts](../src/acp/agent.ts), lines 578–588; [docs/a2a.md](a2a.md), discovery example. **Evidence:** live card and specification check. @@ -312,6 +358,8 @@ the example. See the specification's ### F16 — P2: MCP and ACP rotate the same logs independently +**Status:** Open. + **Location:** [src/acp/agent.ts](../src/acp/agent.ts), lines 58–62; [src/mcp/server.ts](../src/mcp/server.ts), lines 47–68; [src/acp/log.ts](../src/acp/log.ts), lines 41–85. **Evidence:** source review. @@ -329,6 +377,8 @@ accounted for within each service's documented retention window. ### F17 — P2: Logging can still throw into bridge control flow +**Status:** Open. + **Location:** [src/acp/log.ts](../src/acp/log.ts), lines 50–60 and 89–92. **Evidence:** reproduced with an invalid log destination and a non-JSON field. @@ -346,6 +396,8 @@ without repeated exceptions or accidental task failure. ### F18 — P2: Environment values are cast instead of validated +**Status:** Partly narrowed by `109fb67`: the two MCP deadlines fall back to their defaults rather than throwing out of `AbortSignal.timeout`, and `ACP_CWD` must now be a directory rather than merely exist. Everything else in this finding stands. + **Location:** [src/acp/agent.ts](../src/acp/agent.ts), lines 44–60; [src/acp/client.ts](../src/acp/client.ts), lines 286–291; [src/mcp/server.ts](../src/mcp/server.ts), lines 40–59, 73–78. **Evidence:** source review. @@ -366,6 +418,8 @@ Explicitly test `null`, `[]`, inherited property names, blank values, and non-lo ### F19 — P2: Every ACP -32000 error is mislabeled as authentication failure +**Status:** Open. + **Location:** [src/acp/agent.ts](../src/acp/agent.ts), `describeError`, lines 118–124; [docs/troubleshooting.md](troubleshooting.md), authentication section. **Evidence:** source review. @@ -381,6 +435,8 @@ failure reports its actual cause. ### F20 — P2: The proxy does not manage stream backpressure or disconnects +**Status:** Open. + **Location:** [src/proxy.ts](../src/proxy.ts), lines 22–55. **Evidence:** source review. The request body is buffered without a limit. Response chunks are written regardless of @@ -398,6 +454,8 @@ does not cause unbounded buffering, and an upstream reset settles the downstream ### F21 — P2: Verification does not detect the existing TypeScript and failure-path defects +**Status:** Partly closed. `yarn typecheck` carries the flags this document used, CI runs it on every push and pull request, and the source tree is at zero diagnostics. Still open: the smoke client returns 0 when its first turn does not reach `INPUT_REQUIRED`, and offline regression coverage of the lifecycle defects. + **Location:** [package.json](../package.json), scripts; [src/acp/client.ts](../src/acp/client.ts), lines 106–124, 255–269; [src/client.ts](../src/client.ts), lines 84–108. **Evidence:** compiler run and source review. @@ -421,6 +479,8 @@ fail before their corresponding fixes. Do not make paid model calls part of rout ### F22 — P3: Valid filenames starting with two dots are refused +**Status:** Closed by `3ddd7d4`. `insideRoot` refuses `..` and anything beginning `../`, and nothing else. + **Location:** [src/acp/permissions.ts](../src/acp/permissions.ts), lines 86–88. **Evidence:** reproduced: `insideRoot(process.cwd(), "..notes")` returned false. @@ -434,6 +494,8 @@ and symlink escapes fail. ### F23 — P3: LOG_MAX_FILES=1 retains an extra file +**Status:** Open. + **Location:** [src/acp/log.ts](../src/acp/log.ts), `rotate`, lines 78–85. **Evidence:** reproduced with `maxFiles: 1` and two records forcing rotation. @@ -489,6 +551,8 @@ additional concrete boundary failures despite those limitations. ## Suggested implementation order +Steps 1 and 3 have landed, minus F03; step 5's typecheck is in CI and the tree is clean. + 1. **Restore safe defaults:** F01–F03 and F08. Add boundary and template regression fixtures. 2. **Fix process/task ownership together:** F04–F07, then turn bookkeeping and shutdown. 3. **Make MCP continuation and recovery real:** F09–F14, with bounded upstream operations. diff --git a/package.json b/package.json index 7e336a8..a9c25b7 100644 --- a/package.json +++ b/package.json @@ -7,6 +7,7 @@ "test": "vitest run", "test:watch": "vitest", "test:coverage": "vitest run --coverage", + "typecheck": "tsc --noEmit --module nodenext --moduleResolution nodenext --target es2023 --lib es2023,dom --allowImportingTsExtensions --skipLibCheck --strict --types node,express src/*.ts src/acp/*.ts src/mcp/*.ts", "agent": "tsx src/agent.ts", "agent:claude": "ACP_AGENT=claude tsx src/acp/agent.ts", "agent:codex": "ACP_AGENT=codex tsx src/acp/agent.ts", diff --git a/src/acp/client.ts b/src/acp/client.ts index 7148c32..7ba3ea1 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -6,7 +6,7 @@ * editor plays. This file owns that side: spawning adapters, opening sessions, * keeping one turn in flight at a time, and reaping processes that went quiet. */ -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import { spawn, type ChildProcessByStdio } from 'node:child_process'; import { join } from 'node:path'; import { Readable, Writable } from 'node:stream'; import { fileURLToPath } from 'node:url'; @@ -78,8 +78,17 @@ const CLIENT_CAPABILITIES = { terminal: false, }; +/** + * An adapter as it is actually spawned: two pipes and an inherited stderr. + * + * Not `ChildProcessWithoutNullStreams`, which promises three — stderr is inherited on + * purpose, so the adapter's own diagnostics land in the terminal beside ours rather than + * in a buffer nobody reads. + */ +type Adapter = ChildProcessByStdio; + interface Connected { - child: ChildProcessWithoutNullStreams; + child: Adapter; conn: ClientConnection; } @@ -132,7 +141,7 @@ const connect = (backend: Backend, cwd: string, supervise: () => Supervision): C * `error` counts as an ending too: a child that never spawned — a missing binary — emits * no `exit`, and waiting for one would hang the caller cleaning up after it. */ -const stop = async (child: ChildProcessWithoutNullStreams): Promise => { +const stop = async (child: Adapter): Promise => { if (child.exitCode !== null || child.signalCode !== null) return; const exited = new Promise((done) => { child.once('exit', () => done()); @@ -192,7 +201,7 @@ export class Runtime { constructor( readonly contextId: string, readonly boundary: Boundary, - private readonly child: ChildProcessWithoutNullStreams, + private readonly child: Adapter, private readonly conn: ClientConnection, readonly session: ActiveSession, /** The very object the permission handlers read from — shared, not copied. */ @@ -303,11 +312,13 @@ export class AcpRegistry { const { child, conn } = connect(this.opts.backend, process.cwd(), () => ({ boundary })); const started = Date.now(); try { - const result = await conn.agent.request(acp.methods.agent.initialize, { + // The SDK types `request` by method only for its own method constants; this one + // comes back as `unknown`, and the response shape is what the probe exists to read. + const result = (await conn.agent.request(acp.methods.agent.initialize, { protocolVersion: acp.PROTOCOL_VERSION, clientCapabilities: CLIENT_CAPABILITIES, clientInfo: { name: CLIENT_NAME, version: '0.1.0' }, - }); + })) as InitializeResponse; this.opts.logs.call('handshake', { backend: this.opts.backend.id, bin: this.opts.backend.bin, diff --git a/src/client.ts b/src/client.ts index ef4907e..ba340b4 100644 --- a/src/client.ts +++ b/src/client.ts @@ -6,7 +6,7 @@ * Turn 2 — we send the text into THE SAME task → the agent drives it to COMPLETED. * That is the difference between A2A and a plain "POST /do": the call has state. */ -import { Role, TaskState, type Message, type Part, type StreamResponse } from '@a2a-js/sdk'; +import { Role, TaskState, type Message, type Part, type StreamResponse, type TaskStatus } from '@a2a-js/sdk'; import { ClientFactory } from '@a2a-js/sdk/client'; const BASE_URL = process.env.AGENT_URL || 'http://localhost:41241'; @@ -33,26 +33,36 @@ const renderParts = (parts: Part[]): string => }) .join('\n'); +// `status` and `artifact` are optional in the generated types, because every message +// field of a protobuf is. One helper keeps that fact out of the printing below. +const stateOf = (status: TaskStatus | undefined): TaskState => status?.state ?? TaskState.TASK_STATE_UNSPECIFIED; + /** Prints a stream event and returns whatever task identifiers it carried. */ function render(event: StreamResponse): { taskId?: string; contextId?: string; state?: TaskState } { const p = event.payload; switch (p?.$case) { case 'task': - console.log(` [task] id=${p.value.id.slice(0, 8)} state=${TaskState[p.value.status.state]}`); - return { taskId: p.value.id, contextId: p.value.contextId, state: p.value.status.state }; + console.log(` [task] id=${p.value.id.slice(0, 8)} state=${TaskState[stateOf(p.value.status)]}`); + return { taskId: p.value.id, contextId: p.value.contextId, state: stateOf(p.value.status) }; case 'statusUpdate': { - const note = p.value.status.message ? ` — ${renderParts(p.value.status.message.parts)}` : ''; - console.log(` [statusUpdate] ${TaskState[p.value.status.state]}${note}`); - return { taskId: p.value.taskId, contextId: p.value.contextId, state: p.value.status.state }; + const message = p.value.status?.message; + const note = message ? ` — ${renderParts(message.parts)}` : ''; + console.log(` [statusUpdate] ${TaskState[stateOf(p.value.status)]}${note}`); + return { taskId: p.value.taskId, contextId: p.value.contextId, state: stateOf(p.value.status) }; } - case 'artifactUpdate': - console.log(` [artifactUpdate] «${p.value.artifact.name}»\n${renderParts(p.value.artifact.parts)}`); + case 'artifactUpdate': { + const artifact = p.value.artifact; + const body = artifact ? `«${artifact.name}»\n${renderParts(artifact.parts)}` : '(an update with no artifact)'; + console.log(` [artifactUpdate] ${body}`); return { taskId: p.value.taskId, contextId: p.value.contextId }; + } case 'message': console.log(` [message] ${renderParts(p.value.parts)}`); return {}; default: - console.log(` [${p?.$case ?? 'unknown'}] ${JSON.stringify(p)}`); + // The union is exhausted above, so what lands here is either an absent payload or a + // case from a newer protocol than these branches know: nothing to name it by. + console.log(` [unrecognised] ${JSON.stringify(p)}`); return {}; } } @@ -104,7 +114,7 @@ async function main() { // The task outlived both calls and sits on the server in full — history and artifacts included. const task = await client.getTask({ tenant: '', id: taskId, historyLength: 0 }); console.log( - `\n── Result: GetTask(${taskId.slice(0, 8)}) → ${TaskState[task.status.state]}, ` + + `\n── Result: GetTask(${taskId.slice(0, 8)}) → ${TaskState[stateOf(task.status)]}, ` + `artifacts: ${task.artifacts?.length ?? 0}, history messages: ${task.history?.length ?? 0} ──`, ); }