refactor(acp-bridge): lift status, paths, errors, and bridge types (#4175 PR 22b/1)#4298
Open
doudouOUC wants to merge 4 commits into
Open
refactor(acp-bridge): lift status, paths, errors, and bridge types (#4175 PR 22b/1)#4298doudouOUC wants to merge 4 commits into
doudouOUC wants to merge 4 commits into
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
doudouOUC
changed the title
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`.
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