feat(core): foreground → background promote integration (#3831 PR-2 of 3)

packages/core/src/core/coreToolScheduler.test.ts

@@ -51,6 +51,9 @@ import { type NotificationType } from '../hooks/types.js';
 import type { MessageBus } from '../confirmation-bus/message-bus.js';
 import { IdeClient } from '../ide/ide-client.js';
 import { WriteFileTool } from '../tools/write-file.js';
+import { ShellTool, ShellToolInvocation } from '../tools/shell.js';
+import type { ShellToolParams } from '../tools/shell.js';
+import type { ShellExecutionConfig } from '../services/shellExecutionService.js';
 
 vi.mock('fs/promises', () => ({
   writeFile: vi.fn(),
@@ -5379,3 +5382,129 @@ describe('CoreToolScheduler activation wiring', () => {
     expect(matchAndActivateByPaths).not.toHaveBeenCalled();
   });
 });
+
+describe('CoreToolScheduler shell-tool promote integration (#3831 PR-2)', () => {
+  it('stashes promoteAbortController on the executing tool call when shell.ts fires the callback', async () => {
+    // Pin the scheduler-side wiring for the promote-AbortController
+    // callback. PR-3's Ctrl+B keybind will look up the
+    // currently-executing shell tool call by callId and abort
+    // `tc.promoteAbortController`; if the scheduler stops populating
+    // that field, the keybind silently breaks. Direct
+    // ShellToolInvocation tests can't see this — they don't go
+    // through the scheduler.
+    let exposedAc: AbortController | undefined;
+    class TestShellInvocation extends ShellToolInvocation {
+      override async execute(
+        _signal: AbortSignal,
+        _updateOutput?: (output: ToolResultDisplay) => void,
+        _shellExecutionConfig?: ShellExecutionConfig,
+        _setPidCallback?: (pid: number) => void,
+        setPromoteAbortControllerCallback?: (ac: AbortController) => void,
+      ): Promise<ToolResult> {
+        // Mirror the production flow: foreground shell.ts spawns,
+        // calls setPromoteAbortControllerCallback right after spawn,
+        // then waits for the result. We synthesize the callback fire
+        // and immediately complete with a benign success result.
+        const ac = new AbortController();
+        exposedAc = ac;
+        setPromoteAbortControllerCallback?.(ac);
+        return { llmContent: 'ok', returnDisplay: 'ok' };
+      }
+    }
+
+    class TestShellTool extends ShellTool {
+      protected override createInvocation(params: ShellToolParams) {
+        // Cast through unknown — the test invocation extends the real
+        // ShellToolInvocation prototype so the scheduler's `instanceof
+        // ShellToolInvocation` check still routes the call through
+        // the shell-tool-specific branch (which is the branch that
+        // wires setPromoteAbortControllerCallback).
+        return new TestShellInvocation(
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          (this as any).config,
+          params,
+        ) as unknown as ToolInvocation<ShellToolParams, ToolResult>;
+      }
+    }
+
+    const tool = new TestShellTool({} as Config);
+    const mockToolRegistry = {
+      getTool: () => tool,
+      ensureTool: async () => tool,
+      getFunctionDeclarations: () => [],
+      tools: new Map(),
+      discovery: {},
+      registerTool: () => {},
+      getToolByName: () => tool,
+      getToolByDisplayName: () => tool,
+      getTools: () => [],
+      discoverTools: async () => {},
+      getAllTools: () => [],
+      getToolsByServer: () => [],
+    } as unknown as ToolRegistry;
+
+    const onAllToolCallsComplete = vi.fn();
+    const onToolCallsUpdate = vi.fn();
+    const mockConfig = {
+      getSessionId: () => 'test-session-id',
+      getUsageStatisticsEnabled: () => true,
+      getDebugMode: () => false,
+      getApprovalMode: () => ApprovalMode.YOLO,
+      getContentGeneratorConfig: () => ({
+        model: 'test-model',
+        authType: 'gemini',
+      }),
+      getToolRegistry: () => mockToolRegistry,
+      getShellExecutionConfig: () => ({
+        terminalWidth: 80,
+        terminalHeight: 24,
+      }),
+      getChatRecordingService: () => undefined,
+      getMessageBus: vi.fn().mockReturnValue(undefined),
+      getDisableAllHooks: vi.fn().mockReturnValue(true),
+    } as unknown as Config;
+
+    const scheduler = new CoreToolScheduler({
+      config: mockConfig,
+      onAllToolCallsComplete,
+      onToolCallsUpdate,
+      getPreferredEditor: () => 'vscode',
+      onEditorClose: vi.fn(),
+    });
+
+    await scheduler.schedule(
+      [
+        {
+          callId: 'shell-1',
+          name: 'run_shell_command',
+          args: { command: 'echo hi' },
+          isClientInitiated: true,
+          prompt_id: 'p-shell',
+        },
+      ],
+      new AbortController().signal,
+    );
+
+    await vi.waitFor(() => {
+      expect(onAllToolCallsComplete).toHaveBeenCalled();
+    });
+
+    // Find a tool-calls-update emitted while the call was 'executing'
+    // that carries the promoteAbortController. The exact ordering of
+    // updates depends on the scheduler's internal flow, but at SOME
+    // point during the executing window the field must be populated —
+    // otherwise PR-3's Ctrl+B keybind has nothing to abort.
+    const updateBatches = onToolCallsUpdate.mock.calls;
+    const sawPromoteAcWhileExecuting = updateBatches.some((batch) => {
+      const tcs = batch[0] as ToolCall[];
+      return tcs.some(
+        (tc) =>
+          tc.request.callId === 'shell-1' &&
+          tc.status === 'executing' &&
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          (tc as any).promoteAbortController === exposedAc,
+      );
+    });
+    expect(sawPromoteAcWhileExecuting).toBe(true);
+  });
+});

packages/core/src/core/coreToolScheduler.ts

@@ -148,6 +148,16 @@ export type ExecutingToolCall = {
   executionStartTime?: number;
   outcome?: ToolConfirmationOutcome;
   pid?: number;
+  /**
+   * Set during a foreground shell-tool invocation: the AbortController
+   * the user/UI can fire (with `signal.reason = { kind: 'background' }`)
+   * to promote the running command to a background entry. Set right
+   * after `setPidCallback` fires (see ShellTool.execute), cleared
+   * implicitly when the tool transitions to a terminal status. Only
+   * meaningful for the shell tool's foreground path; absent on every
+   * other tool kind.
+   */
+  promoteAbortController?: AbortController;
 };
 
 export type CancelledToolCall = {
@@ -1849,11 +1859,27 @@ export class CoreToolScheduler {
         );
         this.notifyToolCallsUpdate();
       };
+      // Stash the promote AbortController on the executing tool call so
+      // a UI surface (PR-3 Ctrl+B keybind) can find the foreground
+      // shell's promote trigger by callId. Calling `.abort({ kind:
+      // 'background', shellId })` on it tells `ShellExecutionService`
+      // to skip the kill, snapshot output, and return
+      // `result.promoted: true` — `shell.ts` then registers the
+      // `BackgroundShellEntry`.
+      const setPromoteAbortControllerCallback = (ac: AbortController) => {
+        this.toolCalls = this.toolCalls.map((tc) =>
+          tc.request.callId === callId && tc.status === 'executing'
+            ? { ...tc, promoteAbortController: ac }
+            : tc,
+        );
+        this.notifyToolCallsUpdate();
+      };
       promise = invocation.execute(
         signal,
         liveOutputCallback,
         shellExecutionConfig,
         setPidCallback,
+        setPromoteAbortControllerCallback,
       );
     } else {
       promise = invocation.execute(

packages/core/src/services/shellExecutionService.test.ts

@@ -639,7 +639,7 @@ describe('ShellExecutionService', () => {
       );
     });
 
-    it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true', async () => {
+    it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true (and aborted: false per design question 7)', async () => {
       // Critical: do NOT fire onExit — the child is still alive after the
       // background-promote abort. The result Promise must resolve via the
       // abort handler's own immediate resolve, not via the exit handler.
@@ -653,7 +653,11 @@ describe('ShellExecutionService', () => {
         },
       );
 
-      expect(result.aborted).toBe(true);
+      // `aborted: false` (despite signal.aborted = true) is intentional —
+      // see #3831 design question 7. The flag answers "emit cancel/timeout
+      // copy?" not "did the signal fire?", and a promoted shell is
+      // neither cancelled nor timed out.
+      expect(result.aborted).toBe(false);
       expect(result.promoted).toBe(true);
       expect(result.exitCode).toBeNull();
       expect(result.signal).toBeNull();
@@ -1410,7 +1414,7 @@ describe('ShellExecutionService child_process fallback', () => {
       );
     });
 
-    it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true', async () => {
+    it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true (and aborted: false per design question 7)', async () => {
       mockPlatform.mockReturnValue('linux');
       // Critical: do NOT fire 'exit' — the child is still alive after the
       // background-promote abort. The result Promise must resolve via the
@@ -1427,7 +1431,8 @@ describe('ShellExecutionService child_process fallback', () => {
         },
       );
 
-      expect(result.aborted).toBe(true);
+      // See PTY equivalent test for the rationale on `aborted: false`.
+      expect(result.aborted).toBe(false);
       expect(result.promoted).toBe(true);
       expect(result.exitCode).toBeNull();
       expect(result.signal).toBeNull();

packages/core/src/services/shellExecutionService.ts

@@ -141,6 +141,15 @@ export interface ShellExecutionResult {
    * alive and the caller has taken over its lifecycle. Callers receiving
    * `promoted: true` must NOT treat exitCode/signal as terminal — the
    * underlying process has not exited.
+   *
+   * Note on the result shape: when `promoted: true`, `aborted` is set to
+   * `false` even though the AbortSignal fired. The contract is that
+   * `aborted` answers "should the caller emit a cancel/timeout
+   * message?" — and a promoted shell is neither cancelled nor timed
+   * out (the child kept running, ownership simply transferred). This
+   * lets existing `if (result.aborted)` branches stay unchanged; new
+   * promote handling lives in a separate `if (result.promoted)` arm.
+   * Settled in #3831 design question 7 / @tanzhenxin's PR-1 review note.
    */
   promoted?: boolean;
   /** The process ID of the spawned shell. */
@@ -699,7 +708,19 @@ export class ShellExecutionService {
             exitCode: null,
             signal: null,
             error: null,
-            aborted: true,
+            // `aborted: false` (despite the abort signal having fired) is
+            // intentional — this is the result-shape decision settled in
+            // #3831 design question 7 (raised by @tanzhenxin in the PR-1
+            // review). The flag answers "should the caller emit cancel /
+            // timeout copy?" not "did the abort signal fire?" — and a
+            // promoted shell did NOT cancel (the child kept running), so
+            // existing `if (result.aborted)` branches in callers (e.g.
+            // `tools/shell.ts`) fall through naturally to the success-shape
+            // arm where we then check `result.promoted`. Without this,
+            // every consumer would have to remember to check `promoted`
+            // before `aborted` to avoid emitting "cancelled" copy for a
+            // process that's still running.
+            aborted: false,
             promoted: true,
             pid: child.pid,
             executionMethod: 'child_process',
@@ -1289,7 +1310,10 @@ export class ShellExecutionService {
             exitCode: null,
             signal: null,
             error,
-            aborted: true,
+            // See childProcessFallback for the full rationale — promoted
+            // results are NOT user-cancellations, so callers' `if
+            // (result.aborted)` branches must NOT trigger.
+            aborted: false,
             promoted: true,
             pid: ptyProcess.pid,
             executionMethod: