-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(serve): add runtime context injection for per-turn system-reminders #5847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 23 commits
c6dc809
8ca0e0c
8eda5fe
419f3d5
71172e7
5a78b44
887a2ea
cf78b57
f8d4162
ec0428e
e60fe0a
7fb5ff3
5c33b68
421635b
0d7c2f0
6367d5c
02c14cb
d2fa328
2e8192c
48ccb80
2626aa0
8b8c780
541dba6
0677a38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,3 +126,4 @@ tmp/ | |
| .codegraph | ||
| .qwen/computer-use/installed.json | ||
| .playwright-mcp/ | ||
| pnpm-lock.yaml | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -4808,6 +4808,35 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { | |||||||
| } | ||||||||
| }, | ||||||||
|
|
||||||||
| async setSessionRuntimeContext(sessionId, entries, context) { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] No bridge integration tests for Suggested: add tests covering happy path forwarding, unknown sessionId → — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged. The bridge follows the exact same pattern as |
||||||||
| 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); | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Also: If events are intentionally omitted for this mutation, add a comment explaining why (mirroring the pattern at — 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, | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— 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 | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8028,3 +8028,228 @@ describe('deliverClientMcpMessage — reverse tool channel (#5626)', () => { | |||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| describe('sessionRuntimeContext handler', () => { | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Missing closing braces for the
Suggested change
— qwen3.7-max via Qwen Code /review
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The handler classifies three distinct rejection reasons ( Suggested additions:
If the handler's inline byte-length check ( — qwen3.7-max via Qwen Code /review |
||||||||||||||
| let capturedAgentFactory: | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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> = {}) { | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 — 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, | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] All 6 tests in the This is why CI's
Suggested change
Also fix the — 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 () => { | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The — 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/); | ||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6174,6 +6174,55 @@ class QwenAgent implements Agent { | |||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return { language: resolvedLanguage, outputLanguage, refreshed }; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] No tests for the Add tests following the — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0660660. Added 6 handler tests following the |
||||||||||||||||||||||||||||||||||||||||||||
| case SERVE_CONTROL_EXT_METHODS.sessionRuntimeContext: { | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Two concerns: 1. No test coverage. The 2. Silent validation failures. Non-string values are silently skipped ( — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d594328. The handler now returns
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] No tests for the The handler has 5+ branches (invalid sessionId, invalid entries type, non-string value rejection, empty-value removal, set-or-reject) but Suggested: add a — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 — GPT-5 via Qwen Code /review
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
| entries as Record<string, unknown>, | ||||||||||||||||||||||||||||||||||||||||||||
| )) { | ||||||||||||||||||||||||||||||||||||||||||||
| if (typeof value !== 'string') { | ||||||||||||||||||||||||||||||||||||||||||||
| rejected.push({ key, reason: 'value_not_string' }); | ||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if (value === '') { | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Nice to have] The removal path doesn't validate the key against — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d594328. The removal path now checks |
||||||||||||||||||||||||||||||||||||||||||||
| if (config.getRuntimeContext().has(key)) { | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Silent no-op removal. When Suggested: either always push to if (value === '') {
config.removeRuntimeContextEntry(key);
appliedKeys.push(key); // idempotent: always report as applied
} |
||||||||||||||||||||||||||||||||||||||||||||
| config.removeRuntimeContextEntry(key); | ||||||||||||||||||||||||||||||||||||||||||||
| appliedKeys.push(key); | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] When Consider idempotent delete semantics — always push to
Suggested change
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 2dc0034 — the removal path now checks |
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] When
Suggested change
— 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)) { | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Duplicated validation logic. The rejection-reason chain re-inlines Consider having — qwen3.7-max via Qwen Code /review
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The key regex The root cause is that // 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (
Suggested change
This eliminates the duplicated literals and makes the rejection reason authoritative from Config.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rejection-reason classification re-implements Config's validation constants ( |
||||||||||||||||||||||||||||||||||||||||||||
| rejected.push({ key, reason: 'invalid_key' }); | ||||||||||||||||||||||||||||||||||||||||||||
| } else if ( | ||||||||||||||||||||||||||||||||||||||||||||
| Buffer.byteLength(value, 'utf8') > 32 * 1024 | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Key regex Consider exporting the constants from Config or adding a — qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||
| rejected.push({ key, reason: 'value_too_large' }); | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The The reason-mapping logic here re-derives the failure cause by re-running the same regex ( Two suggested tests:
Longer-term, consider having — qwen3.7-max via Qwen Code /review
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Untested rejection branches. The 6 handler tests cover If the rejection classification logic has a bug (wrong reason string, misordered checks), it silently returns incorrect 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 { | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Consider computing the specific reason:
Suggested change
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4af5086 — the handler now returns granular rejection reasons:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Three rejection branches lack test coverage: Suggested tests:
— qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||||||||||||||||||||||||
| rejected.push({ key, reason: 'capacity_full' }); | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Consider having
Suggested change
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 }; | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
— 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. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm-lock.yamllooks 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.