diff --git a/.env.example b/.env.example index f5cf5c63..1241ab18 100644 --- a/.env.example +++ b/.env.example @@ -217,6 +217,17 @@ MCP_DOCMOST_PASSWORD= # active" behavior. # AI_CHAT_DEFERRED_TOOLS=true +# Final-step lockdown for the in-app agent loop (#444). Default OFF. When ON +# (legacy), the LAST allowed step forces a text-only answer: the model's tools are +# stripped (toolChoice=none) and a synthesis instruction is appended. That +# tool-stripping caused a token-degeneration incident — robbed of its tools on the +# final step mid-work, the model emitted a ~255KB block repeating a single token — +# so the default is now OFF: the last step keeps its tools and gets only a SOFT +# nudge to finish with a text summary, and a token-degeneration detector is the +# universal anti-babble guard. Enable this ONLY for a model that reliably ends its +# turns with a clear text answer. +# AI_CHAT_FINAL_STEP_LOCKDOWN=false + # --- Autonomous / detached agent runs (settings.ai.autonomousRuns) --- # Opt-in per workspace (AI settings; off by default). When on, a chat turn becomes # a server-side RUN that survives a browser disconnect — only an explicit Stop ends diff --git a/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts b/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts index 4186ff51..02a44627 100644 --- a/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts @@ -3,6 +3,7 @@ import { buildMcpToolingBlock, buildToolCatalogBlock, } from './ai-chat.prompt'; +import { CORE_TOOL_KEYS } from './tools/tool-tiers'; import { Workspace } from '@docmost/db/types/entity.types'; /** @@ -464,6 +465,19 @@ describe('buildToolCatalogBlock (#332)', () => { expect(block).toContain('- transformPage — run a JS transform.'); expect(block).toContain(''); }); + + it('states core tools are always active, listed DYNAMICALLY from CORE_TOOL_KEYS (#444)', () => { + const block = buildToolCatalogBlock(catalog, true); + // The note carries the always-active statement. + expect(block).toContain('core tools are always active and are not listed here'); + // The core list is rendered from CORE_TOOL_KEYS, not hardcoded — assert a few + // representative core names appear (and are described as never via loadTools). + expect(block).toContain('ALWAYS active'); + expect(block).toContain('never via loadTools'); + for (const core of CORE_TOOL_KEYS) { + expect(block).toContain(core); + } + }); }); describe('buildSystemPrompt gating (#332)', () => { diff --git a/apps/server/src/core/ai-chat/ai-chat.prompt.ts b/apps/server/src/core/ai-chat/ai-chat.prompt.ts index 3e7cb876..db48c6fa 100644 --- a/apps/server/src/core/ai-chat/ai-chat.prompt.ts +++ b/apps/server/src/core/ai-chat/ai-chat.prompt.ts @@ -1,6 +1,6 @@ import { Workspace } from '@docmost/db/types/entity.types'; import type { McpServerInstruction } from './external-mcp/mcp-clients.service'; -import type { ToolCatalogEntry } from './tools/tool-tiers'; +import { CORE_TOOL_KEYS, type ToolCatalogEntry } from './tools/tool-tiers'; /** * Default agent persona used when the admin has not configured a custom system @@ -224,8 +224,11 @@ export function buildToolCatalogBlock( .filter((e) => e && typeof e.catalogLine === 'string' && e.catalogLine.trim()) .map((e) => `- ${e.catalogLine.trim()}`); if (lines.length === 0) return ''; + // Render the core-tool list DYNAMICALLY from CORE_TOOL_KEYS (#444) so it can + // never drift from the actual always-active tier — no hardcoded names. + const coreList = [...CORE_TOOL_KEYS].join(', '); return [ - '', + '', 'The tools below EXIST and are available to you, but their full definitions are', 'NOT loaded into this conversation yet. To use one, first call loadTools with', 'the exact name(s) from this catalog; the loaded tools become callable on your', @@ -234,6 +237,7 @@ export function buildToolCatalogBlock( 'task needs a tool that is not among your active tools, find it here, call', 'loadTools, and continue. Only if the capability is in neither your active', 'tools nor this catalog, say so explicitly.', + `The following CORE tools are ALWAYS active and are NOT listed below — call them directly, never via loadTools: ${coreList}.`, 'Deferred tools (name — purpose):', ...lines, '', diff --git a/apps/server/src/core/ai-chat/ai-chat.service.run-race.spec.ts b/apps/server/src/core/ai-chat/ai-chat.service.run-race.spec.ts index 3721076e..a5db3b3f 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.run-race.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.run-race.spec.ts @@ -53,7 +53,7 @@ describe('AiChatService.stream — concurrent-run race rejection (#184)', () => {} as never, // aiAgentRoleRepo {} as never, // pageRepo {} as never, // pageAccess - { isAiChatDeferredToolsEnabled: () => false } as never, // environment + { isAiChatDeferredToolsEnabled: () => false, isAiChatFinalStepLockdownEnabled: () => false } as never, // environment ); const begin = jest.fn(beginImpl); return { svc, begin, aiChatRepo, aiChatMessageRepo }; @@ -173,7 +173,7 @@ describe('AiChatService.stream — abortSignal wiring (#184 F3)', () => { {} as never, // aiAgentRoleRepo {} as never, // pageRepo (openPage undefined -> never touched) {} as never, // pageAccess - { isAiChatDeferredToolsEnabled: () => false } as never, // environment + { isAiChatDeferredToolsEnabled: () => false, isAiChatFinalStepLockdownEnabled: () => false } as never, // environment ); return { svc }; } @@ -199,7 +199,8 @@ describe('AiChatService.stream — abortSignal wiring (#184 F3)', () => { const { svc } = makeService(); const runController = new AbortController(); const runSignal = runController.signal; - const socketSignal = new AbortController().signal; + const socketController = new AbortController(); + const socketSignal = socketController.signal; const begin = jest.fn(async () => ({ runId: 'run-1', signal: runSignal })); await svc.stream({ @@ -223,13 +224,26 @@ describe('AiChatService.stream — abortSignal wiring (#184 F3)', () => { expect(streamTextMock).toHaveBeenCalledTimes(1); // THE assertion: the agent loop's abort is wired to the RUN, so a browser // disconnect (which aborts only `socketSignal`) cannot end the turn. - expect(streamTextMock.mock.calls[0][0].abortSignal).toBe(runSignal); - expect(streamTextMock.mock.calls[0][0].abortSignal).not.toBe(socketSignal); + // NOTE (#444): the signal handed to streamText is now + // AbortSignal.any([effectiveSignal, degenerationController.signal]), so it is + // no longer identity-equal to `runSignal`. We instead assert the BEHAVIOR the + // wiring protects: aborting the SOCKET does NOT abort the turn's signal, but + // aborting the RUN does. + const passed = streamTextMock.mock.calls[0][0].abortSignal as AbortSignal; + expect(passed).not.toBe(socketSignal); + expect(passed.aborted).toBe(false); + socketController.abort?.(); + // A socket abort must not reach a run-wrapped turn. + expect(passed.aborted).toBe(false); + // A run abort must. + runController.abort(); + expect(passed.aborted).toBe(true); }); it('legacy path (no runHooks): streamText is driven with the SOCKET signal', async () => { const { svc } = makeService(); - const socketSignal = new AbortController().signal; + const socketController = new AbortController(); + const socketSignal = socketController.signal; await svc.stream({ user: { id: 'user-1' } as never, @@ -244,7 +258,12 @@ describe('AiChatService.stream — abortSignal wiring (#184 F3)', () => { }); expect(streamTextMock).toHaveBeenCalledTimes(1); - expect(streamTextMock.mock.calls[0][0].abortSignal).toBe(socketSignal); + // #444: the passed signal is AbortSignal.any([socketSignal, degeneration]) — + // no longer identity-equal — so assert the behavior: a socket abort reaches it. + const passed = streamTextMock.mock.calls[0][0].abortSignal as AbortSignal; + expect(passed.aborted).toBe(false); + socketController.abort(); + expect(passed.aborted).toBe(true); }); /** @@ -414,7 +433,7 @@ describe('AiChatService.stream — begin-failure resilience / legacy fallback (# {} as never, // aiAgentRoleRepo {} as never, // pageRepo {} as never, // pageAccess - { isAiChatDeferredToolsEnabled: () => false } as never, // environment + { isAiChatDeferredToolsEnabled: () => false, isAiChatFinalStepLockdownEnabled: () => false } as never, // environment ); return { svc, aiChatMessageRepo }; } @@ -442,7 +461,8 @@ describe('AiChatService.stream — begin-failure resilience / legacy fallback (# .mockImplementation(() => undefined as never); const { svc, aiChatMessageRepo } = makeService(); - const socketSignal = new AbortController().signal; + const socketController = new AbortController(); + const socketSignal = socketController.signal; // A transient, NON-race begin failure (e.g. a non-unique DB error inserting // the run row). This is the `else` branch of the begin try/catch. @@ -483,7 +503,12 @@ describe('AiChatService.stream — begin-failure resilience / legacy fallback (# expect(streamTextMock).toHaveBeenCalledTimes(1); // The decisive wiring: with no run handle, the fallback uses the SOCKET signal - // (effectiveSignal = signal, runId undefined) — not a run-bound signal. - expect(streamTextMock.mock.calls[0][0].abortSignal).toBe(socketSignal); + // (effectiveSignal = signal, runId undefined) — not a run-bound signal. #444: + // the signal is unioned with the degeneration controller via AbortSignal.any, + // so assert the socket abort still reaches the turn rather than identity. + const passed = streamTextMock.mock.calls[0][0].abortSignal as AbortSignal; + expect(passed.aborted).toBe(false); + socketController.abort(); + expect(passed.aborted).toBe(true); }); }); diff --git a/apps/server/src/core/ai-chat/ai-chat.service.setup-abort.spec.ts b/apps/server/src/core/ai-chat/ai-chat.service.setup-abort.spec.ts index c04057c9..5ed7e34b 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.setup-abort.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.setup-abort.spec.ts @@ -52,7 +52,7 @@ describe('AiChatService.stream — abort during external-MCP setup finalizes the {} as never, // aiAgentRoleRepo {} as never, // pageRepo (openPage undefined -> never touched) {} as never, // pageAccess - { isAiChatDeferredToolsEnabled: () => false } as never, // environment + { isAiChatDeferredToolsEnabled: () => false, isAiChatFinalStepLockdownEnabled: () => false } as never, // environment ); return { svc, tools }; } diff --git a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts index 945ebccd..883d8edd 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts @@ -15,6 +15,7 @@ import { serializeSteps, rowToUiMessage, prepareAgentStep, + stepBudgetWarning, flushAssistant, stripNulChars, chatStreamMetadata, @@ -22,7 +23,9 @@ import { isInterruptResume, sameInstant, MAX_AGENT_STEPS, + STEP_BUDGET_WARNING_LEAD, FINAL_STEP_INSTRUCTION, + FINAL_STEP_NUDGE, } from './ai-chat.service'; import type { AiChatMessage, Workspace } from '@docmost/db/types/entity.types'; import { buildSystemPrompt } from './ai-chat.prompt'; @@ -311,43 +314,67 @@ describe('rowToUiMessage', () => { /** * Unit tests for prepareAgentStep: the pure helper that decides per-step - * overrides for the agent loop. Early steps return undefined (default - * behavior); the final allowed step (stepNumber === MAX_AGENT_STEPS - 1) forces - * a text-only synthesis answer (toolChoice 'none') with the FINAL_STEP_INSTRUCTION - * appended onto — not replacing — the original system prompt. + * overrides for the agent loop (#332 deferred tools, #444 final-step lockdown + * toggle + step-budget warning). Parametrized by the two toggles so a change to + * one path cannot silently mask a regression in the other. + * + * Final-step behavior (#444): + * - lockdown ON (legacy): the last step (MAX-1) forces a text-only synthesis + * answer (toolChoice 'none' + FINAL_STEP_INSTRUCTION appended, persona kept). + * - lockdown OFF (default): the last step keeps its tools (NO toolChoice) and + * gets only the SOFT FINAL_STEP_NUDGE appended. */ // Narrowing helpers for the prepareAgentStep union return type. const asLockdown = (r: ReturnType) => r as { toolChoice: 'none'; system: string }; const asActive = (r: ReturnType) => - r as { activeTools: string[] }; + r as { activeTools: string[]; system?: string }; +const asSystemOnly = (r: ReturnType) => + r as { system: string }; describe('prepareAgentStep', () => { - // --- toggle OFF (default): unchanged behavior --- - it('returns undefined for the first step (toggle off)', () => { + // --- deferred OFF, lockdown OFF (the new default) --- + it('returns undefined for the first step (both toggles off)', () => { expect(prepareAgentStep(0, 'SYS')).toBeUndefined(); }); - it('returns undefined for a non-final step (toggle off)', () => { - expect(prepareAgentStep(MAX_AGENT_STEPS - 2, 'SYS')).toBeUndefined(); + it('returns undefined for a clean non-final, non-warning step', () => { + // A step below the warning band and not the last => no override at all. + expect(prepareAgentStep(MAX_AGENT_STEPS - 10, 'SYS')).toBeUndefined(); }); - it('forces a text-only synthesis on the final allowed step (toggle off)', () => { - const result = asLockdown(prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS')); + it('final step (lockdown OFF) keeps tools and appends only the SOFT nudge', () => { + const result = asSystemOnly(prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS')); expect(result).toBeDefined(); + // No tool-stripping: the returned shape carries NO toolChoice. + expect( + (result as unknown as { toolChoice?: string }).toolChoice, + ).toBeUndefined(); + expect(result.system.startsWith('SYS')).toBe(true); + expect(result.system).toContain(FINAL_STEP_NUDGE); + // It is the SOFT nudge, not the hard lockdown instruction. + expect(result.system).not.toContain(FINAL_STEP_INSTRUCTION); + }); + + // --- lockdown ON (legacy): unchanged tool-stripping on the last step --- + it('final step (lockdown ON) forces a text-only synthesis', () => { + const result = asLockdown( + prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS', [], false, true), + ); expect(result.toolChoice).toBe('none'); // The original persona is preserved (prefix), not replaced. expect(result.system.startsWith('SYS')).toBe(true); - // The synthesis instruction is appended. + // The synthesis instruction is appended (NOT the soft nudge). expect(result.system).toContain(FINAL_STEP_INSTRUCTION); + expect(result.system).not.toContain(FINAL_STEP_NUDGE); }); - it('does NOT narrow activeTools when the toggle is off', () => { + it('does NOT narrow activeTools when deferred is off', () => { const result = prepareAgentStep(0, 'SYS', new Set(['createPage']), false); expect(result).toBeUndefined(); }); - // --- toggle ON (#332): deferred tool visibility --- + // --- deferred ON (#332): deferred tool visibility --- it('a non-final step exposes CORE + loadTools + activatedTools', () => { const activated = new Set(); const result = asActive(prepareAgentStep(0, 'SYS', activated, true)); @@ -358,6 +385,8 @@ describe('prepareAgentStep', () => { // No deferred tool is active before it is loaded. expect(result.activeTools).not.toContain('createPage'); expect(result.activeTools).not.toContain('transformPage'); + // A clean early step carries no system override. + expect(result.system).toBeUndefined(); }); it('adding a name to activatedTools makes it appear on the next step', () => { @@ -380,14 +409,90 @@ describe('prepareAgentStep', () => { expect(result.activeTools).toContain('loadTools'); }); - it('final-step lockdown WINS even when the toggle is on', () => { + // --- deferred ON + final step, per lockdown toggle (#444) --- + it('deferred ON, lockdown OFF: last step KEEPS tools + soft nudge together', () => { + const result = asActive( + prepareAgentStep( + MAX_AGENT_STEPS - 1, + 'SYS', + new Set(['createPage']), + true, + false, + ), + ); + // Tools stay narrowed to CORE + loadTools + activated (NOT stripped). + expect(result.activeTools).toContain('editPageText'); + expect(result.activeTools).toContain('loadTools'); + expect(result.activeTools).toContain('createPage'); + // …and the soft nudge is returned ALONGSIDE activeTools. + expect(result.system).toContain(FINAL_STEP_NUDGE); + expect( + (result as unknown as { toolChoice?: string }).toolChoice, + ).toBeUndefined(); + }); + + it('deferred ON, lockdown ON: lockdown WINS (tools stripped)', () => { const result = asLockdown( - prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS', new Set(['createPage']), true), + prepareAgentStep( + MAX_AGENT_STEPS - 1, + 'SYS', + new Set(['createPage']), + true, + true, + ), ); // The lockdown shape (toolChoice none + synthesis) — not the activeTools shape. expect(result.toolChoice).toBe('none'); expect(result.system).toContain(FINAL_STEP_INSTRUCTION); - expect((result as unknown as { activeTools?: string[] }).activeTools).toBeUndefined(); + expect( + (result as unknown as { activeTools?: string[] }).activeTools, + ).toBeUndefined(); + }); +}); + +/** + * Step-budget warning boundaries (#444). At MAX_AGENT_STEPS=50 the warning fires + * on steps MAX-6 .. MAX-2 (44..48) with a decreasing remaining-count, is CLEAN + * below the band (0..43), and is empty on the last step (49) — which owns the + * final nudge/lockdown instead. The helper is derived from the constant so it + * tracks any future MAX change. + */ +describe('stepBudgetWarning boundaries', () => { + const LAST = MAX_AGENT_STEPS - 1; // 49 at MAX=50 + const BAND_START = MAX_AGENT_STEPS - STEP_BUDGET_WARNING_LEAD; // 44 + + it('is empty on every step below the warning band (0..BAND_START-1)', () => { + for (let s = 0; s < BAND_START; s++) { + expect(stepBudgetWarning(s)).toBe(''); + } + }); + + it('fires on BAND_START..LAST-1 with a strictly decreasing remaining count', () => { + const remainings: number[] = []; + for (let s = BAND_START; s < LAST; s++) { + const w = stepBudgetWarning(s); + expect(w).toContain('tool-use steps remain'); + const m = w.match(/Only (\d+) tool-use steps remain/); + expect(m).not.toBeNull(); + remainings.push(Number(m![1])); + } + // Exactly STEP_BUDGET_WARNING_LEAD-1 warning steps (44..48). + expect(remainings).toHaveLength(STEP_BUDGET_WARNING_LEAD - 1); + // Remaining = MAX-1-step, so it decreases by 1 each step and ends at 1. + for (let i = 1; i < remainings.length; i++) { + expect(remainings[i]).toBe(remainings[i - 1] - 1); + } + expect(remainings[remainings.length - 1]).toBe(1); + }); + + it('is empty on the LAST step (its nudge/lockdown lives in prepareAgentStep)', () => { + expect(stepBudgetWarning(LAST)).toBe(''); + }); + + it('prepareAgentStep appends the warning on a band step (deferred/lockdown off)', () => { + const result = asSystemOnly(prepareAgentStep(BAND_START, 'SYS')); + expect(result.system).toContain('Stop exploring and start acting now'); + expect(result.system).not.toContain(FINAL_STEP_NUDGE); }); }); @@ -1341,6 +1446,7 @@ describe('AiChatService.stream — resumable pipe options (#184 phase 1.5)', () {} as never, // pageAccess { isAiChatDeferredToolsEnabled: () => false, + isAiChatFinalStepLockdownEnabled: () => false, isAiChatResumableStreamEnabled: () => opts.resumable, } as never, streamRegistry as never, 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 71d3071e..f4dbb31d 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -52,11 +52,24 @@ import { startSseHeartbeat, stripStreamingHopByHopHeaders, } from './sse-resilience'; +import { + isDegenerateOutput, + truncateDegeneratedTail, +} from './output-degeneration'; // Max agent steps per turn. One step = one model generation; a step that calls // tools is followed by another step carrying the tool results. Raised from 8 so -// multi-search research questions are not cut off mid-investigation. -const MAX_AGENT_STEPS = 20; +// multi-search research questions are not cut off mid-investigation, then from 20 +// to 50 (#444) so read-heavy turns (e.g. dozens of searchInPage sweeps) do not +// exhaust the budget before acting. +const MAX_AGENT_STEPS = 50; + +// How many steps before the LAST one the step-budget warning starts firing +// (#444). At MAX-STEP_BUDGET_WARNING_LEAD .. MAX-2 the model is told to stop +// exploring and start acting, with the remaining count decreasing each step; the +// last step (MAX-1) has its own final nudge / lockdown instead (see +// prepareAgentStep). +const STEP_BUDGET_WARNING_LEAD = 6; // Wall-clock ceiling for building the external MCP toolset during the per-turn // setup phase (before streamText owns the lifecycle). Defense-in-depth ABOVE the @@ -82,16 +95,69 @@ const FINAL_STEP_INSTRUCTION = 'language. If the information is incomplete, say so explicitly: summarize ' + 'what you found, what is still missing, and give your best partial conclusion.'; -// Pure, unit-testable: decide per-step overrides. Two responsibilities: -// 1. Final-step lockdown (always): on the final allowed step force a text-only -// synthesis answer (toolChoice 'none' + FINAL_STEP_INSTRUCTION). This WINS — -// it takes precedence over the deferred-tool narrowing below. -// 2. Deferred tool visibility (#332): when `deferredEnabled` and NOT the final -// step, expose only the CORE tools + loadTools + whatever loadTools has -// activated so far this turn (`activatedTools`), via `activeTools`. Deferred -// tools stay in the until the model loads them. -// When `deferredEnabled` is false the behavior is unchanged: undefined on normal -// steps (all tools active), lockdown on the final step. +// SOFT final-step nudge (#444), used when the final-step lockdown toggle is OFF +// (the new default). Unlike FINAL_STEP_INSTRUCTION it does NOT strip tools +// (toolChoice stays untouched), so the model is never forced into a tool-less +// state mid-work — that tool-stripping is what triggered the 255KB token-loop +// degeneration incident. It only asks the model to finish with a text summary. +const FINAL_STEP_NUDGE = + 'This is the LAST step of this turn. Write your final answer to the user now.\n' + + 'You may still call tools, but the turn ends after this step either way —\n' + + 'prefer finishing with a clear text summary of what was done and what remains.'; + +// Synthetic marker text appended in onFinish when a step-exhausted turn produced +// NO text at all (#444, mitigates the "empty turn" the lockdown used to prevent +// when the toggle is OFF). Makes the exhausted-without-answer state explicit to +// the user and, on replay, to the model on the next turn. +const STEP_LIMIT_NO_ANSWER_MARKER = + '(Достигнут лимит шагов — итоговый ответ не сформулирован; работа могла ' + + 'остаться незавершённой. Напишите «продолжай», чтобы агент продолжил.)'; + +// Reason recorded in ai_chat_runs.error / the assistant row when the token- +// degeneration detector (#444) aborts a run. Distinct from a user Stop (no error) +// and from a server restart ('streaming' -> swept to 'aborted' with no message). +const OUTPUT_DEGENERATION_ERROR = + 'Output degeneration detected (repeated token loop)'; + +/** + * Compute the step-budget warning text (#444), or '' when this step is outside + * the warning band. The warning fires on steps + * MAX_AGENT_STEPS-STEP_BUDGET_WARNING_LEAD .. MAX_AGENT_STEPS-2 (NOT the last + * step, which has its own final nudge/lockdown), telling the model to stop + * exploring and start acting. `N` is the number of tool-use steps still + * remaining (`MAX_AGENT_STEPS - 1 - stepNumber`), so it decreases toward the + * end. Pure. + */ +export function stepBudgetWarning(stepNumber: number): string { + const isLastStep = stepNumber >= MAX_AGENT_STEPS - 1; + const inBand = stepNumber >= MAX_AGENT_STEPS - STEP_BUDGET_WARNING_LEAD; + if (isLastStep || !inBand) return ''; + const remaining = MAX_AGENT_STEPS - 1 - stepNumber; + return ( + `Only ${remaining} tool-use steps remain in this turn. Stop exploring and start acting now\n` + + '(make the edits / create the comments / produce results). Leave room to finish\n' + + 'with a final text answer.' + ); +} + +// Pure, unit-testable: decide per-step overrides. Responsibilities: +// 1. Final-step handling. Two modes, chosen by `finalStepLockdownEnabled`: +// - toggle ON (legacy): on the final allowed step force a text-only +// synthesis answer (toolChoice 'none' + FINAL_STEP_INSTRUCTION). This WINS +// — it takes precedence over the deferred-tool narrowing below. +// - toggle OFF (new default, #444): do NOT touch toolChoice — tools stay +// available on every step incl. the last, so the model is never stripped +// of its tools mid-work (the cause of the token-loop degeneration +// incident). A SOFT nudge (FINAL_STEP_NUDGE) is appended to `system`, and +// the deferred-tool `activeTools` narrowing still applies to the last step +// (both `activeTools` and `system` are returned together). +// 2. Step-budget warning (#444): on steps in the warning band (but not the +// last, which has its own nudge/lockdown) append stepBudgetWarning(...) to +// `system` so the model starts acting before it runs out of steps. +// 3. Deferred tool visibility (#332): when `deferredEnabled`, expose only the +// CORE tools + loadTools + whatever loadTools has activated so far this turn +// (`activatedTools`), via `activeTools`. Deferred tools stay in the +// until the model loads them. // // `system` is the in-scope system prompt; we CONCATENATE so the original // persona/context is preserved — a bare `system` override would REPLACE the @@ -107,31 +173,53 @@ export function prepareAgentStep( system: string, activatedTools: ReadonlySet | readonly string[] = [], deferredEnabled = false, + finalStepLockdownEnabled = false, ): | { toolChoice: 'none'; system: string } - | { activeTools: string[] } + | { activeTools: string[]; system?: string } + | { system: string } | undefined { - // Final-step lockdown WINS (applies regardless of the deferred toggle). - if (stepNumber >= MAX_AGENT_STEPS - 1) { + const isLastStep = stepNumber >= MAX_AGENT_STEPS - 1; + + // Legacy final-step lockdown (toggle ON): text-only synthesis. WINS over the + // deferred narrowing AND drops tools for this step. + if (isLastStep && finalStepLockdownEnabled) { return { toolChoice: 'none', system: `${system}\n\n${FINAL_STEP_INSTRUCTION}`, }; } - // Deferred tool loading: narrow this step's visible tools to CORE + loadTools - // + the tools already activated this turn. + + // Compute the extra system text for this step: the soft final nudge on the last + // step (toggle OFF), or the step-budget warning in the warning band. At most one + // of these applies (stepBudgetWarning returns '' on the last step). + const extra = isLastStep ? FINAL_STEP_NUDGE : stepBudgetWarning(stepNumber); + const systemForStep = extra ? `${system}\n\n${extra}` : undefined; + + // Deferred tool loading: narrow this step's visible tools to CORE + loadTools + + // the tools already activated this turn. Applies on EVERY step incl. the last + // (toggle OFF), so the model keeps its core tools available while being nudged + // to finish. Return `system` alongside `activeTools` when we have extra text. if (deferredEnabled) { const activated = Array.isArray(activatedTools) ? activatedTools : [...activatedTools]; - return { - activeTools: [...CORE_TOOL_KEYS, LOAD_TOOLS_NAME, ...activated], - }; + const activeTools = [...CORE_TOOL_KEYS, LOAD_TOOLS_NAME, ...activated]; + return systemForStep ? { activeTools, system: systemForStep } : { activeTools }; } - return undefined; + + // Deferred OFF: all tools stay active; only append the extra system text (if any). + return systemForStep ? { system: systemForStep } : undefined; } -export { MAX_AGENT_STEPS, FINAL_STEP_INSTRUCTION }; +export { + MAX_AGENT_STEPS, + STEP_BUDGET_WARNING_LEAD, + FINAL_STEP_INSTRUCTION, + FINAL_STEP_NUDGE, + STEP_LIMIT_NO_ANSWER_MARKER, + OUTPUT_DEGENERATION_ERROR, +}; // Pure, unit-testable post-processing for a model-generated title (#199): trim // whitespace, strip a single pair of surrounding quotes the model often adds, @@ -890,6 +978,12 @@ export class AiChatService implements OnModuleInit { // tools (fat/rare in-app tools + ALL external MCP tools) load on demand. When // OFF, every tool is active and nothing below changes. const deferredEnabled = this.environment.isAiChatDeferredToolsEnabled(); + // Final-step lockdown toggle (#444). Default OFF: the last step keeps its + // tools and gets only a soft nudge (prepareAgentStep), and the token- + // degeneration detector (onChunk below) is the anti-babble guard. ON = + // legacy tool-stripping lockdown on the last step. + const finalStepLockdownEnabled = + this.environment.isAiChatFinalStepLockdownEnabled(); let system: string; let docmostTools: Awaited>; @@ -978,6 +1072,16 @@ export class AiChatService implements OnModuleInit { const capturedSteps: StepLike[] = []; let inProgressText = ''; + // Token-degeneration guard (#444). When the final-step lockdown is OFF, a + // runaway repetition loop (the 255KB "loadTools." incident) is aborted via + // this internal controller, unioned with the run/socket signal below. The + // detector runs on `inProgressText` in onChunk, throttled by growth so the + // pure rules only fire every ~DEGENERATION_CHECK_STEP bytes. + const degenerationController = new AbortController(); + let degenerationDetected = false; + let lastDegenerationCheckLen = 0; + const DEGENERATION_CHECK_STEP = 2000; + // Step-granular durability (#183): create the assistant row UPFRONT in the // 'streaming' state (before any token), then UPDATE it as each step finishes // and finalize it once on the terminal callback. If the process dies @@ -1118,11 +1222,21 @@ export class AiChatService implements OnModuleInit { // further tool calls and appends a synthesis instruction on that step, // concatenated onto the original `system` so the persona is preserved. prepareStep: ({ stepNumber }) => - prepareAgentStep(stepNumber, system, activatedTools, deferredEnabled), + prepareAgentStep( + stepNumber, + system, + activatedTools, + deferredEnabled, + finalStepLockdownEnabled, + ), // #184: the RUN's signal (explicit-stop) when a run wraps this turn, else // the socket-bound signal (legacy). A browser disconnect aborts only in - // the legacy path. - abortSignal: effectiveSignal, + // the legacy path. #444: UNION it with the internal degeneration signal + // so a detected token-loop aborts the run too (AbortSignal.any — Node 20.3+). + abortSignal: AbortSignal.any([ + effectiveSignal, + degenerationController.signal, + ]), onChunk: ({ chunk }) => { // DIAGNOSTIC (Safari stream-drop investigation) — temporary. Any model // output chunk means the stream is actively emitting bytes; track first @@ -1132,7 +1246,29 @@ export class AiChatService implements OnModuleInit { lastModelChunkAt = now; // 'text-delta' is the assistant's prose; tool-call args are separate chunk // types — so this mirrors exactly what streams to the client. - if (chunk.type === 'text-delta') inProgressText += chunk.text; + if (chunk.type === 'text-delta') { + inProgressText += chunk.text; + // Token-degeneration guard (#444). Throttled: only re-run the pure + // rules once the text has grown ~DEGENERATION_CHECK_STEP bytes since + // the last check, so the tail heuristics cost is amortized. On a + // trigger, abort the run ONCE with a distinguishable reason. + if ( + !degenerationDetected && + inProgressText.length - lastDegenerationCheckLen >= + DEGENERATION_CHECK_STEP + ) { + lastDegenerationCheckLen = inProgressText.length; + if (isDegenerateOutput(inProgressText)) { + degenerationDetected = true; + this.logger.warn( + `AI chat stream aborted (chat ${chatId}): ${OUTPUT_DEGENERATION_ERROR}`, + ); + degenerationController.abort( + new Error(OUTPUT_DEGENERATION_ERROR), + ); + } + } + } }, onStepFinish: (step) => { // The finished step's full text is now in `step.text`; fold it in and reset @@ -1174,8 +1310,22 @@ export class AiChatService implements OnModuleInit { // plain-text projection (full-text search / fallback). A multi-step // turn's `content` therefore now holds all steps' prose, not just the // last block. + // Empty-turn mitigation (#444, toggle OFF). If the turn burned all its + // steps WITHOUT ever producing text (every step's text is empty) and + // the model stopped because it hit the step cap, there is no answer to + // show — the lockdown used to force one. Append a synthetic marker as + // the trailing text so the exhausted-without-answer state is explicit + // to the user and, on replay, to the model next turn. `flushAssistant` + // takes this as the `inProgressText` trailing text arg (empty here + // otherwise). `stepCountIs(MAX_AGENT_STEPS)` surfaces as + // finishReason === 'tool-calls' (or a length/other cap), so we key off + // "no text produced" rather than a single finishReason string. + const producedText = (steps as StepLike[]).some((s) => s.text?.trim()); + const stepExhausted = steps.length >= MAX_AGENT_STEPS; + const emptyTurnMarker = + !producedText && stepExhausted ? STEP_LIMIT_NO_ANSWER_MARKER : ''; await finalizeAssistant( - flushAssistant(steps as StepLike[], '', 'completed', { + flushAssistant(steps as StepLike[], emptyTurnMarker, 'completed', { finishReason: finishReason as string, usage: totalUsage as StreamUsage, contextTokens: @@ -1252,6 +1402,30 @@ export class AiChatService implements OnModuleInit { await snapshotTurnEnd(); }, onAbort: async ({ steps }) => { + // #444: distinguish a degeneration abort (our internal controller) from + // a user Stop / disconnect. On degeneration we truncate the runaway tail + // before persist (so hundreds of KB of garbage never reach the DB / + // replay) and record it as an ERROR with a clear, distinguishable reason + // — NOT a bare 'aborted' (a user Stop) and NOT a swept 'streaming' (a + // server restart). + if (degenerationDetected) { + const truncated = truncateDegeneratedTail(inProgressText); + await finalizeAssistant( + flushAssistant(capturedSteps, truncated, 'error', { + error: OUTPUT_DEGENERATION_ERROR, + pageChanged, + }), + ); + if (runId) + await runHooks?.onSettled?.( + runId, + 'error', + OUTPUT_DEGENERATION_ERROR, + ); + await closeExternalClients(); + await snapshotTurnEnd(); + return; + } const partialChars = capturedSteps.reduce((n, s) => n + (s.text?.length ?? 0), 0) + inProgressText.length; diff --git a/apps/server/src/core/ai-chat/output-degeneration.spec.ts b/apps/server/src/core/ai-chat/output-degeneration.spec.ts new file mode 100644 index 00000000..b44fea16 --- /dev/null +++ b/apps/server/src/core/ai-chat/output-degeneration.spec.ts @@ -0,0 +1,165 @@ +import { + hasRepeatedLineRun, + hasPeriodicTail, + isDegenerateOutput, + truncateDegeneratedTail, + REPEATED_LINES_THRESHOLD, + MIN_PERIOD_REPEATS, +} from './output-degeneration'; + +/** + * Unit tests for the token-degeneration detector (#444) — the sole anti-babble + * guard once the final-step lockdown is OFF. The two rules must fire on real + * degeneration (the "loadTools." incident, a no-newline repeat) and MUST NOT fire + * on legitimate long output (edit lists, tables, code). + */ +describe('hasRepeatedLineRun (rule 1: identical-line run)', () => { + it('POSITIVE: fires on "loadTools.\\n" repeated many times (the incident)', () => { + const text = 'Here is my plan.\n' + 'loadTools.\n'.repeat(300); + expect(hasRepeatedLineRun(text)).toBe(true); + expect(isDegenerateOutput(text)).toBe(true); + }); + + it('POSITIVE: fires at exactly the threshold', () => { + const text = 'x\n'.repeat(REPEATED_LINES_THRESHOLD); + expect(hasRepeatedLineRun(text)).toBe(true); + }); + + it('NEGATIVE: does NOT fire just below the threshold', () => { + // threshold-1 identical lines followed by a distinct line. + const text = 'x\n'.repeat(REPEATED_LINES_THRESHOLD - 1) + 'done\n'; + expect(hasRepeatedLineRun(text)).toBe(false); + }); + + it('NEGATIVE: a long edit list of DISTINCT lines never trips', () => { + const lines: string[] = []; + for (let i = 0; i < 200; i++) lines.push(`- edited section ${i}: fixed typo`); + const text = lines.join('\n'); + expect(hasRepeatedLineRun(text)).toBe(false); + expect(isDegenerateOutput(text)).toBe(false); + }); + + it('NEGATIVE: a markdown table with blank separators does not trip', () => { + // Repeated identical rows are unusual, but blank lines break any run. + const block = ['| a | b |', '| - | - |', '', '| a | b |', '']; + const text = Array.from({ length: 60 }, () => block.join('\n')).join('\n'); + expect(hasRepeatedLineRun(text)).toBe(false); + }); + + it('NEGATIVE: blank lines do NOT count toward a run', () => { + const text = '\n'.repeat(100); + expect(hasRepeatedLineRun(text)).toBe(false); + }); +}); + +describe('hasPeriodicTail (rule 2: no-newline suffix periodicity)', () => { + it('POSITIVE: fires on a single char repeated with no newlines', () => { + const text = 'answer: ' + 'a'.repeat(500); + expect(hasPeriodicTail(text)).toBe(true); + expect(isDegenerateOutput(text)).toBe(true); + }); + + it('POSITIVE: fires on a multi-char block repeat with no newlines', () => { + const text = 'prefix ' + 'abcdef'.repeat(100); + expect(hasPeriodicTail(text)).toBe(true); + }); + + it('POSITIVE: at least MIN_PERIOD_REPEATS repeats of a small block', () => { + const text = 'go'.repeat(MIN_PERIOD_REPEATS); + expect(hasPeriodicTail(text)).toBe(true); + }); + + it('NEGATIVE: prose does not look periodic', () => { + const text = + 'The quick brown fox jumps over the lazy dog while the sun sets slowly ' + + 'behind the distant mountains and the river winds through the valley below.'; + expect(hasPeriodicTail(text)).toBe(false); + expect(isDegenerateOutput(text)).toBe(false); + }); + + it('NEGATIVE: a long code block is not flagged', () => { + const code = ` +function compute(values) { + let total = 0; + for (const v of values) { + total += v * 2; + } + return total / values.length; +} +export const helper = (x) => x + 1; +const config = { retries: 3, timeout: 5000, backoff: 'exp' }; +`.repeat(3); + expect(isDegenerateOutput(code)).toBe(false); + }); + + it('NEGATIVE: a short string well under the repeat count is safe', () => { + expect(hasPeriodicTail('ababab')).toBe(false); + }); + + // Regression (#444): a trivial single-char period (p===1) must NOT flag + // legitimate divider/underline/whitespace runs. These are common in real + // model output and previously false-positived at ~20 identical chars, aborting + // the run and truncating output. They must all be treated as clean. + it('NEGATIVE: a markdown horizontal rule is not flagged', () => { + const text = 'text\n' + '-'.repeat(40); + expect(hasPeriodicTail(text)).toBe(false); + expect(isDegenerateOutput(text)).toBe(false); + }); + + it('NEGATIVE: a setext heading underline is not flagged', () => { + const text = 'Title\n' + '='.repeat(30); + expect(hasPeriodicTail(text)).toBe(false); + expect(isDegenerateOutput(text)).toBe(false); + }); + + it('NEGATIVE: a box-drawing divider with no trailing newline is not flagged', () => { + const text = 'done ' + '─'.repeat(50); + expect(hasPeriodicTail(text)).toBe(false); + expect(isDegenerateOutput(text)).toBe(false); + }); + + it('NEGATIVE: trailing spaces are not flagged', () => { + const text = 'answer' + ' '.repeat(40); + expect(hasPeriodicTail(text)).toBe(false); + expect(isDegenerateOutput(text)).toBe(false); + }); + + // Positive counterparts: a GENUINE single-char runaway (hundreds+ of repeats) + // and the real incident (period>=2, "loadTools." ×N) must still fire. + it('POSITIVE: a genuine single-char runaway is still flagged', () => { + const text = 'x'.repeat(5000); + expect(hasPeriodicTail(text)).toBe(true); + expect(isDegenerateOutput(text)).toBe(true); + }); + + it('POSITIVE: the "loadTools." incident (period>=2) is still flagged', () => { + const text = 'loadTools.'.repeat(500); + expect(hasPeriodicTail(text)).toBe(true); + expect(isDegenerateOutput(text)).toBe(true); + }); +}); + +describe('truncateDegeneratedTail', () => { + it('collapses a repeated-line loop to a few reps + marker', () => { + const text = 'plan\n' + 'loadTools.\n'.repeat(20000); + const out = truncateDegeneratedTail(text); + expect(out.length).toBeLessThan(text.length); + expect(out).toContain('output truncated'); + // Keeps the leading context and a few loop reps. + expect(out).toContain('plan'); + expect((out.match(/loadTools\./g) ?? []).length).toBeLessThan(10); + }); + + it('collapses a no-newline periodic loop to a few blocks + marker', () => { + const text = 'answer: ' + 'xy'.repeat(50000); + const out = truncateDegeneratedTail(text); + expect(out.length).toBeLessThan(text.length); + expect(out).toContain('output truncated'); + expect(out).toContain('answer:'); + }); + + it('returns non-degenerate text unchanged (by identity)', () => { + const text = 'A perfectly normal, finished assistant answer.'; + expect(truncateDegeneratedTail(text)).toBe(text); + }); +}); diff --git a/apps/server/src/core/ai-chat/output-degeneration.ts b/apps/server/src/core/ai-chat/output-degeneration.ts new file mode 100644 index 00000000..2834ce7e --- /dev/null +++ b/apps/server/src/core/ai-chat/output-degeneration.ts @@ -0,0 +1,189 @@ +/** + * Token-degeneration detector for the in-app agent stream (#444). + * + * When the final-step lockdown is OFF (the new default) there is no toolChoice + * override to strip the model's tools mid-work, so the anti-babble safety net is + * this detector. It watches the accumulating assistant text and, on a runaway + * repetition loop (the 255KB "loadTools." incident), aborts the run. + * + * Both rules are PURE functions of the text tail so they are cheap to run every + * few KB and are unit-testable in isolation. They operate on the TAIL only + * (`TAIL_WINDOW` chars) so the cost is bounded regardless of how long the turn is. + */ + +/** How many trailing chars of the accumulated text the rules inspect. */ +export const TAIL_WINDOW = 3000; + +/** Rule 1 threshold: minimum consecutive identical non-empty lines to trigger. */ +export const REPEATED_LINES_THRESHOLD = 25; + +/** Rule 2: maximum length of a repeating block considered for periodicity. */ +export const MAX_PERIOD_LEN = 150; + +/** Rule 2: minimum number of consecutive block repeats to trigger. */ +export const MIN_PERIOD_REPEATS = 20; + +/** + * Rule 1 — ≥`REPEATED_LINES_THRESHOLD` consecutive IDENTICAL non-empty lines at + * the tail. Catches the classic newline-delimited loop ("loadTools.\n" ×N). + * Blank lines break a run (a table / list with blank separators never trips it); + * a run of ordinary distinct lines (an edit list, code) never reaches the count. + * + * NB: `REPEATED_LINES_THRESHOLD` (25) is only THIS rule's own trigger, not the + * effective floor for detecting a repeated-line loop. In practice a newline- + * delimited repeat also has a fixed period (line + '\n'), so rule 2 catches it + * via periodicity at `MIN_PERIOD_REPEATS` (20) repeats — the two rules combine + * (see `isDegenerateOutput`), so the effective lower bound for a short identical + * line loop is ~20, not 25. Pure. + */ +export function hasRepeatedLineRun( + text: string, + threshold = REPEATED_LINES_THRESHOLD, +): boolean { + const tail = text.length > TAIL_WINDOW ? text.slice(-TAIL_WINDOW) : text; + const lines = tail.split('\n'); + let run = 1; + let prev: string | null = null; + for (const line of lines) { + if (line.length > 0 && line === prev) { + run += 1; + if (run >= threshold) return true; + } else { + run = 1; + } + prev = line; + } + return false; +} + +/** + * Rule 2 — cheap suffix-periodicity check: the tail ends in + * ≥`MIN_PERIOD_REPEATS` back-to-back repeats of a single block of length + * ≤`MAX_PERIOD_LEN`. Catches a no-newline repeat ("abcabcabc…") the line rule + * misses. For each candidate period length p we verify the last `repeats*p` + * chars are p-periodic; we stop at the smallest p that satisfies the repeat + * count. Bounded by MAX_PERIOD_LEN × TAIL_WINDOW comparisons — negligible. Pure. + */ +export function hasPeriodicTail( + text: string, + maxPeriod = MAX_PERIOD_LEN, + minRepeats = MIN_PERIOD_REPEATS, +): boolean { + const tail = text.length > TAIL_WINDOW ? text.slice(-TAIL_WINDOW) : text; + const n = tail.length; + // Not even the shortest possible loop fits in the tail. + if (n < minRepeats) return false; + // A tail of ONE repeated char (a "trivial period") is common in LEGIT output — + // markdown rules (----/====), setext underlines, box-drawing dividers, + // trailing spaces routinely produce 20–50 identical chars. Such a run is + // p-periodic for EVERY p, so it would otherwise trip the block rule at p>=2 + // too, not just p===1. We therefore split the check: a monochar tail needs far + // more repeats (a real single-char babble loop produces hundreds-to-thousands; + // 60 is well above any realistic divider yet a fifth of TAIL_WINDOW), while a + // genuine multi-char block repeat (>=2 distinct chars, e.g. the "loadTools." + // incident, period ~10) keeps the normal MIN_PERIOD_REPEATS threshold. + const TRIVIAL_MIN_REPEATS = 60; + + // Monochar-tail check (the trivial-period case): count the trailing run of one + // identical char and require TRIVIAL_MIN_REPEATS of them. + { + const last = tail[n - 1]; + let run = 1; + for (let i = n - 2; i >= 0 && tail[i] === last; i--) run++; + if (run >= TRIVIAL_MIN_REPEATS) return true; + } + + const maxP = maxPeriod; + for (let p = 2; p <= maxP; p++) { + // Verify the last (minRepeats*p) chars are p-periodic AND not monochar (a + // monochar span is the trivial case handled above, so skip it here to avoid + // re-flagging a legit divider at a composite period). + const span = minRepeats * p; + // Not enough tail to hold this many repeats of this period. + if (span > n) continue; + const start = n - span; + let periodic = true; + let multiChar = false; + for (let i = n - 1; i >= start + p; i--) { + if (tail[i] !== tail[i - p]) { + periodic = false; + break; + } + } + if (!periodic) continue; + // Confirm the block itself has >=2 distinct chars (else it's monochar). + for (let i = start + 1; i < n; i++) { + if (tail[i] !== tail[start]) { + multiChar = true; + break; + } + } + if (multiChar) return true; + } + return false; +} + +/** + * Combined guard used by the stream's onChunk: true when EITHER rule fires. + * Pure — the caller owns the abort side effect. + */ +export function isDegenerateOutput(text: string): boolean { + return hasRepeatedLineRun(text) || hasPeriodicTail(text); +} + +/** + * Truncate a degenerated tail before persist so hundreds of KB of garbage never + * reach the DB / replay (#444). Keeps everything up to and including the FIRST + * `keepRepeats` repeats of the detected loop, then appends a short marker. If no + * loop is detected the text is returned unchanged (by identity). + * + * Implementation: find the shortest tail period (same check as hasPeriodicTail), + * keep the prefix before the loop plus `keepRepeats` copies of the block, drop + * the rest. This is best-effort cosmetic trimming; correctness does not depend on + * finding the exact minimal loop. Pure. + */ +export function truncateDegeneratedTail( + text: string, + keepRepeats = 3, +): string { + const marker = '\n…[output truncated: repeated token loop detected]'; + // Try the line rule first: collapse a long run of identical lines. + const lines = text.split('\n'); + let runStart = -1; + let run = 1; + for (let i = 1; i < lines.length; i++) { + if (lines[i].length > 0 && lines[i] === lines[i - 1]) { + if (run === 1) runStart = i - 1; + run += 1; + if (run >= REPEATED_LINES_THRESHOLD) { + const kept = lines.slice(0, runStart + keepRepeats).join('\n'); + return kept + marker; + } + } else { + run = 1; + runStart = -1; + } + } + + // Fall back to periodicity over the whole string (bounded by the same block + // length). Find the smallest period that makes the SUFFIX highly repetitive. + const n = text.length; + const maxP = Math.min(MAX_PERIOD_LEN, Math.floor(n / MIN_PERIOD_REPEATS)); + for (let p = 1; p <= maxP; p++) { + // Count how many trailing p-blocks are periodic. + let reps = 1; + let i = n - 1; + for (; i >= p; i--) { + if (text[i] !== text[i - p]) break; + } + // The loop above walks over the periodic suffix; its length is (n-1 - i). + const periodicLen = n - 1 - i; + reps = Math.floor(periodicLen / p) + 1; + if (reps >= MIN_PERIOD_REPEATS) { + const loopStart = n - reps * p; // start of the fully-periodic suffix + const kept = text.slice(0, loopStart + keepRepeats * p); + return kept + marker; + } + } + return text; +} diff --git a/apps/server/src/core/ai-chat/tools/tool-tiers.spec.ts b/apps/server/src/core/ai-chat/tools/tool-tiers.spec.ts index 301be48f..130e9b06 100644 --- a/apps/server/src/core/ai-chat/tools/tool-tiers.spec.ts +++ b/apps/server/src/core/ai-chat/tools/tool-tiers.spec.ts @@ -232,6 +232,16 @@ describe('applyLoadTools (#332)', () => { expect(LOAD_TOOLS_DESCRIPTION).toContain('only ACTIVATES them'); expect(LOAD_TOOLS_DESCRIPTION).toContain('callable on your NEXT step'); }); + + it('loadTools description tells the model CORE tools are always active (#444)', () => { + expect(LOAD_TOOLS_DESCRIPTION).toContain( + 'Tools NOT listed in the catalog are CORE and ALWAYS active', + ); + expect(LOAD_TOOLS_DESCRIPTION).toContain('NEVER via loadTools'); + // Names it out explicitly so the model doesn't loadTools a core tool. + expect(LOAD_TOOLS_DESCRIPTION).toContain('createComment'); + expect(LOAD_TOOLS_DESCRIPTION).toContain('searchInPage'); + }); }); describe('editorial "Corrector" scenario is fully served by CORE (#332)', () => { diff --git a/apps/server/src/core/ai-chat/tools/tool-tiers.ts b/apps/server/src/core/ai-chat/tools/tool-tiers.ts index 040b08a3..94ab6af3 100644 --- a/apps/server/src/core/ai-chat/tools/tool-tiers.ts +++ b/apps/server/src/core/ai-chat/tools/tool-tiers.ts @@ -84,7 +84,10 @@ export const LOAD_TOOLS_DESCRIPTION = 'block in your instructions. Pass the EXACT tool names from the catalog; this\n' + 'call only ACTIVATES them and returns { loaded: [...] } — the tools become\n' + 'callable on your NEXT step. Load several names in one call when the task clearly\n' + - 'needs them. Unknown names are rejected with the list of valid ones.'; + 'needs them. Unknown names are rejected with the list of valid ones.\n' + + 'Tools NOT listed in the catalog are CORE and ALWAYS active — call them directly,\n' + + 'NEVER via loadTools (e.g. createComment, listComments, resolveComment,\n' + + 'editPageText, searchInPage).'; /** * Tier + catalogLine for the INLINE ai-chat tools — those defined per-layer in diff --git a/apps/server/src/integrations/environment/environment.service.spec.ts b/apps/server/src/integrations/environment/environment.service.spec.ts index d4668598..10f0125e 100644 --- a/apps/server/src/integrations/environment/environment.service.spec.ts +++ b/apps/server/src/integrations/environment/environment.service.spec.ts @@ -158,4 +158,27 @@ describe('EnvironmentService', () => { ).toBe('https://app.example.com'); }); }); + + describe('isAiChatFinalStepLockdownEnabled (#444)', () => { + const build = (val?: string) => + new EnvironmentService({ + get: (key: string, def?: string) => + key === 'AI_CHAT_FINAL_STEP_LOCKDOWN' ? (val ?? def) : def, + } as any); + + it('defaults to OFF (false) when unset — the new anti-degeneration default', () => { + expect(build(undefined).isAiChatFinalStepLockdownEnabled()).toBe(false); + }); + + it('is true only for the exact opt-in "true" (case-insensitive)', () => { + expect(build('true').isAiChatFinalStepLockdownEnabled()).toBe(true); + expect(build('TRUE').isAiChatFinalStepLockdownEnabled()).toBe(true); + }); + + it('stays OFF for any other value', () => { + expect(build('false').isAiChatFinalStepLockdownEnabled()).toBe(false); + expect(build('1').isAiChatFinalStepLockdownEnabled()).toBe(false); + expect(build('yes').isAiChatFinalStepLockdownEnabled()).toBe(false); + }); + }); }); diff --git a/apps/server/src/integrations/environment/environment.service.ts b/apps/server/src/integrations/environment/environment.service.ts index 08ff8dd3..cea4d00d 100644 --- a/apps/server/src/integrations/environment/environment.service.ts +++ b/apps/server/src/integrations/environment/environment.service.ts @@ -292,6 +292,24 @@ export class EnvironmentService { return enabled === 'true'; } + /** + * Final-step lockdown for the in-app agent loop (#444). When ON (legacy), the + * LAST allowed step forces a text-only answer: tools are stripped + * (toolChoice:'none') and a synthesis instruction is appended. Defaults to OFF: + * stripping the tools mid-work triggered a token-loop degeneration incident + * (the model, robbed of its tools on the final step, emitted a 255KB block + * repeating a single token). With the toggle OFF the last step keeps its tools + * and gets only a SOFT nudge to finish with a text summary; the universal + * anti-babble guard is the token-degeneration detector instead. Enable this + * only for a model that does NOT reliably end its turns with a text answer. + */ + isAiChatFinalStepLockdownEnabled(): boolean { + const enabled = this.configService + .get('AI_CHAT_FINAL_STEP_LOCKDOWN', 'false') + .toLowerCase(); + return enabled === 'true'; + } + /** * Resumable SSE transport for durable agent runs (#184 phase 1.5). When * enabled, a run tees its SSE frames into the in-memory run-stream registry so