refactor(serve): typed errors for channel-closed and missing-cli-entry (#4299)#4300
Merged
Merged
Conversation
Contributor
📋 Review SummaryThis PR addresses #4299 by replacing fragile regex-based error classification with typed error classes ( 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces two regex-based error classifications in mapDomainErrorToErrorKind with instanceof checks against two new typed error classes, eliminating false-positive misclassification for foreign errors whose messages happen to contain bridge-specific phrases.
Changes:
- Add
BridgeChannelClosedErrorandMissingCliEntryErrorclasses instatus.ts, preserving legacy message wording. - Replace three
new Error('agent channel closed …')sites and onenew Error('Cannot determine CLI entry path …')site with the typed classes. - Update
mapDomainErrorToErrorKindto branch oninstanceof, remove the regex fallbacks/TODO, and add tests covering the new classes plus a regression test for foreign messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/cli/src/serve/status.ts | Adds two typed error classes; replaces regex fallbacks with instanceof checks. |
| packages/cli/src/serve/status.test.ts | Adds class behavior tests and a regression test asserting foreign messages no longer misclassify. |
| packages/cli/src/serve/httpAcpBridge.ts | Throws the new typed errors at the four call sites instead of generic Error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
previously approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen-latest-series-invite-beta-v28 via Qwen Code /review
#4299) Replace regex-on-`.message` classification in `mapDomainErrorToErrorKind` with two new typed error classes living next to `BridgeTimeoutError` in `status.ts`: - `BridgeChannelClosedError(context)` — replaces three `throw new Error('agent channel closed …')` sites in `httpAcpBridge.ts` (`getTransportClosedReject`, `getChannelClosedReject`, restore-time `transportClosed`). The `context` field preserves the legacy message wording verbatim so log greps and existing diagnostics keep working. - `MissingCliEntryError()` — replaces the generic `Error` thrown by `defaultSpawnChannelFactory` when neither `QWEN_CLI_ENTRY` nor `process.argv[1]` resolves. Message bytes preserved. `mapDomainErrorToErrorKind` now branches on `instanceof` for these two kinds, and the regex fallbacks (plus the TODO that called them out) are removed entirely — there are no remaining throw sites we don't control. The test suite gains a regression case: wrapping or foreign errors whose `.message` happens to contain "agent channel closed" or "Cannot determine CLI entry path" no longer misclassify (returns `undefined`, the correct behavior per the issue). Bridge-internal classes don't cross the JSON-RPC boundary into the ACP child, so `instanceof` symmetry is preserved at every consumer of `mapDomainErrorToErrorKind` (3 sites in `httpAcpBridge.ts`, 6 in `acpAgent.ts`). Closes #4299 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
7719f8d to
c3bca99
Compare
wenshao
approved these changes
May 18, 2026
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
Self-review found the package README still listed only PR 22a's four modules (`eventBus`, `inMemoryChannel`, `channel`, `permission`) and hadn't been updated for the eight new ones added across the two follow-up slices. The "What's here today" section now covers all nine modules; consumers reading the README to figure out what to import know the full surface. Targeted minimal update — major restructure (drop the obsolete "What's not here yet" section + the "PR 22a (this)" status table) deferred to PR 22b/3 when the package surface stabilizes. Modules added to the list: - `status` (PR 22b/1) — wire contract status types + 27-symbol acp-integration import surface; mapDomainErrorToErrorKind classifier (regex → typed after #4299/#4300). - `workspacePaths` (PR 22b/1) — canonicalizeWorkspace + MAX_WORKSPACE_PATH_LENGTH. - `bridgeErrors` (PR 22b/1) — 11 typed bridge Error subclasses. - `bridgeTypes` (PR 22b/1) — BridgeSession*, HttpAcpBridge interface. - `bridgeOptions` (PR 22b/2) — BridgeOptions + DaemonStatusProvider seam.
7 tasks
wenshao
pushed a commit
that referenced
this pull request
May 19, 2026
…ider seam (#4175 PR 22b/2) (#4304) * refactor(acp-bridge): lift BridgeOptions + introduce DaemonStatusProvider seam (#4175 PR 22b/2) Design slice of #4175 Wave 5 PR 22b. Lifts the bridge's construction contract (`BridgeOptions`) to `@qwen-code/acp-bridge` and introduces a new `DaemonStatusProvider` interface so the bridge factory no longer hard-imports daemon-host-specific helpers. The mechanical bulk lift of `BridgeClient` + spawn factory + `createHttpAcpBridge` factory closure (~3000 LOC) lands in PR 22b/3 against this stable contract. What's lifted - `BridgeOptions` interface (~150 LOC) from `httpAcpBridge.ts:189-325` → `acp-bridge/src/bridgeOptions.ts`. Adds new optional `statusProvider?: DaemonStatusProvider` field; everything else preserved verbatim. - `buildDaemonPreflightCells` + `safeCheck` (~210 LOC) deleted from `httpAcpBridge.ts:4002-4214` → moved to new `cli/src/serve/daemonStatusProvider.ts`. Logic byte-for-byte identical (slot-by-slot allocation, `Promise.allSettled` fan-out, classified error cells); only the call-site changes from `await buildDaemonPreflightCells(boundWorkspace)` to `await opts.statusProvider?.getDaemonPreflightCells(boundWorkspace) ?? []`. What's new - `DaemonStatusProvider` interface in `acp-bridge/src/bridgeOptions.ts`: two methods (`getEnvStatus`, `getDaemonPreflightCells`) covering the full set of daemon-host cells the bridge currently delegates. Direct positional args (boundWorkspace + acpChannelLive); both methods return `Promise<>` so future async impls (config-file reads, network probes) get the seam without breaking the bridge. No abort signal — YAGNI; future async needs can pull in a typed extension. Single combined interface (vs split into two) because env + preflight are conceptually one daemon-host status surface and the production impl ships them as one factory. - `createDaemonStatusProvider()` in `cli/src/serve/daemonStatusProvider.ts`: thin wrapper that calls `buildEnvStatusFromProcess` (still in `envSnapshot.ts`) and the moved `buildDaemonPreflightCells` body. Stateless; production callers can re-allocate per-request without performance cost. Bridge factory body changes - `getWorkspaceEnvStatus`: routes through `opts.statusProvider?.getEnvStatus(...)`. When `statusProvider` is omitted (Mode A in-process consumers, tests that don't care about env cells) the bridge returns an idle envelope `{ v, workspaceCwd, initialized: true, acpChannelLive, cells: [] }` matching the "queryable but empty" pattern PR 12 / 13 established for diagnostic routes. - `getWorkspacePreflightStatus`: routes the daemon-half through `opts.statusProvider?.getDaemonPreflightCells(...) ?? []`. ACP-side cells (auth / mcp_discovery / skills / providers / tool_registry / egress) keep being fetched separately via the existing `requestWorkspaceStatus` extMethod path; the bridge stitches the two halves together as before. Wiring - `runQwenServe.ts` passes `statusProvider: createDaemonStatusProvider()` into the bridge factory by default. - `httpAcpBridge.test.ts`: `makeBridge` helper now defaults to providing the production status provider so existing 4 env / preflight tests (which exercise the bridge's delegation path) keep seeing populated cells. Tests that want to exercise the no-provider idle fallback can override with `{ statusProvider: undefined }`. - `acp-bridge/package.json`: adds `./bridgeOptions` subpath export. - `acp-bridge/src/index.ts`: barrel re-exports `bridgeOptions.js`. Backward compatibility - `httpAcpBridge.ts` still re-exports `BridgeOptions` and `DaemonStatusProvider` types for callers via `from './httpAcpBridge.js'` (existing pattern from PR 22b/1). server.ts:31, runQwenServe.ts:16, workspaceAgents.ts, workspaceMemory.ts all keep resolving without churn. - Zero `/capabilities`, route, SDK, or behavior changes when the default `runQwenServe` is in play (production wiring is identical to pre-PR; the default `createDaemonStatusProvider()` reproduces the pre-injection cell shapes byte-for-byte). - Direct embeds / tests that omit `statusProvider` see new idle fallback semantics; this is the documented Mode A integration pattern and matches PR 12 / 13's "idle status is queryable" contract. Verification - `cd packages/core && npx tsc --build` clean - `cd packages/acp-bridge && npx tsc --noEmit` clean - `cd packages/cli && npx tsc --noEmit` clean (no errors in `src/serve/`; pre-existing tsconfig-excluded `mcp.test.ts` / `check-i18n.ts` errors unrelated to this PR) - `cd packages/cli && npx vitest run src/serve/` — 701/701 pass (16 test files; 4 env / preflight tests reuse the production provider via updated `makeBridge` helper) - `npm ci --dry-run` clean (no lockfile drift; new file imports resolve via existing `@qwen-code/qwen-code-core` and `@qwen-code/acp-bridge` workspace deps already present from PR 22b/1) Remaining PR 22b checkpoints - 22b/3 (mechanical bulk lift): `BridgeClient` (~400 LOC) + `defaultSpawnChannelFactory` (~250 LOC) + `createHttpAcpBridge` factory closure (~2240 LOC) + 5064-LOC test file move + final `httpAcpBridge.ts` shim. Zero new design decisions — factory consumes `DaemonStatusProvider` per the contract this PR froze. - 22b' (small follow-up): `WorkspaceFileSystem` injection into `BridgeClient.writeTextFile/readTextFile`. Closes PR 18 ws.ts:613 TOCTOU thread. - #4299: `mapDomainErrorToErrorKind` regex → typed errors (parallel follow-up tracked from PR 22b/1 review). * docs(acp-bridge): catch README up to PR 22b/1 + 22b/2 module additions Self-review found the package README still listed only PR 22a's four modules (`eventBus`, `inMemoryChannel`, `channel`, `permission`) and hadn't been updated for the eight new ones added across the two follow-up slices. The "What's here today" section now covers all nine modules; consumers reading the README to figure out what to import know the full surface. Targeted minimal update — major restructure (drop the obsolete "What's not here yet" section + the "PR 22a (this)" status table) deferred to PR 22b/3 when the package surface stabilizes. Modules added to the list: - `status` (PR 22b/1) — wire contract status types + 27-symbol acp-integration import surface; mapDomainErrorToErrorKind classifier (regex → typed after #4299/#4300). - `workspacePaths` (PR 22b/1) — canonicalizeWorkspace + MAX_WORKSPACE_PATH_LENGTH. - `bridgeErrors` (PR 22b/1) — 11 typed bridge Error subclasses. - `bridgeTypes` (PR 22b/1) — BridgeSession*, HttpAcpBridge interface. - `bridgeOptions` (PR 22b/2) — BridgeOptions + DaemonStatusProvider seam. * fix(acp-bridge): address PR 22b/2 review feedback Four review-driven fixes from wenshao + gpt-5.5 + Copilot: 1. **wenshao [Critical] @ httpAcpBridge.ts** — `getEnvStatus` and `getDaemonPreflightCells` calls now wrapped in try/catch with stderr-logged fallback to the idle envelope / empty cells. The pre-injection `buildDaemonPreflightCells` used `Promise.allSettled` internally and was effectively unthrowable; the route's "daemon cells always render" invariant would have silently broken for any custom `DaemonStatusProvider` that throws. Fallback preserves the route-level guarantee structurally. 2. **wenshao/gpt-5.5 [Suggestion] @ httpAcpBridge.ts** — wired `createDaemonStatusProvider()` into the default bridge constructed by `createServeApp()` (in `server.ts`). Pre-fix, direct embeds / tests that didn't inject `deps.bridge` would silently lose the daemon env + preflight cells the default server app reported pre-injection. Now symmetric with `runQwenServe.ts`. 3. **wenshao [Suggestion] @ httpAcpBridge.ts** — extracted `createIdleEnvStatus(workspaceCwd, acpChannelLive)` helper into `acp-bridge/src/status.ts` (matches existing `createIdleAcpPreflightCells` pattern). Single construction site for `ServeWorkspaceEnvStatus`'s idle shape — future optional fields added to the production builder in `envSnapshot.ts buildEnvStatusFromProcess` won't silently diverge from the bridge's fallback. 4. **wenshao [Suggestion] @ httpAcpBridge.test.ts** — added 4 new tests covering the previously-untested provider-omitted / provider-throws paths: - `returns idle env envelope when statusProvider is omitted` - `returns empty daemon preflight cells when statusProvider is omitted` - `falls back to idle env envelope when statusProvider.getEnvStatus throws` - `falls back to empty daemon cells when statusProvider.getDaemonPreflightCells throws` Verification - `cd packages/core && npx tsc --build` clean - `cd packages/acp-bridge && npx tsc --build` clean - `cd packages/cli && npx tsc --noEmit` clean (no errors in `src/serve/`) - `cd packages/cli && npx vitest run src/serve/httpAcpBridge.test.ts` — 165/165 pass (4 new tests + 161 existing) - `cd packages/cli && npx vitest run src/serve/` — 705/705 pass on deterministic isolated runs; full-suite has known flakes unrelated to this PR (server.test.ts state pollution; pre-existing) Declined - **Copilot @ httpAcpBridge.ts:200** — `import type` after `export` statements claim against `import/first` ESLint rule. Verified `npx eslint src/serve/httpAcpBridge.ts` exits 0 — the rule isn't enforced on this file. False positive. * fix(acp-bridge): SkillError detection survives cross-package bundling (#4298 review) Wenshao [Suggestion] on the merged PR #4298 (#4298 thread r3262781757): the same cross-package class-duplication problem the function already documents and works around for `TrustGateError` via `.name` matching applies to `SkillError` — and the existing code uses raw `instanceof SkillError`, which silently returns `false` when bundling produces duplicate class instances (a real risk once `acp-bridge` ever gets consumed outside the monorepo or under a bundler that doesn't dedupe `file:` workspace deps). Fix mirrors the `TrustGateError` pattern: `instanceof SkillError || .name === 'SkillError'`. When the synthesized branch fires we cast to read `.code` (TS can't narrow through the OR), then dispatch to the same `SKILL_PARSE_CODES` / `SKILL_FILE_CODES` lookup the genuine branch already used. Folding into PR 22b/2 rather than a standalone tiny PR because: - `status.ts` is already touched by 22b/2 (added `createIdleEnvStatus`) - the change is 4 LOC + 1 regression test, well below scope - one fewer review cycle Regression test in `status.test.ts` constructs synthesized SkillError instances (`Object.assign(new Error(...), { name: 'SkillError', code: ... })`) that fail `instanceof` but carry the right `.name` + `.code`, then asserts classification still resolves to `parse_error` / `missing_file` / `undefined`. Mirrors the existing `TrustGateError` regression test that pinned this exact pattern. Verification - `npx tsc --build` clean (core + acp-bridge) - `cd packages/cli && npx tsc --noEmit` clean - `cd packages/acp-bridge && npx vitest run` — all tests pass including new SkillError cross-bundle regression - `cd packages/cli && npx vitest run src/serve/httpAcpBridge.test.ts` — 161 pass * docs(acp-bridge): clarify env vs preflight fallback asymmetry (self-review) Self-review fold-in. Inline comment at the no-provider branch of `getWorkspacePreflightStatus` explains why the fallback shape differs from `getWorkspaceEnvStatus`: env is a single envelope (idle helper); preflight is the union of daemon-locality + ACP-locality cells stitched below, so an empty daemon slice IS the right fallback (the ACP slice fills in independently). Comment-only; zero behavior change. Caught during the comprehensive self-review pass between commits — the asymmetry is correct but a future maintainer might find it confusing without context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mapDomainErrorToErrorKindinpackages/cli/src/serve/status.tsno longer regex-matches.messageto classify two failure modes — it now branches oninstanceof BridgeChannelClosedError/instanceof MissingCliEntryError, two new typed classes added next toBridgeTimeoutError. The four throw sites (3×agent channel closed …inhttpAcpBridge.ts, 1×Cannot determine CLI entry path …indefaultSpawnChannelFactory) now throw the typed classes. The regex fallbacks and the in-codeTODOthat called them out are removed..messagemis-classifies any future error whose message happens to contain those phrases (e.g., a wrapping error likeRuntimeError: failed: <inner: agent channel closed>), and silently degrades classification when a maintainer copy-edits the throw-site wording. Typed errors make the contract structural rather than lexical, matching the discipline the bridge already follows for its 11 other error classes (SessionNotFoundError,WorkspaceMismatchError, etc.).status.ts(next toBridgeTimeoutError), not inhttpAcpBridge.tswith the 11 others. Reason:mapDomainErrorToErrorKindis instatus.ts, andacpAgent.ts(the ACP child) importsstatus.ts— pulling bridge-implementation intostatus.tswould break that layering. Same precedent asBridgeTimeoutError.BridgeChannelClosedErrortakes acontextstring so each of the three call-sites' messages reproduce byte-for-byte ('agent channel closed mid-request (session abc)','… (workspace status)','… during session/load').MissingCliEntryErrorhas no constructor args and emits the same multi-line message verbatim.status.test.ts:173-189locks in the intended tightening: a wrapping error likenew Error('wrapped: agent channel closed mid-request')now returnsundefined(correct), where the old regex returned'protocol_error'(false positive, the bug fixed).Diff is ~50 LOC across 3 files, exactly the scope the issue estimated.
Validation
cd packages/cli && npx vitest run src/serve/status.test.ts— see the newBridgeChannelClosedError/MissingCliEntryErrordescribe blocks plus the foreign-error regression test.Scope / Risk
Error('agent channel closed …')and relying on the helper to tag it asprotocol_errorwill now getundefined. We searchedpackages/for both phrases — only one out-of-scope hit (commands/channel/config-utils.ts:25, theqwen channel startCLI path) which never reachesmapDomainErrorToErrorKind, so safe. Also confirmed no telemetry/alerting layer keys on these tags being non-undefined; consumers inacpAgent.tsandhttpAcpBridge.tsonly render the cell payload.qwen serve --port 0smoke run (typed-class change is internal to error classification; no behavior change at HTTP route or/capabilitiespayload).errorKindpayload values for these two failure modes are unchanged ('protocol_error'and'missing_binary'). New classes are additions tostatus.ts's exports — purely additive.Testing Matrix
Testing matrix notes:
vitest runcovers the new class behavior + regression. CI should cover Linux/Windows; no platform-specific code paths in this diff.Linked Issues / Bugs
Closes #4299
References:
packages/cli/src/serve/); the verbatim move in 22b/1 will carry these changes through.🤖 Generated with Qwen Code