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 908c9200..dfa1bb75 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -56,6 +56,7 @@ import { import { isDegenerateOutput, truncateDegeneratedTail, + shouldCheckDegeneration, } from './output-degeneration'; // Max agent steps per turn. One step = one model generation; a step that calls @@ -1101,7 +1102,6 @@ export class AiChatService implements OnModuleInit { 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 @@ -1275,8 +1275,10 @@ export class AiChatService implements OnModuleInit { // trigger, abort the run ONCE with a distinguishable reason. if ( !degenerationDetected && - inProgressText.length - lastDegenerationCheckLen >= - DEGENERATION_CHECK_STEP + shouldCheckDegeneration( + inProgressText.length, + lastDegenerationCheckLen, + ) ) { lastDegenerationCheckLen = inProgressText.length; if (isDegenerateOutput(inProgressText)) { @@ -1296,6 +1298,13 @@ export class AiChatService implements OnModuleInit { // the in-progress accumulator for the next step. capturedSteps.push(step as StepLike); inProgressText = ''; + // Reset the degeneration-check watermark too (#486): it tracks a byte + // offset INTO inProgressText, so once that resets to '' a stale (large) + // mark makes `inProgressText.length - lastDegenerationCheckLen` go + // negative and the throttled detector stays silent until a later step's + // text re-grows past the old offset — a whole degenerate step could slip + // through undetected. Zeroing it re-arms the check from the next byte. + lastDegenerationCheckLen = 0; // Step-granular durability (#183): persist this finished step (its text + // tool calls + tool RESULTS) the moment it ends, so a process death after // this point still recovers the step. Not awaited here (never block the diff --git a/apps/server/src/core/ai-chat/output-degeneration.spec.ts b/apps/server/src/core/ai-chat/output-degeneration.spec.ts index a4f6c36e..426f4e40 100644 --- a/apps/server/src/core/ai-chat/output-degeneration.spec.ts +++ b/apps/server/src/core/ai-chat/output-degeneration.spec.ts @@ -3,6 +3,8 @@ import { hasPeriodicTail, isDegenerateOutput, truncateDegeneratedTail, + shouldCheckDegeneration, + DEGENERATION_CHECK_STEP, REPEATED_LINES_THRESHOLD, MIN_PERIOD_REPEATS, } from './output-degeneration'; @@ -180,3 +182,42 @@ describe('truncateDegeneratedTail', () => { expect(truncateDegeneratedTail(text)).toBe(text); }); }); + +/** + * Throttle + step-boundary reset (#486). The stream keeps a watermark + * (`lastDegenerationCheckLen`) that is an OFFSET into the accumulated step text. + * On a step boundary the accumulator resets to '', so the watermark MUST reset to + * 0 too — otherwise the throttle goes silent for the whole next step. These tests + * pin the pure decision AND the reset property that ai-chat.service.onStepFinish + * now enforces. + */ +describe('shouldCheckDegeneration (throttle) + step-boundary reset (#486)', () => { + it('fires once the text grows a full DEGENERATION_CHECK_STEP past the mark', () => { + expect(shouldCheckDegeneration(DEGENERATION_CHECK_STEP, 0)).toBe(true); + expect(shouldCheckDegeneration(DEGENERATION_CHECK_STEP - 1, 0)).toBe(false); + expect(shouldCheckDegeneration(5000, 3000)).toBe(true); // grew 2000 since mark + expect(shouldCheckDegeneration(4000, 3000)).toBe(false); // grew only 1000 + }); + + it('BUG (no reset): a stale large watermark silences the next step', () => { + // End of a long step: the watermark sits at 5000. The step ends and the + // accumulator resets to '' — but if the watermark is NOT reset, a fresh short + // degenerate burst (length 2000) never triggers a check: 2000 - 5000 < STEP. + const staleWatermark = 5000; + const nextStepLen = DEGENERATION_CHECK_STEP; // a fresh 2KB burst + expect(shouldCheckDegeneration(nextStepLen, staleWatermark)).toBe(false); + }); + + it('FIX (reset to 0): the same short degenerate burst IS checked and detected', () => { + // onStepFinish now zeroes the watermark, so the fresh burst re-arms the check. + const resetWatermark = 0; + const degenerateBurst = 'loadTools.\n'.repeat(300); // real degeneration + expect(degenerateBurst.length).toBeGreaterThanOrEqual(DEGENERATION_CHECK_STEP); + // The throttle now fires... + expect( + shouldCheckDegeneration(degenerateBurst.length, resetWatermark), + ).toBe(true); + // ...and the detector catches the loop that would otherwise stream unchecked. + expect(isDegenerateOutput(degenerateBurst)).toBe(true); + }); +}); diff --git a/apps/server/src/core/ai-chat/output-degeneration.ts b/apps/server/src/core/ai-chat/output-degeneration.ts index 2834ce7e..eff4c080 100644 --- a/apps/server/src/core/ai-chat/output-degeneration.ts +++ b/apps/server/src/core/ai-chat/output-degeneration.ts @@ -131,6 +131,32 @@ export function isDegenerateOutput(text: string): boolean { return hasRepeatedLineRun(text) || hasPeriodicTail(text); } +/** + * How many bytes the in-progress text must grow before the (amortized) tail + * heuristics are re-run. Shared with ai-chat.service so the throttle the stream + * applies is the SAME one the unit test drives. + */ +export const DEGENERATION_CHECK_STEP = 2000; + +/** + * Throttle decision for the degeneration guard (#444/#486). Returns true when + * the accumulated text has grown at least DEGENERATION_CHECK_STEP bytes past the + * last-checked offset, so the pure rules only fire every ~2KB. Pure; the caller + * updates its watermark to `textLen` when this returns true. + * + * The watermark is an offset INTO the accumulator, so when the accumulator is + * reset to '' on a step boundary the caller MUST reset the watermark to 0 too + * (#486). Otherwise `textLen - lastCheckLen` goes negative after the reset and + * this returns false until a later step re-grows past the stale offset — a whole + * degenerate step could stream unchecked. + */ +export function shouldCheckDegeneration( + textLen: number, + lastCheckLen: number, +): boolean { + return textLen - lastCheckLen >= DEGENERATION_CHECK_STEP; +} + /** * 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