Skip to content
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c6dc809
feat(serve): add runtime context injection for per-turn system-reminders
callmeYe Jun 25, 2026
8ca0e0c
fix(sdk): bump browser bundle size limit for runtime context APIs
callmeYe Jun 25, 2026
8eda5fe
fix(test): update test snapshots for runtime context capability
callmeYe Jun 25, 2026
419f3d5
fix: address review feedback for runtime context
callmeYe Jun 25, 2026
71172e7
fix(test): prefix unused context param with underscore
callmeYe Jun 25, 2026
5a78b44
fix: address wenshao review — prototype isolation, trusted client ID,…
callmeYe Jun 25, 2026
887a2ea
fix: address wenshao round-2 suggestions
callmeYe Jun 25, 2026
cf78b57
test(cli): add handler tests for sessionRuntimeContext ext-method
callmeYe Jun 25, 2026
f8d4162
fix(test): use runAcpAgent instead of non-existent runAcpMode
callmeYe Jun 25, 2026
ec0428e
fix(test): fix arrow-body-style lint error in Session mock
callmeYe Jun 25, 2026
e60fe0a
fix(test): pass cwd and mcpServers to newSession in runtime context t…
callmeYe Jun 25, 2026
7fb5ff3
fix(test): resolve connection.closed in afterEach to prevent shutdown…
callmeYe Jun 25, 2026
5c33b68
ci: trigger fresh CI run (flaky no-AK smoke test)
callmeYe Jun 25, 2026
421635b
fix(test): add session_runtime_context to integration test capabiliti…
callmeYe Jun 26, 2026
0d7c2f0
fix(test): restore missing closing braces lost during conflict resolu…
callmeYe Jul 1, 2026
6367d5c
fix(serve): move runtime-context route to routes/session.ts after ser…
callmeYe Jul 1, 2026
02c14cb
merge: sync with main
callmeYe Jul 1, 2026
d2fa328
merge: sync with main, resolve conflicts in routes/session.ts and bui…
callmeYe Jul 1, 2026
2e8192c
fix: remove accidental pnpm-lock.yaml from tracked files
callmeYe Jul 1, 2026
48ccb80
merge: sync with main, resolve config.test.ts conflict
callmeYe Jul 1, 2026
2626aa0
fix: remove pnpm-lock.yaml and add to .gitignore
callmeYe Jul 1, 2026
8b8c780
merge: sync with main (MCP test fix #6120)
callmeYe Jul 1, 2026
541dba6
merge: sync with main
callmeYe Jul 1, 2026
0677a38
merge: sync with main
qwen-code-dev-bot Jul 5, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,4 @@ tmp/
.codegraph
.qwen/computer-use/installed.json
.playwright-mcp/
pnpm-lock.yaml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

1 change: 1 addition & 0 deletions integration-tests/cli/qwen-serve-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ describe('qwen serve — capabilities envelope', () => {
'permission_mediation',
'non_blocking_prompt',
'session_language',
'session_runtime_context',
'session_rewind',
'workspace_hooks',
'session_hooks',
Expand Down
29 changes: 29 additions & 0 deletions packages/acp-bridge/src/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4808,6 +4808,35 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge {
}
},

async setSessionRuntimeContext(sessionId, entries, context) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const entry = byId.get(sessionId);
if (!entry) throw new SessionNotFoundError(sessionId);
const info = channelInfoForEntry(entry);
if (!info || info.isDying) throw new SessionNotFoundError(sessionId);
resolveTrustedClientId(entry, context?.clientId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 response = (await Promise.race([
withTimeout(
entry.connection.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
{ sessionId, entries },
),
initTimeoutMs,
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
),
getTransportClosedReject(entry),
])) as {
keys: string[];
rejected?: Array<{ key: string; reason: string }>;
};

return {
sessionId,
keys: response.keys,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
keys: response.keys,
keys: response?.keys ?? [],
rejected: response?.rejected ?? [],

— qwen3.7-max via Qwen Code /review

rejected: response.rejected ?? [],
};
},

async generateSessionRecap(sessionId, _context) {
// Thin pass-through to `qwen/control/session/
// recap` — the ACP child runs `generateSessionRecap` against the
Expand Down
15 changes: 15 additions & 0 deletions packages/acp-bridge/src/bridgeTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,21 @@ export interface AcpSessionBridge {
persisted: boolean;
}>;

/**
* Set, update, or remove runtime context entries on a live session.
* Entries are injected as per-turn <system-reminder> blocks on the
* next model call. Passing an empty string for a value removes that key.
*/
setSessionRuntimeContext(
sessionId: string,
entries: Record<string, string>,
context?: BridgeClientRequestContext,
): Promise<{
sessionId: string;
keys: string[];
rejected: Array<{ key: string; reason: string }>;
}>;

/**
* Generate a one-sentence "where did I leave off" recap of a live
* session. Forwards through `qwen/control/session/recap`, which
Expand Down
1 change: 1 addition & 0 deletions packages/acp-bridge/src/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export const SERVE_CONTROL_EXT_METHODS = {
sessionGoalClear: 'qwen/control/session/goal/clear',
workspaceMcpRuntimeAdd: 'qwen/control/workspace/mcp/runtime-add',
workspaceMcpRuntimeRemove: 'qwen/control/workspace/mcp/runtime-remove',
sessionRuntimeContext: 'qwen/control/session/runtime_context',
workspaceReload: 'qwen/control/workspace/reload',
workspaceExtensionsRefresh: 'qwen/control/workspace/extensions/refresh',
/**
Expand Down
225 changes: 225 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8028,3 +8028,228 @@ describe('deliverClientMcpMessage — reverse tool channel (#5626)', () => {
});
});
});

describe('sessionRuntimeContext handler', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Missing closing braces for the it('returns the parent reply payload on success') block and the enclosing describe('deliverClientMcpMessage ...') block. The new describe('sessionRuntimeContext handler', ...) block was appended directly after the last expect() call without first closing the preceding it and describe blocks. This causes tsc error TS1005 ('}' expected) at EOF (line 8194) and esbuild Unexpected end of file — the entire packages/cli test file fails to compile.

Suggested change
describe('sessionRuntimeContext handler', () => {
});
});
});
describe('sessionRuntimeContext handler', () => {

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. A test sending a value exceeding 32 KiB to assert reason: 'value_too_large'
  2. 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

let capturedAgentFactory:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

| ((conn: { closed: Promise<void> }) => {
initialize: (args: Record<string, unknown>) => Promise<unknown>;
newSession: (args: Record<string, unknown>) => Promise<unknown>;
extMethod: (
method: string,
args: Record<string, unknown>,
) => Promise<Record<string, unknown>>;
})
| undefined;

let processExitSpy: MockInstance<typeof process.exit>;
let stdinDestroySpy: MockInstance<typeof process.stdin.destroy>;
let stdoutDestroySpy: MockInstance<typeof process.stdout.destroy>;

const mockConnectionState = {
promise: undefined as unknown as Promise<void>,
resolve: undefined as unknown as () => void,
reset() {
this.promise = new Promise<void>((r) => {
this.resolve = r;
});
},
};

const runtimeCtxMap = new Map<string, string>();

function makeRuntimeCtxConfig(overrides: Record<string, unknown> = {}) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

return {
initialize: vi.fn().mockResolvedValue(undefined),
waitForMcpReady: vi.fn().mockResolvedValue(undefined),
getModel: vi.fn().mockReturnValue('m'),
getModelsConfig: vi.fn().mockReturnValue({
getCurrentAuthType: vi.fn().mockReturnValue('api-key'),
syncAfterAuthRefresh: vi.fn(),
}),
reloadModelProvidersConfig: vi.fn(),
refreshAuth: vi.fn().mockResolvedValue(undefined),
getTargetDir: vi.fn().mockReturnValue('/tmp'),
getContentGeneratorConfig: vi.fn().mockReturnValue({}),
getAvailableModels: vi.fn().mockReturnValue([]),
getModes: vi.fn().mockReturnValue([]),
getApprovalMode: vi.fn().mockReturnValue('default'),
getSessionId: vi.fn().mockReturnValue('rt-sid'),
getAuthType: vi.fn().mockReturnValue('api-key'),
getAllConfiguredModels: vi.fn().mockReturnValue([]),
getGeminiClient: vi.fn().mockReturnValue({
isInitialized: vi.fn().mockReturnValue(true),
initialize: vi.fn().mockResolvedValue(undefined),
waitForMcpReady: vi.fn().mockResolvedValue(undefined),
refreshSystemInstruction: vi.fn().mockResolvedValue(undefined),
}),
getFileSystemService: vi.fn().mockReturnValue(undefined),
setFileSystemService: vi.fn(),
getHookSystem: vi.fn().mockReturnValue(undefined),
getDisableAllHooks: vi.fn().mockReturnValue(true),
hasHooksForEvent: vi.fn().mockReturnValue(false),
getWorkspaceContext: vi.fn().mockReturnValue({}),
getDebugMode: vi.fn().mockReturnValue(false),
getRuntimeContext: vi.fn().mockReturnValue(runtimeCtxMap),
setRuntimeContextEntry: vi
.fn()
.mockImplementation((key: string, value: string) => {
if (!/^[a-zA-Z0-9_-]{1,64}$/.test(key)) return false;
if (Buffer.byteLength(value, 'utf8') > 32 * 1024) return false;
if (!runtimeCtxMap.has(key) && runtimeCtxMap.size >= 16) return false;
runtimeCtxMap.set(key, value);
return true;
}),
removeRuntimeContextEntry: vi.fn().mockImplementation((key: string) => {
runtimeCtxMap.delete(key);
}),
...overrides,
};
}

beforeEach(() => {
vi.clearAllMocks();
runtimeCtxMap.clear();
mockConnectionState.reset();
capturedAgentFactory = undefined;

vi.mocked(AgentSideConnection).mockImplementation((factory: unknown) => {
capturedAgentFactory = factory as typeof capturedAgentFactory;
return {
get closed() {
return mockConnectionState.promise;
},
} as unknown as InstanceType<typeof AgentSideConnection>;
});

processExitSpy = vi
.spyOn(process, 'exit')
.mockImplementation((() => undefined) as unknown as typeof process.exit);
stdinDestroySpy = vi
.spyOn(process.stdin, 'destroy')
.mockImplementation(() => process.stdin);
stdoutDestroySpy = vi
.spyOn(process.stdout, 'destroy')
.mockImplementation(() => process.stdout);
});

afterEach(() => {
mockConnectionState.resolve();
processExitSpy.mockRestore();
stdinDestroySpy.mockRestore();
stdoutDestroySpy.mockRestore();
});

async function setupAgent() {
const cfg = makeRuntimeCtxConfig();

vi.mocked(loadSettings).mockReturnValue({
merged: { mcpServers: {} },
getUserHooks: vi.fn().mockReturnValue({}),
getProjectHooks: vi.fn().mockReturnValue({}),
} as unknown as LoadedSettings);

vi.mocked(loadCliConfig).mockResolvedValue(cfg as unknown as Config);

vi.mocked(Session).mockImplementation(
() =>
({
getId: vi.fn().mockReturnValue('rt-sid'),
getConfig: vi.fn().mockReturnValue(cfg),
sendAvailableCommandsUpdate: vi.fn().mockResolvedValue(undefined),
installRewriter: vi.fn(),
startCronScheduler: vi.fn(),
dispose: vi.fn(),
}) as unknown as Session,
);

vi.mocked(buildAvailableCommandsSnapshot).mockResolvedValue({
availableCommands: [],
availableSkills: [],
});

const bootConfig = makeRuntimeCtxConfig();
runAcpAgent(
bootConfig as unknown as Config,
{ merged: { mcpServers: {} } } as unknown as LoadedSettings,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
{ 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

{} as CliArgs,
);
await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined());
const agent = capturedAgentFactory!({
closed: mockConnectionState.promise,
});
await agent.initialize({});
await agent.newSession({ cwd: '/tmp', mcpServers: [] });
return agent;
}

it('sets entries and returns applied keys', async () => {
const agent = await setupAgent();
const result = await agent.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
{
sessionId: 'rt-sid',
entries: { operator: 'Alice', rules: 'no-prod' },
},
);
expect(result['keys']).toEqual(['operator', 'rules']);
expect(result['rejected']).toEqual([]);
expect(runtimeCtxMap.get('operator')).toBe('Alice');
});

it('removes entries with empty string values', async () => {
runtimeCtxMap.set('old-key', 'stale');
const agent = await setupAgent();
const result = await agent.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
{ sessionId: 'rt-sid', entries: { 'old-key': '' } },
);
expect(result['keys']).toEqual(['old-key']);
expect(runtimeCtxMap.has('old-key')).toBe(false);
});

it('rejects non-string values', async () => {
const agent = await setupAgent();
const result = await agent.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
{ sessionId: 'rt-sid', entries: { bad: 123 } },
);
expect(result['keys']).toEqual([]);
expect(
(result['rejected'] as Array<{ key: string; reason: string }>)[0],
).toEqual({
key: 'bad',
reason: 'value_not_string',
});
});

it('rejects invalid keys', async () => {
const agent = await setupAgent();
const result = await agent.extMethod(
SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext,
{ sessionId: 'rt-sid', entries: { 'invalid key!': 'value' } },
);
expect(result['keys']).toEqual([]);
expect(
(result['rejected'] as Array<{ key: string; reason: string }>)[0]?.reason,
).toBe('invalid_key');
});

it('throws on missing sessionId', async () => {
const agent = await setupAgent();
await expect(
agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext, {
entries: { a: 'b' },
}),
).rejects.toThrow(/sessionId/);
});

it('throws on invalid entries shape', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

const agent = await setupAgent();
await expect(
agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext, {
sessionId: 'rt-sid',
entries: 'not-an-object',
}),
).rejects.toThrow(/entries/);
});
});
49 changes: 49 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6174,6 +6174,55 @@ class QwenAgent implements Agent {

return { language: resolvedLanguage, outputLanguage, refreshed };
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext: {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const sessionId = params['sessionId'];
const entries = params['entries'];
if (typeof sessionId !== 'string' || sessionId.length === 0) {
throw RequestError.invalidParams(
undefined,
'Invalid or missing sessionId',
);
}
if (
typeof entries !== 'object' ||
entries === null ||
Array.isArray(entries)
) {
throw RequestError.invalidParams(
undefined,
'`entries` must be a non-null object',
);
}
const session = this.sessionOrThrow(sessionId);
const config = session.getConfig();
const appliedKeys: string[] = [];
const rejected: Array<{ key: string; reason: string }> = [];
for (const [key, value] of Object.entries(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Batch entry processing order makes capacity-full behavior non-deterministic and surprising. If a caller sends { newKey: 'v', oldKey: '' } when the store is already at the 16-entry limit, newKey is processed first and rejected as capacity_full before oldKey: '' is processed to free a slot. Reordering the keys in the same batch (e.g. { oldKey: '', newKey: 'v' }) succeeds. A single logical atomic update — swap one entry — either works or fails purely based on JS object key enumeration order, which is insertion-order for string keys. This produces inconsistent results for callers who cannot predict Object.entries order.

Suggested change
for (const [key, value] of Object.entries(
Apply all empty-string removals first, then process non-empty insertions:
```ts
const toRemove = Object.entries(entries as Record<string, unknown>).filter(([, v]) => typeof v === 'string' && v === '');
const toSet = Object.entries(entries as Record<string, unknown>).filter(([, v]) => typeof v === 'string' && v !== '');
const nonString = Object.entries(entries as Record<string, unknown>).filter(([, v]) => typeof v !== 'string');
for (const [key] of nonString) rejected.push({ key, reason: 'value_not_string' });
for (const [key] of toRemove) {
if (config.getRuntimeContext().has(key)) { config.removeRuntimeContextEntry(key); appliedKeys.push(key); }
}
for (const [key, value] of toSet) {
// ... set/reject logic
}

_— claude-sonnet-4-6 via Qwen Code /review_

---
_Generated by [Claude Code](https://claude.ai/code)_

entries as Record<string, unknown>,
)) {
if (typeof value !== 'string') {
rejected.push({ key, reason: 'value_not_string' });
continue;
}
if (value === '') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (config.getRuntimeContext().has(key)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
}

config.removeRuntimeContextEntry(key);
appliedKeys.push(key);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
appliedKeys.push(key);
if (value === '') {
config.removeRuntimeContextEntry(key);
appliedKeys.push(key);
} else if (config.setRuntimeContextEntry(key, value)) {

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] When value === '' (deletion semantics) and the key does NOT currently exist in the store, the handler silently skips it — the key appears in neither keys nor rejected. A caller issuing { oldKey: '' } receives { keys: [], rejected: [] } and cannot distinguish "key was already absent (idempotent no-op)" from "request was lost". Consider making deletion idempotent: always push to appliedKeys when value === '', since the caller's desired end-state (key absent) is already satisfied.

Suggested change
}
if (value === '') {
if (config.getRuntimeContext().has(key)) {
config.removeRuntimeContextEntry(key);
}
appliedKeys.push(key);
} else if (config.setRuntimeContextEntry(key, value)) {

— 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)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The rejection-reason cascade re-implements Config's internal validation checks using inlined literals (/^[a-zA-Z0-9_-]{1,64}$/ and 32 * 1024) that duplicate Config.RUNTIME_CONTEXT_KEY_RE and Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES (both private static readonly). Because the cascade runs AFTER setRuntimeContextEntry already applied Config's constants, if Config's key pattern or byte limit ever changes, the handler will report the wrong rejection reason — e.g., reporting invalid_key for a key that Config actually accepted (or vice versa), and value_too_large for a size that Config no longer enforces. The constants are not exported, so there is no single source of truth.

Suggested change
} else if (!/^[a-zA-Z0-9_-]{1,64}$/.test(key)) {
Change `Config.setRuntimeContextEntry` to return a discriminated result rather than a bare boolean:
```ts
// config.ts
setRuntimeContextEntry(key: string, value: string): { ok: true } | { ok: false; reason: 'invalid_key' | 'value_too_large' | 'capacity_full' } {
if (!Config.RUNTIME_CONTEXT_KEY_RE.test(key)) return { ok: false, reason: 'invalid_key' };
// ...
if (byteLen > Config.RUNTIME_CONTEXT_MAX_VALUE_BYTES) return { ok: false, reason: 'value_too_large' };
if (!entries.has(key) && entries.size >= Config.RUNTIME_CONTEXT_MAX_ENTRIES) return { ok: false, reason: 'capacity_full' };
entries.set(key, value);
return { ok: true };
}
// acpAgent.ts handler
const result = config.setRuntimeContextEntry(key, value);
if (result.ok) {
appliedKeys.push(key);
} else {
rejected.push({ key, reason: result.reason });
}

This eliminates the duplicated literals and makes the rejection reason authoritative from Config.


_— claude-sonnet-4-6 via Qwen Code /review_

---
_Generated by [Claude Code](https://claude.ai/code)_

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rejected.push({ key, reason: 'invalid_key' });
} else if (
Buffer.byteLength(value, 'utf8') > 32 * 1024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

) {
rejected.push({ key, reason: 'value_too_large' });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. Send { validkey: 'x'.repeat(32 * 1024 + 1) } and assert rejected[0].reason === 'value_too_large'.
  2. 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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' }]
});

} else {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
} 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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

rejected.push({ key, reason: 'capacity_full' });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
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

}
}
return { keys: appliedKeys, rejected };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

}
case SERVE_CONTROL_EXT_METHODS.sessionRecap: {
// Generate a one-sentence "where did I leave off" summary.
// Best-effort: returns `null` on short history or model failure.
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/serve/capabilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ export const SERVE_CAPABILITY_REGISTRY = {
writer_idle_timeout: { since: 'v1' },
non_blocking_prompt: { since: 'v1' },
session_language: { since: 'v1' },
session_runtime_context: { since: 'v1' },
session_rewind: { since: 'v1' },
workspace_hooks: { since: 'v1' },
session_hooks: { since: 'v1' },
Expand Down
Loading
Loading