mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
Fix background native-chat setup terminal seeding (#29810)
This commit is contained in:
@@ -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",
|
||||
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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<string, string> }) => ({
|
||||
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 })
|
||||
})
|
||||
})
|
||||
@@ -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')
|
||||
|
||||
@@ -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) => {
|
||||
|
||||
@@ -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<typeof startRuntimeLocalWorktreeTerminals>[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'
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user