From a42f1ead48302780e7b95be08d3398059accfc93 Mon Sep 17 00:00:00 2001 From: agent_vscode Date: Sun, 12 Jul 2026 05:16:47 +0300 Subject: [PATCH] test(ai-chat): align #332 deferred-tool test with #490 cross-turn persistence The #332 integration test (ai-chat-stream.int-spec.ts) asserted a deferred tool activated in one turn does NOT leak into the next ("cold start per turn"). #490 (f5bbfdb2) deliberately reversed that: it persists the activation set into chat metadata.activatedTools and seeds the next turn from it, so the model need not re-run loadTools. The test only surfaced now because the token-estimate CI fix let the integration step run again. Adopt #490 persistence as the intended behavior: - flip the turn-2 assertion to expect createPage IS active on the fresh turn's first step (seeded from metadata); keep turn-1 cold-start assertions. - rewrite the test docstring, describe/it titles and comments accordingly. - fix two stale "not persisted / per-turn" comments in ai-chat.service.ts (prepareAgentStep + the streaming-loop activation block); no logic change. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/core/ai-chat/ai-chat.service.ts | 18 +++++---- .../integration/ai-chat-stream.int-spec.ts | 38 ++++++++++--------- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 9922ba5a..7ec93545 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -189,10 +189,11 @@ export function stepBudgetWarning(stepNumber: number): string { // // `system` is the in-scope system prompt; we CONCATENATE so the original // persona/context is preserved — a bare `system` override would REPLACE the -// whole system prompt for the step. `activatedTools` is PER-TURN mutable state -// owned by the streaming loop (a closure Set grown by loadTools); it is passed -// in (not module-global, not persisted) so this stays a pure function of its -// arguments. +// whole system prompt for the step. `activatedTools` is a closure Set grown by +// loadTools and owned by the streaming loop; the caller seeds it from and +// persists it to the chat's metadata across turns (#490), but this function only +// READS the Set it is handed, so it stays a pure function of its arguments (not +// module-global). // // NOTE: at AI SDK v7 the per-step `system` field is renamed to `instructions`. // On v6 (`^6.0.134`) `system` is the correct field — adjust when bumping. @@ -1410,10 +1411,11 @@ export class AiChatService implements OnModuleInit, OnModuleDestroy { const baseTools = { ...external.tools, ...docmostTools }; // Deferred tool loading state (#332), scoped to THIS streaming loop: - // - `activatedTools` is per-TURN mutable state — a fresh closure Set created - // per streamText call, NOT module-global and NOT persisted, so a new turn - // starts cold. loadTools.execute adds to it; prepareAgentStep reads it to - // widen `activeTools` on the NEXT step. + // - `activatedTools` is a fresh closure Set per streamText call (not + // module-global), SEEDED from the chat's persisted metadata.activatedTools + // (#490, just below) so activation carries across turns. loadTools.execute + // adds to it; prepareAgentStep reads it to widen `activeTools` on the NEXT + // step; turn end persists it back. // - `validDeferredNames` = every tool that is NOT core (the in-app deferred // tools + ALL external MCP tools), computed from the ACTUAL toolset so an // external tool is loadable by its namespaced name. loadTools rejects any diff --git a/apps/server/test/integration/ai-chat-stream.int-spec.ts b/apps/server/test/integration/ai-chat-stream.int-spec.ts index bd2a8098..0ddbfdcc 100644 --- a/apps/server/test/integration/ai-chat-stream.int-spec.ts +++ b/apps/server/test/integration/ai-chat-stream.int-spec.ts @@ -322,20 +322,21 @@ describe('AiChatService.stream [integration]', () => { }); /** - * #332 deferred tool loading, the ON path. The riskiest property is that the - * per-turn `activatedTools` Set is created FRESH inside each stream() call, so a - * tool a previous turn activated via loadTools is NOT still active when the next - * turn starts — the new turn begins "cold" (CORE + loadTools only). The unit - * tests only exercise pure prepareAgentStep with hand-fed Sets; this pins the - * real wiring end-to-end (loadTools.execute -> activatedTools -> prepareStep -> - * per-step activeTools) against the real streamText loop, and proves there is no - * cross-turn leak. We drive a MockLanguageModelV3 whose step 1 calls - * loadTools(['createPage']) and assert, via the model's recorded per-step - * CallOptions.tools (the AI SDK filters the provider tool list by activeTools), - * that the deferred tool becomes active on the SAME turn's next step but NOT on a - * fresh turn's first step. + * #332 + #490 deferred tool loading, the ON path. Turn 1 starts COLD (CORE + + * loadTools only) and activates a deferred tool via loadTools; that activation + * is PERSISTED into the chat's metadata.activatedTools (#490) so the NEXT turn + * SEEDS from it and the tool is active from the fresh turn's FIRST step — the + * model never re-runs loadTools to re-activate the same tool. The unit tests + * only exercise pure prepareAgentStep with hand-fed Sets; this pins the real + * wiring end-to-end (loadTools.execute -> activatedTools -> persist -> next-turn + * seed -> prepareStep -> per-step activeTools) against the real streamText loop. + * We drive a MockLanguageModelV3 whose step 1 calls loadTools(['createPage']) + * and assert, via the model's recorded per-step CallOptions.tools (the AI SDK + * filters the provider tool list by activeTools), that the deferred tool becomes + * active on the SAME turn's next step AND, seeded from metadata, on the next + * turn's first step. */ - describe('deferred tool loading ON — per-turn activation, no leak (#332)', () => { + describe('deferred tool loading ON — cross-turn activation persistence (#332 + #490)', () => { // A stub deferred (non-core) tool the agent can activate. Its execute is never // called — the model only needs to SEE it become active — but it must be a // valid AI-SDK tool so the SDK includes it in a step's tool list once active. @@ -451,7 +452,7 @@ describe('AiChatService.stream [integration]', () => { } as any); } - it('activates a deferred tool for the SAME turn, and a NEW turn starts cold (no leak)', async () => { + it('activates a deferred tool for the SAME turn, and a NEW turn SEEDS it from persisted chat metadata (#490)', async () => { const chatId = (await createChat(db, { workspaceId, creatorId: userId })).id; // --- Turn 1: loadTools(createPage) on step 1, then answer on step 2. --- @@ -474,7 +475,7 @@ describe('AiChatService.stream [integration]', () => { // Step 2 of the SAME turn sees the just-activated deferred tool. expect(step2Tools).toContain('createPage'); - // --- Turn 2 on the SAME chat: must start cold again. --- + // --- Turn 2 on the SAME chat: seeds the persisted activation (#490). --- const model2 = new MockLanguageModelV3({ doStream: async () => ({ stream: successStream() }), } as any); @@ -485,9 +486,10 @@ describe('AiChatService.stream [integration]', () => { const nextTurnFirstStep = toolNames(model2.doStreamCalls[0]); expect(nextTurnFirstStep).toContain('loadTools'); - // The activated set is per-turn: the prior turn's createPage did NOT leak, - // so the fresh turn's first step sees it deferred again. - expect(nextTurnFirstStep).not.toContain('createPage'); + // #490: activation PERSISTS across turns — turn 1 wrote createPage into the + // chat's metadata.activatedTools, so the next turn seeds from it and the + // deferred tool is active from the FIRST step (no need to re-run loadTools). + expect(nextTurnFirstStep).toContain('createPage'); }); }); });