Jelajahi Sumber

fix(hooks): narrow loop-guard block scope, exempt polling tools, bound session map

Review fixes:
- Reorder callKeys set after the block-throw so refused calls don't leak
  fingerprint entries.
- Warn on all tools, but hard-block only read-only file-analysis tools
  (read/grep/glob). Polling tools (task_*, wait_for_*) legitimately
  re-issue identical calls and must never be refused.
- Exempt the whole task-control/wait family from both axes.
- Bound the per-session state map to 512 entries (FIFO eviction) instead
  of growing unbounded since resetSession isn't wired to lifecycle yet.
- Document the guard in docs/tools.md.
Michael Henke 3 minggu lalu
induk
melakukan
dd64023981
3 mengubah file dengan 116 tambahan dan 14 penghapusan
  1. 22 0
      docs/tools.md
  2. 63 14
      src/hooks/tool-loop-guard/hook.ts
  3. 31 0
      src/hooks/tool-loop-guard/index.test.ts

+ 22 - 0
docs/tools.md

@@ -76,6 +76,28 @@ lifecycle, cancellation, and explicit-wait edge cases behind these tools.
 
 ---
 
+## Repeated Tool-Call Loop Guard
+
+A safety net for model-side infinite loops where a sub-agent (e.g. a model
+that can degenerate, such as DeepSeek V4 Flash in Explorer) re-issues the
+exact same tool call with identical arguments and gets identical results,
+making no progress. The plugin watches each session's consecutive identical
+tool calls and responds:
+
+- On the 3rd identical call: appends a corrective notice to the tool output
+  telling the model to stop repeating and change approach. Applies to all
+  tools.
+- On the 5th identical call: refuses execution for the read-only file tools
+  `read`, `grep`, and `glob`, terminating the loop. Other tools stay
+  warn-only.
+
+Exempt from the entire guard: the task-control and wait tools (`task`,
+`task_status`, `task_result`, `task_cancel`, `task_message`, `task_revive`,
+`wait_for_user`, `wait_for_background_tasks`) — those legitimately re-issue
+identical calls while polling a long-running background task.
+
+---
+
 ## Formatters
 
 OpenCode automatically formats files after they are written or edited, using language-specific formatters. No manual step needed.

+ 63 - 14
src/hooks/tool-loop-guard/hook.ts

@@ -9,20 +9,50 @@
  * Behavior:
  * - N identical consecutive calls (LOOP_GUARD_WARN_AT): append corrective
  *   text to the tool output telling the model to stop and change approach.
- * - M identical consecutive calls (LOOP_GUARD_BLOCK_AT): refuse the call in
- *   tool.execute.before by throwing, so the loop terminates instead of
- *   running forever.
+ * - For read-only file tools (READONLY_BLOCK_TOOLS), M identical consecutive
+ *   calls (LOOP_GUARD_BLOCK_AT): refuse the call in tool.execute.before by
+ *   throwing, so the loop terminates instead of running forever.
+ *
+ * Scope is deliberately narrow to avoid breaking legitimate repeated calls:
+ * - All tools warn at N identical consecutive calls.
+ * - Only the read-only file-analysis tools hard-block: polling tools
+ *   (task_*, wait_for_*) legitimately re-issue identical calls waiting on a
+ *   long-running background task and must never be refused.
+ * - The task tool is exempt entirely for both axes; task-session-manager
+ *   owns its own duplicate-spawn guards (#1056/#1070).
  *
  * Precedent: json-error-recovery (output warning) and task-session-manager
- * (before-hook refusal). The `task` tool is exempt: task-session-manager
- * already owns its duplicate-spawn guards (#1056/#1070).
+ * (before-hook refusal).
  */
 
 const LOOP_GUARD_WARN_AT = 3;
 const LOOP_GUARD_BLOCK_AT = 5;
 
-/** Exempt: task-session-manager already owns duplicate-spawn guards. */
-const EXEMPT_TOOL = 'task';
+/**
+ * Tools exempt from the entire guard: long-lived task supervision/polling
+ * tools whose identical repeated invocation is legitimate.
+ */
+const LOOP_GUARD_EXEMPT: Record<string, true> = {
+  task: true,
+  task_status: true,
+  task_result: true,
+  task_cancel: true,
+  task_message: true,
+  task_revive: true,
+  wait_for_user: true,
+  wait_for_background_tasks: true,
+};
+
+/**
+ * Tools that may be hard-blocked when repeated. Read-only file analysis is
+ * the reported loop surface (#1071); anything with side effects or that
+ * polls external state stays warn-only.
+ */
+const LOOP_GUARD_BLOCK_TOOLS: Record<string, true> = {
+  read: true,
+  grep: true,
+  glob: true,
+};
 
 const LOOP_GUARD_MARKER = '[REPEATED TOOL CALLS - STOP]';
 
@@ -37,6 +67,9 @@ STOP repeating this call. Instead:
 3. If the task is actually done, produce your final answer now instead of calling more tools.
 `;
 
+/** Max sessions tracked before evicting the least-recently-observed session. */
+const MAX_TRACKED_SESSIONS = 512;
+
 /** Deterministic fingerprint of tool + args, insensitive to key order. */
 function fingerprint(tool: string, args: unknown): string {
   return `${tool.toLowerCase()}:${stableStringify(args)}`;
@@ -81,34 +114,50 @@ export function createToolLoopGuardHook(): ToolLoopGuardHook {
   /** Fingerprint per callID so `after` can re-check without re-deriving args. */
   const callKeys = new Map<string, string>();
 
+  /** Prune the session map to MAX_TRACKED_SESSIONS (FIFO by insertion). */
+  function keepSessionsBounded(): void {
+    while (sessions.size > MAX_TRACKED_SESSIONS) {
+      const oldest = sessions.keys().next().value as string | undefined;
+      if (oldest === undefined) break;
+      sessions.delete(oldest);
+    }
+  }
+
   return {
     'tool.execute.before': async (
       input: { tool: string; sessionID?: string; callID?: string },
       output: { args?: unknown },
     ): Promise<void> => {
       const sessionID = input.sessionID;
-      if (!sessionID || input.tool.toLowerCase() === EXEMPT_TOOL) return;
+      if (!sessionID) return;
+      const tool = input.tool.toLowerCase();
+      if (LOOP_GUARD_EXEMPT[tool]) return;
 
-      const key = fingerprint(input.tool, output.args);
-      if (input.callID) callKeys.set(input.callID, key);
+      const key = fingerprint(tool, output.args);
 
       const existing = sessions.get(sessionID);
       const count = existing && existing.last === key ? existing.count + 1 : 1;
       sessions.set(sessionID, { last: key, count });
+      keepSessionsBounded();
 
-      if (count >= LOOP_GUARD_BLOCK_AT) {
+      if (count >= LOOP_GUARD_BLOCK_AT && LOOP_GUARD_BLOCK_TOOLS[tool]) {
         throw new Error(
-          `Refusing to execute "${input.tool}": this exact call (same tool, same arguments) has been issued ${count} times in a row with identical results and constitutes an infinite loop. Stop repeating it. Reassess your goal, make a different call, or produce your final answer.`,
+          `Refusing to execute "${tool}": this exact call (same tool, same arguments) has been issued ${count} times in a row with identical results and constitutes an infinite loop. Stop repeating it. Reassess your goal, make a different call, or produce your final answer.`,
         );
       }
+
+      if (input.callID) callKeys.set(input.callID, key);
     },
 
     'tool.execute.after': async (
       input: { tool: string; sessionID?: string; callID?: string },
       output: { output: unknown; metadata?: unknown },
     ): Promise<void> => {
-      if (!input.sessionID || input.tool.toLowerCase() === EXEMPT_TOOL) return;
-      const state = sessions.get(input.sessionID);
+      const sessionID = input.sessionID;
+      if (!sessionID) return;
+      const tool = input.tool.toLowerCase();
+      if (LOOP_GUARD_EXEMPT[tool]) return;
+      const state = sessions.get(sessionID);
       if (!state) return;
 
       const key = input.callID ? callKeys.get(input.callID) : undefined;

+ 31 - 0
src/hooks/tool-loop-guard/index.test.ts

@@ -99,6 +99,37 @@ describe('tool-loop-guard', () => {
     // no throw
   });
 
+  test('polling tools are exempt from warn and block', async () => {
+    for (let i = 1; i <= 8; i++) {
+      await hook['tool.execute.before'](
+        beforeInput({ tool: 'task_status', callID: `s${i}` }),
+        { args: { task_id: 'child-1' } },
+      );
+      const output = { output: 'state: running', metadata: {} };
+      await hook['tool.execute.after'](
+        afterInput({ tool: 'task_status', callID: `s${i}` }),
+        output,
+      );
+      expect(output.output).toBe('state: running'); // never warned
+    }
+  });
+
+  test('non-readonly tools warn but are never hard-blocked', async () => {
+    // bash repeats warn at 3 but must not throw at 5+
+    for (let i = 1; i <= 6; i++) {
+      await hook['tool.execute.before'](
+        beforeInput({ tool: 'bash', callID: `b${i}` }),
+        { args: { command: 'uname' } },
+      );
+    }
+    const output = { output: 'Linux', metadata: {} };
+    await hook['tool.execute.after'](
+      afterInput({ tool: 'bash', callID: 'b6' }),
+      output,
+    );
+    expect(output.output).toContain(LOOP_GUARD_WARNING);
+  });
+
   test('sessions are isolated', async () => {
     for (let i = 1; i <= 2; i++) {
       await runIdenticalCall(`a${i}`, { filePath: 'a.ts' });