fix(#486): ревью — CHANGELOG + AGENTS.md + два теста

Ре-ревью PR #500 (changes-requested, 4 мелких, все doc/test): F1 [CHANGELOG] CHANGELOG.md [Unreleased]: - Breaking Changes: metrics-листенер 0.0.0.0→127.0.0.1 — кросс-контейнерный скрейп (docmost:9464) молча умрёт без METRICS_BIND=0.0.0.0 + METRICS_TOKEN (миграция в .env.example). - Security: утечка errorText тулов/провайдера анониму (closes #394); /metrics под Bearer (METRICS_TOKEN). - Fixed: ioredis-утечка в /health; ELK вешал event loop; beginRun-призрак → честный 503 A_RUN_BEGIN_FAILED; ai drain-hang. F2 [AGENTS.md] строка про ai-патч: теперь он несёт ДВА фикса (#184 O(n²) partialOutput И #486 drain-hang), оба тривайра названы. F3 [test] metrics.server.spec: добавлен кейс токена ТОЙ ЖЕ длины (Bearer topsecreX) → 401 — пиннит constant-time сравнение (прежние кейсы коротили на length-guard, до timingSafeEqual не доходили). F4 [test] output-degeneration.spec: behavior-тест, гоняющий РЕАЛЬНЫЕ onChunk/onStepFinish из stream() — длинный чистый шаг → граница → свежий дегенеративный бёрст → ассерт abortSignal.aborted (было хардкодом resetWatermark=0, ревёрт правки не краснил). Мутационные пруфы (non-vacuous): F3 — форс compare→true роняет same-length кейс (401→200); F4 — ревёрт lastDegenerationCheckLen=0 роняет behavior-тест (aborted true→false). Оба восстановлены, специи зелёные (34/34).
2026-07-11 04:38:16 +03:00
parent ea7c4d7cd2
commit 4a750c1e7f
4 changed files with 230 additions and 2 deletions
@@ -1,3 +1,5 @@
+import { Logger } from '@nestjs/common';
+import { streamText } from 'ai';
 import {
  hasRepeatedLineRun,
  hasPeriodicTail,
@@ -8,6 +10,15 @@ import {
  REPEATED_LINES_THRESHOLD,
  MIN_PERIOD_REPEATS,
 } from './output-degeneration';
+import { AiChatService } from './ai-chat.service';
+
+// Mock ONLY streamText so we can capture the onChunk/onStepFinish callbacks the
+// service registers and drive them by hand; every other `ai` export the service
+// uses (convertToModelMessages, stepCountIs, …) stays real.
+jest.mock('ai', () => {
+  const actual = jest.requireActual('ai');
+  return { ...actual, streamText: jest.fn() };
+});

 /**
 * Unit tests for the token-degeneration detector (#444) — the sole anti-babble
@@ -221,3 +232,149 @@ describe('shouldCheckDegeneration (throttle) + step-boundary reset (#486)', () =
    expect(isDegenerateOutput(degenerateBurst)).toBe(true);
  });
 });
+
+/**
+ * BEHAVIOR guard for the ACTUAL fix (#486, ai-chat.service.onStepFinish resets
+ * lastDegenerationCheckLen to 0). The pure tests above use a hard-coded
+ * resetWatermark, so a REVERT of the real `lastDegenerationCheckLen = 0` line
+ * would not redden any of them. This drives the REAL onChunk/onStepFinish
+ * closures from stream() end to end and asserts the run is aborted when a fresh
+ * degenerate burst arrives in the step AFTER a long clean step — which only
+ * happens if the watermark was actually zeroed on the step boundary.
+ */
+describe('AiChatService: onStepFinish re-arms the degeneration watermark (#486)', () => {
+  const streamTextMock = streamText as unknown as jest.Mock;
+
+  function makeRes() {
+    return {
+      raw: {
+        writeHead: jest.fn(),
+        write: jest.fn(),
+        once: jest.fn(),
+        on: jest.fn(),
+        flushHeaders: jest.fn(),
+        writableEnded: false,
+        destroyed: false,
+      },
+    };
+  }
+
+  function makeService() {
+    const aiChatRepo = {
+      findById: jest.fn(async () => ({ id: 'chat-1', workspaceId: 'ws-1' })),
+      insert: jest.fn(),
+    };
+    const aiChatMessageRepo = {
+      insert: jest.fn(async () => ({ id: 'msg-1' })),
+      findAllByChat: jest.fn(async () => []),
+      update: jest.fn(async () => ({ id: 'msg-1' })),
+    };
+    const aiSettings = { resolve: jest.fn(async () => ({})) };
+    const tools = { forUser: jest.fn(async () => ({})) };
+    const mcpClients = {
+      toolsFor: jest.fn(async () => ({
+        tools: {},
+        clients: [],
+        outcomes: [],
+        instructions: [],
+      })),
+    };
+    return new AiChatService(
+      {} as never, // ai
+      aiChatRepo as never,
+      aiChatMessageRepo as never,
+      {} as never, // aiChatPageSnapshotRepo
+      aiSettings as never,
+      tools as never,
+      mcpClients as never,
+      {} as never, // aiAgentRoleRepo
+      {} as never, // pageRepo
+      {} as never, // pageAccess
+      {
+        isAiChatDeferredToolsEnabled: () => false,
+        // Lockdown OFF -> the degeneration guard is the active anti-babble path.
+        isAiChatFinalStepLockdownEnabled: () => false,
+      } as never, // environment
+    );
+  }
+
+  beforeEach(() => {
+    streamTextMock.mockReset();
+    jest.spyOn(Logger.prototype, 'log').mockImplementation(() => undefined as never);
+    jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined as never);
+  });
+
+  afterEach(() => jest.restoreAllMocks());
+
+  it('aborts on a fresh degenerate burst in the NEXT step (reverting the reset line reddens this)', async () => {
+    let captured:
+      | {
+          onChunk?: (e: { chunk: { type: string; text: string } }) => void;
+          onStepFinish?: (step: unknown) => void;
+          abortSignal?: AbortSignal;
+        }
+      | undefined;
+    streamTextMock.mockImplementation((opts: never) => {
+      captured = opts;
+      return {
+        consumeStream: jest.fn(),
+        pipeUIMessageStreamToResponse: jest.fn(),
+      };
+    });
+
+    const svc = makeService();
+    await svc.stream({
+      user: { id: 'user-1' } as never,
+      workspace: { id: 'ws-1' } as never,
+      sessionId: 'sess-1',
+      body: {
+        chatId: 'chat-1',
+        messages: [
+          { id: 'm1', role: 'user', parts: [{ type: 'text', text: 'hi' }] },
+        ],
+      } as never,
+      res: makeRes() as never,
+      signal: new AbortController().signal,
+      model: {} as never,
+      role: null,
+      // No runHooks -> legacy path (socket signal), degeneration guard active.
+    });
+
+    expect(streamTextMock).toHaveBeenCalledTimes(1);
+    const onChunk = captured!.onChunk!;
+    const onStepFinish = captured!.onStepFinish!;
+    const abortSignal = captured!.abortSignal!;
+    expect(abortSignal.aborted).toBe(false);
+
+    // STEP 1: a LONG, non-degenerate first step. Distinct lines never trip the
+    // detector, but they advance the throttle watermark far past the burst size
+    // that follows (to ~5x the step). This is the stale watermark that, WITHOUT
+    // the reset, would silence step 2.
+    let counter = 0;
+    let accumulated = 0;
+    while (accumulated < DEGENERATION_CHECK_STEP * 5) {
+      const line = `unique clean line number ${counter++} with distinct words\n`;
+      accumulated += line.length;
+      onChunk({ chunk: { type: 'text-delta', text: line } });
+    }
+    expect(abortSignal.aborted).toBe(false); // clean step must not abort
+
+    // STEP BOUNDARY: the real onStepFinish resets inProgressText AND (the fix)
+    // zeroes lastDegenerationCheckLen.
+    onStepFinish({ text: 'a clean first step', toolCalls: [], toolResults: [] });
+
+    // STEP 2: a FRESH, short degenerate burst (~3.3KB). Its length is far below
+    // the step-1 stale watermark (~10KB), so WITHOUT the reset the throttle stays
+    // silent and this streams unchecked. WITH the reset (watermark 0) it re-arms,
+    // the detector fires, and the run aborts.
+    const burst = 'loadTools.\n'.repeat(300);
+    expect(burst.length).toBeGreaterThanOrEqual(DEGENERATION_CHECK_STEP);
+    expect(burst.length).toBeLessThan(DEGENERATION_CHECK_STEP * 5);
+    onChunk({ chunk: { type: 'text-delta', text: burst } });
+
+    // The decisive assertion: the composed abortSignal (unioned with the
+    // degeneration controller) is now aborted. Reverting `lastDegenerationCheckLen
+    // = 0` in onStepFinish makes this stay false.
+    expect(abortSignal.aborted).toBe(true);
+  });
+});
@@ -128,10 +128,18 @@ describe('metrics server bind + auth (#486)', () => {
    const noAuth = await req(port);
    expect(noAuth.status).toBe(401);

-    // Wrong token -> 401.
+    // Wrong token, DIFFERENT length -> 401 (short-circuits on the length guard).
    const wrong = await req(port, { authorization: 'Bearer nope' });
    expect(wrong.status).toBe(401);

+    // Wrong token, SAME length -> 401. This drives the timingSafeEqual compare
+    // itself (the length guard passes: 'Bearer topsecreX' has the same length as
+    // 'Bearer topsecret'). Pins the constant-time compare: a regression that made
+    // it return true would let this equal-length wrong token through — the
+    // different-length case above would NOT catch that.
+    const sameLen = await req(port, { authorization: 'Bearer topsecreX' });
+    expect(sameLen.status).toBe(401);
+
    // Correct token -> 200 with the metrics body.
    const ok = await req(port, { authorization: 'Bearer topsecret' });
    expect(ok.status).toBe(200);