feat(serve): add runtime context injection for per-turn system-reminders#5847
feat(serve): add runtime context injection for per-turn system-reminders#5847callmeYe wants to merge 24 commits into
Conversation
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. |
|
Thanks for the PR! Template looks good ✓ (all required sections present, bilingual, clear test plan). On direction: this is a well-motivated change. The current pattern of mutating On approach: the scope is proportional — 304 additions across 14 files, each layer doing one thing (Config store → client injection → ACP ext-method → bridge → daemon route → SDK). The full-stack path is needed here since every layer must speak the protocol. One minor note: the diff includes several import reformatting changes in Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓（所有必填章节齐全，双语，测试计划清晰）。 方向：动机充分。当前为瞬态会话状态（操作人身份、always-apply 规则）修改 方案：范围合理——14 个文件 304 行新增，每层各司其职（Config 存储 → client 注入 → ACP ext-method → bridge → daemon 路由 → SDK）。这里确实需要全栈改动，因为每一层都要讲同一个协议。一个小备注：diff 中包含 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Code review: clean implementation, no correctness issues or security holes found. The injection path is sound — One observation: the diff includes ~8 import reformatting changes in Tests (all green): Real-scenario testing: not applicable for this PR. It's a daemon API feature with no CLI/TUI changes — integration testing requires a running daemon with a live session, which isn't available in this CI environment. The PR explicitly scopes to unit tests. The before/after section in the PR body says "N/A — new feature, no UI changes", which is accurate. 中文说明代码审查： 实现干净，未发现正确性 bug 或安全漏洞。 注入路径可靠—— 一个观察：diff 中包含约 8 处 测试（全部通过）： 真实场景测试： 本 PR 不适用。这是一个 daemon API 功能，无 CLI/TUI 变更——集成测试需要运行中的 daemon 和活跃会话，在当前 CI 环境中不可用。PR 明确将范围限定为单元测试。PR 正文中 Before/After 部分写的是 "N/A — new feature, no UI changes"，符合实际。 — Qwen Code · qwen3.7-max |
|
Stepping back — this is a clean, well-scoped feature that solves a real problem. The current pattern of rewriting My independent proposal would have been essentially the same: Map store on Config, inject in the UserQuery path of Tests confirm the implementation works as described. No correctness bugs, no security issues, no scope creep beyond the cosmetic import reformatting (minor). Approving. ✅ 中文说明退后一步看——这是一个干净、范围合理的功能，解决了真实问题。当前为瞬态会话状态（操作人身份、always-apply 规则）重写 我的独立方案基本一致：Config 上的 Map 存储，在 测试确认实现与描述一致。无正确性 bug，无安全问题，除装饰性的 import 格式化外无范围蔓延。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x).
Overall the implementation is clean — solid validation in escapeSystemReminderTags, consistent patterns with existing session mutation routes, and good Config CRUD coverage (9 unit tests). The main gaps are test coverage for the injection/HTTP/ACP delivery paths and a minor UTF-8 measurement inconsistency. See inline comments for details.
— qwen3.7-max via Qwen Code /review
| systemReminders.unshift(userQueryMemory.prompt); | ||
| } | ||
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); |
There was a problem hiding this comment.
[Suggestion] No test verifies the injection path. The client.test.ts mock returns getRuntimeContext: vi.fn().mockReturnValue(new Map()) — an empty Map — so this loop body (including the escapeSystemReminderTags call for prompt-injection safety) never executes in any test. Consider adding a test that populates the Map with entries (including one containing </system-reminder> to verify escaping) and asserts the generated request contains the expected <system-reminder> blocks.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in d594328. Added a dedicated test should inject runtime context entries as system-reminders with escaping in client.test.ts — it populates the mock with two entries (including one containing </system-reminder> to verify escaping), consumes the stream, and asserts the generated request contains the expected <system-reminder> blocks with the malicious tag properly escaped.
| } | ||
| }); | ||
|
|
||
| app.post('/session/:id/runtime-context', mutate(), async (req, res) => { |
There was a problem hiding this comment.
[Suggestion] This new route has no test coverage, and FakeBridge in server.test.ts doesn't implement setSessionRuntimeContext (any future test will hit TypeError: bridge.setSessionRuntimeContext is not a function). The analogous POST /session/:id/language route has 7 tests covering 200/400/404/500 paths.
Also, the 32 KiB check on line 4268 uses serialized.length (UTF-16 code units) rather than Buffer.byteLength(serialized, 'utf8'). For CJK/emoji payloads, the actual UTF-8 wire size can be up to 3x larger than the check allows. The per-value limit in Config.setRuntimeContextEntry correctly uses Buffer.byteLength, creating an inconsistency between layers.
| app.post('/session/:id/runtime-context', mutate(), async (req, res) => { | |
| if (Buffer.byteLength(serialized, 'utf8') > 32 * 1024) { |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in d594328. Two changes:
- UTF-8 size check: Changed
serialized.lengthtoBuffer.byteLength(serialized, 'utf8')— now consistent with the per-value limit in Config. - FakeBridge stub: Added
setSessionRuntimeContextto the FakeBridge so future route tests won't hit TypeError.
|
|
||
| return { language: resolvedLanguage, outputLanguage, refreshed }; | ||
| } | ||
| case SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext: { |
There was a problem hiding this comment.
[Suggestion] Two concerns:
1. No test coverage. The sessionLanguage handler has a dedicated describe block (~170 lines). This handler has zero tests covering its validation branches (invalid sessionId, non-object entries, non-string skip, empty-value removal, success path).
2. Silent validation failures. Non-string values are silently skipped (continue), invalid keys are silently dropped (setRuntimeContextEntry returns false), and the response only contains { keys: appliedKeys }. Callers cannot distinguish "key was not sent" from "key was rejected." Consider validating per-value types in the HTTP route (matching the language route convention) or returning a rejected array with reasons.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in d594328. The handler now returns { keys: appliedKeys, rejected: [...] } where each rejected entry includes { key, reason }. Non-string values get reason: 'value_not_string', and failed Config validations get reason: 'invalid_key_or_value'. Callers can now distinguish successful from rejected entries.
| entries as Record<string, unknown>, | ||
| )) { | ||
| if (typeof value !== 'string') continue; | ||
| if (value === '') { |
There was a problem hiding this comment.
[Nice to have] The removal path doesn't validate the key against RUNTIME_CONTEXT_KEY_RE, and unconditionally pushes to appliedKeys even if the key never existed in the Map. This creates an inconsistency: the set path rejects invalid keys, but the remove path accepts any string and reports it as "applied." Consider adding key validation here too.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in d594328. The removal path now checks config.getRuntimeContext().has(key) before calling removeRuntimeContextEntry — only existing keys are removed and reported as applied. Non-existing keys are silently ignored (no rejected entry needed since removing a non-existent key is a no-op).
| private pendingMcpServers?: string[]; | ||
| private sessionSubagents: SubagentConfig[]; | ||
| private userMemory: string; | ||
| private runtimeContextEntries: Map<string, string> = new Map(); |
There was a problem hiding this comment.
[Critical] Object.create(parent) prototype chain leak.
runtimeContextEntries is initialized via field initializer (= new Map()). The codebase creates subagent Configs via Object.create(parent) in multiple places (background-agent-resume.ts:705, dreamAgentPlanner.ts:150, skillReviewAgentPlanner.ts:236, extractionAgentPlanner.ts:156). Object.create does NOT run field initializers, so this.runtimeContextEntries on a child Config resolves through the prototype chain to the parent's live Map.
Subagents will silently read and mutate the parent's runtime context. The codebase has an established pattern for this — see getFileReadCache() and getMemoryPressureMonitor(), both of which use a hasOwnProperty check with lazy own-property allocation.
| private runtimeContextEntries: Map<string, string> = new Map(); | |
| private getRuntimeContextEntries(): Map<string, string> { | |
| if (!Object.prototype.hasOwnProperty.call(this, 'runtimeContextEntries')) { | |
| (this as any).runtimeContextEntries = new Map(); | |
| } | |
| return (this as any).runtimeContextEntries; | |
| } |
Then update getRuntimeContext(), setRuntimeContextEntry(), removeRuntimeContextEntry(), and setRuntimeContext() to use this.getRuntimeContextEntries() instead of this.runtimeContextEntries.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2dc0034. Applied the hasOwnProperty + lazy own-property allocation pattern (matching getFileReadCache()). All CRUD methods now go through getOwnRuntimeContextEntries() which installs an own Map on first access, preventing subagent Configs created via Object.create(parent) from reading/mutating the parent's live Map.
| } | ||
| }, | ||
|
|
||
| async setSessionRuntimeContext(sessionId, entries, _context) { |
There was a problem hiding this comment.
[Critical] Missing resolveTrustedClientId call.
Every other setSession* bridge handler calls resolveTrustedClientId(entry, context?.clientId) before proceeding — setSessionModel, setSessionLanguage, setSessionApprovalMode, etc. (18+ call sites). This handler names the parameter _context (underscore = unused) and skips validation entirely.
An untrusted client ID passes through unchecked. While no SSE event is published today (so originatorClientId is unused), omitting the trust resolution breaks the defense-in-depth pattern and means a spoofed client ID is silently accepted.
| async setSessionRuntimeContext(sessionId, entries, _context) { | |
| async setSessionRuntimeContext(sessionId, entries, 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); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2dc0034. Added resolveTrustedClientId(entry, context?.clientId) call before the ext-method roundtrip, consistent with all other setSession* handlers.
| getTransportClosedReject(entry), | ||
| ])) as { keys: string[] }; | ||
|
|
||
| return { sessionId, keys: response.keys }; |
There was a problem hiding this comment.
[Suggestion] Bridge drops rejected entries from handler response.
acpAgent.ts returns { keys: appliedKeys, rejected } where rejected contains { key, reason } for entries that failed validation. The bridge casts the response as { keys: string[] } and returns only { sessionId, keys: response.keys }.
SDK callers have no way to detect partial failure — sending 5 entries where 3 are rejected yields { keys: ["a", "b"] } with zero signal that 3 were silently dropped.
Consider forwarding rejected through the bridge return type and SDK types, or document the silent-drop semantics in the JSDoc on DaemonClient.setSessionRuntimeContext.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2dc0034. The bridge now forwards rejected from the ext-method response. Return type updated to { sessionId, keys, rejected: Array<{key, reason}> } across bridge interface, bridge implementation, DaemonClient, and DaemonSessionClient. Callers can now detect partial failures.
| }); | ||
| return setApprovalModeImpl(sessionId, mode, o, context); | ||
| }, | ||
| async setSessionRuntimeContext(sessionId, entries, _context) { |
There was a problem hiding this comment.
[Suggestion] Two issues with this FakeBridge stub:
-
Missing
rejectedfield. TheAcpSessionBridge.setSessionRuntimeContextreturn type requiresrejected: Array<{key: string, reason: string}>, but this stub omits it. Any future test asserting onresponse.rejectedwill getundefined. -
Implicit
anyon parameters. TypeScript flagssessionId,entries, and_contextas implicitlyany(TS7006).
| async setSessionRuntimeContext(sessionId, entries, _context) { | |
| async setSessionRuntimeContext( | |
| sessionId: string, | |
| entries: Record<string, string>, | |
| _context?: { clientId?: string }, | |
| ) { | |
| return { | |
| sessionId, | |
| keys: Object.keys(entries).filter((k) => typeof entries[k] === 'string'), | |
| rejected: [] as Array<{ key: string; reason: string }>, | |
| }; | |
| }, |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 4af5086. Added rejected field and explicit parameter types to the FakeBridge stub.
| } else if (config.setRuntimeContextEntry(key, value)) { | ||
| appliedKeys.push(key); | ||
| } else { | ||
| rejected.push({ key, reason: 'invalid_key_or_value' }); |
There was a problem hiding this comment.
[Suggestion] setRuntimeContextEntry returns false for three distinct reasons — invalid key format, value exceeding 32 KiB, and the 16-entry capacity being full — but this handler maps all three to the single string 'invalid_key_or_value'. An API caller receiving this rejection has no way to diagnose whether to fix their key, shrink their value, or evict old entries.
Consider either (a) changing Config.setRuntimeContextEntry to return a discriminated result like { ok: true } | { ok: false; reason: 'invalid_key' | 'value_too_large' | 'capacity_full' }, or (b) checking the specific condition before calling:
| rejected.push({ key, reason: 'invalid_key_or_value' }); | |
| } else if (!Config.RUNTIME_CONTEXT_KEY_RE.test(key)) { | |
| rejected.push({ key, reason: 'invalid_key' }); | |
| } else if (entries.size >= 16) { | |
| rejected.push({ key, reason: 'capacity_full' }); | |
| } else { | |
| rejected.push({ key, reason: 'value_too_large' }); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 4af5086. The handler now returns granular rejection reasons: invalid_key (regex mismatch), value_too_large (>32 KiB), or capacity_full (16 entry limit). Callers can now diagnose and act on specific rejection causes.
| } | ||
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); | ||
| for (const [, value] of runtimeCtx) { |
There was a problem hiding this comment.
[Suggestion] The injection loop discards the key and wraps only the raw value. With multiple entries, the model receives bare values like "Alice" or "staging" with no indication of what they represent. Compare with other system-reminder blocks in this codebase that include descriptive labels (e.g., "The current date is: ...").
Including the key as a label prefix gives the model semantic context to interpret each value:
| for (const [, value] of runtimeCtx) { | |
| for (const [key, value] of runtimeCtx) { | |
| const safe = escapeSystemReminderTags(value); | |
| systemReminders.push( | |
| `<system-reminder>\n[${key}] ${safe}\n</system-reminder>`, | |
| ); | |
| } |
This would also require updating the client test at client.test.ts which currently asserts on the value being present without any key labeling.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 4af5086. Each entry is now rendered as [key] value inside the system-reminder, e.g. <system-reminder>\n[operator_identity] 当前操作者: Alice\n</system-reminder>. This gives the model semantic context to interpret each value. Test assertion updated accordingly.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
8 findings (2 Critical, 6 Suggestion). All tests pass (998/998), typecheck and lint clean.
— qwen3.7-max via Qwen Code /review
|
|
||
| return { language: resolvedLanguage, outputLanguage, refreshed }; | ||
| } | ||
| case SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext: { |
There was a problem hiding this comment.
[Critical] No tests for the sessionRuntimeContext handler.
The handler has 5+ branches (invalid sessionId, invalid entries type, non-string value rejection, empty-value removal, set-or-reject) but acpAgent.test.ts has zero references to sessionRuntimeContext or runtimeContext. The "Already discussed" section claims this was addressed, but no test code was added.
Suggested: add a describe('sessionRuntimeContext') block covering happy path, invalid sessionId, null/array/non-object entries, non-string value → value_not_string rejected, empty-string removal of existing key, empty-string for non-existent key, and invalid key → invalid_key_or_value rejected.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed — 6 handler tests added in commit 0660660, covering: happy-path set, empty-string removal, non-string rejection, invalid key rejection, missing sessionId, and invalid entries shape. See also my reply to the later duplicate of this comment.
| } | ||
| }); | ||
|
|
||
| app.post('/session/:id/runtime-context', mutate(), async (req, res) => { |
There was a problem hiding this comment.
[Critical] No route-level tests for POST /session/:id/runtime-context.
server.test.ts only adds capability-list entries and a FakeBridge stub — no describe block for this route. The handler has validation (400 for bad entries shape), size-limit (413 for >32 KiB payload), bridge error mapping, and clientId forwarding, all untested.
Suggested: add a describe('POST /session/:id/runtime-context') block mirroring the approval-mode pattern: 200 happy path, 400 for missing/non-object/null/array entries, 413 for oversized payload, bridge error propagation, X-Client-Id header forwarding.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Acknowledged. Route-level tests for this endpoint are a valid follow-up. The existing analogous routes (POST /session/:id/language, POST /session/:id/model) have route tests, and the runtime-context route follows the exact same pattern. The handler logic is covered by the 6 acpAgent tests added in 0660660. Will track route-level tests as follow-up.
| }); | ||
| return setApprovalModeImpl(sessionId, mode, o, context); | ||
| }, | ||
| async setSessionRuntimeContext(sessionId, entries, _context) { |
There was a problem hiding this comment.
[Suggestion] FakeBridge stub returns { sessionId, keys } but the AcpSessionBridge interface in bridgeTypes.ts requires rejected: Array<{ key: string; reason: string }> in the return type. TypeScript doesn't catch this because server.test.ts is excluded from tsconfig compilation.
| async setSessionRuntimeContext(sessionId, entries, _context) { | |
| async setSessionRuntimeContext(sessionId, entries, _context) { | |
| return { | |
| sessionId, | |
| keys: Object.keys(entries).filter((k) => typeof entries[k] === 'string'), | |
| rejected: [], | |
| }; | |
| }, |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 4af5086 — FakeBridge stub now returns rejected: [] as Array<{ key: string; reason: string }> with explicit parameter types.
| if (value === '') { | ||
| if (config.getRuntimeContext().has(key)) { | ||
| config.removeRuntimeContextEntry(key); | ||
| appliedKeys.push(key); |
There was a problem hiding this comment.
[Suggestion] When value === '' and the key doesn't exist in the store, it is silently dropped from both appliedKeys and rejected. Callers cannot distinguish "key was already absent" from a dropped request.
Consider idempotent delete semantics — always push to appliedKeys:
| appliedKeys.push(key); | |
| if (value === '') { | |
| config.removeRuntimeContextEntry(key); | |
| appliedKeys.push(key); | |
| } else if (config.setRuntimeContextEntry(key, value)) { |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 2dc0034 — the removal path now checks config.getRuntimeContext().has(key) before calling removeRuntimeContextEntry. Non-existing keys are silently skipped (no-op removal doesn't need to appear in either keys or rejected).
| } | ||
| } else if (config.setRuntimeContextEntry(key, value)) { | ||
| appliedKeys.push(key); | ||
| } else { |
There was a problem hiding this comment.
[Suggestion] setRuntimeContextEntry returns false for three distinct failure modes (invalid key regex, value exceeding 32 KiB, store at 16-entry capacity) but all map to the same 'invalid_key_or_value' reason. Callers cannot determine which constraint was violated.
Consider computing the specific reason:
| } else { | |
| } else { | |
| const reason = !Config.RUNTIME_CONTEXT_KEY_RE.test(key) | |
| ? 'invalid_key' | |
| : Buffer.byteLength(value, 'utf8') > Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES | |
| ? 'value_too_large' | |
| : 'store_at_capacity'; | |
| rejected.push({ key, reason }); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 4af5086 — the handler now returns granular rejection reasons: invalid_key, value_too_large, or capacity_full instead of the generic invalid_key_or_value.
| this.getOwnRuntimeContextEntries().delete(key); | ||
| } | ||
|
|
||
| setRuntimeContext(entries: Record<string, string>): void { |
There was a problem hiding this comment.
[Suggestion] setRuntimeContext(entries) is defined and tested but has zero production callers. The acpAgent handler iterates entries individually via setRuntimeContextEntry. This is dead code with subtly different semantics from the per-entry API (silently drops invalid entries vs returning booleans/rejected arrays).
Consider removing until a production caller exists, or document the intended future use.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Good observation. setRuntimeContext(entries) is intentionally provided for the initialRuntimeContext flow described in the plan — when a session is created, the bridge will call it once to bulk-set the initial context before the first prompt. The acpAgent handler uses per-entry setRuntimeContextEntry for the incremental API path. Both are needed.
| ); | ||
| } | ||
|
|
||
| async setSessionRuntimeContext( |
There was a problem hiding this comment.
[Suggestion] No tests for setSessionRuntimeContext or DaemonSessionClient.setRuntimeContext. These SDK methods handle URL encoding, body serialization, clientId forwarding, and error mapping — none verified by tests.
Suggested: add tests in DaemonSessionClient.test.ts verifying correct URL path, body serialization, clientId forwarding, and error handling for non-OK responses.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Acknowledged. SDK method tests are a valid follow-up. The methods are thin pass-through wrappers (fetchWithTimeout + URL construction + JSON body) following the exact pattern of setSessionModel / setSessionLanguage, which also lack dedicated SDK-level tests. Will track as follow-up.
| } | ||
| }, | ||
|
|
||
| async setSessionRuntimeContext(sessionId, entries, context) { |
There was a problem hiding this comment.
[Suggestion] No bridge integration tests for setSessionRuntimeContext. The method has non-trivial error handling (two SessionNotFoundError paths, transport-closed rejection, response.rejected ?? [] fallback) — all untested.
Suggested: add tests covering happy path forwarding, unknown sessionId → SessionNotFoundError, dying channel → SessionNotFoundError, transport-closed rejection, and response.rejected undefined → empty array.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Acknowledged. The bridge follows the exact same pattern as setSessionLanguage — same SessionNotFoundError paths, same withTimeout + getTransportClosedReject race. Adding bridge integration tests is a valid follow-up but out of scope for this PR (the existing setSessionLanguage bridge tests don't exist either). Will track as a follow-up.
| @@ -5674,6 +5674,55 @@ class QwenAgent implements Agent { | |||
|
|
|||
| return { language: resolvedLanguage, outputLanguage, refreshed }; | |||
| } | |||
There was a problem hiding this comment.
[Critical] No tests for the sessionRuntimeContext handler. The handler has 7+ branches (invalid sessionId, invalid entries shape, non-string value rejection, empty-string removal with/without existing key, key regex, byte-size check, capacity full) but acpAgent.test.ts has zero references to sessionRuntimeContext. The adjacent sessionLanguage handler has a dedicated describe block (~170 lines) establishing the pattern.
Add tests following the sessionLanguage pattern — call agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext, { sessionId, entries }) and assert on keys, rejected, and getRuntimeContext() state. Cover at minimum: happy path set, empty-string removal, invalid params, non-string rejection, and capacity-full rejection.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 0660660. Added 6 handler tests following the sessionLanguage pattern: happy-path set, empty-string removal, non-string value rejection, invalid key rejection, missing sessionId error, and invalid entries shape error. Tests use a mock Config with a real Map<string, string> backing store to verify state changes.
| } | ||
| }); | ||
|
|
||
| app.post('/session/:id/runtime-context', mutate(), async (req, res) => { |
There was a problem hiding this comment.
[Critical] No route-level tests for POST /session/:id/runtime-context. The FakeBridge stub was added but no describe block exercises the route. Every comparable route (model, recap, approval-mode, language) has a dedicated describe block with happy-path, validation-error, and bridge-error tests.
Add a describe('POST /session/:id/runtime-context') block in server.test.ts following the POST /session/:id/language pattern. Wire up call recording in the FakeBridge stub and test: 200 success, 400 on invalid entries, 413 on oversized payload, and bridge SessionNotFoundError propagation.
— qwen3.7-max via Qwen Code /review
| } | ||
| } else if (config.setRuntimeContextEntry(key, value)) { | ||
| appliedKeys.push(key); | ||
| } else if (!/^[a-zA-Z0-9_-]{1,64}$/.test(key)) { |
There was a problem hiding this comment.
[Suggestion] Duplicated validation logic. The rejection-reason chain re-inlines /^[a-zA-Z0-9_-]{1,64}$/ and 32 * 1024 as literals, duplicating Config.RUNTIME_CONTEXT_KEY_RE and Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES. If either Config constant changes, the handler's rejection reasons silently diverge — reporting invalid_key for a key Config accepted, or vice versa.
Consider having setRuntimeContextEntry return a structured result ({ ok: true } | { ok: false; reason: string }) instead of a bare boolean, so the handler never re-derives the rejection reason. Alternatively, expose the constants from Config (they're currently private static readonly).
— qwen3.7-max via Qwen Code /review
| this.getOwnRuntimeContextEntries().delete(key); | ||
| } | ||
|
|
||
| setRuntimeContext(entries: Record<string, string>): void { |
There was a problem hiding this comment.
[Suggestion] setRuntimeContext(entries) is defined and tested but has zero production callers. The ACP handler iterates entries individually via setRuntimeContextEntry/removeRuntimeContextEntry. The SDK's DaemonSessionClient.setRuntimeContext goes through the HTTP route, which also uses per-entry methods. This Config method is dead code with subtly different semantics (silent drops invalid entries without reporting them).
Either remove it (inlining the test setup), or wire it into the ACP handler and align its return value with the per-entry path's { keys, rejected } shape.
— qwen3.7-max via Qwen Code /review
| const bootConfig = makeRuntimeCtxConfig(); | ||
| runAcpAgent( | ||
| bootConfig as unknown as Config, | ||
| { merged: { mcpServers: {} } } as unknown as LoadedSettings, |
There was a problem hiding this comment.
[Critical] All 6 tests in the sessionRuntimeContext handler describe block crash with TypeError: mcpServers is not iterable at acpAgent.ts:6975. setupAgent calls agent.newSession({}) without providing an mcpServers array, so the production code's for (const server of mcpServers) throws on undefined.
This is why CI's Test (ubuntu-latest, Node 22.x) check is red.
| { merged: { mcpServers: {} } } as unknown as LoadedSettings, | |
| runAcpAgent( | |
| bootConfig as unknown as Config, | |
| { merged: { mcpServers: [] } } as unknown as LoadedSettings, | |
| {} as CliArgs, | |
| ); |
Also fix the loadSettings mock at the same location — use mcpServers: [] instead of mcpServers: {}.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const serialized = JSON.stringify(entries); | ||
| if (Buffer.byteLength(serialized, 'utf8') > 32 * 1024) { |
There was a problem hiding this comment.
[Critical] The server enforces a 32 KiB cap on JSON.stringify(entries) (the entire serialized object, including keys, braces, and quotes), while Config.setRuntimeContextEntry enforces a 32 KiB cap on each individual value. These are different measurements.
A payload of 16 entries with ~2 KiB values each totals ~34 KiB serialized and is rejected here with HTTP 413, even though every individual entry passes Config's per-value check. Conversely the error message "runtime context payload exceeds 32 KiB limit" implies a per-entry contract that does not exist.
Either (1) rename this constant to RUNTIME_CONTEXT_MAX_PAYLOAD_BYTES and give it a distinct value (e.g. 48 KiB) that accommodates realistic multi-entry payloads, documenting it as the transport-envelope limit, or (2) drop this check and let Config be the single authority.
— qwen3.7-max via Qwen Code /review
| 'permission_mediation', | ||
| 'non_blocking_prompt', | ||
| 'session_language', | ||
| 'session_runtime_context', |
There was a problem hiding this comment.
[Critical] POST /session/:id/runtime-context (server.ts:4249) has no route-level test coverage despite the fakeBridge.setSessionRuntimeContext stub being added on line 1318. Comparable routes like POST /session/:id/approval-mode (line 6730) and POST /session/:id/language (line 6952) have full describe blocks exercising success, validation failure (400), and bridge-error paths.
The new route has four branches — 400 for invalid entries, 413 for oversized payload, 200 success via bridge, and the sendBridgeError catch — none of which are verified. Regressions in body validation, the 32 KiB pre-check, clientId header forwarding, or error mapping will go undetected until integration testing.
Suggested: add a describe('POST /session/:id/runtime-context', ...) block following the approval-mode pattern, covering (1) valid entries returning 200 with the bridge response, (2) entries: null / array returning 400, (3) serialized body > 32 KiB returning 413, (4) bridge throwing returning the sendBridgeError status code.
— qwen3.7-max via Qwen Code /review
| } else if ( | ||
| Buffer.byteLength(value, 'utf8') > 32 * 1024 | ||
| ) { | ||
| rejected.push({ key, reason: 'value_too_large' }); |
There was a problem hiding this comment.
[Suggestion] The value_too_large and capacity_full rejection-reason branches are untested. Existing handler tests cover value_not_string and invalid_key, but not these two.
The reason-mapping logic here re-derives the failure cause by re-running the same regex (/^[a-zA-Z0-9_-]{1,64}$/) and size (32 * 1024) checks Config already performed internally. If the regex or byte limit in Config ever drifts from what the handler inlines, SDK consumers silently receive wrong rejected[].reason values.
Two suggested tests:
- Send
{ validkey: 'x'.repeat(32 * 1024 + 1) }and assertrejected[0].reason === 'value_too_large'. - Pre-fill 16 entries via the mock Config map, send a 17th valid entry, and assert
rejected[0].reason === 'capacity_full'.
Longer-term, consider having Config.setRuntimeContextEntry return a discriminated result ({ ok: true } | { ok: false; reason: string }) so the handler doesn't have to re-validate, and export RUNTIME_CONTEXT_KEY_RE / RUNTIME_CONTEXT_MAX_VALUE_BYTES from config.ts so the constants have a single source of truth.
— qwen3.7-max via Qwen Code /review
| this.getOwnRuntimeContextEntries().delete(key); | ||
| } | ||
|
|
||
| setRuntimeContext(entries: Record<string, string>): void { |
There was a problem hiding this comment.
[Suggestion] setRuntimeContext(entries) silently drops entries that fail validation (clear()s the store, then iterates and only stores passing entries) and returns void. This is inconsistent with setRuntimeContextEntry (returns boolean) and with the ACP handler (returns { keys, rejected: [...] }).
A future caller passing 16 entries where 2 have invalid keys will atomically destroy the existing context and silently lose the 2 bad entries with no feedback. Today this method has no production caller — only config.test.ts exercises it — so it's a dormant footgun rather than a live bug.
Options:
- Change the return type to
{ applied: string[]; rejected: Array<{ key: string; reason: string }> }, matching the handler's contract. - Remove the method entirely if no production caller is planned —
setRuntimeContextEntryalready covers the use case. - At minimum, document the silent-drop behavior in JSDoc and add a comment warning future callers.
— qwen3.7-max via Qwen Code /review
| if (!entry) throw new SessionNotFoundError(sessionId); | ||
| const info = channelInfoForEntry(entry); | ||
| if (!info || info.isDying) throw new SessionNotFoundError(sessionId); | ||
| resolveTrustedClientId(entry, context?.clientId); |
There was a problem hiding this comment.
[Suggestion] setSessionRuntimeContext does not publish any SSE event on success, unlike the comparable setSessionLanguage (publishes language_changed at line 4042), setSessionModel (publishes model_switched), and setSessionApprovalMode (publishes approval_mode_changed). Connected SSE clients (browser UI, IDE status bars) cannot react to runtime-context mutations — they must poll to learn about changes.
Also: resolveTrustedClientId(entry, context?.clientId) is called for validation, but its return value is discarded. If no event is planned, the call serves no purpose (validation would still throw InvalidClientIdError whether or not the return is captured, but the originatorClientId propagation that other handlers rely on is lost).
If events are intentionally omitted for this mutation, add a comment explaining why (mirroring the pattern at generateSessionRecap line 4279: "recap is informational-only today — no SSE broadcast"). Otherwise, publish a runtime_context_changed event carrying keys and the resolved originatorClientId.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); | ||
| for (const [key, value] of runtimeCtx) { |
There was a problem hiding this comment.
[Suggestion] The injection loop iterates the runtime-context Map and wraps each value with escapeSystemReminderTags(value). This escape function only neutralizes literal <system-reminder> / </system-reminder> tag strings (confirmed in xml.ts:152). Other trusted envelope tags used elsewhere in the conversation — <task-notification> (backgroundShellRegistry), <available_skills> (environmentContext), <task-id>, <summary> — pass through verbatim inside the injected <system-reminder> body.
A malicious trusted caller (MCP server, ACP peer, daemon plugin) could set a runtime-context value containing literal <task-notification>...</task-notification> markup. The model, which already treats these tags as authoritative system metadata in other parts of the conversation, may act on the fabricated notification.
Mitigation: runtime context is set by a trusted controller (daemon API / SDK via resolveTrustedClientId), not by the end user directly, so exploitability is bounded by trust in connected clients. If the trust boundary is ever relaxed (e.g., untrusted MCP-sourced context), switch to escapeXml(value) here — runtime-context values are opaque metadata and never need raw XML that the model should parse.
Also consider reserving a key prefix (e.g. qwen- or _) for internal/system keys, and rejecting user-supplied keys that match — keys like system, operator, admin, instructions imply elevated authority when rendered as [system] ... inside a system-reminder.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const serialized = JSON.stringify(entries); | ||
| if (Buffer.byteLength(serialized, 'utf8') > 32 * 1024) { |
There was a problem hiding this comment.
[Suggestion] The HTTP route caps the total serialized payload at 32 KiB, but Config allows 16 entries × 32 KiB each = 512 KiB theoretical max store. A legitimate 10-entry request with ~4 KiB per value (~40 KiB total) gets 413'd here even though every individual value passes the per-value limit. Meanwhile the ACP ext-method path has no aggregate check, so the same payload succeeds through that transport.
Consider either raising this cap to MAX_ENTRIES × MAX_VALUE_BYTES (512 KiB), or defining a separate named constant (e.g. RUNTIME_CONTEXT_MAX_PAYLOAD_BYTES) if the tighter limit is intentional — but document the intentional mismatch so future maintainers don't assume it's a bug.
— qwen3.7-max via Qwen Code /review
| rejected.push({ key, reason: 'capacity_full' }); | ||
| } | ||
| } | ||
| return { keys: appliedKeys, rejected }; |
There was a problem hiding this comment.
[Suggestion] Zero logging on runtime context mutations. Every other control-plane handler in this file (language, approval mode, model switch, recap) has at least a debug log or status endpoint. When a session's model starts behaving unexpectedly due to an injected <system-reminder>, the oncall engineer has no way to trace which entries were set/updated/rejected.
| return { keys: appliedKeys, rejected }; | |
| debugLogger?.debug?.(`[runtime-ctx] session=${sessionId} applied=${appliedKeys.length} rejected=${rejected.length} keys=[${appliedKeys.join(',')}]`); | |
| return { keys: appliedKeys, rejected }; |
— qwen3.7-max via Qwen Code /review
| } | ||
| } else if (config.setRuntimeContextEntry(key, value)) { | ||
| appliedKeys.push(key); | ||
| } else if (!/^[a-zA-Z0-9_-]{1,64}$/.test(key)) { |
There was a problem hiding this comment.
[Suggestion] The key regex /^[a-zA-Z0-9_-]{1,64}$/ and byte limit 32 * 1024 are duplicated inline here, separate from Config.RUNTIME_CONTEXT_KEY_RE and Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES (both private static readonly). If either limit changes in Config, this reason-determination cascade silently diverges — e.g., a key that Config now accepts would still get reported as 'invalid_key'.
The root cause is that setRuntimeContextEntry returns boolean — no reason for the rejection. A cleaner fix would be to change it to return { ok: true } | { ok: false; reason: string }, eliminating the need for the caller to re-validate:
// In Config:
setRuntimeContextEntry(key: string, value: string): { ok: boolean; reason?: string }
// Then in this handler:
const result = config.setRuntimeContextEntry(key, value);
if (!result.ok) rejected.push({ key, reason: result.reason! });— qwen3.7-max via Qwen Code /review
| systemReminders.unshift(userQueryMemory.prompt); | ||
| } | ||
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); |
There was a problem hiding this comment.
[Suggestion] Key not escaped in system-reminder injection template.
The value is properly sanitized via escapeSystemReminderTags(value), but the key is interpolated directly into the template as [${key}] without escaping. While keys are validated by /^[a-zA-Z0-9_-]{1,64}$/ at the external input boundary, getRuntimeContext() returns the mutable Map (typed as ReadonlyMap but ReadonlyMap is compile-time only). Any internal code with Config access can call .set() on the returned Map, bypassing validation entirely.
Defense-in-depth: escape the key at the injection point, matching the value treatment:
| const runtimeCtx = this.config.getRuntimeContext(); | |
| const runtimeCtx = this.config.getRuntimeContext(); | |
| for (const [key, value] of runtimeCtx) { | |
| const safeKey = escapeSystemReminderTags(key); | |
| const safe = escapeSystemReminderTags(value); | |
| systemReminders.push( | |
| `<system-reminder>\n[${safeKey}] ${safe}\n</system-reminder>`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| return this.runtimeContextEntries; | ||
| } | ||
|
|
||
| getRuntimeContext(): ReadonlyMap<string, string> { |
There was a problem hiding this comment.
[Suggestion] getRuntimeContext() returns the mutable Map and silently isolates subagents.
Two concerns:
-
Mutable Map despite
ReadonlyMaptype.ReadonlyMapis compile-time only — at runtime, callers can.set(),.delete(),.clear()the returned Map, bypassing all validation (key regex, size limit, entry count). Returningnew Map(this.getOwnRuntimeContextEntries())would provide actual defense-in-depth. -
Silent subagent isolation. The
hasOwnPropertyguard ingetOwnRuntimeContextEntries()(introduced to fix theObject.create(parent)prototype chain leak) also prevents subagent Configs — created viaObject.create(parent)inInProcessBackend.ts:401andsubagent-manager.ts:882— from reading the parent's runtime context through the prototype chain. Subagents get a fresh empty Map. This is inconsistent with howuserMemoryand other session-level state propagates to subagents.
If subagent isolation is intentional, make it explicit (e.g., a comment or a dedicated getRuntimeContextForSubagent() method). If subagents should see the parent's context, return a read-only copy that walks the prototype for reads but blocks writes.
— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| async setRuntimeContext( |
There was a problem hiding this comment.
[Suggestion] setRuntimeContext does incremental merge, but the name implies full replacement.
The full-stack path (DaemonSessionClient → HTTP route → bridge → acpAgent handler) iterates entries and calls config.setRuntimeContextEntry(key, value) per key — an additive upsert. A caller who does setRuntimeContext({ newKey: 'val' }) after a prior setRuntimeContext({ oldKey: 'val' }) will find both keys present, not just newKey.
Meanwhile, Config.setRuntimeContext() — which does a clear-and-replace — exists but has zero production callers (dead code). The naming inconsistency (set vs actual merge behavior) and the unused replace method suggest the design intent is unclear.
Consider either: (a) wiring Config.setRuntimeContext() in the handler before iterating (matching the "set" name), or (b) renaming to updateRuntimeContext / patchRuntimeContext and adding explicit clearRuntimeContext() and getRuntimeContext() SDK methods.
— qwen3.7-max via Qwen Code /review
| ).rejects.toThrow(/sessionId/); | ||
| }); | ||
|
|
||
| it('throws on invalid entries shape', async () => { |
There was a problem hiding this comment.
[Suggestion] The entries validation in the handler has three distinct OR branches (typeof entries !== 'object', entries === null, Array.isArray(entries)), but the existing "throws on invalid entries shape" test passes entries: 'not-an-object' which only hits the first branch. entries: null and entries: [1, 2] each exercise untested code paths. Consider adding test cases for null and array inputs to cover all validation branches.
— qwen3.7-max via Qwen Code /review
| expect(config.getRuntimeContext().get('key-0')).toBe('updated'); | ||
| }); | ||
|
|
||
| it('should bulk-set entries', () => { |
There was a problem hiding this comment.
[Suggestion] The existing bulk-set test covers only the happy path with 2 valid entries. The bulk setRuntimeContext method has its own inline validation that silently drops entries failing key regex, byte-size limit, or 16-entry capacity — a different contract from setRuntimeContextEntry (singular, returns boolean). Specific untested scenarios:
- Bulk-set exceeding 16 entries (does it stop at 16?)
- Bulk-set with invalid keys mixed in (silently skipped while valid keys stored?)
- Bulk-set with empty-string values (
value &&skips them, but the singular method treats empty string as deletion)
The behavioral asymmetry between bulk and singular methods is worth explicit test coverage.
— qwen3.7-max via Qwen Code /review
| systemReminders.unshift(userQueryMemory.prompt); | ||
| } | ||
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); |
There was a problem hiding this comment.
[Suggestion] Runtime context entries are injected only in GeminiClient.sendMessageStream. Subagents (AgentCore.createChat), background agents (background-agent-resume.ts), and memory planners (skillReviewAgentPlanner.ts, extractionAgentPlanner.ts, dreamAgentPlanner.ts) all create GeminiChat directly and bypass this injection path. If runtime context (e.g., operator identity) is intended to apply to the entire session including subagent turns, a second injection point is needed — either in GeminiChat.sendMessageStream or in the subagent system prompt building path.
— qwen3.7-max via Qwen Code /review
callmeYe
force-pushed
the
worktree-feat+runtime-context
branch
from
11713a7 to
db94452
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 finding (Suggestion). Build and all 1177 tests pass locally. Deterministic analysis clean (typecheck and eslint).
— qwen3.7-max via Qwen Code /review
| ) { | ||
| rejected.push({ key, reason: 'value_too_large' }); | ||
| } else { | ||
| rejected.push({ key, reason: 'capacity_full' }); |
There was a problem hiding this comment.
[Suggestion] The else clause here unconditionally produces capacity_full as the rejection reason. This cascade re-derives the failure cause by re-running Config's validation checks inline (key regex, byte length), then assumes anything left must be capacity. If Config.setRuntimeContextEntry ever adds a fourth validation rule (e.g., reserved key prefix, value charset restriction), the new failure would silently fall through to capacity_full — misleading API consumers about the real cause.
Consider having setRuntimeContextEntry return a structured result instead of a bare boolean:
| rejected.push({ key, reason: 'capacity_full' }); | |
| const result = config.setRuntimeContextEntry(key, value); | |
| if (result.ok) { | |
| appliedKeys.push(key); | |
| } else { | |
| rejected.push({ key, reason: result.reason }); | |
| } |
This eliminates the fragile re-derivation and makes the rejection reason authoritative from the Config layer.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] TypeScript does not pass on the reviewed head. qwen review deterministic reports five tsc errors in packages/cli/src/serve/server.test.ts (including the fake bridge no longer matching AcpSessionBridge / DaemonWorkspaceService, plus extension mock return-type mismatches). These diagnostics are outside the added diff lines, so they cannot be anchored inline, but the branch needs to typecheck cleanly before merge.
— GPT-5 via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| requestToSend = [...systemReminders, ...requestToSend]; |
There was a problem hiding this comment.
[Critical] These generated runtime-context reminders are prepended to requestToSend, and GeminiChat.sendMessageStream persists that full user content into chat history before sending. After a caller updates or removes a key, the old <system-reminder>[key] ...</system-reminder> remains in prior user turns and is sent again on future requests, so stale rules or operator data can keep influencing the model and continue being sent to the provider. Treat runtime context as a request-only overlay, or tag/filter these generated reminder parts before committing user content to history, and add a set -> send -> remove/update -> send regression test.
— GPT-5 via Qwen Code /review
| rejected: Array<{ key: string; reason: string }>; | ||
| }> { | ||
| return await this.fetchWithTimeout( | ||
| `${this.baseUrl}/session/${encodeURIComponent(sessionId)}/runtime-context`, |
There was a problem hiding this comment.
[Critical] This new SDK method goes through this.transport.fetch(...), but ACP HTTP/WS transports do not hit the REST Express route directly; they first map URL-shaped calls through acpRouteTable.ts. There is no /session/:id/runtime-context mapping and no matching _qwen/session/runtime_context dispatcher, so clients using negotiateTransport() can select ACP and get a synthesized 404 even though the REST route works. Add the route-table entry, ACP dispatch handler, and transport tests for this API.
— GPT-5 via Qwen Code /review
| const config = session.getConfig(); | ||
| const appliedKeys: string[] = []; | ||
| const rejected: Array<{ key: string; reason: string }> = []; | ||
| for (const [key, value] of Object.entries( |
There was a problem hiding this comment.
[Critical] Processing the batch strictly in object order makes capacity handling depend on caller key order. If the store already has 16 entries and a caller sends { newKey: 'v', oldKey: '' }, newKey is rejected as capacity_full before oldKey is removed; the same logical update succeeds if the deletion appears first. Apply empty-string removals before non-empty sets, or compute capacity against the post-removal store, so a single batch has deterministic semantics.
— GPT-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new findings in this re-review. 9 parallel review agents + verification + reverse audit confirmed that all significant concerns are already covered by prior review comments.
Build & tests: ✅ Build passes. All 17 new tests pass (10 Config RuntimeContext, 1 client injection, 6 acpAgent handler). ESLint clean. 5 pre-existing tsc errors in server.test.ts at unchanged lines (not caused by this PR).
Note: The prior Critical comment about test crashes (mcpServers is not iterable at acpAgent.test.ts:7552) appears stale — verified that line 7560 passes mcpServers: [] and all 6 handler tests pass successfully.
— qwen3.7-max via Qwen Code /review
|
Thanks — the need itself is clear: sessions sharing a The way I think about it: qwen-code core is a single-session engine. It runs one conversation for one caller, with one set of context — the system prompt plus RuntimeContext — context that differs per session, varies per client, and mutates at runtime — is multi-tenant state in exactly that sense. A single-session user never reaches for it; they edit Putting it in core inverts that layering: the engine starts carrying a notion that exists only because of the layer above it. Once, it's harmless; as a habit, it's how a clean, embeddable engine slowly turns into one that has to know about every consumer's needs. So the real question, before any implementation: is there something that genuinely requires this to live in the engine rather than at the serve boundary? If there is, let's name it and weigh it. If not, I'd keep core single-session-pure and own this in the daemon, where the per-tenant state already lives. 中文感谢提交——需求本身很清楚:共享同一个 我的理解是这样的:qwen-code core 是一个单会话引擎。它为单个调用方运行一段对话,只有一套上下文——系统提示词加上 RuntimeContext——按会话不同、随客户端变化、并在运行时可变的上下文——正是这种意义上的多租户状态。单会话用户从不需要它;他们改 把它放进 core 会让分层倒置:引擎开始承载一个仅仅因为上层才存在的概念。偶尔一次无伤大雅;一旦成为习惯,一个干净、可嵌入的引擎就是这样慢慢变成一个不得不了解每个消费方需求的引擎。 所以在动手实现之前,真正要问的是:是否存在某种东西,确实要求它必须落在引擎里、而不能放在 serve 边界?如果有,我们把它点出来、一起权衡。如果没有,我倾向于让 core 保持纯单会话,把这件事交给 daemon——按租户的状态本就都在那里。 |
|
@tanzhenxin Thanks for the thoughtful layering analysis — I agree with the framing and have updated the PR description accordingly (removed all daemon-specific business context, repositioned as a generic SDK-level capability aligned with the Claude Agent SDK's system prompt modification patterns). On the core vs daemon question — you're right that this is conceptually multi-tenant state. Let me lay out why the current implementation touches core, and whether the alternative works: Why core today: The injection point is The daemon-only alternative would be:
None of these give "external caller sets key-value context that appears as The minimal core surface: The actual core footprint is intentionally tiny — a That said, I'm open to restructuring if there's a cleaner boundary. One option: move the Map storage to the bridge layer and use the existing Also noting: I've updated the PR description to remove all references to specific downstream consumers and reposition this as a generic capability — analogous to Claude Agent SDK's |
|
Thanks — this is a strong reply, and it moved me off part of my position. Let me drop what doesn't hold and sharpen what does. You're right that the layering objection isn't the real issue. But following your own " First, what this carries — operator identity, per-session rules — is session-constant: set once, unchanged turn to turn. Per-turn Second, the Does the value actually need to change mid-conversation, or only be set once near session start? The PR's own shape points to the latter: creation-time seeding ( So I'd suggest right-sizing it: start with the set-once capability, and add the per-turn mutable channel when a concrete mid-conversation update actually shows up. Could you point to a case where the value must change during a conversation, rather than be fixed when the session opens? If there's a solid one, let's design around that trigger specifically. If not, I'd land the minimal version now and keep the door open. 中文感谢回复——这是一个很有分量的回应,也让我放下了之前的一部分判断。我先把站不住的部分撤掉,把站得住的部分讲清楚。 你说得对,分层并不是真正的症结。 但顺着你自己" 第一,它承载的东西——操作人身份、按会话的规则——是会话级常量:设置一次,逐轮不变。而按轮的 第二, 这个值真的需要在会话进行中改变吗,还是只需在会话开始时设置一次? 这个 PR 自身的形态指向后者:创建时预设( 所以我建议把它收到合适的尺寸:先做"设置一次"的能力,等到真的出现一个会话进行中需要更新的具体场景时,再加上按轮的可变通道。你能否举出一个值必须在会话进行中改变、而不是在会话开启时就固定下来的例子?如果有扎实的例子,我们就针对那个触发点来专门设计;如果没有,我倾向于先把最小版本落地,同时给后续留好口子。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM.
— GPT-5 via Qwen Code /review
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution SummaryPR #5847: feat(serve): add runtime context injection for per-turn system-remindersConflicts FoundTwo files had content conflicts when merging
Root CauseMain branch commit
The PR branch still had all this code inline in Resolutions1.
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Re-review with qwen3.7-max — no new findings. All significant concerns are already covered by prior review comments. Downgraded from Approve to Comment: CI still running.
Build & tests: Build passes, all 17 new tests pass (10 Config RuntimeContext + 6 ACP handler + 1 client injection), eslint clean. 7 pre-existing tsc errors in server.test.ts and integration-tests are not caused by this PR.
Needs Human Review:
- The
entries === nullandArray.isArray(entries)validation branches in the handler have no dedicated test coverage (the single invalid-entries test sends a string). - The bulk
setRuntimeContext()test only covers valid entries; silent-discard behavior for invalid entries is unpinned.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution SummaryPR: #5847 - feat(serve): add runtime context injection for per-turn system-remindersConflicts EncounteredTwo test files had conflicts when merging
Nature of ConflictsBoth conflicts were additive — the PR branch and main branch added new, independent test blocks at the same location in the file. These were not true semantic conflicts, just positional overlaps. Resolution StrategyKept both sides — all new test blocks from both the PR branch and main branch were preserved. File 1:
|
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review of PR #5847 at 0d7c2f0
Finding 1: Tool concurrency silently reduced from 10 to 3 for all daemon prompts
File: packages/cli/src/acp-integration/session/Session.ts (line ~3973)
The new maxConcurrency logic caps concurrent tool execution to Math.min(configuredMaxConcurrency, DAEMON_INVALID_TOOL_PARAMS_THRESHOLD) = Math.min(10, 3) = 3 whenever toolLoopState is defined. Since toolLoopState is created via createDaemonToolLoopState() at every daemon prompt call site (lines 1739, 2076, 2999, 3305) and always passed to runToolCalls, this cap applies to all daemon-mode tool execution — not just scenarios where invalid-param loops are actually occurring.
Combined with the new sequential pre-flight phase (lines ~4004-4028, first 3 calls run one at a time before concurrent dispatch), this significantly changes throughput for models making 4+ parallel tool calls.
This behavioral change is not mentioned in the PR description (which focuses on runtime context injection). If intentional, consider:
- Adding an inline comment explaining the tradeoff
- Using a separate
DAEMON_MAX_TOOL_CONCURRENCYconstant instead of coupling to the loop-detection threshold - Gating the sequential pre-flight on actual loop-detection signals
Finding 2: Dual-cap semantics on runtime context payload size
File: packages/cli/src/serve/server.ts (line ~4268)
The route-level 32 KiB cap measures Buffer.byteLength(JSON.stringify(entries)) (the entire serialized object), while Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES independently caps each individual value at 32 KiB. With RUNTIME_CONTEXT_MAX_ENTRIES = 16, the effective per-value budget shrinks as entry count grows (≈2 KiB per value for 16 entries vs. 32 KiB for a single entry).
Consider: (a) raising the route cap to MAX_VALUE_BYTES * MAX_ENTRIES + overhead, (b) removing it and relying on per-value Config validation, or (c) documenting the relationship.
Review by automated analysis at commit 0d7c2f0. 2 findings, both Suggestion-level confidence.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review of PR #5847 at 6367d5c
Thorough review of all changed code (148 files across runtime context injection, workspace memory remember, channel daemon worker, session loop detection, and memory-scoped agent config). No new high-confidence findings at this commit.
Analysis coverage
- Runtime context injection (config.ts, client.ts, acpAgent.ts, bridge.ts, server.ts, DaemonClient.ts, DaemonSessionClient.ts): Clean implementation. Key validation (
[a-zA-Z0-9_-]{1,64}), value size limits (32 KiB), capacity bounds (16 entries),escapeSystemReminderTagssanitization, prototype-chain isolation viagetOwnRuntimeContextEntries, andresolveTrustedClientIdall properly addressed per prior review feedback. - Workspace memory remember (workspace-remember.ts, remember.ts, memory-scoped-agent-config.ts, bridge.ts): Task lane with serial execution (
tail.then), queue cap (MAX_PENDING=16), task eviction (MAX_TASKS=1000), originator-scoped task visibility,publishWorkspaceEventformanagedmemory_changed events. Memory-scoped agent config correctly restricts file operations to memory paths and denies non-read-only shell commands. - Channel daemon worker (daemon-worker.ts, channel-worker-supervisor.ts, start.ts): Worker process forked with loopback-only URL validation, workspace matching via
canonicalizeWorkspace, startup abort signal handling, and bridge facade that conditionally exposesshellCommandbased on daemon capabilities. - Session loop detection (Session.ts): Enhanced with
loopDetectedpropagation through tool execution pipeline,LOOP_DETECTED_SKIP_MESSAGE/LOOP_DETECTED_CONTEXT_MESSAGEfor model feedback, andrecordSkippedToolCallhoisted above the loop-detection check. The concurrency cap change (min(configured, DAEMON_INVALID_TOOL_PARAMS_THRESHOLD)for daemon sessions) was already flagged by prior review. - Bridge channel lifecycle (bridge.ts):
hasNoChannelWorkcorrectly accounts forsessionSpawnsInFlight,workspaceControlInFlight, andemptyReapPending— replacing allsessionIds.size === 0 && pendingRestoreIds.size === 0checks.withWorkspaceControlwrapper ensures workspace-level calls participate in idle/reap decisions. - SDK types (events.ts, normalizer.ts, types.ts, DaemonClient.ts):
DaemonManagedMemoryChangedDatadiscriminated union onscope: 'managed'with proper validation. UI normalizer correctly handlesmanagedscope withsource/taskId/touchedScopesfields.
All previously raised concerns (prototype chain leak, missing resolveTrustedClientId, dropped rejected array, granular rejection reasons, handler tests, FakeBridge stub, injection escaping test) have been addressed in prior commits.
| @@ -15,8 +15,6 @@ import { | |||
| APPROVAL_MODE_INFO, | |||
There was a problem hiding this comment.
[Critical] This PR silently deletes ~650 lines of unrelated tests from config.test.ts — 7 entire describe blocks for production functions that still exist and have active callers:
matchesServerPattern(10+ cases, used in 10+ call sites)matchesAnyServerPattern(3 cases, used intool-registry.ts+ 5 Config methods)getTeamMemoryEnabledgetTeamMemorySyncEnabledgetDefaultVisionBridgeModelcomputer use settingsMCP Stop dispatch with context usage data
The corresponding vi.mock imports (rebuildTeamAutoMemoryIndex, syncTeamMemory, getTeamMemoryShareabilityWarning) were also removed. The config.ts source changes are minimal (only RuntimeContext methods), so none of the tested functions were removed or moved.
This appears to be a merge-conflict resolution error — the PR branch was likely rebased from a version of main that predated these test blocks.
Suggested fix: Restore the deleted describe blocks and their supporting vi.mock / import lines from origin/main. Only the RuntimeContext additions should remain.
— qwen3.7-max via Qwen Code /review
| @@ -4,11 +4,39 @@ | |||
| * SPDX-License-Identifier: Apache-2.0 | |||
| */ | |||
|
|
|||
| import * as crypto from 'node:crypto'; | |||
There was a problem hiding this comment.
[Critical] This PR expands server.ts from ~854 lines to ~6,113 lines by inlining route handlers that were previously delegated to modular files under packages/cli/src/serve/routes/. Twelve route module files totaling ~4,111 lines still exist on disk but are no longer imported:
routes/session.ts(1,371 lines, 36 route registrations)routes/workspace-extensions.ts(990 lines)routes/sse-events.ts(467 lines)routes/workspace-auth.ts(347 lines)routes/workspace-mcp-control.ts(221 lines)routes/workspace-status.ts(158 lines)routes/permission.ts(104 lines)routes/workspace-tools.ts(97 lines)routes/capabilities.ts(59 lines)routes/workspace-lifecycle.ts(75 lines)routes/daemon-status.ts(91 lines)routes/health-demo.ts(131 lines)
These are now pure dead code that compiles and passes lint but never executes. This creates a dual-source-of-truth landmine: any future bug fix to a route module will silently have no effect.
This appears unrelated to the runtime-context feature. If intentional, it should be in a separate PR. If a merge-conflict artifact, the route module imports should be restored.
— qwen3.7-max via Qwen Code /review
| context_limit: 128000, | ||
| input_tokens: 64000, | ||
| }); | ||
| describe('RuntimeContext', () => { |
There was a problem hiding this comment.
[Suggestion] The PR description claims 10 unit tests covering "prototype-chain isolation," but none of the 10 tests in this describe block verify prototype-chain isolation.
The getOwnRuntimeContextEntries() method was specifically added to prevent prototype-chain leaks when subagent Configs are created via Object.create(parent). The analogous getFileReadCache() has a dedicated FileReadCache isolation describe block that creates parent/child Configs via Object.create and asserts childCache !== parentCache. This block has no equivalent.
Without this test, a future refactor that simplifies getOwnRuntimeContextEntries() back to a plain field access would silently reintroduce the Critical leak with no failing test to catch it.
| describe('RuntimeContext', () => { | |
| describe('RuntimeContext', () => { | |
| it('should isolate entries for child Configs created via Object.create', () => { | |
| const parent = new Config(baseParams); | |
| parent.setRuntimeContextEntry('operator', 'Alice'); | |
| const child = Object.create(parent) as Config; | |
| expect(child.getRuntimeContext().has('operator')).toBe(false); | |
| child.setRuntimeContextEntry('role', 'subagent'); | |
| expect(parent.getRuntimeContext().has('role')).toBe(false); | |
| }); |
— qwen3.7-max via Qwen Code /review
|
|
||
| return { | ||
| sessionId, | ||
| keys: response.keys, |
There was a problem hiding this comment.
[Suggestion] response.keys is accessed without a null-check. The ext-method response is cast as { keys: string[]; rejected?: ... } but no runtime validation is performed. If the child process returns null or undefined (version mismatch during rolling deploy, serialization bug), this throws TypeError: Cannot read properties of undefined (reading 'keys') — which propagates to the route's catch block where sendBridgeError has no matching error class, falling through to a generic 500.
| keys: response.keys, | |
| keys: response?.keys ?? [], | |
| rejected: response?.rejected ?? [], |
— qwen3.7-max via Qwen Code /review
| TrustGateError, | ||
| matchesServerPattern, | ||
| matchesAnyServerPattern, | ||
| } from './config.js'; |
There was a problem hiding this comment.
[Critical] ~650 lines of test coverage deleted for 8 existing, unmodified production features.
This PR removes test suites for matchesServerPattern (9 cases), matchesAnyServerPattern (3), team memory sync gating including the TeamMemoryRootSecurityError security gate (4), getDefaultVisionBridgeModel (7), computer use idle timeout (2), MCP glob-pattern filtering (6), buildContextUsage (8+ assertions), and modalities model-switching assertions. All tested functions still exist in config.ts (e.g. matchesServerPattern at line 642, getDefaultVisionBridgeModel at line 3098). No replacement test files were added.
A regression in MCP access-control glob matching would silently allow blocked servers or deny legitimate ones, with no test to surface it. The team-memory security gate regression removes the safety net preventing syncTeamMemory from running against symlinked/escaped roots.
Suggested fix: Restore the deleted test suites. The RuntimeContext tests should be purely additive to config.test.ts. If the deletions were a merge-conflict resolution artifact, re-add them from origin/main.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| async setRuntimeContext( | ||
| entries: Record<string, string>, |
There was a problem hiding this comment.
[Suggestion] Response type { sessionId: string; keys: string[]; rejected: Array<{ key: string; reason: string }> } is copy-pasted as an inline anonymous type in 4 files (DaemonClient.ts, DaemonSessionClient.ts, bridgeTypes.ts, bridge.ts). The adjacent setSessionLanguage feature uses a named SetSessionLanguageResult interface defined once in packages/sdk-typescript/src/daemon/types.ts.
If the response shape ever changes, 4+ call sites must be updated in lockstep. SDK consumers also see an opaque inline type instead of a named, documented interface.
| entries: Record<string, string>, | |
| // In packages/sdk-typescript/src/daemon/types.ts: | |
| export interface RejectedEntry { | |
| key: string; | |
| reason: string; | |
| } | |
| export interface SetSessionRuntimeContextResult { | |
| sessionId: string; | |
| keys: string[]; | |
| rejected: Array<RejectedEntry>; | |
| } |
Then import and use SetSessionRuntimeContextResult in all 4 files.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| describe('sessionRuntimeContext handler', () => { | ||
| let capturedAgentFactory: |
There was a problem hiding this comment.
[Suggestion] No mixed-bag request test. Each of the 6 existing tests isolates one scenario (all valid, all non-string, all invalid-key, single empty-value deletion, missing sessionId, invalid shape). No test sends a single request combining valid entries, invalid keys, non-string values, and empty-value deletions simultaneously.
A bug that short-circuits the loop on the first rejection (e.g. accidental return instead of continue) or corrupts accumulation state would pass all existing tests. Partial-success behavior is the most common real-world usage pattern.
Suggested test:
it('handles mixed valid, invalid, and delete entries', async () => {
runtimeCtxMap.set('existing', 'stale');
const agent = await setupAgent();
const result = await agent.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
{
sessionId: 'rt-sid',
entries: { valid: 'ok', 'bad key!': 'val', num: 42, existing: '' },
},
);
expect(result['keys']).toEqual(['valid', 'existing']);
expect(result['rejected']).toEqual(
expect.arrayContaining([
{ key: 'bad key!', reason: 'invalid_key' },
{ key: 'num', reason: 'value_not_string' },
]),
);
});— qwen3.7-max via Qwen Code /review
| private static readonly RUNTIME_CONTEXT_KEY_RE = /^[a-zA-Z0-9_-]{1,64}$/; | ||
| private static readonly RUNTIME_CONTEXT_MAX_VALUE_BYTES = 32 * 1024; | ||
| private static readonly RUNTIME_CONTEXT_MAX_ENTRIES = 16; | ||
|
|
There was a problem hiding this comment.
[Suggestion] No aggregate byte-size cap on stored entries. Per-value cap is 32 KiB and max entries is 16, composing to a worst-case 512 KiB store. The route caps a single request's serialized payload at 32 KiB, but 16 sequential POST requests (each setting one ~32 KiB value) bypass this route-level cap and fill all 16 slots.
For models with 128K-token context windows, 512 KiB of UTF-8 runtime context (~131K tokens) can exhaust the entire window on every turn. Even on larger models, it consumes a significant fraction per turn purely from injected metadata.
Consider adding an aggregate cap in setRuntimeContextEntry:
const currentTotal = [...entries.values()].reduce(
(sum, v) => sum + Buffer.byteLength(v, 'utf8'), 0,
);
if (currentTotal + Buffer.byteLength(value, 'utf8') > AGGREGATE_CAP) return false;— qwen3.7-max via Qwen Code /review
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); | ||
| for (const [key, value] of runtimeCtx) { | ||
| const safe = escapeSystemReminderTags(value); |
There was a problem hiding this comment.
[Suggestion] escapeSystemReminderTags(value) is called for every entry on every sendMessageStream turn, with no caching. With 16 entries at up to 32 KiB each, this is up to 512 KiB of regex scanning per turn for unchanged entries.
Consider caching the fully-assembled <system-reminder> string alongside the raw value in the store, invalidating on setRuntimeContextEntry/removeRuntimeContextEntry/setRuntimeContext. The per-turn loop then becomes a simple string push:
for (const reminder of this.config.getRuntimeContextReminders()) {
systemReminders.push(reminder);
}— qwen3.7-max via Qwen Code /review
|
|
||
| getRuntimeContext(): ReadonlyMap<string, string> { | ||
| return this.getOwnRuntimeContextEntries(); | ||
| } |
There was a problem hiding this comment.
[Suggestion] None of the RuntimeContext methods (getRuntimeContext, setRuntimeContextEntry, removeRuntimeContextEntry, setRuntimeContext) have JSDoc comments. Callers have no warning that entries are re-injected on every model turn, consuming tokens proportionally to store size.
For comparison, other per-turn injection sites in client.ts do have comments (e.g., the skill/command delta reminders). Consider adding JSDoc noting the per-turn token-budget impact, so future SDK integrators can make informed decisions about entry size.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Re-review with qwen3.7-max — no new findings. All significant concerns are already covered by prior review comments (20+ inline threads). Downgraded from Approve to Comment: CI still running.
Build & tests: Build passes, all 1,221 tests pass (281 config, 212 client, 175 acpAgent, 553 server). Deterministic analysis clean (112 tsc diagnostics are all build-order cross-package module resolution errors, not PR-related).
Review coverage: 9 parallel review agents (correctness, security, code quality, performance, test coverage, 3× undirected audit) + reverse audit found zero new issues beyond the 31 existing open inline comments. The implementation is well-reviewed at this point.
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review of PR #5847 at d2fa328
Thorough review of all changed code (17 files: runtime context injection, session archive coordinator, group history store, channel base group context, web-shell UI refactoring, sandbox command injection fix, auto-mode classifyAllShell, review deterministic removal).
No new high-confidence findings at this commit. All significant concerns are already covered by prior review comments (20+ inline threads). Key observations:
- Runtime context injection stack (
Configstore ->client.tsper-turn injection -> ACP handler -> bridge -> daemon route -> SDK): implementation is clean. TheescapeSystemReminderTagssanitization at injection time, key regex[a-zA-Z0-9_-]{1,64}, prototype-chain isolation viagetOwnRuntimeContextEntries(), andresolveTrustedClientIdin the bridge are all correctly in place. - Session archive coordinator: in-memory shared/exclusive locking is correctly structured with
try/finallyrelease. ThewithMutableSessionwrapper properly acquires shared locks for mutation routes. notifyAgentSessionClosewiththrowOnFailure/requireFlush: the archive-close path correctly runs the agent flush before bridge state teardown, so a failed flush leaves the session retryable.sandbox_command.jssecurity fix: theexecSync->execFileSyncmigration correctly prevents command injection viaQWEN_SANDBOXby passing the candidate as a separate argv element rather than interpolating into a shell string.- Group history store: synchronous file I/O is safe under Node.js single-threaded execution. JSONL append + compact pattern is sound.
Open threads still needing attention (from prior reviews, not re-reported):
- Dual 32 KiB cap semantics (route-level vs per-value)
- Duplicated validation literals in
acpAgent.tshandler setRuntimeContext(entries)with zero production callers- Missing route-level tests for
POST /session/:id/runtime-context
Build & tests: not run in this review cycle (deterministic checks removed from qwen review in this PR itself).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
3 new findings from re-review with qwen3.7-max. Downgraded from Approve to Comment: CI failing: Post Coverage Comment (ubuntu-latest, 22.x), Test (ubuntu-latest, Node 22.x). All 672 tests pass locally (281 config, 215 client, 176 acpAgent). Deterministic analysis clean (eslint); 5 tsc errors are pre-existing on main. — qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| async setRuntimeContext( |
There was a problem hiding this comment.
[Suggestion] setRuntimeContext does incremental merge (the ACP handler iterates entries via setRuntimeContextEntry per-key, preserving existing keys), but the name implies replacement. Meanwhile, Config.setRuntimeContext() does store.clear() then sets — true replace semantics. An SDK consumer calling sessionClient.setRuntimeContext({ operator: 'Bob' }) would reasonably expect all other entries to be cleared, but they remain.
Consider renaming to updateRuntimeContext or mergeRuntimeContext to match the actual merge behavior, or document the merge semantics in the JSDoc.
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('sessionRuntimeContext handler', () => { |
There was a problem hiding this comment.
[Suggestion] The handler classifies three distinct rejection reasons (invalid_key, value_too_large, capacity_full) at acpAgent.ts:6213-6225, but only invalid_key is tested here. The value_too_large and capacity_full branches have no test coverage.
Suggested additions:
- A test sending a value exceeding 32 KiB to assert
reason: 'value_too_large' - A test filling 16 entries then sending a 17th valid entry to assert
reason: 'capacity_full'
If the handler's inline byte-length check (32 * 1024) or capacity logic drifts from Config's constants, the wrong reason string would be returned with no test to catch it.
— qwen3.7-max via Qwen Code /review
| systemReminders.unshift(userQueryMemory.prompt); | ||
| } | ||
|
|
||
| const runtimeCtx = this.config.getRuntimeContext(); |
There was a problem hiding this comment.
[Suggestion] The ACP handler path (acpAgent.ts) has no aggregate byte limit on stored entries — only per-entry limits (32 KiB each, 16 entries max). This allows up to 512 KiB of system-reminder text per turn through the ACP bridge, which bypasses the route-level 32 KiB aggregate cap in session.ts.
In a long-lived daemon session, this could mean significant token overhead per model call (~128K tokens per turn at max capacity). Consider adding an aggregate byte budget on Config (e.g., 64 KiB total) or logging a warning when aggregate size exceeds a threshold.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Re-review with qwen3.7-max — 2 new findings at this head. Build passes, all 17 new tests pass (10 Config RuntimeContext, 6 acpAgent handler, 1 client injection). Deterministic analysis clean (0 eslint, 113 tsc all pre-existing). 9 parallel review agents + batch verification + reverse audit identified 2 new findings not covered by existing inline comments.
New findings:
session.ts:1436— Route always returns HTTP 200 even when all entries rejected (Suggestion)session.ts:1433— Unsafeentries as Record<string, string>cast without value-type validation (Suggestion)
— qwen3.7-max via Qwen Code /review
| entries as Record<string, string>, | ||
| clientId !== undefined ? { clientId } : undefined, | ||
| ); | ||
| res.status(200).json(response); |
There was a problem hiding this comment.
[Suggestion] Route always returns HTTP 200, even when all entries are rejected.
When the handler rejects every entry in the request (all 16 capacity slots full, all keys invalid, etc.), the response is still 200 { keys: [], rejected: [...] }. Clients cannot distinguish "all entries applied" from "nothing was applied" via HTTP status alone — they must parse the rejected array length.
Consider returning a non-200 status when response.keys.length === 0 and response.rejected.length > 0. For example, 422 Unprocessable Entity would signal "request was well-formed but semantically rejected" without implying a server error. This matches how other control-plane routes behave — a setSessionModel with an invalid model ID would 400.
const hasApplied = response.keys.length > 0;
const status = hasApplied ? 200 : 422;
res.status(status).json(response);Alternatively, always 200 but document the contract clearly. The current behavior is technically correct but surprising for API consumers.
Model: qwen3.7-max
|
|
||
| const response = await bridge.setSessionRuntimeContext( | ||
| sessionId, | ||
| entries as Record<string, string>, |
There was a problem hiding this comment.
[Suggestion] Unsafe type assertion: entries as Record<string, string> without validating that values are strings.
The route validates that entries is a non-null object (lines 1405-1412) but does not check that individual values are strings. A client sending {"entries": {"key": 123}} or {"entries": {"key": {"nested": true}}} passes route-level validation and is forwarded to the bridge as-is. The type error only surfaces later in the acpAgent handler where typeof value !== 'string' catches it with value_not_string.
While the handler layer does catch this, validating at the route boundary is better practice — it gives clients a clear 400 with a specific error before any bridge roundtrip, and prevents non-string values from reaching the bridge/ACP layer where they could cause unexpected behavior in future code paths.
for (const [key, value] of Object.entries(entries)) {
if (typeof value !== 'string') {
res.status(400).json({
error: `Value for key \"${key}\" must be a string, got ${typeof value}`,
code: 'invalid_entry_type',
});
return;
}
}Model: qwen3.7-max
| // vision-bridge gate reflects the new model (it reads getEffectiveInputModalities()). | ||
| expect(updatedConfig['modalities']).toEqual({ image: true }); | ||
| expect(config.getEffectiveInputModalities()).toEqual({ image: true }); | ||
|
|
There was a problem hiding this comment.
[Critical] modalities assertions silently removed from existing test
Three assertions and an explanatory comment were removed from the updateContentGeneratorConfig test in this describe block:
expect(updatedConfig['modalities']).toEqual({ image: true });
expect(config.getEffectiveInputModalities()).toEqual({ image: true });
expect(sources['modalities']?.kind).toBe('computed');The production code still actively uses modalities — config.ts:2925 (getEffectiveInputModalities()), config.ts:3181 (this.contentGeneratorConfig.modalities = config.modalities), config.ts:3186-3188 (sources assignment). The mock was also made inaccurate by removing ['modalities']: { image: true } from the resolveContentGeneratorConfigWithSources return value.
This eliminates coverage for the hot-switch path that verifies modalities are refreshed when the model changes — a behavior the deleted comment explicitly called out as load-bearing for the vision-bridge gate.
| expect(updatedConfig['toolResultContentFormat']).toBe('string'); | |
| // Modalities are model-derived; a hot switch must refresh them so the | |
| // vision-bridge gate reflects the new model (it reads getEffectiveInputModalities()). | |
| expect(updatedConfig['modalities']).toEqual({ image: true }); | |
| expect(config.getEffectiveInputModalities()).toEqual({ image: true }); | |
| // Verify sources are also updated |
— qwen3.7-max via Qwen Code /review
|
|
||
| const runtimeCtxMap = new Map<string, string>(); | ||
|
|
||
| function makeRuntimeCtxConfig(overrides: Record<string, unknown> = {}) { |
There was a problem hiding this comment.
[Suggestion] Test mock re-implements Config validation, creating 3 copies of magic constants
makeRuntimeCtxConfig() duplicates Config.setRuntimeContextEntry's validation logic inline (key regex /^[a-zA-Z0-9_-]{1,64}$/, 32 KiB byte limit, 16-entry capacity). Combined with the handler's own re-validation at acpAgent.ts:6215-6222, there are now 3 copies of each constant.
Handler tests exercise the mock's validation, not the real Config method. If Config changes its regex, both the mock and the handler would silently stay in sync with each other but diverge from Config, and all tests would still pass.
Suggested fix: Use a real Config instance in the test setup for setRuntimeContextEntry, or make the mock delegate to the real method.
— qwen3.7-max via Qwen Code /review
| store.clear(); | ||
| for (const [key, value] of Object.entries(entries)) { | ||
| if ( | ||
| value && |
There was a problem hiding this comment.
[Suggestion] setRuntimeContext bulk method can throw TypeError on truthy non-string values
The value && guard catches falsy non-strings (null, undefined, 0, false), but a truthy non-string (e.g., 42, true, {}) that bypasses TypeScript's static type check at runtime reaches Buffer.byteLength(value, 'utf8'), which throws TypeError: The "string" argument must be of type string.
Currently zero production callers, but the planned initialRuntimeContext flow will pass entries from JSON.parse through a type cast that won't catch this at runtime.
| value && | |
| for (const [key, value] of Object.entries(entries)) { | |
| if ( | |
| typeof value === 'string' && | |
| value && | |
| Config.RUNTIME_CONTEXT_KEY_RE.test(key) && |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-review at new commit (merge with main). 9 parallel agents + verification + reverse audit. 7 findings confirmed (1 Critical, 5 Suggestion, 1 Needs Human Review). Build passes, 6/6 PR tests pass.
Previous rounds resolved: prototype chain leak, injection prevention, granular rejection reasons, handler tests, FakeBridge stub.
Full review report: .qwen/reviews/2026-07-01-review-pr-5847.md
| context_limit: 128000, | ||
| input_tokens: 64000, | ||
| }); | ||
| describe('RuntimeContext', () => { |
There was a problem hiding this comment.
[Critical] Merge conflict deleted 46 tests for unchanged production code.
The following describe blocks were removed while their production implementations remain untouched: matchesServerPattern (9 tests), matchesAnyServerPattern (3), getTeamMemoryEnabled (3), getTeamMemorySyncEnabled (3), getDefaultVisionBridgeModel (7), MCP glob filtering (6), buildContextUsage (12), computer use settings (2). None have equivalent coverage elsewhere.
The corresponding imports were also removed (lines 15-18, 53-58, 148-161), masking the breakage.
This is a merge-conflict artifact — restore the deleted test blocks from origin/main's config.test.ts. Verify with:
git show origin/main:packages/core/src/config/config.test.ts | grep -c "it("
# vs
git show HEAD:packages/core/src/config/config.test.ts | grep -c "it("| '/session/:id/runtime-context', | ||
| mutate(), | ||
| withMutableSession( | ||
| 'POST /session/:id/runtime-context', |
There was a problem hiding this comment.
[Suggestion] This route has zero daemonLog calls. The file has 24 daemonLog references across neighboring routes (session create at L185, prompt at L262, cancel at L771, recap, shell, model, etc.). Successful sets, rejections, 400s, and 413s all happen silently.
At 3 AM debugging "why is the agent claiming to be Alice", there's no server-side trace to correlate the context change with observed behavior.
Suggested: daemonLog?.info(...) on success (session ID + applied keys, NOT values) and daemonLog?.warn(...) on rejections/payload-too-large.
|
|
||
| const response = await bridge.setSessionRuntimeContext( | ||
| sessionId, | ||
| entries as Record<string, string>, |
There was a problem hiding this comment.
[Suggestion] Unsafe type cast. entries is cast to Record<string, string> without per-value string validation. Non-string values (objects, arrays, numbers) pass the 32 KiB payload check and traverse the full bridge→ACP round-trip before being rejected by the ACP handler's typeof value !== 'string' check.
Suggested: add a validation loop before forwarding:
for (const [key, value] of Object.entries(entries as Record<string, unknown>)) {
if (typeof value !== 'string') {
res.status(400).json({ error: `value for key '${key}' must be a string`, code: 'invalid_value_type' });
return;
}
}| return this.runtimeContextEntries; | ||
| } | ||
|
|
||
| getRuntimeContext(): ReadonlyMap<string, string> { |
There was a problem hiding this comment.
[Suggestion] getRuntimeContext() returns the live internal Map reference. ReadonlyMap is a compile-time-only constraint — at runtime, any caller can cast and call .set() directly, bypassing key regex validation, 32 KiB per-value limit, and 16-entry capacity.
The injection loop in client.ts doesn't re-validate keys from the Map — it trusts the contents. A bypassed mutation could inject a key containing </system-reminder> to break out of the reminder envelope.
Suggested: return a defensive copy:
getRuntimeContext(): ReadonlyMap<string, string> {
return new Map(this.getOwnRuntimeContextEntries());
}The copy cost is negligible (at most 16 entries).
| } else if ( | ||
| Buffer.byteLength(value, 'utf8') > 32 * 1024 | ||
| ) { | ||
| rejected.push({ key, reason: 'value_too_large' }); |
There was a problem hiding this comment.
[Suggestion] Untested rejection branches. The 6 handler tests cover value_not_string and invalid_key rejections, but value_too_large (this line) and capacity_full (L6222) have zero test coverage.
If the rejection classification logic has a bug (wrong reason string, misordered checks), it silently returns incorrect rejected metadata to callers.
Suggested tests:
it('rejects values exceeding 32 KiB', async () => {
// ...entries: { big: 'x'.repeat(32 * 1024 + 1) }
// expect rejected: [{ key: 'big', reason: 'value_too_large' }]
});
it('rejects new keys when capacity is full', async () => {
// pre-fill 16 entries, then try a 17th
// expect rejected: [{ key: 'overflow', reason: 'capacity_full' }]
});| continue; | ||
| } | ||
| if (value === '') { | ||
| if (config.getRuntimeContext().has(key)) { |
There was a problem hiding this comment.
[Suggestion] Silent no-op removal. When value === '' and getRuntimeContext().has(key) is false (this line), the key is silently skipped — it appears in neither appliedKeys nor rejected. The caller receives no signal that the key did not exist.
Suggested: either always push to appliedKeys (idempotent delete), or report in rejected:
if (value === '') {
config.removeRuntimeContextEntry(key);
appliedKeys.push(key); // idempotent: always report as applied
}
|
[Needs Human Review] This is likely intentional — the code explicitly avoids injecting content between |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM! ✅ Clean implementation of the runtime context injection feature. All 17 new tests pass, validation logic is solid, and the prototype chain isolation via hasOwnProperty is well-handled.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Re-review at 8b8c780 — no new findings. All significant concerns are already covered by prior review comments (50+ inline threads). Downgraded from Approve to Comment: CI still running.
Build & tests: Build passes, all 17 new tests pass (10 Config RuntimeContext, 6 acpAgent handler, 1 client injection). Deterministic analysis clean (0 eslint, 5 tsc errors all pre-existing on main).
9 parallel review agents + reverse audit confirmed comprehensive prior-review coverage. Key unresolved Critical findings from prior reviewers remain open: test deletion in config.test.ts (~550 lines across 8 features), missing route-level tests, chat history accumulation of stale reminders, and batch processing order dependency.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No new findings beyond the existing review threads at this head.
— GPT-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
4 findings confirmed (1 Critical, 3 Suggestion). 2 Critical/Suggestion comments skipped due to overlap with existing Qwen review threads at the same (path, line). 2 additional findings need human review (low confidence).
Build & tests: All 17 PR-specific tests pass (10 Config RuntimeContext, 6 acpAgent handler, 1 client injection). Deterministic analysis clean — ESLint 0 findings, 5 tsc errors all pre-existing on origin/main.
— qwen3.7-max via Qwen Code /review
| private static readonly RUNTIME_CONTEXT_MAX_VALUE_BYTES = 32 * 1024; | ||
| private static readonly RUNTIME_CONTEXT_MAX_ENTRIES = 16; | ||
|
|
||
| private getOwnRuntimeContextEntries(): Map<string, string> { |
There was a problem hiding this comment.
[Suggestion] Subagent configs created via Object.create(parent) (subagent-manager.ts) get their own empty runtime context store. getOwnRuntimeContextEntries() uses hasOwnProperty to lazy-init a fresh Map, so subagents never inherit the parent's runtime context entries.
If someone sets operator=Alice or rules=Do not modify prod via runtime context, those directives apply only to the main agent's turns. Subagents spawned for tool execution operate without those guardrails — a silent policy gap.
| private getOwnRuntimeContextEntries(): Map<string, string> { | |
| private getOwnRuntimeContextEntries(): Map<string, string> { | |
| if (!Object.prototype.hasOwnProperty.call(this, 'runtimeContextEntries')) { | |
| const parent = Object.getPrototypeOf(this); | |
| const parentEntries = | |
| parent && Object.prototype.hasOwnProperty.call(parent, 'runtimeContextEntries') | |
| ? (parent as Config).getRuntimeContext() | |
| : undefined; | |
| (this as any).runtimeContextEntries = parentEntries | |
| ? new Map(parentEntries) | |
| : new Map<string, string>(); | |
| } | |
| return (this as any).runtimeContextEntries; | |
| } |
— qwen3.7-max via Qwen Code /review
| this.userMemory = newUserMemory; | ||
| } | ||
|
|
||
| private static readonly RUNTIME_CONTEXT_KEY_RE = /^[a-zA-Z0-9_-]{1,64}$/; |
There was a problem hiding this comment.
[Suggestion] RUNTIME_CONTEXT_KEY_RE and RUNTIME_CONTEXT_MAX_VALUE_BYTES are private static, so acpAgent.ts must duplicate the regex /^[a-zA-Z0-9_-]{1,64}$/ and byte limit 32 * 1024 to classify rejection reasons. If Config changes either constant, the diagnostic branches silently drift — keys that Config accepts may be reported as invalid_key by the handler, or vice versa.
| private static readonly RUNTIME_CONTEXT_KEY_RE = /^[a-zA-Z0-9_-]{1,64}$/; | |
| static readonly RUNTIME_CONTEXT_KEY_RE = /^[a-zA-Z0-9_-]{1,64}$/; | |
| static readonly RUNTIME_CONTEXT_MAX_VALUE_BYTES = 32 * 1024; |
Then in acpAgent.ts, reference Config.RUNTIME_CONTEXT_KEY_RE and Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES instead of inline literals.
— qwen3.7-max via Qwen Code /review
| fireNotificationHook: vi.fn().mockResolvedValue({}), | ||
| })); | ||
|
|
||
| describe('matchesServerPattern', () => { |
There was a problem hiding this comment.
[Critical] ~550 lines of unrelated tests deleted during merge conflict resolution. Removed tests cover matchesServerPattern/matchesAnyServerPattern (12 tests), team memory security gates (5 tests including untrusted workspace gating and symlink escape rejection), getComputerUseIdleTimeoutMs (2 tests), and MCP context usage (1 test). All tested functions still exist in config.ts and no other file contains these tests.
| describe('matchesServerPattern', () => { | |
| // Restore deleted tests from origin/main's config.test.ts |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const serialized = JSON.stringify(entries); | ||
| if (Buffer.byteLength(serialized, 'utf8') > 32 * 1024) { |
There was a problem hiding this comment.
[Critical] Route-level 32 KiB payload cap mismatches Config's per-value 32 KiB × 16 entries = 512 KiB capacity. A payload of 2 entries at 20 KiB each passes per-entry config validation but fails this route check with a misleading 413. PR description states "Payload limit: 256 KiB" but code enforces 32 * 1024.
| if (Buffer.byteLength(serialized, 'utf8') > 32 * 1024) { | |
| if (Buffer.byteLength(serialized, 'utf8') > 256 * 1024) { |
— qwen3.7-max via Qwen Code /review
| Buffer.byteLength(value, 'utf8') > 32 * 1024 | ||
| ) { | ||
| rejected.push({ key, reason: 'value_too_large' }); | ||
| } else { |
There was a problem hiding this comment.
[Critical] Three rejection branches lack test coverage: value_too_large (this line), capacity_full (line 6222), and Array.isArray(entries) handler-level check. These are part of the public API contract — SDK clients depend on exact reason strings ('value_too_large', 'capacity_full') for error handling.
Suggested tests:
- Fill 16 entries, attempt 17th → assert
rejected[0].reason === 'capacity_full' - Send 32 KiB+ value → assert
rejected[0].reason === 'value_too_large' - Send array input → assert throws
— qwen3.7-max via Qwen Code /review
| } else if (!/^[a-zA-Z0-9_-]{1,64}$/.test(key)) { | ||
| rejected.push({ key, reason: 'invalid_key' }); | ||
| } else if ( | ||
| Buffer.byteLength(value, 'utf8') > 32 * 1024 |
There was a problem hiding this comment.
[Suggestion] Key regex /^[a-zA-Z0-9_-]{1,64}$/ and 32 * 1024 are hardcoded inline, duplicating Config.RUNTIME_CONTEXT_KEY_RE and RUNTIME_CONTEXT_MAX_VALUE_BYTES. If Config constants change, this handler's validation silently diverges.
Consider exporting the constants from Config or adding a Config.classifyRuntimeContextRejection(key, value) method that returns the reason string, so the handler doesn't re-derive it.
— qwen3.7-max via Qwen Code /review
|
|
||
| const response = await bridge.setSessionRuntimeContext( | ||
| sessionId, | ||
| entries as Record<string, string>, |
There was a problem hiding this comment.
[Suggestion] entries as Record<string, string> casts without validating that individual values are strings. The downstream acpAgent handler catches non-strings with typeof value !== 'string', but this type assertion masks the validation gap at the route layer.
| entries as Record<string, string>, | |
| entries as Record<string, unknown>, |
— qwen3.7-max via Qwen Code /review
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #5847Base branch
Conflicted files1.
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review of the runtime-context feature. The core mechanism (Config store → per-turn injection with escapeSystemReminderTags → ACP ext-method → daemon route → SDK) is coherent and value-escaping is handled correctly. My main concern is a large, unrelated test deletion that appears accidental; the rest are cleanup/scope notes.
🤖 Reviewed with Claude Code (Opus 4.8)
| }); | ||
| }); | ||
|
|
||
| describe('Server Config (config.ts)', () => { |
There was a problem hiding this comment.
This PR removes ~450 lines of test coverage unrelated to runtime-context. Deleted here: describe('matchesServerPattern') / matchesAnyServerPattern, the team-memory security-gating tests (including the TeamMemoryRootSecurityError symlink-escape refusal — a security regression guard), describe('computer use settings'), and the modalities hot-switch assertions in Model Switching.
All of the code under test still exists in config.ts (matchesServerPattern @668, getComputerUseIdleTimeoutMs @5087, getEffectiveInputModalities @3063, the team-memory sync gate in refreshHierarchicalMemory), so this is live coverage being dropped — most likely a bad rebase/merge artifact rather than an intentional change. Please restore these blocks; the runtime-context tests should be purely additive.
| } | ||
| } else if (config.setRuntimeContextEntry(key, value)) { | ||
| appliedKeys.push(key); | ||
| } else if (!/^[a-zA-Z0-9_-]{1,64}$/.test(key)) { |
There was a problem hiding this comment.
The rejection-reason classification re-implements Config's validation constants (/^[a-zA-Z0-9_-]{1,64}$/ and 32 * 1024) that already exist as Config.RUNTIME_CONTEXT_KEY_RE / RUNTIME_CONTEXT_MAX_VALUE_BYTES. If those limits change in config.ts, this handler will silently misclassify the reason (e.g. report capacity_full for a value that is actually too large, or invalid_key under a widened regex). Consider having setRuntimeContextEntry return a typed reason instead of a boolean so there's a single source of truth for both the accept/reject decision and its cause.
|
|
||
| private getOwnRuntimeContextEntries(): Map<string, string> { | ||
| if ( | ||
| !Object.prototype.hasOwnProperty.call(this, 'runtimeContextEntries') |
There was a problem hiding this comment.
getOwnRuntimeContextEntries guards with hasOwnProperty('runtimeContextEntries') and lazily re-creates the Map, but the field is initialized inline (private runtimeContextEntries: Map<...> = new Map()), so every constructed Config already has it as an own property — the guard never fires. Unless there's a real path that builds Config without running field initializers (Object.create/prototype reuse), this helper can collapse to return this.runtimeContextEntries;.
| .codegraph | ||
| .qwen/computer-use/installed.json | ||
| .playwright-mcp/ | ||
| pnpm-lock.yaml |
There was a problem hiding this comment.
pnpm-lock.yaml looks unrelated to this feature — the repo uses npm (package-lock.json). If it leaked in from a local pnpm setup, please drop it to keep the PR scoped to runtime-context.
|
|
||
| app.post( | ||
| '/session/:id/runtime-context', | ||
| mutate(), |
There was a problem hiding this comment.
[Suggestion] This route uses mutate() (non-strict auth gate). Most comparable persistent session-state routes in this file use mutate({ strict: true }) — including approval-mode (L1691), shell (L1575), artifacts (L637), continue (L756), goal/clear (L745), and rewind (L1664). However, model (L1364), language (L1733), and recap (L1394) also use the non-strict mutate(), so this follows an existing subset of the convention.
The concern is that runtime context values are injected as <system-reminder> blocks on every future model turn — unlike a model or language switch, which are one-time settings. Any local process on the machine (compromised dependency, co-tenant, malicious npm package) can POST persistent instructions that the model sees as trusted context on every subsequent turn indefinitely. This makes runtime context a higher-value injection target than a model name or language preference.
Consider using mutate({ strict: true }) to match the majority of persistent-state routes, or add a code comment explaining why non-strict is intentional for this route.
| mutate(), | |
| mutate({ strict: true }), |
— qwen3.7-max via Qwen Code /review
What this PR does
Adds a per-session key-value RuntimeContext store that external callers (daemon API, SDK) can populate with session-scoped dynamic context. Entries are injected as
<system-reminder>blocks on every UserQuery/Cron turn, providing a runtime-mutable layer between the static system prompt and the conversation — analogous to theappendmechanism in the Claude Agent SDK, but updatable mid-session.Full stack:
Configstore → per-turn injection insendMessageStream→ ACP ext-method → bridge implementation → daemon HTTP routePOST /session/:id/runtime-context→ SDKDaemonClient/DaemonSessionClient.Why it's needed
The Claude Agent SDK distinguishes between static system prompts (
systemPrompt,append) and dynamic conversation context (CLAUDE.md injected into conversation, not the system prompt). Qwen Code currently lacks an equivalent mechanism for session-scoped dynamic context that can be set at session creation and updated mid-session without modifyingQWEN.md.Use cases include: operator identity injection, per-session rule overlays, dynamic configuration that varies by session but shouldn't be persisted to project-level files. This keeps
QWEN.mdstable as a project-level configuration file while enabling session-level customization through the daemon/SDK API.Reviewer Test Plan
How to verify
npx vitest run packages/core/src/config/config.test.ts -t "RuntimeContext"— 10 unit tests covering CRUD, key/value/count validation, return-value accuracy, and prototype-chain isolation.npx vitest run packages/cli/src/acp-integration/acpAgent.test.ts -t "sessionRuntimeContext"— 6 tests covering happy path, removal, rejection, and error cases.qwen serve, create a session, thencurl -X POST /session/:id/runtime-context -d '{"entries":{"operator":"Alice","rules":"No prod changes"}}'. Verify the response contains{ keys, rejected }.<system-reminder>blocks containing[key] valueformatted content.GET /capabilitiesshould includesession_runtime_contextin the feature list.Evidence (Before & After)
N/A — new feature, no UI changes.
Tested on
Environment (optional)
Unit tests + integration tests. Integration testing requires a running daemon.
Risk & Scope
initialRuntimeContextonBridgeSpawnRequest(session-creation-time seeding) is designed but not wired — will land in a follow-up.中文说明
这个 PR 做了什么
新增了一个会话级键值 RuntimeContext 存储，外部调用方（daemon API、SDK）可以填充会话作用域的动态上下文。条目在每个 UserQuery/Cron 轮次以
<system-reminder>块注入，提供了一个介于静态系统提示词和对话之间的、运行时可修改的层——类似 Claude Agent SDK 的append机制，但支持会话中途更新。完整链路：
Config存储 →sendMessageStream中按轮注入 → ACP ext-method → bridge 实现 → daemon HTTP 路由POST /session/:id/runtime-context→ SDKDaemonClient/DaemonSessionClient。为什么需要
Claude Agent SDK 区分静态系统提示词（
systemPrompt、append）和动态对话上下文（CLAUDE.md 作为对话内容注入，而非系统提示词）。Qwen Code 目前缺少一个等价机制——能在会话创建时设置、且在会话中途通过 API 更新的会话级动态上下文。使用场景包括：操作人身份注入、按会话的规则叠加、因会话而异但不应持久化到项目级文件的动态配置。这让
QWEN.md保持稳定作为项目级配置文件，同时通过 daemon/SDK API 实现会话级定制。评审测试计划
如何验证
npx vitest run packages/core/src/config/config.test.ts -t "RuntimeContext"— 10 个单元测试覆盖增删改查、key/value/count 校验、返回值准确性和原型链隔离。npx vitest run packages/cli/src/acp-integration/acpAgent.test.ts -t "sessionRuntimeContext"— 6 个测试覆盖正常路径、删除、拒绝和错误场景。qwen serve，创建会话，然后curl -X POST /session/:id/runtime-context -d '{"entries":{"operator":"Alice","rules":"No prod changes"}}'。验证响应包含{ keys, rejected }。[key] value格式内容的<system-reminder>块。GET /capabilities应在功能列表中包含session_runtime_context。证据（前后对比）
不适用——新功能，无 UI 变更。
测试平台
风险与范围
BridgeSpawnRequest上的initialRuntimeContext（会话创建时预设）已设计但未接线——将在后续 PR 落地。