Merge pull request #511 from corbitsdev/cl-6634-repetition-detector-ignores-thinking-token-loops

Detect repetition loops in thinking token streams

Author: TheGreatAxios

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.
 
 

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<Array<{ reason: string; text: string }>> {
+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");

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<void>;
   /**
@@ -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<void> => {
-    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<void> => {
+    if (text.trim().length === 0 && thinkingText.trim().length === 0) return;
+    const record: Record<string, unknown> = { 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<void> => {
     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,
+  };
 }

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", () => {