refactor(acp-bridge): lift status, paths, errors, and bridge types (#4175 PR 22b/1)#4298
Merged
Conversation
…kpoint 1) First three slices of the PR 22b bulk lift. All purely mechanical moves with re-export wrappers at the old locations so every existing import keeps resolving. What moves - `packages/cli/src/serve/status.ts` (600 LOC) → `packages/acp-bridge/src/status.ts` (acpAgent.ts:85-109 imports 25 symbols from this; lifting unifies the wire-contract owner.) - `packages/cli/src/serve/status.test.ts` → `packages/acp-bridge/src/status.test.ts` - `canonicalizeWorkspace` from `cli/src/serve/fs/paths.ts:61-97` → `packages/acp-bridge/src/workspacePaths.ts` (cross-module contract; bridge package now owns it directly) - `MAX_WORKSPACE_PATH_LENGTH` from `httpAcpBridge.ts:762` → same `workspacePaths.ts` file - 11 bridge error classes from `httpAcpBridge.ts` (`SessionNotFoundError`, `RestoreInProgressError`, `InvalidSessionScopeError`, `SessionLimitExceededError`, `WorkspaceMismatchError`, `InvalidClientIdError`, `InvalidPermissionOptionError`, `InvalidSessionMetadataError`, `WorkspaceInitConflictError`, `McpServerNotFoundError`, `McpServerRestartFailedError`) → new `packages/acp-bridge/src/bridgeErrors.ts` Wrappers - `cli/src/serve/status.ts` shrinks to a 1-line `export *` re-export - `cli/src/serve/fs/paths.ts` keeps its other exports (`resolveWithinWorkspace`, `Intent`, `ResolvedPath`, `hasSuspiciousPathPattern`); imports + re-exports the lifted two - `httpAcpBridge.ts` consolidated `import` + `export` block for all 11 error classes plus `MAX_WORKSPACE_PATH_LENGTH`; the local factory code keeps using these symbols since they're now imported into module scope Package wiring - `packages/acp-bridge/package.json` — adds `@qwen-code/qwen-code-core` dependency (status types reference `SkillError` from core); adds subpath exports for `./status`, `./workspacePaths`, `./bridgeErrors`, plus future `/bridgeTypes`, `/bridgeOptions`, `/bridge`, `/spawnChannel` - `packages/acp-bridge/tsconfig.json` — adds `paths` mapping for `@qwen-code/qwen-code-core` and a project reference so typecheck resolves core sources directly without needing dist/ first - `packages/cli/tsconfig.json` already has `@qwen-code/acp-bridge/*` path mapping from PR 22a, no change needed - `package-lock.json` — surgical patch adding `@qwen-code/qwen-code-core` dep to acp-bridge's workspace entry; restored from origin/main first to avoid the npm-11 peer-flag churn that broke PR 22a's first push Backward compatibility - Every existing relative import from `cli/src/serve/` keeps working through wrappers (commands/serve.ts, server.ts, runQwenServe.ts, workspaceAgents.ts, workspaceMemory.ts, acpAgent.ts) - Zero `/capabilities`, route, SDK, or behavior changes Verification - `npm ci --no-audit --ignore-scripts` clean (1453 packages) - `cd packages/core && npx tsc --build` clean - `cd packages/acp-bridge && npx tsc --noEmit` clean (with moved tests) - `cd packages/cli && npx tsc --noEmit` clean Remaining PR 22b checkpoints - 22b/2: bridge types + BridgeOptions + DaemonStatusProvider seam - 22b/3: BridgeClient class - 22b/4: defaultSpawnChannelFactory - 22b/5: createHttpAcpBridge factory (~2240 LOC of moves) - 22b/6: serve/daemonStatusProvider.ts + runQwenServe wire - 22b/7: move bridge tests - 22b/8: shrink httpAcpBridge.ts to final re-export shim
Pure type lift of the bridge's public TypeScript surface — no behavior
or runtime change.
What moves
- 11 type declarations (~520 LOC) from `httpAcpBridge.ts:101-620` →
`packages/acp-bridge/src/bridgeTypes.ts`:
- `BridgeSpawnRequest`, `BridgeSession`, `BridgeRestoreSessionRequest`
- `BridgeSessionState` (alias of ACP `LoadSessionResponse | ResumeSessionResponse`)
- `BridgeRestoredSession`, `BridgeSessionSummary`, `SessionMetadataUpdate`
- `BridgeClientRequestContext`, `BridgeHeartbeatResult`, `BridgeHeartbeatState`
- `HttpAcpBridge` interface (the bridge's public facade — ~30 methods
+ 2 getter properties)
`bridgeTypes.ts` imports
- `ApprovalMode` from `@qwen-code/qwen-code-core` (status types reference it)
- ACP wire types (`CancelNotification`, `LoadSessionResponse`,
`PromptRequest`, `PromptResponse`, `RequestPermissionResponse`,
`ResumeSessionResponse`, `SetSessionModelRequest`,
`SetSessionModelResponse`) from `@agentclientprotocol/sdk`
- `BridgeEvent` + `SubscribeOptions` from local `./eventBus.js`
- All `Serve*Status` types from local `./status.js` (also part of
acp-bridge after checkpoint 1)
Wrappers
- `httpAcpBridge.ts` lines 101-620 replaced with a single `import type` +
`export type` re-export block; the local `BridgeClient` class +
`createHttpAcpBridge` factory continue to reference these names
through the import binding
- Cleaned up imports from `@agentclientprotocol/sdk` and `./status.js` —
`LoadSessionResponse`, `PromptResponse`, `ResumeSessionResponse`,
`SubscribeOptions`, and 5 `ServeWorkspace*Status` types are no longer
used inline in `httpAcpBridge.ts` (all consumed via lifted types)
- Added `./bridgeTypes` subpath export to `acp-bridge/package.json`
Backward compatibility
- All existing imports of `from './httpAcpBridge.js'` keep resolving
(server.ts:31, runQwenServe.ts:16, workspaceAgents.ts:21,
workspaceMemory.ts:19, index.ts:90-97)
- Zero `/capabilities`, route, SDK, or behavior changes
Verification
- `cd packages/core && npx tsc --build` clean
- `cd packages/acp-bridge && npx tsc --build` clean
- `cd packages/cli && npx tsc --noEmit` clean
Remaining PR 22b checkpoints
- 22b/3: BridgeOptions + DaemonStatusProvider injection seam
- 22b/4: BridgeClient class
- 22b/5: defaultSpawnChannelFactory
- 22b/6: createHttpAcpBridge factory (~2240 LOC of moves) — the bulk
- 22b/7: serve/daemonStatusProvider.ts + runQwenServe wire
- 22b/8: move bridge tests (5064 lines)
- 22b/9: shrink httpAcpBridge.ts to final re-export shim
Contributor
📋 Review SummaryThis PR (PR 22b checkpoints 1-2) continues the mechanical extraction of ACP bridge primitives from 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is the second slice of the ACP bridge refactor (Wave 5 PR 22b/1), moving “pure types + pure utilities” out of packages/cli/src/serve/ into the shared @qwen-code/acp-bridge package while keeping backward-compatible re-export shims in the CLI.
Changes:
- Lifted serve status schema/types + helpers (and tests) into
packages/acp-bridge/src/status.ts, leaving a CLI re-export wrapper. - Introduced
workspacePaths.tsinacp-bridgeforcanonicalizeWorkspace+MAX_WORKSPACE_PATH_LENGTH, with CLIfs/paths.tsre-exporting for compatibility. - Lifted bridge public contract types (
HttpAcpBridge+ related) and bridge error classes intoacp-bridge, withhttpAcpBridge.tsre-exporting for existing importers; updatedacp-bridgepackage wiring.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/status.ts | Replaced implementation with a re-export shim to @qwen-code/acp-bridge/status. |
| packages/cli/src/serve/httpAcpBridge.ts | Removed in-file type/error declarations; now imports/re-exports from @qwen-code/acp-bridge subpaths. |
| packages/cli/src/serve/fs/paths.ts | Re-exports canonicalizeWorkspace + MAX_WORKSPACE_PATH_LENGTH from @qwen-code/acp-bridge/workspacePaths. |
| packages/acp-bridge/tsconfig.json | Added project reference + path mapping so acp-bridge can typecheck against core sources. |
| packages/acp-bridge/src/workspacePaths.ts | New shared workspace canonicalization utility + path length limit constant. |
| packages/acp-bridge/src/status.ts | New home for serve status schema/types/helpers. |
| packages/acp-bridge/src/status.test.ts | Moved/updated tests for the status module in the new package. |
| packages/acp-bridge/src/bridgeTypes.ts | New shared bridge contract types, including HttpAcpBridge. |
| packages/acp-bridge/src/bridgeErrors.ts | New shared bridge error classes previously defined in httpAcpBridge.ts. |
| packages/acp-bridge/package.json | Added subpath exports and core dependency for lifted modules. |
| package-lock.json | Updated lockfile for @qwen-code/qwen-code-core dependency under acp-bridge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+50
to
+61
| "./bridgeOptions": { | ||
| "types": "./dist/bridgeOptions.d.ts", | ||
| "import": "./dist/bridgeOptions.js" | ||
| }, | ||
| "./bridge": { | ||
| "types": "./dist/bridge.d.ts", | ||
| "import": "./dist/bridge.js" | ||
| }, | ||
| "./spawnChannel": { | ||
| "types": "./dist/spawnChannel.d.ts", | ||
| "import": "./dist/spawnChannel.js" | ||
| }, |
Collaborator
Author
There was a problem hiding this comment.
Adopted in 33c83033d. Codex's P2 review converged on the same finding from a separate run, confirming it's a real ERR_MODULE_NOT_FOUND waiting for the first consumer that follows the advertised subpath. Removed the three premature exports — ./bridgeOptions, ./bridge, ./spawnChannel — they will come back in PR 22b/2 alongside their source files.
Verified: 40/40 acp-bridge tests pass, tsc --noEmit clean across acp-bridge + cli.
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. |
…dule JSDoc Two review-driven fixes for PR 22b/1: 1. **Codex P2 + Copilot inline (real bug)**: removed the three subpath exports `./bridgeOptions`, `./bridge`, `./spawnChannel` from `packages/acp-bridge/package.json`. They pointed at `dist/*.js` files that this PR never builds (no corresponding `src/*.ts` exists yet — those modules ship in PR 22b/2). A consumer following any of those advertised subpaths would have hit `ERR_MODULE_NOT_FOUND` at runtime. The exports come back when 22b/2 adds the source files. 2. **github-actions Low #6 (polish)**: added a module-level JSDoc header to `packages/acp-bridge/src/bridgeErrors.ts` explaining the centralized error taxonomy + how `instanceof`-branching maps to HTTP status codes + how the structured fields drive SDK consumers' typed prompts. Also notes the lift origin (#4175 PR 22b/1) and the re-export shim that keeps server.ts / workspaceAgents.ts / workspaceMemory.ts callers green. Verification - `cd packages/core && npx tsc --build` clean - `cd packages/acp-bridge && npx tsc --noEmit` clean - `cd packages/acp-bridge && npx vitest run` — 40/40 pass (status 12 + eventBus 20 + inMemoryChannel 8) Other review feedback handled separately (replies on PR; follow-up issue for `mapDomainErrorToErrorKind` regex tech debt).
Collaborator
Author
Review feedback handling — github-actions summaryWalking through each item in the summary review (codex ✅ Adopted🔴 Codex P2 / Copilot inline ( 🔵 Low #6 — module-level JSDoc on 📌 Tracked as follow-up issue🟢 Medium #4 — 📋 Verified — no code change needed🟡 High #2 — tsconfig completeness — confirmed full configuration is in place. "paths": {
"@qwen-code/qwen-code-core": ["../core/src/index.ts"],
"@qwen-code/qwen-code-core/*": ["../core/src/*"]
},
...
"references": [{ "path": "../core" }]Reviewer was reading the diff context-only; the actual file has the full setup. 🟡 Declined with rationale🟢 Medium #3 — symmetric truncation of 🟢 Medium #5 — 🔵 Low #7 — 🔵 Low #8 — 🔵 Low #9 — Net result: 1 real bug fixed + 1 polish adopted + 1 follow-up issue tracked + 6 declined with rationale. Re-pushed cc @wenshao @chiga0 — happy to revisit any of the declines if the rationale doesn't hold up. |
Self-review caught: `acp-bridge/src/index.ts` only re-exported the four
PR 22a primitives (`eventBus`, `inMemoryChannel`, `channel`, `permission`)
but missed the four modules added in PR 22b/1 (`status`, `workspacePaths`,
`bridgeErrors`, `bridgeTypes`). The README I wrote in PR 22a explicitly
promised "root for application/test code that uses several primitives at
once" — leaving these out broke the contract: anyone using
`import { ServeStatusCell } from '@qwen-code/acp-bridge'` would get
`Module ... has no exported member`.
Subpath imports (`@qwen-code/acp-bridge/status` etc.) already worked, so
this is a documentation-vs-code drift rather than a runtime regression
in tracked consumers — but it would have surprised the first downstream
consumer that followed the README's guidance.
Verification
- `cd packages/acp-bridge && npx tsc --build` clean
- `cd packages/acp-bridge && npx vitest run` — 40/40 pass
- `cd packages/cli && npx tsc --noEmit` clean
- `cd packages/cli && npx vitest run src/serve/` — 680/680 pass
Found during self-review pass requested after the Codex/Copilot/github-
actions review feedback was addressed in `33c83033d`.
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
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
7 tasks
wenshao
reviewed
May 18, 2026
| err: unknown, | ||
| ): ServeErrorKind | undefined { | ||
| if (err instanceof BridgeTimeoutError) return 'init_timeout'; | ||
| if (err instanceof SkillError) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] mapDomainErrorToErrorKind uses instanceof SkillError while the same function already documents and works around the identical cross-package class-duplication problem for TrustGateError via .name matching. The code comment at line ~580 explicitly explains: "we match by .name rather than instanceof because cross-package bundling can produce duplicate class instances where instanceof returns false."
Now that this function lives in @qwen-code/acp-bridge (which has its own resolution of @qwen-code/qwen-code-core), and SkillError instances originate from the CLI's SkillManager path, the same bundle-duplication scenario could cause instanceof SkillError to silently return false, making skill parse/file errors lose their errorKind classification in diagnostic cells.
Suggested change
| if (err instanceof SkillError) { | |
| if (err instanceof SkillError || (err as Error)?.name === 'SkillError') { | |
| const code = (err as { code?: string }).code; | |
| if (code && SKILL_PARSE_CODES.has(code)) return 'parse_error'; | |
| if (code && SKILL_FILE_CODES.has(code)) return 'missing_file'; | |
| return undefined; | |
| } |
This matches the existing TrustGateError pattern rather than requiring a separate path.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Adopted in 7acc9e41b on PR #4304 (PR 22b/2). The fix lands there rather than as a standalone follow-up because:
status.tsis already touched by 22b/2 (addedcreateIdleEnvStatus+ theDaemonStatusProviderinjection seam)- the change is exactly 4 LOC + 1 regression test, well within scope
- one fewer review cycle than a standalone tiny PR
Implementation mirrors your suggestion — instanceof SkillError || (err as Error)?.name === 'SkillError'. Inside the block we cast through (err as { code?: string }).code since TS can't narrow through the OR (same shape your snippet had). Comment block expanded to explain the cross-package bundling rationale + reference this thread.
Regression test in acp-bridge/src/status.test.ts follows the existing TrustGateError test pattern: synthesize a foreign-bundled SkillError via Object.assign(new Error(...), { name: 'SkillError', code: 'PARSE_ERROR' }), assert classification still resolves to the right kind. Includes negative case (unknown code → undefined) so a future maintainer can't accidentally widen the lookup.
Verified: 161 cli serve bridge tests + acp-bridge status tests all pass after rebase + lift.
doudouOUC
added a commit
that referenced
this pull request
May 19, 2026
…#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
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
Second slice of #4175 Wave 5 PR 22 (
refactor(serve): extract acp bridge primitives). PR 22a (#4295, commit `f97cb680a`) lifted the zero-coupling primitives + the `PermissionMediator` interface stub. PR 22b/1 lifts the bridge's pure-type + pure-utility surface — status types, workspace-path canonicalization, error classes, and the `HttpAcpBridge` interface contract — leaving the implementation lift (`BridgeClient`, `createHttpAcpBridge` factory, `defaultSpawnChannelFactory`) to a follow-up PR 22b/2.This split exists because the type/utility lift is mechanical and low-risk (~1500 LOC of moves + re-export wrappers), while the implementation lift adds the `DaemonStatusProvider` injection seam and is substantially larger (~3300 LOC). Splitting lets reviewer attention stage cleanly per slice and unblocks chiga0's three open client-adapter PRs (#4261/#4266/#4267) to start importing the new bridge types right away.
What moves (mechanical, no behavior changes)
`status.ts` (600 LOC)
The 25-symbol import block in `acp-integration/acpAgent.ts:85-109` (the ACP child code) keeps resolving through the wrapper without any churn — it imports `STATUS_SCHEMA_VERSION`, `SERVE_STATUS_EXT_METHODS`, `SERVE_CONTROL_EXT_METHODS`, `mapDomainErrorToErrorKind`, `ACP_PREFLIGHT_KINDS`, plus 20 `Serve*Status` types.
`workspacePaths.ts` (NEW, 80 LOC)
`bridgeErrors.ts` (NEW, ~200 LOC)
11 error classes lifted from `httpAcpBridge.ts` to `acp-bridge/src/bridgeErrors.ts`:
`httpAcpBridge.ts` imports + re-exports them so server.ts:31 (which imports 7 of these), workspaceAgents.ts, workspaceMemory.ts keep resolving without churn.
`bridgeTypes.ts` (NEW, ~520 LOC)
11 type declarations from `httpAcpBridge.ts:101-620` → `acp-bridge/src/bridgeTypes.ts`:
`httpAcpBridge.ts` imports + re-exports the types.
Package wiring
Backward compatibility guarantees
What's deferred to PR 22b/2
PR 22b/1 (this PR) is fully self-contained and mergeable on its own.
Test plan
References