fix(core): treat a target's direct and bridge retry channels as one f…

…amily The channel marker keeps a bridged pre-check refusal from prefix-pruning the same target's direct validation failure WITHIN a batch, but the batch-start presence prune widened the presence set only for requests carrying an INVALID_TOOL_PARAMS bridge refusal. Each batch therefore preserved just the channel it contained and deleted the other, so a model alternating between bridging a deferred target and calling it directly reset both counters every turn: neither ever reached VALIDATION_RETRY_LOOP_THRESHOLD and RETRY LOOP DETECTED was never injected. That is a regression against the merge base, where both channels produced the same message under the same key and accrued together. Fix both halves, since widening presence alone is not safe: - presence: a request naming X now preserves both `X` and `bridgeRetryToolName(X)`, so alternating channels across batches accrue toward one threshold. - clearing: `clearRetryCountsForTool` now also deletes the bridge-marked keys. A resolved bridge renames the request to the target, so the widened presence set keeps its stale bridge count alive across the successful execution; without the clearing half that resurrects the premature RETRY LOOP DETECTED the marker's prune was relied on to drop. Unrelated tools are unaffected: the widening is keyed on names present in the current batch, and an EXECUTION_DENIED refusal still keeps the wrapper name, so it cannot retain the denied target's counters. Tests: add 'accrues alternating bridged and direct failures of one target across separate batches' (six single-request batches; the directive fires once, on the fifth) and 'clears a target's bridge-marked counter when a bridged call to it succeeds'; extend the existing mixed-channel test to also assert the directive on the bridged half, which its name already claimed. Dropping the presence widening turns the first red; reverting clearRetryCountsForTool to the bare prefix turns the second red. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmumpkv7rf3
yiliang114 · Oct 6, 2026 · Sep 28, 2026 · Sep 28, 2026 · Sep 28, 2026 · Sep 28, 2026
commit 4c101ec361dbb2174776f3ca1c9a67ed681b0eb6
@@ -2064,13 +2064,192 @@ describe('CoreToolScheduler', () => {
     const [bridgeRefusal, directFailure] = third;
     expect(bridgeRefusal.status).toBe('error');
     expect(directFailure.status).toBe('error');
+    // Both halves of the claim this test's name makes: each channel reaches
+    // the threshold on its own key by the third batch.
+    if (bridgeRefusal.status === 'error') {
+      expect(bridgeRefusal.response.error?.message).toContain(
+        'RETRY LOOP DETECTED',
+      );
+    }
     if (directFailure.status === 'error') {
       expect(directFailure.response.error?.message).toContain(
         'RETRY LOOP DETECTED',
       );
     }
   });
 
+  it('accrues alternating bridged and direct failures of one target across separate batches', async () => {
+    // The channel marker alone is not enough: the batch-start presence prune
+    // runs per batch, so if each batch preserves only the channel it contains,
+    // a model that alternates between bridging a deferred target and calling
+    // it directly deletes the OTHER channel's counter every turn. Neither
+    // counter then exceeds 1, RETRY LOOP DETECTED is never injected, and the
+    // mixed-channel loop runs indefinitely — the stagnation this PR exists to
+    // stop. Mutation check: dropping `bridgeRetryToolName(r.name)` from the
+    // presence set in _schedule turns this red (no batch ever fires).
+    const bridge = new MockTool({ name: ToolNames.TOOL_CALL });
+    const writeFile = new MockTool({
+      name: 'write_file',
+      shouldDefer: true,
+      params: {
+        type: 'object',
+        properties: {
+          file_path: { type: 'string' },
+          content: { type: 'string' },
+        },
+        required: ['file_path', 'content'],
+        additionalProperties: false,
+      },
+    });
+    const { scheduler, onAllToolCallsComplete } =
+      createSchedulerForLegacyToolTests({
+        toolsByName: new Map([
+          [bridge.name, bridge],
+          [writeFile.name, writeFile],
+        ]),
+        deferredHiddenNames: new Set([writeFile.name]),
+      });
+
+    // One request per batch, so each batch's presence set holds exactly one
+    // channel plus whatever the widening adds for it.
+    const runSingleBatch = async (
+      callId: string,
+      request: { name: string; args: Record<string, unknown> },
+    ) => {
+      onAllToolCallsComplete.mockClear();
+      await scheduler.schedule(
+        {
+          callId,
+          name: request.name,
+          args: request.args,
+          isClientInitiated: false,
+          prompt_id: 'prompt-bridge-alternating',
+        },
+        new AbortController().signal,
+      );
+      await vi.waitFor(() => expect(onAllToolCallsComplete).toHaveBeenCalled());
+      return onAllToolCallsComplete.mock.calls[0][0][0] as ToolCall;
+    };
+    const bridged = {
+      name: ToolNames.TOOL_CALL,
+      args: { name: 'write_file', arguments: {} },
+    };
+    const direct = { name: 'write_file', args: {} };
+
+    const messages: string[] = [];
+    for (const batchId of [1, 2, 3, 4, 5, 6]) {
+      const completed = await runSingleBatch(
+        `alternating-${batchId}`,
+        batchId % 2 === 1 ? bridged : direct,
+      );
+      expect(completed.status).toBe('error');
+      if (completed.status === 'error') {
+        expect(completed.response.errorType).toBe(
+          ToolErrorType.INVALID_TOOL_PARAMS,
+        );
+        messages.push(completed.response.error?.message ?? '');
+      }
+    }
+
+    // The bridged channel climbs 1, 2, 3 across the odd batches while the
+    // even batches restart the direct channel, so the directive fires exactly
+    // once — on the fifth batch — and never prematurely.
+    expect(messages).toHaveLength(6);
+    for (const early of messages.slice(0, 4)) {
+      expect(early).not.toContain('RETRY LOOP DETECTED');
+    }
+    expect(messages[4]).toContain('RETRY LOOP DETECTED');
+    expect(messages[5]).not.toContain('RETRY LOOP DETECTED');
+  });
+
+  it('clears a target’s bridge-marked counter when a bridged call to it succeeds', async () => {
+    // The clearing half of the presence widening. A resolved bridge renames
+    // the request to the TARGET, so the batch-start presence set now keeps
+    // both of that target's channels and the prune no longer drops the stale
+    // bridge-marked count. clearRetryCountsForTool must cover the marked
+    // channel too, or the surviving count of 2 plus two later refusals would
+    // fire RETRY LOOP DETECTED prematurely. Mutation check: reverting
+    // clearRetryCountsForTool to the bare `${toolName}:` prefix turns this red.
+    const execute = vi.fn().mockResolvedValue({
+      llmContent: [{ text: 'published' }],
+      returnDisplay: 'published',
+    });
+    const bridge = new MockTool({ name: ToolNames.TOOL_CALL });
+    // A neutral target: PATH_ARG_KEYS (file_path/path/...) are rewritten on
+    // request.args during execution, which is not what this case measures.
+    const publishNote = new MockTool({
+      name: 'publish_note',
+      shouldDefer: true,
+      execute,
+      params: {
+        type: 'object',
+        properties: {
+          note_id: { type: 'string' },
+          body: { type: 'string' },
+        },
+        required: ['note_id', 'body'],
+        additionalProperties: false,
+      },
+    });
+    const { scheduler, onAllToolCallsComplete } =
+      createSchedulerForLegacyToolTests({
+        toolsByName: new Map([
+          [bridge.name, bridge],
+          [publishNote.name, publishNote],
+        ]),
+        deferredHiddenNames: new Set([publishNote.name]),
+      });
+
+    const runBridged = async (
+      callId: string,
+      args: Record<string, unknown>,
+    ) => {
+      onAllToolCallsComplete.mockClear();
+      await scheduler.schedule(
+        {
+          callId,
+          name: ToolNames.TOOL_CALL,
+          args: { name: 'publish_note', arguments: args },
+          isClientInitiated: false,
+          prompt_id: 'prompt-bridge-clear',
+        },
+        new AbortController().signal,
+      );
+      await vi.waitFor(() => expect(onAllToolCallsComplete).toHaveBeenCalled());
+      return onAllToolCallsComplete.mock.calls[0][0][0] as ToolCall;
+    };
+
+    // Two bridged refusals take the marked channel to 2.
+    for (const batchId of [1, 2]) {
+      const completed = await runBridged(`clear-${batchId}`, {});
+      expect(completed.status).toBe('error');
+      if (completed.status === 'error') {
+        expect(completed.response.error?.message).not.toContain(
+          'RETRY LOOP DETECTED',
+        );
+      }
+    }
+
+    // A successful bridged execution of the SAME target clears both channels.
+    const succeeded = await runBridged('clear-success', {
+      note_id: 'n-1',
+      body: 'ok',
+    });
+    expect(succeeded.status).toBe('success');
+    expect(execute).toHaveBeenCalledTimes(1);
+
+    // Two more refusals restart at 1 instead of inheriting the stale count.
+    for (const batchId of [3, 4]) {
+      const completed = await runBridged(`clear-after-${batchId}`, {});
+      expect(completed.status).toBe('error');
+      if (completed.status === 'error') {
+        expect(completed.response.error?.message).not.toContain(
+          'RETRY LOOP DETECTED',
+        );
+      }
+    }
+  });
+
   it('applies the retry-loop directive to repeated invalid tool_call envelopes', async () => {
     const bridge = new MockTool({ name: ToolNames.TOOL_CALL });
     const { scheduler, onAllToolCallsComplete } =

@@ -2882,12 +2882,15 @@ export class CoreToolScheduler {
   /**
    * Removes all validation retry counters for the given tool. Keys are
    * "<toolName>:<errorMessage>", so a plain `Map.delete(toolName)` would not
-   * match anything.
+   * match anything. The bridge-marked channel is cleared too: the two channels
+   * are one family for presence (see the prune in _schedule), so clearing must
+   * cover both or a successful execution of the target would leave its stale
+   * bridge-channel count behind to fire RETRY LOOP DETECTED prematurely.
    */
   private clearRetryCountsForTool(toolName: string): void {
-    const prefix = `${toolName}:`;
+    const prefixes = [`${toolName}:`, `${bridgeRetryToolName(toolName)}:`];
     for (const key of this.validationRetryCounts.keys()) {
-      if (key.startsWith(prefix)) {
+      if (prefixes.some((prefix) => key.startsWith(prefix))) {
         this.validationRetryCounts.delete(key);
       }
     }
@@ -2972,24 +2975,40 @@ export class CoreToolScheduler {
       // present in the current batch. Keeping every tracked tool's counters
       // whenever any current request matched caused stale counts for
       // unrelated tools to survive and fire RETRY LOOP DETECTED prematurely
-      // the next time those tools were used. A refused bridge request keeps
-      // the wrapper name (`tool_call`), so the channel-marked name of the
-      // validated target must join the presence set alongside it — but only
-      // for INVALID_TOOL_PARAMS refusals, the one error type that accrues
-      // below: an EXECUTION_DENIED (policy) refusal records nothing, so it
-      // must not keep the denied target's stale counters alive either.
+      // the next time those tools were used.
+      //
+      // A target's direct and bridge-marked channels are ONE family for
+      // presence: a request naming X preserves both `X` and
+      // `bridgeRetryToolName(X)`. Widening presence only for the bridged
+      // channel let alternating channels ACROSS batches prune each other — a
+      // bridged batch kept just the marked key and a direct batch just the
+      // bare one, so neither counter ever reached
+      // VALIDATION_RETRY_LOOP_THRESHOLD and a mixed-channel loop rode on
+      // without the stop directive. clearRetryCountsForTool clears both
+      // channels, so this widening cannot resurrect the stale bridge count
+      // that a resolved-and-executed target leaves behind.
+      //
+      // A refused bridge request keeps the wrapper name (`tool_call`), so the
+      // channel-marked name of the validated target must join the presence set
+      // alongside it — but only for INVALID_TOOL_PARAMS refusals, the one error
+      // type that accrues below: an EXECUTION_DENIED (policy) refusal records
+      // nothing, so it must not keep the denied target's stale counters alive
+      // either.
       if (this.validationRetryCounts.size > 0) {
         const currentToolNames = new Set(
-          requestsToProcess.flatMap((r) =>
-            r.bridgeResolutionError?.type ===
-              ToolErrorType.INVALID_TOOL_PARAMS &&
-            r.bridgeResolutionError.targetName !== undefined
-              ? [
-                  r.name,
-                  bridgeRetryToolName(r.bridgeResolutionError.targetName),
-                ]
-              : [r.name],
-          ),
+          requestsToProcess.flatMap((r) => {
+            const names = [r.name, bridgeRetryToolName(r.name)];
+            if (
+              r.bridgeResolutionError?.type ===
+                ToolErrorType.INVALID_TOOL_PARAMS &&
+              r.bridgeResolutionError.targetName !== undefined
+            ) {
+              names.push(
+                bridgeRetryToolName(r.bridgeResolutionError.targetName),
+              );
+            }
+            return names;
+          }),
         );
         for (const key of [...this.validationRetryCounts.keys()]) {
           const sep = key.indexOf(':');