refactor(serve): extract createInMemoryChannel helper (#4156 A1)#4160
Merged
Conversation
…dge.test.ts (#4156 A1) Sub-PR A1 of issue #4156 (Stage 1.5b Mode A daemon). Pure refactor with zero behavior change. Extracts the inline paired NDJSON channel construction (`new TransformStream` × 2 + `ndJsonStream` × 2) that was duplicated across `httpAcpBridge.test.ts` into a production helper `createInMemoryChannel()` at `packages/cli/src/serve/inMemoryChannel.ts`. The helper is added to `packages/cli/src/serve/index.ts`'s barrel export alongside the rest of the serve module's public API. The helper is intentionally bare — it returns only the stream pair, no lifecycle / teardown surface. Two reasons: 1. Consumer behavior diverges widely (stuck channel, crashable child simulation, no-op, real in-process termination); a one-size-fits-all `close()` would either pull test-fixture concerns into a production module or force a single shape on consumers that don't want it. 2. The SDK's `ndJsonStream` outer wrapper does not reliably propagate close on `Stream.writable` to the opposite `Stream.readable`; consumers needing to simulate a child exit hold their own underlying `TransformStream` references and close those directly. 10 of 11 inline call sites in `httpAcpBridge.test.ts` migrate cleanly to the new helper. The 11th (`makeChannel` at line 151) keeps the inline 4-line construction because its `kill()` closure needs the underlying `ab` / `ba` writables to simulate child-process termination — a comment above the function explains the asymmetry. The helper is also a primitive for the future A2 PR's `inProcessAcpBridge.ts`, which will use it to wrap an in-process `QwenAgent` without spawning a `qwen --acp` child (see issue #4156 §3 decision 1 and §8). Test plan: - New `inMemoryChannel.test.ts`: 5 tests covering bidirectional round-trip, ordering preservation, and bidirectional direction isolation - Existing `httpAcpBridge.test.ts`: 70 tests, identical count and behavior before vs after migration - `vitest run packages/cli/src/serve/inMemoryChannel.test.ts packages/cli/src/serve/httpAcpBridge.test.ts` — 75/75 pass - `tsc --noEmit -p packages/cli/tsconfig.json` — clean for changed files 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
22 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the serve subsystem’s test-only in-memory ACP stream wiring into a reusable production helper (createInMemoryChannel) intended to be a primitive for upcoming Stage 1.5b in-process bridge work, while updating existing bridge tests to use the helper and adding targeted unit tests for it.
Changes:
- Added
createInMemoryChannel()helper that constructs a paired in-memory NDJSONStreamduplex (client ↔ agent). - Migrated 10 duplicated inline stream-pair constructions in
httpAcpBridge.test.tsto the new helper. - Added a dedicated
inMemoryChannel.test.tssuite to validate round-trip, ordering, and direction isolation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cli/src/serve/inMemoryChannel.ts | Adds the extracted in-memory NDJSON stream-pair helper (now part of serve’s public API). |
| packages/cli/src/serve/inMemoryChannel.test.ts | Introduces unit tests for the helper’s bidirectional behavior and isolation properties. |
| packages/cli/src/serve/index.ts | Exposes createInMemoryChannel via the serve barrel export. |
| packages/cli/src/serve/httpAcpBridge.test.ts | Replaces duplicated inline stream wiring at 10 sites with the new helper (keeps one inline site for kill semantics). |
Comments suppressed due to low confidence (1)
packages/cli/src/serve/inMemoryChannel.test.ts:145
- Same issue as the previous test: a pending
reader.read()is raced against a timeout, but the read promise is left outstanding andreleaseLock()is called without cancelling/handling it. This can lead to rejected read promises (unhandled) or intermittent failures depending on the stream implementation.
const reader = agentStream.readable.getReader();
try {
const winner = await Promise.race([
reader.read().then(() => 'leaked' as const),
new Promise<'isolated'>((res) => setTimeout(() => res('isolated'), 50)),
]);
expect(winner).toBe('isolated');
} finally {
reader.releaseLock();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
📋 Review SummaryThis PR extracts duplicated inline NDJSON channel construction logic (4 lines × 10 sites) from 🔍 General Feedback
🔴 CriticalNo critical issues identified. 🟡 HighNo high priority issues identified. 🟢 Medium
🔵 Low
✅ Highlights
|
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. |
…nnel Two small follow-ups from #4160 review: 1. inMemoryChannel.test.ts:113,137 — handle the pending `reader.read()` that the isolation tests intentionally leave hanging when the timeout wins the race. `reader.releaseLock()` in `finally` rejects that pending read per Web Streams spec; without a rejection handler this could surface as an unhandled rejection / flaky test signal. Added a no-op rejection handler via the two-arg `.then(onResolve, onReject)` form so the cleanup-path rejection settles cleanly. 2. inMemoryChannel.ts:11 — the JSDoc said "two `TransformStream<...>` pairs" which reads ambiguously as "two pairs of TransformStream" (i.e., 4 streams). The implementation creates exactly two TransformStreams (one per direction). Reworded to "two `TransformStream<...>` instances (one per direction)" to disambiguate. Tests still 5/5 pass. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Clean refactor — verified end-to-end:
- All 10 migration sites use only
clientStream/agentStreampost-construction;ab/bareferences survive only in the deliberately-retainedmakeChannel(whosekill()closure needs them), which is locked in by the type system itself. - 5 new tests cover round-trip (× 2 directions), order preservation, and direction-isolation (× 2 directions). The isolation
Promise.raceagainst a 50ms timer correctly fails an aliased/echo impl: an early read-rejection settles the race branch tonull, not'isolated', so the assertion still flags real regressions. - The post-
releaseLockrejection handler (two-arg.then(onResolve, onReject)) only swallows the cleanup-path rejection that fires AFTER the race settles — does not mask premature-close failures. - Deterministic checks:
tsc --noEmitclean, eslint clean,npm run buildclean, 75/75 vitest pass (5 new + 70 migrated). - The two prior Copilot review threads (JSDoc "two pairs" wording, isolation-test releaseLock handling) are addressed in 374546d.
— claude-opus-4-7 via Claude Code /qreview
wenshao
approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. Clean refactor — 10 duplicated inline TransformStream + ndJsonStream construction sites correctly extracted into a well-documented createInMemoryChannel() helper. Thorough JSDoc on design rationale (no lifecycle surface, close propagation limitation). 5 targeted unit tests covering bidirectional round-trip, ordering, and direction isolation. All 75 tests pass (5 new + 70 existing). Deterministic analysis (tsc + eslint) clean on changed files.
— glm-5.1 via Qwen Code /review
wenshao
requested changes
May 15, 2026
…hannel + route test through barrel Two follow-ups from #4160 review: 1. Expose `abort(reason?)` on the helper return value (per @wenshao critical comment). Reasoning: the helper previously returned only the `Stream` pair, leaving consumers no way to tear the channel down. `ndJsonStream`'s outer wrapper does not reliably propagate `close()`, but `abort()` on the underlying byte-level `TransformStream` is forceful-by-spec — pending reads on both sides settle immediately so GC can reclaim. This unblocks the future Stage 1.5b in-process bridge (#4156, sub-PR A2) which needs teardown on daemon shutdown. The settlement shape is documented honestly in JSDoc: at the inner byte-level layer pending reads reject with the supplied reason; at the outer SDK-wrapped `Stream` the wrapper translates that into a clean `{done: true}` signal. Either way, pending operations no longer hang — that's the teardown invariant we care about. 2. Route the test's import through the `serve/index.js` barrel rather than the source file (per @wenshao suggestion). Without a test that exercises the public API path, a typo or missing re-export in the barrel would go undetected in CI. Tests: 8/8 helper tests pass (5 existing + 3 new abort tests covering teardown invariant + idempotency + no-reason variant). 70/70 existing httpAcpBridge tests still pass. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Both points from the prior CHANGES_REQUESTED on 374546d2f are addressed in 83d36c00d:
- abort() teardown primitive —
createInMemoryChannel()now returns{ clientStream, agentStream, abort(reason?) }. Implementation callsWritableStream.abort()on bothTransformStreams and swallows the expected post-abort rejections. JSDoc documents the settlement-shape subtlety (SDK'sndJsonStreamouter wrapper translates the inner reject into a clean{done: true}at the message layer) and the rationale for exposingabortrather thanclose. - Barrel export exercised by CI —
inMemoryChannel.test.ts:12now imports from./index.js(the barrel), so a missing/broken re-export will fail CI immediately. The 10 migration sites inhttpAcpBridge.test.tscontinue to import from the source file by intent (colocated package internals).
Local verification on 83d36c00d:
| Check | Result |
|---|---|
inMemoryChannel.test.ts |
8/8 pass (3 new abort-specific: settle-pending-readers, idempotency, no-reason) |
| Flakiness × 10 runs (100ms abort race) | 10/10 clean |
httpAcpBridge.test.ts (10 migrated sites) |
70/70 pass |
ESLint --max-warnings 0 on changed files |
clean |
tsc --noEmit on changed files |
clean (27 unrelated errors exist on main too — stale packages/core/dist) |
| Full cli suite | 5986 pass; 10 fail = all reproduce on main (8× channel/config-utils from stale channels/base/dist, 1× doctorChecks Node-20-vs-22 env, 1× flaky languageCommand) |
The abort() primitive resolves the teardown-hang risk the helper had as a pure-construction utility, and the barrel-routed test import closes the public-API-surface gap. Ready to merge.
— claude-opus-4-7 via Claude Code
wenshao
added a commit
to wenshao/codeagents
that referenced
this pull request
May 15, 2026
…A 启动 — 文档跟踪 [Issue #4156](QwenLM/qwen-code#4156) doudouOUC 提出 "proposal(serve): qwen --serve (Mode A) — TUI + in-process HTTP daemon, 3-phase plan (Stage 1.5b)"，直接引用本系列 §04 / §06 作为设计依据。 3-phase plan： - Phase A: Loopback-only minimal skeleton (~4.3d, 拆 A0/A1/A2/A3) - Phase B: Remote bind + auth/CORS defaults (~1d) - Phase C: Lifecycle coordination (Ctrl+C drain + 5s force + ink SIGINT 协调, ~1d) Phase A 4 sub-PR 拆分： - A0: 抽 validateAndCanonicalizeWorkspace helper from PR#4113 code (~50 LOC) - A1: 抽 createInMemoryChannel helper (~73 LOC) — ✅ PR#4160 MERGED 2026-05-15 - A2: 新 inProcessAcpBridge.ts wrapping in-process QwenAgent (~200 LOC) - A3: 集成到 gemini.tsx --serve flag handling [PR#4160](QwenLM/qwen-code#4160) MERGED 2026-05-15 06:43:07 UTC, merge commit ff63da26, +315/-40 across 4 files (含 73 LOC inMemoryChannel.ts + 224 LOC tests + 17/-40 httpAcpBridge.test.ts dedup)。 Pure refactor with zero behavior change. 关键技术决策（Issue #4156 §3）: 1. In-process bridge 用 paired channel + 完整 ACP（server.ts/eventBus.ts 0 改动）—— ~180-220 LOC，比 httpAcpBridge.ts ~2400 LOC 少一个量级（in-process 无 child / spawn race / SIGTERM grace） 2. TUI ↔ daemon session decoupled（Phase A 阶段；Phase D 才统一） 3. 默认 port 0 避免与 Mode B 4170 冲突 4. Phase A 拒绝远端 authenticate（避免清掉 TUI credentials） 5. 复用 PR#4113 --workspace flag + canonicalize 文档更新（3 文件）: - §01 §五 Stage 演进表 1.5b 行加 Issue #4156 引用 + A1 ship 状态 - §06 §三 Stage 1.5b 大幅扩展（3-phase 表 + 4 sub-PR 分解 + 9 项关键技术决策 + 已知限制清单） - README Stage 进展表 + 已合并 PR 表 + TL;DR 当前状态行 外部观察：Issue #4156 由第三方 contributor doudouOUC 提出，直接引用本系列文档 作为设计 anchor —— daemon-design 系列已成为社区设计输入。
wenshao
approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
QwenLM#4160) * refactor(serve): extract createInMemoryChannel helper from httpAcpBridge.test.ts (QwenLM#4156 A1) Sub-PR A1 of issue QwenLM#4156 (Stage 1.5b Mode A daemon). Pure refactor with zero behavior change. Extracts the inline paired NDJSON channel construction (`new TransformStream` × 2 + `ndJsonStream` × 2) that was duplicated across `httpAcpBridge.test.ts` into a production helper `createInMemoryChannel()` at `packages/cli/src/serve/inMemoryChannel.ts`. The helper is added to `packages/cli/src/serve/index.ts`'s barrel export alongside the rest of the serve module's public API. The helper is intentionally bare — it returns only the stream pair, no lifecycle / teardown surface. Two reasons: 1. Consumer behavior diverges widely (stuck channel, crashable child simulation, no-op, real in-process termination); a one-size-fits-all `close()` would either pull test-fixture concerns into a production module or force a single shape on consumers that don't want it. 2. The SDK's `ndJsonStream` outer wrapper does not reliably propagate close on `Stream.writable` to the opposite `Stream.readable`; consumers needing to simulate a child exit hold their own underlying `TransformStream` references and close those directly. 10 of 11 inline call sites in `httpAcpBridge.test.ts` migrate cleanly to the new helper. The 11th (`makeChannel` at line 151) keeps the inline 4-line construction because its `kill()` closure needs the underlying `ab` / `ba` writables to simulate child-process termination — a comment above the function explains the asymmetry. The helper is also a primitive for the future A2 PR's `inProcessAcpBridge.ts`, which will use it to wrap an in-process `QwenAgent` without spawning a `qwen --acp` child (see issue QwenLM#4156 §3 decision 1 and §8). Test plan: - New `inMemoryChannel.test.ts`: 5 tests covering bidirectional round-trip, ordering preservation, and bidirectional direction isolation - Existing `httpAcpBridge.test.ts`: 70 tests, identical count and behavior before vs after migration - `vitest run packages/cli/src/serve/inMemoryChannel.test.ts packages/cli/src/serve/httpAcpBridge.test.ts` — 75/75 pass - `tsc --noEmit -p packages/cli/tsconfig.json` — clean for changed files 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(serve): address Copilot review feedback on createInMemoryChannel Two small follow-ups from QwenLM#4160 review: 1. inMemoryChannel.test.ts:113,137 — handle the pending `reader.read()` that the isolation tests intentionally leave hanging when the timeout wins the race. `reader.releaseLock()` in `finally` rejects that pending read per Web Streams spec; without a rejection handler this could surface as an unhandled rejection / flaky test signal. Added a no-op rejection handler via the two-arg `.then(onResolve, onReject)` form so the cleanup-path rejection settles cleanly. 2. inMemoryChannel.ts:11 — the JSDoc said "two `TransformStream<...>` pairs" which reads ambiguously as "two pairs of TransformStream" (i.e., 4 streams). The implementation creates exactly two TransformStreams (one per direction). Reworded to "two `TransformStream<...>` instances (one per direction)" to disambiguate. Tests still 5/5 pass. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(serve): expose abort() teardown primitive on createInMemoryChannel + route test through barrel Two follow-ups from QwenLM#4160 review: 1. Expose `abort(reason?)` on the helper return value (per @wenshao critical comment). Reasoning: the helper previously returned only the `Stream` pair, leaving consumers no way to tear the channel down. `ndJsonStream`'s outer wrapper does not reliably propagate `close()`, but `abort()` on the underlying byte-level `TransformStream` is forceful-by-spec — pending reads on both sides settle immediately so GC can reclaim. This unblocks the future Stage 1.5b in-process bridge (QwenLM#4156, sub-PR A2) which needs teardown on daemon shutdown. The settlement shape is documented honestly in JSDoc: at the inner byte-level layer pending reads reject with the supplied reason; at the outer SDK-wrapped `Stream` the wrapper translates that into a clean `{done: true}` signal. Either way, pending operations no longer hang — that's the teardown invariant we care about. 2. Route the test's import through the `serve/index.js` barrel rather than the source file (per @wenshao suggestion). Without a test that exercises the public API path, a typo or missing re-export in the barrel would go undetected in CI. Tests: 8/8 helper tests pass (5 existing + 3 new abort tests covering teardown invariant + idempotency + no-reason variant). 70/70 existing httpAcpBridge tests still pass. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) (cherry picked from commit ff63da2)
42 tasks
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
Sub-PR A1 of #4156 (Stage 1.5b Mode A daemon). Pure refactor with zero behavior change.
Extracts the inline paired NDJSON channel construction (
new TransformStream× 2 +ndJsonStream× 2) that was duplicated acrosshttpAcpBridge.test.tsinto a production helpercreateInMemoryChannel()atpackages/cli/src/serve/inMemoryChannel.ts. The helper is added toserve/index.ts's barrel export alongside the rest of the module's public API.Why production module (not test-utils)
The
httpAcpBridge.ts:289-294FIXME explicitly names a future Stage 2InProcessAcpChannelthat needs exactly this primitive. Putting it inserve/aligns with whereAcpChanneland the channel factory live. This helper is also a primitive for the future A2 PR'sinProcessAcpBridge.ts, which will use it to wrap an in-processQwenAgentwithout spawning aqwen --acpchild (see #4156 §3 decision 1 and §8).Why bare API (no exited/crash/kill)
The 11 inline duplicates use only the stream-pair layer; their
exited/killshapes vary widely (stuck channel / crashable / no-op) and are bespoke to each test scenario. Bundling those into the helper would either pull test-fixture concerns into a production module or force callers into a one-size-fits-all child-process simulation. The SDK'sndJsonStreamouter wrapper also does not reliably propagate close onStream.writableto the oppositeStream.readable, so aclose()on this helper would have to either expose internals or be silently incomplete.Migration scope
10 of 11 inline call sites in
httpAcpBridge.test.tsmigrate cleanly to the new helper:setupForPermission()factorymodelServiceIdsetup factorysetupRecording()setupForFs()(file proxy) factorysetSessionModelsetup factoryThe 11th site (
makeChannelat line 151) keeps the inline 4-line construction because itskill()closure needs the underlyingab/bawritables to simulate child-process termination — a 6-line comment above the function explains the asymmetry. The bare helper deliberately does not expose those references; see the helper's JSDoc for the rationale.Behavior change
None. All 10 migrated sites' downstream behavior —
AgentSideConnectionconstruction,exitedpromise shapes,killclosures — is preserved verbatim. Only the 4-line stream-pair boilerplate is replaced with a singlecreateInMemoryChannel()call.Test plan
inMemoryChannel.test.ts: 5 tests covering bidirectional round-trip, ordering preservation, and bidirectional direction isolationnpx vitest run packages/cli/src/serve/inMemoryChannel.test.ts packages/cli/src/serve/httpAcpBridge.test.ts— 75/75 pass (5 new + 70 existing, identical count and behavior before vs after migration)npx tsc --noEmit -p packages/cli/tsconfig.json— clean for changed filespre-commithook (prettier + eslint) — passedStack context
This is the first of 4 stacked sub-PRs in the Phase A plan from #4156:
A1 has zero dependency on #4113, so it can review and land in parallel.
🤖 Generated with Qwen Code