Merge remote-tracking branch 'origin/main' into brennanb2025/fix-9168-codex-tui-settings-remirror

src/main/browser/grab-guest-script.test.ts

@@ -1,5 +1,8 @@
+import { types } from 'node:util'
+import { runInNewContext } from 'node:vm'
 import { describe, expect, it } from 'vitest'
 import { buildGuestOverlayScript } from './grab-guest-script'
+import { clampGrabPayload } from './browser-grab-payload'
 
 describe('buildGuestOverlayScript', () => {
   it('returns a non-empty string for arm action', () => {
@@ -158,3 +161,199 @@ describe('buildGuestOverlayScript', () => {
     expect(script).not.toContain('ariaLabelledBy.split(/\\s+/)')
   })
 })
+
+// Regression coverage for issue #9947: Zone.js swaps the global Promise for a
+// non-native thenable, which executeJavaScript won't unwrap — so a page-global
+// `new Promise(...)` crossed as its raw `__zone_symbol__*` wrapper, not { page, target }.
+describe('awaitClick under a Zone.js-patched global Promise', () => {
+  type Executor<T> = (resolve: (value: T) => void, reject: (reason: unknown) => void) => void
+
+  // Native promises are kept off the instance so its own enumerable keys match
+  // Zone.js exactly: ['__zone_symbol__state', '__zone_symbol__value'].
+  const nativeOf = new WeakMap<object, Promise<unknown>>()
+
+  /** A non-native thenable that mimics Zone.js's ZoneAwarePromise wrapper. */
+  class ZoneAwarePromiseLike<T = unknown> {
+    __zone_symbol__state: unknown = null
+    __zone_symbol__value: unknown = undefined
+
+    constructor(executor: Executor<T>) {
+      nativeOf.set(
+        this,
+        new Promise<T>((res, rej) => {
+          executor(
+            (value) => {
+              this.__zone_symbol__state = true
+              this.__zone_symbol__value = value
+              res(value)
+            },
+            (reason) => {
+              this.__zone_symbol__state = false
+              this.__zone_symbol__value = reason
+              rej(reason)
+            }
+          )
+        })
+      )
+    }
+
+    // Why: real Zone.js defines `get [Symbol.toStringTag]() { return 'Promise' }`,
+    // so an Object.prototype.toString check is fooled into seeing a promise. The
+    // boundary below deliberately uses the brand-based V8 check instead, and the
+    // control test asserts this tag does NOT let the wrapper masquerade as native.
+    get [Symbol.toStringTag](): string {
+      return 'Promise'
+    }
+
+    // oxlint-disable-next-line unicorn/no-thenable -- intentionally a non-native thenable modeling Zone.js's ZoneAwarePromise
+    then(
+      onFulfilled?: ((value: unknown) => unknown) | null,
+      onRejected?: ((reason: unknown) => unknown) | null
+    ): Promise<unknown> {
+      const native = nativeOf.get(this) as Promise<unknown>
+      return native.then(onFulfilled ?? undefined, onRejected ?? undefined)
+    }
+  }
+
+  /**
+   * Models Electron's boundary: it awaits only genuine V8 promises and
+   * serializes anything else — including non-native thenables — by value.
+   * `util.types.isPromise` is V8's brand-based IsPromise (what Electron uses):
+   * realm-independent and unforgeable, unlike an Object.prototype.toString /
+   * Symbol.toStringTag check that a ZoneAwarePromise would defeat.
+   */
+  async function crossExecuteJavaScriptBoundary(completionValue: unknown): Promise<unknown> {
+    if (types.isPromise(completionValue)) {
+      return await (completionValue as Promise<unknown>)
+    }
+    return { ...(completionValue as Record<string, unknown>) }
+  }
+
+  const validPayload = (): Record<string, unknown> => ({
+    // A minimal but structurally valid payload — page + target are what
+    // clampGrabPayload requires and what the bug stripped away.
+    page: { title: 'Angular App' },
+    target: { tagName: 'button' },
+    nearbyText: [],
+    ancestorPath: [],
+    screenshot: null
+  })
+
+  function armGrabHarness(options?: {
+    extractPayload?: () => unknown
+    getCurrentElement?: () => unknown
+  }): {
+    window: { __orcaGrab: Record<string, unknown> }
+    click: () => void
+    contextmenu: () => void
+    cancel: () => void
+  } {
+    const handlers: Record<string, (event: unknown) => void> = {}
+    const noopEvent = {
+      preventDefault(): void {},
+      stopPropagation(): void {},
+      stopImmediatePropagation(): void {}
+    }
+    const grab: Record<string, unknown> = {
+      host: {
+        addEventListener(type: string, fn: (event: unknown) => void): void {
+          handlers[type] = fn
+        },
+        removeEventListener(): void {}
+      },
+      extractPayload: options?.extractPayload ?? validPayload,
+      getCurrentElement: options?.getCurrentElement ?? ((): unknown => ({})),
+      freezeHighlight(): void {},
+      cleanup(): void {}
+    }
+    const window = { __orcaGrab: grab }
+    return {
+      window,
+      click: () => handlers.click?.(noopEvent),
+      contextmenu: () => handlers.contextmenu?.(noopEvent),
+      // cancelAwait is installed on __orcaGrab by the script itself at runtime.
+      cancel: () => (window.__orcaGrab.cancelAwait as (() => void) | undefined)?.()
+    }
+  }
+
+  const runAwaitClick = (harness: ReturnType<typeof armGrabHarness>): unknown =>
+    runInNewContext(buildGuestOverlayScript('awaitClick'), {
+      window: harness.window,
+      Promise: ZoneAwarePromiseLike,
+      Error
+    })
+
+  it('returns the payload through a native async promise, not the page-global Promise', () => {
+    const script = buildGuestOverlayScript('awaitClick')
+    expect(script).toContain('(async function()')
+    expect(script).toContain('return await new Promise(')
+  })
+
+  it('resolves { page, target } across the boundary despite ZoneAwarePromise', async () => {
+    const harness = armGrabHarness()
+    const completion = runAwaitClick(harness)
+
+    // The async IIFE hands Electron an intrinsic promise even though the global
+    // Promise is a non-native thenable — so the boundary unwraps it.
+    expect(types.isPromise(completion)).toBe(true)
+
+    harness.click()
+    const received = await crossExecuteJavaScriptBoundary(completion)
+
+    expect(received).toHaveProperty('page')
+    expect(received).toHaveProperty('target')
+    expect(received).not.toHaveProperty('__zone_symbol__value')
+    expect(clampGrabPayload(received)).not.toBeNull()
+  })
+
+  it('resolves the context-menu marker across the boundary despite ZoneAwarePromise', async () => {
+    const harness = armGrabHarness()
+    const completion = runAwaitClick(harness)
+
+    harness.contextmenu()
+    const received = (await crossExecuteJavaScriptBoundary(completion)) as Record<string, unknown>
+
+    expect(received).toHaveProperty('__orcaContextMenu', true)
+    expect(received.payload).toHaveProperty('page')
+    expect(clampGrabPayload(received.payload)).not.toBeNull()
+  })
+
+  it('resolves the teardown cancel marker across the boundary despite ZoneAwarePromise', async () => {
+    const harness = armGrabHarness()
+    const completion = runAwaitClick(harness)
+
+    harness.cancel()
+    const received = await crossExecuteJavaScriptBoundary(completion)
+
+    expect(received).toEqual({ __orcaCancelled: true })
+  })
+
+  it('rejects across the boundary when selection fails despite ZoneAwarePromise', async () => {
+    // getCurrentElement -> null drives onClick's reject(new Error('cancelled')),
+    // which must surface as a rejected intrinsic promise (not a serialized value)
+    // so the controller classifies it as a cancellation rather than a payload.
+    const harness = armGrabHarness({ getCurrentElement: () => null })
+    const completion = runAwaitClick(harness)
+
+    harness.click()
+    await expect(crossExecuteJavaScriptBoundary(completion)).rejects.toThrow('cancelled')
+  })
+
+  it('control: a bare page-global new Promise would cross as the raw wrapper', async () => {
+    // Proves the harness detects the regression: without the async wrapper the
+    // completion value is the ZoneAwarePromise itself, which the boundary
+    // serializes to __zone_symbol__* fields with no page/target.
+    const bare = runInNewContext('new Promise(function(r){ r({ page: {}, target: {} }); })', {
+      Promise: ZoneAwarePromiseLike
+    })
+    // Symbol.toStringTag='Promise' fools a toString check — exactly why the
+    // boundary must use the brand-based IsPromise, which still rejects it.
+    expect(Object.prototype.toString.call(bare)).toBe('[object Promise]')
+    expect(types.isPromise(bare)).toBe(false)
+
+    const received = await crossExecuteJavaScriptBoundary(bare)
+    expect(received).not.toHaveProperty('page')
+    expect(received).toHaveProperty('__zone_symbol__value')
+    expect(clampGrabPayload(received)).toBeNull()
+  })
+})

src/main/browser/grab-guest-script.ts

@@ -823,80 +823,90 @@ const ARM_SCRIPT = `(function() {
 })()`
 
 // awaitClick: resolve when the user clicks the overlay; stopPropagation + pointer-events:all keep the click off the page.
-const AWAIT_CLICK_SCRIPT = `new Promise(function(resolve, reject) {
-  'use strict';
-  var grab = window.__orcaGrab;
-  if (!grab) {
-    reject(new Error('Grab not armed'));
-    return;
-  }
-
-  function extractSelectedPayload(el) {
-    try {
-      return grab.extractPayload(el);
-    } catch (error) {
-      grab.cleanup();
-      reject(error instanceof Error ? error : new Error('Failed to extract element context'));
-      return null;
-    }
-  }
-
-  function onClick(e) {
-    e.preventDefault();
-    e.stopPropagation();
-    e.stopImmediatePropagation();
-    grab.host.removeEventListener('click', onClick, true);
-    grab.host.removeEventListener('contextmenu', onContext, true);
-    var el = grab.getCurrentElement();
-    if (!el) {
-      grab.cleanup();
-      reject(new Error('cancelled'));
+const AWAIT_CLICK_SCRIPT = `(async function() {
+  // Why: hand the click result to executeJavaScript through a native (intrinsic)
+  // Promise. On pages that replace the global Promise with a non-native thenable
+  // — e.g. Angular Zone.js's ZoneAwarePromise — a bare \`new Promise(...)\` is not
+  // recognized as a promise by Electron, so its raw wrapper object (exposing
+  // __zone_symbol__state/__value instead of { page, target }) crosses the boundary
+  // and main rejects it as an invalid payload structure. An async function's
+  // promise comes from the engine intrinsic that page code cannot reassign, so
+  // Electron always unwraps it to the resolved payload.
+  return await new Promise(function(resolve, reject) {
+    'use strict';
+    var grab = window.__orcaGrab;
+    if (!grab) {
+      reject(new Error('Grab not armed'));
       return;
     }
-    var payload = extractSelectedPayload(el);
-    if (!payload) return;
-    // Why: freeze the highlight instead of removing it so the user sees
-    // which element was selected while the copy menu is shown. Teardown
-    // happens later when the renderer calls setGrabMode(false) or re-arms.
-    grab.freezeHighlight();
-    resolve(payload);
-  }
 
-  function onContext(e) {
-    // Why: right-click resolves with the payload wrapped in a context-menu
-    // marker so the renderer can show the full action dropdown instead of
-    // auto-copying. This gives users a deliberate path to screenshot and
-    // other secondary actions while keeping left-click as the fast copy path.
-    e.preventDefault();
-    e.stopPropagation();
-    e.stopImmediatePropagation();
-    grab.host.removeEventListener('click', onClick, true);
-    grab.host.removeEventListener('contextmenu', onContext, true);
-    var el = grab.getCurrentElement();
-    if (!el) {
-      grab.cleanup();
-      reject(new Error('cancelled'));
-      return;
+    function extractSelectedPayload(el) {
+      try {
+        return grab.extractPayload(el);
+      } catch (error) {
+        grab.cleanup();
+        reject(error instanceof Error ? error : new Error('Failed to extract element context'));
+        return null;
+      }
     }
-    var payload = extractSelectedPayload(el);
-    if (!payload) return;
-    grab.freezeHighlight();
-    resolve({ __orcaContextMenu: true, payload: payload });
-  }
 
-  grab.host.addEventListener('click', onClick, true);
-  grab.host.addEventListener('contextmenu', onContext, true);
+    function onClick(e) {
+      e.preventDefault();
+      e.stopPropagation();
+      e.stopImmediatePropagation();
+      grab.host.removeEventListener('click', onClick, true);
+      grab.host.removeEventListener('contextmenu', onContext, true);
+      var el = grab.getCurrentElement();
+      if (!el) {
+        grab.cleanup();
+        reject(new Error('cancelled'));
+        return;
+      }
+      var payload = extractSelectedPayload(el);
+      if (!payload) return;
+      // Why: freeze the highlight instead of removing it so the user sees
+      // which element was selected while the copy menu is shown. Teardown
+      // happens later when the renderer calls setGrabMode(false) or re-arms.
+      grab.freezeHighlight();
+      resolve(payload);
+    }
 
-  // Store cancel hook so teardown can settle the Promise
-  grab.cancelAwait = function() {
-    grab.host.removeEventListener('click', onClick, true);
-    grab.host.removeEventListener('contextmenu', onContext, true);
-    grab.cleanup();
-    // Why: teardown cancellation is a normal user flow; resolving a marker
-    // avoids a noisy guest-console Error while main still treats it as cancel.
-    resolve({ __orcaCancelled: true });
-  };
-})`
+    function onContext(e) {
+      // Why: right-click resolves with the payload wrapped in a context-menu
+      // marker so the renderer can show the full action dropdown instead of
+      // auto-copying. This gives users a deliberate path to screenshot and
+      // other secondary actions while keeping left-click as the fast copy path.
+      e.preventDefault();
+      e.stopPropagation();
+      e.stopImmediatePropagation();
+      grab.host.removeEventListener('click', onClick, true);
+      grab.host.removeEventListener('contextmenu', onContext, true);
+      var el = grab.getCurrentElement();
+      if (!el) {
+        grab.cleanup();
+        reject(new Error('cancelled'));
+        return;
+      }
+      var payload = extractSelectedPayload(el);
+      if (!payload) return;
+      grab.freezeHighlight();
+      resolve({ __orcaContextMenu: true, payload: payload });
+    }
+
+    grab.host.addEventListener('click', onClick, true);
+    grab.host.addEventListener('contextmenu', onContext, true);
+
+    // Store cancel hook so teardown can settle the Promise
+    grab.cancelAwait = function() {
+      grab.host.removeEventListener('click', onClick, true);
+      grab.host.removeEventListener('contextmenu', onContext, true);
+      grab.cleanup();
+      // Why: teardown cancellation is a normal user flow; resolving a marker
+      // avoids a noisy guest-console Error while main still treats it as cancel.
+      resolve({ __orcaCancelled: true });
+    };
+  });
+})()`
 
 const FINALIZE_SCRIPT = `(function() {
   'use strict';
@@ -933,8 +943,9 @@ const TEARDOWN_SCRIPT = `(function() {
   'use strict';
   var grab = window.__orcaGrab;
   if (!grab) return true;
-  // If there's an active awaitClick Promise, cancel it so the
-  // executeJavaScript call in main rejects and settles the grab op.
+  // If there's an active awaitClick Promise, cancel it: cancelAwait resolves
+  // it with the __orcaCancelled marker so the executeJavaScript call in main
+  // settles the grab op as a cancellation.
   if (grab.cancelAwait) {
     grab.cancelAwait();
   } else {

src/renderer/src/lib/agent-background-session-test-state.ts

@@ -30,7 +30,10 @@ export type AgentBackgroundSessionTestState = {
     { id: string; repoId: string; projectId: string; path: string; displayName: string }[]
   >
   tabsByWorktree: Record<string, { id: string; title: string }[]>
-  terminalLayoutsByTabId: Record<string, { ptyIdsByLeafId?: Record<string, string | undefined> }>
+  terminalLayoutsByTabId: Record<
+    string,
+    { ptyIdsByLeafId?: Record<string, string | undefined> }
+  >
   ptyIdsByTabId: Record<string, string[]>
   sshConnectionStates: Map<string, { status: string }>
   transientClearedAgentStatusConnectionIds: Record<string, true>