refactor(serve): typed errors for channel-closed and missing-cli-entry (#4299)#4300
Open
doudouOUC wants to merge 1 commit into
Open
refactor(serve): typed errors for channel-closed and missing-cli-entry (#4299)#4300doudouOUC wants to merge 1 commit into
doudouOUC wants to merge 1 commit into
Conversation
#4299) Replace regex-on-`.message` classification in `mapDomainErrorToErrorKind` with two new typed error classes living next to `BridgeTimeoutError` in `status.ts`: - `BridgeChannelClosedError(context)` — replaces three `throw new Error('agent channel closed …')` sites in `httpAcpBridge.ts` (`getTransportClosedReject`, `getChannelClosedReject`, restore-time `transportClosed`). The `context` field preserves the legacy message wording verbatim so log greps and existing diagnostics keep working. - `MissingCliEntryError()` — replaces the generic `Error` thrown by `defaultSpawnChannelFactory` when neither `QWEN_CLI_ENTRY` nor `process.argv[1]` resolves. Message bytes preserved. `mapDomainErrorToErrorKind` now branches on `instanceof` for these two kinds, and the regex fallbacks (plus the TODO that called them out) are removed entirely — there are no remaining throw sites we don't control. The test suite gains a regression case: wrapping or foreign errors whose `.message` happens to contain "agent channel closed" or "Cannot determine CLI entry path" no longer misclassify (returns `undefined`, the correct behavior per the issue). Bridge-internal classes don't cross the JSON-RPC boundary into the ACP child, so `instanceof` symmetry is preserved at every consumer of `mapDomainErrorToErrorKind` (3 sites in `httpAcpBridge.ts`, 6 in `acpAgent.ts`). Closes #4299 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
📋 Review SummaryThis PR addresses #4299 by replacing fragile regex-based error classification with typed error classes ( 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces two regex-based error classifications in mapDomainErrorToErrorKind with instanceof checks against two new typed error classes, eliminating false-positive misclassification for foreign errors whose messages happen to contain bridge-specific phrases.
Changes:
- Add
BridgeChannelClosedErrorandMissingCliEntryErrorclasses instatus.ts, preserving legacy message wording. - Replace three
new Error('agent channel closed …')sites and onenew Error('Cannot determine CLI entry path …')site with the typed classes. - Update
mapDomainErrorToErrorKindto branch oninstanceof, remove the regex fallbacks/TODO, and add tests covering the new classes plus a regression test for foreign messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/cli/src/serve/status.ts | Adds two typed error classes; replaces regex fallbacks with instanceof checks. |
| packages/cli/src/serve/status.test.ts | Adds class behavior tests and a regression test asserting foreign messages no longer misclassify. |
| packages/cli/src/serve/httpAcpBridge.ts | Throws the new typed errors at the four call sites instead of generic Error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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. |
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
mapDomainErrorToErrorKindinpackages/cli/src/serve/status.tsno longer regex-matches.messageto classify two failure modes — it now branches oninstanceof BridgeChannelClosedError/instanceof MissingCliEntryError, two new typed classes added next toBridgeTimeoutError. The four throw sites (3×agent channel closed …inhttpAcpBridge.ts, 1×Cannot determine CLI entry path …indefaultSpawnChannelFactory) now throw the typed classes. The regex fallbacks and the in-codeTODOthat called them out are removed..messagemis-classifies any future error whose message happens to contain those phrases (e.g., a wrapping error likeRuntimeError: failed: <inner: agent channel closed>), and silently degrades classification when a maintainer copy-edits the throw-site wording. Typed errors make the contract structural rather than lexical, matching the discipline the bridge already follows for its 11 other error classes (SessionNotFoundError,WorkspaceMismatchError, etc.).status.ts(next toBridgeTimeoutError), not inhttpAcpBridge.tswith the 11 others. Reason:mapDomainErrorToErrorKindis instatus.ts, andacpAgent.ts(the ACP child) importsstatus.ts— pulling bridge-implementation intostatus.tswould break that layering. Same precedent asBridgeTimeoutError.BridgeChannelClosedErrortakes acontextstring so each of the three call-sites' messages reproduce byte-for-byte ('agent channel closed mid-request (session abc)','… (workspace status)','… during session/load').MissingCliEntryErrorhas no constructor args and emits the same multi-line message verbatim.status.test.ts:173-189locks in the intended tightening: a wrapping error likenew Error('wrapped: agent channel closed mid-request')now returnsundefined(correct), where the old regex returned'protocol_error'(false positive, the bug fixed).Diff is ~50 LOC across 3 files, exactly the scope the issue estimated.
Validation
cd packages/cli && npx vitest run src/serve/status.test.ts— see the newBridgeChannelClosedError/MissingCliEntryErrordescribe blocks plus the foreign-error regression test.Scope / Risk
Error('agent channel closed …')and relying on the helper to tag it asprotocol_errorwill now getundefined. We searchedpackages/for both phrases — only one out-of-scope hit (commands/channel/config-utils.ts:25, theqwen channel startCLI path) which never reachesmapDomainErrorToErrorKind, so safe. Also confirmed no telemetry/alerting layer keys on these tags being non-undefined; consumers inacpAgent.tsandhttpAcpBridge.tsonly render the cell payload.qwen serve --port 0smoke run (typed-class change is internal to error classification; no behavior change at HTTP route or/capabilitiespayload).errorKindpayload values for these two failure modes are unchanged ('protocol_error'and'missing_binary'). New classes are additions tostatus.ts's exports — purely additive.Testing Matrix
Testing matrix notes:
vitest runcovers the new class behavior + regression. CI should cover Linux/Windows; no platform-specific code paths in this diff.Linked Issues / Bugs
Closes #4299
References:
packages/cli/src/serve/); the verbatim move in 22b/1 will carry these changes through.🤖 Generated with Qwen Code