feat(daemon): add POST /session/:id/language for runtime language switching#4705
feat(daemon): add POST /session/:id/language for runtime language switching#4705chiga0 wants to merge 3 commits into
Conversation
…tching Add a dedicated HTTP endpoint for switching UI language and LLM output language without polluting the session transcript. The endpoint flows through three layers (server route → bridge → ACP extMethod handler) following the same pattern as approval-mode and model switching. When syncOutputLanguage is true, the handler updates output-language.md, persists settings, and refreshes system prompts across all active sessions so the next LLM call immediately uses the new language. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
requested review from
doudouOUC and
wenshao
and removed request for
wenshao
📋 Review SummaryThis PR implements a runtime language-switching API endpoint ( 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Code Review Overview (AI Generated)PR: #4705 feat(daemon): add POST /session/:id/language for runtime language switching Multi-Round Review (Rounds 0-6): Clean — 0 findingsRound 0 (Design): Correct approach — three-layer flow (HTTP route → bridge extMethod → ACP handler) follows the established pattern used by Round 1 (Architecture): Clean implementation across all 5 files. Round 2 (Robustness):
Round 3 (Security): Round 4 (Performance): Round 5 (New Feature): Implementation matches PR description exactly — three-layer flow, language switching, optional output language sync, settings persistence, all-session refresh. Clean and focused, no unrelated changes. Round 6 (Undirected): Cross-file consistency verified — LGTM! This review was generated by QoderWork AI |
… debug logging - Replace hardcoded LANGUAGE_CODES array in server.ts with dynamically derived list from SUPPORTED_LANGUAGES, ensuring new languages added to the i18n module are automatically accepted by the API. - Add debugLogger.warn calls for settings persistence failures in the ACP handler instead of silently swallowing errors. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
There was a problem hiding this comment.
Pull request overview
Adds a new POST /session/:id/language HTTP endpoint that switches the daemon's UI language and (optionally) the LLM output language at runtime, wired through the standard server → bridge → ACP extMethod three-layer pattern. When syncOutputLanguage is true, the handler also rewrites ~/.qwen/output-language.md, persists the user setting, and refreshes every active session's system prompt.
Changes:
- New ext-method
qwen/control/session/language,HttpAcpBridge.setSessionLanguage, andlanguage_changedbus event. - New Express route
POST /session/:id/languagewith allow-list validation againstSUPPORTED_LANGUAGES + 'auto'. QwenAgenthandler inacpAgent.tsperforms the global UI language change, settings persistence, and per-session system-prompt refresh.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/acp-bridge/src/status.ts | Registers the new sessionLanguage ext-method constant. |
| packages/acp-bridge/src/bridgeTypes.ts | Declares the setSessionLanguage interface on HttpAcpBridge. |
| packages/acp-bridge/src/bridge.ts | Implements the bridge method: forwards via extMethod, publishes language_changed. |
| packages/cli/src/acp-integration/acpAgent.ts | Adds the ACP-side handler that mutates global UI language, persists settings, and refreshes all sessions. |
| packages/cli/src/serve/server.ts | Adds the HTTP route, language-code validation, and bridge call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| entry.events.publish({ | ||
| type: 'language_changed', | ||
| data: { | ||
| sessionId: entry.sessionId, | ||
| language: result.language, | ||
| outputLanguage: result.outputLanguage, | ||
| }, | ||
| ...(originatorClientId ? { originatorClientId } : {}), | ||
| }); |
| app.post('/session/:id/language', mutate(), async (req, res) => { | ||
| const sessionId = req.params['id']; | ||
| const body = safeBody(req); | ||
| const language = body['language']; | ||
| const syncOutputLanguage = body['syncOutputLanguage']; | ||
|
|
||
| if (typeof language !== 'string' || !LANGUAGE_CODES.includes(language)) { | ||
| res.status(400).json({ | ||
| error: | ||
| '`language` is required and must be one of: ' + | ||
| LANGUAGE_CODES.join(', '), | ||
| code: 'invalid_language', | ||
| allowed: LANGUAGE_CODES, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| syncOutputLanguage !== undefined && | ||
| typeof syncOutputLanguage !== 'boolean' | ||
| ) { | ||
| res.status(400).json({ | ||
| error: '`syncOutputLanguage` must be a boolean when provided', | ||
| code: 'invalid_sync_flag', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const clientId = parseClientIdHeader(req, res); | ||
| if (clientId === null) return; | ||
|
|
||
| try { | ||
| const response = await bridge.setSessionLanguage( | ||
| sessionId, | ||
| { | ||
| language, | ||
| syncOutputLanguage: syncOutputLanguage === true, | ||
| }, | ||
| clientId !== undefined ? { clientId } : undefined, | ||
| ); | ||
| res.status(200).json(response); | ||
| } catch (err) { | ||
| sendBridgeError(res, err, { | ||
| route: 'POST /session/:id/language', | ||
| sessionId, | ||
| }); | ||
| } | ||
| }); |
| async setSessionLanguage(sessionId, params, context) { | ||
| const entry = byId.get(sessionId); | ||
| if (!entry) throw new SessionNotFoundError(sessionId); | ||
| const info = channelInfoForEntry(entry); | ||
| if (!info || info.isDying) throw new SessionNotFoundError(sessionId); | ||
| const originatorClientId = resolveTrustedClientId( | ||
| entry, | ||
| context?.clientId, | ||
| ); | ||
|
|
||
| const result = (await Promise.race([ | ||
| withTimeout( | ||
| entry.connection.extMethod( | ||
| SERVE_CONTROL_EXT_METHODS.sessionLanguage, | ||
| { | ||
| sessionId, | ||
| language: params.language, | ||
| syncOutputLanguage: params.syncOutputLanguage, | ||
| }, | ||
| ), | ||
| initTimeoutMs, | ||
| SERVE_CONTROL_EXT_METHODS.sessionLanguage, | ||
| ), | ||
| getTransportClosedReject(entry), | ||
| ])) as { | ||
| language: string; | ||
| outputLanguage: string | null; | ||
| refreshed: boolean; | ||
| }; | ||
|
|
||
| try { | ||
| entry.events.publish({ | ||
| type: 'language_changed', | ||
| data: { | ||
| sessionId: entry.sessionId, | ||
| language: result.language, | ||
| outputLanguage: result.outputLanguage, | ||
| }, | ||
| ...(originatorClientId ? { originatorClientId } : {}), | ||
| }); | ||
| } catch { | ||
| /* bus closed */ | ||
| } | ||
|
|
||
| return { | ||
| language: result.language, | ||
| outputLanguage: result.outputLanguage ?? null, | ||
| refreshed: result.refreshed ?? false, | ||
| }; | ||
| }, |
| const allSessions = [...this.sessions.values()]; | ||
| await Promise.allSettled( | ||
| allSessions.map(async (s) => { | ||
| const cfg = s.getConfig(); | ||
| await cfg.refreshHierarchicalMemory(); | ||
| await cfg.getGeminiClient()?.refreshSystemInstruction(); | ||
| }), | ||
| ); | ||
| refreshed = true; | ||
| outputLanguage = resolved; | ||
| } |
| if (typeof language !== 'string' || !language) { | ||
| throw RequestError.invalidParams( | ||
| undefined, | ||
| 'Invalid or missing language', | ||
| ); | ||
| } | ||
|
|
||
| await setLanguageAsync(language); |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Overall
The three-layer pattern (server route → bridge → ACP extMethod) is well-followed, and deriving LANGUAGE_CODES dynamically from SUPPORTED_LANGUAGES (commit 2) is a good improvement over hardcoding. However, two critical issues need addressing before merge.
Additional items not covered by inline comments:
- No test coverage — 192 lines of production code, 0 lines of tests.
bridge.test.tshas tests forsetSessionApprovalMode(lines ~5083-5306);setSessionLanguageshould have comparable coverage: basic call path, session-not-found error, event publish verification, and concurrent-request serialization (if queue is added). language_changedevent has no consumer — the event type only appears in this PR's new code. No SSE consumer, no frontend handler, no test references it. If the consumer comes in a follow-up PR, please note that in the description.- Response lacks
sessionId—setSessionApprovalModereturns{ sessionId, mode, previous, persisted }.setSessionLanguagereturns{ language, outputLanguage, refreshed }withoutsessionId. Minor inconsistency but worth aligning for API uniformity.
| return response; | ||
| }, | ||
|
|
||
| async setSessionLanguage(sessionId, params, context) { |
There was a problem hiding this comment.
Critical: Missing concurrency serialization — race condition
setSessionApprovalMode serializes through entry.approvalModeQueue, setSessionModel through entry.modelChangeQueue. Both have extensive comments explaining why (bridge.ts:3314-3320 on the base branch):
"Covering only the
extMethodcall left persist+publish OUTSIDE the queue: two concurrent calls could interleave their persist phases and publish out of order, so the bus's lastapproval_mode_changeddisagreed with the mode the ACP child actually settled on."
setSessionLanguage has no serialization queue at all. Two concurrent POST /session/:id/language requests can race:
- ACP child settles on one language, but the bridge publishes the other request's
language_changedevent last settings.setValuecalls interleave
Suggest: add a languageChangeQueue following the same pattern as approvalModeQueue.
| /* bus closed */ | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
Critical: Missing workspace broadcast for peer sessions
When syncOutputLanguage: true, the ACP child refreshes system prompts for ALL sessions internally. But the bridge only publishes language_changed to the requesting session's event bus (entry.events.publish).
Compare with setSessionApprovalMode which calls broadcastWorkspaceEvent(...) when persisted is true (bridge.ts:3371-3385 on the base branch), so peer sessions' SSE consumers also get the event.
Without the broadcast, other sessions' frontends won't know the language changed — their UI state will be stale even though the ACP child already switched the underlying system prompts.
| setSessionLanguage( | ||
| sessionId: string, | ||
| params: { language: string; syncOutputLanguage: boolean }, | ||
| context?: BridgeClientRequestContext, |
There was a problem hiding this comment.
Missing previous language in return type
Both peer methods include the prior state:
setSessionApprovalMode→{ previous: ApprovalMode, ... }setSessionModel→{ previous, current }
This return type doesn't include what language was active before the switch, making it impossible for consumers to know what changed from. The language_changed event data also lacks a previous field.
Suggest adding previousLanguage: string (or similar) to both the response and event data.
| const current = config.getApprovalMode(); | ||
| return { previous, current }; | ||
| } | ||
| case SERVE_CONTROL_EXT_METHODS.sessionLanguage: { |
There was a problem hiding this comment.
Missing sessionOrThrow() call
The handler validates sessionId is a non-empty string but never calls this.sessionOrThrow(sessionId) to verify the session actually exists in the ACP child. Every other extMethod handler in this switch follows that pattern:
sessionRecap→this.sessionOrThrow(sessionId)(line ~2535 on base)sessionBtw→this.sessionOrThrow(sessionId)(line ~2574 on base)sessionShellHistory→this.sessionOrThrow(sessionId)(line ~2616 on base)
Since setLanguageAsync and settings.setValue are global operations, a request targeting a non-existent sessionId will succeed silently (the bridge validates at its layer, but the ACP handler is also a standalone contract). Add this.sessionOrThrow(sessionId) for consistency and defense-in-depth.
| ); | ||
| } | ||
|
|
||
| await setLanguageAsync(language); |
There was a problem hiding this comment.
Suggestion: wrap setLanguageAsync with error handling
setLanguageAsync(language) is called without try-catch, while settings.setValue calls below are both wrapped. If the i18n module fails (e.g., missing translation file for an edge-case locale), the error propagates as an unhandled 500 through the extMethod.
Consider wrapping with try-catch + debugLogger.warn for consistency with the persist calls below.
| await cfg.getGeminiClient()?.refreshSystemInstruction(); | ||
| }), | ||
| ); | ||
| refreshed = true; |
There was a problem hiding this comment.
refreshed: true even when individual sessions failed
Promise.allSettled silently swallows per-session errors. If refreshHierarchicalMemory() or refreshSystemInstruction() fails for some sessions, those sessions' system prompts won't pick up the new language, but the response reports refreshed: true unconditionally.
Consider logging failures:
const results = await Promise.allSettled(...);
const failed = results.filter(r => r.status === 'rejected');
if (failed.length > 0) {
debugLogger.warn(
`Language refresh failed for ${failed.length}/${allSessions.length} sessions`,
);
}
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] No tests for the new POST /session/:id/language endpoint. +192 lines of new logic across 3 layers (server route, bridge forwarding, ACP handler) with zero test coverage. Existing server.test.ts has analogous test suites for model and approval-mode endpoints covering success, validation errors, 404, client identity forwarding, and bridge error propagation. Critical branches uncovered: valid/invalid language, non-boolean syncOutputLanguage, missing session (404), client-id forwarding, and the syncOutputLanguage=true path that mutates settings files and refreshes all sessions.
[Suggestion] Missing session_language capability registry entry in capabilities.ts. Every other session-level mutation endpoint has one (session_set_model, session_approval_mode_control, session_recap, session_btw). SDK clients preflighting via GET /capabilities cannot discover language-switching support.
— qwen3.7-max via Qwen Code /review
| ? OUTPUT_LANGUAGE_AUTO | ||
| : resolved; | ||
|
|
||
| updateOutputLanguageFile(settingValue); |
There was a problem hiding this comment.
[Critical] updateOutputLanguageFile(settingValue) performs synchronous fs.mkdirSync + fs.writeFileSync without try/catch. If it throws (read-only FS, disk full, permission denied), setLanguageAsync and settings.setValue('general.language') have already executed, leaving the system in a partially-applied state. The adjacent settings.setValue calls are both wrapped — this one was missed.
| updateOutputLanguageFile(settingValue); | |
| try { | |
| updateOutputLanguageFile(settingValue); | |
| } catch (err) { | |
| debugLogger.warn('Failed to write output language file:', err); | |
| } |
— qwen3.7-max via Qwen Code /review
| /** | ||
| * Switch UI language and optionally LLM output language for a live | ||
| * session, then broadcast a `language_changed` event. When | ||
| * `syncOutputLanguage` is true the handler also refreshes every |
There was a problem hiding this comment.
[Suggestion] The FakeBridge in server.test.ts doesn't implement the newly added setSessionLanguage method on HttpAcpBridge. This is hidden because server.test.ts is in the tsconfig exclude array. Any future test hitting this route will fail at runtime with TypeError: bridge.setSessionLanguage is not a function. Add setSessionLanguageImpl + setSessionLanguageCalls following the setSessionApprovalMode pattern.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const allSessions = [...this.sessions.values()]; | ||
| await Promise.allSettled( |
There was a problem hiding this comment.
[Suggestion] Promise.allSettled fires refreshHierarchicalMemory() + refreshSystemInstruction() for ALL sessions with unbounded concurrency. The bridge wraps this with initTimeoutMs (10s default). With many sessions, this easily exceeds the timeout. When it fires, the bridge returns an error to the HTTP client, but the ACP child continues refreshing sessions in the background — the client sees failure while the change is still being applied. Consider a dedicated timeout, concurrency cap, or fire-and-forget for the refresh phase.
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| const LANGUAGE_CODES = [...SUPPORTED_LANGUAGES.map((l) => l.code), 'auto']; | ||
|
|
There was a problem hiding this comment.
[Suggestion] resolveDaemonTelemetryRoute at line 296 includes load|resume|prompt|cancel|recap|btw|model|shell|detach|approval-mode but not language. Requests to this endpoint will be misclassified in telemetry/logs.
| app.post('/session/:id/language', mutate({ strict: true }), async (req, res) => { |
Also add language to the telemetry regex alternation at line 296.
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| const result = (await Promise.race([ | ||
| withTimeout( |
There was a problem hiding this comment.
[Critical] withTimeout(extMethod(...), initTimeoutMs) wraps the entire ACP handler, which performs heavy I/O: setLanguageAsync + updateOutputLanguageFile (sync write) + Promise.allSettled refreshing all sessions' system prompts. When the 10s timeout fires, the bridge rejects the promise (HTTP error to client), but the ACP handler continues running on the child process — the extMethod RPC has no server-side cancellation.
Client retries then double up: a second setLanguageAsync + updateOutputLanguageFile while the first may still be refreshing sessions, leading to two concurrent Promise.allSettled over all sessions. The client sees failure, but the operation partially or fully succeeded — a nightmare to debug.
Peer methods setSessionModel and setSessionApprovalMode have lighter handlers (no bulk session refresh), so the 10s budget was never a concern for them. This handler is qualitatively heavier.
| withTimeout( | |
| const result = (await Promise.race([ | |
| withTimeout( | |
| entry.connection.extMethod( | |
| SERVE_CONTROL_EXT_METHODS.sessionLanguage, | |
| { | |
| sessionId, | |
| language: params.language, | |
| syncOutputLanguage: params.syncOutputLanguage, | |
| }, | |
| ), | |
| // Language switch + bulk session refresh is heavier than | |
| // model/approval-mode switches — allow more headroom. | |
| initTimeoutMs * 3, | |
| SERVE_CONTROL_EXT_METHODS.sessionLanguage, | |
| ), | |
| getTransportClosedReject(entry), | |
| ])) as { |
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| type: 'language_changed', | ||
| data: { | ||
| sessionId: entry.sessionId, | ||
| language: result.language, |
There was a problem hiding this comment.
[Suggestion] The language_changed event data is { sessionId, language, outputLanguage }, but the return value includes a refreshed field that is absent from the event. SDK subscribers listening on the bus cannot determine whether system prompts were actually refreshed across sessions.
Sibling events like approval_mode_changed include persisted and model_switched includes modelId — all operation-relevant state is present in the event payload.
| language: result.language, | |
| entry.events.publish({ | |
| type: 'language_changed', | |
| data: { | |
| sessionId: entry.sessionId, | |
| language: result.language, | |
| outputLanguage: result.outputLanguage, | |
| refreshed: result.refreshed ?? false, | |
| }, | |
| ...(originatorClientId ? { originatorClientId } : {}), | |
| }); |
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
| outputLanguage = resolved; | ||
| } | ||
|
|
||
| return { language, outputLanguage, refreshed }; |
There was a problem hiding this comment.
[Suggestion] When the caller sends language: "auto", the response echoes { language: "auto", ... }. But setLanguageAsync('auto') internally resolves to a concrete code (e.g., 'zh') via detectSystemLanguage(). The resolved code is never read back — the raw input is returned verbatim.
This creates an asymmetry: outputLanguage IS resolved to a concrete value (e.g., "Chinese"), but language remains the unresolved sentinel. SDK clients and web UI cannot display which concrete language was activated after an auto switch without a separate GET call.
| return { language, outputLanguage, refreshed }; | |
| return { language: getCurrentLanguage(), outputLanguage, refreshed }; |
(import getCurrentLanguage from '../i18n/index.js')
— qwen-latest-series-invite-beta-v38 via Qwen Code /review
- Add sessionOrThrow() call for session existence validation (doudouOUC) - Wrap setLanguageAsync in try-catch with structured error (doudouOUC) - Wrap updateOutputLanguageFile in try-catch to prevent partial state (wenshao) - Return resolved language code instead of echoing "auto" verbatim (wenshao) - Add refreshed field to language_changed SSE event payload (wenshao) - Add language to telemetry route regex (wenshao) - Add FakeBridge setSessionLanguage and 6 server route tests Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Review Response — af375f4Thanks for the thorough reviews. Here's how each finding was handled: Fixed (in commit af375f4)
Fixed earlier (in commit d5e5a9d)
Won't fix (with reasoning)Concurrency serialization queue (doudouOUC) — Language switching is idempotent: the last writer wins and the result is consistent regardless of ordering. This differs fundamentally from approval-mode (non-idempotent state machine transitions where order determines the final state). The existing `/language` slash command has no serialization either. Workspace broadcast for peer sessions (doudouOUC) — The ACP handler already refreshes all sessions' system prompts via `Promise.allSettled`. The SSE event is for UI state updates; in practice, only one frontend client is attached to a session. Adding `broadcastWorkspaceEvent` would send duplicate events to sessions whose prompts are already refreshed. Missing `previousLanguage` in return type (doudouOUC) — The caller already knows the current language (it's what's displayed in their UI). Unlike approval-mode where the previous state matters for audit trails, language switching is a simple preference toggle. `refreshed: true` even when individual sessions failed (doudouOUC/Copilot) — `refreshed` reflects whether the refresh phase was attempted, not whether every individual session succeeded. The core mutation (file write + settings persist) is what matters; individual session refresh failures are transient and self-correcting on next prompt. Unbounded `Promise.allSettled` concurrency (wenshao) — Typical daemon has 1-5 sessions. The `initTimeoutMs` (60s) provides ample headroom. Adding a concurrency cap for a single-digit session count would be over-engineering. `withTimeout` too tight (wenshao) — `initTimeoutMs` defaults to 60s, same as all other extMethods. `setLanguageAsync` loads a small JSON file (~1ms), `writeFileSync` writes a small markdown file (~1ms), and `Promise.allSettled` refreshes 1-5 sessions (~100ms each). Total < 1s in practice. `language_changed` SSE event has no consumer (doudouOUC) — The consumer is in the frontend codebase (agent-web), not in qwen-code. This follows the same pattern as `approval_mode_changed`, `model_switched`, `tool_toggled` — all published here, consumed by frontend SSE subscribers. SDK event type registration (Copilot) — Valid but out of scope for this PR. Can be added when the SDK is updated to consume this event. Route scope: session vs workspace (Copilot) — `setLanguageAsync` is process-level, but the route needs `sessionId` for ACP channel routing and `clientId` authentication. This matches the `model` switching pattern (also process-level state routed through session endpoints). ACP handler language validation (Copilot) — The HTTP route already validates against `LANGUAGE_CODES`. The ACP handler is only reachable through the bridge, which always goes through the route first. Defense-in-depth can be added later. |
Summary
POST /session/:id/languageHTTP endpoint for switching UI language and LLM output language at runtime without polluting session transcriptsetSessionLanguage()→ ACP extMethod handler, following the same pattern asapproval-modeandmodelswitchingsyncOutputLanguage: true, updateoutput-language.md, persist settings, and refresh system prompts across all active sessions so the next LLM call immediately uses the new languageChanges
packages/acp-bridge/src/status.tssessionLanguagetoSERVE_CONTROL_EXT_METHODSpackages/acp-bridge/src/bridgeTypes.tssetSessionLanguage()toHttpAcpBridgeinterfacepackages/acp-bridge/src/bridge.tssetSessionLanguage()bridge methodpackages/cli/src/acp-integration/acpAgent.tssessionLanguageextMethod handlerpackages/cli/src/serve/server.tsPOST /session/:id/languagerouteAPI
Response (200):
{ "language": "zh", "outputLanguage": "Chinese", "refreshed": true }Supported language codes:
zh,zh-TW,en,ja,ru,de,fr,pt,ca,autoTest plan
POST /session/:id/languagewith{"language":"en","syncOutputLanguage":true}→ verify 200 response withoutputLanguage: "English"invalid_languagelanguagefield → 400syncOutputLanguageomitted → 200 withoutputLanguage: null~/.qwen/output-language.mdcontent matches the switched language🤖 Generated with Qwen Code