From 4a750c1e7f96829a59110e751eede372da4403e4 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 04:38:16 +0300 Subject: [PATCH] =?UTF-8?q?fix(#486):=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E=20?= =?UTF-8?q?=E2=80=94=20CHANGELOG=20+=20AGENTS.md=20+=20=D0=B4=D0=B2=D0=B0?= =?UTF-8?q?=20=D1=82=D0=B5=D1=81=D1=82=D0=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ре-ревью 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). --- AGENTS.md | 2 +- CHANGELOG.md | 63 +++++++ .../core/ai-chat/output-degeneration.spec.ts | 157 ++++++++++++++++++ .../metrics/metrics.server.spec.ts | 10 +- 4 files changed, 230 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 642b927f..08756773 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -470,7 +470,7 @@ Vite SPA. Code is organized by feature under `apps/client/src/features/*` (mirro - **Errors must never be swallowed or shown as generic messages.** Every caught error MUST (1) be logged in full to the console/logger — error name, message, stack, `cause`, and (for HTTP/provider failures) the status code and response body — and (2) be surfaced to the user with a *specific, human-readable explanation of what actually went wrong*, never a bare generic string like "Something went wrong" / "Could not start recording" / "Transcription failed". Include the real reason (the underlying error/provider message) in the user-facing text. On the server, wrap third-party/provider failures with `describeProviderError` (or equivalent) and rethrow as a meaningful HTTP status + message — never let them collapse into an opaque 500. On the client, `console.error(, err)` the raw error AND show the extracted reason (e.g. `err.response?.data?.message`, or the error `name: message`) in the notification. - The version string shown in the UI comes from `APP_VERSION` (CI/Docker) or `git describe --tags --always` (local), resolved in `vite.config.ts` — not from `package.json`. - Server TS config is permissive (`noImplicitAny: false`, `strictNullChecks: false`, `no-explicit-any` lint disabled). Follow the existing relaxed style rather than tightening types broadly. -- Dependency versions are heavily pinned via `pnpm.overrides` and `pnpm.patchedDependencies` (`scimmy`, `yjs`, `ai`) in the root `package.json`. Don't bump pinned/patched deps casually; the patches and overrides exist for compatibility/security reasons. The `ai@6.0.134` patch disables the SDK's O(n²) cumulative `partialOutput` accumulation when no output strategy is requested (server heap OOM on long agent runs, #184; tripwire test: `apps/server/src/integrations/ai/ai-sdk-partial-output.patch.spec.ts`) — it MUST be re-created via `pnpm patch` when bumping `ai`. +- Dependency versions are heavily pinned via `pnpm.overrides` and `pnpm.patchedDependencies` (`scimmy`, `yjs`, `ai`) in the root `package.json`. Don't bump pinned/patched deps casually; the patches and overrides exist for compatibility/security reasons. The `ai@6.0.134` patch carries TWO independent server fixes, each with its own tripwire test: (1) it disables the SDK's O(n²) cumulative `partialOutput` accumulation when no output strategy is requested (server heap OOM on long agent runs, #184; tripwire: `apps/server/src/integrations/ai/ai-sdk-partial-output.patch.spec.ts`); (2) it fixes `writeToServerResponse`'s drain-hang — the loop awaited only `"drain"` under backpressure, so a mid-write client disconnect parked the pipe forever and leaked the reader/buffers until restart; it now races `"drain"` against `"close"`/`"error"`, cancels the reader on disconnect, and swallows the fire-and-forget read rejection (#486; tripwire: `apps/server/src/integrations/ai/ai-sdk-drain-hang.patch.spec.ts`). Both tripwires assert BOTH installed dist builds carry their patch marker. The patch MUST be re-created via `pnpm patch` when bumping `ai`. - **The MCP tool inventory in `SERVER_INSTRUCTIONS` is GENERATED from the registry** (`packages/mcp/src/server-instructions.ts`: `buildToolInventory()` over `SHARED_TOOL_SPECS`) and spliced into the hand-written routing prose (`ROUTING_PROSE`). So adding/renaming/removing a **shared** spec in `packages/mcp/src/tool-specs.ts` auto-updates the `` — no manual `SERVER_INSTRUCTIONS` edit needed. Only an **inline** MCP-only tool (those registered via `server.registerTool(...)` in `index.ts`, not through the registry) needs a one-line entry in `INLINE_MCP_INVENTORY`. Enforced by `packages/mcp/test/unit/tool-inventory.test.mjs`, which fails when a registered tool is missing from the generated inventory (there is no `EXCEPTIONS` opt-out anymore — every tool must appear). Update `ROUTING_PROSE` when a tool's *intent guidance* (when-to-use) changes. `packages/mcp/build/` is gitignored and rebuilt in CI/Docker via `pnpm build` (same convention as `git-sync`/`prosemirror-markdown`) — never commit it; rebuild locally after editing to run the tests. ## CI / release diff --git a/CHANGELOG.md b/CHANGELOG.md index 795c7502..ed4d5a03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the old ProseMirror-JSON output. Released together with the `#411`/`#412` breaking window so external configs break exactly once. (#413) +- **The Prometheus `/metrics` listener now binds to `127.0.0.1` (loopback) by + default instead of `0.0.0.0` (all interfaces).** This closes an unauthenticated + endpoint that was previously reachable on every interface. **DEPLOY MIGRATION — + cross-container scraping breaks silently otherwise:** if your scraper runs in a + SEPARATE container and reaches the app as `docmost:9464` (the exact topology the + old `0.0.0.0` hardcode served), you MUST now set `METRICS_BIND=0.0.0.0` — and, + because that re-exposes the endpoint, also set `METRICS_TOKEN=` and + configure the scraper with a matching Bearer token. Without `METRICS_BIND`, the + scraper can no longer connect and metrics go dark with no error. See the + `METRICS_BIND` / `METRICS_TOKEN` block in `.env.example` for the migration. + Same-host (loopback) scrapers need no change. (#486) + ### Added - **Place several images side by side in a row.** A new "Inline (side by @@ -310,6 +322,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `tee()` branch of the stream result — a ~20-step, ~28k-chunk agent run retained ~1.7 GB and OOM'd the 2 GB JS heap. Streaming granularity is unchanged; the patch must be re-created if `ai` is ever bumped. (#184) + +- **The server no longer leaks a hung stream pipe on every mid-run client + disconnect.** The same `ai@6.0.134` pnpm patch now also fixes the SDK's + `writeToServerResponse`, which awaited only a `"drain"` event under + backpressure: when a client disconnected mid-write the socket never drained, so + the write loop parked forever, `response.end()` was unreachable, and the stream + reader plus buffered chunks were pinned until process restart (every mid-run + disconnect in autonomous mode leaked one). The patch races `"drain"` against + `"close"`/`"error"`, cancels the reader and ends the response on disconnect, and + swallows the fire-and-forget read rejection instead of crashing on an + unhandledRejection. (#486) + +- **A failed autonomous agent-run start no longer becomes an unstoppable ghost + run.** When `beginRun` failed for a transient reason (e.g. a DB-pool blip), + the turn previously continued with NO run row — invisible to `/stop`, not + aborted on disconnect, and able to slip a second run past the one-run-per-chat + gate, leaving an unstoppable run until restart. The turn now fails fast with an + honest `503 A_RUN_BEGIN_FAILED` before the first byte (no orphan state), and the + client shows a "temporary — please try again" message instead of a misleading + "provider not configured". (#486) + +- **A pathological draw.io graph can no longer wedge the whole server.** The ELK + auto-layout (`layout:"elk"`) ran elkjs synchronously on the main event loop, so + a graph at the node/edge cap blocked ALL HTTP/SSE/loopback traffic while it + churned — and the old `setTimeout` "timeout" could never fire because the same + thread was blocked. Layout now runs in a worker thread with the timeout enforced + by `worker.terminate()`; the main loop stays responsive. (#486) + +- **The `/health` Redis probe no longer leaks a client on every tick while Redis + is down.** It built a new `ioredis` client per probe and disconnected it only on + success, so during an outage each health tick added another forever-reconnecting + client (an unbounded handle leak). A single long-lived probe client is now + reused and closed on shutdown. (#486) - **Internal links in exported Markdown no longer lose their visible text.** A link whose target page name had no file extension (e.g. a bare title) was collapsed to empty text during export, producing an unclickable, label-less @@ -386,6 +431,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 share); any other value now returns the generic "not found" instead of serving the page. (#218) +- **Tool and provider error text no longer leaks to anonymous readers in the + public-share AI chat.** A failing tool's raw error (which could carry an + internal page title or a stack fragment) and a provider error (which bundles the + provider `statusCode` and response body — potentially the internal baseUrl or + model name) were streamed verbatim to the anonymous reader over SSE. Errors are + now sanitized at the source: the share toolset collapses any unclassified tool + error to a safe generic string (safe, classified tool messages still pass + through for the model's self-correction), and the anonymous stream `onError` + maps provider failures to a fixed set of neutral strings — the full detail goes + only to the server log. A UI render gate is layered on top. (closes #394) + +- **The Prometheus `/metrics` endpoint can now require Bearer authentication and + is loopback-bound by default.** Previously it listened on all interfaces with no + auth. Setting `METRICS_TOKEN` requires every scrape to present + `Authorization: Bearer ` (compared in constant time), and the listener + defaults to `127.0.0.1` (see the Breaking Changes entry for the cross-container + migration). (#486) + ## [0.94.0] - 2026-06-26 This release makes AI chat durable and fast: assistant turns are persisted to 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 426f4e40..3595465d 100644 --- a/apps/server/src/core/ai-chat/output-degeneration.spec.ts +++ b/apps/server/src/core/ai-chat/output-degeneration.spec.ts @@ -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); + }); +}); diff --git a/apps/server/src/integrations/metrics/metrics.server.spec.ts b/apps/server/src/integrations/metrics/metrics.server.spec.ts index 1589db31..116961ea 100644 --- a/apps/server/src/integrations/metrics/metrics.server.spec.ts +++ b/apps/server/src/integrations/metrics/metrics.server.spec.ts @@ -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);