From 7f9b51cf5cc1de8769baa646b40c0c93af76be81 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:38:17 -0700 Subject: [PATCH] Fix background native-chat setup terminal seeding (#29810) --- config/tsconfig.node.json | 1 + docs/native-chat-setup-provisioning-design.md | 33 ++++ docs/native-chat-setup-provisioning-review.md | 55 +++++++ .../worktree-native-chat-setup.spec.ts | 145 ++++++++++++++++++ src/main/runtime/orca-runtime.test.ts | 1 + ...me-local-worktree-terminal-startup.test.ts | 18 +++ ...runtime-local-worktree-terminal-startup.ts | 17 +- .../lib/worktree-creation-chat-setup.test.ts | 4 + 8 files changed, 271 insertions(+), 3 deletions(-) create mode 100644 docs/native-chat-setup-provisioning-design.md create mode 100644 docs/native-chat-setup-provisioning-review.md create mode 100644 src/main/runtime/orca-runtime-tests/worktree-native-chat-setup.spec.ts diff --git a/config/tsconfig.node.json b/config/tsconfig.node.json index fafa7267bc5..6fec2a1ccf2 100644 --- a/config/tsconfig.node.json +++ b/config/tsconfig.node.json @@ -14,6 +14,7 @@ "./scripts/vitest-node-runtime-pool.ts", "../src/main/**/*", "../src/renderer/src/lib/skill-freshness-display-status.ts", + "../src/renderer/src/store/slices/worktrees/create/worktree-create-payload.ts", "../src/renderer/src/components/native-chat/native-chat-resolution-receipt.ts", "../src/renderer/src/components/native-chat/structured-agent-question-projection.ts", "../src/renderer/src/components/native-chat/native-chat-subagent-delegation.ts", diff --git a/docs/native-chat-setup-provisioning-design.md b/docs/native-chat-setup-provisioning-design.md new file mode 100644 index 00000000000..5c89ff49d79 --- /dev/null +++ b/docs/native-chat-setup-provisioning-design.md @@ -0,0 +1,33 @@ +# Native-chat background setup provisioning + +## Problem and evidence + +A structured native-chat creation deliberately omits terminal startup in `worktree-creation-flow-execute.ts`, while passing the selected agent as `createdWithAgent`. `buildRuntimeWorktreeCreateParams` and the RPC `buildManagedWorktreeCreateArgs` preserve that agent and leave activation false when startup is absent. The execution host therefore handles this as background creation, even when the renderer later opens the chat. + +The host already treats a submitted agent as a caller-provided surface for background creation without setup. With setup/default tabs, `provisionArgs` instead recognizes only the internal `promisedAgentChat` flag. In new-tab setup mode this inconsistency creates an idle shell before Setup. Renderer-only tests returning unprovisioned setup do not exercise that host decision. + +## Proposed policy + +Use one host-local caller-surface predicate for terminal provisioning and fallback shell seeding: explicit `promisedAgentChat` always counts; submitted `createdWithAgent` counts only when neither `activate` nor `runHooks` is true. These are transient inputs to this create, not persisted workspace metadata. Apply the same predicate whether or not setup/default tabs exist. + +Continue using `shouldSeedWorktreePrimaryTerminal`: a real startup, issue work, existing default terminal, setup-only choice, and split setup anchors retain their existing precedence. Activated attribution-only requests still owe a primary terminal; attribution must not become an activation promise. Preserve every setup split mode and current setup sequencing, surfacing, completion, and failure behavior. + +## Scope and compatibility + +Change only the host startup policy and regression coverage. No new RPC field, surface store, ownership/retry redesign, split removal, process command change, or UI styling. Renderer and RPC already transmit enough information for the existing background policy; a new wire intent would duplicate that policy and leave older clients unfixed. Old clients benefit when connected to the updated execution host; updated clients connected to old hosts retain the old behavior. Nothing changes execution-host ownership or introduces client-local SSH fallback. + +Folder creation has its own matching no-setup background policy and no automated setup provisioning path; retain it and cover its existing contract. The shared host policy is platform independent and leaves macOS/Linux/Windows/WSL runner construction unchanged. Manual splits, explicit commands, issue automation, default tabs, and startup failures remain owned by their existing mechanisms. + +Background attribution-only callers intentionally inherit their established no-setup policy. The pre-existing conditional issue-only/first-terminal coupling and asynchronous setup failure reporting are P2 follow-ups outside this change; successful setup/default provisioning still supplies the terminal used for queued issue work. Independent Astra design review accepted this scope with no P0/P1 before implementation. + +## Verification + +Add coverage that feeds actual structured renderer creation arguments through the production payload builder, RPC schema/argument mapping, host startup, and real terminal provisioner, recording created terminals. New-tab setup should create Setup alone for background native chat, while blank/background creation still receives its shell. Cover both agents, activated attribution-only creation, run-hooks, promised chat, explicit startup, default tabs, split anchors, and setup-only/failure behavior using existing tests where adequate. + +Demonstrate the regression failing against the original policy and passing after the change. Run relevant renderer/runtime/RPC/provisioning/folder tests, node and web typechecks, and changed-file format/lint checks with `ORCA_BACKGROUND_LAUNCH=1`. No platform or rendered UI result is claimed without an actual run. Independent Astra review must converge before implementation and review the final diff with no unresolved P0/P1; record P2 dispositions in the review report. + +## Results + +The original policy failed both agent regressions by spawning an undefined-command shell before Setup; nine controls passed. With the fix, seven selected suites passed 1,451 tests with one existing skip; 15 focused route/promised-chat controls and four first-surface tests also passed. Node/web typechecks, changed-file lint, formatting, and the Electron e2e build passed on macOS ARM64. + +A temporary Playwright probe used isolated hidden host/client apps, the production renderer create action with `createdWithAgent: codex` and no startup, real RPC/PTY provisioning, host terminal inventory, and DOM tab-strip assertions. The baseline host rendered two terminal tabs; the fixed host rendered Setup alone. Both runs passed hidden/unfocused-window assertions and captured CDP screenshots. This verifies the terminal provisioning behavior, without claiming a real structured-agent launch or live SSH/Windows/Linux/WSL run. Probe source, logs, and screenshots are retained locally under `.bench-fixtures/native-chat-setup-proof/`; screenshots are attached to the PR, not committed. Final source was restored and rebuilt after the baseline capture. diff --git a/docs/native-chat-setup-provisioning-review.md b/docs/native-chat-setup-provisioning-review.md new file mode 100644 index 00000000000..9466d57e2d2 --- /dev/null +++ b/docs/native-chat-setup-provisioning-review.md @@ -0,0 +1,55 @@ +# Independent native-chat setup provisioning review + +## Design verdict + +Accepted before implementation and after final diff/test review: no unresolved P0/P1 findings. All P2 concerns have explicit dispositions below. + +The reviewed design is `docs/native-chat-setup-provisioning-design.md`. Review is confined to this worktree; this report is the reviewer's only edited file. + +## Root cause and simplest mechanism + +The structured route in `worktree-creation-flow-execute.ts` deliberately omits terminal startup and startup draft, but supplies the selected agent. `worktree-create-payload.ts`, the RPC schema, and `worktree-create-args.ts` preserve that attribution. No startup means no activation request from this payload builder. + +`runtime-local-worktree-terminal-startup.ts` already suppresses a fallback shell for a background create with either `promisedAgentChat` or `createdWithAgent`. Its setup/default provisioning branch supplies only `promisedAgentChat` to `callerProvidesSurface`. The extra shell is therefore a disagreement between two branches of the same host policy. + +Use the existing background policy consistently. Keep explicit `promisedAgentChat` unconditional, and count submitted agent attribution only when neither `activate` nor `runHooks` is true. A new RPC flag is unnecessary: it duplicates existing intent and would leave older clients' requests unfixed. Do not infer this from persisted worktree metadata or introduce a surface store. + +## Challenges and dispositions + +| Concern | Severity | Disposition | +| --------------------------------------------------------------------------------------------- | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Attribution is weaker than an explicit chat promise. | P2 | Accepted existing background policy, not a new universal interpretation. Activated/run-hooks attribution-only creates must retain a primary surface. | +| A new unconditional chat promise could strand renderer issue commands waiting for a terminal. | P1 candidate | Withdrawn for the chosen design: no-setup policy is unchanged, while successful setup/default provisioning supplies a mirrored terminal. No new wire promise is being added. | +| Renderer-only setup tests cannot catch the execution host's extra shell. | P2 | Require a regression through production payload/schema/mapper and host startup/provisioner, with real creation argument evidence. | +| Older clients and hosts may differ. | P2 | No wire change: new hosts apply the corrected policy to existing requests; old hosts retain the known extra shell. No capability negotiation or client-local fallback needed. | +| Setup spawn failure and renderer issue-only delivery have existing asynchronous limitations. | P2 | Keep out of this policy correction; do not claim these are repaired. Preserve current setup receipt/return behavior and failure controls. | + +## Invariants checked during design review + +- Execution host retains terminal provisioning and setup ownership; navigation does not choose another execution host. +- The shared first-surface predicate still requires anchors for both setup split orientations and real terminal startup. +- Configured default tabs remain explicit work; they are not discarded to make the terminal count smaller. +- Setup runner construction, quoting, environment, sequencing, completion observation, and shell selection remain untouched across macOS/Linux/Windows/WSL. +- Native IPC returns setup to the renderer; paired/runtime creation reaches the host policy under review. No direct SSH fallback is introduced. +- Folder workspace creation has a separate background no-setup policy and no corresponding setup-provisioning branch; preserve it. +- Manual splits and issue command queuing remain in their existing paths. No claim is made that a setup terminal is itself a structured agent surface. + +## Final verification + +Final review requested and performed on the production predicate, route regression, renderer assertions, explicit promised-chat controls, and node typecheck inclusion. No unresolved P0/P1 found. Final `git diff --check` passed. + +The sole production change replaces both branch-specific decisions with `callerProvidesInitialSurface`. The original no-setup branch can execute only when activation/run-hooks are false, so adding those guards to the common predicate does not change its behavior. Setup/default provisioning is the only newly affected branch. Explicit startup still supplies `startupNeedsTerminal`; successful startup/default terminals still count as existing primary surfaces; split modes still override suppression. + +The route regression invokes the production renderer payload builder, RPC schema/mapper, runtime creation, and terminal provisioner. The renderer test separately checks the actual execute call's selected agent, absent startup, and absent startup draft. This is composed route evidence, not a rendered end-to-end UI test. The runtime fixture waits for provisioning to observe terminal calls deterministically. + +Reviewed the lead's pre-fix log `/tmp/orca-native-chat-before.log`: both agent cases failed on the unexpected `undefined` command preceding Setup, while nine controls passed. Reviewed `/tmp/orca-native-chat-tests.log`: seven files passed, 1,451 tests passed and one skipped. Supplemental validation is attributed below. + +P2 test hardening resolved during final review: the split control now asserts that `ORCA_TAB_ID` is a nonempty string before comparing the two panes' values. + +Independent baseline run: `ORCA_BACKGROUND_LAUNCH=1 pnpm test` for `runtime-local-worktree-terminal-startup.test.ts`, `runtime-worktree-terminal-provisioning.test.ts`, `runtime-folder-worktree-create.test.ts`, and `rpc/methods/worktree-create-args.test.ts` passed all 37 tests in four files. These existing tests establish controls; they do not demonstrate the new regression fix. + +The added node typecheck include names only the pure, type-only-importing renderer payload builder used by the regression; it follows existing cross-layer fixture includes and introduces no runtime renderer dependency. + +Lead-reported supplemental validation: 15 focused promised-chat/route controls and four `worktree-agent-first-surface` controls passed; node/web typechecks exited zero and changed-file lint passed. The reviewed final source contains the corrected predicate and the strengthened split assertion. + +The coordinator explicitly closed the independent code review after these checks. Any hidden-renderer before/after proof is lead-owned and is not an independently verified claim in this report. The reviewer launched no application or visible window. No live SSH, Windows, or WSL validation is claimed; compatibility conclusions concern the unchanged routing, wire contract, and runner code. diff --git a/src/main/runtime/orca-runtime-tests/worktree-native-chat-setup.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-native-chat-setup.spec.ts new file mode 100644 index 00000000000..040f37d57cb --- /dev/null +++ b/src/main/runtime/orca-runtime-tests/worktree-native-chat-setup.spec.ts @@ -0,0 +1,145 @@ +import { describe, expect, it, vi } from 'vitest' +import { getDefaultWorkspaceSession } from '../../../shared/constants' +import { WorktreeCreate } from '../../../shared/rpc-contract/worktree-create-params' +import type { SetupScriptLaunchMode } from '../../../shared/worktree/launch-types' +import { + buildRuntimeWorktreeCreateParams, + type WorktreeCreateRequest +} from '../../../renderer/src/store/slices/worktrees/create/worktree-create-payload' +import { buildManagedWorktreeCreateArgs } from '../rpc/methods/worktree-create-args' +import { + OrcaRuntimeService, + computeWorktreePathMock, + createSetupRunnerScript, + ensurePathWithinWorkspaceMock, + getDefaultTabsLaunch, + getEffectiveHooks, + listWorktrees +} from '../orca-runtime-test-mocks.spec' +import { makeRuntimeStoreWithWorkspaceSession, store } from '../orca-runtime-test-fixtures.spec' + +const WORKTREE_PATH = '/tmp/workspaces/native-chat-setup' +const RUNNER = '/tmp/repo/.git/orca/setup-runner.sh' +const SETUP_ENV = { ORCA_WORKTREE_PATH: WORKTREE_PATH, SETUP_INPUT: 'preserved' } + +async function createFromRendererPayload(options: { + agent?: WorktreeCreateRequest['createdWithAgent'] + mode?: SetupScriptLaunchMode + activate?: boolean + runHooks?: boolean + withSetup?: boolean + defaultTabs?: boolean + startup?: boolean +}) { + const mode = options.mode ?? 'new-tab' + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession(getDefaultWorkspaceSession()) + const runtime = new OrcaRuntimeService({ + ...runtimeStore, + getSettings: () => ({ ...store.getSettings(), setupScriptLaunchMode: mode }) + }) + const spawn = vi.fn(async (_args: { command?: string; env?: Record }) => ({ + id: `pty-native-chat-${spawn.mock.calls.length}` + })) + runtime.setPtyController({ + spawn, + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + computeWorktreePathMock.mockReturnValue(WORKTREE_PATH) + ensurePathWithinWorkspaceMock.mockReturnValue(WORKTREE_PATH) + vi.mocked(listWorktrees).mockResolvedValue([ + { + path: WORKTREE_PATH, + head: 'def', + branch: 'native-chat-setup', + isBare: false, + isMainWorktree: false + } + ]) + vi.mocked(getEffectiveHooks).mockReturnValue({ scripts: { setup: 'pnpm install' } }) + vi.mocked(createSetupRunnerScript).mockReturnValue({ + runnerScriptPath: RUNNER, + envVars: SETUP_ENV + }) + if (options.defaultTabs) { + vi.mocked(getDefaultTabsLaunch).mockReturnValue({ + runCommands: true, + tabs: [{ title: 'Dev', command: 'pnpm dev' }] + }) + } + const params = WorktreeCreate.parse({ + ...buildRuntimeWorktreeCreateParams( + { + repoId: 'repo-1', + name: 'native-chat-setup', + setupDecision: options.withSetup === false ? 'skip' : 'run', + ...(options.agent ? { createdWithAgent: options.agent } : {}), + ...(options.startup ? { startup: { command: 'codex' } } : {}) + }, + { name: 'native-chat-setup' } + ), + ...(options.activate ? { activate: true } : {}), + ...(options.runHooks ? { runHooks: true } : {}) + }) + const args = buildManagedWorktreeCreateArgs(params, {}, { clientKind: 'runtime' }) + expect(args.createdWithAgent).toBe(options.agent) + expect(args.startup?.command).toBe(options.startup ? 'codex' : undefined) + expect(args.activate).toBe(options.activate === true || options.startup === true) + expect(args.runHooks).toBe(options.runHooks === true) + const result = await runtime.createManagedWorktree({ ...args, awaitTerminalProvisioning: true }) + return { result, spawn } +} + +describe('native-chat setup through renderer payload, RPC and execution host', () => { + it.each(['claude', 'codex'] as const)( + 'creates only Setup for a background %s chat in new-tab mode', + async (agent) => { + const { result, spawn } = await createFromRendererPayload({ agent }) + expect(spawn.mock.calls.map(([args]) => args.command)).toEqual([`bash ${RUNNER}`]) + expect(spawn.mock.calls[0][0].env).toMatchObject(SETUP_ENV) + expect(result.setup).toBeUndefined() + expect(result.setupReceipt).toMatchObject({ state: 'running' }) + expect(result.startupTerminal).toBeUndefined() + } + ) + + it.each([false, true])('keeps existing no-setup background policy (agent=%s)', async (agent) => { + const { spawn } = await createFromRendererPayload({ + ...(agent ? { agent: 'codex' } : {}), + withSetup: false + }) + expect(spawn.mock.calls.map(([args]) => args.command)).toEqual(agent ? [] : [undefined]) + }) + + it.each([ + { label: 'blank selection' }, + { label: 'activated attribution', agent: 'codex' as const, activate: true }, + { label: 'run-hooks attribution', agent: 'codex' as const, runHooks: true } + ])('keeps a primary shell for $label', async (options) => { + const { spawn } = await createFromRendererPayload(options) + expect(spawn.mock.calls.map(([args]) => args.command)).toEqual([undefined, `bash ${RUNNER}`]) + }) + + it.each(['split-horizontal', 'split-vertical'] as const)( + 'keeps the primary shell needed by %s setup', + async (mode) => { + const { spawn } = await createFromRendererPayload({ agent: 'codex', mode }) + expect(spawn.mock.calls.map(([args]) => args.command)).toEqual([undefined, `bash ${RUNNER}`]) + expect(spawn.mock.calls[0][0].env?.ORCA_TAB_ID).toEqual(expect.any(String)) + expect(spawn.mock.calls[0][0].env?.ORCA_TAB_ID).not.toBe('') + expect(spawn.mock.calls[0][0].env?.ORCA_TAB_ID).toBe(spawn.mock.calls[1][0].env?.ORCA_TAB_ID) + } + ) + + it('keeps configured default commands alongside Setup', async () => { + const { spawn } = await createFromRendererPayload({ agent: 'codex', defaultTabs: true }) + expect(spawn.mock.calls.map(([args]) => args.command)).toEqual(['pnpm dev', `bash ${RUNNER}`]) + }) + + it('keeps the actual startup terminal alongside Setup', async () => { + const { result, spawn } = await createFromRendererPayload({ agent: 'codex', startup: true }) + expect(spawn.mock.calls.map(([args]) => args.command)).toEqual(['codex', `bash ${RUNNER}`]) + expect(result.startupTerminal).toMatchObject({ spawned: true }) + }) +}) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index d09a368dfc4..14d01fac31d 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -114,6 +114,7 @@ await import('./orca-runtime-tests/worktree-setup-and-startup-part-02.spec') await import('./orca-runtime-tests/worktree-setup-and-startup-part-03.spec') await import('./orca-runtime-tests/worktree-setup-and-startup-part-04.spec') await import('./orca-runtime-tests/worktree-setup-single-creator.spec') +await import('./orca-runtime-tests/worktree-native-chat-setup.spec') await import('./orca-runtime-tests/gitlab-and-pr-bases.spec') await import('./orca-runtime-tests/gitlab-and-pr-bases-part-02.spec') await import('./orca-runtime-tests/gitlab-and-pr-bases-part-03.spec') diff --git a/src/main/runtime/runtime-local-worktree-terminal-startup.test.ts b/src/main/runtime/runtime-local-worktree-terminal-startup.test.ts index a8bd5144bb3..98794bbf3a9 100644 --- a/src/main/runtime/runtime-local-worktree-terminal-startup.test.ts +++ b/src/main/runtime/runtime-local-worktree-terminal-startup.test.ts @@ -88,6 +88,24 @@ describe('startRuntimeLocalWorktreeTerminals reserved startup pane', () => { }) describe('startRuntimeLocalWorktreeTerminals default shell seeding', () => { + it.each([{}, { activate: true }, { runHooks: true }])( + 'preserves an explicit promised chat during setup (%j)', + async (activation) => { + const { ports } = createPorts() + await startRuntimeLocalWorktreeTerminals({ + request: { repoSelector: repo.id, name: 'chat', ...activation }, + repo, + worktree, + setup: { runnerScriptPath: '/repo/setup.sh', envVars: {} }, + promisedAgentChat: true, + ports + }) + expect(ports.provision).toHaveBeenCalledWith( + expect.objectContaining({ callerProvidesSurface: true }) + ) + } + ) + it.each([false, true])( 'passes the submitted setup-only choice to the host (%s)', async (setupOnly) => { diff --git a/src/main/runtime/runtime-local-worktree-terminal-startup.ts b/src/main/runtime/runtime-local-worktree-terminal-startup.ts index 3a613f42d7c..f4a9e7a4b27 100644 --- a/src/main/runtime/runtime-local-worktree-terminal-startup.ts +++ b/src/main/runtime/runtime-local-worktree-terminal-startup.ts @@ -190,8 +190,7 @@ export async function startRuntimeLocalWorktreeTerminals(args: { ports.canSpawn && shouldSeedWorktreePrimaryTerminal({ hasPrimarySurface: didSpawnStartup, - // Preserve background create behavior for attribution-only callers. - callerProvidesSurface: args.promisedAgentChat === true || args.createdWithAgent !== undefined + callerProvidesSurface: callerProvidesInitialSurface(args) }) ) { try { @@ -236,7 +235,7 @@ function provisionArgs( primaryTerminalHandle, hasStartupTerminal, ...(args.request.setupOnly !== undefined ? { setupOnly: args.request.setupOnly } : {}), - callerProvidesSurface: args.promisedAgentChat === true, + callerProvidesSurface: callerProvidesInitialSurface(args), startupNeedsTerminal: args.startup !== undefined, setupCommandPlatform: setupPlatform(args.setup, 'posix'), observeSetupCompletion: args.request.observeSetupCompletion, @@ -244,6 +243,18 @@ function provisionArgs( } } +function callerProvidesInitialSurface( + args: Parameters[0] +): boolean { + // Background agent creates already leave their initial surface to the caller. + return ( + args.promisedAgentChat === true || + (args.createdWithAgent !== undefined && + args.request.activate !== true && + args.request.runHooks !== true) + ) +} + function setupPlatform( setup: CreateWorktreeResult['setup'], fallback: 'windows' | 'posix' diff --git a/src/renderer/src/lib/worktree-creation-chat-setup.test.ts b/src/renderer/src/lib/worktree-creation-chat-setup.test.ts index 3c6be8b26af..c41afd2b851 100644 --- a/src/renderer/src/lib/worktree-creation-chat-setup.test.ts +++ b/src/renderer/src/lib/worktree-creation-chat-setup.test.ts @@ -66,6 +66,10 @@ describe('native chat creation completed in the background', () => { await executeWorktreeCreation('creation-1', request) + const createArgs = vi.mocked(useAppStore.getState().createWorktree).mock.calls[0] + expect(createArgs[10]).toBe(agent) + expect(createArgs[16]).toBeUndefined() + expect(createArgs[25]).not.toHaveProperty('startupDraft') const state = useAppStore.getState() const tabs = state.tabsByWorktree[worktree.id] expect(tabs).toHaveLength(1)