refactor(acp-bridge): lift BridgeOptions + introduce DaemonStatusProvider seam (#4175 PR 22b/2)#4304
Conversation
📋 Review SummaryThis PR lifts 🔍 General Feedback
🎯 Specific Feedback🟡 High Priority
🟢 Medium Priority
🔵 Low Priority
✅ Highlights
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Stage 1 HTTP→ACP bridge construction contract by lifting BridgeOptions into @qwen-code/acp-bridge and introducing an injectable DaemonStatusProvider seam so the bridge no longer directly imports daemon-host helpers for env/preflight status.
Changes:
- Added
DaemonStatusProvider+ movedBridgeOptionsto@qwen-code/acp-bridge/bridgeOptions(and re-exported from the package root). - Introduced
packages/cli/src/serve/daemonStatusProvider.tsas the production implementation wrapping env snapshot + daemon-side preflight checks. - Updated the bridge and
runQwenServewiring to delegate env/preflight toopts.statusProviderwith a documented idle fallback when omitted.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/runQwenServe.ts | Wires createDaemonStatusProvider() into BridgeOptions.statusProvider for production qwen serve. |
| packages/cli/src/serve/httpAcpBridge.ts | Delegates env/preflight status to statusProvider and re-exports lifted BridgeOptions/DaemonStatusProvider. |
| packages/cli/src/serve/httpAcpBridge.test.ts | Defaults test bridge construction to a real statusProvider, with opt-out for idle fallback tests. |
| packages/cli/src/serve/daemonStatusProvider.ts | Adds the daemon-host implementation for env snapshot + daemon-side preflight cells. |
| packages/acp-bridge/src/index.ts | Re-exports bridgeOptions from the @qwen-code/acp-bridge package root. |
| packages/acp-bridge/src/bridgeOptions.ts | Introduces the shared BridgeOptions contract and the DaemonStatusProvider interface. |
| packages/acp-bridge/package.json | Adds the ./bridgeOptions subpath export. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { | ||
| BridgeOptions, | ||
| DaemonStatusProvider, | ||
| } from '@qwen-code/acp-bridge/bridgeOptions'; | ||
| export type { BridgeOptions, DaemonStatusProvider }; |
There was a problem hiding this comment.
Verified — declined. Ran `npx eslint src/serve/httpAcpBridge.ts` from `packages/cli` and got exit 0 with no output. The `import/first` rule isn't enforced on this file (the project's eslint config doesn't include the recommended preset that would enable it). The shape "imports interleaved with exports" is consistent with the patterns PR 22b/1 already landed — `bridgeErrors` and `bridgeTypes` blocks both follow the same import/export interleaving and have been clean across CI. Happy to revisit if a future eslint config bump activates the rule.
…ider 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).
48d5887 to
50dfe52
Compare
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. |
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.
|
@wenshao @chiga0 — review request for the design slice of PR 22b. The mechanical bulk lift (PR 22b/3) doesn't depend on this PR's code, but it does depend on the Where to focusThe 6 decisions baked into
What's not in this PRThe actual Bonus#4300 (your typed-error refactor closing #4299) was merged before this PR rebased — the rebase was clean and the 701/701 cli serve tests + acp-bridge tests still pass with If you want a quicker decline path, I'm happy to drop |
| // half is empty `[]`; ACP-side cells are still fetched normally | ||
| // when a child is live. | ||
| const daemonCells: ServePreflightCell[] = opts.statusProvider | ||
| ? await opts.statusProvider.getDaemonPreflightCells(boundWorkspace) |
There was a problem hiding this comment.
[Critical] opts.statusProvider.getDaemonPreflightCells(boundWorkspace) is awaited without a try/catch. If a custom DaemonStatusProvider implementation throws, the error propagates past the bridge to the route handler — the entire GET /workspace/preflight response fails, losing both daemon cells AND the ACP-side cells that would have been fetched below.
Pre-PR, buildDaemonPreflightCells used Promise.allSettled internally and was effectively unthrowable. The DaemonStatusProvider interface contract documents return types but not this "never throw" invariant, so a custom implementation that throws silently breaks the "daemon cells always render" guarantee.
The same pattern applies to getEnvStatus at line 3144 — a throwing provider causes the entire /workspace/env envelope to fail.
| ? await opts.statusProvider.getDaemonPreflightCells(boundWorkspace) | |
| let daemonCells: ServePreflightCell[]; | |
| try { | |
| daemonCells = opts.statusProvider | |
| ? await opts.statusProvider.getDaemonPreflightCells(boundWorkspace) | |
| : []; | |
| } catch (err) { | |
| // Provider threw — fall back to empty so ACP cells still render. | |
| // Matches pre-PR invariant: daemon cells always answerable. | |
| console.error('[bridge] statusProvider.getDaemonPreflightCells failed:', err); | |
| daemonCells = []; | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Adopted in c6bfd5af7. Wrapped both getEnvStatus and getDaemonPreflightCells calls in try/catch with stderr-logged fallback. The route-level "daemon cells always render" invariant is now structural — pre-injection it was implicit (because buildDaemonPreflightCells used Promise.allSettled and buildEnvStatusFromProcess was synchronous self-contained), post-injection it's explicit at the call site so any custom DaemonStatusProvider that throws degrades gracefully. Added 2 new tests pinning the catch behavior:
falls back to idle env envelope when statusProvider.getEnvStatus throwsfalls back to empty daemon cells when statusProvider.getDaemonPreflightCells throws
Both use a throwingProvider and assert the route still answers with the idle shape.
| * exercise the no-provider idle fallback can override with | ||
| * `{ statusProvider: undefined }`. | ||
| */ | ||
| function makeBridge(opts: Partial<BridgeOptions> = {}): HttpAcpBridge { |
There was a problem hiding this comment.
[Suggestion] makeBridge always provides createDaemonStatusProvider(), and the JSDoc mentions { statusProvider: undefined } as the opt-out — but no test in the diff actually exercises this path. The new fallback branches in getWorkspaceEnvStatus (idle envelope, line 3146) and getWorkspacePreflightStatus (empty daemon cells, line 3160) are entirely untested.
Consider adding at least one test that creates a bridge with { statusProvider: undefined } and asserts the idle envelope shape:
it('returns idle env envelope when statusProvider is omitted', async () => {
const bridge = makeBridge({ statusProvider: undefined });
const env = await bridge.getWorkspaceEnvStatus();
expect(env).toMatchObject({
v: expect.any(Number),
workspaceCwd: WS_A,
initialized: true,
acpChannelLive: false,
cells: [],
});
});— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Adopted in c6bfd5af7. Added 2 fallback-path tests:
returns idle env envelope when statusProvider is omitted (Mode A fallback)returns empty daemon preflight cells when statusProvider is omitted (Mode A fallback)
Both construct the bridge with { statusProvider: undefined } (the documented Mode A integration pattern) and assert the idle envelope shape — covering the previously-untested no-provider branches in getWorkspaceEnvStatus (line 3146) and getWorkspacePreflightStatus (line 3160). Plus 2 more tests for the throws-fallback path (per your [Critical] comment) so all 4 fallback branches now have coverage.
| if (opts.statusProvider) { | ||
| return opts.statusProvider.getEnvStatus(boundWorkspace, acpChannelLive); | ||
| } | ||
| return { |
There was a problem hiding this comment.
[Suggestion] This inline fallback creates a second construction site for ServeWorkspaceEnvStatus alongside buildEnvStatusFromProcess in envSnapshot.ts. If a future optional field is added to the production builder, this fallback silently diverges — TypeScript won't flag the omission because the field is optional, and the two sites live in different files.
Consider extracting a createIdleEnvStatus(workspaceCwd, acpChannelLive) helper (similar to the existing createIdleAcpPreflightCells pattern) so there's a single construction site:
return createIdleEnvStatus(boundWorkspace, acpChannelLive);— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Adopted in c6bfd5af7. Extracted createIdleEnvStatus(workspaceCwd, acpChannelLive) to acp-bridge/src/status.ts (placed next to createIdleWorkspaceProvidersStatus for consistency with the other createIdle* factories). Bridge factory now calls it from both the no-provider branch and the throwing-provider catch — single construction site, future optional fields on ServeWorkspaceEnvStatus only need updating in envSnapshot.ts buildEnvStatusFromProcess and this helper.
| workspaceCwd: boundWorkspace, | ||
| initialized: true, | ||
| acpChannelLive, | ||
| cells: [], |
There was a problem hiding this comment.
[Suggestion] Omitting statusProvider now makes the default bridge return an empty env/preflight daemon surface. runQwenServe() wires createDaemonStatusProvider(), but createServeApp() still builds its default bridge without one, so direct createServeApp() callers that do not inject deps.bridge silently lose the daemon env cells and daemon preflight cells that the default server app reported before this PR.
Please either wire the production statusProvider into the default bridge constructed by createServeApp(), or make that dependency explicit there so only callers that intentionally construct createHttpAcpBridge() without a provider get the empty fallback.
— gpt-5.5 via Qwen Code /review
There was a problem hiding this comment.
Good catch — adopted in c6bfd5af7. Wired createDaemonStatusProvider() into the default bridge constructed by createServeApp() in server.ts. Now symmetric with runQwenServe.ts: any caller of createServeApp() that doesn't inject deps.bridge gets the same daemon env + preflight cells they got pre-injection. Direct createHttpAcpBridge() callers (Mode A in-process consumers, future) still get the optional opt-out via statusProvider: undefined, which is the documented integration pattern.
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.
…#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
…eview) 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.
| // reported pre-injection. Wiring the production status provider | ||
| // here preserves byte-for-byte route output on the default | ||
| // bridge construction path. | ||
| statusProvider: createDaemonStatusProvider(), |
There was a problem hiding this comment.
[Suggestion] The default createServeApp path now injects createDaemonStatusProvider(), but the route tests for /workspace/env and /workspace/preflight still exercise those endpoints through an injected fake bridge. That means a future regression that removes or mis-threads this default statusProvider would make production return the idle/no-provider cells while the current route tests still pass.
Please add route-level coverage that constructs createServeApp({ ...baseOpts, workspace: <real temp dir> }) without deps.bridge and asserts that /workspace/env returns non-empty env cells and /workspace/preflight includes daemon-local cells such as node_version, cli_entry, workspace_dir, ripgrep, git, and npm.
— gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen-latest-series-invite-beta-v28 via Qwen Code /review
Local validation report (post-merge)Verified head Build & static checks
Unit tests
Backward-compat verification
🎯 Byte-level wire parity (the load-bearing claim)Started the daemon on this branch, captured The 119-line preflight envelope (with daemon-half cells CaveatsTwo test failures ( VerdictApproved and merged. Design slice cleanly executed:
PR 22b/2 is the right shape to freeze the contract before PR 22b/3 ships the ~3000-LOC mechanical |
Co-authored-by: Scott Densmore <scottdensmore@mac.com>
Summary
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 helpers (`buildEnvStatusFromProcess`, `buildDaemonPreflightCells`).
The mechanical bulk lift of `BridgeClient` + spawn factory + `createHttpAcpBridge` factory closure (~3000 LOC) lands in PR 22b/3 against this stable contract — that follow-up has zero design decisions; it just moves code and consumes `DaemonStatusProvider` per the contract this PR freezes.
Review focus on this PR: the shape of `DaemonStatusProvider`. Five decision points spelled out in the per-field JSDoc — argument granularity (positional vs context object), return-type granularity (full envelope vs cells), abort/timeout support, optional vs required, single combined interface vs split.
Decisions baked in
What moves
What's new
Bridge factory body changes
Wiring
Backward compatibility
Test plan
Remaining PR 22b checkpoints
22b/3 is purely mechanical — zero design decisions; suitable for IDE-driven manual `git mv` (1-2 hours).
References