diff --git a/CHANGELOG.md b/CHANGELOG.md index aa8ee49..f2245b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased (master, since v0.1.38) - **feat(delegate): 按角色配置默认模型与 thinking level (closes #117)** — `acp_delegate` 现支持为不同角色持久化配置默认模型与 reasoning/thinking level：`delegate.agents..model`（`"provider/id"`）、`delegate.agents..thinkingLevel`，以及全局 `delegate.thinkingLevel`。模型解析优先级 per-call `model` > 角色默认 > 父 Agent 当前模型；thinking level 优先级 per-call > 角色 > 全局 > Pi 默认。角色配置的模型经 `ctx.modelRegistry.find()` 校验，不存在时回退到父模型并记警告（**绝不失败**，omo 教训）；per-call / 继承的模型原样透传（允许自定义非目录模型）。无任何配置时行为完全不变（继承父模型 + Pi 默认）。子进程 CLI 新增 `--thinking ` 透传（pi 支持 off|minimal|low|medium|high|xhigh|max，非法值告警不失败）；`acp_delegate` 新增可选 `thinkingLevel` 参数 - **feat(delegate): `delegate.maxConcurrent` 后台子代理并发上限 / 强制串行开关（closes #294）** — 此前每个 `acp_delegate` async 调用立即 spawn 独立 pi 进程，无并发上限；低性能机器上并行子代理争抢 CPU 导致全部 watchdog 超时退出，主代理被迫自己干完。新增数值配置 `delegate.maxConcurrent`（默认 unlimited）：限制**同时运行**的后台（async）delegate 数量，达上限后后续启动进入 FIFO 队列、有空位自动开始（不丢弃、只是等待）。`1` = 强制严格串行。优先级 env `PI_ACP_DELEGATE_MAX_CONCURRENT` > acp.json > unlimited，无效值（非整数 / `<1`）带警告回退而非失败。新增 `RunStatus:"queued"`：排队 run 立即可被 wait（挂起 waiter 跨排队→终态）与 cancel（释放槽位并显式唤醒挂起的 waiter，因其无子进程触发 finalize）；watchdog 计时从实际 spawn 起算（不含排队时间）；remaining-counts 计入 queued。sync 调用始终立即运行、不受影响。实现：`ConcurrencyGate`（provider 容量、FIFO、cancel 只标记不删除以防槽位泄漏）+ `spawnDelegateChild` 抽取；并发上限解析并入 `resolveDelegate`/`DelegatePolicy`；工具描述在有限制时动态提示；CONFIGURATION.md(+zh-CN) 文档。测试：`resolveDelegate().maxConcurrent` 解析 + ConcurrencyGate（FIFO/容量 N/cancel-跳过/无限）+ env>acp.json 解析与无效回退 +- **fix(guardrail): 通用工具调用重复熔断 — 打断字节级相同调用的死循环（closes #308）** — 贪心解码小模型会把完全一致的 `(工具调用 → 工具返回)` 对逐轮自我强化（序列级吸引子，token 级惩罚跨轮无效），实测 `acp_status` 同参连发 30 次、上下文 57K→130K。现有防线全部失效：`compress-retry-capped` 熔断只统计 compress 失败（acp_status 永远成功 → 零计数）、nudge 去重按 turnKey（循环期无新用户消息 → 全程 `nudge-suppressed`）、`tool-guardrails.ts` 只有 bash 输出上限/超时检测。新增 `repetitionGuard`（默认开启，可配 `{enabled,warn,abort}`）：每次调用指纹化 `sha1(toolName + 键排序 canonical JSON(args))`，只比参数不比结果（结果里可见消息数会微变也不影响命中）；会话内跟踪连续相同次数，达 `warn`(默认 3) 在该 toolResult 尾部追加强警告，达 `abort`(默认 5) 拦截该调用不执行 + 中止本轮 + 终端通知；参数变化 / 换工具 / 真实用户消息重置计数（扩展自发消息不重置）。实现挂 `src/tool-guardrails.ts`（`RepetitionTracker` / `canonicalStringify` / `repetitionFingerprint`），配置解析在 `src/config.ts`（`resolveRepetitionGuard` / `REPETITION_GUARD_DEFAULTS`） - **fix(delegate): 并发完成的 delegate 通知合并为单条批量消息（closes #157）** — 多个 subagent 同时（或主窗口正忙时接连）完成时，每条完成各自 `sendUserMessage(deliverAs: followUp)` 注入一条通知，N 个完成的 delegate 吃掉 N 个完整模型轮次，且模型已收尾后通知仍在持续涌入。现在 finalize 不再直接注入，而是进入 2s 尾沿防抖窗口（自首个排队完成起硬上限 10s，防连续错峰完成饿死投递）；窗口关闭时 `flushDelegateNotifications` 发**一条**批量消息：头部计数（`[acp_delegate] 3 delegates finished (2 completed, 1 FAILED)`）+ 每个 run 一节（状态/exit code/超时注记/任务/结果文件/失败错误摘录）+ 单一尾部（仍在跑的 delegate 数、session delegate usage、收尾指令）。窗口期内获得 waiter 或被 `acp_delegate_wait`/`acp_delegate_cancel` 消费的 run 自动出批（不重复投递）；发送失败不置 delivered，run 留在未送达集由后续 carrier 补投（`findUndeliveredRuns` 将排队中视为已排程而非丢失）；单 run flush 保留原单条格式。净效果：N 个同时完成 → 1 个模型轮次 - **feat(delegate): 失败/取消保留日志 + 失败诊断 + `resumeFrom` 续跑中断的 run (closes #235)** — 此前 cancel 与 spawn error 路径直接删除 `.out`/`.activity` 文件，失败通知只有 exit code（信号被丢弃、stderr 可能为空、activity 轨迹不可见）。现在：① 所有终止路径保留文件（spawn error 把错误写入 `.out`；cancel 回填部分回复，cancel/wait 结果明确给出文件路径）；② 失败通知带 exit 信号（`exit SIGTERM`）、stderr、activity 日志尾部（400 字符）与 activity 文件路径；③ pi 宿主 delegate 用 `--session /.session.jsonl` + `--session-dir` 持久化自身会话（omp 不变，保持 `--no-session`），新增 `resumeFrom: ""` 参数让新 run 恢复原会话（原任务 + 已执行的 tool calls + 部分结果）从中断处继续——`task` 变为可选（提供时作为本次追加指引）；校验：原 run 不得仍在运行、session 文件必须存在、非 pi 宿主拒绝 - **fix(floor): provider-usage floor 跳过压缩后一轮的 stale anchor(#258 评审)** — pi 的 `getContextUsage()` 锚定最后一条有效 assistant usage;成功 compress 落在锚点之后时,下一个 LLM 调用仍以压缩前的大数字 floor,会在刚缩小的上下文上重跑 emergency(nudge 注入 + 工具结果机械截断),面板也继续显示压缩前数字,直到下一条 usage 到达。新增 `src/floor-stale.ts`(`usageAnchorPredatesCompression`):锚点(跳过 aborted/error/全零)早于最后一条成功 compress toolResult(失败/0-block no-op 不算)时跳过 floor,context transform / `acp_status` / `/acp` 三处一致(面板 sessionTokens 同步);对齐 pi 自身 compaction 的 "usage source must be post-compaction" 检查。已知接受:provider 永不报 usage 时(omp #18 tree-sum 回退)floor 仍会永久高位 diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 3933d79..df4d206 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -100,6 +100,7 @@ All keys below are currently **ACTIVE**. | `toolBashDefaultTimeout` | number | `60` | 🟢 ACTIVE | Default `bash` tool timeout in seconds when the model omits it. | | `toolOutputMaxBytes` | number | `200000` | 🟢 ACTIVE | Hard byte cap on tool result text. | | `throttleRetry` | boolean \| object | `true` | 🟢 ACTIVE | Auto-retry provider token rate-limit errors with progressive backoff. | +| `repetitionGuard` | boolean \| object | `true` | 🟢 ACTIVE | Break infinite loops of byte-identical tool calls (warn at 3 consecutive, block + abort at 5). | **Delegate keys** @@ -125,6 +126,14 @@ All keys below are currently **ACTIVE**. | `throttleRetry.maxDelayMs` | number | `300000` | 🟢 ACTIVE | Cap for paced kick delays. | | `throttleRetry.backoffMode` | string | `"exponential"` | 🟢 ACTIVE | Delay progression: `"exponential"` (×2 per kick) or `"fixed"`. | +**Repetition guard keys** + +| Key | Type | Default | Status | Description | +|-----|------|---------|--------|-------------| +| `repetitionGuard.enabled` | boolean | `true` | 🟢 ACTIVE | Enable the repetition breaker. `false` disables it entirely. | +| `repetitionGuard.warn` | number | `3` | 🟢 ACTIVE | Consecutive byte-identical calls before a strong warning is appended to the tool result. | +| `repetitionGuard.abort` | number | `5` | 🟢 ACTIVE | Consecutive byte-identical calls before the call is blocked (not executed) and the turn is aborted. Must exceed `warn`. | + **Compression keys** | Key | Type | Default | Status | Description | @@ -363,6 +372,57 @@ How it works: --- +## Tool Call Repetition Guard + +The `repetitionGuard` key breaks **infinite loops of byte-identical tool calls**. Small greedy-decoding models can get stuck re-emitting the exact same `(tool call → tool result)` pair turn after turn — e.g. calling `acp_status {"scope":"uncompressed","view":"ranges"}` dozens of times with identical arguments while context grows each round. Token-level penalties cannot break this, because it is a *sequence-level* attractor (the repetition crosses turn boundaries), not a within-sequence token repetition. + +The guard fingerprints each tool call as `sha1(toolName + canonical-JSON(args))`, where the JSON serialization sorts object keys so that only the *arguments* matter — key order and result content are ignored. It tracks the length of the current run of consecutive identical calls per session: + +- At **`warn`** consecutive identical calls, a strong warning is appended to the matching tool result telling the model to stop repeating the call. +- At **`abort`** consecutive identical calls, the call is **blocked** (not executed), the turn is aborted, and a terminal notification is shown. + +Any change to the arguments (or a switch to a different tool) resets the counter, as does a real user message (extension-sent messages do not reset it). + +### `repetitionGuard` + +- **Type:** boolean \| object +- **Default:** `true` +- **Status:** 🟢 ACTIVE +- **Description:** Enable/disable the repetition breaker and tune its thresholds. `repetitionGuard: false` disables it entirely. Object form (any subset): + + ```json + { + "repetitionGuard": { + "enabled": true, + "warn": 3, + "abort": 5 + } + } + ``` + +### `repetitionGuard.enabled` + +- **Type:** boolean +- **Default:** `true` +- **Status:** 🟢 ACTIVE +- **Description:** Turn the feature on/off. `false` (or top-level `repetitionGuard: false`) disables all repetition detection. + +### `repetitionGuard.warn` + +- **Type:** number +- **Default:** `3` +- **Status:** 🟢 ACTIVE +- **Description:** Number of consecutive byte-identical calls before a warning is appended to the tool result. Must be at least 1. + +### `repetitionGuard.abort` + +- **Type:** number +- **Default:** `5` +- **Status:** 🟢 ACTIVE +- **Description:** Number of consecutive byte-identical calls before the call is blocked and the turn is aborted. Must be greater than `warn`; if misconfigured lower, it is clamped up to `warn + 1`. + +--- + ## Compression Tuning The `compress` sub-object groups the three thresholds that form a **three-tier escalation** for context management. They control *when* the model is nudged to compress and *when* large outputs are forcibly truncated to keep the session alive. Lower thresholds mean the extension compresses earlier and more aggressively. diff --git a/CONFIGURATION.zh-CN.md b/CONFIGURATION.zh-CN.md index f8509f8..3d3ce08 100644 --- a/CONFIGURATION.zh-CN.md +++ b/CONFIGURATION.zh-CN.md @@ -99,6 +99,7 @@ | `toolBashDefaultTimeout` | number | `60` | 🟢 ACTIVE | 模型省略 `timeout` 时注入 bash 工具的默认超时秒数。 | | `toolOutputMaxBytes` | number | `200000` | 🟢 ACTIVE | 工具返回文本的硬性字节上限。 | | `throttleRetry` | boolean \| object | `true` | 🟢 ACTIVE | 自动重试 provider 侧 token 限流错误（递进退避）。 | +| `repetitionGuard` | boolean \| object | `true` | 🟢 ACTIVE | 打断字节级完全相同的工具调用死循环（连续 3 次告警，连续 5 次拦截并中止本轮）。 | **delegate 键** @@ -124,6 +125,14 @@ | `throttleRetry.maxDelayMs` | number | `300000` | 🟢 ACTIVE | 递进 kick 延迟上限。 | | `throttleRetry.backoffMode` | string | `"exponential"` | 🟢 ACTIVE | 延迟递进方式：`"exponential"`（每次 kick ×2）或 `"fixed"`。 | +**重复熔断键** + +| 键 | 类型 | 默认值 | 状态 | 说明 | +|----|------|--------|------|------| +| `repetitionGuard.enabled` | boolean | `true` | 🟢 ACTIVE | 启用重复熔断。`false` 完全关闭。 | +| `repetitionGuard.warn` | number | `3` | 🟢 ACTIVE | 连续字节级相同调用达到该次数后，在工具返回尾部追加强警告。 | +| `repetitionGuard.abort` | number | `5` | 🟢 ACTIVE | 连续字节级相同调用达到该次数后，拦截该调用（不执行）并中止本轮。必须大于 `warn`。 | + **compress 键** | 键 | 类型 | 默认值 | 状态 | 说明 | @@ -355,6 +364,57 @@ --- +## 工具调用重复熔断 + +`repetitionGuard` 键用于打断**字节级完全相同的工具调用死循环**。小模型在贪心解码下可能卡住，逐轮重复发出完全一致的 `(工具调用 → 工具返回)` 对——例如带着相同参数几十次调用 `acp_status {"scope":"uncompressed","view":"ranges"}`，而上下文每轮都在增长。token 级惩罚无法打破这种循环，因为它是**序列级吸引子**（重复跨越轮次边界），而非序列内的 token 重复。 + +该熔断器把每次工具调用指纹化为 `sha1(toolName + canonical-JSON(args))`，其中 JSON 序列化会对对象键排序，因此只比较*参数*本身——键顺序与返回内容都不影响判定。它在会话内跟踪当前连续相同调用的长度： + +- 连续相同调用达到 **`warn`** 次时，在对应工具返回尾部追加一条强警告，提示模型停止重复该调用； +- 连续相同调用达到 **`abort`** 次时，**拦截**该调用（不执行）、中止本轮，并在终端弹出通知。 + +任何参数变化（或切换到另一个工具）都会重置计数；一条真实用户消息也会重置（扩展自己发送的消息不会重置）。 + +### `repetitionGuard` + +- **类型：** boolean \| object +- **默认值：** `true` +- **状态：** 🟢 ACTIVE +- **说明：** 启用/禁用重复熔断并调整阈值。`repetitionGuard: false` 完全关闭该功能。object 形式（任意子集）： + + ```json + { + "repetitionGuard": { + "enabled": true, + "warn": 3, + "abort": 5 + } + } + ``` + +### `repetitionGuard.enabled` + +- **类型：** boolean +- **默认值：** `true` +- **状态：** 🟢 ACTIVE +- **说明：** 开关。`false`（或顶层 `repetitionGuard: false`）禁用所有重复检测。 + +### `repetitionGuard.warn` + +- **类型：** number +- **默认值：** `3` +- **状态：** 🟢 ACTIVE +- **说明：** 连续字节级相同调用达到该次数后，在工具返回尾部追加警告。最小为 1。 + +### `repetitionGuard.abort` + +- **类型：** number +- **默认值：** `5` +- **状态：** 🟢 ACTIVE +- **说明：** 连续字节级相同调用达到该次数后，拦截该调用并中止本轮。必须大于 `warn`；若误配成更小值会被向上钳制到 `warn + 1`。 + +--- + ## 压缩调优 `compress` 子对象包含三个阈值，构成上下文管理的**三级递进**。它们控制模型*何时*被 nudge 压缩，以及大输出*何时*被强制截断以维持会话存活。阈值越低，扩展压缩得越早、越激进。 diff --git a/src/config.ts b/src/config.ts index f6163c5..09aa42f 100644 --- a/src/config.ts +++ b/src/config.ts @@ -150,9 +150,32 @@ export interface CompressConfig extends CompressSettings { providers?: Record; } +/** Generic tool-call repetition guard. Detects the same tool being called + * repeatedly with byte-identical arguments — a sequence-level attractor that + * token-level penalties cannot break (greedy small models loop on e.g. + * acp_status or bash polls forever, each round adding protected tokens that + * the compression protection band then refuses to reclaim). Consecutive + * identical calls accumulate per session: at `warn` a strong warning is + * appended to that call's toolResult; at `abort` the call is refused (blocked, + * error toolResult) and the turn aborted so the attractor breaks. Any change + * to the arguments (or a switch to a different tool) resets the counter. + * Accepts a boolean shorthand (`false` disables) or an object. Default: + * enabled, warn=3, abort=5. See issue #308. */ +export interface RepetitionGuardConfig { + /** Enable/disable the guard. Default: true. */ + enabled?: boolean; + /** Consecutive identical calls before a strong warning is appended to the + * matching toolResult. Default: 3. */ + warn?: number; + /** Consecutive identical calls before the call is refused and the turn + * aborted. Default: 5. Must be greater than `warn`; clamped up + * automatically if not. */ + abort?: number; +} + /** * Adapter configuration. Maps onto acp-kernel's `Config` plus Pi-specific knobs - * (live model context window, protected tools, state persistence). + * (live model context window, protected tools, state persistence). */ export interface AdapterConfig { /** Master switch. Default: true. Set `enabled: false` in acp.json (or @@ -202,6 +225,11 @@ export interface AdapterConfig { * disables) or a ThrottleRetryConfig object. Default: enabled, 10 retries, * 60s exponential base capped at 300s per kick. */ throttleRetry?: boolean | ThrottleRetryConfig; + /** Generic tool-call repetition guard (see RepetitionGuardConfig). Accepts a + * boolean shorthand (`false` disables) or an object. Default: enabled, + * warn=3, abort=5. Stops greedy small models looping on byte-identical + * tool calls (issue #308). */ + repetitionGuard?: boolean | RepetitionGuardConfig; /** Legacy flat alias for `delegate.displayUsage`. Kept for backward * compatibility with existing acp.json files. Prefer `delegate.displayUsage`. */ displayUsage?: "merged" | "separate"; @@ -295,6 +323,28 @@ function resolveMaxConcurrent(envValue: string | undefined, cfgValue: number | u return DEFAULT_DELEGATE_POLICY.maxConcurrent; } +/** Defaults for the generic tool-call repetition guard (issue #308). */ +export const REPETITION_GUARD_DEFAULTS = { warn: 3, abort: 5 } as const; + +function positiveInt(v: unknown, fallback: number): number { + return typeof v === "number" && Number.isFinite(v) && v >= 1 ? Math.floor(v) : fallback; +} + +/** Resolve the repetition-guard configuration from the adapter, handling the + * boolean shorthand (`false` disables) and clamping `abort` to stay strictly + * above `warn` (a guard that aborts at or before its warn threshold would be + * meaningless). Non-numeric / out-of-range values fall back to defaults. */ +export function resolveRepetitionGuard(adapter: AdapterConfig): { enabled: boolean; warn: number; abort: number } { + const g = adapter.repetitionGuard; + if (g === false) return { enabled: false, warn: REPETITION_GUARD_DEFAULTS.warn, abort: REPETITION_GUARD_DEFAULTS.abort }; + if (typeof g === "object" && g !== null) { + const warn = positiveInt(g.warn, REPETITION_GUARD_DEFAULTS.warn); + const abort = Math.max(positiveInt(g.abort, REPETITION_GUARD_DEFAULTS.abort), warn + 1); + return { enabled: g.enabled !== false, warn, abort }; + } + return { enabled: true, warn: REPETITION_GUARD_DEFAULTS.warn, abort: REPETITION_GUARD_DEFAULTS.abort }; +} + /** Per-field deepest-wins merge of the three compression levels (global → * provider → model). An undefined field at a deeper level does NOT clear a * value set at a shallower level — only a defined value overrides. */ diff --git a/src/tool-guardrails.ts b/src/tool-guardrails.ts index 2ea8171..1bc1e78 100644 --- a/src/tool-guardrails.ts +++ b/src/tool-guardrails.ts @@ -1,9 +1,10 @@ +import { createHash } from "node:crypto"; import { isToolCallEventType, type ExtensionAPI, type ToolResultEvent, } from "@earendil-works/pi-coding-agent"; -import { DEFAULT_TOOL_BASH_TIMEOUT, DEFAULT_TOOL_OUTPUT_MAX_BYTES } from "./config.js"; +import { DEFAULT_TOOL_BASH_TIMEOUT, DEFAULT_TOOL_OUTPUT_MAX_BYTES, resolveRepetitionGuard } from "./config.js"; import { debug, logInfo, logWarn } from "./log.js"; import type { AcpRuntime } from "./runtime.js"; @@ -61,11 +62,10 @@ export function detectBashTimeout(content: ToolResultEvent["content"]): number | return undefined; } -export function appendTimeoutNotice( +function appendTrailingText( content: ToolResultEvent["content"], - secs: number, + notice: string, ): ToolResultEvent["content"] { - const notice = buildTimeoutNotice(secs); const next = [...content]; for (let i = next.length - 1; i >= 0; i--) { const part = next[i]; @@ -78,6 +78,13 @@ export function appendTimeoutNotice( return next; } +export function appendTimeoutNotice( + content: ToolResultEvent["content"], + secs: number, +): ToolResultEvent["content"] { + return appendTrailingText(content, buildTimeoutNotice(secs)); +} + function keepHead(str: string, maxBytes: number): string { const buf = Buffer.from(str, "utf8"); if (buf.length <= maxBytes) return str; @@ -109,13 +116,115 @@ function formatBytes(n: number): string { return n >= 1024 ? `${(n / 1024).toFixed(1)}KB` : `${n}B`; } +// --- Generic tool-call repetition guard (issue #308) --- +// Token-level penalties act on within-sequence tokens and cannot break a +// sequence-level attractor where a greedy small model re-emits the same +// (assistant -> toolResult) pair every turn. The only reliable signal is the +// byte-for-byte identity of consecutive tool calls. We track the length of the +// current consecutive run per session: at `warn` we append a strong warning to +// the matching toolResult; at `abort` we refuse the call (block + error +// toolResult) and abort the turn so the loop breaks. Any argument change +// (or a switch to another tool) resets the run. +export function canonicalStringify(value: unknown): string { + if (value === null || value === undefined || typeof value !== "object") { + try { + return JSON.stringify(value) ?? "null"; + } catch { + return String(value); + } + } + if (Array.isArray(value)) { + return `[${value.map((v) => canonicalStringify(v)).join(",")}]`; + } + const entries = Object.entries(value as Record) + .filter(([, v]) => v !== undefined) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)); + return `{${entries.map(([k, v]) => `${JSON.stringify(k)}:${canonicalStringify(v)}`).join(",")}}`; +} + +export function repetitionFingerprint(toolName: string, input: unknown): string { + const payload = `${toolName}\u0000${canonicalStringify(input ?? {})}`; + try { + return createHash("sha1").update(payload).digest("hex"); + } catch { + return `unhashable:${toolName}`; + } +} + +export type RepetitionAction = "none" | "warn" | "abort"; +export interface RepetitionDecision { + action: RepetitionAction; + count: number; + fingerprint: string; + toolName: string; +} + +export class RepetitionTracker { + private lastFp: string | null = null; + private count = 0; + constructor(private readonly thresholds: { warn: number; abort: number }) {} + note(toolName: string, input: unknown): RepetitionDecision { + const fp = repetitionFingerprint(toolName, input); + if (fp === this.lastFp) this.count += 1; + else { + this.lastFp = fp; + this.count = 1; + } + let action: RepetitionAction = "none"; + if (this.count >= this.thresholds.abort) action = "abort"; + else if (this.count >= this.thresholds.warn) action = "warn"; + return { action, count: this.count, fingerprint: fp, toolName }; + } + reset(): void { + this.lastFp = null; + this.count = 0; + } +} + +function buildRepetitionWarnNotice(toolName: string, count: number): string { + return `\n\n[ACP guardrail: you have issued ${count} CONSECUTIVE identical \`${toolName}\` calls (name + arguments byte-for-byte identical). Re-running the exact same call will not produce a new result. STOP issuing this identical call — change your approach, change the arguments, or stop and report to the user.]`; +} + +function buildRepetitionAbortNotice(toolName: string, count: number): string { + return `[ACP guardrail: BLOCKED — you issued ${count} consecutive identical \`${toolName}\` calls (byte-for-byte identical arguments). This call was refused and NOT executed. You MUST change strategy: use different arguments, a different tool, or stop and report to the user. Do not re-issue the identical call. The turn has been aborted.`; +} + export function wireToolGuardrails(pi: ExtensionAPI, runtime: AcpRuntime): void { - pi.on("tool_call", (event) => { - if (!isToolCallEventType("bash", event)) return; - const t = resolveBashTimeout(event.input, runtime.adapter.toolBashDefaultTimeout); - if (t !== undefined) { - event.input.timeout = t; - debug.event("guardrail-bash-timeout", { applied: t }); + const trackers = new Map(); + const pendingWarns = new Map(); + + pi.on("tool_call", (event, ctx) => { + if (isToolCallEventType("bash", event)) { + const t = resolveBashTimeout(event.input, runtime.adapter.toolBashDefaultTimeout); + if (t !== undefined) { + event.input.timeout = t; + debug.event("guardrail-bash-timeout", { applied: t }); + } + } + const cfg = resolveRepetitionGuard(runtime.adapter); + if (!cfg.enabled) return; + const sid = ctx.sessionManager.getSessionId(); + let tracker = trackers.get(sid); + if (!tracker) { + tracker = new RepetitionTracker(cfg); + trackers.set(sid, tracker); + } + const decision = tracker.note(event.toolName, event.input); + debug.event("guardrail-repetition", { sid, tool: decision.toolName, count: decision.count, action: decision.action }); + if (decision.action === "abort") { + const msg = buildRepetitionAbortNotice(decision.toolName, decision.count); + logWarn("guardrail", { event: "repetition-abort", sid, tool: decision.toolName, count: decision.count }); + if (ctx.hasUI) ctx.ui.notify(`[ACP] ${msg}`, "warning"); + try { + ctx.abort(); + } catch { + // mid-tool abort is best-effort; the block below guarantees the refusal + } + return { block: true, reason: msg }; + } + if (decision.action === "warn") { + logInfo("guardrail", { event: "repetition-warn", sid, tool: decision.toolName, count: decision.count }); + pendingWarns.set(event.toolCallId, decision.count); } }); @@ -143,6 +252,21 @@ export function wireToolGuardrails(pi: ExtensionAPI, runtime: AcpRuntime): void logInfo("guardrail", { event: "bash-timeout-notice", secs: timeoutSecs }); } + const warnCount = pendingWarns.get(event.toolCallId); + if (warnCount !== undefined) { + pendingWarns.delete(event.toolCallId); + modified = appendTrailingText(modified ?? event.content, buildRepetitionWarnNotice(event.toolName, warnCount)); + debug.event("guardrail-repetition-warn-injected", { tool: event.toolName, count: warnCount }); + } + if (modified) return { content: modified }; }); + + pi.on("input", (event, ctx) => { + if (event.source === "extension") return; + trackers.get(ctx.sessionManager.getSessionId())?.reset(); + }); + pi.on("session_shutdown", (_e, ctx) => { + trackers.delete(ctx.sessionManager.getSessionId()); + }); } diff --git a/tests/config.test.ts b/tests/config.test.ts index b368a93..c962832 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,6 +1,6 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { resolveConfig, resolveCompress, mergeCompress, resolveDelegate, type AdapterConfig } from "../src/config.js"; +import { resolveConfig, resolveCompress, mergeCompress, resolveDelegate, resolveRepetitionGuard, REPETITION_GUARD_DEFAULTS, type AdapterConfig } from "../src/config.js"; const EMPTY: AdapterConfig = {}; @@ -371,3 +371,47 @@ test("resolveConfig without provider/modelId behaves as before (global only)", ( const cfg = resolveConfig({ compress: { maxContextLimit: "80%" } }, 1_000_000); assert.equal(cfg.nudge.maxContextLimitPct, 0.8); }); + +test("resolveRepetitionGuard defaults to enabled with warn=3 abort=5", () => { + const r = resolveRepetitionGuard(EMPTY); + assert.equal(r.enabled, true); + assert.equal(r.warn, 3); + assert.equal(r.abort, 5); +}); + +test("resolveRepetitionGuard defaults match the exported defaults constant", () => { + const r = resolveRepetitionGuard(EMPTY); + assert.equal(r.warn, REPETITION_GUARD_DEFAULTS.warn); + assert.equal(r.abort, REPETITION_GUARD_DEFAULTS.abort); +}); + +test("resolveRepetitionGuard boolean false shorthand disables the guard", () => { + const r = resolveRepetitionGuard({ repetitionGuard: false }); + assert.equal(r.enabled, false); +}); + +test("resolveRepetitionGuard object with explicit warn/abort", () => { + const r = resolveRepetitionGuard({ repetitionGuard: { warn: 4, abort: 8 } }); + assert.equal(r.enabled, true); + assert.equal(r.warn, 4); + assert.equal(r.abort, 8); +}); + +test("resolveRepetitionGuard clamps abort to at least warn+1 when misconfigured", () => { + const r = resolveRepetitionGuard({ repetitionGuard: { warn: 5, abort: 2 } }); + assert.ok(r.abort > r.warn, "abort must be strictly greater than warn"); + assert.equal(r.abort, 6); +}); + +test("resolveRepetitionGuard falls back for non-positive thresholds", () => { + const r = resolveRepetitionGuard({ repetitionGuard: { warn: 0, abort: -1 } }); + assert.equal(r.warn, 3); + assert.equal(r.abort, 5); +}); + +test("resolveRepetitionGuard keeps custom thresholds while honoring enabled:false", () => { + const r = resolveRepetitionGuard({ repetitionGuard: { enabled: false, warn: 4, abort: 9 } }); + assert.equal(r.enabled, false); + assert.equal(r.warn, 4); + assert.equal(r.abort, 9); +}); diff --git a/tests/tool-guardrails.test.ts b/tests/tool-guardrails.test.ts index 0207ced..2492934 100644 --- a/tests/tool-guardrails.test.ts +++ b/tests/tool-guardrails.test.ts @@ -7,8 +7,11 @@ import { appendTimeoutNotice, isBashToolResult, wireToolGuardrails, + canonicalStringify, + repetitionFingerprint, + RepetitionTracker, } from "../src/tool-guardrails.js"; -import type { ExtensionAPI, ToolResultEvent } from "@earendil-works/pi-coding-agent"; +import type { ExtensionAPI, ToolCallEvent, ToolResultEvent } from "@earendil-works/pi-coding-agent"; import type { AcpRuntime } from "../src/runtime.js"; type Content = ToolResultEvent["content"]; @@ -182,3 +185,181 @@ test("wireToolGuardrails honors an explicit smaller cap", () => { const t = textOf(ret.content[0]); assert.ok(Buffer.byteLength(t, "utf8") < 2000, "payload must be truncated to the explicit cap"); }); + +// --- generic tool-call repetition guard (issue #308) --- + +test("canonicalStringify is key-order independent for objects", () => { + assert.equal(canonicalStringify({ a: 1, b: 2 }), canonicalStringify({ b: 2, a: 1 })); +}); + +test("canonicalStringify preserves array order (arrays are ordered)", () => { + assert.notEqual(canonicalStringify([1, 2]), canonicalStringify([2, 1])); +}); + +test("canonicalStringify recurses into nested objects and drops undefined values", () => { + assert.equal( + canonicalStringify({ x: { p: 1, q: 2 }, y: 3 }), + canonicalStringify({ y: 3, x: { q: 2, p: 1 } }), + ); + assert.equal(canonicalStringify({ a: 1, b: undefined }), canonicalStringify({ a: 1 })); +}); + +test("repetitionFingerprint is stable across argument key order", () => { + assert.equal( + repetitionFingerprint("acp_status", { scope: "uncompressed", view: "ranges" }), + repetitionFingerprint("acp_status", { view: "ranges", scope: "uncompressed" }), + ); +}); + +test("repetitionFingerprint changes when any argument differs", () => { + const base = repetitionFingerprint("acp_status", { scope: "uncompressed", view: "ranges" }); + assert.notEqual(base, repetitionFingerprint("acp_status", { scope: "uncompressed", view: "messages" })); + assert.notEqual(base, repetitionFingerprint("acp_status", {})); +}); + +test("repetitionFingerprint distinguishes different tools with identical args", () => { + assert.notEqual(repetitionFingerprint("read", { path: "/x" }), repetitionFingerprint("bash", { path: "/x" })); +}); + +test("RepetitionTracker escalates none -> warn -> abort by consecutive count", () => { + const t = new RepetitionTracker({ warn: 3, abort: 5 }); + assert.equal(t.note("x", {}).action, "none"); + assert.equal(t.note("x", {}).action, "none"); + assert.equal(t.note("x", {}).action, "warn"); + assert.equal(t.note("x", {}).action, "warn"); + assert.equal(t.note("x", {}).action, "abort"); + assert.equal(t.note("x", {}).action, "abort"); +}); + +test("RepetitionTracker resets on a different fingerprint (args or tool name)", () => { + const t = new RepetitionTracker({ warn: 2, abort: 99 }); + t.note("x", { a: 1 }); + t.note("x", { a: 1 }); + assert.equal(t.note("x", { a: 2 }).action, "none"); + assert.equal(t.note("y", { a: 1 }).action, "none"); +}); + +test("RepetitionTracker.reset() clears the run", () => { + const t = new RepetitionTracker({ warn: 2, abort: 3 }); + t.note("x", {}); + t.note("x", {}); + t.reset(); + assert.equal(t.note("x", {}).action, "none"); +}); + +function makeRepCtx(sid = "sess-rep") { + const ctx = { + hasUI: false, + ui: { notify: () => {} }, + abortCalls: 0, + sessionManager: { getSessionId: () => sid }, + abort: () => { + ctx.abortCalls += 1; + }, + }; + return ctx; +} + +function wireRepetition(adapter: Record) { + const handlers: Record unknown> = {}; + const pi = { + on: (name: string, fn: (...a: never[]) => unknown) => { + handlers[name] = fn as (...a: unknown[]) => unknown; + }, + } as unknown as ExtensionAPI; + const runtime = { adapter } as AcpRuntime; + const ctx = makeRepCtx(); + wireToolGuardrails(pi, runtime); + return { + call: (event: ToolCallEvent) => handlers["tool_call"]!(event, ctx), + result: (event: ToolResultEvent) => handlers["tool_result"]!(event), + input: (source: string) => handlers["input"]!({ source }, ctx), + shutdown: () => handlers["session_shutdown"]!({}, ctx), + ctx, + }; +} + +const statusCall = (id: string): ToolCallEvent => + ({ type: "tool_call", toolName: "acp_status", toolCallId: id, input: { scope: "uncompressed", view: "ranges" } }) as unknown as ToolCallEvent; + +const statusResult = (id: string): ToolResultEvent => + ({ toolName: "acp_status", toolCallId: id, content: text("status ok"), isError: false }) as unknown as ToolResultEvent; + +const hasContent = (ret: unknown): ret is { content: Content } => + !!ret && typeof ret === "object" && "content" in ret; + +test("repetition guard queues a warning at warn=3 (default) without blocking", () => { + const w = wireRepetition({}); + assert.equal(w.call(statusCall("c1")), undefined); + assert.equal(w.call(statusCall("c2")), undefined); + assert.equal(w.call(statusCall("c3")), undefined, "warn threshold must not block"); + const res = w.result(statusResult("c3")); + assert.ok(hasContent(res), "warned result must be modified"); + const t = textOf(res.content[0]); + assert.match(t, /CONSECUTIVE identical/); + assert.match(t, /acp_status/); +}); + +test("repetition guard blocks and aborts at abort=5 (default)", () => { + const w = wireRepetition({}); + for (const id of ["k1", "k2", "k3", "k4"]) { + assert.equal(w.call(statusCall(id)), undefined, "below abort must not block"); + } + const fifth = w.call(statusCall("k5")) as { block?: boolean; reason?: string }; + assert.equal(fifth?.block, true, "5th identical call must be blocked"); + assert.match(fifth?.reason ?? "", /BLOCKED/); + assert.equal(w.ctx.abortCalls, 1, "turn must be aborted exactly once"); + const sixth = w.call(statusCall("k6")) as { block?: boolean }; + assert.equal(sixth?.block, true, "still blocked after abort"); +}); + +test("repetition guard resets the run when arguments change", () => { + const w = wireRepetition({}); + const other: ToolCallEvent = { + type: "tool_call", + toolName: "acp_status", + toolCallId: "x", + input: { scope: "compressed" }, + } as unknown as ToolCallEvent; + w.call(statusCall("r1")); + w.call(statusCall("r2")); + w.call(other); + w.call(statusCall("r3")); + w.call(statusCall("r4")); + assert.equal(w.result(statusResult("r4")), undefined, "only 2 consecutive identical -> no warning yet"); + w.call(statusCall("r5")); + const res5 = w.result(statusResult("r5")); + assert.ok(hasContent(res5), "3rd consecutive identical -> warning"); +}); + +test("repetition guard run resets on real user input but not extension-sent input", () => { + const w = wireRepetition({}); + w.call(statusCall("u1")); + w.call(statusCall("u2")); + w.input("interactive"); + w.call(statusCall("u3")); + w.call(statusCall("u4")); + assert.equal(w.result(statusResult("u4")), undefined, "after user-input reset, only 2 consecutive -> no warning"); + w.input("extension"); + w.call(statusCall("u5")); + const res5 = w.result(statusResult("u5")); + assert.ok(hasContent(res5), "extension-sent input must not reset the run"); +}); + +test("repetition guard is fully inert when repetitionGuard: false", () => { + const w = wireRepetition({ repetitionGuard: false }); + for (let i = 1; i <= 6; i++) { + assert.equal(w.call(statusCall(`d${i}`)), undefined, `identical call ${i} must not be blocked when disabled`); + } + assert.equal(w.ctx.abortCalls, 0, "never aborts when disabled"); +}); + +test("repetition guard honors custom warn/abort thresholds", () => { + const w = wireRepetition({ repetitionGuard: { warn: 2, abort: 3 } }); + assert.equal(w.call(statusCall("t1")), undefined); + assert.equal(w.call(statusCall("t2")), undefined); + const res2 = w.result(statusResult("t2")); + assert.ok(hasContent(res2), "warn fires at custom warn=2"); + const third = w.call(statusCall("t3")) as { block?: boolean }; + assert.equal(third?.block, true, "abort fires at custom abort=3"); +});