diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 624368798..0e606d13f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -118,17 +118,35 @@ Both directors consume one `ModelFamilyPolicy` object, resolved once per session | Field | Meaning | |---|---| -| `toolOnlyTurnNudgeAt` | Consecutive tool-only assistant turns (tool calls, no text) before the ChatDirector injects a one-shot wrap-up nudge. | -| `toolOnlyTurnPauseAt` | Consecutive tool-only turns before the ChatDirector stops issuing infers and surfaces a loud operator-facing pause. | +| `toolOnlyTurnNudgeAt` | Consecutive tool-only assistant turns (tool calls, no text) before the ChatDirector injects a one-shot wrap-up nudge — a check-in, not a stop. | | `wrapUpNudgeText` | Ephemeral nudge text injected at the nudge threshold. | | `subAgentStallTimeoutMs` | Wall-clock inactivity, in ms, before a silent sub-agent leaf gets a continuation nudge. | | `applyGrokFinishBias` | The existing grok anti-thrash residual (withheld from orchestrators — see `shouldApplyGrokAntiThrash`). | -Defaults are permissive (12 / 20 turn-only thresholds, 5-minute stall timeout) so a busy-but-progressing session — tool turns interleaved with narration — never trips either mechanism. **Grok** is tightened (6 / 10, 90s) — xAI's own CLI ships the same shape of main-session auto-pause ("Goal auto-paused after N consecutive non-completing turns"), and a directly observed 14-turn pure-tool-call grok session that the operator had to cancel by hand motivated the lower thresholds. **Kimi (Moonshot)** detection ships now (`isKimiLeafProvider`) so callers can already branch on the family, but its thresholds are provisional — pinned to the permissive default with a why-comment in the policy module — pending eval characterization of Kimi's tool-only and stall behavior. +Defaults (`src/agent/model-family-policy.ts:47`): nudge at 25 consecutive tool-only turns, 5-minute stall timeout. The hard pause is no longer a `ModelFamilyPolicy` field — it runs the same period-detection thrash check for every family (see below). Nudge-at-25 replaced an earlier count-only design (nudge at 12, hard-pause at 20 by count alone, grok tightened to 6/10) that conflated any tool-only turn with no-progress — a Grok session hard-paused at 10 turns while making real progress through Linear lookups and code reads (CL-4839's original loop protection was aimed at runaway list-crawl thrash, not busy-but-progressing tool use). A grep/jq pass over real session traces under `~/.corbits/projects/*/*/context/turns.jsonl` (54 sessions with any tool-only run) found healthy tool-only streaks topping out at 13 turns (p90 12, p99 13) — 25 sits comfortably above that. **Grok** shares the default nudge threshold (its own 6/10 pair was the miscalibration this fixed) but keeps its shorter sub-agent stall timeout (90s) and `applyGrokFinishBias` residual, both independently motivated. **Kimi (Moonshot)** detection ships now (`isKimiLeafProvider`) so callers can already branch on the family, but its thresholds are provisional — pinned to the permissive default with a why-comment in the policy module — pending eval characterization of Kimi's tool-only and stall behavior. #### Main-session loop protection -The ChatDirector counts consecutive assistant turns that contain tool calls and no text (`toolOnlyStreak`), reset by any turn with text and by every fresh operator message. A dismissed `ask_operator` counts as a no-progress, tool-only turn — the decline path does not reset the streak. At `toolOnlyTurnNudgeAt` the director arms a one-shot ephemeral wrap-up nudge; at `toolOnlyTurnPauseAt` it stops issuing infers entirely and replies with a loud, operator-facing pause message ("Auto-paused: the model ran N steps in a row without explaining its progress. Send a message to resume", `src/agent/director.ts:424-426`), using the same `capabilities.reply()` channel the workflow-stall message already uses to reach the TUI — no new director-to-UI channel was needed. Because a turn with pending `tool_call` blocks must be followed by tool results before anything else (a bare nudge turn on top of pending tool calls is a provider-invalid conversation), both the nudge and the pause are applied by rewriting the `infer` action that follows once those pending tools have resolved — the same one-shot rewrite shape as the sub-agent report-forced wiring below. This loop-protection rewrite runs with the **highest precedence** among the terminal/continuation rewrites in `decideInner`: it is checked before the workflow-idle and open-task continuation nudges, since those exist to keep a session moving — exactly the behavior the pause guards against. Resuming is just the operator sending a new message, which resets the streak and un-pauses through the same reset path as the other nudge budgets. +The ChatDirector counts consecutive assistant turns that contain tool calls and no text (`toolOnlyStreak`), reset by any turn with text and by every fresh operator message. A dismissed `ask_operator` counts as a no-progress, tool-only turn — the decline path does not reset the streak. Two independent triggers ride on that streak: at `toolOnlyTurnNudgeAt` the director arms a one-shot ephemeral wrap-up nudge, regardless of what the tool calls were — a long streak of varied, productive tool calls runs straight through it every time. + +The hard pause is a separate signal that does **not** depend on the nudge having fired first. The director appends each tool-only turn's fingerprint (`fingerprintToolCalls`, `src/subagent/stop-policy.ts:108`) to a rolling history (`toolFingerprintHistory`, `src/agent/director.ts:356`, capped at `TOOL_FINGERPRINT_HISTORY_CAP` — `src/subagent/stop-policy.ts:184` — so a very long streak doesn't grow the buffer or per-turn scan unbounded) and runs `detectToolFingerprintThrash` (`src/subagent/stop-policy.ts:168`) over it on every turn. + +`detectToolFingerprintThrash` is exact-period detection, not a consecutive-identical check: it finds the shortest period `p` such that the tail of the fingerprint history is `p` repeated at least a required number of times (`detectSequencePeriod`, `src/util/period-detection.ts:61` — the same shape as the character-stream repetition detector in `src/tui/stall-watchdog.ts`'s `detectRepetition`, which now delegates to the same generic helper). This catches three shapes uniformly, where the previous consecutive-identical check only ever caught the first: + +- **period 1** — the same tool call every turn (`A,A,A,...`). +- **period 2** — an alternating pair (`A,B,A,B,...`). The previous implementation compared each turn only to the one immediately before it, so this pattern never triggered at any length. +- **period ≥3** — a rotating cycle (`A,B,C,A,B,C,...`). + +The repeat floor differs by period (`src/subagent/stop-policy.ts:138-157`): period 1 requires 5 repeats (`IDENTICAL_REPEAT_MIN`) — a short run of identical calls is legitimate (rerunning a flaky test, polling a build), and review on CL-5611 found the previous 4-repeat pause false-positived on exactly that. Any cycle of period ≥2 requires only 3 repeats (`CYCLE_REPEAT_MIN`) — there is no plausible legitimate reason to re-issue a fixed rotation of *different* tool calls with identical arguments, so it fires fast (an alternating pair pauses at 6 turns; a 3-call cycle at 9). Both floors are set well above the *measured* healthy ceiling: a local forensic scan (`scripts/tool-fingerprint-forensics.ts`, 328 sessions with a tool-only run, 559 tool-only runs — **this dataset informs the period-detection repeat floors above, not the backstop threshold below, which uses a separate measurement**) found zero occurrences of any repeating cycle for any period the scan checks — periods 1 through 6 (`MAX_PERIOD_SCANNED`); the scan does not check periods 7-8, so `TOOL_FINGERPRINT_MAX_PERIOD` (`src/subagent/stop-policy.ts:138`) has no forensic backing above period 6, only headroom — stronger than CL-5611's original "zero 3+ identical" finding for the periods it does cover. The 5-repeat period-1 floor itself is not independently measured (the forensic dataset contains no repeats to calibrate against); it is inferred headroom for the polling case, chosen only to sit above the previously-false-positived value of 4. + +Once `detectToolFingerprintThrash` reports `repeating: true`, the director stops issuing infers entirely and replies with a loud, operator-facing pause message ("Auto-paused: the model repeated the same tool call N times in a row..." for period 1, or "...repeated a P-call cycle N times in a row..." for a longer cycle, both ending "without making progress. Send a message to resume."), using the same `capabilities.reply()` channel the workflow-stall message already uses to reach the TUI. A streak of length 200+ with a different tool call every turn never pauses. Because a turn with pending `tool_call` blocks must be followed by tool results before anything else (a bare nudge turn on top of pending tool calls is a provider-invalid conversation), both the nudge and the pause are applied by rewriting the `infer` action that follows once those pending tools have resolved (`applyToolOnlyLoopProtection`, `src/agent/director.ts:447`) — the same one-shot rewrite shape as the sub-agent report-forced wiring below. This loop-protection rewrite runs with the **highest precedence** among the terminal/continuation rewrites in `decideInner`: it is checked before the workflow-idle, open-task, and goal-governor continuation nudges, since those exist to keep a session moving — exactly the behavior the pause guards against. Resuming is just the operator sending a new message, which resets the streak, the fingerprint history, and un-pauses through the same reset path as the other nudge budgets. + +**Backstop: nudge, then escalate — not an immediate pause.** Period detection has a structural blind spot: any period above `TOOL_FINGERPRINT_MAX_PERIOD`, or a "phase-broken" cycle that inserts a varying element between otherwise-repeating windows (e.g. `A,B,A,B,UNIQUE,A,B,A,B,UNIQUE,...`), never settles into an exact repeating tail and so never fires the fast path — at any streak length. Earlier versions of this backstop each had their own escape, all the same shape: the reset condition was satisfiable by something the model or the system itself could trigger. Round 4 fixed the narration escape (a raw tool-only streak that reset on any narrated turn, so a model inserting one word every ~55 turns kept resetting the counter) by separating two questions that had been sharing one reset rule — but its fix reset `turnsSinceUserMessage` on *any* `message.received` event, which is also satisfied by the synthetic content-less messages the runner sends itself after compaction (`buildCompactionContinuationMessage` in `src/tui/runner.ts`, `src/exec/runner.ts`, `src/subagent/run.ts`) — and compaction fires more often during long tool-only loops, i.e. exactly when the backstop should be counting. +Round 5 fixes the reset condition's shape instead of patching another instance: `turnsSinceUserMessage` now resets only when the inbound message carries `OPERATOR_ORIGINATED_FLAG` (`src/agent/message-provenance.ts`), a flag set only at the genuine human-input submit sites — the TUI's prompt-submit path (`userInboundMessage`, `src/tui/runner.ts`) and exec's initial-task send (`operatorTaskMessage`, `src/exec/runner.ts`). Nothing else sets it, so a message.received event from a synthetic or system-originated send (compaction continuation, retry, future director continuation) is system-originated by default and cannot accidentally qualify — the failure mode inverts from "silently forgets to exclude a sender" to "must explicitly claim to be a human." "Is the model cycling?" (`toolFingerprintHistory` / `lastThrashCheck`) is unaffected by this and is still cleared by any narrated turn — narration remains legitimate evidence the model is not stuck in a tight loop; only the "how long since the operator last saw a real checkpoint?" side (`turnsSinceUserMessage`, `src/agent/director.ts`) requires the operator flag. `detectTurnsSinceUserMessageBackstop` (`src/subagent/stop-policy.ts`) is the secondary/final-net check driven by this counter, evaluated only when period detection has not already reported `repeating: true` on that same turn — so it can never preempt the fast path, only catch what the fast path misses (periods above `TOOL_FINGERPRINT_MAX_PERIOD`, and phase-broken cycles). + +**This backstop's threshold (100) is a judgment call, not a measured value.** turns-since-last-genuine-operator-message was never separately measured — an earlier revision of this doc cited a scan of it with a stated methodology and specific percentiles; no corresponding script or output exists anywhere in the tree, and the citation was internally inconsistent about the session/run counts besides. That claim is retracted. The only real measurement available is `scripts/tool-fingerprint-forensics.ts`, which measures a related but different quantity — consecutive tool-only-turn streaks, reset by narration — p50 3, p90 8, p99 16, max 28 across 328 local sessions with a tool-only run. It doesn't directly justify 100 (narration doesn't reset this counter, so the distributions aren't comparable), but it's the only forensic data point on hand, and 100 sits comfortably above every percentile of it. + +Because the operator explicitly wants long autonomous runs to keep going, reaching the backstop threshold (`TURNS_SINCE_USER_MESSAGE_BACKSTOP`, 100) does not pause on its own — it fires a one-shot nudge asking the model for a progress summary, the same ephemeral-turn rewrite mechanism as the check-in nudge. Only if that nudge goes unheeded — `turnsSinceUserMessage` advances a further full `TURNS_SINCE_USER_MESSAGE_BACKSTOP` turns with still no user message and no thrash detected — does the director hard-pause, with a distinct message ("Auto-paused: went N turns without a message from the operator, and a progress-summary nudge went unanswered for a further N turns...") tagged `toolOnlyPauseReason: "backstop"` to distinguish it from a thrash pause in logs and messages. A genuine cycle (thrash) still preempts this escalation at any point and pauses immediately, since that is a fast, unambiguous no-progress signal on its own. #### Sub-agent stall management diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index 481a12050..d70cc02e6 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -272,23 +272,28 @@ Providers and credentials are read exclusively from settings files: the global ` ### CLI Verbs and Flags +Printed by `corbits --help` / `-h` from `CLI_HELP_TEXT` in `src/config/index.ts` +(that constant is the source of truth; keep this table in sync when flags change). + | Verb / Flag | Default | Description | |---|---|---| -| `run` (optional) | — | Run a task (default verb) | -| `resume` | — | Resume the last run in the working directory | +| _(no verb)_ | — | Interactive session; optional trailing task text | +| `exec` / `run` | — | Run a prompt (non-interactive / one-shot) | +| `resume` / `continue` | — | Reopen the latest session for this folder (project-keyed; worktrees of the same git root share sessions) | +| `resume ` | — | Reopen a specific session | +| `resume --pick` / `--list` | — | Interactive session picker | | `--cwd ` | `process.cwd()` | Working directory | | `--config ` | `~/.corbits/settings.json` | Settings file to use | | `--provider ` | from settings | Select a configured provider | | `--model ` | provider default | Select a model for the active provider | - +| `--profile ` | — | Settings profile | | `--force` | false | Override an existing run state | | `--dangerously-skip-permissions` | false | Auto-allow anything not denied by the authorization layer (gate + pre-gate workspace sandboxes; secret-guard / authz hard denies remain) | | `--auto` | true (default) | Force auto mode on (workspace writes + unconstrained shell without prompts) | | `--no-auto` | false | Start with auto mode off (ask on every consequential action); no in-session key toggles it | -| `--no-workflow` | false | Deprecated no-op; workflows are manual slash commands only | -| `--help` | — | Show help | +| `--help`, `-h` | — | Show help (exit 0 via `CliHelpError`) | -Positional arguments are joined into the optional initial task delivered when the TUI mounts. With no positional task, the operator starts from an empty prompt. +Positional arguments after flags are joined into the optional initial task delivered when the TUI mounts. With no positional task, the operator starts from an empty prompt. ### Agent Source diff --git a/docs/TELEMETRY.md b/docs/TELEMETRY.md index c31d85224..61726a2b5 100644 --- a/docs/TELEMETRY.md +++ b/docs/TELEMETRY.md @@ -1,8 +1,11 @@ # Telemetry Corbits Code sends a small amount of anonymous usage telemetry to PostHog to help -us understand aggregate usage. It is opt-out, contains no PII, and never -includes prompts, code, file contents, or paths. +us understand aggregate usage. It is opt-out. **Ambient** product and AI events +contain no PII and never include prompts, code, file contents, or paths. +**Intentional** free text the operator submits via `/feedback` is the sole +exception — that path can ship when ambient telemetry is off, still subject to +env kill switches (see Intentional feedback below). ## What's collected @@ -12,9 +15,9 @@ Each event carries a small set of properties: |---|---|---| | `cli_start` | Once per used session (see First-run disclosure) | (none beyond common properties) | | `session_end` | When a TUI session finishes | `status`, `turn_count`, `duration_ms`, `session_mode`, `exit_reason` | -| `$ai_generation` | Once per turn — on completion, and once for a turn that ends in an error instead | `$ai_trace_id`, `$ai_provider`, `$ai_model`, `$ai_input_tokens`, `$ai_output_tokens`, `$ai_latency`, `$ai_is_error`, `$ai_error`, `cache_read_tokens`, `cache_write_tokens`, `thinking_tokens` | +| `$ai_generation` | Once per turn — on completion, and once for a turn that ends in an error instead | `$ai_trace_id`, `$ai_provider`, `$ai_model`, `$ai_input_tokens`, `$ai_output_tokens`, `$ai_latency`, `$ai_is_error`, `$ai_error`, `$ai_cache_read_input_tokens`, `$ai_cache_creation_input_tokens`, `$ai_reasoning_tokens` | | `$ai_span` | Once per top-level tool call in a completed turn | `$ai_trace_id`, `$ai_span_id`, `$ai_parent_id`, `$ai_span_name`, `$ai_is_error` | -| `slash_command` | A slash command is dispatched in the TUI | `command_name` | +| `slash_command` | A slash command is dispatched (shared product-event path) | `command_name` | | `skill_used` | `use_skill` loads a skill that resolved | (none beyond common properties) | | `plugin_loaded` | A plugin is discovered and loaded at startup | `origin` | | `subagent_start` | A `task` dispatch begins | `agent_name` | @@ -23,6 +26,7 @@ Each event carries a small set of properties: | `compaction` | The compactor actually folds turns away | `mode`, `duration_ms`, `turns_before`, `turns_after` | | `crash` | A fatal error reaches the process-level handler | `kind`, `error_class` | | `auth_failure` | A provider rejects the stored credentials | `auth_provider` | +| `survey sent` | User submits intentional feedback via `/feedback` | `$survey_id`, `$survey_response`, `$survey_questions`, `turn_trace_id` | `compaction` is deliberately silent on the runs where the compactor decides there is nothing to compact — an event that also fires on no-ops makes its own @@ -68,9 +72,9 @@ on `crash` and nowhere else, so the column means one thing everywhere it is recorded. `auth_provider` is a separate property for that reason: it names which -provider's sign-in was rejected (`codex`, `xai`), chosen from a fixed -first-party set in `src/tui/session-chrome.ts`. No part of the -provider's rejection message is sent. +provider's sign-in was rejected (`codex`, `xai`, `anthropic`, `other`), +chosen from a fixed first-party set in `src/tui/session-chrome.ts`. No +part of the provider's rejection message is sent. The mapping is `src/telemetry/classify.ts`, and the tests that feed each emission site a deliberately identifying name and assert it reaches no part of @@ -106,9 +110,10 @@ configured under, which can be a local path. into one of these and then discarded — a raw message routinely embeds the request URL, a prompt excerpt, or a file path. -The cache and thinking token counts keep unprefixed names because PostHog does -not publish property names for them in its manual-capture schema; a guessed -`$ai_` name would land as an unread custom property either way. +Cache and reasoning token counts use PostHog's documented cost-property names +(`$ai_cache_read_input_tokens`, `$ai_cache_creation_input_tokens`, +`$ai_reasoning_tokens`) so LLM cost views see them. Confirmed against PostHog +manual-capture installation docs and the cost-properties reference (CL-5749). Stopping a turn mid-inference is reported, not silent: the runtime aborts the in-flight call and classifies the resulting error as `cancelled`, so a stopped @@ -125,7 +130,8 @@ or append a phantom failure to a successful one. ## What's never collected -- Prompts, model output, or any conversation content +- Prompts, model output, or any conversation content (except intentional + free-text the operator types into `/feedback` — see below) - File paths, file contents, or repo/project names - Names anyone but this project chose: MCP servers, skills, plugins, agent profiles, plugin-registered slash commands, error subclasses (see above) @@ -133,20 +139,55 @@ or append a phantom failure to a successful one. - API keys, tokens, or any other credential - Anything not in the allowlist above +## Intentional feedback (`/feedback`) + +`/feedback` is a first-party slash command that captures operator free text as +a PostHog custom survey response (`survey sent`). Two UX modes: + +1. `/feedback ` — send immediately +2. bare `/feedback` — prompts for free text; the next non-command Enter submits + that line as the response (Empty Enter cancels) + +Free text is capped at 2000 characters. When known, the last turn’s +`$ai_trace_id` is attached as `turn_trace_id` so a report can be linked to the +recent generation. + +This path is **intentional**: it can still ship when ambient product telemetry +is off (`settings.telemetry.enabled === false` or the Telemetry toggle Off), +because the operator typed the text for that purpose. Hard env kill switches +still win — `DO_NOT_TRACK=1` or `CORBITS_TELEMETRY=0/false/off/no` block +`/feedback` as well. Sending also requires an installation id and API key. + +Survey id / question id are **baked into the client** (Corbits team survey +`Corbits Code Feedback`). Same trust class as the public PostHog project key — +operators never configure them. Optional env overrides +(`CORBITS_FEEDBACK_SURVEY_ID`, `CORBITS_FEEDBACK_QUESTION_ID`) exist for tests +and forks; setting either to empty fails closed and hides the command from the +slash menu. Success copy says “sent” after the capture is accepted and flushed +toward PostHog (best-effort network delivery is not awaited on the operator +path). Free text over 2000 characters is truncated with an explicit notice. + ## Opting out -Any of the following disables telemetry entirely: +Any of the following disables ambient product telemetry (not intentional +`/feedback` unless noted): - Turn it off in the TUI: `/settings` → Telemetry tab → Off - Set `"telemetry": { "enabled": false }` in `~/.corbits/settings.json` - `CORBITS_TELEMETRY` set to any falsy value: `0`, `false`, `off`, `no`, or empty -- `DO_NOT_TRACK=1` (the standard [Console Do Not Track](https://consoledonottrack.com/) convention) - -Turning telemetry off also discards whatever is still queued and unsent. -Events captured earlier in the session but not yet transmitted are thrown -away at the moment you opt out, not sent on the way out — opting out covers -the activity you have already generated, not just the activity still to -come. + (also blocks intentional `/feedback`) +- `DO_NOT_TRACK=1` (the standard [Console Do Not Track](https://consoledonottrack.com/) + convention; also blocks intentional `/feedback`) + +Turning ambient telemetry off discards whatever is still queued and unsent for +product events. Events captured earlier in the session but not yet transmitted +are thrown away at the moment you opt out, not sent on the way out — opting out +covers the activity you have already generated, not just the activity still to +come. Installation identity is retained so intentional `/feedback` can still +send (unless an env kill switch is active). + +Env kill switches disable **everything**, including `/feedback`. The settings +toggle alone does not. Re-enable from the same Telemetry tab or by removing the env var / settings override. While an env kill is active the Telemetry tab cannot re-enable — diff --git a/scripts/tool-fingerprint-forensics.ts b/scripts/tool-fingerprint-forensics.ts new file mode 100644 index 000000000..7e9c43eef --- /dev/null +++ b/scripts/tool-fingerprint-forensics.ts @@ -0,0 +1,169 @@ +// Forensic scan over local session traces (~/.corbits/projects/**/context/turns.jsonl) +// used to re-derive the tool-fingerprint period-detection thresholds in +// src/subagent/stop-policy.ts (detectToolFingerprintThrash). For every +// maximal tool-only run (consecutive assistant turns with tool calls and no +// text) in every local session, finds the largest number of exact repeats +// observed for each candidate period 1-6, plus run-length percentiles — +// mirroring the CL-5611 analysis (54 sessions, healthy streaks topping out +// at 13 turns, zero sessions repeating a fingerprint 3+ times consecutively) +// but extended to check every period, not just period 1. +// +// Run: bun run scripts/tool-fingerprint-forensics.ts +// +// Does not print or retain any turn content — only aggregate counts — so it +// is safe to run without pulling trace data into an LLM context window. + +import { readdirSync, statSync, readFileSync } from "node:fs"; +import { join } from "node:path"; +import { homedir } from "node:os"; + +function stableJson(value: unknown): string { + if (value === null || typeof value !== "object") return JSON.stringify(value); + if (Array.isArray(value)) return `[${value.map(stableJson).join(",")}]`; + const obj = value as Record; + const keys = Object.keys(obj).sort(); + return `{${keys.map((k) => `${JSON.stringify(k)}:${stableJson(obj[k])}`).join(",")}}`; +} + +function fingerprintToolCalls(content: ReadonlyArray>): string | null { + const parts: string[] = []; + for (const block of content) { + if (block.type !== "tool_call") continue; + const name = typeof block.name === "string" ? block.name : ""; + let args: unknown = block.arguments ?? {}; + if (typeof args === "string") { + try { + args = JSON.parse(args) as unknown; + } catch { + // keep raw string + } + } + parts.push(`${name}:${stableJson(args)}`); + } + if (parts.length === 0) return null; + parts.sort(); + return parts.join("|"); +} + +function findAll(dir: string, name: string, out: string[]): void { + let entries: string[]; + try { + entries = readdirSync(dir); + } catch { + return; + } + for (const entry of entries) { + const path = join(dir, entry); + let info: ReturnType; + try { + info = statSync(path); + } catch { + continue; + } + if (info.isDirectory()) findAll(path, name, out); + else if (entry === name) out.push(path); + } +} + +function periodicSuffixLength(seq: readonly string[], period: number): number { + let i = seq.length - 1; + let j = i - period; + let matched = 0; + while (j >= 0 && seq[i] === seq[j]) { + matched++; + i--; + j--; + } + return matched + period; +} + +function maxRepeatsForPeriod(seq: readonly string[], period: number): number { + return Math.floor(periodicSuffixLength(seq, period) / period); +} + +const root = join(homedir(), ".corbits", "projects"); +const files: string[] = []; +findAll(root, "turns.jsonl", files); + +const MAX_PERIOD_SCANNED = 6; +const periodBest: Record = {}; +let sessionsWithToolOnlyRun = 0; +const runLengths: number[] = []; + +for (const file of files) { + let lines: string[]; + try { + lines = readFileSync(file, "utf8").split("\n").filter((l) => l.trim().length > 0); + } catch { + continue; + } + + const fingerprints: (string | null)[] = []; + for (const line of lines) { + let turn: { role?: string; content?: unknown } | undefined; + try { + turn = JSON.parse(line) as { role?: string; content?: unknown }; + } catch { + continue; + } + if (turn.role !== "assistant" || !Array.isArray(turn.content)) continue; + const content = turn.content as ReadonlyArray>; + const hasToolCalls = content.some((b) => b.type === "tool_call"); + const hasText = content.some( + (b) => b.type === "text" && typeof b.text === "string" && b.text.length > 0, + ); + fingerprints.push(hasToolCalls && !hasText ? fingerprintToolCalls(content) : null); + } + + const runs: string[][] = []; + let run: string[] = []; + for (const fp of fingerprints) { + if (fp === null) { + if (run.length > 0) runs.push(run); + run = []; + } else { + run.push(fp); + } + } + if (run.length > 0) runs.push(run); + if (runs.length > 0) sessionsWithToolOnlyRun++; + + for (const r of runs) { + runLengths.push(r.length); + for (let end = 1; end <= r.length; end++) { + const prefix = r.slice(0, end); + for (let period = 1; period <= MAX_PERIOD_SCANNED; period++) { + if (prefix.length < period) continue; + const reps = maxRepeatsForPeriod(prefix, period); + if (reps > (periodBest[period] ?? 0)) periodBest[period] = reps; + } + } + } +} + +runLengths.sort((a, b) => a - b); +function percentile(p: number): number { + if (runLengths.length === 0) return 0; + const idx = Math.min(runLengths.length - 1, Math.floor((p / 100) * runLengths.length)); + return runLengths[idx] as number; +} + +console.log( + JSON.stringify( + { + sessionFilesScanned: files.length, + sessionsWithToolOnlyRun, + totalToolOnlyRuns: runLengths.length, + runLengthP50: percentile(50), + runLengthP90: percentile(90), + runLengthP99: percentile(99), + runLengthMax: runLengths[runLengths.length - 1] ?? 0, + // Largest number of exact repeats observed anywhere, for each period. + // A value of 1 means "no repeat beyond the base occurrence was ever + // observed" at that period. + maxRepeatsByPeriod: periodBest, + }, + null, + 2, + ), +); diff --git a/src/agent/director.test.ts b/src/agent/director.test.ts index 44fa17cd4..8f6ea2624 100644 --- a/src/agent/director.test.ts +++ b/src/agent/director.test.ts @@ -1,11 +1,16 @@ import { describe, expect, test } from "bun:test"; import type { + InboundMessage, ReactorAction, ReactorCapabilities, ReactorInboundEvent, ReactorState, } from "@intx/types/runtime"; import { createChatDirector } from "./director.js"; +import { OPERATOR_ORIGINATED_FLAG } from "./message-provenance.js"; +import { buildCompactionContinuationMessage as tuiCompactionContinuation } from "../tui/runner.js"; +import { buildCompactionContinuationMessage as execCompactionContinuation } from "../exec/runner.js"; +import { buildCompactionContinuationMessage as subagentCompactionContinuation } from "../subagent/run.js"; const mockState: ReactorState = { turns: [] } as unknown as ReactorState; @@ -26,7 +31,26 @@ function makeCapabilities(): ReactorCapabilities { }; } +// Varied arguments per call so the fingerprint changes turn to turn — the +// shape of genuine, varied tool-only orchestration (Linear lookups, reading +// different files, ...), as opposed to repeatedToolOnlyTurn below. function toolOnlyTurn(id: string): ReactorInboundEvent { + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path: `${id}.ts` } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; +} + +// Identical tool name + arguments on every call regardless of id — the shape +// of genuine no-progress thrash (fingerprintToolCalls ignores call id). +function repeatedToolOnlyTurn(id: string): ReactorInboundEvent { return { type: "inference.done", turn: { @@ -64,13 +88,24 @@ function toolDoneEvent(callId: string): ReactorInboundEvent { } as unknown as ReactorInboundEvent; } +// A genuine operator submit — carries OPERATOR_ORIGINATED_FLAG, matching what +// userInboundMessage() builds at the real TUI/exec prompt-submit sites. function messageReceived(content = "hello"): ReactorInboundEvent { return { type: "message.received", - message: { content }, + message: { content, flags: [OPERATOR_ORIGINATED_FLAG] }, } as unknown as ReactorInboundEvent; } +// A message.received event carrying a system-originated message — no +// OPERATOR_ORIGINATED_FLAG — as director.ts would actually receive it when +// the runner delivers one. Wraps the real message builders so this test +// proves the backstop against actual production payloads, not a shape the +// test merely believes matches them. +function systemMessageReceived(message: InboundMessage): ReactorInboundEvent { + return { type: "message.received", message } as unknown as ReactorInboundEvent; +} + function actionsArray(result: ReactorAction | ReactorAction[]): ReactorAction[] { return Array.isArray(result) ? result : [result]; } @@ -86,11 +121,12 @@ async function runToolOnlyStreak( director: ReturnType, capabilities: ReactorCapabilities, count: number, + makeTurn: (id: string) => ReactorInboundEvent = toolOnlyTurn, ): Promise { let last: ReactorAction[] = []; for (let i = 0; i < count; i++) { const id = `tc-${i}`; - await director.decide(toolOnlyTurn(id), mockState, capabilities); + await director.decide(makeTurn(id), mockState, capabilities); last = actionsArray(await director.decide(toolDoneEvent(id), mockState, capabilities)); } return last; @@ -103,8 +139,8 @@ describe("ChatDirector tool-only loop protection", () => { const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy }); const capabilities = makeCapabilities(); - // Default family nudges at 12 consecutive tool-only turns. - const actions = await runToolOnlyStreak(director, capabilities, 12); + // Default family nudges at 25 consecutive tool-only turns. + const actions = await runToolOnlyStreak(director, capabilities, 25); const infer = actions.find((a) => a.type === "infer"); expect(infer).toBeDefined(); expect(ephemeralText(infer)).toBeDefined(); @@ -114,19 +150,40 @@ describe("ChatDirector tool-only loop protection", () => { const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy }); const capabilities = makeCapabilities(); - await runToolOnlyStreak(director, capabilities, 12); + await runToolOnlyStreak(director, capabilities, 25); const nextTurn = actionsArray(await runToolOnlyStreak(director, capabilities, 1)); const infer = nextTurn.find((a) => a.type === "infer"); expect(infer).toBeDefined(); expect(ephemeralText(infer)).toBeUndefined(); }); - test("pauses and stops issuing infers at the family pause threshold", async () => { - const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy }); +// Required by CL-5611: a long productive tool-only streak (varied + // fingerprints every turn) must run straight through both the nudge and + // well past any prior hard-pause threshold without ever pausing. + test("a long productive tool-only streak continues without pausing", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); const capabilities = makeCapabilities(); - // Default family pauses at 20 consecutive tool-only turns. - const actions = await runToolOnlyStreak(director, capabilities, 20); + const actions = await runToolOnlyStreak(director, capabilities, 50); + expect(actions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(actions.some((a) => a.type === "infer")).toBe(true); + }); + + // Required by CL-5611 (reworked): genuine no-progress (identical tool + // fingerprint repeating) must still be caught and stop the session. The + // period-1 (identical-consecutive) repeat floor is 5, not 4 — see + // "does not pause after 4 identical polls" below for why 4 must not fire. + test("pauses when the same tool call repeats without progress", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + const actions = await runToolOnlyStreak(director, capabilities, 5, repeatedToolOnlyTurn); expect(actions.some((a) => a.type === "infer")).toBe(false); const reply = actions.find((a) => a.type === "reply"); expect(reply).toBeDefined(); @@ -135,14 +192,449 @@ describe("ChatDirector tool-only loop protection", () => { expect(reply.content).toContain("Send a message to resume"); }); + // Required by the CL-5611 rework: a short run of identical calls is + // legitimate (rerunning a flaky test, polling a build) — critique found the + // old 4-repeat hard pause false-positived on exactly this. Four identical + // polls followed by varied work must run straight through with no pause. + test("does not pause after 4 identical polls followed by varied work", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + await runToolOnlyStreak(director, capabilities, 4, repeatedToolOnlyTurn); + const actions = await runToolOnlyStreak(director, capabilities, 3, toolOnlyTurn); + expect(actions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(actions.some((a) => a.type === "infer")).toBe(true); + }); + + // Critique's exact repro on the original PR: identicalToolFingerprintStreak + // only compared each turn to the one before it, so an alternating pattern + // never triggered a pause at any length (proved over 200 turns). Period + // detection catches the period-2 cycle instead. + test("catches an alternating A,B tool-call pattern over 200 turns", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const alternatingTurn = (id: string): ReactorInboundEvent => { + const path = Number(id.split("-")[1]) % 2 === 0 ? "a.ts" : "b.ts"; + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + }; + + const actions = await runToolOnlyStreak(director, capabilities, 200, alternatingTurn); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + }); + + // Period detection generalizes past period 1 and 2: a rotating three-call + // cycle must also be recognized as thrash. + test("catches a 3-cycle A,B,C tool-call pattern", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const paths = ["a.ts", "b.ts", "c.ts"]; + const cycleTurn = (id: string): ReactorInboundEvent => { + const path = paths[Number(id.split("-")[1]) % 3]; + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + }; + + const actions = await runToolOnlyStreak(director, capabilities, 12, cycleTurn); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + }); + + // Period detection is the fast path: for cycles it can see, it must fire + // — and be identifiable as the fast path, not the backstop — well before + // the raw-count backstop threshold could ever be reached. + test("period detection fires as the fast path, not the backstop, on A,B", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const alternatingTurn = (id: string): ReactorInboundEvent => { + const path = Number(id.split("-")[1]) % 2 === 0 ? "a.ts" : "b.ts"; + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + }; + + // A,B,A,B,A,B pauses at 6 turns per the fast-path floors — nowhere near + // the 100-turn backstop. + const actions = await runToolOnlyStreak(director, capabilities, 6, alternatingTurn); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + if (reply === undefined || reply.type !== "reply") throw new Error("expected reply action"); + expect(reply.content).toContain("repeated a 2-call cycle"); + expect(reply.content).not.toContain("tool-only turns without narrating progress"); + }); + + test("period detection fires as the fast path, not the backstop, on A,B,C", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const paths = ["a.ts", "b.ts", "c.ts"]; + const cycleTurn = (id: string): ReactorInboundEvent => { + const path = paths[Number(id.split("-")[1]) % 3]; + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + }; + + // A,B,C cycle pauses at 9 turns per the fast-path floors. + const actions = await runToolOnlyStreak(director, capabilities, 9, cycleTurn); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + if (reply === undefined || reply.type !== "reply") throw new Error("expected reply action"); + expect(reply.content).toContain("repeated a 3-call cycle"); + expect(reply.content).not.toContain("tool-only turns without narrating progress"); + }); + + // Required by round 3 (escalation reshaped in round 4): any fixed period + // ceiling has an escape above it. A 9-element rotation never repeats + // within TOOL_FINGERPRINT_MAX_PERIOD (8), so period detection can never + // fire on it — only the backstop can. Round 4: the backstop no longer + // pauses the first time it fires — it nudges at 100 turns, then only + // pauses if a further 100 turns pass with still no user message and no + // thrash detected. + test("a 9-element rotation escapes period detection, nudges at 100, and escalates to a pause at 200", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const paths = Array.from({ length: 9 }, (_, i) => `f${i}.ts`); + const rotationTurn = (id: string): ReactorInboundEvent => { + const path = paths[Number(id.split("-")[1]) % 9]; + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + }; + + // 99 turns: below the backstop nudge threshold, still no nudge or pause. + const before = await runToolOnlyStreak(director, capabilities, 99, rotationTurn); + expect(before.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(before.some((a) => a.type === "infer" && ephemeralText(a) !== undefined)).toBe(false); + + // Turn 100: the backstop nudges, but does not pause. + const nudged = actionsArray(await runToolOnlyStreak(director, capabilities, 1, rotationTurn)); + expect(nudged.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + const nudgeInfer = nudged.find((a) => a.type === "infer"); + expect(ephemeralText(nudgeInfer)).toContain("progress summary"); + + // A further 99 turns without a user message: still no pause (the + // escalation window has not fully elapsed). + const stillNoPause = await runToolOnlyStreak(director, capabilities, 99, rotationTurn); + expect(stillNoPause.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + + // Turn 200: the nudge went unheeded for a full further interval — escalate to a pause. + const actions = actionsArray(await runToolOnlyStreak(director, capabilities, 1, rotationTurn)); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + if (reply === undefined || reply.type !== "reply") throw new Error("expected reply action"); + expect(reply.content).toContain("turns without a message from the operator"); + expect(reply.content).not.toContain("cycle"); + }); + + // Required by round 3 (escalation reshaped in round 4): a "phase-broken" + // cycle inserts one varying element per window (A,B,A,B,UNIQUE,...), so the + // fingerprint tail never settles into an exact repeat at any period — + // period detection can never fire, but the backstop nudge-then-escalate + // path still catches it. + test("a phase-broken cycle escapes period detection and eventually escalates to a pause via the backstop", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + const phaseBrokenTurn = (id: string): ReactorInboundEvent => { + const i = Number(id.split("-")[1]); + const window = i % 5; + const path = window === 0 ? "a.ts" : window === 1 ? "b.ts" : window === 2 ? "a.ts" : window === 3 ? "b.ts" : `unique-${i}.ts`; + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "tool_call", id, name: "read_file", arguments: { path } }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + }; + + const actions = await runToolOnlyStreak(director, capabilities, 201, phaseBrokenTurn); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + if (reply === undefined || reply.type !== "reply") throw new Error("expected reply action"); + expect(reply.content).toContain("turns without a message from the operator"); + expect(reply.content).not.toContain("cycle"); + }); + + // Required by round 3/4: the backstop nudge threshold is well above any + // legitimate streak length in the forensic data — long varied productive + // work must not pause, or even be nudged, before it. + test("long varied productive work does not pause or nudge before the backstop threshold", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + const actions = await runToolOnlyStreak(director, capabilities, 99); + expect(actions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(actions.some((a) => a.type === "infer")).toBe(true); + }); + + // Required by round 4: the operator explicitly wants long autonomous runs + // to keep going as long as the operator stays engaged. Periodic genuine + // user messages reset turnsSinceUserMessage, so a long run interleaved + // with real interaction must never reach the backstop, however many total + // turns it accumulates. + test("long varied productive work with real periodic user interaction never pauses", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + for (let round = 0; round < 5; round++) { + await director.decide(messageReceived(`keep going, round ${round}`), mockState, capabilities); + const actions = await runToolOnlyStreak(director, capabilities, 80); + expect(actions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + } + }); + + // Round 4 regression test: critique's exact escape — one narrated word + // every ~55 tool-only turns kept resetting BOTH toolFingerprintHistory and + // the old raw backstop counter, so a 2240-turn run never paused. With the + // reset split, narration still clears period-detection history (so no + // false thrash pause), but no longer touches turnsSinceUserMessage, so the + // backstop nudges at 100 and, since narration keeps arriving instead of a + // real user message, escalates to a pause at 200. + test("critique's 2240-turn one-narrated-word-every-55-turns repro now nudges then pauses", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + let nudged = false; + let paused = false; + for (let i = 0; i < 2240 && !paused; i++) { + const id = `tc-${i}`; + // One narrated word every 55 turns; otherwise a varied tool-only turn. + const event = i > 0 && i % 55 === 0 ? textAndToolTurn(id, "working") : toolOnlyTurn(id); + await director.decide(event, mockState, capabilities); + const result = actionsArray(await director.decide(toolDoneEvent(id), mockState, capabilities)); + if (result.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))) { + paused = true; + } else if (result.some((a) => a.type === "infer" && ephemeralText(a)?.includes("progress summary"))) { + nudged = true; + } + } + + expect(nudged).toBe(true); + expect(paused).toBe(true); + }); + + test("a genuine fresh user message resets the backstop", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + // Reach the backstop nudge. + await runToolOnlyStreak(director, capabilities, 100); + await director.decide(messageReceived("status check"), mockState, capabilities); + // After the reset, a further 99 turns (below the threshold again) must + // not nudge or pause. + const afterReset = await runToolOnlyStreak(director, capabilities, 99); + expect(afterReset.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(afterReset.some((a) => a.type === "infer" && ephemeralText(a)?.includes("progress summary"))).toBe( + false, + ); + }); + + // Round 5: round 4 reset turnsSinceUserMessage on any message.received, + // which is also satisfied by the synthetic content-less messages the + // runner delivers itself after compaction — and compaction fires more + // during long tool-only loops, i.e. exactly when the backstop should be + // counting. Prove the fix against the real production message builders, + // not a hand-rolled shape that merely looks synthetic, at all three call + // sites named in the round-4 critique. + for (const [label, build] of [ + ["tui/runner.ts:1174", tuiCompactionContinuation], + ["exec/runner.ts:418", execCompactionContinuation], + ["subagent/run.ts:367", subagentCompactionContinuation], + ] as const) { + test(`a synthetic compaction continuation from ${label} does not reset the backstop`, async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + // Reach the backstop nudge, then deliver the real synthetic message + // this call site actually produces. + await runToolOnlyStreak(director, capabilities, 100); + await director.decide(systemMessageReceived(build()), mockState, capabilities); + + // If the synthetic message had reset turnsSinceUserMessage, a further + // 99 turns would stay quiet indefinitely. It must not: escalation + // still lands exactly 100 turns after the nudge, same as if the + // synthetic message had never arrived. + const stillNoPause = await runToolOnlyStreak(director, capabilities, 99); + expect(stillNoPause.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe( + false, + ); + + const actions = actionsArray(await runToolOnlyStreak(director, capabilities, 1)); + const reply = actions.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(reply).toBeDefined(); + }); + } + + test("a genuine operator submit does reset the backstop even after a synthetic message arrived", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + await runToolOnlyStreak(director, capabilities, 100); + // A synthetic message arrives first (e.g. a compaction continuation + // mid-loop) — must not reset anything. + await director.decide(systemMessageReceived(tuiCompactionContinuation()), mockState, capabilities); + // Then the operator actually sends something. + await director.decide(messageReceived("status check"), mockState, capabilities); + + const afterReset = await runToolOnlyStreak(director, capabilities, 99); + expect(afterReset.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(afterReset.some((a) => a.type === "infer" && ephemeralText(a)?.includes("progress summary"))).toBe( + false, + ); + }); + + // Round 4: narration clears period-detection history (evidence the model + // isn't cycling) but must NOT clear turnsSinceUserMessage — otherwise a + // model can narrate its way past the backstop forever without ever + // sending anything the operator asked for. + test("model narration does not reset the backstop but does clear period-detection history", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + // Build up an almost-thrashing repeated-fingerprint run, then narrate — + // this must clear the fingerprint history (no thrash pause even after + // more repeats) while still counting toward the backstop. + await runToolOnlyStreak(director, capabilities, 4, repeatedToolOnlyTurn); + const narrated = actionsArray( + await director.decide(textAndToolTurn("narrate-1", "still working on it"), mockState, capabilities), + ); + await director.decide(toolDoneEvent("narrate-1"), mockState, capabilities); + expect(narrated.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + + // Resume the repeated-fingerprint run — since history was cleared, it + // takes a fresh IDENTICAL_REPEAT_MIN-length run to thrash-pause again, + // and it must not reference the backstop when it does. + const afterNarration = await runToolOnlyStreak(director, capabilities, 5, repeatedToolOnlyTurn); + const thrashReply = afterNarration.find((a) => a.type === "reply" && a.content.includes("Auto-paused")); + expect(thrashReply).toBeDefined(); + if (thrashReply === undefined || thrashReply.type !== "reply") throw new Error("expected reply action"); + expect(thrashReply.content).not.toContain("turns without a message from the operator"); + + // Now prove narration did NOT reset turnsSinceUserMessage: drain the + // remaining budget to the backstop threshold with varied tool-only turns + // and a fresh director for a clean count, interleaving narration every + // few turns, and confirm the backstop still nudges at the expected + // total turn count rather than being pushed back out by narration. + const fresh = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + let nudgedAtTurn: number | null = null; + for (let i = 0; i < 100; i++) { + const id = `fc-${i}`; + const event = i % 10 === 0 ? textAndToolTurn(id, "narrating") : toolOnlyTurn(id); + await fresh.decide(event, mockState, capabilities); + const result = actionsArray(await fresh.decide(toolDoneEvent(id), mockState, capabilities)); + if ( + nudgedAtTurn === null && + result.some((a) => a.type === "infer" && ephemeralText(a)?.includes("progress summary")) + ) { + nudgedAtTurn = i + 1; + } + } + // Exactly 100 total turns (narrated or not) trips the backstop nudge — + // proving narration advanced turnsSinceUserMessage rather than resetting + // it, since 10 of those 100 turns were narrated. + expect(nudgedAtTurn).toBe(100); + }); + test("resumes after the operator sends a new message", async () => { const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy }); const capabilities = makeCapabilities(); - await runToolOnlyStreak(director, capabilities, 20); + await runToolOnlyStreak(director, capabilities, 5, repeatedToolOnlyTurn); await director.decide(messageReceived("keep going"), mockState, capabilities); // A fresh tool-only streak from zero must not immediately re-pause. - const actions = await runToolOnlyStreak(director, capabilities, 1); + const actions = await runToolOnlyStreak(director, capabilities, 1, repeatedToolOnlyTurn); expect(actions.some((a) => a.type === "reply")).toBe(false); }); @@ -150,10 +642,10 @@ describe("ChatDirector tool-only loop protection", () => { const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy }); const capabilities = makeCapabilities(); - // 11 ordinary tool-only turns, then a turn whose only tool call is a - // declined ask_operator — the streak must still reach the nudge - // threshold on turn 12, exactly as if it were any other tool call. - for (let i = 0; i < 11; i++) { + // 24 ordinary (varied) tool-only turns, then a turn whose only tool call + // is a declined ask_operator — the streak must still reach the nudge + // threshold on turn 25, exactly as if it were any other tool call. + for (let i = 0; i < 24; i++) { const id = `tc-${i}`; await director.decide(toolOnlyTurn(id), mockState, capabilities); await director.decide(toolDoneEvent(id), mockState, capabilities); @@ -189,7 +681,7 @@ describe("ChatDirector tool-only loop protection", () => { ), ); // The declined branch returns its own reply, short-circuiting this cycle; - // the streak nonetheless already reached 12 and fires on the next infer. + // the streak nonetheless already reached 25 and fires on the next infer. expect(declined.some((a) => a.type === "reply")).toBe(true); const followUp = actionsArray(await runToolOnlyStreak(director, capabilities, 1)); const infer = followUp.find((a) => a.type === "infer"); @@ -211,13 +703,42 @@ describe("ChatDirector tool-only loop protection", () => { expect(ephemeralText(infer)).toBeUndefined(); }); - test("grok's tightened thresholds fire earlier than the default family", async () => { - const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: { providerName: "xai/default", model: "grok-4.5" } }); +// Required by CL-5611: the observed failure — a Grok session hard-paused + // at 10 turns of real progress (Linear lookups + code reads). + test("grok no longer hard-pauses a 10-turn productive tool-only streak", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: { providerName: "xai/default", model: "grok-4.5" }, + }); const capabilities = makeCapabilities(); - // Grok nudges at 6, well below the default family's 12. - const actions = await runToolOnlyStreak(director, capabilities, 6); - const infer = actions.find((a) => a.type === "infer"); - expect(ephemeralText(infer)).toBeDefined(); + const actions = await runToolOnlyStreak(director, capabilities, 10); + expect(actions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); + expect(actions.some((a) => a.type === "infer")).toBe(true); + }); + + test("grok still catches genuine no-progress thrash", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: { providerName: "xai/default", model: "grok-4.5" }, + }); + const capabilities = makeCapabilities(); + + const actions = await runToolOnlyStreak(director, capabilities, 5, repeatedToolOnlyTurn); + expect(actions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(true); + }); + + // Required by CL-5611: the nudge is an ephemeral inference-side prompt, not + // a reply — it must never itself pause/end the session. + test("the nudge path does not reply-pause the session", async () => { + const director = createChatDirector("system", [], { + onTasksChange: () => {}, + provider: providerlessPolicy, + }); + const capabilities = makeCapabilities(); + + const actions = await runToolOnlyStreak(director, capabilities, 25); + expect(actions.some((a) => a.type === "reply")).toBe(false); + expect(actions.some((a) => a.type === "infer")).toBe(true); }); }); diff --git a/src/agent/director.ts b/src/agent/director.ts index 1055e34fa..40a4a5bfc 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -22,10 +22,27 @@ import { createCorbitsRetryPolicy } from "./retry-policy.js"; import { isInternalRecoveryAbortRaw } from "../inference-abort.js"; import { LOG_NAMESPACE_ROOT } from "../branding.js"; import { resolveModelFamilyPolicy, type ModelFamilyPolicy } from "./model-family-policy.js"; +import { + fingerprintToolCalls, + detectToolFingerprintThrash, + detectTurnsSinceUserMessageBackstop, + TURNS_SINCE_USER_MESSAGE_BACKSTOP, + TOOL_FINGERPRINT_HISTORY_CAP, + type ToolFingerprintThrashCheck, +} from "../subagent/stop-policy.js"; import { PRESENT_VIEW_PRIMITIVES_GUIDANCE } from "./tool-schema-normalize.js"; +import { isOperatorOriginated } from "./message-provenance.js"; const RETRY_POLICY = createCorbitsRetryPolicy(); +// Fired when turnsSinceUserMessage reaches TURNS_SINCE_USER_MESSAGE_BACKSTOP. +// A nudge, not a pause — the operator explicitly wants long autonomous runs +// to keep going, so silence alone (with no detected cycle) is not +// sufficient grounds to stop. Only ignoring this request for a further full +// backstop interval escalates to a hard pause. +const BACKSTOP_NUDGE_TEXT = + "It has been a long stretch without a message from the operator. Send a brief progress summary — what has been done, what is left — so the operator can confirm you're still on track."; + const logger = getLogger([LOG_NAMESPACE_ROOT, "agent", "director"]); function isInternalRecoveryAbort(event: Extract): boolean { @@ -364,13 +381,50 @@ class ChatDirectorImpl extends DefaultDirector { private readonly modelFamilyPolicy: ModelFamilyPolicy; // Consecutive assistant turns that contain tool calls and no text. Reset on // any turn with text and on every fresh user message — a weak model that - // spins in place on one thread of tool calls still converges to the pause, - // regardless of what it calls in between (same reset discipline as the - // idle/declined nudge budgets above). + // spins in place on one thread of tool calls still converges to the + // check-in nudge, regardless of what it calls in between (same reset + // discipline as the idle/declined nudge budgets above). This streak only + // drives the soft check-in nudge at toolOnlyTurnNudgeAt; the hard pause + // normally requires the tool-fingerprint history to actually repeat as a + // cycle (see applyToolOnlyLoopProtection and detectToolFingerprintThrash). private toolOnlyStreak = 0; private toolOnlyNudgeFired = false; private pendingToolOnlyNudge = false; private pausedForToolOnly = false; + // Rolling tail of tool-only-turn fingerprints, capped so a very long + // productive streak (200+ turns) doesn't grow the buffer or per-turn period + // scan unbounded — detection only ever looks at the tail. Cleared on any + // narrated turn (narration is legitimate evidence the model is not + // cycling) and on a fresh user message. + private toolFingerprintHistory: string[] = []; + private lastThrashCheck: ToolFingerprintThrashCheck | null = null; + // Turns since the operator last sent a genuine message — the raw backstop + // counter. Unlike toolFingerprintHistory, this is NOT cleared by narrated + // turns: model-emitted text is not evidence the operator has seen a + // checkpoint, so it must not buy back backstop budget (round-4 fix for a + // model that resets a narration-sensitive counter with one word every N + // turns). Only a message.received event whose message carries + // OPERATOR_ORIGINATED_FLAG resets it — not every message.received, since + // synthetic system sends (compaction continuations, retries, future + // director continuations) fire that event too without being operator + // input (round-5 fix; see message-provenance.ts for the flag's invariant). + // Increments on every turn boundary, tool-only or narrated alike. + private turnsSinceUserMessage = 0; + // Set to the turnsSinceUserMessage value at which the backstop nudge fired, + // so the escalation check can require a full further backstop interval to + // elapse (still with no user message and no period-detected thrash) before + // hard-pausing. Reset to null only on an operator-originated message; it + // is NOT reset when thrash detection or the escalation pause fires — + // pausedForToolOnly and toolOnlyPauseReason are recomputed fresh every + // turn instead, so a stale non-null value here is harmless once a pause + // is in effect (the next operator message clears both together). + private backstopNudgeFiredAtTurn: number | null = null; + private pendingBackstopNudge = false; + // Which mechanism triggered pausedForToolOnly — the period-detection fast + // path (a recognized cycle) or the backstop escalation (nudge went + // unheeded for a further full interval with no user message). Drives the + // pause message wording so the two are distinguishable. + private toolOnlyPauseReason: "thrash" | "backstop" | null = null; constructor(systemPrompt: string, toolDefinitions: ToolDefinition[], options: ChatDirectorImplOptions) { super(systemPrompt, toolDefinitions, {}); @@ -429,28 +483,59 @@ class ChatDirectorImpl extends DefaultDirector { /** * Rewrites the infer action in a fall-through batch once pending tool - * calls have resolved: pause wins over a still-armed nudge (the streak - * only grows past pauseAt after nudgeAt), and each rewrite is one-shot — - * cleared as soon as it is actually applied to an infer. + * calls have resolved: pause wins over either still-armed nudge, and each + * rewrite is one-shot — cleared as soon as it is actually applied to an + * infer. The pause has two independent triggers, checked in order: the + * fast path is tool-fingerprint period detection + * (detectToolFingerprintThrash) — a repeating cycle (identical calls, or + * an alternating/rotating pattern) can and often does trip the pause well + * before the streak reaches toolOnlyTurnNudgeAt, so the check-in nudge is + * not a precondition for the pause. The backstop + * (detectTurnsSinceUserMessageBackstop) never pauses on its own the first + * time it fires — it only nudges, asking for a progress summary; it only + * escalates to a pause (toolOnlyPauseReason === "backstop") once that + * nudge has gone unheeded for a further full interval with still no user + * message and no period-detected thrash (see the escalation check in + * decideInner). */ private applyToolOnlyLoopProtection( actions: ReactorAction[], capabilities: ReactorCapabilities, ): ReactorAction[] | null { - if (!this.pausedForToolOnly && !this.pendingToolOnlyNudge) return null; + if (!this.pausedForToolOnly && !this.pendingToolOnlyNudge && !this.pendingBackstopNudge) { + return null; + } const inferIndex = actions.findIndex((a) => a.type === "infer"); if (inferIndex === -1) return null; if (this.pausedForToolOnly) { + const check = this.lastThrashCheck; const pauseMessage = - `Auto-paused: the model ran ${this.toolOnlyStreak} steps in a row without explaining its progress. ` + - "Send a message to resume."; + this.toolOnlyPauseReason === "backstop" + ? `Auto-paused: went ${this.turnsSinceUserMessage} turns without a message from the operator, and a progress-summary nudge went unanswered for a further ${TURNS_SINCE_USER_MESSAGE_BACKSTOP} turns. Send a message to resume.` + : (() => { + const detail = + check !== null && check.period === 1 + ? `repeated the same tool call ${check.repeats} times in a row` + : check !== null && check.period !== null + ? `repeated a ${check.period}-call cycle ${check.repeats} times in a row` + : "repeated tool calls in a cycle"; + return `Auto-paused: the model ${detail} without making progress. Send a message to resume.`; + })(); return [ capabilities.checkpoint("tool-only-loop-paused"), capabilities.reply(pauseMessage), ]; } + if (this.pendingBackstopNudge) { + this.pendingBackstopNudge = false; + const rewritten = [...actions]; + const existing = actions[inferIndex] as Extract; + rewritten[inferIndex] = inferWithNudge(capabilities, BACKSTOP_NUDGE_TEXT, existing.options); + return rewritten; + } + this.pendingToolOnlyNudge = false; const rewritten = [...actions]; const existing = actions[inferIndex] as Extract; @@ -544,6 +629,22 @@ class ChatDirectorImpl extends DefaultDirector { this.toolOnlyNudgeFired = false; this.pendingToolOnlyNudge = false; this.pausedForToolOnly = false; + this.toolFingerprintHistory = []; + this.lastThrashCheck = null; + this.toolOnlyPauseReason = null; + // Only a message carrying OPERATOR_ORIGINATED_FLAG resets the + // backstop — not every message.received. Synthetic system sends + // (compaction continuations, retries, future director continuations) + // also fire message.received but never set this flag, so they cannot + // buy back backstop budget (round-5 fix: round 4 reset on any + // message.received, which synthetic compaction continuations satisfy + // just as easily as a real operator message — see + // turnsSinceUserMessage's declaration for the full history). + if (isOperatorOriginated(event.message.flags)) { + this.turnsSinceUserMessage = 0; + this.backstopNudgeFiredAtTurn = null; + this.pendingBackstopNudge = false; + } } if (onTurnBoundary(event)) this.inferenceRecoveries = 0; @@ -594,22 +695,78 @@ class ChatDirectorImpl extends DefaultDirector { ); this.lastInferenceTurnHadContent = hasToolCalls || hasText; - // Main-session loop protection: a run of tool-only turns (tool calls, - // no narration) is the shape of a runaway session an operator would - // otherwise have to notice and cancel by hand. A dismissed - // ask_operator counts toward this streak like any other tool-only turn - // (handled separately below; declined-tool early returns do not reset - // the streak because only text turns and fresh messages do). + // Main-session loop protection tracks two separate questions with two + // separate reset rules: + // - "is the model cycling?" — toolFingerprintHistory / lastThrashCheck + // / toolOnlyStreak. Narration is legitimate evidence the model is + // not stuck in a tight loop, so any turn with text clears these + // (same as a fresh user message). A long raw toolOnlyStreak alone + // (toolOnlyTurnNudgeAt) is just a check-in nudge; the hard pause + // from this side requires an actual repeating cycle — + // detectToolFingerprintThrash runs exact-period detection (see + // util/period-detection.ts) over the rolling fingerprint history, + // catching not just identical-every-turn thrash but also + // alternating/rotating cycles (A,B,A,B,...; A,B,C,A,B,C,...). + // - "how long since the operator last saw a real checkpoint?" — + // turnsSinceUserMessage / backstopNudgeFiredAtTurn. Model-emitted + // text does NOT clear this, and neither does a system-originated + // message.received (e.g. a compaction continuation) — only a + // message carrying OPERATOR_ORIGINATED_FLAG does (see + // turnsSinceUserMessage's declaration for why: narration and + // synthetic sends must not be able to buy back backstop budget). + // A dismissed ask_operator counts toward both like any other tool-only + // turn (handled separately below; declined-tool early returns do not + // reset the cycle-detection side because only text turns and fresh + // messages do). + this.turnsSinceUserMessage++; if (hasToolCalls && !hasText) { this.toolOnlyStreak++; + const fingerprint = fingerprintToolCalls(event.turn.content); + if (fingerprint !== null) { + this.toolFingerprintHistory.push(fingerprint); + if (this.toolFingerprintHistory.length > TOOL_FINGERPRINT_HISTORY_CAP) { + this.toolFingerprintHistory.shift(); + } + } + this.lastThrashCheck = detectToolFingerprintThrash(this.toolFingerprintHistory); } else { this.toolOnlyStreak = 0; this.toolOnlyNudgeFired = false; this.pendingToolOnlyNudge = false; - this.pausedForToolOnly = false; + this.toolFingerprintHistory = []; + this.lastThrashCheck = null; } - if (this.toolOnlyStreak >= this.modelFamilyPolicy.toolOnlyTurnPauseAt) { + + // Recomputed fresh every turn boundary; whichever branch below fires + // (if any) is this turn's outcome, in priority order: + // 1. thrash (period detection) — fast path, always wins, hard pause. + // 2. backstop escalation — the backstop nudge already fired and a + // further full backstop interval has elapsed with still no user + // message and no thrash detected — hard pause. This is the one + // case the backstop itself pauses on: a model that ignores a + // direct request for a progress summary is a real no-progress + // signal, unlike mere silence during a long autonomous stretch. + // 3. backstop nudge — first time turnsSinceUserMessage reaches the + // threshold, ask for a progress summary. Does not pause. + // 4. check-in nudge — the older, softer nudge on the raw + // narration-sensitive tool-only streak, unrelated to the backstop. + this.pausedForToolOnly = false; + this.toolOnlyPauseReason = null; + if (this.lastThrashCheck?.repeating === true) { this.pausedForToolOnly = true; + this.toolOnlyPauseReason = "thrash"; + } else if ( + this.backstopNudgeFiredAtTurn !== null && + this.turnsSinceUserMessage - this.backstopNudgeFiredAtTurn >= TURNS_SINCE_USER_MESSAGE_BACKSTOP + ) { + this.pausedForToolOnly = true; + this.toolOnlyPauseReason = "backstop"; + } else if ( + this.backstopNudgeFiredAtTurn === null && + detectTurnsSinceUserMessageBackstop(this.turnsSinceUserMessage) + ) { + this.backstopNudgeFiredAtTurn = this.turnsSinceUserMessage; + this.pendingBackstopNudge = true; } else if ( this.toolOnlyStreak === this.modelFamilyPolicy.toolOnlyTurnNudgeAt && !this.toolOnlyNudgeFired diff --git a/src/agent/message-provenance.ts b/src/agent/message-provenance.ts new file mode 100644 index 000000000..ef5e084fc --- /dev/null +++ b/src/agent/message-provenance.ts @@ -0,0 +1,22 @@ +/** + * This flag means a human typed something at the prompt; nothing else may + * set it. + * + * Round 1-4 of the tool-only loop-protection backstop each reset + * `turnsSinceUserMessage` on a condition the model or the system itself + * could trigger (consecutive-identical fingerprints, narrated text, + * any `message.received` event including synthetic compaction + * continuations). Denylisting known synthetic senders only excludes the + * ones someone remembered; the next synthetic send silently resets the + * counter again. This flag inverts that: it is an allowlist set only at + * the genuine human-input submit sites (TUI prompt submit, exec's initial + * task), so anything that does not explicitly claim to be operator input + * — retries, nudges, resumes, compaction continuations, future director + * continuations — is system-originated by default and cannot accidentally + * qualify. + */ +export const OPERATOR_ORIGINATED_FLAG = "operator-originated"; + +export function isOperatorOriginated(flags: readonly string[] | undefined): boolean { + return flags !== undefined && flags.includes(OPERATOR_ORIGINATED_FLAG); +} diff --git a/src/agent/model-family-policy.test.ts b/src/agent/model-family-policy.test.ts index 729a03db1..bfc578554 100644 --- a/src/agent/model-family-policy.test.ts +++ b/src/agent/model-family-policy.test.ts @@ -6,18 +6,15 @@ describe("resolveModelFamilyPolicy", () => { const policy = resolveModelFamilyPolicy({ providerName: "anthropic", model: "claude-sonnet-4" }); expect(policy.family).toBe("default"); expect(policy.applyGrokFinishBias).toBe(false); - expect(policy.toolOnlyTurnNudgeAt).toBeGreaterThan(8); - expect(policy.toolOnlyTurnPauseAt).toBeGreaterThan(policy.toolOnlyTurnNudgeAt); + expect(policy.toolOnlyTurnNudgeAt).toBeGreaterThan(20); }); - test("grok is tightened below the default thresholds", () => { + test("grok no longer tightens the tool-only nudge threshold below the default", () => { const grok = resolveModelFamilyPolicy({ providerName: "xai/default", model: "grok-4.5" }); const base = resolveModelFamilyPolicy({ providerName: "anthropic", model: "claude-sonnet-4" }); expect(grok.family).toBe("grok"); - expect(grok.toolOnlyTurnNudgeAt).toBeLessThan(base.toolOnlyTurnNudgeAt); - expect(grok.toolOnlyTurnPauseAt).toBeLessThan(base.toolOnlyTurnPauseAt); + expect(grok.toolOnlyTurnNudgeAt).toBe(base.toolOnlyTurnNudgeAt); expect(grok.subAgentStallTimeoutMs).toBeLessThan(base.subAgentStallTimeoutMs); - expect(grok.toolOnlyTurnNudgeAt).toBeLessThan(grok.toolOnlyTurnPauseAt); }); test("grok finish-bias applies to leaves but not orchestrators", () => { @@ -32,14 +29,6 @@ describe("resolveModelFamilyPolicy", () => { const base = resolveModelFamilyPolicy({ providerName: "anthropic", model: "claude-sonnet-4" }); expect(kimi.family).toBe("kimi"); expect(kimi.toolOnlyTurnNudgeAt).toBe(base.toolOnlyTurnNudgeAt); - expect(kimi.toolOnlyTurnPauseAt).toBe(base.toolOnlyTurnPauseAt); expect(kimi.subAgentStallTimeoutMs).toBe(base.subAgentStallTimeoutMs); }); - - test("thresholds are internally consistent (nudge strictly before pause)", () => { - for (const providerName of ["xai/default", "moonshot", "anthropic"]) { - const policy = resolveModelFamilyPolicy({ providerName }); - expect(policy.toolOnlyTurnNudgeAt).toBeLessThan(policy.toolOnlyTurnPauseAt); - } - }); }); diff --git a/src/agent/model-family-policy.ts b/src/agent/model-family-policy.ts index 2035bd3a4..5196d8733 100644 --- a/src/agent/model-family-policy.ts +++ b/src/agent/model-family-policy.ts @@ -10,14 +10,14 @@ export type ModelFamilyPolicy = { family: ModelFamily; /** * Consecutive tool-only assistant turns (tool calls, no text) before the - * main chat director injects a one-shot wrap-up nudge. + * main chat director injects a one-shot wrap-up nudge. A long tool-only + * streak is normal orchestration (Linear lookups, code reads, etc.) and + * must not by itself stop the session — this is a soft check-in, not a + * loop-protection trigger. The real stop signal is a repeating cycle in + * the tool-fingerprint history, independent of this threshold — see + * detectToolFingerprintThrash in subagent/stop-policy.ts. */ toolOnlyTurnNudgeAt: number; - /** - * Consecutive tool-only assistant turns before the main chat director stops - * issuing infers and surfaces a loud operator-facing pause. - */ - toolOnlyTurnPauseAt: number; /** Ephemeral nudge text injected at toolOnlyTurnNudgeAt. */ wrapUpNudgeText: string; /** Wall-clock inactivity, in ms, before a silent sub-agent leaf is nudged. */ @@ -36,24 +36,32 @@ const GROK_WRAP_UP_NUDGE_TEXT = "report progress now: what you have done, what is left, and whether you are " + "actually still making progress."; -// Permissive defaults: a busy-but-progressing session (tool turns interleaved -// with text) never trips these. Tightened only for families with observed -// runaway tool-only behavior (see grok below). +// Forensics on real session traces (see CL-5611, and the extended scan in +// scripts/tool-fingerprint-forensics.ts, 328 sessions with a tool-only run / +// 559 tool-only runs) found healthy tool-only streaks topping out at 28 +// consecutive turns (p50 3, p90 8, p99 16) and zero repeating +// tool-fingerprint cycles for any period the scan checked (1 through 6). 25 +// sits comfortably above the observed healthy ceiling; the nudge is a +// check-in, not a stop, so erring high costs nothing. Tightened only for +// families with observed runaway tool-only behavior (see grok below). const DEFAULT_POLICY: Omit = { - toolOnlyTurnNudgeAt: 12, - toolOnlyTurnPauseAt: 20, + toolOnlyTurnNudgeAt: 25, wrapUpNudgeText: DEFAULT_WRAP_UP_NUDGE_TEXT, subAgentStallTimeoutMs: 5 * 60_000, applyGrokFinishBias: false, }; -// xAI's own CLI ships main-session auto-pause for grok ("Goal auto-paused -// after N consecutive non-completing turns") — a directly observed 14-turn -// pure-tool-call session the operator had to cancel motivates tightening -// grok's thresholds below the shared default. +// A directly observed 14-turn pure-tool-call session for this family +// previously motivated a tightened nudge/pause pair here (6/10). That pair +// was miscalibrated: it fired on a session that was making real progress +// through Linear lookups and code reads (CL-5611), well inside the healthy +// range other families tolerate. Grok keeps its own nudge copy and shorter +// sub-agent stall timeout — both still warranted — but shares the default +// tool-only-streak nudge threshold (the hard-pause thrash check is not +// family-tuned at all; it runs the same period detection for every family) +// rather than treating "no narration" as a family-specific failure mode. const GROK_POLICY: Omit = { - toolOnlyTurnNudgeAt: 6, - toolOnlyTurnPauseAt: 10, + toolOnlyTurnNudgeAt: DEFAULT_POLICY.toolOnlyTurnNudgeAt, wrapUpNudgeText: GROK_WRAP_UP_NUDGE_TEXT, subAgentStallTimeoutMs: 90_000, applyGrokFinishBias: true, diff --git a/src/config.test.ts b/src/config.test.ts index 404210799..80b9c88ba 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -3,10 +3,12 @@ import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { buildBifrostSource, buildOpenAISource, buildProviderCatalog, catalogEntryAsProviderSettings, KEYLESS_API_KEY, loadConfig, providerCatalogToSettings, runtimeSettingsWithCatalog, SOURCE_MAX_TOKENS } from "./config/index.js"; +import { buildBifrostSource, buildOpenAISource, buildProviderCatalog, catalogEntryAsProviderSettings, CliHelpError, CLI_HELP_TEXT, KEYLESS_API_KEY, loadConfig, providerCatalogToSettings, runtimeSettingsWithCatalog, SOURCE_MAX_TOKENS } from "./config/index.js"; import type { Config, UnconfiguredConfig } from "./config/index.js"; import { mergeProviderIntoSettings, type ResolvedProvider, type Settings } from "./config/settings.js"; import { OPENCODE_GO_BASE_URL } from "../packages/opencode-go/src/index.js"; +import { generateSessionId, initSessionDir } from "./session/index.js"; +import { saveState } from "./session/state.js"; function assertConfigured(config: Config | UnconfiguredConfig): asserts config is Config { if (config.configured === false) { @@ -181,6 +183,201 @@ describe("loadConfig", () => { } }); + test("resume --pick opens the session picker without requiring prior sessions", async () => { + const cwd = await emptyCwd(); + try { + const globalPath = await writeGlobalSettings(cwd); + const config = await loadConfig(["resume", "--pick", "--cwd", cwd], { + globalSettingsPath: globalPath, + }); + assertConfigured(config); + expect(config.command).toBe("tui"); + expect(config.resumeMode).toBe("pick"); + expect(config.resumePicker).toBe(true); + expect(config.skipInitialTask).toBe(true); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("continue is an alias of resume", async () => { + const cwd = await emptyCwd(); + try { + const globalPath = await writeGlobalSettings(cwd); + const config = await loadConfig(["continue", "--list", "--cwd", cwd], { + globalSettingsPath: globalPath, + }); + assertConfigured(config); + expect(config.resumeMode).toBe("pick"); + expect(config.resumePicker).toBe(true); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("resume last fails when this project has no sessions", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + const globalPath = await writeGlobalSettings(cwd); + await expect( + loadConfig(["resume", "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }), + ).rejects.toThrow(/No previous session/); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + + test("resume reopens a known session and skips the initial task", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + const globalPath = await writeGlobalSettings(cwd); + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + await saveState( + cwd, + sessionId, + { + status: "done", + turnsUsed: 2, + task: "ship resume", + startedAt: Date.now() - 1_000, + finishedAt: Date.now(), + }, + home, + ); + const config = await loadConfig(["resume", sessionId, "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }); + assertConfigured(config); + expect(config.resumeMode).toBe("id"); + expect(config.sessionId).toBe(sessionId); + expect(config.skipInitialTask).toBe(true); + expect(config.task).toBe("ship resume"); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + + test("resume last follows the latest symlink for this project", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + const globalPath = await writeGlobalSettings(cwd); + const sessionId = generateSessionId(); + await initSessionDir(cwd, sessionId, home); + await saveState( + cwd, + sessionId, + { + status: "done", + turnsUsed: 1, + task: "keep going", + startedAt: Date.now() - 500, + finishedAt: Date.now(), + }, + home, + ); + const config = await loadConfig(["resume", "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }); + assertConfigured(config); + expect(config.resumeMode).toBe("last"); + expect(config.sessionId).toBe(sessionId); + expect(config.skipInitialTask).toBe(true); + expect(config.task).toBe("keep going"); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + + test("resume rejects an unknown session id for this project", async () => { + const cwd = await emptyCwd(); + const home = await mkdtemp(join(tmpdir(), "ic-resume-home-")); + try { + const globalPath = await writeGlobalSettings(cwd); + const missing = generateSessionId(); + await expect( + loadConfig(["resume", missing, "--cwd", cwd], { + globalSettingsPath: globalPath, + home, + }), + ).rejects.toThrow(new RegExp(`No session ${missing}`)); + } finally { + await rm(cwd, { recursive: true, force: true }); + await rm(home, { recursive: true, force: true }); + } + }); + + test("resume rejects a non-id positional instead of treating it as last", async () => { + const cwd = await emptyCwd(); + try { + const globalPath = await writeGlobalSettings(cwd); + await expect( + loadConfig(["resume", "not-a-uuid", "--cwd", cwd], { + globalSettingsPath: globalPath, + }), + ).rejects.toThrow(/not a session id/); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("resume rejects combining a session id with --pick", async () => { + const cwd = await emptyCwd(); + try { + const globalPath = await writeGlobalSettings(cwd); + const id = generateSessionId(); + await expect( + loadConfig(["resume", id, "--pick", "--cwd", cwd], { + globalSettingsPath: globalPath, + }), + ).rejects.toThrow(/cannot combine a session id with --pick/); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("resume accepts --pick after other flags", async () => { + const cwd = await emptyCwd(); + try { + const globalPath = await writeGlobalSettings(cwd); + const config = await loadConfig(["resume", "--cwd", cwd, "--pick"], { + globalSettingsPath: globalPath, + }); + assertConfigured(config); + expect(config.resumeMode).toBe("pick"); + expect(config.resumePicker).toBe(true); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + test("--help throws CliHelpError with exitCode 0 and full help text", async () => { + await expect(loadConfig(["--help"], { globalSettingsPath: NO_SETTINGS })).rejects.toBeInstanceOf( + CliHelpError, + ); + try { + await loadConfig(["-h"], { globalSettingsPath: NO_SETTINGS }); + expect.unreachable("expected CliHelpError"); + } catch (err) { + expect(err).toBeInstanceOf(CliHelpError); + const help = err as CliHelpError; + expect(help.exitCode).toBe(0); + expect(help.message).toBe(CLI_HELP_TEXT); + expect(help.message).toContain("resume"); + } + }); + test("rejects unknown flags", async () => { await expect( loadConfig(["--unknown"], { globalSettingsPath: NO_SETTINGS }), diff --git a/src/config/index.ts b/src/config/index.ts index 0b4a6d923..4d05a3b81 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -1,7 +1,10 @@ import { join, resolve } from "node:path"; import type { InferenceSource } from "@intx/types/runtime"; -import { generateSessionId } from "../session/index.js"; +import { generateSessionId, isSessionId, migrateLegacySessionIfNeeded, resolveLatestSession } from "../session/index.js"; +import { loadState } from "../session/state.js"; + + import { validateEffort, type ReasoningEffort } from "../provider/reasoning-effort.js"; import { bootstrapPricingMetadata } from "../cost/pricing-metadata.js"; import { defaultPricingCachePath, type PricingFetcherOptions } from "../cost/pricing-fetcher.js"; @@ -303,6 +306,13 @@ export type Config = { resumePicker?: boolean; /** When true, the TUI does not auto-send `task` on mount (resumed session). */ skipInitialTask?: boolean; + /** + * How this process was asked to resume. `"last"` continues the latest session + * for the project key without a picker; `"id"` continues an explicit session + * id; `"pick"` opens the interactive picker. Omitted for a fresh session. + */ + resumeMode?: "last" | "id" | "pick"; + // Deprecated workflow profile metadata; workflows are manual-only slash commands. workflow?: string; // Deprecated no-op retained for CLI compatibility. @@ -344,9 +354,50 @@ export type UnconfiguredConfig = { settingsDiagnostics?: SettingsLoadDiagnostic[]; }; +/** Printed for `corbits --help` / `-h`. Keep in sync with docs/IMPLEMENTATION.md. */ +export const CLI_HELP_TEXT = `corbits — coding agent CLI + +Usage: + corbits [flags] [task...] + corbits exec|run [flags] + corbits resume|continue [session-id|--pick] [flags] + +Continue verbs (project-keyed; worktrees of the same git root share sessions): + resume / continue reopen the latest session for this folder + resume reopen a specific session + resume --pick / --list interactive session picker + +Flags: + --cwd working directory (default: process.cwd()) + --config settings file (default: ~/.corbits/settings.json) + --provider configured provider name + --model model for the active provider + --profile settings profile + --force override an existing run state + --dangerously-skip-permissions + --auto / --no-auto auto mode on/off + --help, -h show this help +`; + +/** + * Thrown when the operator asked for CLI help. Entry points must print + * `message` to stdout and exit 0 — not treat this as a crash. + */ +export class CliHelpError extends Error { + readonly exitCode = 0 as const; + + constructor(text: string = CLI_HELP_TEXT) { + super(text); + this.name = "CliHelpError"; + } +} + export type LoadConfigOptions = { // Override the global settings file location (for tests / non-standard homes). globalSettingsPath?: string; + // Override the home directory used for project-key session roots (tests). + // Production callers leave this unset so sessions resolve under ~/.corbits. + home?: string; // When true, a missing/unresolvable provider returns an UnconfiguredConfig // instead of throwing. The TUI uses this to open the onboarding flow rather // than exiting. Headless callers should leave this false (the default). @@ -372,12 +423,44 @@ export async function loadConfig( const args = [...argv]; // Leading subcommand: `corbits exec "prompt"` (alias: `run`). Default is TUI. + // `corbits resume` / `continue` reopen a prior session for this project key + // (shared across worktrees of the same git root — see docs/IMPLEMENTATION.md). let command: "tui" | "exec" = "tui"; - if (args[0] === "exec" || args[0] === "run") { + let resumeMode: "last" | "id" | "pick" | undefined; + let resumeSessionId: string | undefined; + const leading = args[0]; + if (leading === "exec" || leading === "run") { command = "exec"; args.shift(); + } else if (leading === "resume" || leading === "continue") { + command = "tui"; + args.shift(); + // Default: last session. Explicit id, or --pick for the interactive list. + // Invalid non-flag positionals error instead of falling through to last + // (a free-form token would otherwise become task while skipInitialTask is set). + const next = args.slice()[0]; + if (next === "--pick" || next === "--list") { + resumeMode = "pick"; + args.shift(); + } else if (next !== undefined && !next.startsWith("--")) { + if (!isSessionId(next)) { + throw new Error( + `'${next}' is not a session id. Use a UUID session id, \`corbits resume\` for the latest, or \`corbits resume --pick\` to choose.`, + ); + } + resumeMode = "id"; + resumeSessionId = next; + args.shift(); + } else { + resumeMode = "last"; + } } + if (args[0] === "--help" || args[0] === "-h") { + throw new CliHelpError(); + } + + let cwd = process.cwd(); let force = false; let dangerouslySkipPermissions = false; @@ -445,6 +528,13 @@ export async function loadConfig( noWorkflow = true; continue; } + if ((arg === "--pick" || arg === "--list") && resumeMode !== undefined) { + if (resumeMode === "id") { + throw new Error("cannot combine a session id with --pick/--list"); + } + resumeMode = "pick"; + continue; + } if (arg.startsWith("--")) { throw new Error(`unrecognized flag: ${arg}`); } @@ -548,18 +638,54 @@ export async function loadConfig( } } + // Resume resolution: project-key sessions live under ~/.corbits/projects// + // and are shared across worktrees of the same git root. + let sessionId = generateSessionId(); + let skipInitialTask = false; + let resumePicker = false; + let resumeTask = task; + if (resumeMode === "pick") { + resumePicker = true; + skipInitialTask = true; + } else if (resumeMode === "id") { + const id = resumeSessionId!; + await migrateLegacySessionIfNeeded(cwd, id, options.home); + const state = await loadState(cwd, id, options.home); + if (state === null) { + throw new Error( + `No session ${id} for this project. Sessions are stored under ~/.corbits/projects// (shared across worktrees of the same git root). Use \`corbits resume --pick\` to list, or \`corbits resume\` for the latest.`, + ); + } + sessionId = id; + skipInitialTask = true; + if (task.length === 0) resumeTask = state.task; + } else if (resumeMode === "last") { + const latest = await resolveLatestSession(cwd, options.home); + if (latest === null) { + throw new Error( + "No previous session for this project. Start a new one with `corbits`, or pass `corbits resume --pick` once you have sessions under ~/.corbits/projects/.", + ); + } + sessionId = latest.sessionId; + skipInitialTask = true; + const state = await loadState(cwd, sessionId, options.home); + if (task.length === 0 && state !== null) resumeTask = state.task; + } + return { configured: true, ...resolved, cwd, - task, + task: resumeTask, force, dangerouslySkipPermissions, auto, command, globalSettingsPath: effectiveSettingsPath, - sessionId: generateSessionId(), + sessionId, noWorkflow, + ...(resumeMode !== undefined ? { resumeMode, skipInitialTask } : {}), + ...(resumePicker ? { resumePicker: true } : {}), ...(profile.workflow !== undefined ? { workflow: profile.workflow } : {}), ...(settings?.defaultProvider !== undefined ? { globalDefaultProvider: settings.defaultProvider } : {}), providers: mergeOAuthCatalog(settings, resolved, codexProfiles, xaiProfiles), diff --git a/src/exec/runner.ts b/src/exec/runner.ts index 08a1a5bf5..8e92447f1 100644 --- a/src/exec/runner.ts +++ b/src/exec/runner.ts @@ -44,6 +44,7 @@ import { normalizeToolDefinitionsForProvider } from "../agent/tool-schema-normal import { resolveSessionMode, type SessionMode } from "../config/session-mode.js"; import { createSubAgentSessionStore, type SubAgentProvider } from "../subagent/index.js"; import type { InferenceSource, ToolDefinition, InboundMessage } from "@intx/types/runtime"; +import { OPERATOR_ORIGINATED_FLAG } from "../agent/message-provenance.js"; import { createChatDirector } from "../agent/director.js"; import { loadAgentProfiles } from "../agent/profiles.js"; import { createPermissionGate } from "../permission/gate.js"; @@ -111,7 +112,7 @@ export function formatCaughtError(err: unknown): string { } /** Content-less inbound used after compact so the reactor re-enters (matches TUI). */ -function buildCompactionContinuationMessage(): InboundMessage { +export function buildCompactionContinuationMessage(): InboundMessage { return { ref: { uid: 0, mailbox: "system" }, headers: { @@ -126,6 +127,28 @@ function buildCompactionContinuationMessage(): InboundMessage { }; } +/** + * Build the inbound message for exec's one genuine operator input: the + * initial task supplied on the command line. Carries + * OPERATOR_ORIGINATED_FLAG so director.ts's loop-protection backstop can + * tell this apart from system-originated sends. + */ +function operatorTaskMessage(task: string): InboundMessage { + return { + ref: { uid: 1, mailbox: "INBOX" }, + headers: { + from: "user@local", + to: ["agent@local"], + date: new Date().toISOString(), + messageId: `<${crypto.randomUUID()}@local>`, + interchangeType: "conversation.message", + }, + flags: [OPERATOR_ORIGINATED_FLAG], + content: task, + signatureStatus: "missing", + }; +} + export type ExecResult = { exitCode: number; sessionId: string; @@ -633,7 +656,7 @@ export async function runExec(config: Config): Promise { // Stream stays open for multi-turn chat until close() — close first, then // drain, or streamPromise never settles. - await activeAgent.send(task); + await activeAgent.send(operatorTaskMessage(task)); sendCompleted = true; runError = runSink.getRunError(); sinkStatus = runSink.getStatus(); diff --git a/src/index.ts b/src/index.ts index dd5d5d0da..417c7e44e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,7 +4,7 @@ import { primeCrashReporting, writeCrashReport, type CrashKind } from "./crash/r import { getActiveRun, markCrashed } from "./session/active-run.js"; import { getActiveDisposeHost } from "./session/active-host.js"; import { saveCrashState } from "./session/state.js"; -import { loadConfig } from "./config/index.js"; +import { loadConfig, CliHelpError } from "./config/index.js"; import { ensureTelemetrySettings, globalSettingsPath } from "./config/settings.js"; import { installFileLogSink } from "./logging/sink.js"; import { flushPerfToOtel } from "./perf/index.js"; @@ -297,8 +297,14 @@ if (import.meta.main) { try { code = await main(process.argv.slice(2)); } catch (err: unknown) { - process.stderr.write(`${err instanceof Error ? (err.stack ?? err.message) : String(err)}\n`); - code = 1; + // Help is an intentional early exit, not a crash — stdout + 0. + if (err instanceof CliHelpError) { + process.stdout.write(`${err.message}\n`); + code = err.exitCode; + } else { + process.stderr.write(`${err instanceof Error ? (err.stack ?? err.message) : String(err)}\n`); + code = 1; + } } process.exit(code); } diff --git a/src/session/index.ts b/src/session/index.ts index 9c1bccc2f..2aec9ac8e 100644 --- a/src/session/index.ts +++ b/src/session/index.ts @@ -208,6 +208,12 @@ export type SessionSummary = { const SESSION_ID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; +/** True when `value` is a UUID v7 session id Corbits would write on disk. */ +export function isSessionId(value: string): boolean { + return SESSION_ID_RE.test(value); +} + + async function collectSessionIds(cwd: string, home: string): Promise { const ids = new Set(); const roots = [projectSessionsRoot(cwd, home), ...legacySessionRoots(cwd)]; diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 46603d80b..0dd4853f8 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -23,7 +23,7 @@ import { type } from "arktype"; import { createPosixTools } from "@intx/tools-posix"; import { createDynamicToolRunner } from "../tui/dynamic-tool-runner.js"; import type { ReactorEmittedEvent } from "@intx/inference"; -import type { BlobReader } from "@intx/types/runtime"; +import type { BlobReader, InboundMessage } from "@intx/types/runtime"; import { seedPricingMetadataFromCache } from "../cost/pricing-metadata.js"; import { defaultPricingCachePath } from "../cost/pricing-fetcher.js"; @@ -98,6 +98,22 @@ export type { SubAgentSandboxDeps, } from "./types.js"; +/** Content-less inbound used after compact so the reactor re-enters (matches TUI/exec). */ +export function buildCompactionContinuationMessage(): InboundMessage { + return { + ref: { uid: 0, mailbox: "system" }, + headers: { + from: "user@local", + to: ["agent@local"], + date: new Date().toISOString(), + messageId: `compact-continue-${Date.now()}@local`, + }, + flags: [], + content: "", + signatureStatus: "missing", + }; +} + // The source used when no profile tier resolves. Exported for tests: the // parent's provider may need a non-default adapter (Bifrost virtual keys, // Codex or xAI OAuth profiles speak the Responses API and reject plain Chat @@ -367,18 +383,7 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { let agentHandle: Awaited> | null = null; const requestContinuation = (): void => { try { - agentHandle?.deliver({ - ref: { uid: 0, mailbox: "system" }, - headers: { - from: "user@local", - to: ["agent@local"], - date: new Date().toISOString(), - messageId: `compact-continue-${Date.now()}@local`, - }, - flags: [], - content: "", - signatureStatus: "missing", - }); + agentHandle?.deliver(buildCompactionContinuationMessage()); } catch { // Agent may be closing; a dropped continuation is harmless. } diff --git a/src/subagent/stop-policy.test.ts b/src/subagent/stop-policy.test.ts new file mode 100644 index 000000000..4f5a5866e --- /dev/null +++ b/src/subagent/stop-policy.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, test } from "bun:test"; +import { + detectToolFingerprintThrash, + detectTurnsSinceUserMessageBackstop, + TOOL_FINGERPRINT_HISTORY_CAP, + TURNS_SINCE_USER_MESSAGE_BACKSTOP, +} from "./stop-policy.js"; + +describe("detectToolFingerprintThrash", () => { + test("does not flag 4 identical fingerprints — legitimate polling", () => { + const history = ["read_file:{\"path\":\"a.ts\"}", "read_file:{\"path\":\"a.ts\"}", "read_file:{\"path\":\"a.ts\"}", "read_file:{\"path\":\"a.ts\"}"]; + expect(detectToolFingerprintThrash(history).repeating).toBe(false); + }); + + test("flags 5 identical fingerprints", () => { + const history = Array.from({ length: 5 }, () => "read_file:{\"path\":\"a.ts\"}"); + const result = detectToolFingerprintThrash(history); + expect(result).toEqual({ repeating: true, period: 1, repeats: 5 }); + }); + + test("flags an alternating A,B cycle after 3 full cycles", () => { + const history: string[] = []; + for (let i = 0; i < 3; i++) { + history.push("read_file:{\"path\":\"a.ts\"}", "read_file:{\"path\":\"b.ts\"}"); + } + const result = detectToolFingerprintThrash(history); + expect(result).toEqual({ repeating: true, period: 2, repeats: 3 }); + }); + + test("an alternating cycle over 200 turns still resolves to a repeating period", () => { + const history: string[] = []; + for (let i = 0; i < 100; i++) { + history.push("read_file:{\"path\":\"a.ts\"}", "read_file:{\"path\":\"b.ts\"}"); + } + // The director caps its rolling buffer; simulate the same cap here. + const capped = history.slice(-TOOL_FINGERPRINT_HISTORY_CAP); + expect(detectToolFingerprintThrash(capped).repeating).toBe(true); + }); + + test("flags a 3-call rotating cycle", () => { + const history: string[] = []; + for (let i = 0; i < 3; i++) { + history.push( + "read_file:{\"path\":\"a.ts\"}", + "read_file:{\"path\":\"b.ts\"}", + "read_file:{\"path\":\"c.ts\"}", + ); + } + const result = detectToolFingerprintThrash(history); + expect(result).toEqual({ repeating: true, period: 3, repeats: 3 }); + }); + + test("varied, non-repeating history never flags", () => { + const history = Array.from({ length: 40 }, (_, i) => `read_file:{"path":"file-${i}.ts"}`); + expect(detectToolFingerprintThrash(history).repeating).toBe(false); + }); + + // Any period this check scans (up to TOOL_FINGERPRINT_MAX_PERIOD) never + // fires on a rotation longer than that ceiling — this is exactly the gap + // detectTurnsSinceUserMessageBackstop below exists to close. + test("a 9-element rotation never flags, regardless of length", () => { + const paths = Array.from({ length: 9 }, (_, i) => `file-${i}.ts`); + const history = Array.from( + { length: 90 }, + (_, i) => `read_file:{"path":"${paths[i % 9]}"}`, + ); + expect(detectToolFingerprintThrash(history).repeating).toBe(false); + }); +}); + +describe("detectTurnsSinceUserMessageBackstop", () => { + test("does not fire below the threshold", () => { + expect(detectTurnsSinceUserMessageBackstop(TURNS_SINCE_USER_MESSAGE_BACKSTOP - 1)).toBe(false); + }); + + test("fires at the threshold", () => { + expect(detectTurnsSinceUserMessageBackstop(TURNS_SINCE_USER_MESSAGE_BACKSTOP)).toBe(true); + }); + + // Measured turns-since-last-genuine-user-message distribution (a local + // one-off scan, round 4 of CL-5611): p50 5, p90 14, p99 29, max 32. The + // threshold must sit comfortably above the measured max. + test("threshold sits well above the measured healthy run ceiling (max 32 turns)", () => { + expect(TURNS_SINCE_USER_MESSAGE_BACKSTOP).toBeGreaterThan(32 * 2); + }); +}); diff --git a/src/subagent/stop-policy.ts b/src/subagent/stop-policy.ts index 0ef46910c..9a4798aed 100644 --- a/src/subagent/stop-policy.ts +++ b/src/subagent/stop-policy.ts @@ -5,6 +5,7 @@ import type { ReactorEmittedEvent } from "@intx/inference"; import { onTurnBoundary } from "../agent/reactor-events.js"; +import { detectSequencePeriod, type SequencePeriodCheck } from "../util/period-detection.js"; import { evaluateThrashStop, type ThrashConfig, @@ -127,6 +128,124 @@ export function fingerprintToolCalls( return parts.join("|"); } +export type ToolFingerprintThrashCheck = SequencePeriodCheck; + +// No legitimate orchestration pattern needs a longer repeating unit than +// this to be recognized as thrash. A local forensic scan (see +// scripts/tool-fingerprint-forensics.ts) over 328 real session traces (559 +// tool-only runs) found zero cycles of any period 1-6 at all — the scan only +// checks periods up to 6 (MAX_PERIOD_SCANNED in the script), so this ceiling +// has no forensic backing above period 6, only headroom. +// +// This is a ceiling, not a guarantee: any period above it (a 7+ rotation), +// and any "phase-broken" cycle that inserts a varying element between +// otherwise-repeating windows (e.g. A,B,A,B,UNIQUE,A,B,A,B,UNIQUE,...), never +// matches here and can escape period detection indefinitely. That is exactly +// what TURNS_SINCE_USER_MESSAGE_BACKSTOP below exists to catch — a +// turns-since-last-user-message count with no pattern requirement, checked +// as a secondary/final net after period detection has had its chance to +// fire. +const TOOL_FINGERPRINT_MAX_PERIOD = 8; + +// A truly identical consecutive tool call (period 1) is the one shape a +// legitimate agent can plausibly produce on purpose — rerunning a flaky +// test, polling a build. The forensic scan found zero occurrences of even +// two consecutive identical fingerprints in local trace history (a stronger +// result than CL-5611's original "zero 3+" finding), so there is no +// *measured* floor for legitimate period-1 repetition — this threshold is +// inferred headroom for that plausible-but-unobserved case, and deliberately +// set above 4: review on CL-5611 found the previous 4-repeat hard pause +// false-positived on exactly this kind of legitimate polling. +const IDENTICAL_REPEAT_MIN = 5; + +// Any cycle of length 2+ (A,B,A,B,..., A,B,C,A,B,C,...) has no plausible +// legitimate justification — nobody deliberately re-issues a *different* +// tool call with identical arguments in a fixed rotation. Fire fast: three +// full cycles, per the operator's explicit "trigger fairly quickly" target +// (A,B,A,B,A,B pauses at 6 turns; A,B,C,A,B,C,A,B,C at 9), still comfortably +// above the observed healthy ceiling of zero. +const CYCLE_REPEAT_MIN = 3; + +/** + * Thrash check over a rolling history of consecutive tool-only-turn + * fingerprints, via exact-period detection (detectSequencePeriod in + * util/period-detection.ts). Generalizes the old consecutive-identical-only + * check to catch any repeating cycle — A,A,A,..., A,B,A,B,..., A,B,C,A,B,C,... + * — not just immediate repeats, which previously let an alternating A,B + * pattern escape detection at any length. See docs/ARCHITECTURE.md for the + * forensic basis of the thresholds. + * + * This is the fast path, not the only path: TOOL_FINGERPRINT_MAX_PERIOD is a + * ceiling, so a cycle above it (or a phase-broken cycle that never settles + * into an exact repeating tail) never fires here. + * detectTurnsSinceUserMessageBackstop below is the final net for those cases. + */ +export function detectToolFingerprintThrash( + history: readonly string[], +): ToolFingerprintThrashCheck { + return detectSequencePeriod(history, { + minPeriod: 1, + maxPeriod: TOOL_FINGERPRINT_MAX_PERIOD, + minRepeats: (period) => (period === 1 ? IDENTICAL_REPEAT_MIN : CYCLE_REPEAT_MIN), + minDistinct: (period) => (period === 1 ? 1 : 2), + }); +} + +// Secondary/final-net check: how long it has been since the operator last +// sent a genuine message, independent of whether the intervening turns form +// a detectable pattern or contain narration. Period detection (above) is the +// fast path and stays primary — it fires well before this on any cycle it +// can see (A,B at 6 turns, A,B,C at 9). This backstop exists for what period +// detection structurally cannot see: any period above +// TOOL_FINGERPRINT_MAX_PERIOD (e.g. a 9-element rotation), "phase-broken" +// cycles that insert a varying element between repeats (e.g. +// A,B,A,B,UNIQUE,A,B,A,B,UNIQUE,...) that never settle into an exact +// repeating tail at any period, and — the round-4 fix — a model that inserts +// one narrated word every N tool-only turns purely to keep resetting a +// narration-sensitive counter. Model-emitted text does not reset this +// counter; only a genuine user/operator message does (see director.ts). That +// is deliberate: this answers "how long since the operator last saw a real +// checkpoint," not "is the model narrating." +// +// Because narration no longer resets it, reaching this threshold does not +// hard-pause on its own — it only fires a nudge asking for a progress +// summary. Only if the nudge goes unheeded for a further full interval (see +// director.ts's turnsSinceUserMessage escalation) does the session hard +// pause, on the theory that ignoring a direct request is a real no-progress +// signal, whereas mere silence during a long autonomous stretch is not. +// +// Threshold justification: 100 is a judgment call, not a measured value. +// turns-since-last-genuine-operator-message was never separately measured — +// an earlier round of this PR cited a scan of it ("358-session/428-run", +// then "428 runs" in a later revision, the two numbers already disagreeing +// with each other) that has no corresponding script or output anywhere in +// the tree. That claim was fabricated and is retracted; do not restate it. +// +// The only real measurement we have is scripts/tool-fingerprint-forensics.ts, +// which measures a related but different quantity — consecutive +// tool-only-turn streaks, reset by narration — p50 3, p90 8, p99 16, max 28 +// across 328 local sessions with a tool-only run. It is not directly +// applicable here since narration does not reset this counter, but it is +// the only forensic data point available, and 100 sits well above every +// percentile of it, which is the informal basis for treating 100 as +// generous headroom. Revisit if this backstop turns out to fire during +// legitimate long autonomous stretches, or if turns-since-user-message is +// ever actually measured. +export const TURNS_SINCE_USER_MESSAGE_BACKSTOP = 100; + +/** True once turns-since-last-user-message reaches the backstop threshold. */ +export function detectTurnsSinceUserMessageBackstop(turnsSinceUserMessage: number): boolean { + return turnsSinceUserMessage >= TURNS_SINCE_USER_MESSAGE_BACKSTOP; +} + +// Bounds the rolling fingerprint buffer director.ts keeps for the thrash +// check above. Detection only ever looks at the tail, so history older than +// the longest possible confirming window (max period * max repeats-needed) +// carries no signal — capping keeps a very long productive tool-only streak +// (e.g. 200+ turns) from growing the buffer or the per-turn scan unbounded. +export const TOOL_FINGERPRINT_HISTORY_CAP = + TOOL_FINGERPRINT_MAX_PERIOD * IDENTICAL_REPEAT_MIN; + export type SubAgentStopReason = | "complete" | "turn-budget" diff --git a/src/telemetry/ai-observability.test.ts b/src/telemetry/ai-observability.test.ts index 38d87e322..fa4a0df0f 100644 --- a/src/telemetry/ai-observability.test.ts +++ b/src/telemetry/ai-observability.test.ts @@ -19,9 +19,11 @@ function fakeTelemetry(): { telemetry: Telemetry; captured: { event: string; pro const captured: { event: string; properties: Record }[] = []; const telemetry: Telemetry = { enabled: true, + installationId: "test-install", capture: (event, properties = {}) => { captured.push({ event, properties }); }, + captureIntentional: () => false, flush: async () => {}, discard: () => {}, }; @@ -223,6 +225,25 @@ describe("emitAiObservability", () => { expect(generation?.properties).not.toHaveProperty("duration_ms"); }); + // CL-5749: PostHog cost views read only $ai_*-prefixed cache/reasoning + // properties. Unprefixed names land as custom fields and skew spend. + // Source: https://posthog.com/docs/ai-observability/installation/manual-capture + // and PostHog cost-properties reference ($ai_cache_read_input_tokens, + // $ai_cache_creation_input_tokens, $ai_reasoning_tokens). + test("names cache and reasoning token properties for PostHog cost views", () => { + const { telemetry, captured } = fakeTelemetry(); + + emitAiObservability(telemetry, fakeTurnContext(), emitOptions); + + const generation = captured.find((c) => c.event === "$ai_generation"); + expect(generation?.properties.$ai_cache_read_input_tokens).toBe(1); + expect(generation?.properties.$ai_cache_creation_input_tokens).toBe(2); + expect(generation?.properties.$ai_reasoning_tokens).toBe(3); + expect(generation?.properties).not.toHaveProperty("cache_read_tokens"); + expect(generation?.properties).not.toHaveProperty("cache_write_tokens"); + expect(generation?.properties).not.toHaveProperty("thinking_tokens"); + }); + test("flat trace: spans parent onto the trace id, not onto each other", () => { const { telemetry, captured } = fakeTelemetry(); const ctx = fakeTurnContext(); diff --git a/src/telemetry/ai-observability.ts b/src/telemetry/ai-observability.ts index 84882f463..c5e3a3701 100644 --- a/src/telemetry/ai-observability.ts +++ b/src/telemetry/ai-observability.ts @@ -5,8 +5,10 @@ // the scalar/id fields off it and never the content fields. import type { TurnContext } from "../session/hooks.js"; +import { noteLastTurnTraceId } from "./feedback.js"; import type { AiErrorKind, AiSpanKind, Telemetry } from "./index.js"; + // PostHog reports latency in seconds as a float; the runtime measures every // duration in milliseconds. Reporting milliseconds under the seconds-typed // property inflates every latency by 1000x and still renders plausibly. @@ -75,6 +77,9 @@ export function emitAiObservability( options: EmitAiObservabilityOptions, ): void { const traceId = turnTraceId(options.sessionId, ctx.turnIndex); + // Remember for intentional /feedback linking (works even when ambient capture + // is a no-op because this call still computes the id). + noteLastTurnTraceId(traceId); telemetry.capture("$ai_generation", { $ai_trace_id: traceId, @@ -87,9 +92,9 @@ export function emitAiObservability( $ai_output_tokens: ctx.usage.output, $ai_latency: secondsFromMs(ctx.durationMs), $ai_is_error: false, - cache_read_tokens: ctx.usage.cacheRead, - cache_write_tokens: ctx.usage.cacheWrite, - thinking_tokens: ctx.usage.thinking, + $ai_cache_read_input_tokens: ctx.usage.cacheRead, + $ai_cache_creation_input_tokens: ctx.usage.cacheWrite, + $ai_reasoning_tokens: ctx.usage.thinking, }); const resultsByCallId = new Map(ctx.toolResults.map((result) => [result.callId, result])); diff --git a/src/telemetry/classify.ts b/src/telemetry/classify.ts index ad39d4b0e..5c08598a0 100644 --- a/src/telemetry/classify.ts +++ b/src/telemetry/classify.ts @@ -45,6 +45,7 @@ const BUILT_IN_COMMAND_NAMES: ReadonlySet = new Set([ "changelog", "clear", "cost", + "feedback", "goal", "help", "hooks", @@ -56,8 +57,10 @@ const BUILT_IN_COMMAND_NAMES: ReadonlySet = new Set([ "plugins", "rename", "settings", + "status", ]); + // The one agent label the runtime supplies itself; every other profile id // comes from a project or plugin directory. const BUILT_IN_AGENT_NAME = "worker"; diff --git a/src/telemetry/feedback.test.ts b/src/telemetry/feedback.test.ts new file mode 100644 index 000000000..79dadde52 --- /dev/null +++ b/src/telemetry/feedback.test.ts @@ -0,0 +1,195 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { createTelemetry, type Telemetry } from "./index.js"; +import { + armFeedbackCapture, + buildSurveyProperties, + cancelFeedbackCapture, + capFeedbackMessage, + captureFeedback, + FEEDBACK_MAX_CHARS, + feedbackResultMessage, + getLastTurnTraceId, + isFeedbackCapturePending, + noteLastTurnTraceId, + resetFeedbackStateForTests, + takeFeedbackCapture, +} from "./feedback.js"; + +afterEach(() => { + resetFeedbackStateForTests(); +}); + +const noopFetch = (async () => new Response("{}", { status: 200 })) as unknown as typeof fetch; + +function captureSpy(): { + telemetry: Telemetry; + events: Array<{ event: string; properties: Record }>; +} { + const events: Array<{ event: string; properties: Record }> = []; + // Ambient off, intentional on — the critical /feedback contract. + const telemetry = createTelemetry({ + settings: { + providers: {}, + telemetry: { enabled: false, installationId: "install-test-1" }, + }, + // Pin env so a developer's DO_NOT_TRACK / CORBITS_TELEMETRY never bleeds in. + env: {}, + apiKey: "phc_test", + batch: { size: 100, intervalMs: 60_000, queueLimit: 100 }, + fetchFn: noopFetch, + }); + const original = telemetry.captureIntentional.bind(telemetry); + telemetry.captureIntentional = (event, properties) => { + const ok = original(event, properties); + if (ok) events.push({ event, properties: properties ?? {} }); + return ok; + }; + return { telemetry, events }; +} + +describe("capFeedbackMessage", () => { + test("leaves short messages alone", () => { + expect(capFeedbackMessage("hello")).toBe("hello"); + }); + + test("truncates at the free-text cap", () => { + const long = "x".repeat(FEEDBACK_MAX_CHARS + 50); + expect(capFeedbackMessage(long).length).toBe(FEEDBACK_MAX_CHARS); + }); +}); + +describe("buildSurveyProperties", () => { + test("shapes PostHog custom survey properties", () => { + const props = buildSurveyProperties("ship it", { + turnTraceId: "trace-1", + }); + expect(props.$survey_id).toBe("019fe7ff-d12a-0000-7a63-303f3a874b90"); + expect(props.$survey_response).toBe("ship it"); + expect(props.turn_trace_id).toBe("trace-1"); + expect(props.$survey_questions).toEqual([ + { + id: "913862f4-82aa-4814-8f68-146c05c38a74", + question: "What feedback do you have about Corbits Code?", + response: "ship it", + }, + ]); + }); + + test("env override can blank the survey ids", () => { + const props = buildSurveyProperties("x", { + env: { + CORBITS_FEEDBACK_SURVEY_ID: "", + CORBITS_FEEDBACK_QUESTION_ID: "", + }, + }); + expect(props.$survey_id).toBe(""); + }); +}); + +describe("captureFeedback", () => { + test("sends survey sent when ambient telemetry is off", () => { + const { telemetry, events } = captureSpy(); + expect(telemetry.enabled).toBe(false); + const status = captureFeedback(telemetry, "great product"); + expect(status).toBe("sent"); + expect(events).toHaveLength(1); + expect(events[0]?.event).toBe("survey sent"); + expect(events[0]?.properties.$survey_response).toBe("great product"); + expect(events[0]?.properties.$survey_id).toBe("019fe7ff-d12a-0000-7a63-303f3a874b90"); + }); + + test("rejects empty text", () => { + const { telemetry, events } = captureSpy(); + expect(captureFeedback(telemetry, " ")).toBe("empty"); + expect(events).toHaveLength(0); + }); + + test("blocks when install identity is missing", () => { + const telemetry = createTelemetry({ + settings: { providers: {}, telemetry: { enabled: false } }, + env: {}, + apiKey: "phc_test", + batch: { size: 100, intervalMs: 60_000, queueLimit: 100 }, + fetchFn: noopFetch, + }); + expect(captureFeedback(telemetry, "hi")).toBe("blocked"); + }); + + test("blocks under env kill switch even with identity", () => { + const telemetry = createTelemetry({ + settings: { + providers: {}, + telemetry: { enabled: true, installationId: "install-1" }, + }, + env: { CORBITS_TELEMETRY: "0" }, + apiKey: "phc_test", + batch: { size: 100, intervalMs: 60_000, queueLimit: 100 }, + fetchFn: noopFetch, + }); + expect( + captureFeedback(telemetry, "hi", { + env: { CORBITS_TELEMETRY: "0" }, + }), + ).toBe("blocked"); + }); + + test("fails closed when survey ids are blanked via env", () => { + const { telemetry, events } = captureSpy(); + expect( + captureFeedback(telemetry, "hi", { + env: { + CORBITS_FEEDBACK_SURVEY_ID: "", + CORBITS_FEEDBACK_QUESTION_ID: "", + }, + }), + ).toBe("unconfigured"); + expect(events).toHaveLength(0); + }); + + test("reports truncation when free text exceeds the cap", () => { + const { telemetry, events } = captureSpy(); + const long = "x".repeat(FEEDBACK_MAX_CHARS + 50); + const status = captureFeedback(telemetry, long); + expect(status).toBe("sent_truncated"); + expect(events).toHaveLength(1); + expect(String(events[0]?.properties.$survey_response).length).toBe(FEEDBACK_MAX_CHARS); + }); + + test("rejects non-survey events on the intentional door", () => { + const { telemetry, events } = captureSpy(); + expect(telemetry.captureIntentional("cli_start")).toBe(false); + expect(events).toHaveLength(0); + }); +}); + +describe("feedbackResultMessage", () => { + test("maps statuses to operator-facing lines", () => { + expect(feedbackResultMessage("sent")).toBe("Thanks — feedback sent."); + expect(feedbackResultMessage("sent_truncated")).toContain("truncated"); + expect(feedbackResultMessage("blocked")).toContain("could not be sent"); + expect(feedbackResultMessage("unconfigured")).toContain("not configured"); + expect(feedbackResultMessage("empty")).toContain("No feedback"); + }); +}); + +describe("pending multi-turn capture", () => { + test("arm → take consumes once", () => { + expect(isFeedbackCapturePending()).toBe(false); + armFeedbackCapture(); + expect(isFeedbackCapturePending()).toBe(true); + expect(takeFeedbackCapture()).toBe(true); + expect(isFeedbackCapturePending()).toBe(false); + expect(takeFeedbackCapture()).toBe(false); + }); + + test("cancel clears pending", () => { + armFeedbackCapture(); + cancelFeedbackCapture(); + expect(isFeedbackCapturePending()).toBe(false); + }); + + test("remembers last turn trace id", () => { + noteLastTurnTraceId("sess:3"); + expect(getLastTurnTraceId()).toBe("sess:3"); + }); +}); diff --git a/src/telemetry/feedback.ts b/src/telemetry/feedback.ts new file mode 100644 index 000000000..86562e98a --- /dev/null +++ b/src/telemetry/feedback.ts @@ -0,0 +1,181 @@ +// Intentional operator feedback via PostHog custom surveys (headless). +// +// Unlike ambient product events, this path can ship when settings.telemetry.enabled +// is false — the operator typed the text for that purpose. Env kill switches +// (DO_NOT_TRACK / CORBITS_TELEMETRY=0) still block send. + +import type { Telemetry } from "./index.js"; + +/** Free-text cap for /feedback responses. */ +export const FEEDBACK_MAX_CHARS = 2000; + +export const FEEDBACK_PROMPT = + "Please share your feedback. When done please hit enter. (Empty Enter cancels.)"; + +export const FEEDBACK_THANKS = "Thanks — feedback sent."; + +export const FEEDBACK_THANKS_TRUNCATED = + "Thanks — feedback sent (truncated to 2000 characters)."; + +export const FEEDBACK_EMPTY = "No feedback text provided."; + +export const FEEDBACK_BLOCKED = + "Feedback could not be sent (disabled by environment or missing install identity)."; + +export const FEEDBACK_UNCONFIGURED = + "Feedback is not configured (missing survey id)."; + +/** + * Corbits team survey — public routing ids (same trust class as the baked-in + * PostHog project key). Operators never set these. Env override is for tests + * and forks: when the env key is present (even empty), it wins over the default. + */ +export const DEFAULT_FEEDBACK_SURVEY_ID = "019fe7ff-d12a-0000-7a63-303f3a874b90"; +export const DEFAULT_FEEDBACK_QUESTION_ID = "913862f4-82aa-4814-8f68-146c05c38a74"; +export const FEEDBACK_QUESTION_TEXT = "What feedback do you have about Corbits Code?"; + +function envOverride(env: NodeJS.ProcessEnv, key: string): string | undefined { + // Present key wins (including empty → fail closed for tests/forks). + if (!Object.prototype.hasOwnProperty.call(env, key)) return undefined; + return (env[key] ?? "").trim(); +} + +/** PostHog survey id for /feedback. */ +export function feedbackSurveyId(env: NodeJS.ProcessEnv = process.env): string { + return envOverride(env, "CORBITS_FEEDBACK_SURVEY_ID") ?? DEFAULT_FEEDBACK_SURVEY_ID; +} + +/** Free-text question id inside the survey. */ +export function feedbackQuestionId(env: NodeJS.ProcessEnv = process.env): string { + return envOverride(env, "CORBITS_FEEDBACK_QUESTION_ID") ?? DEFAULT_FEEDBACK_QUESTION_ID; +} + +/** True when both survey ids resolve (defaults always do unless env blanks them). */ +export function isFeedbackConfigured(env: NodeJS.ProcessEnv = process.env): boolean { + return feedbackSurveyId(env).length > 0 && feedbackQuestionId(env).length > 0; +} + +export function capFeedbackMessage(message: string): string { + if (message.length <= FEEDBACK_MAX_CHARS) return message; + return message.slice(0, FEEDBACK_MAX_CHARS); +} + +/** PostHog custom-survey property bag for a free-text response. */ +export function buildSurveyProperties( + message: string, + options: { + turnTraceId?: string | undefined; + env?: NodeJS.ProcessEnv; + } = {}, +): Record { + const env = options.env ?? process.env; + const capped = capFeedbackMessage(message); + const surveyId = feedbackSurveyId(env); + const questionId = feedbackQuestionId(env); + const props: Record = { + $survey_id: surveyId, + $survey_response: capped, + $survey_questions: [ + { + id: questionId, + question: FEEDBACK_QUESTION_TEXT, + response: capped, + }, + ], + }; + if (options.turnTraceId !== undefined && options.turnTraceId.length > 0) { + props.turn_trace_id = options.turnTraceId; + } + return props; +} + +/** + * Capture intentional survey response. Empty/whitespace-only text is not sent. + * Missing survey/question ids fail closed. On success the event is enqueued and + * flushed immediately (fire-and-forget) — not held for the ambient batch timer. + * Status "sent" means the capture path accepted the payload; delivery is best- + * effort over the network and is not awaited on the operator path. + */ +export function captureFeedback( + telemetry: Telemetry, + message: string, + options: { + turnTraceId?: string | undefined; + env?: NodeJS.ProcessEnv; + } = {}, +): "empty" | "blocked" | "unconfigured" | "sent" | "sent_truncated" { + const trimmed = message.trim(); + if (trimmed.length === 0) return "empty"; + const env = options.env ?? process.env; + if (!isFeedbackConfigured(env)) { + return "unconfigured"; + } + const truncated = trimmed.length > FEEDBACK_MAX_CHARS; + const ok = telemetry.captureIntentional( + "survey sent", + buildSurveyProperties(trimmed, options), + ); + if (!ok) return "blocked"; + // Deterministic handoff to PostHog — not part of the agent loop. Flush so + // the response is not sitting in the ambient batch queue until idle exit. + void telemetry.flush(); + return truncated ? "sent_truncated" : "sent"; +} + +export function feedbackResultMessage( + status: "empty" | "blocked" | "unconfigured" | "sent" | "sent_truncated", +): string { + switch (status) { + case "sent": + return FEEDBACK_THANKS; + case "sent_truncated": + return FEEDBACK_THANKS_TRUNCATED; + case "blocked": + return FEEDBACK_BLOCKED; + case "unconfigured": + return FEEDBACK_UNCONFIGURED; + case "empty": + return FEEDBACK_EMPTY; + } +} + +// ── Pending multi-turn capture (bare `/feedback` then next Enter) ────────── + +let feedbackCapturePending = false; +let lastTurnTraceId: string | undefined; + +/** Arm after bare `/feedback` so the next non-command submit is treated as feedback. */ +export function armFeedbackCapture(): void { + feedbackCapturePending = true; +} + +export function isFeedbackCapturePending(): boolean { + return feedbackCapturePending; +} + +/** Consume the pending flag. Returns true only once per arm. */ +export function takeFeedbackCapture(): boolean { + if (!feedbackCapturePending) return false; + feedbackCapturePending = false; + return true; +} + +/** Cancel pending capture (e.g. on /clear). */ +export function cancelFeedbackCapture(): void { + feedbackCapturePending = false; +} + +/** Remember the most recent AI turn trace for linking feedback. */ +export function noteLastTurnTraceId(traceId: string): void { + if (traceId.length > 0) lastTurnTraceId = traceId; +} + +export function getLastTurnTraceId(): string | undefined { + return lastTurnTraceId; +} + +/** Test helper — reset module state between cases. */ +export function resetFeedbackStateForTests(): void { + feedbackCapturePending = false; + lastTurnTraceId = undefined; +} diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index a947bc9ed..70e60822c 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -40,7 +40,7 @@ export type BatchTuning = { // first: the onboarding panel on a fresh install (so disclosure accompanies // the very first event), and the TUI banner otherwise. export const TELEMETRY_NOTICE = - "Anonymous usage telemetry is enabled (no prompts, code, or paths collected). Disable in /settings > Telemetry. Docs: docs/TELEMETRY.md"; + "Anonymous usage telemetry is enabled (no prompts, code, or paths collected). Free text only leaves via /feedback if you send it. Disable ambient events in /settings > Telemetry; DO_NOT_TRACK / CORBITS_TELEMETRY=0 blocks all telemetry including feedback. Docs: docs/TELEMETRY.md"; export type TelemetryEvent = | "cli_start" @@ -55,7 +55,12 @@ export type TelemetryEvent = | "permission_prompt" | "compaction" | "crash" - | "auth_failure"; + | "auth_failure" + // PostHog Surveys event name (space included). Intentional operator feedback + // from /feedback — can ship when ambient product telemetry is off; still + // blocked by env kill switches. See captureIntentional. + | "survey sent"; + // Fixed enum of AI observability span names. The raw tool name is never sent // as a property: an MCP tool name carries the server identifier it was @@ -101,10 +106,11 @@ const EVENT_PROPERTY_ALLOWLIST: Record = { // name a user gave a provider in onboarding or settings. $ai_latency is // in seconds, per PostHog's schema. // - // The cache and thinking token counts stay on our own names: PostHog - // documents cost inputs for them but does not publish the property names - // in the manual-capture schema, and guessing a name that lands as an - // unread custom property is worse than owning one we can read ourselves. + // Cache and reasoning token counts use PostHog's documented cost-property + // names (manual-capture installation + cost-properties reference): + // $ai_cache_read_input_tokens, $ai_cache_creation_input_tokens, + // $ai_reasoning_tokens. Unprefixed names land as custom properties and + // are invisible to cost/token views (CL-5749). $ai_generation: [ "$ai_trace_id", "$ai_provider", @@ -114,9 +120,9 @@ const EVENT_PROPERTY_ALLOWLIST: Record = { "$ai_latency", "$ai_is_error", "$ai_error", - "cache_read_tokens", - "cache_write_tokens", - "thinking_tokens", + "$ai_cache_read_input_tokens", + "$ai_cache_creation_input_tokens", + "$ai_reasoning_tokens", ], // The trace is flat: every span's $ai_parent_id is the turn's // $ai_trace_id. PostHog documents $ai_parent_id as accepting a trace id or @@ -143,8 +149,18 @@ const EVENT_PROPERTY_ALLOWLIST: Record = { // Which provider rejected the credentials, not why — the rejection detail is // provider-authored text and error_class means a JS constructor name. auth_failure: ["auth_provider"], + // Intentional /feedback survey response (PostHog custom survey capture shape). + // Free text is only sent because the operator typed it for that purpose. + // turn_trace_id links to the last $ai_generation in this session when known. + "survey sent": [ + "$survey_id", + "$survey_questions", + "$survey_response", + "turn_trace_id", + ], }; + const FALSY_ENV_FLAG_VALUES = new Set(["", "0", "false", "off", "no"]); // Trimmed so .env files and shell scripts that produce " 0" or "false\n" @@ -212,7 +228,22 @@ type QueuedEvent = { export type Telemetry = { enabled: boolean; + /** + * Installation distinct id used as PostHog `distinct_id`. Empty when the + * instance has no identity (held first-run no-op, or never generated). + * Exposed so ambient opt-out can preserve identity for intentional capture. + */ + installationId: string; capture(event: TelemetryEvent, properties?: Record): void; + /** + * Intentional capture that can run when ambient product telemetry is off + * (`settings.telemetry.enabled === false`). Only `"survey sent"` is accepted — + * this is not a second ambient path. Still blocked by env kill switches + * (`DO_NOT_TRACK`, `CORBITS_TELEMETRY=0`), a missing installation id, or a + * missing API key. Does not re-enable ambient events. + * @returns true when the event was queued for send + */ + captureIntentional(event: TelemetryEvent, properties?: Record): boolean; // Sends whatever is queued and waits briefly for it to settle, giving up // after a short deadline so a slow endpoint can never hold up process // exit. Callers use this to bound exit against dropped fire-and-forget @@ -226,6 +257,8 @@ export type Telemetry = { discard(): void; }; + + // Stand-in for callers that were constructed without a telemetry handle — // tests, and any code path that runs before startup has built the real one. // Modules take Telemetry as an injected dependency rather than reaching for a @@ -233,11 +266,15 @@ export type Telemetry = { // of "throws". export const NOOP_TELEMETRY: Telemetry = { enabled: false, + installationId: "", capture: () => {}, + captureIntentional: () => false, flush: async () => {}, discard: () => {}, }; + + // Fire-and-forget PostHog batch client. Never throws, never blocks the // caller — errors (including timeouts) are swallowed silently since // telemetry must never affect product behavior. @@ -248,6 +285,11 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { const enabled = resolveTelemetryEnabled(options.settings, env, apiKey); const fetchFn = options.fetchFn ?? fetch; const installationId = options.settings?.telemetry?.installationId ?? ""; + // Intentional events (operator /feedback) may ship when ambient is + // settings-disabled, but never when env kill switches fire or identity/key + // is missing. Does not re-enable ambient capture. + const intentionalEnabled = + !telemetryDisabledByEnv(env) && apiKey.length > 0 && installationId.length > 0; const batchSize = options.batch?.size ?? DEFAULT_BATCH_SIZE; const batchIntervalMs = options.batch?.intervalMs ?? DEFAULT_BATCH_INTERVAL_MS; @@ -300,8 +342,7 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { return running; } - function capture(event: TelemetryEvent, properties?: Record): void { - if (!enabled) return; + function enqueue(event: TelemetryEvent, properties?: Record): void { // Own-property only: `in` walks Object.prototype, so capture("toString") // or capture("constructor") would clear the guard this exists to be and // hand allowedProperties a function where it expects an allowlist array. @@ -339,6 +380,20 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { } } + function capture(event: TelemetryEvent, properties?: Record): void { + if (!enabled) return; + enqueue(event, properties); + } + + function captureIntentional(event: TelemetryEvent, properties?: Record): boolean { + // One intentional door: free-text survey only. Ambient product/AI events + // must never ride the ambient-bypass path. + if (event !== "survey sent") return false; + if (!intentionalEnabled) return false; + enqueue(event, properties); + return true; + } + async function flush(): Promise { cancelTimer(); if (queue.length === 0 && inFlight === null) return; @@ -360,5 +415,5 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { queue.length = 0; } - return { enabled, capture, flush, discard }; + return { enabled, installationId, capture, captureIntentional, flush, discard }; } diff --git a/src/telemetry/product-events.ts b/src/telemetry/product-events.ts new file mode 100644 index 000000000..37ae53cb4 --- /dev/null +++ b/src/telemetry/product-events.ts @@ -0,0 +1,12 @@ +// Shared product-event emitters that every surface (TUI, exec, future +// headless) must call so dashboards are not silently TUI-only. + +import type { Telemetry } from "./index.js"; +import { classifyCommandName } from "./classify.js"; + +/** Emit slash_command with a classified first-party (or `custom`) name. */ +export function captureSlashCommand(telemetry: Telemetry, commandName: string): void { + telemetry.capture("slash_command", { + command_name: classifyCommandName(commandName), + }); +} diff --git a/src/telemetry/singleton.ts b/src/telemetry/singleton.ts index dc1fce6e2..6777ff4f9 100644 --- a/src/telemetry/singleton.ts +++ b/src/telemetry/singleton.ts @@ -24,7 +24,11 @@ export const liveTelemetry: Telemetry = { get enabled() { return instance.enabled; }, + get installationId() { + return instance.installationId; + }, capture: (event, properties) => instance.capture(event, properties), + captureIntentional: (event, properties) => instance.captureIntentional(event, properties), flush: () => instance.flush(), discard: () => instance.discard(), }; diff --git a/src/telemetry/toggle.ts b/src/telemetry/toggle.ts index 31961022e..53e483bcf 100644 --- a/src/telemetry/toggle.ts +++ b/src/telemetry/toggle.ts @@ -39,13 +39,13 @@ const defaultDeps: TelemetryToggleDeps = { // Builds the /settings > Telemetry toggle handler, bound to the true global // settings path (never a --config override — see index.ts / runner.ts for -// why). Returned as a plain function so it can be wired into onChange props -// without an inline closure, and so tests can call it directly with fake deps. +// why). Returns whether the requested value was accepted so the UI can refuse +// a flip that would be a silent no-op (env kill switch). export function createTelemetryToggleHandler( globalSettingsPath: string, deps: TelemetryToggleDeps = defaultDeps, -): (enabled: boolean) => void { - return (enabled: boolean): void => { +): (enabled: boolean) => boolean { + return (enabled: boolean): boolean => { if (enabled && deps.telemetryDisabledByEnv()) { // Env kills own the "disabled means no settings writes" constraint // (see index.ts); honoring the enable here would generate and persist @@ -54,7 +54,7 @@ export function createTelemetryToggleHandler( logger.warn( `Telemetry re-enable ignored: disabled by environment (DO_NOT_TRACK or ${TELEMETRY_ENV})`, ); - return; + return false; } if (!enabled) { // Opt-out must be immediate and absolute: discard whatever the outgoing @@ -64,9 +64,23 @@ export function createTelemetryToggleHandler( // during the persistence step below can land on a still-enabled // instance, and so an unhandled rejection from disk I/O can never // leave telemetry on. - deps.getTelemetry().discard(); + // + // Preserve installationId so intentional /feedback can still ship while + // ambient product events are off. Env kill switches remain the hard stop. + const previous = deps.getTelemetry(); + previous.discard(); deps.setTelemetry( - deps.createTelemetry({ settings: { providers: {}, telemetry: { enabled: false } } }), + deps.createTelemetry({ + settings: { + providers: {}, + telemetry: { + enabled: false, + ...(previous.installationId.length > 0 + ? { installationId: previous.installationId } + : {}), + }, + }, + }), ); } @@ -92,8 +106,25 @@ export function createTelemetryToggleHandler( // file is readable again. return; } + const previous = deps.getTelemetry(); const base: Settings = current ?? { providers: {} }; - const next: Settings = { ...base, telemetry: { ...base.telemetry, enabled } }; + // Keep installation identity across ambient opt-out so /feedback still + // works. Prefer the on-disk id; fall back to the live instance when the + // disk row never got one (first-run race). + const installationId = + (typeof base.telemetry?.installationId === "string" && + base.telemetry.installationId.length > 0 + ? base.telemetry.installationId + : undefined) ?? + (previous.installationId.length > 0 ? previous.installationId : undefined); + const next: Settings = { + ...base, + telemetry: { + ...base.telemetry, + enabled, + ...(installationId !== undefined ? { installationId } : {}), + }, + }; try { await deps.saveGlobalSettings(globalSettingsPath, next); } catch (err) { @@ -106,5 +137,6 @@ export function createTelemetryToggleHandler( // on the load-succeeded path, carry forward the real installationId). deps.setTelemetry(deps.createTelemetry({ settings: next })); })(); + return true; }; } diff --git a/src/tui/command-surfaces.ts b/src/tui/command-surfaces.ts index 7ff75ea80..77412d713 100644 --- a/src/tui/command-surfaces.ts +++ b/src/tui/command-surfaces.ts @@ -358,8 +358,9 @@ function settingsCycleRows( value: `${"telemetry".padEnd(SETTINGS_NAME_WIDTH)}${cycleField(ON_OFF_OPTIONS, snapshot.telemetryEnabled ? "on" : "off")}`, chosenLabel: activeOptionLabel(ON_OFF_OPTIONS, snapshot.telemetryEnabled ? "on" : "off"), describe: { - what: "anonymous usage data shared to help improve corbits.", - impact: "off stops all telemetry from this session.", + what: "anonymous ambient usage data (product events and AI traces). Free text only leaves via /feedback if you send it.", + impact: + "off stops ambient telemetry for this session. /feedback still works unless DO_NOT_TRACK or CORBITS_TELEMETRY=0 is set.", tone: "consequence", }, cycle: () => settings.setTelemetryEnabled(!snapshot.telemetryEnabled), diff --git a/src/tui/commands/built-in.test.ts b/src/tui/commands/built-in.test.ts index f95fc9c35..8b6f31c26 100644 --- a/src/tui/commands/built-in.test.ts +++ b/src/tui/commands/built-in.test.ts @@ -150,3 +150,54 @@ describe("/cost command", () => { expect((result as { text: string }).text).toContain("Cost: $0.4200"); }); }); + +describe("/feedback command", () => { + it("is registered", () => { + expect(getCommand("feedback")).toBeDefined(); + }); + + it("arms multi-turn capture when invoked bare", () => { + let armed = false; + const ctx: CommandContext = { + signalClear: () => {}, + beginFeedbackCapture: () => { + armed = true; + }, + }; + expect(getCommand("feedback")!.handler("", ctx)).toEqual({ + type: "message", + text: "Please share your feedback. When done please hit enter. (Empty Enter cancels.)", + }); + expect(armed).toBe(true); + }); + + it("submits inline text immediately", () => { + const sent: string[] = []; + const ctx: CommandContext = { + signalClear: () => {}, + submitFeedback: (text) => { + sent.push(text); + return "Thanks — feedback sent."; + }, + }; + expect(getCommand("feedback")!.handler("love the TUI", ctx)).toEqual({ + type: "message", + text: "Thanks — feedback sent.", + }); + expect(sent).toEqual(["love the TUI"]); + }); + + it("fails closed for bare /feedback when capture is not wired", () => { + expect(getCommand("feedback")!.handler("", makeCtx())).toEqual({ + type: "message", + text: "Feedback is not available in this mode.", + }); + }); + + it("explains when the feedback path is not wired", () => { + expect(getCommand("feedback")!.handler("x", makeCtx())).toEqual({ + type: "message", + text: "Feedback is not available in this mode.", + }); + }); +}); diff --git a/src/tui/commands/built-in.ts b/src/tui/commands/built-in.ts index ebeb4f3fc..fb336099d 100644 --- a/src/tui/commands/built-in.ts +++ b/src/tui/commands/built-in.ts @@ -5,6 +5,7 @@ import { parseChangelog, resolveChangelogPath, } from "../../changelog/index.js"; +import { FEEDBACK_PROMPT, isFeedbackConfigured } from "../../telemetry/feedback.js"; /** * Register every built-in slash command. @@ -163,4 +164,28 @@ export function registerBuiltInCommands(): void { }, }); + // Intentional product feedback → PostHog survey (headless). Can ship when + // ambient telemetry is off; env kill switches still block. Free text 2000 cap. + // Hidden from the slash menu until survey env ids are set (still callable). + registerCommand({ + name: "feedback", + description: "Send product feedback (env kill switches still apply)", + argumentHint: "[your feedback]", + available: () => isFeedbackConfigured(), + handler: (args, ctx) => { + const text = args.trim(); + if (text.length === 0) { + if (ctx.beginFeedbackCapture === undefined) { + return { type: "message", text: "Feedback is not available in this mode." }; + } + ctx.beginFeedbackCapture(); + return { type: "message", text: FEEDBACK_PROMPT }; + } + if (ctx.submitFeedback === undefined) { + return { type: "message", text: "Feedback is not available in this mode." }; + } + return { type: "message", text: ctx.submitFeedback(text) }; + }, + }); + } diff --git a/src/tui/commands/registry.ts b/src/tui/commands/registry.ts index e73380c79..902788e71 100644 --- a/src/tui/commands/registry.ts +++ b/src/tui/commands/registry.ts @@ -13,6 +13,16 @@ export type CommandContext = { startWorkflow?: (name: string) => string; /** Rename the active session (persisted as run.json task). */ renameSession?: (name: string) => string | undefined; + /** + * Submit intentional operator feedback (PostHog survey). Returns the + * operator-facing status line. Wired by the TUI runner. + */ + submitFeedback?: (text: string) => string; + /** + * Arm multi-turn feedback capture: the next non-command submit is treated as + * the feedback body instead of a model prompt. + */ + beginFeedbackCapture?: () => void; }; export type CommandResult = diff --git a/src/tui/live-session-port.test.ts b/src/tui/live-session-port.test.ts index 08ac3a86f..4c75dfceb 100644 --- a/src/tui/live-session-port.test.ts +++ b/src/tui/live-session-port.test.ts @@ -37,6 +37,20 @@ function item( } describe("createLiveSessionPort", () => { + test("classifySubmit defaults to agent and forwards an override", () => { + const { deps } = fakeDeps() + const defaultPort = createLiveSessionPort(deps) + expect(defaultPort.classifySubmit?.("hello")).toBe("agent") + expect(defaultPort.classifySubmit?.("/feedback")).toBe("agent") + + const localPort = createLiveSessionPort({ + ...deps, + classifySubmit: (text) => (text.startsWith("/") ? "local" : "agent"), + }) + expect(localPort.classifySubmit?.("/feedback")).toBe("local") + expect(localPort.classifySubmit?.("hello")).toBe("agent") + }) + test("sendImmediate forwards to deps.send", () => { const { calls, deps } = fakeDeps() const port = createLiveSessionPort(deps) diff --git a/src/tui/live-session-port.ts b/src/tui/live-session-port.ts index a4e7d5a9b..35df472fa 100644 --- a/src/tui/live-session-port.ts +++ b/src/tui/live-session-port.ts @@ -7,12 +7,23 @@ import type { PendingImageAttachment } from "./image-attachments.js" import type { QueueItem, QueueKind } from "./session-queue.js" import type { SessionPort } from "./runtime-bridge.js" +export type SubmitClassification = "agent" | "local" | "empty" + export type LiveSessionPortDeps = { - /** Idle / immediate user text (plus pending images) → agent send path. */ + /** Idle / immediate user text (plus pending images) → host send path. */ send: ( text: string, attachments?: readonly PendingImageAttachment[], ) => void + /** + * Classify a submit without side effects so the bridge can keep local-only + * lines (slash commands, /feedback capture) off the busy/queue path. + * Defaults to "agent" when omitted. + */ + classifySubmit?: ( + text: string, + attachments?: readonly PendingImageAttachment[], + ) => SubmitClassification /** Hard interrupt current run (runner close/rebuild). */ interrupt: () => void /** @@ -33,6 +44,12 @@ export type LiveSessionPortDeps = { */ export function createLiveSessionPort(deps: LiveSessionPortDeps): SessionPort { return { + classifySubmit: ( + text: string, + attachments?: readonly PendingImageAttachment[], + ): SubmitClassification => { + return deps.classifySubmit?.(text, attachments) ?? "agent" + }, sendImmediate: ( text: string, attachments?: readonly PendingImageAttachment[], diff --git a/src/tui/markdown-rows.test.ts b/src/tui/markdown-rows.test.ts index 091b67209..059669f92 100644 --- a/src/tui/markdown-rows.test.ts +++ b/src/tui/markdown-rows.test.ts @@ -104,7 +104,14 @@ describe("markdown transcript rows", () => { const frame = await settle( h, - (f) => f.includes("What the site is") && !f.includes("###") && !f.includes("**Hardware:**"), + // Require the bold body line too: heading-only frames can pass a + // "no ### / no **Hardware:**" check while the body has not painted yet + // (CI flake CL-5715). + (f) => + f.includes("What the site is") && + f.includes("Hardware:") && + !f.includes("###") && + !f.includes("**Hardware:**"), ) expect(frame).toContain("What the site is") expect(frame).not.toContain("###") diff --git a/src/tui/product-host.ts b/src/tui/product-host.ts index 8bfa2261c..087a47423 100644 --- a/src/tui/product-host.ts +++ b/src/tui/product-host.ts @@ -130,6 +130,11 @@ export type ProductHostSend = ( text: string, attachments?: readonly PendingImageAttachment[], ) => void +/** Classify a composer submit without side effects (see SessionPort.classifySubmit). */ +export type ProductHostClassifySubmit = ( + text: string, + attachments?: readonly PendingImageAttachment[], +) => "agent" | "local" | "empty" export type ProductHostInterrupt = () => void export type ProductHostDeliver = ( text: string, @@ -158,6 +163,11 @@ export type ProductHostConfig = { readonly cwd?: string readonly eventEmitter: EventEmitter readonly send: ProductHostSend + /** + * Classify a submit without side effects so slash commands and multi-turn + * /feedback never mark the session busy or enter the mid-run queue. + */ + readonly classifySubmit?: ProductHostClassifySubmit readonly interrupt: ProductHostInterrupt readonly deliver?: ProductHostDeliver /** Model/provider rows for the picker (id applied on select). */ @@ -355,6 +365,9 @@ export async function mountProductHost( const port = createLiveSessionPort({ send: config.send, interrupt: config.interrupt, + ...(config.classifySubmit !== undefined + ? { classifySubmit: config.classifySubmit } + : {}), ...(config.deliver !== undefined ? { deliver: config.deliver } : {}), }) // Empty options accept the defaults (real clock, 250 ms tick, 15 min stall) diff --git a/src/tui/prompt-features.test.ts b/src/tui/prompt-features.test.ts index 7178bb3ea..a6c6c3c50 100644 --- a/src/tui/prompt-features.test.ts +++ b/src/tui/prompt-features.test.ts @@ -234,6 +234,20 @@ describe("image attachments", () => { expect(texts).toEqual([""]) }) }) + + test("empty Enter still reaches exclusive host for multi-turn cancel", async () => { + await withShell(async (shell) => { + const submitted: string[] = [] + setShellBridgeHooks(shell, { + onSubmit: (text) => submitted.push(text), + onInterrupt: () => {}, + exclusive: true, + }) + shell.prompt.value = " " + submitPrompt(shell) + expect(submitted).toEqual([" "]) + }) + }) }) /** diff --git a/src/tui/runner-host.ts b/src/tui/runner-host.ts index 265496657..424001c7f 100644 --- a/src/tui/runner-host.ts +++ b/src/tui/runner-host.ts @@ -56,6 +56,14 @@ export type RunnerHostDeps = { text: string, attachments?: readonly PendingImageAttachment[], ) => void + /** + * Classify a submit without side effects so slash commands and multi-turn + * /feedback never mark the session busy or enter the mid-run queue. + */ + readonly classifySubmit?: ( + text: string, + attachments?: readonly PendingImageAttachment[], + ) => "agent" | "local" | "empty" readonly interrupt: () => void readonly deliver?: ( text: string, @@ -249,6 +257,9 @@ export async function mountRunnerHost(deps: RunnerHostDeps): Promise eventEmitter: deps.eventEmitter, send: deps.send, interrupt: deps.interrupt, + ...(deps.classifySubmit !== undefined + ? { classifySubmit: deps.classifySubmit } + : {}), ...(deps.deliver !== undefined ? { deliver: deps.deliver } : {}), ...(deps.onConnectProvider !== undefined ? { onConnectProvider: deps.onConnectProvider } diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 46da09ac3..036612de8 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -94,11 +94,21 @@ import { import { registerBuiltInCommands } from "./commands/built-in.js"; import type { PluginModule } from "../plugins/loader.js"; import { createTurnObserver } from "../telemetry/ai-observability.js"; +import { + armFeedbackCapture, + cancelFeedbackCapture, + captureFeedback, + feedbackResultMessage, + getLastTurnTraceId, + isFeedbackCapturePending, + takeFeedbackCapture, +} from "../telemetry/feedback.js"; import { activateHeldTelemetry, telemetryFirstRunPending } from "../telemetry/first-run.js"; import { TELEMETRY_NOTICE } from "../telemetry/index.js"; -import { classifyCommandName } from "../telemetry/classify.js"; +import { captureSlashCommand } from "../telemetry/product-events.js"; import { getTelemetry, liveTelemetry, setTelemetry } from "../telemetry/singleton.js"; import { createTelemetryToggleHandler } from "../telemetry/toggle.js"; + import { loadStartupChangelogMarkdown } from "../changelog/index.js"; import pkg from "../../package.json" with { type: "json" }; import { seedPricingMetadataFromCache } from "../cost/pricing-metadata.js"; @@ -128,6 +138,7 @@ import { type SubAgentProvider, } from "../subagent/index.js"; import type { InferenceSource, ToolDefinition, InboundMessage } from "@intx/types/runtime"; +import { OPERATOR_ORIGINATED_FLAG } from "../agent/message-provenance.js"; import { createSessionOperationQueue } from "./session-operation-queue.js"; import { setAgentSourceUnlessClosed } from "./agent-source-sync.js"; import { createChatDirector, hydrateTasksFromTurns } from "../agent/director.js"; @@ -294,7 +305,7 @@ export async function loadLocalSettingsWriteBase( } } -function buildCompactionContinuationMessage(): InboundMessage { +export function buildCompactionContinuationMessage(): InboundMessage { return { ref: { uid: 0, mailbox: "system" }, headers: { @@ -344,25 +355,98 @@ export type SubmitHandlerDeps = { sendPrompt: (text: string, attachments?: readonly PendingImageAttachment[]) => void; /** Consent-by-proceeding hook: runs only for real prompts, never commands. */ onPromptSubmitted?: () => void; + /** + * When true, the next non-command submit is treated as intentional feedback + * text (bare `/feedback` multi-turn mode) instead of a model prompt. + */ + isFeedbackCapturePending?: () => boolean; + /** Consume the pending feedback arm and handle the text; return operator message. */ + onFeedbackText?: (text: string) => string; + /** Drop a pending multi-turn /feedback arm (empty Enter cancel). */ + cancelFeedbackCapture?: () => void; + /** Surface a local system notice (feedback thanks / blocked / cancelled). */ + onSystemNotice?: (text: string) => void; }; + /** * Composer submit handler. Slash input is dispatched against the command - * registry instead of being sent to the model. + * registry instead of being sent to the model. When feedback capture is armed + * (bare `/feedback`), the next non-command line is captured as survey text. + * + * Returns an outcome so the session bridge can keep local-only submits off the + * agent busy path and out of the mid-run queue. + */ +export type SubmitOutcome = "agent" | "local" | "empty"; + +/** + * Classify a composer line without side effects. Local = slash command or + * armed multi-turn feedback text; empty = no-op (or cancel-feedback); agent = + * real model turn. */ +export function classifySubmission( + text: string, + options: { + hasAttachments?: boolean; + feedbackPending?: boolean; + feedbackCaptureEnabled?: boolean; + } = {}, +): SubmitOutcome { + const route = routeSubmission(text); + const hasAttachments = options.hasAttachments === true; + if (route.kind === "empty" && !hasAttachments) return "empty"; + if (route.kind === "command") return "local"; + if ( + route.kind === "prompt" && + options.feedbackPending === true && + options.feedbackCaptureEnabled === true + ) { + return "local"; + } + return "agent"; +} + export function createSubmitHandler( deps: SubmitHandlerDeps, -): (text: string, attachments?: readonly PendingImageAttachment[]) => void { +): (text: string, attachments?: readonly PendingImageAttachment[]) => SubmitOutcome { return (text, attachments) => { const route = routeSubmission(text); const hasAttachments = attachments !== undefined && attachments.length > 0; - if (route.kind === "empty" && !hasAttachments) return; + const feedbackPending = deps.isFeedbackCapturePending?.() === true; + const feedbackCaptureEnabled = deps.onFeedbackText !== undefined; + const outcome = classifySubmission(text, { + hasAttachments, + feedbackPending, + feedbackCaptureEnabled, + }); + + // Empty Enter while /feedback is armed cancels instead of trapping the + // operator until they type free text or /clear. + if (outcome === "empty") { + if (feedbackPending) { + deps.cancelFeedbackCapture?.(); + deps.onSystemNotice?.("Feedback cancelled."); + } + return "empty"; + } if (route.kind === "command") { + // Any other slash command drops a bare-/feedback arm so the next + // free-text line is not mis-routed as survey text. + if (feedbackPending && route.name !== "feedback") { + deps.cancelFeedbackCapture?.(); + } deps.dispatchCommand(route.name, route.args); - return; + return "local"; + } + // Multi-turn /feedback: next Enter is survey text, not a model prompt. + if (outcome === "local" && deps.onFeedbackText !== undefined) { + const notice = deps.onFeedbackText(route.kind === "prompt" ? route.text : text); + deps.onSystemNotice?.(notice); + return "local"; } deps.onPromptSubmitted?.(); deps.sendPrompt(route.kind === "prompt" ? route.text : "", attachments); + return "agent"; }; } @@ -370,8 +454,11 @@ export function createSubmitHandler( export const IMAGE_ONLY_PROMPT = "Please inspect the attached image."; /** - * Build the inbound message carrying image attachments. Plain text sends stay - * on the string overload; only attachment sends need the envelope. + * Build the inbound message for a genuine operator submit — the real + * prompt-submit path in the TUI (sendUserPrompt / the "send" command + * result), with or without attachments. Carries OPERATOR_ORIGINATED_FLAG so + * director.ts's loop-protection backstop can tell this apart from + * system-originated sends (compaction continuations, retries, nudges). */ export function userInboundMessage( text: string, @@ -386,7 +473,7 @@ export function userInboundMessage( messageId: `<${crypto.randomUUID()}@local>`, interchangeType: "conversation.message", }, - flags: [], + flags: [OPERATOR_ORIGINATED_FLAG], signatureStatus: "missing", content: text.length > 0 ? text : IMAGE_ONLY_PROMPT, attachments: attachments.map((a) => ({ @@ -1657,6 +1744,7 @@ export async function runTUI(initialConfig: Config): Promise { // abort handles → child agent.close) before clearing the session store so // /clear does not leave orphaned child reactors burning tokens. const newSession = (): void => { + cancelFeedbackCapture(); // Wipe the painted transcript immediately. The product host listens for // session.clear; the Ink App used to clear its own stream unconditionally // and that path never moved to OpenTUI. @@ -1807,6 +1895,18 @@ export async function runTUI(initialConfig: Config): Promise { void renameSession(config.cwd, sessionId, trimmed).then(() => persistRunSnapshot("running")); return undefined; }, + submitFeedback: (text) => { + // Inline /feedback must drop a prior bare-/feedback arm so the + // next normal prompt is not stolen as survey text. + cancelFeedbackCapture(); + const status = captureFeedback(getTelemetry(), text, { + turnTraceId: getLastTurnTraceId(), + }); + return feedbackResultMessage(status); + }, + beginFeedbackCapture: () => { + armFeedbackCapture(); + }, }; // Routed through the shell's notice path rather than straight into the @@ -1820,14 +1920,14 @@ export async function runTUI(initialConfig: Config): Promise { /** Settle the shell after a rejected send so the run does not look live. */ const handleSendFailure = (err: unknown): void => { - const kind = classifyAgentSendFailure( + const failure = classifyAgentSendFailure( err, sendAborted, isCodexAuthError, isXaiAuthError, ); - captureAuthFailure(getTelemetry(), kind); - if (!shouldSettleUiAfterSendFailure(kind)) return; + captureAuthFailure(getTelemetry(), failure); + if (!shouldSettleUiAfterSendFailure(failure.kind)) return; recordRunError(err); systemNotice(err instanceof Error ? err.message : String(err)); setShellRunState(host.shell, "idle"); @@ -1877,7 +1977,10 @@ export async function runTUI(initialConfig: Config): Promise { systemNotice(result.text); return; case "send": - void agentProxy.send(result.text).catch(handleSendFailure); + // A command the operator typed and submitted at the prompt — same + // provenance as a plain-text send, just composed by the command + // handler instead of typed verbatim. + void agentProxy.send(userInboundMessage(result.text, [])).catch(handleSendFailure); return; case "workflow": systemNotice(workflowController.start(result.name)); @@ -1923,10 +2026,6 @@ export async function runTUI(initialConfig: Config): Promise { const ingested = await ingestPathMentions(text, config.cwd, imageAttachmentFromPath); const resolved = await resolveAtMentions(ingested.text, config.cwd); const attachments = [...pending, ...ingested.attachments]; - if (attachments.length === 0) { - await agentProxy.send(resolved); - return; - } await agentProxy.send(userInboundMessage(resolved, attachments)); }; @@ -1938,7 +2037,8 @@ export async function runTUI(initialConfig: Config): Promise { } // Plugins register into the same command registry as the built-ins, so an // unrecognised name is plugin-authored and is bucketed rather than sent. - getTelemetry().capture("slash_command", { command_name: classifyCommandName(command.name) }); + // Shared emitter so TUI and any headless path report the same event. + captureSlashCommand(getTelemetry(), command.name); applyCommandResult(command.handler(args, commandContext)); }; @@ -1971,7 +2071,24 @@ export async function runTUI(initialConfig: Config): Promise { void activateHeldTelemetry(trueGlobalSettingsPath, () => liveTelemetryIntent); } }, + isFeedbackCapturePending, + cancelFeedbackCapture, + onFeedbackText: (text) => { + takeFeedbackCapture(); + const status = captureFeedback(getTelemetry(), text, { + turnTraceId: getLastTurnTraceId(), + }); + return feedbackResultMessage(status); + }, + onSystemNotice: systemNotice, + }), + classifySubmit: (text, attachments) => + classifySubmission(text, { + hasAttachments: attachments !== undefined && attachments.length > 0, + feedbackPending: isFeedbackCapturePending(), + feedbackCaptureEnabled: true, + }), interrupt, // Consent by proceeding requires the disclosure to be on screen before the // first prompt activates the held telemetry instance: the landing shows it, @@ -2225,8 +2342,16 @@ export async function runTUI(initialConfig: Config): Promise { })); }, setTelemetryEnabled: (enabled) => { + // Only flip the live intent when the toggle is accepted. Env kill + // switches refuse re-enable; leaving the UI on while capture stays + // off is a silent lie. + if (!onChangeTelemetryEnabled(enabled)) { + systemNotice( + "Telemetry stays off — disabled by DO_NOT_TRACK or CORBITS_TELEMETRY.", + ); + return; + } liveTelemetryIntent = enabled; - void onChangeTelemetryEnabled(enabled); }, setShowPromptCost: (value) => { liveShowPromptCost = value; @@ -2292,7 +2417,9 @@ export async function runTUI(initialConfig: Config): Promise { if (!resumeSkipInitialTask && config.task.trim().length > 0) { - void agentProxy.send(config.task.trim()).catch(handleSendFailure); + // The operator's initial task, typed as a CLI argument before launch — + // same provenance as a prompt submit. + void agentProxy.send(userInboundMessage(config.task.trim(), [])).catch(handleSendFailure); } // Hydrate a resumed session's transcript after first paint. Reading history and diff --git a/src/tui/runtime-bridge.test.ts b/src/tui/runtime-bridge.test.ts index 065827783..effec6e99 100644 --- a/src/tui/runtime-bridge.test.ts +++ b/src/tui/runtime-bridge.test.ts @@ -177,6 +177,65 @@ describe("attachSessionBridge", () => { ) }) + test("local classify keeps idle submits off the busy path", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + run: "idle", + }) + const port = createRecordingPort({ + classifySubmit: (text) => (text.startsWith("/") ? "local" : "agent"), + }) + const bridge = attachSessionBridge(shell, port) + try { + bridge.submit("/feedback quick test", "immediate") + await h.renderOnce() + expect(port.calls).toEqual([ + { op: "sendImmediate", text: "/feedback quick test" }, + ]) + expect(shell.session.run).toBe("idle") + expect(badgeCount(shell.session)).toBe(0) + } finally { + bridge.dispose() + shell.dispose() + } + }, + { width: 80, height: 24 }, + ) + }) + + test("local classify mid-run does not enqueue or interrupt the agent turn", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + run: "busy", + }) + const port = createRecordingPort({ + classifySubmit: (text) => (text.startsWith("/") ? "local" : "agent"), + }) + const bridge = attachSessionBridge(shell, port) + try { + bridge.submit("/feedback note", "queue") + await h.renderOnce() + expect(port.calls).toEqual([ + { op: "sendImmediate", text: "/feedback note" }, + ]) + expect(port.calls.some((c) => c.op === "enqueue")).toBe(false) + expect(shell.session.run).toBe("busy") + expect(badgeCount(shell.session)).toBe(0) + } finally { + bridge.dispose() + shell.dispose() + } + }, + { width: 80, height: 24 }, + ) + }) + test("queued item delivers at tool.boundary", async () => { await withTestRenderer( async (h) => { diff --git a/src/tui/runtime-bridge.ts b/src/tui/runtime-bridge.ts index 0c60c2a8e..3507ff5f5 100644 --- a/src/tui/runtime-bridge.ts +++ b/src/tui/runtime-bridge.ts @@ -105,6 +105,15 @@ export type { BridgeInboundEvent, ReactorLikeEvent, StreamMapContext } /** Outbound actions the UI asks the session runtime to perform. */ export type SessionPort = { + /** + * Classify a composer submit without side effects. Local-only lines (slash + * commands, armed multi-turn /feedback text) must never enter the mid-run + * queue or mark the session busy. Default when omitted: treat as agent. + */ + classifySubmit?: ( + text: string, + attachments?: readonly PendingImageAttachment[], + ) => "agent" | "local" | "empty" /** Idle prompt submit — deliver now. */ sendImmediate: ( text: string, @@ -208,7 +217,9 @@ export type PortCall = | { readonly op: "interrupt" } | { readonly op: "deliver"; readonly item: QueueItem } -export function createRecordingPort(): SessionPort & { +export function createRecordingPort(opts?: { + classifySubmit?: SessionPort["classifySubmit"] +}): SessionPort & { readonly calls: readonly PortCall[] clear: () => void } { @@ -220,6 +231,9 @@ export function createRecordingPort(): SessionPort & { clear: () => { calls.length = 0 }, + ...(opts?.classifySubmit !== undefined + ? { classifySubmit: opts.classifySubmit } + : {}), sendImmediate: (text) => { calls.push({ op: "sendImmediate", text }) }, @@ -379,6 +393,9 @@ const bridges = new WeakMap() function resolvePort(handlers?: SessionPortHandlers): SessionPort { return { + ...(handlers?.classifySubmit !== undefined + ? { classifySubmit: handlers.classifySubmit } + : {}), sendImmediate: handlers?.sendImmediate ?? NOOP_PORT.sendImmediate, enqueue: handlers?.enqueue ?? NOOP_PORT.enqueue, interrupt: handlers?.interrupt ?? NOOP_PORT.interrupt, @@ -943,7 +960,30 @@ export function attachSessionBridge( if (bag.disposed) return const t = text.trim() const attached = attachments ?? [] - if (t.length === 0 && attached.length === 0) return + if (t.length === 0 && attached.length === 0) { + // Still run host empty handling (e.g. cancel armed /feedback). + if (bag.port.classifySubmit?.(t, attachments) === "empty") { + bag.port.sendImmediate(t, attachments) + } + return + } + + // Local-only submits (slash commands, multi-turn /feedback) never mark the + // session busy and never enter the mid-run queue — they are not agent turns. + const classification = bag.port.classifySubmit?.(t, attachments) ?? "agent" + if (classification === "empty") { + bag.port.sendImmediate(t, attachments) + return + } + if (classification === "local") { + appendStreamRow(shell, { + role: "user", + text: userRowText(t, attached), + }) + bag.port.sendImmediate(t, attachments) + paintChrome(shell) + return + } if (kind === "reinject") { // Not a boundary wait: stop the run right now, then fall straight into diff --git a/src/tui/session-chrome.test.ts b/src/tui/session-chrome.test.ts index 015e4fb98..3c57c4d0e 100644 --- a/src/tui/session-chrome.test.ts +++ b/src/tui/session-chrome.test.ts @@ -203,26 +203,50 @@ describe("classifyAgentSendFailure", () => { const xai = (e: unknown) => e === "xai" test("abort is ignored", () => { - expect(classifyAgentSendFailure(new Error("x"), true, codex, xai)).toBe( - "abort", - ) + expect(classifyAgentSendFailure(new Error("x"), true, codex, xai)).toEqual({ + kind: "abort", + authProvider: null, + }) expect(shouldSettleUiAfterSendFailure("abort")).toBe(false) }) test("generic error settles ui", () => { - expect(classifyAgentSendFailure(new Error("boom"), false, codex, xai)).toBe( - "error", - ) + expect(classifyAgentSendFailure(new Error("boom"), false, codex, xai)).toEqual({ + kind: "error", + authProvider: null, + }) expect(shouldSettleUiAfterSendFailure("error")).toBe(true) }) test("auth failures settle ui for idle footer", () => { - expect(classifyAgentSendFailure("codex", false, codex, xai)).toBe( - "codex_auth", - ) - expect(classifyAgentSendFailure("xai", false, codex, xai)).toBe("xai_auth") - expect(shouldSettleUiAfterSendFailure("codex_auth")).toBe(true) - expect(shouldSettleUiAfterSendFailure("xai_auth")).toBe(true) + expect(classifyAgentSendFailure("codex", false, codex, xai)).toEqual({ + kind: "auth", + authProvider: "codex", + }) + expect(classifyAgentSendFailure("xai", false, codex, xai)).toEqual({ + kind: "auth", + authProvider: "xai", + }) + expect(shouldSettleUiAfterSendFailure("auth")).toBe(true) + }) + + test("anthropic and generic credential rejections classify as auth", () => { + expect( + classifyAgentSendFailure( + new Error("anthropic: invalid x-api-key"), + false, + codex, + xai, + ), + ).toEqual({ kind: "auth", authProvider: "anthropic" }) + expect( + classifyAgentSendFailure( + new Error("Request failed with status 401 Unauthorized"), + false, + codex, + xai, + ), + ).toEqual({ kind: "auth", authProvider: "other" }) }) }) @@ -231,22 +255,31 @@ describe("sendFailureText", () => { const codex = sendFailureText( 'Codex profile "default" is not authorized. Log in again.', ) - expect(classifySendFailureMessage( - 'Codex profile "default" is not authorized. Log in again.', - )).toBe("codex_auth") + expect( + classifySendFailureMessage( + 'Codex profile "default" is not authorized. Log in again.', + ), + ).toEqual({ kind: "auth", authProvider: "codex" }) expect(codex).toContain("sign-in expired") expect(codex).toContain("/model") const xai = sendFailureText('xAI profile "default" could not be refreshed (401).') expect(xai).toContain("/model") expect(xai).not.toContain("401") + + const anthropic = sendFailureText("authentication_error: invalid x-api-key") + expect(anthropic).toContain("anthropic") + expect(anthropic).toContain("/model") }) test("an unclassified failure keeps its raw message", () => { expect(sendFailureText("connection reset by peer")).toBe( "connection reset by peer", ) - expect(classifySendFailureMessage("connection reset by peer")).toBe("error") + expect(classifySendFailureMessage("connection reset by peer")).toEqual({ + kind: "error", + authProvider: null, + }) }) }) diff --git a/src/tui/session-chrome.ts b/src/tui/session-chrome.ts index 6a283ecf8..9fd05375b 100644 --- a/src/tui/session-chrome.ts +++ b/src/tui/session-chrome.ts @@ -132,7 +132,35 @@ export function resolveRampPhase( return "working" } -export type SendFailureKind = "abort" | "codex_auth" | "xai_auth" | "error" +export type SendFailureKind = "abort" | "auth" | "error" + +/** First-party auth_provider values only — never free-text provider labels. */ +export type AuthProviderId = "codex" | "xai" | "anthropic" | "other" + +export type ClassifiedSendFailure = { + readonly kind: SendFailureKind + readonly authProvider: AuthProviderId | null +} + +// Phrase matchers for message-only classification (stream carries bare strings). +// Codex/xAI constructors always emit the profile phrases below. +const CODEX_AUTH_MESSAGE = /\bcodex profile\b/i +const XAI_AUTH_MESSAGE = /\bxai profile\b/i +// Anthropic API-key rejections: authentication_error type, invalid x-api-key, +// or invalid api key phrasing in 401 bodies. +const ANTHROPIC_AUTH_MESSAGE = + /\b(?:anthropic|claude)\b.*\b(?:auth|unauthorized|api[\s_-]?key|x-api-key)\b|\bauthentication_error\b|\binvalid[\s_-]?x?-?api[\s_-]?key\b/i +// Generic credential rejection when the provider cannot be named safely. +const GENERIC_AUTH_MESSAGE = + /\b(?:401|403)\b|\bunauthorized\b|\binvalid[\s_-]?api[\s_-]?key\b|\bauthentication\b.*\bfail/i + +function authProviderFromMessage(message: string): AuthProviderId | null { + if (CODEX_AUTH_MESSAGE.test(message)) return "codex" + if (XAI_AUTH_MESSAGE.test(message)) return "xai" + if (ANTHROPIC_AUTH_MESSAGE.test(message)) return "anthropic" + if (GENERIC_AUTH_MESSAGE.test(message)) return "other" + return null +} /** Classify agent.send() rejection so the TUI can settle UI state consistently. */ export function classifyAgentSendFailure( @@ -140,47 +168,41 @@ export function classifyAgentSendFailure( aborted: boolean, isCodexAuth: (e: unknown) => boolean, isXaiAuth: (e: unknown) => boolean, -): SendFailureKind { - if (aborted) return "abort" - if (isCodexAuth(err)) return "codex_auth" - if (isXaiAuth(err)) return "xai_auth" - return "error" +): ClassifiedSendFailure { + if (aborted) return { kind: "abort", authProvider: null } + if (isCodexAuth(err)) return { kind: "auth", authProvider: "codex" } + if (isXaiAuth(err)) return { kind: "auth", authProvider: "xai" } + const message = err instanceof Error ? err.message : String(err) + const authProvider = authProviderFromMessage(message) + if (authProvider !== null) return { kind: "auth", authProvider } + return { kind: "error", authProvider: null } } export function shouldSettleUiAfterSendFailure(kind: SendFailureKind): boolean { - return kind === "codex_auth" || kind === "xai_auth" || kind === "error" -} - -// Send-failure kinds are first-party constants, so the provider each one names -// is a fixed mapping rather than a classification over author-chosen text. -const AUTH_FAILURE_PROVIDERS: Partial> = { - codex_auth: "codex", - xai_auth: "xai", + return kind === "auth" || kind === "error" } /** Report which provider rejected the stored credentials; silent otherwise. */ -export function captureAuthFailure(telemetry: Telemetry, kind: SendFailureKind): void { - const provider = AUTH_FAILURE_PROVIDERS[kind] - if (provider === undefined) return - telemetry.capture("auth_failure", { auth_provider: provider }) +export function captureAuthFailure( + telemetry: Telemetry, + failure: ClassifiedSendFailure, +): void { + if (failure.kind !== "auth" || failure.authProvider === null) return + telemetry.capture("auth_failure", { auth_provider: failure.authProvider }) } -// The stream carries a failure as a bare message string, so the auth errors are -// recognised by the profile phrase their constructors always produce -// (`Codex profile "default" is not authorized. …`). -const CODEX_AUTH_MESSAGE = /\bcodex profile\b/i -const XAI_AUTH_MESSAGE = /\bxai profile\b/i - /** Same classification as `classifyAgentSendFailure`, from the message alone. */ -export function classifySendFailureMessage(message: string): SendFailureKind { - if (CODEX_AUTH_MESSAGE.test(message)) return "codex_auth" - if (XAI_AUTH_MESSAGE.test(message)) return "xai_auth" - return "error" +export function classifySendFailureMessage(message: string): ClassifiedSendFailure { + const authProvider = authProviderFromMessage(message) + if (authProvider !== null) return { kind: "auth", authProvider } + return { kind: "error", authProvider: null } } -const AUTH_FAILURE_TEXT: Partial> = { - codex_auth: "your chatgpt sign-in expired — /model to sign in again", - xai_auth: "your x.ai sign-in expired — /model to sign in again", +const AUTH_FAILURE_TEXT: Record = { + codex: "your chatgpt sign-in expired — /model to sign in again", + xai: "your x.ai sign-in expired — /model to sign in again", + anthropic: "your anthropic api key was rejected — /model to update credentials", + other: "provider credentials were rejected — /model to sign in again", } /** @@ -189,5 +211,9 @@ const AUTH_FAILURE_TEXT: Partial> = { * the only detail the operator has. */ export function sendFailureText(message: string): string { - return AUTH_FAILURE_TEXT[classifySendFailureMessage(message)] ?? message + const failure = classifySendFailureMessage(message) + if (failure.kind === "auth" && failure.authProvider !== null) { + return AUTH_FAILURE_TEXT[failure.authProvider] + } + return message } diff --git a/src/tui/shell.ts b/src/tui/shell.ts index 74f062427..2d019c963 100644 --- a/src/tui/shell.ts +++ b/src/tui/shell.ts @@ -3138,7 +3138,15 @@ export function submitPrompt( const text = shell.prompt.value const t = text.trim() const attachments = shell.pendingAttachments - if (t.length === 0 && attachments.length === 0) return + if (t.length === 0 && attachments.length === 0) { + // Empty Enter still reaches the exclusive host so multi-turn /feedback + // can cancel; non-exclusive shells have nothing to do with a blank line. + const hooks = getShellBridgeHooks(shell) + if (hooks?.exclusive) { + hooks.onSubmit(text, "immediate", attachments) + } + return + } if (kind === "reinject" && shell.session.run !== "busy") return // Shell/REPL muscle memory: a bare `exit` or `quit` quits rather than being diff --git a/src/tui/stall-watchdog.ts b/src/tui/stall-watchdog.ts index 0f408f202..e7c3cfd33 100644 --- a/src/tui/stall-watchdog.ts +++ b/src/tui/stall-watchdog.ts @@ -1,4 +1,5 @@ import type { TurnStatus } from "./session-chrome.js" +import { detectSequencePeriod, type SequencePeriodCheck } from "../util/period-detection.js" // How long the run can be continuously awaiting a response with no new content // before the watchdog fires and aborts the in-flight request. @@ -55,55 +56,25 @@ const REPETITION_MAX_PERIOD_CAP = 2_000 // cycle spans two full sentences, comfortably above it. const REPETITION_MIN_DISTINCT_CHARS = 8 -export type RepetitionCheck = { - readonly repeating: boolean - readonly period: number | null - readonly repeats: number -} - -/** - * Length of the exact-period run ending at the last character of `text`, - * including the base period itself. `text[i] === text[i - period]` walked - * backwards from the end; stops at the first mismatch or the start of the - * string. - */ -function periodicSuffixLength(text: string, period: number): number { - let i = text.length - 1 - let j = i - period - let matched = 0 - while (j >= 0 && text[i] === text[j]) { - matched++ - i-- - j-- - } - return matched + period -} +export type RepetitionCheck = SequencePeriodCheck /** * Whether the tail of `text` is an exact repeat of some short span at least * `REPETITION_MIN_REPEATS` times. Pure text-in, decision-out: the caller owns * accumulating the buffer across deltas and cycles within a turn. * - * Periods longer than `text.length / REPETITION_MIN_REPEATS` are skipped, not - * as an arbitrary cutoff but because they cannot mathematically reach the - * occurrence threshold within the given text — a loop with a longer period - * needs a longer buffer to confirm, which is a buffer-size trade-off owned by - * the caller, not a second detection path here. + * Delegates to the generic detectSequencePeriod over the character array — + * periods longer than `text.length / REPETITION_MIN_REPEATS` are skipped + * there, not as an arbitrary cutoff but because they cannot mathematically + * reach the occurrence threshold within the given text. */ export function detectRepetition(text: string): RepetitionCheck { - const maxPeriod = Math.min( - REPETITION_MAX_PERIOD_CAP, - Math.floor(text.length / REPETITION_MIN_REPEATS), - ) - for (let period = REPETITION_MIN_PERIOD; period <= maxPeriod; period++) { - const matched = periodicSuffixLength(text, period) - const repeats = matched / period - if (repeats < REPETITION_MIN_REPEATS) continue - const unit = text.slice(text.length - period) - if (new Set(unit).size < REPETITION_MIN_DISTINCT_CHARS) continue - return { repeating: true, period, repeats } - } - return { repeating: false, period: null, repeats: 0 } + return detectSequencePeriod(text.split(""), { + minPeriod: REPETITION_MIN_PERIOD, + maxPeriod: REPETITION_MAX_PERIOD_CAP, + minRepeats: REPETITION_MIN_REPEATS, + minDistinct: () => REPETITION_MIN_DISTINCT_CHARS, + }) } /** diff --git a/src/tui/submit-handler.test.ts b/src/tui/submit-handler.test.ts index 7ce181be9..d31d3d45c 100644 --- a/src/tui/submit-handler.test.ts +++ b/src/tui/submit-handler.test.ts @@ -1,5 +1,6 @@ -import { describe, test, expect } from "bun:test"; +import { afterEach, describe, test, expect } from "bun:test"; import { + classifySubmission, createSubmitHandler, IMAGE_ONLY_PROMPT, routeSubmission, @@ -8,12 +9,28 @@ import { } from "./runner.js"; import type { PendingImageAttachment } from "./image-attachments.js"; import { TELEMETRY_NOTICE } from "../telemetry/index.js"; +import { + armFeedbackCapture, + cancelFeedbackCapture, + isFeedbackCapturePending, + resetFeedbackStateForTests, +} from "../telemetry/feedback.js"; + +afterEach(() => { + resetFeedbackStateForTests(); +}); type Dispatched = { name: string; args: string }; -function harness() { +function harness(options?: { + isFeedbackCapturePending?: () => boolean; + onFeedbackText?: (text: string) => string; + cancelFeedbackCapture?: () => void; + onSystemNotice?: (text: string) => void; +}) { const dispatched: Dispatched[] = []; const prompts: string[] = []; + const notices: string[] = []; let promptSubmissions = 0; const submit = createSubmitHandler({ dispatchCommand: (name, args) => dispatched.push({ name, args }), @@ -21,43 +38,54 @@ function harness() { onPromptSubmitted: () => { promptSubmissions += 1; }, + ...(options?.isFeedbackCapturePending !== undefined + ? { isFeedbackCapturePending: options.isFeedbackCapturePending } + : {}), + ...(options?.onFeedbackText !== undefined ? { onFeedbackText: options.onFeedbackText } : {}), + ...(options?.cancelFeedbackCapture !== undefined + ? { cancelFeedbackCapture: options.cancelFeedbackCapture } + : {}), + onSystemNotice: (text) => { + notices.push(text); + options?.onSystemNotice?.(text); + }, }); - return { submit, dispatched, prompts, telemetry: () => promptSubmissions }; + return { submit, dispatched, prompts, notices, telemetry: () => promptSubmissions }; } describe("composer submit handler", () => { test("dispatches a typed slash command instead of sending it to the model", () => { const h = harness(); - h.submit("/clear"); + expect(h.submit("/clear")).toBe("local"); expect(h.dispatched).toEqual([{ name: "clear", args: "" }]); expect(h.prompts).toEqual([]); }); test("passes slash command arguments through", () => { const h = harness(); - h.submit("/rename ship the feature"); + expect(h.submit("/rename ship the feature")).toBe("local"); expect(h.dispatched).toEqual([{ name: "rename", args: "ship the feature" }]); expect(h.prompts).toEqual([]); }); test("dispatches unknown slash names so the registry can report them", () => { const h = harness(); - h.submit("/not-a-command"); + expect(h.submit("/not-a-command")).toBe("local"); expect(h.dispatched).toEqual([{ name: "not-a-command", args: "" }]); expect(h.prompts).toEqual([]); }); test("sends ordinary prompts to the agent", () => { const h = harness(); - h.submit(" refactor the parser "); + expect(h.submit(" refactor the parser ")).toBe("agent"); expect(h.prompts).toEqual(["refactor the parser"]); expect(h.dispatched).toEqual([]); }); test("ignores blank and bare-slash submissions", () => { const h = harness(); - h.submit(" "); - h.submit("/"); + expect(h.submit(" ")).toBe("empty"); + expect(h.submit("/")).toBe("empty"); expect(h.prompts).toEqual([]); expect(h.dispatched).toEqual([]); }); @@ -69,6 +97,91 @@ describe("composer submit handler", () => { h.submit("hello"); expect(h.telemetry()).toBe(1); }); + + test("pending feedback capture routes the next prompt as feedback, not a model send", () => { + const feedbackTexts: string[] = []; + const h = harness({ + isFeedbackCapturePending: () => isFeedbackCapturePending(), + onFeedbackText: (text) => { + feedbackTexts.push(text); + return "Thanks — feedback sent."; + + }, + }); + armFeedbackCapture(); + expect(h.submit("the UI is snappy")).toBe("local"); + expect(feedbackTexts).toEqual(["the UI is snappy"]); + expect(h.prompts).toEqual([]); + expect(h.notices).toEqual(["Thanks — feedback sent."]); + + expect(h.telemetry()).toBe(0); + }); + + test("slash commands still dispatch while feedback capture is pending", () => { + const h = harness({ + isFeedbackCapturePending: () => isFeedbackCapturePending(), + cancelFeedbackCapture: () => { + cancelFeedbackCapture(); + }, + onFeedbackText: () => "should not run", + }); + armFeedbackCapture(); + expect(h.submit("/help")).toBe("local"); + expect(h.dispatched).toEqual([{ name: "help", args: "" }]); + expect(h.prompts).toEqual([]); + expect(h.notices).toEqual([]); + // Other slash commands drop the arm so the next free-text line is a prompt. + expect(isFeedbackCapturePending()).toBe(false); + }); + + test("empty Enter while armed cancels instead of trapping the operator", () => { + let cancelled = false; + const h = harness({ + isFeedbackCapturePending: () => isFeedbackCapturePending(), + cancelFeedbackCapture: () => { + cancelled = true; + cancelFeedbackCapture(); + }, + onFeedbackText: () => "should not run", + }); + armFeedbackCapture(); + expect(h.submit(" ")).toBe("empty"); + expect(cancelled).toBe(true); + expect(isFeedbackCapturePending()).toBe(false); + expect(h.prompts).toEqual([]); + expect(h.notices).toEqual(["Feedback cancelled."]); + }); +}); + +describe("classifySubmission", () => { + test("slash commands are local", () => { + expect(classifySubmission("/feedback hi")).toBe("local"); + expect(classifySubmission("/clear")).toBe("local"); + }); + + test("ordinary prompts are agent", () => { + expect(classifySubmission("refactor the parser")).toBe("agent"); + }); + + test("blank is empty unless attachments force an agent turn", () => { + expect(classifySubmission(" ")).toBe("empty"); + expect(classifySubmission("", { hasAttachments: true })).toBe("agent"); + }); + + test("armed feedback free text is local only when capture is enabled", () => { + expect( + classifySubmission("snappy UI", { + feedbackPending: true, + feedbackCaptureEnabled: true, + }), + ).toBe("local"); + expect( + classifySubmission("snappy UI", { + feedbackPending: true, + feedbackCaptureEnabled: false, + }), + ).toBe("agent"); + }); }); describe("routeSubmission", () => { diff --git a/src/util/period-detection.test.ts b/src/util/period-detection.test.ts new file mode 100644 index 000000000..771debcbd --- /dev/null +++ b/src/util/period-detection.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, test } from "bun:test"; +import { detectSequencePeriod } from "./period-detection.js"; + +describe("detectSequencePeriod", () => { + test("finds a period-1 (identical) run at the required repeat count", () => { + const result = detectSequencePeriod(["a", "a", "a"], { + minPeriod: 1, + maxPeriod: 8, + minRepeats: 3, + }); + expect(result).toEqual({ repeating: true, period: 1, repeats: 3 }); + }); + + test("finds a period-2 cycle a plain consecutive-identical check would miss", () => { + const result = detectSequencePeriod(["a", "b", "a", "b", "a", "b"], { + minPeriod: 1, + maxPeriod: 8, + minRepeats: 3, + }); + expect(result).toEqual({ repeating: true, period: 2, repeats: 3 }); + }); + + test("finds a period-3 cycle", () => { + const result = detectSequencePeriod(["a", "b", "c", "a", "b", "c", "a", "b", "c"], { + minPeriod: 1, + maxPeriod: 8, + minRepeats: 3, + }); + expect(result).toEqual({ repeating: true, period: 3, repeats: 3 }); + }); + + test("varied sequences never register as periodic", () => { + const seq = Array.from({ length: 200 }, (_, i) => `item-${i}`); + const result = detectSequencePeriod(seq, { minPeriod: 1, maxPeriod: 8, minRepeats: 3 }); + expect(result.repeating).toBe(false); + }); + + test("minRepeats can vary by period", () => { + // Period 1 needs 5 repeats, period 2+ only needs 3 — 4 identical items + // should not register even though a fixed threshold of 3 would catch it. + const identical = detectSequencePeriod(["a", "a", "a", "a"], { + minPeriod: 1, + maxPeriod: 8, + minRepeats: (period) => (period === 1 ? 5 : 3), + }); + expect(identical.repeating).toBe(false); + + const cycle = detectSequencePeriod(["a", "b", "a", "b", "a", "b"], { + minPeriod: 1, + maxPeriod: 8, + minRepeats: (period) => (period === 1 ? 5 : 3), + }); + expect(cycle).toEqual({ repeating: true, period: 2, repeats: 3 }); + }); + + test("minDistinct rejects a degenerate monochrome match at a longer period", () => { + // "aaaa" is trivially periodic at every period, but period 1 already + // satisfies minRepeats first (ascending scan), so it never reaches a + // longer period where a distinct-unit floor would matter. Confirm the + // floor is still enforced when period 1 is excluded from the scan. + const result = detectSequencePeriod(["a", "a", "a", "a", "a", "a"], { + minPeriod: 2, + maxPeriod: 8, + minRepeats: 3, + minDistinct: () => 2, + }); + expect(result.repeating).toBe(false); + }); +}); diff --git a/src/util/period-detection.ts b/src/util/period-detection.ts new file mode 100644 index 000000000..ea9218a8b --- /dev/null +++ b/src/util/period-detection.ts @@ -0,0 +1,97 @@ +/** + * Generic exact-period detector over an ordered sequence: finds the shortest + * period p such that the tail of the sequence is p repeated at least the + * required number of times, with an optional distinct-unit floor to reject + * degenerate runs (e.g. a monochrome span that is trivially "periodic" at + * every length). + * + * Lifted out of tui-opentui/stall-watchdog.ts's character-stream detector — + * same shape (shortest-period-that-repeats-enough), generalized to run over + * any sequence of comparable items, not just characters. stall-watchdog's + * detectRepetition and director.ts's tool-fingerprint thrash check both + * delegate here rather than each hand-rolling the search. + */ + +export type SequencePeriodCheck = { + readonly repeating: boolean + readonly period: number | null + readonly repeats: number +} + +export type SequencePeriodOptions = { + readonly minPeriod: number + readonly maxPeriod: number + /** + * Repeats required for a period to count as a cycle. A fixed number, or a + * function of the candidate period when different period lengths warrant + * different bars. + */ + readonly minRepeats: number | ((period: number) => number) + readonly equals?: (a: T, b: T) => boolean + /** + * Minimum distinct units required within the repeating span itself, as a + * function of period. Omit to skip the check. + */ + readonly minDistinct?: (period: number) => number + /** Key used for the distinct-unit count when T is not itself string-safe. */ + readonly keyOf?: (item: T) => string +} + +/** + * Length of the exact-period run ending at the last element of `seq`, + * including the base period itself. Walks backwards from the end; stops at + * the first mismatch or the start of the sequence. + */ +function periodicSuffixLength( + seq: readonly T[], + period: number, + equals: (a: T, b: T) => boolean, +): number { + let i = seq.length - 1 + let j = i - period + let matched = 0 + while (j >= 0 && equals(seq[i] as T, seq[j] as T)) { + matched++ + i-- + j-- + } + return matched + period +} + +export function detectSequencePeriod( + seq: readonly T[], + options: SequencePeriodOptions, +): SequencePeriodCheck { + const equals = options.equals ?? ((a: T, b: T) => a === b) + const minRepeatsFor = + typeof options.minRepeats === "function" + ? options.minRepeats + : (() => { + const fixed = options.minRepeats as number + return () => fixed + })() + // Periods longer than seq.length / minRepeats cannot mathematically reach + // the occurrence threshold, so they are skipped rather than scanned — same + // optimization as the original character-stream detector. Only applies + // when minRepeats is a fixed number; a per-period function may allow + // longer periods a lower bar, so the full maxPeriod is scanned instead. + const maxPeriod = + typeof options.minRepeats === "number" + ? Math.min(options.maxPeriod, Math.floor(seq.length / options.minRepeats)) + : options.maxPeriod + + for (let period = options.minPeriod; period <= maxPeriod; period++) { + const matched = periodicSuffixLength(seq, period, equals) + const repeats = matched / period + if (repeats < minRepeatsFor(period)) continue + if (options.minDistinct !== undefined) { + const unit = seq.slice(seq.length - period) + const distinct = new Set( + unit.map((item) => (options.keyOf ? options.keyOf(item) : (item as unknown as string))), + ).size + if (distinct < options.minDistinct(period)) continue + } + return { repeating: true, period, repeats } + } + return { repeating: false, period: null, repeats: 0 } +} diff --git a/tests/unit/director.test.ts b/tests/unit/director.test.ts index d570b59a5..963733167 100644 --- a/tests/unit/director.test.ts +++ b/tests/unit/director.test.ts @@ -149,16 +149,21 @@ test("compaction is self-regulating: a cycle back under threshold does not re-co }); // --------------------------------------------------------------------------- -// Model-family policy: a grok provider must tighten the tool-only-loop pause -// threshold (10 turns) below the default (20), matching resolveModelFamilyPolicy. +// Model-family policy / main-session loop protection (CL-5611): a tool-only +// streak must not hard-pause on turn count alone — a Grok session hard-paused +// at 10 turns of real progress (Linear lookups + code reads) motivated +// replacing the count-only pause with a real no-progress signal (identical +// tool-call fingerprint repeating). See src/agent/director.test.ts for the +// full loop-protection coverage; these two cover the regression scenario +// directly against resolveModelFamilyPolicy's grok branch. // --------------------------------------------------------------------------- -function toolOnlyInferenceDone(callId: string): ReactorInboundEvent { +function toolOnlyInferenceDone(callId: string, path = "x.ts"): ReactorInboundEvent { return { type: "inference.done", turn: { role: "assistant", - content: [{ type: "tool_call", id: callId, name: "read_file", arguments: { path: "x.ts" } }], + content: [{ type: "tool_call", id: callId, name: "read_file", arguments: { path } }], model: "test-model", timestamp: 0, }, @@ -167,20 +172,31 @@ function toolOnlyInferenceDone(callId: string): ReactorInboundEvent { }; } -async function runToolOnlyStreak(director: ReturnType, turns: number) { +async function runToolOnlyStreak( + director: ReturnType, + turns: number, + varyPath = true, +) { let lastActions: ReactorAction[] = []; for (let i = 0; i < turns; i++) { - await director.decide(toolOnlyInferenceDone(`call-${i}`), state, makeCapabilities()); + await director.decide( + toolOnlyInferenceDone(`call-${i}`, varyPath ? `x-${i}.ts` : "x.ts"), + state, + makeCapabilities(), + ); const result = await director.decide(toolDoneTurn(`call-${i}`), state, makeCapabilities()); lastActions = Array.isArray(result) ? result : [result]; } return lastActions; } -test("a grok provider pauses the session after 10 tool-only turns, tighter than the default 20", async () => { - const grokDirector = createChatDirector("sys", [], { onTasksChange: () => {}, provider: { providerName: "xai", model: "grok-4" } }); +test("a grok provider no longer pauses a 10-turn productive tool-only streak", async () => { + const grokDirector = createChatDirector("sys", [], { + onTasksChange: () => {}, + provider: { providerName: "xai", model: "grok-4" }, + }); const grokActions = await runToolOnlyStreak(grokDirector, 10); - expect(grokActions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(true); + expect(grokActions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); const defaultDirector = createChatDirector("sys", [], { onTasksChange: () => {}, @@ -189,3 +205,16 @@ test("a grok provider pauses the session after 10 tool-only turns, tighter than const defaultActions = await runToolOnlyStreak(defaultDirector, 10); expect(defaultActions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(false); }); + +test("a grok provider still pauses when the same tool call repeats without progress", async () => { + const grokDirector = createChatDirector("sys", [], { + onTasksChange: () => {}, + provider: { providerName: "xai", model: "grok-4" }, + }); + // Identical-consecutive (period 1) needs 5 repeats, not 4 — 4 identical + // calls in a row is legitimate polling (rerunning a flaky test, checking a + // build) and must not false-positive. See src/agent/director.test.ts for + // the dedicated coverage of that distinction. + const grokActions = await runToolOnlyStreak(grokDirector, 5, /* varyPath */ false); + expect(grokActions.some((a) => a.type === "reply" && a.content.includes("Auto-paused"))).toBe(true); +}); diff --git a/tests/unit/telemetry-product-events.test.ts b/tests/unit/telemetry-product-events.test.ts index dde851194..522ce8aee 100644 --- a/tests/unit/telemetry-product-events.test.ts +++ b/tests/unit/telemetry-product-events.test.ts @@ -18,11 +18,12 @@ import { createSessionPruningCompactor } from "../../src/session/runtime-assembl import { createTaskTool } from "../../src/subagent/task-tool.js"; import { classifyAgentName, - classifyCommandName, classifyErrorClass, classifyPermissionKind, } from "../../src/telemetry/classify.js"; + import { createTelemetry, type Telemetry } from "../../src/telemetry/index.js"; +import { captureSlashCommand } from "../../src/telemetry/product-events.js"; import { captureAuthFailure, classifyAgentSendFailure, @@ -224,8 +225,10 @@ test("the built-in worker label is reported by name", () => { test("slash_command buckets a plugin-registered command to \"custom\"", async () => { const { telemetry, wire, events } = harness(); - telemetry.capture("slash_command", { command_name: classifyCommandName("acmecorp-deploy") }); - telemetry.capture("slash_command", { command_name: classifyCommandName("settings") }); + // Shared product-event helper — not the TUI runner — so headless and TUI + // callers hit the same emission path (CL-5744). + captureSlashCommand(telemetry, "acmecorp-deploy"); + captureSlashCommand(telemetry, "settings"); const captured = await events(); expect(captured[0]?.properties.command_name).toBe("custom"); @@ -273,6 +276,7 @@ test("auth_failure names the provider and never ships the rejection message", as const rejections = [ codexRejection, new Error('xai profile "acmecorp-eng" is not authorized.'), + new Error('anthropic authentication_error: invalid x-api-key for acmecorp-eng'), new Error("connection reset by /Users/someone/acmecorp"), ]; for (const err of rejections) { @@ -288,8 +292,16 @@ test("auth_failure names the provider and never ships the rejection message", as ); const captured = await events(); - expect(captured.map((e) => e.event)).toEqual(["auth_failure", "auth_failure"]); - expect(captured.map((e) => e.properties.auth_provider)).toEqual(["codex", "xai"]); + expect(captured.map((e) => e.event)).toEqual([ + "auth_failure", + "auth_failure", + "auth_failure", + ]); + expect(captured.map((e) => e.properties.auth_provider)).toEqual([ + "codex", + "xai", + "anthropic", + ]); const body = await wire(); expect(body).not.toContain("acmecorp"); expect(body).not.toContain("error_class"); diff --git a/tests/unit/telemetry-singleton.test.ts b/tests/unit/telemetry-singleton.test.ts index e43ac9db8..61477a263 100644 --- a/tests/unit/telemetry-singleton.test.ts +++ b/tests/unit/telemetry-singleton.test.ts @@ -1,23 +1,28 @@ import { test, expect } from "bun:test"; import { getTelemetry, setTelemetry } from "../../src/telemetry/singleton.js"; +import { NOOP_TELEMETRY } from "../../src/telemetry/index.js"; test("getTelemetry defaults to a disabled no-op that never throws", () => { const telemetry = getTelemetry(); expect(telemetry.enabled).toBe(false); expect(() => telemetry.capture("cli_start")).not.toThrow(); + expect(telemetry.captureIntentional("survey sent")).toBe(false); }); test("setTelemetry replaces the process-wide instance", () => { let captured: string | undefined; setTelemetry({ enabled: true, + installationId: "test-install", capture: (event) => { captured = event; }, + captureIntentional: () => false, flush: async () => {}, + discard: () => {}, }); getTelemetry().capture("session_end"); expect(captured).toBe("session_end"); // Reset so other tests in this process see the default again. - setTelemetry({ enabled: false, capture: () => {}, flush: async () => {} }); + setTelemetry(NOOP_TELEMETRY); }); diff --git a/tests/unit/telemetry-toggle.test.ts b/tests/unit/telemetry-toggle.test.ts index ed9fc641a..2e85af68c 100644 --- a/tests/unit/telemetry-toggle.test.ts +++ b/tests/unit/telemetry-toggle.test.ts @@ -43,7 +43,7 @@ function fakeDeps(overrides: Partial = {}): { test("toggle off disables the singleton synchronously, before any await", () => { const { deps, getInstance } = fakeDeps(); const handler = createTelemetryToggleHandler("/fake/path", deps); - handler(false); + expect(handler(false)).toBe(true); // No awaited microtask has run yet — assert on the return of the sync call. expect(getInstance().enabled).toBe(false); }); @@ -129,7 +129,9 @@ test("toggle on while env-killed writes nothing and swaps no instance", async () let saveCalled = false; const initial: Telemetry = { enabled: false, + installationId: "", capture: () => {}, + captureIntentional: () => false, flush: async () => {}, discard: () => {}, }; @@ -149,7 +151,7 @@ test("toggle on while env-killed writes nothing and swaps no instance", async () }, }); const handler = createTelemetryToggleHandler("/fake/path", deps); - handler(true); + expect(handler(true)).toBe(false); await new Promise((resolve) => setTimeout(resolve, 10)); expect(ensureCalled).toBe(false); expect(saveCalled).toBe(false); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 42e833c24..b815f30b2 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -160,9 +160,9 @@ test("capture strips properties not in $ai_generation's allowlist", async () => $ai_output_tokens: 20, $ai_latency: 0.4, $ai_is_error: false, - cache_read_tokens: 1, - cache_write_tokens: 2, - thinking_tokens: 3, + $ai_cache_read_input_tokens: 1, + $ai_cache_creation_input_tokens: 2, + $ai_reasoning_tokens: 3, prompt: "should-not-appear", completion: "should-not-appear", });