diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0eef84dc7..9bb9d717f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -107,7 +107,7 @@ Two directors, selected by role: - **ChatDirector** (interactive, `src/agent/director.ts`) — Extends `DefaultDirector` with task list tracking, workflow nudges, LSP auto-activation, and multi-turn chat semantics. It never terminates the session: operator declines are surfaced as replies and the reactor stays alive for the next message. Auto mode is toggled by CLI flags (`--auto` / `--no-auto`); there is currently no in-session key to toggle it (default on; constrained envelope — workspace writes and unconstrained shell auto-allow; installs, recursive rm, force/uncontained worktree changes, sensitive-path and opaque-wrapper shell still ask; contained non-force `git worktree add`/`remove`/`prune` and `list` auto-allow; shell file-mutation denied). It is not a separate edit/plan mode. - **SubAgentDirector** (delegated work, `src/subagent/index.ts`) — Drives a dispatched worker until a turn arrives with no tool calls, then replies with the final assistant text and ends the run. A tool-less turn **after tools** completes only with the four-heading envelope (Summary, Findings, Blockers, Paths); a missing envelope nudges once then salvages as **incomplete-report**. A tool-less completion with **zero tool calls in the entire run** is returned as a **never-acted** salvage report (not a successful implement). When `task(intent="implement")` is set, a tool-using run that never wrote/edited/deleted a file is returned as **never-edited** instead of complete — so a pure-explore "plan" cannot look shipped to the parent (tracked via `thrashState.editedPaths` from `edit_file` / `write_file` / `delete_file`). Explore/read-only workers that used tools then replied with findings remain normal completes. Hard stops also fire after 2 consecutive identical tool-call fingerprints (**no-progress**), on progressive re-read pressure (**thrash** — the same path re-read past a limit amid enough tool volume, tracked by `src/subagent/thrash.ts`), or after the leaf turn budget (**turn-budget**, default 30, overridable via `task(maxTurns)`, agent profile `maxTurns`, or `settings.subagentMaxTurns`, capped at 100), each returning a structured salvage report (reason, partial findings, blockers) so a thrashing child cannot burn tokens indefinitely. Before hard thrash, a one-shot **re-read-nudge** fires when re-read pressure crosses a soft threshold (default 3 same-path reads with enough tool volume, still below the hard re-read limit of 4): the director injects an ephemeral redirect — implement leaves are asked to edit or wrap up; explore leaves are asked to expand findings / change approach / report, never forced into edit — then keeps running so hard thrash remains reachable if the leaf ignores it. A fourth hard stop, **repetition**, is detected outside the director entirely: - `runSubAgent`'s stream sink watches the streamed text of the in-flight cycle for degenerate token loops (`src/subagent/repetition.ts`) — format chars (ZWSP, BOM, bidi marks, soft hyphen, …) stripped then whitespace-collapsed raw text, a smallest-period KMP check over the probe tail, default window >= 16 chars repeated >= 8 times, evaluated every 256 streamed chars — and on a hit aborts the run controller mid-cycle, returning a `repetition` salvage report that leads with the looped window and warns the parent against re-dispatching the identical brief. Because directors only see completed turns, this is the only stop that can catch a loop inside a single turn that never finishes. A one-shot **report-forced** signal fires a few turns before the cap while the leaf is still tooling — it is not a stop: the director injects a wrap-up nudge and lets the leaf finish on its own, so turn-budget stays reachable for a leaf still making progress. When both report-forced and re-read-nudge apply, report-forced wins (near-budget wrap-up is more urgent than a mid-run redirect). Operator/parent cancel after any progress likewise returns a **cancelled** salvage report (partial findings + tool activity) instead of a bare cancel string; cancel before progress still surfaces as cancelled-by-operator. + `runSubAgent`'s stream sink watches the streamed text of the in-flight cycle for degenerate token loops (`src/subagent/repetition.ts`) — format chars (ZWSP, BOM, bidi marks, soft hyphen, …) stripped then whitespace-collapsed raw text, a smallest-period KMP check over the probe tail, default window >= 16 chars repeated >= 8 times, evaluated every 256 streamed chars — and on a hit aborts the run controller mid-cycle, returning a `repetition` salvage report that leads with the looped window and warns the parent against re-dispatching the identical brief. `inference.thinking.delta` is sampled the same way on its own buffer, but with digit runs folded to one placeholder and a shorter window (>= 4 chars repeated >= 32 times), gated to periods <= 16 chars once folded: thinking is never rendered to the user, so a monotonic counter (e.g. `0/1 1/2 2/3 …`, which stays non-periodic and escapes the raw-text check) can be caught, but folding still erases real information — a healthy templated enumeration line becomes byte-identical to its neighbors once digits are erased, so the period-length cap only lets counter-shaped folded periods (a handful of chars) through and refuses the much longer periods a folded prose line produces. Because directors only see completed turns, this is the only stop that can catch a loop inside a single turn that never finishes. A one-shot **report-forced** signal fires a few turns before the cap while the leaf is still tooling — it is not a stop: the director injects a wrap-up nudge and lets the leaf finish on its own, so turn-budget stays reachable for a leaf still making progress. When both report-forced and re-read-nudge apply, report-forced wins (near-budget wrap-up is more urgent than a mid-run redirect). Operator/parent cancel after any progress likewise returns a **cancelled** salvage report (partial findings + tool activity) instead of a bare cancel string; cancel before progress still surfaces as cancelled-by-operator. Optional `task(tier=)` (`fast` | `standard` | `clever`) overrides profile inference, profile tier, and the parent provider for that spawn only, and fails closed when the tier is unconfigured. The parent `task` tool keeps a session-scoped brief-dispatch ledger (`src/subagent/brief-dispatch.ts`): fingerprints cover prompt + agent + intent + success_criteria + do_not (not maxTurns/description/tier). After thrash / no-progress / repetition / never-acted / never-edited salvage, an identical re-dispatch is hard-blocked for the rest of the parent chat; change at least one fingerprint field to force a re-run. Turn-budget salvage still invites a higher maxTurns for a few same-brief retries without a successful complete, then flips the parent hint to stop and change approach (soft — further identical dispatches are still admitted). A successful complete resets the same-brief retry budget. diff --git a/src/session/stream-journal.test.ts b/src/session/stream-journal.test.ts index 752fdc30e..101671261 100644 --- a/src/session/stream-journal.test.ts +++ b/src/session/stream-journal.test.ts @@ -21,12 +21,26 @@ function delta(token: string): ReactorEmittedEvent { return { type: "inference.text.delta", data: { token } } as unknown as ReactorEmittedEvent; } -async function readPartialRecords(): Promise> { +function thinkingDelta(token: string): ReactorEmittedEvent { + return { type: "inference.thinking.delta", data: { token } } as unknown as ReactorEmittedEvent; +} + +async function readPartialRecords(): Promise< + Array<{ reason: string; text: string; thinkingText?: string; thinkingChars?: number }> +> { const raw = await readFile(join(dir, PARTIAL_FILE), "utf8"); return raw .trim() .split("\n") - .map((line) => JSON.parse(line) as { reason: string; text: string }); + .map( + (line) => + JSON.parse(line) as { + reason: string; + text: string; + thinkingText?: string; + thinkingChars?: number; + }, + ); } describe("createCycleTextRecorder", () => { @@ -119,6 +133,43 @@ describe("createCycleTextRecorder", () => { expect(records[0]?.text).toBe("buffered text"); }); + test("buffers thinking deltas separately from text and flushes both", async () => { + const recorder = createCycleTextRecorder(() => dir); + recorder.handleEvent(delta("visible reply")); + recorder.handleEvent(thinkingDelta("0/1 1/2 2/3 ")); + expect(recorder.text()).toBe("visible reply"); + expect(recorder.thinkingText()).toBe("0/1 1/2 2/3 "); + + await recorder.flush("repetition"); + const records = await readPartialRecords(); + expect(records[0]?.text).toBe("visible reply"); + expect(records[0]?.thinkingText).toBe("0/1 1/2 2/3 "); + expect(recorder.thinkingText()).toBe(""); + }); + + test("a thinking-only loop still writes a partial record with the looped window", async () => { + // No visible text ever streamed (the observed live failure): the salvage + // must still be diagnosable from thinkingText alone. + const recorder = createCycleTextRecorder(() => dir); + recorder.handleEvent(thinkingDelta("0/1 1/2 2/3 3/4 4/5 ")); + const snapshot = await recorder.dispose("repetition"); + + expect(snapshot).toBe(""); + const records = await readPartialRecords(); + expect(records[0]?.reason).toBe("repetition"); + expect(records[0]?.text).toBe(""); + expect(records[0]?.thinkingText).toBe("0/1 1/2 2/3 3/4 4/5 "); + }); + + test("a turn boundary resets both the text and thinking buffers", () => { + const recorder = createCycleTextRecorder(() => dir); + recorder.handleEvent(delta("hello")); + recorder.handleEvent(thinkingDelta("thinking")); + recorder.handleEvent({ type: "inference.done", data: {} } as unknown as ReactorEmittedEvent); + expect(recorder.text()).toBe(""); + expect(recorder.thinkingText()).toBe(""); + }); + test("reset reopens a closed recorder so new deltas buffer and flush normally", async () => { const recorder = createCycleTextRecorder(() => dir); await recorder.dispose("rotation"); diff --git a/src/session/stream-journal.ts b/src/session/stream-journal.ts index a6a6e159c..59130d1d4 100644 --- a/src/session/stream-journal.ts +++ b/src/session/stream-journal.ts @@ -46,8 +46,10 @@ export type PartialFlushReason = export type CycleTextRecorder = { /** Feed every stream event; buffers deltas, resets on done, flushes on error. */ handleEvent: (event: ReactorEmittedEvent) => void; - /** The buffered text of the current (unfinished) cycle. */ + /** The buffered visible text of the current (unfinished) cycle. */ text: () => string; + /** The buffered thinking text of the current (unfinished) cycle. */ + thinkingText: () => string; /** Write the buffer to partial.jsonl with a reason, then reset it. */ flush: (reason: PartialFlushReason) => Promise; /** @@ -71,13 +73,25 @@ export function createCycleTextRecorder( resolveContextDir: () => string, ): CycleTextRecorder { let cycleText = ""; + let cycleThinkingText = ""; let closed = false; - const writeRecord = async (reason: PartialFlushReason, text: string): Promise => { - if (text.trim().length === 0) return; - const record = JSON.stringify({ reason, chars: text.length, text }); + const writeRecord = async ( + reason: PartialFlushReason, + text: string, + thinkingText: string, + ): Promise => { + if (text.trim().length === 0 && thinkingText.trim().length === 0) return; + const record: Record = { reason, chars: text.length, text }; + // Omitted when empty: a text-only abort (the common case) keeps the + // existing record shape, and diagnosing a thinking-loop abort needs the + // looped window that never reached visible text. + if (thinkingText.length > 0) { + record.thinkingChars = thinkingText.length; + record.thinkingText = thinkingText; + } try { - await appendFile(join(resolveContextDir(), PARTIAL_FILE), `${record}\n`, "utf8"); + await appendFile(join(resolveContextDir(), PARTIAL_FILE), `${JSON.stringify(record)}\n`, "utf8"); } catch (err) { getLogger([LOG_NAMESPACE_ROOT, "session", "partial"]).warn( "failed to write partial stream output: {error}", @@ -88,8 +102,10 @@ export function createCycleTextRecorder( const flush = async (reason: PartialFlushReason): Promise => { const text = cycleText; + const thinkingText = cycleThinkingText; cycleText = ""; - await writeRecord(reason, text); + cycleThinkingText = ""; + await writeRecord(reason, text, thinkingText); }; const handleEvent = (event: ReactorEmittedEvent): void => { @@ -99,8 +115,14 @@ export function createCycleTextRecorder( if (typeof token === "string") cycleText = appendCycleText(cycleText, token); return; } + if (event.type === "inference.thinking.delta") { + const token = (event.data as { token?: unknown }).token; + if (typeof token === "string") cycleThinkingText = appendCycleText(cycleThinkingText, token); + return; + } if (onTurnBoundary(event)) { cycleText = ""; + cycleThinkingText = ""; return; } if (event.type === "inference.error") { @@ -115,16 +137,26 @@ export function createCycleTextRecorder( if (closed) return ""; closed = true; const snapshot = cycleText; + const thinkingSnapshot = cycleThinkingText; cycleText = ""; + cycleThinkingText = ""; if (opts?.drain !== undefined) await opts.drain.catch(() => undefined); - await writeRecord(reason, snapshot); + await writeRecord(reason, snapshot, thinkingSnapshot); return snapshot; }; const reset = (): void => { closed = false; cycleText = ""; + cycleThinkingText = ""; }; - return { handleEvent, text: () => cycleText, flush, dispose, reset }; + return { + handleEvent, + text: () => cycleText, + thinkingText: () => cycleThinkingText, + flush, + dispose, + reset, + }; } diff --git a/src/subagent/repetition.test.ts b/src/subagent/repetition.test.ts index 0adaef0be..bae1e2210 100644 --- a/src/subagent/repetition.test.ts +++ b/src/subagent/repetition.test.ts @@ -4,9 +4,17 @@ import { appendCycleText, CYCLE_TEXT_CAP_CHARS } from "../session/stream-journal import { detectRepetition, DEFAULT_REPETITION_CONFIG, + DEFAULT_THINKING_REPETITION_CONFIG, REPETITION_CHECK_INTERVAL_CHARS, } from "./repetition.js"; +// A monotonic counter that never repeats verbatim: each pair's numerator and +// denominator both grow, so raw text is never byte-periodic (the shape that +// escaped detection live: ~64k thinking tokens of "0/1 1/2 2/3 …"). +function monotonicCounterStream(pairs: number): string { + return Array.from({ length: pairs }, (_, i) => `${i}/${i + 1} `).join(""); +} + const LOOP_SENTENCE = "next: dig footer/chrome and module structure for plan. 0/1.0 done. 1 remaining. 1h left. 0 errors. "; @@ -69,6 +77,63 @@ describe("detectRepetition", () => { expect(detectRepetition("short")).toBeNull(); expect(detectRepetition("")).toBeNull(); }); + + test("a strictly monotonic counter escapes the default (text) config even with thousands of tokens", () => { + // Documents the known, deliberate limitation for visible text: a growing + // counter is never byte-periodic, so it stays indistinguishable from a + // legitimate numbered list without digit normalization. + const text = monotonicCounterStream(4000); + expect(detectRepetition(text)).toBeNull(); + }); + + test("digit-normalized detection catches the monotonic counter (thinking-stream shape)", () => { + const text = monotonicCounterStream(4000); + const hit = detectRepetition(text, DEFAULT_THINKING_REPETITION_CONFIG, { normalizeDigits: true }); + expect(hit).not.toBeNull(); + expect(hit?.repeats).toBeGreaterThanOrEqual(DEFAULT_THINKING_REPETITION_CONFIG.repeatThreshold); + }); + + test("a healthy numbered list stays untripped under the default (text) config", () => { + // The run loop never passes normalizeDigits for inference.text.delta — + // this pins that visible text keeps the digit-preserving path regardless + // of how many items stream. + const items = Array.from( + { length: 400 }, + (_, i) => `${i + 1}. Ran batch ${i + 1} and verified ${i * 3} records migrated\n`, + ).join(""); + expect(detectRepetition(`Migration progress:\n${items}`)).toBeNull(); + }); + + test("does not flag templated enumeration in thinking after digit folding", () => { + // Regression: folding digits collapses a healthy templated line to a + // byte-identical ~40+ char unit once its digits are erased. 200 lines + // (~10KB) would trip windowMinChars 4 / repeatThreshold 32 without the + // maxFoldedPeriodChars gate, aborting a healthy worker mid-reasoning. + const items = Array.from( + { length: 200 }, + (_, i) => `${i + 1}. Ran batch ${i + 1} and verified ${i * 3} records migrated\n`, + ).join(""); + const hit = detectRepetition(items, DEFAULT_THINKING_REPETITION_CONFIG, { + normalizeDigits: true, + }); + expect(hit).toBeNull(); + }); + + test("still catches the monotonic counter with thousands of pairs", () => { + const text = monotonicCounterStream(4000); + const hit = detectRepetition(text, DEFAULT_THINKING_REPETITION_CONFIG, { normalizeDigits: true }); + expect(hit).not.toBeNull(); + expect(hit?.repeats).toBeGreaterThanOrEqual(DEFAULT_THINKING_REPETITION_CONFIG.repeatThreshold); + }); + + test("a near-counter with a short prose wrapper still folds to a short period and trips", () => { + // "step N/N done. " folds to "step 0/0 done. " — a 15-char period, still + // within maxFoldedPeriodChars (16), so this shape is (deliberately) still + // caught: it reads as a stalled step counter, not templated enumeration. + const text = Array.from({ length: 100 }, (_, i) => `step ${i}/${i + 1} done. `).join(""); + const hit = detectRepetition(text, DEFAULT_THINKING_REPETITION_CONFIG, { normalizeDigits: true }); + expect(hit).not.toBeNull(); + }); }); describe("repetition check accounting at the cycle-text cap", () => { diff --git a/src/subagent/repetition.ts b/src/subagent/repetition.ts index c78384455..529669b85 100644 --- a/src/subagent/repetition.ts +++ b/src/subagent/repetition.ts @@ -16,6 +16,17 @@ export type RepetitionConfig = { repeatThreshold: number; /** How much normalized tail text is examined per check. */ probeChars: number; + /** + * Largest normalized window (chars) the digit-folded path may fire on. + * Only meaningful with opts.normalizeDigits — folding digit runs to one + * placeholder can turn a healthy templated enumeration line into a + * byte-identical period once its digits are erased. A true oscillating- or + * monotonic-counter loop folds to a tiny period (a few chars); a templated + * prose line folds to a much longer one. Capping the folded period length + * lets the short, counter-shaped periods through while refusing to fire on + * the long, prose-shaped ones. Ignored when normalizeDigits is false. + */ + maxFoldedPeriodChars?: number; }; // windowMinChars * repeatThreshold = 128 chars of exactly periodic text — @@ -32,6 +43,28 @@ export const DEFAULT_REPETITION_CONFIG: RepetitionConfig = { // cutting the cost by two orders of magnitude. export const REPETITION_CHECK_INTERVAL_CHARS = 256; +// Thinking streams are never shown to the user, so unlike text (see +// normalize() below) they can fold digit runs into one placeholder without +// risking a numbered-list or table rendering complaint. But folding still +// erases real information: a healthy templated enumeration line (a worker +// narrating "N. Ran batch N and verified N*3 records migrated" once per +// iteration) is only distinct because of its digits, so once folded, many +// such lines become one repeating ~40+ char unit and look exactly like a +// loop. The discriminator that keeps that safe is period length: a true +// oscillating- or monotonic-counter loop ("0/1 1/2 2/3 …") folds to a tiny +// period (a handful of chars — the counter digits and their separators), +// while a templated prose line folds to a much longer one (the surrounding +// sentence survives folding intact). maxFoldedPeriodChars caps the folded +// path to short periods so it only ever catches counter-shaped loops, never +// prose-shaped enumeration; the repeat threshold on top of that still +// requires a long sustained run before it trips. +export const DEFAULT_THINKING_REPETITION_CONFIG: RepetitionConfig = { + windowMinChars: 4, + repeatThreshold: 32, + probeChars: 8192, + maxFoldedPeriodChars: 16, +}; + export type RepetitionHit = { /** The normalized window that repeats. */ window: string; @@ -50,10 +83,16 @@ export type RepetitionHit = { // Format / invisible separators (ZWSP, BOM, soft hyphen, bidi marks, …) are // stripped so a model that injects them between identical windows cannot // evade the detector. Observed thrash loops used U+200B between repeats. -function normalize(text: string): string { - return text +// `normalizeDigits` opts a caller into folding digit runs to one placeholder, +// which collapses a monotonic counter's varying digits into a repeating unit. +// Reserved for thinking streams (see DEFAULT_THINKING_REPETITION_CONFIG), +// which are never rendered to the user and so carry none of the numbered-list +// / table false-positive risk that keeps text normalization digit-preserving. +function normalize(text: string, normalizeDigits: boolean): string { + const stripped = text .replace(/[\u200B-\u200D\uFEFF\u00AD\u2060\u200E\u200F\u202A-\u202E\u2066-\u2069]/g, "") .replace(/\s+/g, " "); + return normalizeDigits ? stripped.replace(/\d+/g, "0") : stripped; } function prefixFunction(s: string): Int32Array { @@ -77,8 +116,9 @@ function prefixFunction(s: string): Int32Array { export function detectRepetition( text: string, config: RepetitionConfig = DEFAULT_REPETITION_CONFIG, + opts: { normalizeDigits?: boolean } = {}, ): RepetitionHit | null { - const tail = normalize(text.slice(-config.probeChars)); + const tail = normalize(text.slice(-config.probeChars), opts.normalizeDigits ?? false); if (tail.length < config.windowMinChars * config.repeatThreshold) return null; const reversed = [...tail].reverse().join(""); @@ -89,6 +129,9 @@ export function detectRepetition( const suffixLen = i + 1; const period = suffixLen - (pi[i] ?? 0); if (period < config.windowMinChars) continue; + if (opts.normalizeDigits && config.maxFoldedPeriodChars !== undefined) { + if (period > config.maxFoldedPeriodChars) continue; + } if (suffixLen < period * config.repeatThreshold) continue; const repeats = Math.floor(suffixLen / period); if (best === null || repeats > best.repeats) { diff --git a/src/subagent/run.ts b/src/subagent/run.ts index b77238a48..c0fc6c31e 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -51,6 +51,7 @@ import { consumeStream } from "../session/stream-consumer.js"; import { createCycleTextRecorder } from "../session/stream-journal.js"; import { detectRepetition, + DEFAULT_THINKING_REPETITION_CONFIG, REPETITION_CHECK_INTERVAL_CHARS, type RepetitionHit, } from "./repetition.js"; @@ -548,6 +549,7 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { // the later catch-site reads. const repetition: { hit: RepetitionHit | null } = { hit: null }; let charsSinceRepetitionCheck = 0; + let charsSinceThinkingRepetitionCheck = 0; const streamSink = (event: ReactorEmittedEvent): void => { const name = subAgentToolName(event); if (name !== null) { @@ -572,6 +574,26 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { } } } + // Thinking deltas are never shown to the user, so a monotonic-counter + // loop confined to them (the observed live thrash) never trips the + // turn-level stop checks either — sample them on the same interval with + // digit-normalized detection tuned for the collapsed period. + if (event.type === "inference.thinking.delta" && repetition.hit === null) { + const token = (event.data as { token?: unknown }).token; + charsSinceThinkingRepetitionCheck += typeof token === "string" ? token.length : 0; + if (charsSinceThinkingRepetitionCheck >= REPETITION_CHECK_INTERVAL_CHARS) { + charsSinceThinkingRepetitionCheck = 0; + const hit = detectRepetition(cycleRecorder.thinkingText(), DEFAULT_THINKING_REPETITION_CONFIG, { + normalizeDigits: true, + }); + if (hit !== null) { + repetition.hit = hit; + runController.abort( + new Error(`sub-agent thinking output repeated the same window ${hit.repeats} times`), + ); + } + } + } const partial = partialTextFromEvent(event); if (partial !== null) lastPartialText = partial; params.onEvent?.(event);