refactor(acp-bridge): create skeleton + lift zero-coupling primitives (#4175 PR 22a)#4295
Merged
Conversation
…#4175 PR 22a) First slice of #4175 Wave 5 PR 22 (`refactor(serve): extract acp bridge primitives`). Lifts the three primitives from `packages/cli/src/serve/` that have zero coupling to the rest of `serve/`, so PR 22a can land ahead of PR 17 (#4282) and PR 14b (#4271) without merge-conflict risk on `httpAcpBridge.ts`. Also seeds the `PermissionMediator` interface contract that PR 24 will implement (4 strategies — first-responder / designated / consensus / local-only — replacing the inline first-responder voting in `BridgeClient.requestPermission`). What moves - `cli/src/serve/eventBus.ts` (578 LOC, no internal imports) → `packages/acp-bridge/src/eventBus.ts` - `cli/src/serve/inMemoryChannel.ts` (73 LOC, only depends on `@agentclientprotocol/sdk`) → `packages/acp-bridge/src/inMemoryChannel.ts` - `httpAcpBridge.ts:638-677` `AcpChannel` / `AcpChannelExitInfo` / `ChannelFactory` types → `packages/acp-bridge/src/channel.ts` - Both test files moved alongside their sources What's new - `packages/acp-bridge/` package (`@qwen-code/acp-bridge`, internal, not published to npm yet) - `packages/acp-bridge/src/permission.ts` — type-only `PermissionMediator` / `PermissionPolicy` / `PermissionResolution` / `PermissionRequestRecord` / `PermissionVote` / `PermissionVoteOutcome`. No implementation; PR 24 fills it in. - `packages/cli/src/serve/eventBus.ts` and `inMemoryChannel.ts` are now one-line re-export wrappers for backward compat. - `httpAcpBridge.ts:638-677` is now a one-line `import type` + `export type` re-export. Backward compatibility - All existing relative imports (`./eventBus.js`, `./inMemoryChannel.js`, `../serve/eventBus.js`) keep resolving via the wrappers — no churn for the 8 importer sites in `cli/src/serve/` plus `cli/src/commands/serve.ts:14`. - `httpAcpBridge.ts` continues to export `AcpChannel` / `AcpChannelExitInfo` / `ChannelFactory` so any external consumer is unaffected. - Zero `/capabilities` payload changes, zero HTTP route changes, zero SDK behavior changes, zero spawn-site behavior changes. What's not in this slice (deferred to PR 22b after PR 17/14b land) - `BridgeClient` + `createHttpAcpBridge` factory + `defaultSpawnChannelFactory` (~3000 LOC, lines 1082-3770 + 4106-4307 of `httpAcpBridge.ts`). - Channel and VSCode-IDE-companion own-spawn migrations. Tests - `npm test --workspace @qwen-code/acp-bridge` — 28/28 pass (eventBus 20 + inMemoryChannel 8) at the new location. - `cd packages/cli && npx vitest run src/serve/` — 567/567 pass (no regressions). - `cd packages/cli && npx tsc --noEmit` clean. References - #4175 Wave 5 PR 22 row - #3803 chiga0's "Stage 1.5-prereq AcpChannel lift" - `httpAcpBridge.ts:679-696` FIXME for the four `PermissionMediator` strategies this slice declares
Contributor
📋 Review SummaryThis PR (22a of Wave 5, #4175) extracts three zero-coupling ACP bridge primitives ( 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Lifts three zero-coupling primitives (EventBus, createInMemoryChannel, and the AcpChannel/AcpChannelExitInfo/ChannelFactory type contract) out of packages/cli/src/serve/ into a new internal workspace package @qwen-code/acp-bridge, and seeds a type-only PermissionMediator interface that a later PR (24) will implement. The original serve/ files become one-line re-export wrappers so existing callers are unaffected. This is the first slice (PR 22a) of #4175 Wave 5, intentionally scoped to land ahead of #4282 (PR 17) and #4271 (PR 14b) without colliding on httpAcpBridge.ts.
Changes:
- New
packages/acp-bridgeworkspace package witheventBus,inMemoryChannel,channel,permissionmodules + tests + README, wired into the cli via tsconfig paths and project references. cli/src/serve/eventBus.ts,cli/src/serve/inMemoryChannel.ts, and theAcpChannel*block inhttpAcpBridge.tsreduced to re-export wrappers; the FIXME comment is updated to note partial completion.- Type-only
PermissionMediator/PermissionPolicy/PermissionVote*/PermissionResolution/PermissionRequestRecordcontract added (no implementation), declaring the four upcoming strategies.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/acp-bridge/package.json |
New workspace package manifest (subpath exports, deps on @agentclientprotocol/sdk). |
packages/acp-bridge/tsconfig.json |
Composite TS project, ES2023 lib. |
packages/acp-bridge/vitest.config.ts |
Vitest config with silent: true. |
packages/acp-bridge/README.md |
Slice scope + back-compat notes. |
packages/acp-bridge/src/index.ts |
Barrel re-exporting the four modules. |
packages/acp-bridge/src/eventBus.ts (+ .test.ts) |
Lifted bus implementation + 20 tests. |
packages/acp-bridge/src/inMemoryChannel.ts (+ .test.ts) |
Lifted paired channel helper + 8 tests. |
packages/acp-bridge/src/channel.ts |
New AcpChannel/AcpChannelExitInfo/ChannelFactory type definitions. |
packages/acp-bridge/src/permission.ts |
Type-only PermissionMediator interface stub. |
packages/cli/src/serve/eventBus.ts |
Reduced to export * wrapper. |
packages/cli/src/serve/inMemoryChannel.ts |
Reduced to export * wrapper. |
packages/cli/src/serve/httpAcpBridge.ts |
Channel types now re-exported from the new package; FIXME updated. |
packages/cli/tsconfig.json |
Adds path mappings + project reference. |
packages/cli/package.json |
Adds file: dep on the new package. |
package-lock.json |
New workspace link + a large amount of unrelated peer-flag/entry churn. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Use case: workstations where remote control should never grant | ||
| * privilege escalation. | ||
| * | ||
| * See `httpAcpBridge.ts:1144-1154` for the original FIXME that |
Collaborator
Author
There was a problem hiding this comment.
Good catch — adopted in a8263141c. Verified the actual FIXME location via grep -n "FIXME\|first-responder\|designated\|consensus\|local-only" packages/cli/src/serve/httpAcpBridge.ts:
1096-1106is the chiga0 finding 3 FIXME (permission flow → 4-strategyPermissionMediator) — this is whatpermission.tsdeclares646-666is the chiga0 finding 1+4 FIXME (channel/transport split +FileSystemServiceinjection) — this is PR 22b's territory
Updated permission.ts:33 and README.md:62 to cite 1096-1106. Also fixed the PR description's two 679-696 references (the off-by-many range you flagged) to 1096-1106 so all four citations now point at the same place. The 679-696 was a stale citation from when the channel/transport FIXME lived earlier in the file pre-PR 14b coordination — it's been line-shifted multiple times during Wave 3/4 churn, hence the drift. Thanks for the catch.
Comment on lines
+47
to
+50
| "dependencies": { | ||
| "@agentclientprotocol/sdk": "^0.14.1" | ||
| }, | ||
| "devDependencies": { |
Collaborator
Author
There was a problem hiding this comment.
Sound advice in general but deferring to PR 28 (Wave 6 npm alpha publish) for the reason below:
@qwen-code/acp-bridge is currently internal-only — package.json has files: ["dist"] but no npm publish target, and every consumer (today: cli; tomorrow: channels/base, vscode-ide-companion, remoteControl) reaches it via monorepo file: workspace links. npm dedupes those automatically against the root-level @agentclientprotocol/sdk, so the double-instantiation footgun you describe (Stream / AnyMessage becoming structurally-but-not-nominally compatible) cannot fire today.
The risk will materialize the moment this package goes to npm alpha (PR 28 in #4175 Wave 6), at which point external consumers can pin a different @agentclientprotocol/sdk major. Switching to peerDependencies then is mechanical — same pattern most "shared SDK contract" packages adopt at first publish.
Recording the migration as a follow-up checklist item in PR 28's body. If you'd prefer to take the conservative path and switch now (cost is one extra peerDependencies block + devDependencies mirror so local tests still work), happy to do it — just want to flag the cost/benefit since it's pure pre-emption today.
wenshao
reviewed
May 18, 2026
| "types": ["node", "vitest/globals"] | ||
| }, | ||
| "include": ["src/**/*.ts"], | ||
| "exclude": ["node_modules", "dist", "src/**/*.test.ts"] |
Collaborator
There was a problem hiding this comment.
[Suggestion] The exclude array blanket-excludes src/**/*.test.ts, which means eventBus.test.ts and inMemoryChannel.test.ts are no longer type-checked after the move from packages/cli (whose tsconfig did not exclude them). If a future refactor changes BridgeEvent, SubscribeOptions, or other shared types in a type-breaking way, test files won't catch the mismatch until runtime.
Suggested change
| "exclude": ["node_modules", "dist", "src/**/*.test.ts"] | |
| "exclude": ["node_modules", "dist"] |
Alternatively, add a tsconfig.test.json and wire it into the typecheck script. Removing the test exclude is safe since tsc --noEmit (typecheck) doesn't emit output, and composite: true with tsc --build already handles output via outDir.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Adopted in a8263141c — src/**/*.test.ts removed from tsconfig.json exclude. Verified via tsc --noEmit that both moved test files now participate in typecheck (would have caught any drift in BridgeEvent / SubscribeOptions shape). Matches packages/core/tsconfig.json convention (no test exclude; emitted *.test.js in dist/ is accepted across the monorepo). 28/28 acp-bridge tests still pass under the new include set.
…s only The previous commit ran `npm install --ignore-scripts`, which npm 11 treated as license to renormalize peer-dependency markers across the entire lockfile and resolve `@types/react@18.3.28`, `@types/react-dom@18.3.7`, and `@types/prop-types@15.7.15` away from their pinned versions. CI's `npm ci` then refused to install because the manifests no longer matched the lockfile. Restored package-lock.json to origin/main and surgically added only the three entries the new package actually requires: - `node_modules/@qwen-code/acp-bridge` workspace symlink - `packages/acp-bridge` workspace manifest snapshot - `@qwen-code/acp-bridge` listed under `packages/cli`'s dependencies `npm ci --no-audit --ignore-scripts` now succeeds (1453 packages, no warnings about stale lockfile entries). Re-verified `acp-bridge` package tests (28/28 pass) and `tsc --build` clean.
Three review-driven fixes: 1. **wenshao**: removed `src/**/*.test.ts` from `tsconfig.json` exclude so the moved `eventBus.test.ts` and `inMemoryChannel.test.ts` regain typecheck coverage. Pre-fix, a future change to `BridgeEvent` / `SubscribeOptions` shape would only fail at runtime; post-fix `tsc --noEmit` catches the mismatch. Matches `packages/core/tsconfig.json`'s pattern (no test exclude; emitted test artefacts in dist/ are accepted convention). 2. **Copilot**: corrected the FIXME line citation in `permission.ts` and `README.md` from `1144-1154` to the actual range `1096-1106` (the four-strategy FIXME inside `BridgeClient.requestPermission`). Verified via grep against current `httpAcpBridge.ts`. 3. **Copilot review summary**: added a "Imports — root vs subpaths" section to README.md explaining when to use the barrel root (`@qwen-code/acp-bridge`) vs per-module subpaths (`/eventBus`, `/inMemoryChannel`, `/channel`, `/permission`), and added `@see` JSDoc pointers in `cli/src/serve/eventBus.ts` and `inMemoryChannel.ts` wrappers to the implementation files for the design-rationale comments that moved to acp-bridge. Verification: 28/28 acp-bridge tests + 567/567 cli serve tests pass; `tsc --noEmit` clean across both packages including the moved test files. Declined (replied on the PR): - Move `@agentclientprotocol/sdk` to `peerDependencies` — sound advice in general but not yet relevant; package is internal (`files: ["dist"]`, no npm publish), so dedupe is automatic through monorepo file: links. Will revisit during PR 28 (npm alpha publish).
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
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (windows-latest, Node 22.x), Test (ubuntu-latest, Node 22.x), Test (macos-latest, Node 22.x). — gpt-5.5 via Qwen Code /review
Collaborator
Local validation reportVerified head Build & static checks
Unit tests
Backward-compat verification (the load-bearing claim)The PR's central promise is "no churn for the 8 importer sites in
Not a single existing importer file was modified to reach into Live HTTP parity (refactor must not change behavior)Daemon launched on a fresh workspace. PR's base predates #4255 / #4271 / #4282 / #4279, so the expected feature set is the pre-merge baseline (30 features), not the current main's (37+). The point of this test is that behavior is preserved through the wrapper chain, not that features grow.
The eventBus's defensive Last-Event-ID validation logging is the cleanest single proof that the 578 LoC of moved code is executing through the wrapper exactly as before. Forward-looking surface (
|
wenshao
approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Approving based on the local validation pass posted above — pure refactor, mechanically verified: file moves preserve content via git mv, wrappers are two lines (docstring + export *) so there is no opportunity for behavioral drift, all 13 existing import sites resolve through the wrappers unchanged, the new package builds and passes 28/28 of its own tests, and the daemon's SSE / ring replay / Last-Event-ID defensive validation paths all execute through the lifted code as before. PermissionMediator landing as type-only is correct — implementation belongs to PR 24. The 2-commit lag behind origin/main has no conflict (clean git merge-tree); a rebase before merge would tidy history but isn't required.
wenshao
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Extraction is mechanically clean — moved files are byte-identical, re-export wrappers preserve backward compatibility, build passes, 567/567 cli/serve tests pass.
[Suggestion] packages/acp-bridge/src/eventBus.test.ts — No test covers the SubscriberLimitExceededError path. The error class (eventBus.ts:141) is thrown at line 339 when maxSubscribers is exceeded — a public DoS defense — but zero tests exercise it. Suggested test:
it('throws SubscriberLimitExceededError when cap is reached', () => {
const bus = new EventBus(8, 2);
bus.subscribe();
bus.subscribe();
expect(() => bus.subscribe()).toThrow(SubscriberLimitExceededError);
});Additional suggestions (files not in this PR diff):
scripts/build.js—buildOrderarray doesn't includepackages/acp-bridge. Build works viatsc --buildfollowing project references, but this creates an invisible coupling. Consider adding it explicitly beforepackages/cli.packages/cli/vitest.config.ts— No resolve alias for@qwen-code/acp-bridge. Tests pass after build, but the dev-loop requires a prior build for acp-bridge, unlike@qwen-code/qwen-code-corewhich has an alias.
| /** Best-effort terminate; resolves when teardown is complete. */ | ||
| kill(): Promise<void>; | ||
| /** | ||
| * Synchronous force-kill for the second-signal force-exit path. |
Collaborator
There was a problem hiding this comment.
[Suggestion] Traceability tags Bd1y6: and (BX9_P) were dropped during extraction from httpAcpBridge.ts. The original interface at httpAcpBridge.ts:774 (Bd1y6: synchronous force-kill...) and :795 (...to grep stderr for the pid (BX9_P)) carried these tags. The sibling BX9_p tag in eventBus.ts:186 was correctly preserved, confirming these drops were unintentional.
Suggested change
| * Synchronous force-kill for the second-signal force-exit path. | |
| * Bd1y6: synchronous force-kill for the second-signal force-exit path. |
And for the exited JSDoc (line 45):
Suggested change
| * Synchronous force-kill for the second-signal force-exit path. | |
| * to grep stderr for the pid (BX9_P). |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| } | ||
| } | ||
|
|
||
| // FIXME(stage-1.5, chiga0 finding 2): |
Collaborator
There was a problem hiding this comment.
[Suggestion] This FIXME is stale after this PR. It states "EventBus is currently private to the SSE route handler" and suggests lifting to packages/event-bus — but this PR has already lifted EventBus to packages/acp-bridge. The sibling FIXME in httpAcpBridge.ts:646-665 was correctly updated to note PR 22a, but this one was missed.
Consider updating to mirror the httpAcpBridge.ts pattern — note that the lift to @qwen-code/acp-bridge landed in PR 22a, and clarify whether packages/event-bus is still the intended final home.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
This was referenced May 18, 2026
This was referenced May 21, 2026
yiliang114
added a commit
that referenced
this pull request
May 21, 2026
The nightly/preview release workflow has been failing for 3 days with `TS5055: Cannot write file ... because it would overwrite input file` in packages/core during the version bump step. Root cause: `npm install --package-lock-only` in version.js triggers the root `prepare` lifecycle, which re-runs `tsc --build` while packages/core/dist/ already exists from the initial `npm ci`. The unbuilt acp-bridge reference (added in #4295 but missing from build.js) corrupts TypeScript's incremental project graph resolution. Fixes: 1. Add --ignore-scripts to the lock-file-only install in version.js 2. Add packages/acp-bridge to the build order in build.js Closes #4368, closes #4339, closes #4307
DragonnZhang
pushed a commit
that referenced
this pull request
May 21, 2026
The nightly/preview release workflow has been failing for 3 days with `TS5055: Cannot write file ... because it would overwrite input file` in packages/core during the version bump step. Root cause: `npm install --package-lock-only` in version.js triggers the root `prepare` lifecycle, which re-runs `tsc --build` while packages/core/dist/ already exists from the initial `npm ci`. The unbuilt acp-bridge reference (added in #4295 but missing from build.js) corrupts TypeScript's incremental project graph resolution. Fixes: 1. Add --ignore-scripts to the lock-file-only install in version.js 2. Add packages/acp-bridge to the build order in build.js Closes #4368, closes #4339, closes #4307
2 tasks
yiliang114
added a commit
that referenced
this pull request
May 21, 2026
The prepackage script called `npm run build` (full tsc compilation of all workspace packages) before bundling. This second tsc pass fails with TS5055 in CI because: 1. `npm ci` already built everything via the `prepare` lifecycle 2. `release:version` bumps package versions 3. `npm run generate` rewrites git-commit.ts with the new version 4. tsc --build detects stale tsbuildinfo, attempts full rebuild, but composite project references cause dist/*.d.ts to be resolved as input files → TS5055 The fix removes the redundant `npm run build` call. The CLI bundle is produced by esbuild directly from TypeScript source files (entry: packages/cli/index.ts), so compiled dist/ artifacts are not needed. The `npm run bundle` step already includes `npm run generate` to ensure git-commit.ts is up to date. Root cause introduced by #4295 (acp-bridge composite references).
yiliang114
added a commit
that referenced
this pull request
May 21, 2026
…#4401) The prepackage script called `npm run build` (full tsc compilation of all workspace packages) before bundling. This second tsc pass fails with TS5055 in CI because: 1. `npm ci` already built everything via the `prepare` lifecycle 2. `release:version` bumps package versions, making tsbuildinfo stale 3. The redundant `npm run build` triggers tsc --build which attempts a full rebuild, but composite project references cause dist/*.d.ts to be resolved as input files → TS5055 The fix removes the redundant `npm run build` call. The CLI bundle is produced by esbuild directly from TypeScript source files (entry: packages/cli/index.ts), so compiled dist/ artifacts are not needed. Root cause introduced by #4295 (composite project references).
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
First slice of #4175 Wave 5 PR 22 (
refactor(serve): extract acp bridge primitives). Lifts the three primitives frompackages/cli/src/serve/that have zero coupling to the rest ofserve/, so PR 22a can land ahead of PR 17 (#4282) and PR 14b (#4271) without merge-conflict risk onhttpAcpBridge.ts. Also seeds thePermissionMediatorinterface contract that PR 24 will implement (4 strategies —first-responder/designated/consensus/local-only), replacing the inline first-responder voting inBridgeClient.requestPermission.This was the "Stage 1.5-prereq AcpChannel lift" originally scoped by chiga0 in #3803.
What moves (verified zero coupling,
git mvpreserves blame)cli/src/serve/eventBus.ts(578 LOC, no internal imports) →packages/acp-bridge/src/eventBus.tscli/src/serve/inMemoryChannel.ts(73 LOC, only depends on@agentclientprotocol/sdk) →packages/acp-bridge/src/inMemoryChannel.tshttpAcpBridge.ts:638-677AcpChannel/AcpChannelExitInfo/ChannelFactorytypes →packages/acp-bridge/src/channel.ts.test.tsfiles moved alongside their sourcesWhat's new
packages/acp-bridge/— internal package@qwen-code/acp-bridge, not published to npm yetpackages/acp-bridge/src/permission.ts— type-onlyPermissionMediator/PermissionPolicy/PermissionResolution/PermissionRequestRecord/PermissionVote/PermissionVoteOutcome. No implementation; PR 24 fills it in. Inline FIXME athttpAcpBridge.ts:1096-1106already named the four strategies — this slice declares them.packages/cli/src/serve/eventBus.tsandinMemoryChannel.tsare now one-line re-export wrappers for backward compathttpAcpBridge.ts:638-677is now a one-lineimport type+export typere-exportBackward compatibility
./eventBus.js,./inMemoryChannel.js,../serve/eventBus.js) keep resolving via the wrappers — no churn for the 8 importer sites incli/src/serve/pluscli/src/commands/serve.ts:14httpAcpBridge.tscontinues to exportAcpChannel/AcpChannelExitInfo/ChannelFactoryso any external consumer is unaffected/capabilitiespayload changesWhat's NOT in this slice (deferred to PR 22b after PR 17 + PR 14b land)
BridgeClient+createHttpAcpBridgefactory +defaultSpawnChannelFactory(~3000 LOC, lines 1082-3770 + 4106-4307 ofhttpAcpBridge.ts)packages/channels/base/AcpBridge.ts:61) and VSCode IDE companion (packages/vscode-ide-companion/src/services/acpConnection.ts:132) own-spawn migrationsFileSystemServiceparameterization throughBridgeClient.writeTextFile/readTextFile(PR 18 introduced the boundary; PR 22b will wire it)Test plan
npm test --workspace @qwen-code/acp-bridge— 28/28 pass at the new location (eventBus 20 + inMemoryChannel 8)cd packages/cli && npx vitest run src/serve/— 567/567 pass, no regressionscd packages/cli && npx tsc --noEmit— cleantsc --build packages/acp-bridge packages/cli— cleangrep -r "from.*serve/eventBus" packages/cli/src/confirms wrapper resolves forcommands/serve.ts:14References
httpAcpBridge.ts:1096-1106— FIXME for the fourPermissionMediatorstrategies this slice declares@qwen-code/acp-bridgeinstead ofcli/src/serve/, locking out future churn from PR 22b