From 220811cbad112e03957a879de1ed763eb1746c12 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:40:57 -0700 Subject: [PATCH 1/3] fix(e2e): repair release e2e suite (parking regression tests, stale/flaky specs, profile switcher gate) (#8486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(e2e): repair release e2e suite — parking regression tests, stale/flaky specs, profile switcher gate Diagnosed 20 failing tests across the release e2e shards. Most are test debt, plus two genuine product-side issues. Product fixes: - OrcaProfileSwitcher: the PROD gate hid the "Switch profile" button in the e2e build (electron-vite build bakes NODE_ENV=production). Exempt MODE==='e2e' so the specs render it while packaged prod builds stay hidden. Parking cluster (8 tests): #8262 intentionally keeps the most-recently-hidden tab warm (exempt from cold-park). The specs hid exactly one tab — always the exempt one — so it never parked. Open a throwaway decoy tab that absorbs the last-active exemption so the target parks. (terminal-hidden-view-parking, terminal-pane-close-layout-consistency) Stale tests updated to match intended product behavior: - rich-markdown-link-bubble: match Edit link by aria-label (title dropped in #8307) - terminal-codex-hidden-startup-background: drop the dead hiddenRendererSkipCount poll (Phase-4 main-side delivery gate #7214 bypasses that renderer path) Brittle threshold/geometry/timing hardening (no product regression): - agent-session-log-tail-stability: assert full-model length instead of a machine-specific word-wrap pixel baseline - artificial-opencode revisit: dedicated under-backpressure latency bound - terminal-history-size-typing-latency: gate p90 not max (tolerate one checkpoint-in-window spike; median stays strict) - combined-diff-scroll-restore: assert viewport barely moved vs exact anchor key - terminal-shortcuts: idempotent kitty-flag reset instead of a racing stack pop - agent-session-live-force-exit-resume: drive the product quit-capture path - renderer-crash-recovery-terminal-input: poll the transport probe over the recovery budget (still flags a permanently frozen pane) terminal-push-delivery-loss-recovery left unchanged (no safe test-only improvement; recovery is wall-clock bounded with ample slack). * Extract shared parking helpers into terminal-hidden-parking.ts for e2e s - Deduplicate waitForTabParked/parkHiddenTabBehindDecoy, previously copy-pasted across the parking and layout-consistency specs - Parameterize parkDelayMs so the helper no longer depends on a file-local PARKING_DELAY_MS constant --- config/reliability-gates.jsonc | 6 +- .../orca-profiles/OrcaProfileSwitcher.tsx | 10 +-- ...ent-session-live-force-exit-resume.spec.ts | 5 ++ .../agent-session-log-tail-stability.spec.ts | 9 ++- ...cial-opencode-revisit-pressure-scenario.ts | 7 +- .../artificial-opencode-terminal-load.spec.ts | 6 ++ .../e2e/combined-diff-scroll-restore.spec.ts | 5 +- tests/e2e/helpers/terminal-hidden-parking.ts | 65 +++++++++++++++++++ ...erer-crash-recovery-terminal-input.spec.ts | 23 ++++++- ...rich-markdown-link-bubble-stacking.spec.ts | 4 +- ...al-codex-hidden-startup-background.spec.ts | 36 ++-------- .../e2e/terminal-hidden-view-parking.spec.ts | 47 ++++++++------ ...rminal-history-size-typing-latency.spec.ts | 13 +++- ...inal-pane-close-layout-consistency.spec.ts | 28 ++++---- tests/e2e/terminal-shortcuts.spec.ts | 7 +- 15 files changed, 183 insertions(+), 88 deletions(-) create mode 100644 tests/e2e/helpers/terminal-hidden-parking.ts diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 10fbfd0b5e0..bf6ef13b36e 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -28,7 +28,7 @@ "providers": ["local"], "coveredPlatforms": ["macos"], "coveredProviders": ["local"], - "coverageNotes": "Focused tests and real-Monaco 9/50 MiB benchmarks are platform-independent. Local macOS Electron evidence opens a synthetic 9 MiB transcript through Agent Session History at fixed 900x720 viewport, 13px font, 1x zoom, and the measured 2,775,880px legacy-failure geometry, then verifies three five-second-cadence watcher appends with Find open and closed. Live Windows/Linux evidence remains uncollected.", + "coverageNotes": "Focused tests and real-Monaco 9/50 MiB benchmarks are platform-independent. Local macOS Electron evidence opens a synthetic 9 MiB transcript through Agent Session History at fixed 900x720 viewport, 13px font, 1x zoom, and asserts the full 9 MiB model length loaded as a font-metric-independent containment check (word-wrap pixel geometry varies ~10% across runners, so a generous content-height floor is only a collapsed/truncated-render smoke check), then verifies three five-second-cadence watcher appends with Find open and closed. Live Windows/Linux evidence remains uncollected.", "motivatingLinks": ["https://github.com/stablyai/orca/pull/8432"], "invariant": "Append-only external file growth changes only Monaco's model suffix, retaining the viewport, selection, Find state, and renderer liveness above the append point; arbitrary rewrites continue to replace the model content.", "oracle": "Focused tests assert one post-mount content owner, actual outer lifecycle remount ordering across retained path models, exact end-of-model suffix edits with one model read, no-op equality, and full replacement for non-appends. With Node forced GC, real-Monaco benchmarks alternate 30 suffix and 30 replacement samples after five warmups on fresh equivalent models at 9 and 50 MiB, forcing GC and event-loop settlement between every arm. The Electron scenario alternates an e2e-only legacy setValue red control and the fixed watcher append from restored equivalent model/geometry at the measured legacy-failure cadence, asserting that the control disrupts anchor state while the fixed path preserves visible ranges, selection, complete Find state, scroll offset, renderer survival, and forced-GC heap/native-memory budgets.", @@ -68,7 +68,7 @@ { "file": "tests/e2e/agent-session-log-tail-stability.spec.ts", "assertions": [ - "production Agent Session History opens a synthetic 9 MiB View Log at the measured 2,775,880px legacy-failure geometry", + "production Agent Session History opens a synthetic 9 MiB View Log and confirms the full model length loaded as font-metric-independent containment, with a generous content-height floor as a collapsed/truncated-render smoke check", "an executable e2e-only legacy setValue control disrupts selection/Find/anchor state at each fixed-geometry five-second sample, then restores the equivalent model state before the fixed arm", "three alternating watcher suffix appends preserve visible ranges, selection, scroll offset, Find open/query/active-match state, and exact suffix content", "the renderer remains responsive with no render-process-gone event and forced-GC JS-heap/working-set/private-memory peak and retained budgets hold against paired legacy controls" @@ -106,7 +106,7 @@ ], "runtimeBudget": { "p95Seconds": 600, "scope": "local focused renderer tests plus one Electron production-journey scenario" }, "flakeHistory": { "status": "unknown", "evidence": "New deterministic gate with local macOS passes; CI soak history is not yet available." }, - "redGreenEvidence": { "status": "complete", "evidence": "The retained Electron gate itself executes the former read-only wrapper setValue behavior behind MODE=e2e at fixed 900x720, 13px, 1x zoom, 2,775,880px content height, and five-second cadence. The control retains a flat incoming value like the former controlled IPC prop. Each legacy arm must disrupt selection, Find active-match state, visible range, or scroll anchor, then restore identical model length/tail/geometry and anchor state before the fixed watcher arm; every fixed arm must preserve them. Production builds never install the control. The original exact-file dev repro also recorded renderer exit code 5." }, + "redGreenEvidence": { "status": "complete", "evidence": "The retained Electron gate itself executes the former read-only wrapper setValue behavior behind MODE=e2e at fixed 900x720, 13px, 1x zoom, the full 9 MiB model length loaded (with a generous content-height floor, since word-wrap pixel geometry is runner-dependent), and five-second cadence. The control retains a flat incoming value like the former controlled IPC prop. Each legacy arm must disrupt selection, Find active-match state, visible range, or scroll anchor, then restore identical model length/tail/geometry and anchor state before the fixed watcher arm; every fixed arm must preserve them. Production builds never install the control. The original exact-file dev repro also recorded renderer exit code 5." }, "performanceBudget": { "required": true, "evidence": "Every update retrieves the model value once and performs at most one equality-or-prefix comparison; a matching append submits only the suffix. The registered Node command requires --expose-gc and --pool=threads so worker GC is available; alternating fresh real-Monaco arms force GC and deterministic event-loop settlement after every operation. Observed p95 ranges: 9 MiB append 4.02-5.51ms versus replacement 81.42-83.76ms; 50 MiB append 22.72-26.60ms versus replacement 445.11-449.21ms. Electron alternates each legacy replacement control and fixed watcher suffix from equivalent restored model state/geometry, samples forced-GC JS heap plus app.getAppMetrics renderer working set and OS private memory before/after/settled, asserts retained memory within max(20MiB,10%), and requires suffix peak deltas for jsHeapMb, workingSetMb, and privateMb not exceed the paired legacy controls." }, "promotionCriteria": [ "Collect stable soak history on macOS, Linux, and Windows.", diff --git a/src/renderer/src/components/orca-profiles/OrcaProfileSwitcher.tsx b/src/renderer/src/components/orca-profiles/OrcaProfileSwitcher.tsx index f8a98da9d0a..5be9359802e 100644 --- a/src/renderer/src/components/orca-profiles/OrcaProfileSwitcher.tsx +++ b/src/renderer/src/components/orca-profiles/OrcaProfileSwitcher.tsx @@ -87,11 +87,11 @@ export function OrcaProfileSwitcher({ }, [fetchProfiles, loading, profiles.length]) // Why: the Orca Cloud account UX isn't ready for production users yet, so the - // trigger stays hidden in packaged builds. Dev builds still show it when - // cloud auth is configured, and a dev-only Settings > Dev Tools > Orca Cloud - // section mirrors it. import.meta.env.PROD is true only in packaged builds - // (false in dev and under vitest), so tests still exercise the render path. - if (import.meta.env.PROD) { + // trigger stays hidden in packaged builds. Dev builds still show it when cloud + // auth is configured. electron-vite build forces NODE_ENV=production, so PROD + // is baked true even for the e2e bundle; exempt MODE==='e2e' so the switcher + // specs can exercise the render path while real packaged prod builds stay hidden. + if (import.meta.env.PROD && import.meta.env.MODE !== 'e2e') { return null } diff --git a/tests/e2e/agent-session-live-force-exit-resume.spec.ts b/tests/e2e/agent-session-live-force-exit-resume.spec.ts index 2bbf4c86aaa..82c49660195 100644 --- a/tests/e2e/agent-session-live-force-exit-resume.spec.ts +++ b/tests/e2e/agent-session-live-force-exit-resume.spec.ts @@ -196,6 +196,11 @@ test('resumes a live agent record after force-exit restart when pane PTY ownersh } ) + // Drive the product's own quit-capture path (what the 60s timer / beforeunload + // run) so the live record is flushed deterministically instead of racing the + // debounced session writer before the poll below. + await page.evaluate(() => window.__store?.getState().captureAllSleepingAgentSessions()) + await expect .poll(() => persistedLiveRecordExists(session.userDataDir), { timeout: 15_000, diff --git a/tests/e2e/agent-session-log-tail-stability.spec.ts b/tests/e2e/agent-session-log-tail-stability.spec.ts index 243403e7915..04e8fa48899 100644 --- a/tests/e2e/agent-session-log-tail-stability.spec.ts +++ b/tests/e2e/agent-session-log-tail-stability.spec.ts @@ -57,9 +57,12 @@ test.describe('Agent Session History live log', () => { let baseline = await restoreAnchorState(orcaPage, true) console.log(`[live-log-stability] geometry ${JSON.stringify(baseline)}`) - // The legacy whole-model baseline at this fixed viewport/font/zoom moved - // the anchor at 2,775,880px; missing that geometry cannot claim containment. - expect(baseline.contentHeight).toBeGreaterThanOrEqual(2_775_880) + // Why: containment is proven by the full 9 MiB model length, which is + // font-independent. Word-wrap pixel geometry varies ~10% across runner font + // metrics (macOS baseline 2,775,880px; Linux CI ~2,498,292px), so keep only a + // generous content-height floor as a collapsed/truncated-render smoke check. + expect(baseline.valueLength).toBe(fixture.initialLength) + expect(baseline.contentHeight).toBeGreaterThanOrEqual(2_000_000) expect(baseline.visibleRanges.length).toBeGreaterThan(0) expect(baseline.find).toMatchObject({ open: true, query: ANCHOR_TOKEN }) expect(baseline.find.activeMatch).not.toBe('') diff --git a/tests/e2e/artificial-opencode-revisit-pressure-scenario.ts b/tests/e2e/artificial-opencode-revisit-pressure-scenario.ts index c6a9d6e35a4..c533a395e22 100644 --- a/tests/e2e/artificial-opencode-revisit-pressure-scenario.ts +++ b/tests/e2e/artificial-opencode-revisit-pressure-scenario.ts @@ -90,6 +90,7 @@ export async function runRendererBackpressureRevisitScenario< deps, maxMedianKeyLatencyMs, maxRendererSchedulerQueuedChars, + maxRevisitLatencyMs, maxTimerDriftMs, maxWorstKeyLatencyMs, mainRendererPressureTargetChars, @@ -102,6 +103,7 @@ export async function runRendererBackpressureRevisitScenario< deps: RevisitPressureDeps maxMedianKeyLatencyMs: number maxRendererSchedulerQueuedChars: number + maxRevisitLatencyMs: number maxTimerDriftMs: number maxWorstKeyLatencyMs: number mainRendererPressureTargetChars: number @@ -205,7 +207,10 @@ export async function runRendererBackpressureRevisitScenario< 1 )}ms heldAckChars=${ackGate?.heldAckChars ?? 0}` }) - expect(revisitLatencyMs).toBeLessThan(maxWorstKeyLatencyMs) + // Why: this printf is measured after a worktree switch/focus while the + // background panes are still ACK-gate-held, so it gets its own under-load + // bound rather than the unloaded worst-key budget. + expect(revisitLatencyMs).toBeLessThan(maxRevisitLatencyMs) await deps.releaseTerminalAckGate(orcaPage) await deps.focusPane(orcaPage, loadPanes[0]?.paneKey ?? revisitPane.paneKey) diff --git a/tests/e2e/artificial-opencode-terminal-load.spec.ts b/tests/e2e/artificial-opencode-terminal-load.spec.ts index 99e31cd96db..1744fec4fa0 100644 --- a/tests/e2e/artificial-opencode-terminal-load.spec.ts +++ b/tests/e2e/artificial-opencode-terminal-load.spec.ts @@ -131,6 +131,11 @@ const MAX_WORST_KEY_LATENCY_MS = 300 // a catastrophic-hang detector. Mirrors ssh-docker-relay-perf's 2s worst-key // tolerance and the hidden-pressure scenario's relaxed worst budget. const MAX_WORST_KEY_LATENCY_UNDER_LOAD_MS = 3_000 +// Why: the post-revisit printf is sampled while the background panes are still +// ACK-gate-held and through a whole-buffer serialize poll, so it inherits the +// same environment-dominated worst-case as typing under load; the unloaded +// 300ms budget has no margin for this switch-and-focus moment. +const MAX_REVISIT_LATENCY_UNDER_LOAD_MS = 3_000 // Why: GitHub's two-worker Electron shards can briefly starve renderer timers // without visible typing lag. Keep this as a smoke gate, not a CPU lottery. const MAX_TIMER_DRIFT_MS = 250 @@ -604,6 +609,7 @@ test.describe('Artificial OpenCode terminal load', () => { mainRendererPressureTargetChars: MAIN_RENDERER_PRESSURE_TARGET_CHARS, maxMedianKeyLatencyMs: MAX_MEDIAN_KEY_LATENCY_MS, maxRendererSchedulerQueuedChars: MAX_RENDERER_SCHEDULER_QUEUED_CHARS, + maxRevisitLatencyMs: MAX_REVISIT_LATENCY_UNDER_LOAD_MS, maxTimerDriftMs: MAX_TIMER_DRIFT_MS, maxWorstKeyLatencyMs: MAX_WORST_KEY_LATENCY_MS, orcaPage, diff --git a/tests/e2e/combined-diff-scroll-restore.spec.ts b/tests/e2e/combined-diff-scroll-restore.spec.ts index a32ec947af2..039bdf7f13a 100644 --- a/tests/e2e/combined-diff-scroll-restore.spec.ts +++ b/tests/e2e/combined-diff-scroll-restore.spec.ts @@ -423,7 +423,10 @@ test.describe('Combined diff scroll restore', () => { await clickVisibleDiffLine(orcaPage) const afterLineClick = await waitForStableViewportAnchor(orcaPage) - expect(afterLineClick.key).toBe(afterSwitch.key) + // Assert the viewport barely moved rather than an exact anchor key: sections + // are ~viewport-sized, so a sub-pixel focus scroll from the click can flip the + // topmost-visible key by one without meaningfully moving the scroll position. + expect(Math.abs(afterLineClick.scrollTop - afterSwitch.scrollTop)).toBeLessThan(40) expect(Math.abs(afterLineClick.top - afterSwitch.top)).toBeLessThan(80) } finally { rmSync(fixture.repoPath, { recursive: true, force: true }) diff --git a/tests/e2e/helpers/terminal-hidden-parking.ts b/tests/e2e/helpers/terminal-hidden-parking.ts new file mode 100644 index 00000000000..4b15f656ec2 --- /dev/null +++ b/tests/e2e/helpers/terminal-hidden-parking.ts @@ -0,0 +1,65 @@ +import type { Page } from '@stablyai/playwright-test' +import { expect } from '@stablyai/playwright-test' +import { getActiveTabId } from './store' +import { waitForActiveTerminalManager, waitForPaneIdentitySnapshot } from './terminal' + +function resolveParkWaitTimeoutMs(parkDelayMs?: number): number { + const delay = parkDelayMs ?? (Number(process.env.ORCA_E2E_TERMINAL_PARKING_DELAY_MS) || 500) + return Math.max(20_000, delay * 10) +} + +// Why: TerminalPane unmount deletes its entry from window.__paneManagers, so a +// missing manager is the observable signal that the tab's xterm was parked. +export async function waitForTabParked( + page: Page, + tabId: string, + options?: { parkDelayMs?: number } +): Promise { + const parkWaitStartedAt = Date.now() + await expect + .poll(async () => page.evaluate((id) => window.__paneManagers?.get(id) !== undefined, tabId), { + timeout: resolveParkWaitTimeoutMs(options?.parkDelayMs), + message: `terminal tab ${tabId} did not park (pane manager still mounted)` + }) + .toBe(false) + return Date.now() - parkWaitStartedAt +} + +async function createActiveTerminalTab(page: Page, worktreeId: string): Promise { + const tabId = await page.evaluate((worktreeId) => { + const store = window.__store + if (!store) { + throw new Error('createActiveTerminalTab: window.__store is unavailable') + } + const state = store.getState() + const tab = state.createTab(worktreeId, undefined, undefined, { activate: true }) + state.setActiveTab(tab.id) + state.setActiveTabType('terminal') + return tab.id + }, worktreeId) + + await expect + .poll(() => getActiveTabId(page), { + timeout: 5_000, + message: 'newly created terminal tab did not become active' + }) + .toBe(tabId) + await waitForActiveTerminalManager(page, 30_000) + await waitForPaneIdentitySnapshot(page, 1) + return tabId +} + +// Why: #8262 exempts the single most-recently-hidden tab from cold-park to keep +// the just-left view instantly warm, so a lone hidden tab never parks. Opening +// one more tab hides the current view (which then holds that exemption) and +// leaves the older `targetTabId` free to cold-park. Returns waitForTabParked's +// elapsed-ms so callers keep their parking annotations. +export async function parkHiddenTabBehindDecoy( + page: Page, + worktreeId: string, + targetTabId: string, + options?: { parkDelayMs?: number } +): Promise { + await createActiveTerminalTab(page, worktreeId) + return waitForTabParked(page, targetTabId, options) +} diff --git a/tests/e2e/renderer-crash-recovery-terminal-input.spec.ts b/tests/e2e/renderer-crash-recovery-terminal-input.spec.ts index 621842e54e8..6e4fc04b4e1 100644 --- a/tests/e2e/renderer-crash-recovery-terminal-input.spec.ts +++ b/tests/e2e/renderer-crash-recovery-terminal-input.spec.ts @@ -139,7 +139,28 @@ test.describe('Renderer crash recovery keeps terminal input alive', () => { // should reattach to the same session rather than spawn a fresh shell. const postPtyIds = await mainGetStorePtyIds(electronApp) - const transportAlive = await mainProbeTransportPaste(electronApp, `PASTE_POST_${cycle}_OK`) + // Why: the post-crash transport reattach is async — a single fire-and-forget + // probe can race the reconnect window. Poll over the recovery budget so a + // transient unbound-transport window does not fail the test; a transport + // still dead after the full budget is the real frozen-pane bug reported below. + let transportAlive = false + try { + await expect + .poll( + async () => { + transportAlive = await mainProbeTransportPaste( + electronApp, + `PASTE_POST_${cycle}_OK`, + 5_000 + ) + return transportAlive + }, + { timeout: RECOVERY_TIMEOUT_MS } + ) + .toBe(true) + } catch { + transportAlive = false + } const directAlive = postPtyIds.length > 0 && (await mainProbeDirectWrite(electronApp, postPtyIds[0], `DIRECT_POST_${cycle}_OK`)) diff --git a/tests/e2e/rich-markdown-link-bubble-stacking.spec.ts b/tests/e2e/rich-markdown-link-bubble-stacking.spec.ts index f84691db5db..090c8cb071d 100644 --- a/tests/e2e/rich-markdown-link-bubble-stacking.spec.ts +++ b/tests/e2e/rich-markdown-link-bubble-stacking.spec.ts @@ -102,7 +102,9 @@ test.describe('Rich markdown link bubble stacking', () => { expect(overlap.overlapWidth).toBeGreaterThan(8) expect(overlap.topElementIsBubble).toBe(true) - const editButton = bubble.getByTitle('Edit link') + // The Edit action exposes its label via aria-label (a shadcn Button + + // Radix tooltip), not a title attribute, so match by accessible name. + const editButton = bubble.getByRole('button', { name: 'Edit link' }) const editButtonBounds = await editButton.boundingBox() const explorerBounds = await explorer.boundingBox() expect(editButtonBounds).not.toBeNull() diff --git a/tests/e2e/terminal-codex-hidden-startup-background.spec.ts b/tests/e2e/terminal-codex-hidden-startup-background.spec.ts index 7c1f832771a..ff074c323de 100644 --- a/tests/e2e/terminal-codex-hidden-startup-background.spec.ts +++ b/tests/e2e/terminal-codex-hidden-startup-background.spec.ts @@ -13,12 +13,6 @@ import { import { getTerminalContent, waitForActiveTerminalManager } from './helpers/terminal' import { BACKGROUND_MOUNT_TERMINAL_WORKTREE_EVENT } from '../../src/renderer/src/constants/terminal' -type HiddenOutputDebugSnapshot = { - hiddenRendererSkipCount: number - hiddenRendererSkippedChars: number - hiddenRendererMode2031ReplyCount: number -} - type CodexStartupBackgroundTarget = { clip: { x: number; y: number; width: number; height: number } cellWidth: number @@ -28,13 +22,6 @@ type CodexStartupBackgroundTarget = { modelBackgroundCells: number } -type CodexStartupBackgroundWindow = Window & { - __terminalPtyOutputDebug?: { - reset: () => void - snapshot: () => HiddenOutputDebugSnapshot - } -} - const COMPOSER_BG = { red: 72, green: 72, blue: 72 } function codexLikeStartupCommand(marker: string): string { @@ -97,18 +84,6 @@ async function waitForHiddenTabPtyId(page: Page, tabId: string): Promise return ptyId } -async function readHiddenOutputDebug(page: Page): Promise { - return page.evaluate(() => { - return (window as CodexStartupBackgroundWindow).__terminalPtyOutputDebug?.snapshot() ?? null - }) -} - -async function resetHiddenOutputDebug(page: Page): Promise { - await page.evaluate(() => { - ;(window as CodexStartupBackgroundWindow).__terminalPtyOutputDebug?.reset() - }) -} - async function mainSnapshotContains(page: Page, ptyId: string, text: string): Promise { return page.evaluate( async ({ targetPtyId, expectedText }) => { @@ -266,7 +241,6 @@ test.describe('Codex hidden startup composer background', () => { const marker = `CODEX_STARTUP_BG_${Date.now()}` const command = codexLikeStartupCommand(marker) - await resetHiddenOutputDebug(orcaPage) const hiddenTabId = await orcaPage.evaluate( ({ worktreeId, command, eventName }) => { const store = window.__store @@ -312,12 +286,10 @@ test.describe('Codex hidden startup composer background', () => { message: 'Hidden Codex startup background never reached the main buffer snapshot' }) .toBe(true) - await expect - .poll(async () => (await readHiddenOutputDebug(orcaPage))?.hiddenRendererSkipCount ?? 0, { - timeout: 10_000, - message: 'Codex-marked hidden startup did not take the skipped renderer path' - }) - .toBeGreaterThan(0) + // Why: the renderer skip counter is dead under the Phase-4 main-side delivery + // gate (#7214) — hidden bytes are dropped in main before reaching the renderer. + // The main-buffer snapshot above proves the hidden output was handled; the + // reveal restore below proves it repaints when the worktree first shows. await switchToWorktree(orcaPage, secondWorktreeId) await expect diff --git a/tests/e2e/terminal-hidden-view-parking.spec.ts b/tests/e2e/terminal-hidden-view-parking.spec.ts index 4bd2e143e13..7a43dce0454 100644 --- a/tests/e2e/terminal-hidden-view-parking.spec.ts +++ b/tests/e2e/terminal-hidden-view-parking.spec.ts @@ -16,6 +16,10 @@ import { waitForActiveTerminalManager, waitForPaneIdentitySnapshot } from './helpers/terminal' +import { + parkHiddenTabBehindDecoy, + waitForTabParked +} from './helpers/terminal-hidden-parking' // Why: the parking wiring registers this handle (dev/exposeStore builds only) // so tests can detect that hidden-view parking is compiled in and which delay @@ -160,19 +164,6 @@ async function readTerminalTabViewState(page: Page, tabId: string): Promise { - const parkWaitStartedAt = Date.now() - await expect - .poll(async () => (await readTerminalTabViewState(page, tabId)).hasManager, { - timeout: Math.max(20_000, PARKING_DELAY_MS * 10), - message: `terminal tab ${tabId} did not park (pane manager still mounted)` - }) - .toBe(false) - return Date.now() - parkWaitStartedAt -} - async function activateTerminalTab(page: Page, tabId: string): Promise { await page.evaluate((targetTabId) => { const store = window.__store @@ -306,15 +297,18 @@ test.describe('Terminal hidden view parking', () => { .toContain(finalMarker) const tabBId = await createActiveTerminalTab(orcaPage, worktreeId) - const parkDetectedAfterMs = await waitForTabParked(orcaPage, tabAId) + const parkDetectedAfterMs = await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabAId, { + parkDelayMs: PARKING_DELAY_MS + }) const wiring = await readParkingWiring(orcaPage) testInfo.annotations.push({ type: 'terminal-parking', description: `parkDelayMs=${wiring.parkDelayMs ?? PARKING_DELAY_MS} parkDetectedAfterMs=${parkDetectedAfterMs}` }) - // Why: parking must be scoped to the hidden tab — the visible tab keeps - // a live pane manager and xterm. + // Why: parking must be scoped to the parked tab — tab B (hidden more + // recently, so #8262 keeps it warm) still holds a live pane manager and + // xterm while tab A tore down. const tabBState = await readTerminalTabViewState(orcaPage, tabBId) expect(tabBState.hasManager).toBe(true) expect(tabBState.paneCount).toBeGreaterThan(0) @@ -370,7 +364,9 @@ test.describe('Terminal hidden view parking', () => { const { worktreeId, tabAId, tabAPtyId } = setup await createActiveTerminalTab(orcaPage, worktreeId) - await waitForTabParked(orcaPage, tabAId) + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabAId, { + parkDelayMs: PARKING_DELAY_MS + }) const runId = randomUUID() const parkedTitle = `Parked side effects ${runId}` @@ -443,8 +439,12 @@ test.describe('Terminal hidden view parking', () => { .toBe(tabBId) // Why: tab A parking proves the machinery ran past the delay in this app - // instance, so the tab C assertion below is not vacuously green. - await waitForTabParked(orcaPage, tabAId) + // instance, so the tab C assertion below is not vacuously green. A decoy + // takes the #8262 last-active exemption (tab C is excluded, not a candidate) + // so tab A is the one that cold-parks. + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabAId, { + parkDelayMs: PARKING_DELAY_MS + }) await orcaPage.waitForTimeout(PARKING_DELAY_MS * 3) // Premise guard: nothing consumed the pending startup while hidden. @@ -483,8 +483,11 @@ test.describe('Terminal hidden view parking', () => { .toContain(marker) // Tab B stays visible whenever tab A is parked; toggling the active tab - // between them is the deterministic hide/reveal driver. + // between them is the deterministic hide/reveal driver. The decoy tab + // absorbs the #8262 last-active exemption each cycle (hidden after B) so + // tab A — not the just-hidden view — is the one that cold-parks. const tabBId = await createActiveTerminalTab(orcaPage, worktreeId) + const decoyTabId = await createActiveTerminalTab(orcaPage, worktreeId) // One park/reveal cycle to run the frame through the snapshot restore for a // baseline. Why not compare against the visible-before-park content: an @@ -495,7 +498,9 @@ test.describe('Terminal hidden view parking', () => { // both sides pass through identical machinery, so any later diff is drift. const runOneParkRevealCycle = async (cycle: number): Promise => { await activateTerminalTab(orcaPage, tabBId) - await waitForTabParked(orcaPage, tabAId) + // Hide tab B behind the decoy so B (not A) holds the #8262 exemption. + await activateTerminalTab(orcaPage, decoyTabId) + await waitForTabParked(orcaPage, tabAId, { parkDelayMs: PARKING_DELAY_MS }) await activateTerminalTab(orcaPage, tabAId) await waitForActiveTerminalManager(orcaPage, 30_000) const revealed = await waitForPaneIdentitySnapshot(orcaPage, 1) diff --git a/tests/e2e/terminal-history-size-typing-latency.spec.ts b/tests/e2e/terminal-history-size-typing-latency.spec.ts index 3f0f0d0a1f1..559bf87494e 100644 --- a/tests/e2e/terminal-history-size-typing-latency.spec.ts +++ b/tests/e2e/terminal-history-size-typing-latency.spec.ts @@ -163,6 +163,12 @@ function median(values: number[]): number { return sorted[Math.floor(sorted.length / 2)] ?? 0 } +function percentile(values: number[], fraction: number): number { + const sorted = [...values].sort((a, b) => a - b) + const index = Math.min(sorted.length - 1, Math.max(0, Math.ceil(fraction * sorted.length) - 1)) + return sorted[index] ?? 0 +} + type PhaseLatency = { label: string bufferRows: number @@ -281,9 +287,12 @@ test.describe('Terminal typing latency vs scrollback history size', () => { phase.medianMs, `${phase.label}: median latency regressed with history size` ).toBeLessThan(MAX_MEDIAN_KEY_LATENCY_MS) + // Why: the fill keeps the session dirty so a daemon checkpoint + // serialization can land inside a single sample; guard the p90 so one + // environment-dominated spike is not a failure while median stays strict. expect( - phase.worstMs, - `${phase.label}: worst latency regressed with history size` + percentile(phase.samples, 0.9), + `${phase.label}: sustained (p90) latency regressed with history size` ).toBeLessThan(MAX_WORST_KEY_LATENCY_MS) } } finally { diff --git a/tests/e2e/terminal-pane-close-layout-consistency.spec.ts b/tests/e2e/terminal-pane-close-layout-consistency.spec.ts index db837c317cb..a1cafa94b7c 100644 --- a/tests/e2e/terminal-pane-close-layout-consistency.spec.ts +++ b/tests/e2e/terminal-pane-close-layout-consistency.spec.ts @@ -12,6 +12,7 @@ import { waitForActiveTerminalManager, waitForPaneIdentitySnapshot } from './helpers/terminal' +import { parkHiddenTabBehindDecoy } from './helpers/terminal-hidden-parking' /** * Repro hunt for the "ghost blank pane" field incident: a split pane whose PTY @@ -210,17 +211,6 @@ async function activateTerminalTab(page: Page, tabId: string): Promise { .toBe(tabId) } -// Why: TerminalPane unmount deletes its __paneManagers entry — that absence is -// the observable "parked" signal (same detection as the parking spec). -async function waitForTabParked(page: Page, tabId: string): Promise { - await expect - .poll(async () => page.evaluate((id) => window.__paneManagers?.get(id) !== undefined, tabId), { - timeout: Math.max(20_000, PARKING_DELAY_MS * 10), - message: `terminal tab ${tabId} did not park (pane manager still mounted)` - }) - .toBe(false) -} - async function waitForTabRemounted(page: Page, tabId: string): Promise { await expect .poll(async () => page.evaluate((id) => window.__paneManagers?.get(id) !== undefined, tabId), { @@ -298,7 +288,9 @@ test.describe('terminal pane close vs hidden/park lifecycle keeps layout consist const { worktreeId, tabId } = await setUpSplitTab(orcaPage) await createActiveTerminalTab(orcaPage, worktreeId) await closeLastPaneOnTab(orcaPage, tabId) - await waitForTabParked(orcaPage, tabId) + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { + parkDelayMs: PARKING_DELAY_MS + }) await activateTerminalTab(orcaPage, tabId) await waitForTabRemounted(orcaPage, tabId) await expectLayoutConsistent(orcaPage, tabId, 1, 'close-while-hidden-mounted') @@ -307,7 +299,9 @@ test.describe('terminal pane close vs hidden/park lifecycle keeps layout consist test('close immediately after reveal remount, before panes settle', async ({ orcaPage }) => { const { worktreeId, tabId } = await setUpSplitTab(orcaPage) await createActiveTerminalTab(orcaPage, worktreeId) - await waitForTabParked(orcaPage, tabId) + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { + parkDelayMs: PARKING_DELAY_MS + }) await activateTerminalTab(orcaPage, tabId) await waitForTabRemounted(orcaPage, tabId) // Close as soon as the manager exists — panes may still be attaching. @@ -326,7 +320,9 @@ test.describe('terminal pane close vs hidden/park lifecycle keeps layout consist await closeLastPaneOnTab(orcaPage, tabId) await expectLayoutConsistent(orcaPage, tabId, 1, 'pre-park close') await createActiveTerminalTab(orcaPage, worktreeId) - await waitForTabParked(orcaPage, tabId) + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { + parkDelayMs: PARKING_DELAY_MS + }) await activateTerminalTab(orcaPage, tabId) await waitForTabRemounted(orcaPage, tabId) await expectLayoutConsistent(orcaPage, tabId, 1, 'post-park-reveal') @@ -347,7 +343,9 @@ test.describe('terminal pane close vs hidden/park lifecycle keeps layout consist test('split pane shell exits while the tab is parked', async ({ orcaPage }) => { const { worktreeId, tabId, splitPtyId } = await setUpSplitTab(orcaPage) await createActiveTerminalTab(orcaPage, worktreeId) - await waitForTabParked(orcaPage, tabId) + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { + parkDelayMs: PARKING_DELAY_MS + }) await sendToTerminal(orcaPage, splitPtyId, 'exit\r') await orcaPage.waitForTimeout(PARKING_DELAY_MS) await activateTerminalTab(orcaPage, tabId) diff --git a/tests/e2e/terminal-shortcuts.spec.ts b/tests/e2e/terminal-shortcuts.spec.ts index 17d837aa1ce..9c3fea54d02 100644 --- a/tests/e2e/terminal-shortcuts.spec.ts +++ b/tests/e2e/terminal-shortcuts.spec.ts @@ -533,9 +533,10 @@ test.describe('Terminal Shortcuts', () => { await expect.poll(() => getKittyKeyboardFlags(orcaPage)).toBe(1) await pressAndExpectWrite(orcaPage, electronApp, 'Shift+Enter', '\x1b[13;2u') - // The shell is only standing in for a KKP-aware TUI and does not consume - // the CSI-u input above, so cancel that synthetic line before its reset. - await execInTerminal(orcaPage, ptyId, "\x03printf '\\033[ getKittyKeyboardFlags(orcaPage)).toBe(0) await pressAndExpectWrite(orcaPage, electronApp, 'Shift+Enter', '\x1b\r') }) From 52388c1ab7eccdc4be1e336a680167a881a73dab Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:45:33 -0700 Subject: [PATCH 2/3] Fix worker done pane identity (#8473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(orchestration): bind lifecycle sends to dispatched pane * fix(orchestration): bind injected worker messaging * Fix worker_done pane identity gating and rename SSH linear CLI files - Lifecycle reconciliation now returns an explicit `rejected` action (instead of `ignored`) for persisted sender_not_assignee markers, and only treats a payload's `_orcaLifecycleRejection` as trusted state when it exactly matches that reserved shape, preventing caller- supplied markers from spoofing a rejected send as success. - SSH legacy fallback CLI now fails closed with `no_active_sender_terminal` when a worker_done/heartbeat send has no resolvable sender identity, rejects mixed raw/structured payload flags, and reconstructs structured lifecycle payload fields (task/dispatch id, files-modified, report-path, phase) instead of dropping them — extracted into ssh-remote-orchestration-send.ts. - Renamed ssh-remote-linear-argument-error.ts to ssh-remote-cli-argument-error.ts since RemoteCliArgumentError is now shared beyond the Linear CLI. * Add re-read logging for already-converted lifecycle rejections Send-path reconcile converts worker_done/heartbeat rejections with a no-op logger, so the coordinator's later re-read is the only chance to surface the rejection message. Log it there instead of dropping it. --- .../orchestration-lifecycle-rejection.test.ts | 45 ++++ src/cli/handlers/orchestration.test.ts | 20 ++ src/cli/handlers/orchestration.ts | 32 ++- src/cli/specs/orchestration.ts | 2 +- .../__snapshots__/preamble.test.ts.snap | 10 +- .../runtime/orchestration/coordinator.test.ts | 19 +- src/main/runtime/orchestration/coordinator.ts | 1 + src/main/runtime/orchestration/db.ts | 37 +++ .../runtime/orchestration/formatter.test.ts | 4 +- src/main/runtime/orchestration/formatter.ts | 6 +- .../lifecycle-reconciliation.test.ts | 253 ++++++++++++++++-- .../orchestration/lifecycle-reconciliation.ts | 119 +++++--- .../runtime/orchestration/preamble.test.ts | 24 +- src/main/runtime/orchestration/preamble.ts | 11 +- .../runtime/rpc/methods/orchestration.test.ts | 136 +++++++++- src/main/runtime/rpc/methods/orchestration.ts | 32 ++- ...or.ts => ssh-remote-cli-argument-error.ts} | 0 src/main/ssh/ssh-remote-linear-cli.ts | 4 +- src/main/ssh/ssh-remote-linear-read-cli.ts | 2 +- src/main/ssh/ssh-remote-orca-cli.test.ts | 189 ++++++++++++- src/main/ssh/ssh-remote-orca-cli.ts | 32 ++- .../ssh/ssh-remote-orchestration-send.test.ts | 70 +++++ src/main/ssh/ssh-remote-orchestration-send.ts | 83 ++++++ 23 files changed, 1022 insertions(+), 109 deletions(-) create mode 100644 src/cli/handlers/orchestration-lifecycle-rejection.test.ts rename src/main/ssh/{ssh-remote-linear-argument-error.ts => ssh-remote-cli-argument-error.ts} (100%) create mode 100644 src/main/ssh/ssh-remote-orchestration-send.test.ts create mode 100644 src/main/ssh/ssh-remote-orchestration-send.ts diff --git a/src/cli/handlers/orchestration-lifecycle-rejection.test.ts b/src/cli/handlers/orchestration-lifecycle-rejection.test.ts new file mode 100644 index 00000000000..269dfd7a47a --- /dev/null +++ b/src/cli/handlers/orchestration-lifecycle-rejection.test.ts @@ -0,0 +1,45 @@ +import { afterEach, expect, it, vi } from 'vitest' + +const callMock = vi.fn() +const originalExitCode = process.exitCode +vi.mock('../format', () => ({ printResult: vi.fn() })) +vi.mock('../selectors', () => ({ getTerminalHandle: vi.fn() })) + +import { printResult } from '../format' +import { ORCHESTRATION_HANDLERS } from './orchestration' + +afterEach(() => { + process.exitCode = originalExitCode +}) + +it('prints a lifecycle rejection and exits unsuccessfully', async () => { + const response = { + result: { + message: { id: 'msg_rejected' }, + lifecycle: { + action: 'rejected' as const, + code: 'sender_not_assignee', + reason: 'dispatch ctx_1 expected the assigned pane' + } + } + } + callMock.mockResolvedValueOnce(response) + + await ORCHESTRATION_HANDLERS['orchestration send']({ + flags: new Map([ + ['from', 'term_foreign'], + ['to', 'term_coord'], + ['subject', 'done'], + ['type', 'worker_done'] + ]), + client: { call: callMock }, + cwd: '/tmp/repo', + json: false + } as never) + + expect(process.exitCode).toBe(1) + const formatter = vi.mocked(printResult).mock.calls[0]?.[2] + expect(formatter?.(response.result)).toBe( + 'Rejected msg_rejected: dispatch ctx_1 expected the assigned pane' + ) +}) diff --git a/src/cli/handlers/orchestration.test.ts b/src/cli/handlers/orchestration.test.ts index 735a72a53cf..2a8f6d44dfb 100644 --- a/src/cli/handlers/orchestration.test.ts +++ b/src/cli/handlers/orchestration.test.ts @@ -317,6 +317,26 @@ describe('orchestration send structured payload flags', () => { }) expect(callMock).not.toHaveBeenCalled() }) + + it.each(['worker_done', 'heartbeat'] as const)( + 'does not resolve an identity-less %s sender from the active terminal', + async (type) => { + getTerminalHandleMock.mockResolvedValue('term_active_coordinator') + + await expect( + invokeSend( + new Map([ + ['to', 'term_coord'], + ['subject', 'update'], + ['type', type] + ]) + ) + ).rejects.toMatchObject({ code: 'no_active_sender_terminal' }) + + expect(getTerminalHandleMock).not.toHaveBeenCalled() + expect(callMock).not.toHaveBeenCalled() + } + ) }) describe('orchestration dispatch coordinator handle', () => { diff --git a/src/cli/handlers/orchestration.ts b/src/cli/handlers/orchestration.ts index d215fcb2e2e..fc285c56440 100644 --- a/src/cli/handlers/orchestration.ts +++ b/src/cli/handlers/orchestration.ts @@ -80,6 +80,16 @@ type MessageSummary = { read?: number } +type LifecycleSendRejection = { + action: 'rejected' + code: string + reason: string +} + +type OrchestrationSendResult = + | { message: { id: string }; lifecycle?: LifecycleSendRejection } + | { messages: { id: string }[]; recipients: number } + function getOptionalStructuredMessagePayload( flags: Map ): string | undefined { @@ -347,6 +357,16 @@ export const ORCHESTRATION_HANDLERS: Record = { const type = getOptionalStringFlag(flags, 'type') rejectLifecycleGroupRecipient(type, to) + if ( + (type === 'worker_done' || type === 'heartbeat') && + !getOptionalStringFlag(flags, 'from') && + !process.env.ORCA_TERMINAL_HANDLE + ) { + // Why: focus is not lifecycle authority; injected dispatches carry an + // explicit worker handle so an identity-less subprocess must fail closed. + throwNoActiveSenderTerminal() + } + // Why: lifecycle senders keep ORCA_TERMINAL_HANDLE verbatim — no liveness // probe (terminal.show throws runtime_unavailable in the exact mid-restart // window worker_done must survive) and no pane remint (pre-payload- @@ -354,9 +374,7 @@ export const ORCHESTRATION_HANDLERS: Record = { // and coordinator replies route to the sender row while the worker's own // `check` reads its env-handle inbox). const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from') - const result = await client.call< - { message: { id: string } } | { messages: { id: string }[]; recipients: number } - >('orchestration.send', { + const result = await client.call('orchestration.send', { from, to, subject: getRequiredStringFlag(flags, 'subject'), @@ -370,8 +388,16 @@ export const ORCHESTRATION_HANDLERS: Record = { senderPaneKey: process.env.ORCA_PANE_KEY || undefined, devMode: isDevCliInvocation() }) + if ('message' in result.result && result.result.lifecycle?.action === 'rejected') { + // Why: a persisted diagnostic is not a successful lifecycle signal; + // the non-zero status prevents workers from treating it as completion. + process.exitCode = 1 + } printResult(result, json, (r) => { if ('message' in r) { + if (r.lifecycle?.action === 'rejected') { + return `Rejected ${r.message.id}: ${r.lifecycle.reason}` + } return `Sent ${r.message.id}` } return `Sent ${r.messages.length} messages to ${r.recipients} recipients` diff --git a/src/cli/specs/orchestration.ts b/src/cli/specs/orchestration.ts index 3eb022dd7ed..bb63f261d00 100644 --- a/src/cli/specs/orchestration.ts +++ b/src/cli/specs/orchestration.ts @@ -26,7 +26,7 @@ export const ORCHESTRATION_COMMAND_SPECS: CommandSpec[] = [ notes: [ 'On Windows PowerShell, quote group addresses such as --to "@all" or --to "@worktree:".', 'worker_done and heartbeat must target a concrete coordinator terminal handle; use status for broadcast updates.', - 'A worker_done with the active task/dispatch IDs completes that task when sent from the dispatched pane (or when pane identity is unavailable); on older runtimes the sender must match the dispatch assignee handle, so avoid overriding --from.', + 'A worker_done with the active task/dispatch IDs completes that task only from the dispatched pane. When stable pane identity is unavailable, the sender handle must exactly match the dispatch assignee; injected preambles include the correct --from value.', 'Prefer --task-id/--dispatch-id/etc. over raw --payload JSON in worker commands; PowerShell strips JSON quotes easily.' ] }, diff --git a/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap b/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap index 2f8c48d1a6c..c55426c4216 100644 --- a/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap +++ b/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap @@ -22,7 +22,7 @@ Slack, GitHub comments, or any other channel to reach a human during the run. # with subject like "Failed: " — never silently exit. # Include BOTH taskId and dispatchId in the payload so a late completion # from a failed retry cannot complete the current dispatch. - orca orchestration send --to term_COORD \\ + orca orchestration send --to term_COORD --from term_WORKER \\ --type worker_done --subject "" \\ --body "<3-sentence summary: what you did, what you found, what's left>" \\ --task-id task_SNAP --dispatch-id ctx_SNAP \\ @@ -39,7 +39,7 @@ Slack, GitHub comments, or any other channel to reach a human during the run. # attributes the heartbeat to the specific dispatch context, not just # the task, so a straggler heartbeat from a previously-failed dispatch # cannot mask a hung retry. - orca orchestration send --to term_COORD \\ + orca orchestration send --to term_COORD --from term_WORKER \\ --type heartbeat --subject "alive" \\ --task-id task_SNAP --dispatch-id ctx_SNAP \\ --phase "" @@ -56,20 +56,20 @@ Slack, GitHub comments, or any other channel to reach a human during the run. # blocks on \`check --wait\` until the coordinator replies, then prints the # reply body. Use it anywhere you would otherwise have reached for # AskUserQuestion. - orca orchestration ask --to term_COORD \\ + orca orchestration ask --to term_COORD --from term_WORKER \\ --question "" \\ --options "" \\ --timeout-ms 600000 # Escalate a blocker or failure (pre-completion, when you need the # coordinator to do something before you can continue): - orca orchestration send --to term_COORD \\ + orca orchestration send --to term_COORD --from term_WORKER \\ --type escalation --subject "Blocked: " \\ --body "
" \\ --task-id task_SNAP # Check for messages from the coordinator: - orca orchestration check + orca orchestration check --terminal term_WORKER === AFTER YOU SEND worker_done === diff --git a/src/main/runtime/orchestration/coordinator.test.ts b/src/main/runtime/orchestration/coordinator.test.ts index 1e94018c931..8563c3a1e87 100644 --- a/src/main/runtime/orchestration/coordinator.test.ts +++ b/src/main/runtime/orchestration/coordinator.test.ts @@ -76,6 +76,7 @@ function insertWorkerDone( from?: string dispatchId?: string filesModified?: string[] + senderPaneKey?: string } ): void { const dispatch = db.getDispatchContext(params.taskId) @@ -83,8 +84,9 @@ function insertWorkerDone( if (!dispatchId) { throw new Error(`No dispatch for task ${params.taskId}`) } + const from = params.from ?? dispatch?.assignee_handle ?? 'term_unknown' db.insertMessage({ - from: params.from ?? dispatch?.assignee_handle ?? 'term_unknown', + from, to: params.to ?? 'coord', subject: 'Done', type: 'worker_done', @@ -92,7 +94,10 @@ function insertWorkerDone( taskId: params.taskId, dispatchId, ...(params.filesModified ? { filesModified: params.filesModified } : {}) - }) + }), + senderPaneKey: + params.senderPaneKey ?? + (from === dispatch?.assignee_handle ? (dispatch.assignee_pane_key ?? undefined) : undefined) }) } @@ -595,20 +600,22 @@ describe('Coordinator', () => { expect(db.getDispatchContextById(activeCtx.id)?.status).toBe('completed') }) - it('accepts worker_done payload provenance after an assignee handle changes', async () => { + it('accepts worker_done pane provenance after an assignee handle changes', async () => { db = new OrchestrationDb(':memory:') const runtime = createMockRuntime() const logs: string[] = [] const task = db.createTask({ spec: 'owned work' }) - const ctx = db.createDispatchContext(task.id, 'term_owner') + const leafId = '11111111-1111-4111-8111-111111111111' + const ctx = db.createDispatchContext(task.id, 'term_owner', `tab_before:${leafId}`) db.insertMessage({ from: 'term_reminted', to: 'coord', subject: 'Done after restart', type: 'worker_done', - payload: JSON.stringify({ taskId: task.id, dispatchId: ctx.id }) + payload: JSON.stringify({ taskId: task.id, dispatchId: ctx.id }), + senderPaneKey: `tab_after:${leafId}` }) const coordinator = new Coordinator(db, runtime, { @@ -622,7 +629,7 @@ describe('Coordinator', () => { expect(result.status).toBe('completed') expect(db.getTask(task.id)?.status).toBe('completed') expect(db.getDispatchContextById(ctx.id)?.status).toBe('completed') - expect(logs.some((m) => m.includes('accepting payload provenance'))).toBe(true) + expect(logs.some((m) => m.includes('Task') && m.includes('completed'))).toBe(true) }) it('can be stopped', async () => { diff --git a/src/main/runtime/orchestration/coordinator.ts b/src/main/runtime/orchestration/coordinator.ts index e09e48942d3..73307eddd35 100644 --- a/src/main/runtime/orchestration/coordinator.ts +++ b/src/main/runtime/orchestration/coordinator.ts @@ -484,6 +484,7 @@ export class Coordinator { // part of its instructions). taskSpec: strippedSpec, coordinatorHandle: this.opts.coordinatorHandle, + workerHandle: targetHandle, devMode: process.env.ORCA_USER_DATA_PATH?.includes('orca-dev'), // Why (§3.2): drift section fires only when behind > 0. The preamble // builder gates on this itself; passing the object unconditionally lets diff --git a/src/main/runtime/orchestration/db.ts b/src/main/runtime/orchestration/db.ts index ddbdb54a0f6..80a842da530 100644 --- a/src/main/runtime/orchestration/db.ts +++ b/src/main/runtime/orchestration/db.ts @@ -46,6 +46,22 @@ function generateId(prefix: string): string { return `${prefix}_${randomBytes(6).toString('hex')}` } +function addLifecycleRejectionMarker(payload: string | null, reason: string): string { + let parsed: Record = {} + try { + const value: unknown = payload ? JSON.parse(payload) : {} + if (value && typeof value === 'object' && !Array.isArray(value)) { + parsed = value as Record + } + } catch { + // Authority reconciliation only reaches this path with object payloads. + } + return JSON.stringify({ + ...parsed, + _orcaLifecycleRejection: { code: 'sender_not_assignee', reason } + }) +} + // Why: v1 → v2 added `'heartbeat'` to messages.type CHECK + `last_heartbeat_at` // column (preamble-hardening PR). v2 → v3 adds `delivered_at` column so // push-on-idle can distinguish queued-but-undelivered from user-acknowledged @@ -354,6 +370,27 @@ export class OrchestrationDb { .all(toHandle) as MessageRow[] } + convertLifecycleMessageToRejection(messageId: string, reason: string): MessageRow | undefined { + const message = this.getMessageById(messageId) + if (!message || (message.type !== 'worker_done' && message.type !== 'heartbeat')) { + return message + } + + const originalBody = message.body ? `\n\nOriginal body:\n${message.body}` : '' + const body = `Orca rejected this ${message.type}: ${reason}${originalBody}` + const payload = addLifecycleRejectionMarker(message.payload, reason) + // Why: rejected lifecycle signals must remain auditable without reaching + // later read paths as actionable completion or liveness events. + this.db + .prepare( + `UPDATE messages + SET priority = 'high', subject = ?, body = ?, payload = ? + WHERE id = ?` + ) + .run(`Rejected ${message.type}: ${message.subject}`, body, payload, messageId) + return this.getMessageById(messageId) + } + // Why: push-on-idle delivery must not replay messages that were already // injected into the PTY. `read` flips only when a check-caller consumes a // message, so delivered-but-unread rows would otherwise be re-injected on diff --git a/src/main/runtime/orchestration/formatter.test.ts b/src/main/runtime/orchestration/formatter.test.ts index f932aec359c..ea8ae94bc19 100644 --- a/src/main/runtime/orchestration/formatter.test.ts +++ b/src/main/runtime/orchestration/formatter.test.ts @@ -71,7 +71,9 @@ describe('formatMessageBanner', () => { it('includes reply hint with message ID', () => { const banner = formatMessageBanner(makeMessage({ id: 'msg_xyz789' })) - expect(banner).toContain('[Reply: orca orchestration reply --id msg_xyz789 --body "..."]') + expect(banner).toContain( + '[Reply: orca orchestration reply --id msg_xyz789 --from term_coord --body "..."]' + ) }) it('ends with a separator line', () => { diff --git a/src/main/runtime/orchestration/formatter.ts b/src/main/runtime/orchestration/formatter.ts index f5526553d3b..a86f3c61edf 100644 --- a/src/main/runtime/orchestration/formatter.ts +++ b/src/main/runtime/orchestration/formatter.ts @@ -25,7 +25,11 @@ export function formatMessageBanner(msg: MessageRow): string { lines.push(`[Payload: ${msg.payload}]`) } - lines.push(`[Reply: orca orchestration reply --id ${msg.id} --body "..."]`) + // Why: injected reply commands must retain the receiving pane's identity + // even when an older shell lacks Orca's terminal environment variables. + lines.push( + `[Reply: orca orchestration reply --id ${msg.id} --from ${msg.to_handle} --body "..."]` + ) lines.push(SEPARATOR) return lines.join('\n') diff --git a/src/main/runtime/orchestration/lifecycle-reconciliation.test.ts b/src/main/runtime/orchestration/lifecycle-reconciliation.test.ts index 3ba68a67910..8fed046d421 100644 --- a/src/main/runtime/orchestration/lifecycle-reconciliation.test.ts +++ b/src/main/runtime/orchestration/lifecycle-reconciliation.test.ts @@ -7,7 +7,7 @@ describe('lifecycle reconciliation', () => { afterEach(() => db?.close()) - it('completes an active dispatch from payload IDs after its terminal handle is reminted', () => { + it('rejects handle churn when neither side has stable pane identity', () => { db = new OrchestrationDb(':memory:') const task = db.createTask({ spec: 'work' }) const dispatch = db.createDispatchContext(task.id, 'term_before_restart') @@ -20,14 +20,13 @@ describe('lifecycle reconciliation', () => { payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }) }) - expect(reconcileLifecycleMessage(db, message, (line) => logs.push(line))).toEqual({ - action: 'completed', - taskId: task.id, - dispatchId: dispatch.id + expect(reconcileLifecycleMessage(db, message, (line) => logs.push(line))).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' }) - expect(db.getTask(task.id)?.status).toBe('completed') - expect(db.getDispatchContextById(dispatch.id)?.status).toBe('completed') - expect(logs.some((line) => line.includes('accepting payload provenance'))).toBe(true) + expect(db.getTask(task.id)?.status).toBe('dispatched') + expect(db.getDispatchContextById(dispatch.id)?.status).toBe('dispatched') + expect(logs.some((line) => line.includes('expected handle'))).toBe(true) }) // Real leaf UUIDs: pane keys are `${tabId}:${leafUuid}` and only the leaf @@ -71,7 +70,7 @@ describe('lifecycle reconciliation', () => { expect(db.getTask(task.id)?.status).toBe('completed') }) - it('completes worker_done when a pane key is unparseable (legacy format)', () => { + it('rejects mismatched opaque pane keys instead of treating them as legacy', () => { db = new OrchestrationDb(':memory:') const task = db.createTask({ spec: 'work' }) const dispatch = db.createDispatchContext(task.id, 'term_owner', `tab_w:${LEAF_A}`) @@ -84,10 +83,194 @@ describe('lifecycle reconciliation', () => { senderPaneKey: 'tab_w:42' }) - expect(reconcileLifecycleMessage(db, message).action).toBe('completed') + expect(reconcileLifecycleMessage(db, message).action).toBe('rejected') + expect(db.getTask(task.id)?.status).toBe('dispatched') }) - it('ignores worker_done sent from a different pane', () => { + it('rejects worker_done from a foreign pane that claims the assignee handle', () => { + db = new OrchestrationDb(':memory:') + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_owner', `tab_w1:${LEAF_A}`) + const message = db.insertMessage({ + from: 'term_owner', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }), + senderPaneKey: `tab_w2:${LEAF_B}` + }) + + expect(reconcileLifecycleMessage(db, message)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee', + reason: expect.stringContaining('expected handle term_owner') + }) + expect(db.getTask(task.id)?.status).toBe('dispatched') + expect(db.getMessageById(message.id)).toMatchObject({ + type: 'worker_done', + priority: 'high', + subject: 'Rejected worker_done: Done', + body: expect.stringContaining('Orca rejected this worker_done') + }) + const persisted = db.getMessageById(message.id) + expect(JSON.parse(persisted?.payload ?? '{}')).toMatchObject({ + taskId: task.id, + dispatchId: dispatch.id, + _orcaLifecycleRejection: { code: 'sender_not_assignee' } + }) + const rereadLogs: string[] = [] + expect( + persisted && reconcileLifecycleMessage(db, persisted, (line) => rereadLogs.push(line)) + ).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) + // Why: the send-path conversion logs nothing (no-op logger), so the + // coordinator's re-read must still surface the rejection to its log stream. + expect(rereadLogs.some((line) => line.includes('worker_done rejected'))).toBe(true) + }) + + it('does not let a caller-supplied rejection marker turn completion into success', () => { + db = new OrchestrationDb(':memory:') + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_worker', `tab_w:${LEAF_A}`) + const message = db.insertMessage({ + from: 'term_worker', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ + taskId: task.id, + dispatchId: dispatch.id, + _orcaLifecycleRejection: { + code: 'sender_not_assignee', + reason: 'caller supplied' + } + }), + senderPaneKey: `tab_w:${LEAF_A}` + }) + + expect(reconcileLifecycleMessage(db, message)).toEqual({ + action: 'rejected', + code: 'sender_not_assignee', + reason: 'caller supplied' + }) + expect(db.getTask(task.id)?.status).toBe('dispatched') + }) + + it('rejects a coordinator completion for a pane-bound dispatch', () => { + db = new OrchestrationDb(':memory:') + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_worker', `tab_w:${LEAF_A}`) + const message = db.insertMessage({ + from: 'term_coordinator', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }) + }) + + expect(reconcileLifecycleMessage(db, message)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) + expect(db.getTask(task.id)?.status).toBe('dispatched') + }) + + it('uses exact handle equality only for a legacy dispatch without a pane key', () => { + db = new OrchestrationDb(':memory:') + const acceptedTask = db.createTask({ spec: 'legacy work' }) + const acceptedDispatch = db.createDispatchContext(acceptedTask.id, 'term_legacy') + const accepted = db.insertMessage({ + from: 'term_legacy', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: acceptedTask.id, dispatchId: acceptedDispatch.id }) + }) + expect(reconcileLifecycleMessage(db, accepted).action).toBe('completed') + + const rejectedTask = db.createTask({ spec: 'other legacy work' }) + const rejectedDispatch = db.createDispatchContext(rejectedTask.id, 'term_other_legacy') + const rejected = db.insertMessage({ + from: 'term_foreign', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: rejectedTask.id, dispatchId: rejectedDispatch.id }) + }) + expect(reconcileLifecycleMessage(db, rejected)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) + expect(db.getTask(rejectedTask.id)?.status).toBe('dispatched') + }) + + it('does not release a dependent when a foreign completion wins the arrival race', () => { + db = new OrchestrationDb(':memory:') + const parent = db.createTask({ spec: 'parent' }) + const child = db.createTask({ spec: 'child', deps: [parent.id] }) + const dispatch = db.createDispatchContext(parent.id, 'term_worker', `tab_w:${LEAF_A}`) + const payload = JSON.stringify({ taskId: parent.id, dispatchId: dispatch.id }) + + const foreign = db.insertMessage({ + from: 'term_coordinator', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload, + senderPaneKey: `tab_c:${LEAF_B}` + }) + expect(reconcileLifecycleMessage(db, foreign)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) + expect(db.getTask(child.id)?.status).toBe('pending') + + const owner = db.insertMessage({ + from: 'term_worker_reminted', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload, + senderPaneKey: `tab_w_after:${LEAF_A}` + }) + expect(reconcileLifecycleMessage(db, owner).action).toBe('completed') + expect(db.getTask(child.id)?.status).toBe('ready') + }) + + it('does not let a foreign replay overwrite an authorized completion', () => { + db = new OrchestrationDb(':memory:') + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_worker', `tab_w:${LEAF_A}`) + const payload = JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }) + const owner = db.insertMessage({ + from: 'term_worker', + to: 'term_coordinator', + subject: 'Done', + type: 'worker_done', + payload, + senderPaneKey: `tab_w:${LEAF_A}` + }) + expect(reconcileLifecycleMessage(db, owner).action).toBe('completed') + const result = db.getTask(task.id)?.result + + const replay = db.insertMessage({ + from: 'term_foreign', + to: 'term_coordinator', + subject: 'Forged replay', + type: 'worker_done', + payload, + senderPaneKey: `tab_f:${LEAF_B}` + }) + expect(reconcileLifecycleMessage(db, replay)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) + expect(db.getTask(task.id)?.result).toBe(result) + }) + + it('surfaces worker_done sent from a different pane as rejected', () => { db = new OrchestrationDb(':memory:') const task = db.createTask({ spec: 'work' }) const dispatch = db.createDispatchContext(task.id, 'term_owner', `tab_w1:${LEAF_A}`) @@ -101,15 +284,16 @@ describe('lifecycle reconciliation', () => { senderPaneKey: `tab_w2:${LEAF_B}` }) - expect(reconcileLifecycleMessage(db, message, (line) => logs.push(line))).toEqual({ - action: 'ignored' + expect(reconcileLifecycleMessage(db, message, (line) => logs.push(line))).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' }) expect(db.getTask(task.id)?.status).toBe('dispatched') expect(db.getDispatchContextById(dispatch.id)?.status).toBe('dispatched') - expect(logs.some((line) => line.includes(`expected pane tab_w1:${LEAF_A}`))).toBe(true) + expect(logs.some((line) => line.includes('worker_done rejected'))).toBe(true) }) - it('ignores a heartbeat sent from a different pane without recording liveness', () => { + it('surfaces a heartbeat sent from a different pane without recording liveness', () => { db = new OrchestrationDb(':memory:') const task = db.createTask({ spec: 'work' }) const dispatch = db.createDispatchContext(task.id, 'term_owner', `tab_w1:${LEAF_A}`) @@ -122,7 +306,44 @@ describe('lifecycle reconciliation', () => { senderPaneKey: `tab_w2:${LEAF_B}` }) - expect(reconcileLifecycleMessage(db, heartbeat)).toEqual({ action: 'ignored' }) + expect(reconcileLifecycleMessage(db, heartbeat)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) + expect(db.getDispatchContextById(dispatch.id)?.last_heartbeat_at).toBeNull() + expect(db.getMessageById(heartbeat.id)).toMatchObject({ + type: 'heartbeat', + subject: 'Rejected heartbeat: alive' + }) + // Why: the coordinator's re-read of the already-converted heartbeat must + // still log the rejection, since the send-path conversion logged nothing. + const persisted = db.getMessageById(heartbeat.id) + const rereadLogs: string[] = [] + expect( + persisted && reconcileLifecycleMessage(db, persisted, (line) => rereadLogs.push(line)) + ).toMatchObject({ + action: 'rejected' + }) + expect(rereadLogs.some((line) => line.includes('Heartbeat rejected'))).toBe(true) + }) + + it('surfaces a foreign heartbeat that claims the assignee handle', () => { + db = new OrchestrationDb(':memory:') + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_owner', `tab_w1:${LEAF_A}`) + const heartbeat = db.insertMessage({ + from: 'term_owner', + to: 'term_coordinator', + subject: 'alive', + type: 'heartbeat', + payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }), + senderPaneKey: `tab_w2:${LEAF_B}` + }) + + expect(reconcileLifecycleMessage(db, heartbeat)).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee' + }) expect(db.getDispatchContextById(dispatch.id)?.last_heartbeat_at).toBeNull() }) diff --git a/src/main/runtime/orchestration/lifecycle-reconciliation.ts b/src/main/runtime/orchestration/lifecycle-reconciliation.ts index d784840a28c..96917babaf6 100644 --- a/src/main/runtime/orchestration/lifecycle-reconciliation.ts +++ b/src/main/runtime/orchestration/lifecycle-reconciliation.ts @@ -2,17 +2,29 @@ import type { OrchestrationDb } from './db' import type { MessageRow } from './types' import { parsePaneKey } from '../../../shared/stable-pane-id' -// Why: the tab half of a pane key changes when a pane is broken out into its -// own tab, so only the leaf UUID is identity. Reject only when both keys -// parse and name different leaves; an unparseable (e.g. legacy numeric) key -// degrades to payload authority rather than stranding a completion. -function isForeignPane(assigneePaneKey: string, senderPaneKey: string): boolean { +// Why: the tab half can change on pane break-out, while opaque legacy keys +// have no safe equivalence beyond exact equality. +function isSamePane(assigneePaneKey: string, senderPaneKey: string): boolean { if (assigneePaneKey === senderPaneKey) { - return false + return true } const assigneeLeaf = parsePaneKey(assigneePaneKey)?.leafId const senderLeaf = parsePaneKey(senderPaneKey)?.leafId - return Boolean(assigneeLeaf && senderLeaf && assigneeLeaf !== senderLeaf) + return Boolean(assigneeLeaf && senderLeaf && assigneeLeaf === senderLeaf) +} + +function hasLifecycleAuthority( + dispatch: { assignee_handle: string | null; assignee_pane_key: string | null }, + msg: MessageRow +): boolean { + if (dispatch.assignee_pane_key) { + return Boolean( + msg.sender_pane_key && isSamePane(dispatch.assignee_pane_key, msg.sender_pane_key) + ) + } + // Why: rows created before pane identity existed can only use the exact + // handle recorded at dispatch; payload knowledge alone is not authority. + return dispatch.assignee_handle === msg.from_handle } export type LifecycleReconciliationResult = @@ -21,9 +33,16 @@ export type LifecycleReconciliationResult = // read); senders must not wake waiters for it, unlike `ignored` rows that // stay unread and still need delivery. | { action: 'suppressed' } + | LifecycleRejectionResult | { action: 'completed'; taskId: string; dispatchId: string } | { action: 'heartbeat_recorded'; dispatchId: string } +export type LifecycleRejectionResult = { + action: 'rejected' + code: 'sender_not_assignee' + reason: string +} + type LogFn = (msg: string) => void const noopLog: LogFn = () => {} @@ -42,6 +61,27 @@ function parseObjectPayload(msg: MessageRow, onInvalidJson: () => void): Record< } } +function getPersistedLifecycleRejection( + payload: Record +): LifecycleRejectionResult | undefined { + const rejection = payload._orcaLifecycleRejection + if ( + !rejection || + typeof rejection !== 'object' || + (rejection as { code?: unknown }).code !== 'sender_not_assignee' || + typeof (rejection as { reason?: unknown }).reason !== 'string' + ) { + return undefined + } + // Why: the marker is reserved persistence state; treating it as a rejection + // also prevents caller-supplied markers from turning lifecycle sends into success. + return { + action: 'rejected', + code: 'sender_not_assignee', + reason: (rejection as { reason: string }).reason + } +} + export function reconcileLifecycleMessage( db: OrchestrationDb, msg: MessageRow, @@ -75,6 +115,13 @@ function reconcileHeartbeatMessage( const payload = parseObjectPayload(msg, () => { onLog(`Heartbeat from ${msg.from_handle} has invalid JSON payload; ignored`) }) + const persistedRejection = getPersistedLifecycleRejection(payload) + if (persistedRejection) { + // Why: the send-path reconcile converts with a no-op logger, so the + // coordinator's re-read is the only chance to surface the rejection. + onLog(`Heartbeat rejected: ${persistedRejection.reason}`) + return persistedRejection + } const dispatchId = payload.dispatchId if (typeof dispatchId !== 'string' || dispatchId.length === 0) { onLog(`Heartbeat from ${msg.from_handle} missing dispatchId; ignored`) @@ -90,17 +137,13 @@ function reconcileHeartbeatMessage( return { action: 'suppressed' } } - if ( - dispatch.assignee_pane_key && - msg.sender_pane_key && - isForeignPane(dispatch.assignee_pane_key, msg.sender_pane_key) - ) { + if (!hasLifecycleAuthority(dispatch, msg)) { // Why: a wrong-pane heartbeat must not refresh liveness — it would mask // a hung assignee behind another agent's timer. - onLog( - `Heartbeat for dispatch ${dispatchId} came from pane ${msg.sender_pane_key}, expected pane ${dispatch.assignee_pane_key}; ignored` - ) - return { action: 'ignored' } + const reason = buildLifecycleAuthorityRejectionReason(dispatchId, dispatch, msg) + onLog(`Heartbeat rejected: ${reason}`) + db.convertLifecycleMessageToRejection(msg.id, reason) + return { action: 'rejected', code: 'sender_not_assignee', reason } } // Why: dispatchId-specific writes let the DB ignore late heartbeats for @@ -119,6 +162,13 @@ function reconcileWorkerDoneMessage( const payload = parseObjectPayload(msg, () => { onLog(`Warning: invalid payload in worker_done from ${msg.from_handle}`) }) + const persistedRejection = getPersistedLifecycleRejection(payload) + if (persistedRejection) { + // Why: the send-path reconcile converts with a no-op logger, so the + // coordinator's re-read is the only chance to surface the rejection. + onLog(`Warning: worker_done rejected: ${persistedRejection.reason}`) + return persistedRejection + } const taskId = payload.taskId if (typeof taskId !== 'string' || taskId.length === 0) { @@ -151,26 +201,11 @@ function reconcileWorkerDoneMessage( ) return { action: 'ignored' } } - if (dispatch.assignee_handle !== msg.from_handle) { - // Why: pane leaves are the remint-stable identity behind handles. When - // both sides carry one, a foreign leaf is a different pane completing - // someone else's task — reject it; the same leaf is the same pane after - // a handle remint or tab break-out. Without pane data (older CLI, - // sessions without ORCA_PANE_KEY) payload IDs stay the completion - // authority. - if ( - dispatch.assignee_pane_key && - msg.sender_pane_key && - isForeignPane(dispatch.assignee_pane_key, msg.sender_pane_key) - ) { - onLog( - `Warning: worker_done for dispatch ${dispatchId} came from pane ${msg.sender_pane_key}, expected pane ${dispatch.assignee_pane_key}; ignored` - ) - return { action: 'ignored' } - } - onLog( - `Warning: worker_done for dispatch ${dispatchId} came from ${msg.from_handle}, expected ${dispatch.assignee_handle ?? ''}; accepting payload provenance` - ) + if (!hasLifecycleAuthority(dispatch, msg)) { + const reason = buildLifecycleAuthorityRejectionReason(dispatchId, dispatch, msg) + onLog(`Warning: worker_done rejected: ${reason}`) + db.convertLifecycleMessageToRejection(msg.id, reason) + return { action: 'rejected', code: 'sender_not_assignee', reason } } // Why: `orchestration.send` can release the DB lock before waking the // coordinator; the later coordinator read still needs to observe completion. @@ -204,6 +239,18 @@ function reconcileWorkerDoneMessage( return { action: 'completed', taskId, dispatchId } } +function buildLifecycleAuthorityRejectionReason( + dispatchId: string, + dispatch: { assignee_handle: string | null; assignee_pane_key: string | null }, + msg: MessageRow +): string { + return ( + `dispatch ${dispatchId} expected handle ${dispatch.assignee_handle ?? ''}, ` + + `pane ${dispatch.assignee_pane_key ?? ''}; received handle ${msg.from_handle}, ` + + `pane ${msg.sender_pane_key ?? ''}` + ) +} + function suppressEarlierHeartbeats( db: OrchestrationDb, workerDone: MessageRow, diff --git a/src/main/runtime/orchestration/preamble.test.ts b/src/main/runtime/orchestration/preamble.test.ts index 23fac4843ab..198437e4cd7 100644 --- a/src/main/runtime/orchestration/preamble.test.ts +++ b/src/main/runtime/orchestration/preamble.test.ts @@ -8,6 +8,7 @@ function baseParams(overrides: Partial[ dispatchId: 'ctx_def456', taskSpec: 'Implement the login form', coordinatorHandle: 'term_coord', + workerHandle: 'term_worker', ...overrides } } @@ -46,6 +47,7 @@ describe('buildDispatchPreamble', () => { expect(result).toContain('--dispatch-id ctx_def456') expect(result).toContain('--files-modified "path/a,path/b"') expect(result).toContain('--report-path ""') + expect(result).toMatch(/orchestration send --to term_coord --from term_worker/) }) it( @@ -83,11 +85,12 @@ describe('buildDispatchPreamble', () => { expect(result).toContain('--task-id task_abc123') expect(result).toContain('--dispatch-id ctx_def456') expect(result).toContain('--phase ""') + expect(result).toMatch(/orchestration send --to term_coord --from term_worker/) }) it('includes ask block with BEHAVIOR RULE #1 forbidding AskUserQuestion', () => { const result = buildDispatchPreamble(baseParams()) - expect(result).toContain('orchestration ask') + expect(result).toMatch(/orchestration ask --to term_coord --from term_worker/) expect(result).toContain('--question') expect(result).toContain('--timeout-ms 600000') // Why: the exact phrase is asserted so the rule can't be trimmed away by @@ -103,6 +106,16 @@ describe('buildDispatchPreamble', () => { expect(occurrences).toBe(3) }) + it('binds every injected worker command to the dispatched terminal', () => { + const result = buildDispatchPreamble(baseParams()) + + expect(result).toMatch(/orchestration ask --to term_coord --from term_worker/) + expect(result).toMatch( + /orchestration send --to term_coord --from term_worker \\\n --type escalation/ + ) + expect(result).toContain('orchestration check --terminal term_worker') + }) + it('tells prompt-returning workers to idle without post-done polling', () => { const result = buildDispatchPreamble(baseParams()) const section = afterWorkerDoneSection(result) @@ -168,6 +181,7 @@ describe('buildDispatchPreamble', () => { dispatchId: 'ctx_x', taskSpec: 'do stuff', coordinatorHandle: 'term_c', + workerHandle: 'term_w', baseDrift: { base: 'origin/main', behind: 7, @@ -190,6 +204,7 @@ describe('buildDispatchPreamble', () => { dispatchId: 'ctx_x', taskSpec: 'do stuff', coordinatorHandle: 'term_c', + workerHandle: 'term_w', baseDrift: { base: 'origin/main', behind: 0, @@ -206,7 +221,8 @@ describe('buildDispatchPreamble', () => { taskId: 'task_x', dispatchId: 'ctx_x', taskSpec: 'do stuff', - coordinatorHandle: 'term_c' + coordinatorHandle: 'term_c', + workerHandle: 'term_w' }) expect(result).not.toContain('--- BASE DRIFT ---') @@ -219,6 +235,7 @@ describe('buildDispatchPreamble', () => { dispatchId: 'ctx_x', taskSpec: 'do stuff', coordinatorHandle: 'term_c', + workerHandle: 'term_w', baseDrift: { base: 'origin/main', behind: 3, @@ -241,7 +258,8 @@ describe('buildDispatchPreamble', () => { taskId: 'task_SNAP', dispatchId: 'ctx_SNAP', taskSpec: 'TASK_BODY', - coordinatorHandle: 'term_COORD' + coordinatorHandle: 'term_COORD', + workerHandle: 'term_WORKER' }) expect(result).toMatchSnapshot() }) diff --git a/src/main/runtime/orchestration/preamble.ts b/src/main/runtime/orchestration/preamble.ts index fdee65505e5..9aaa488318f 100644 --- a/src/main/runtime/orchestration/preamble.ts +++ b/src/main/runtime/orchestration/preamble.ts @@ -8,6 +8,7 @@ export type PreambleParams = { dispatchId: string taskSpec: string coordinatorHandle: string + workerHandle: string devMode?: boolean // Why: populated by the coordinator's dispatch pre-flight (§3.1) only // when the target worktree is behind its tracking remote. When absent @@ -68,7 +69,7 @@ Slack, GitHub comments, or any other channel to reach a human during the run. # with subject like "Failed: " — never silently exit. # Include BOTH taskId and dispatchId in the payload so a late completion # from a failed retry cannot complete the current dispatch. - ${cli} orchestration send --to ${params.coordinatorHandle} \\ + ${cli} orchestration send --to ${params.coordinatorHandle} --from ${params.workerHandle} \\ --type worker_done --subject "" \\ --body "<3-sentence summary: what you did, what you found, what's left>" \\ --task-id ${params.taskId} --dispatch-id ${params.dispatchId} \\ @@ -85,7 +86,7 @@ Slack, GitHub comments, or any other channel to reach a human during the run. # attributes the heartbeat to the specific dispatch context, not just # the task, so a straggler heartbeat from a previously-failed dispatch # cannot mask a hung retry. - ${cli} orchestration send --to ${params.coordinatorHandle} \\ + ${cli} orchestration send --to ${params.coordinatorHandle} --from ${params.workerHandle} \\ --type heartbeat --subject "alive" \\ --task-id ${params.taskId} --dispatch-id ${params.dispatchId} \\ --phase "" @@ -102,20 +103,20 @@ Slack, GitHub comments, or any other channel to reach a human during the run. # blocks on \`check --wait\` until the coordinator replies, then prints the # reply body. Use it anywhere you would otherwise have reached for # AskUserQuestion. - ${cli} orchestration ask --to ${params.coordinatorHandle} \\ + ${cli} orchestration ask --to ${params.coordinatorHandle} --from ${params.workerHandle} \\ --question "" \\ --options "" \\ --timeout-ms 600000 # Escalate a blocker or failure (pre-completion, when you need the # coordinator to do something before you can continue): - ${cli} orchestration send --to ${params.coordinatorHandle} \\ + ${cli} orchestration send --to ${params.coordinatorHandle} --from ${params.workerHandle} \\ --type escalation --subject "Blocked: " \\ --body "
" \\ --task-id ${params.taskId} # Check for messages from the coordinator: - ${cli} orchestration check + ${cli} orchestration check --terminal ${params.workerHandle} ${postDoneInstructions}` diff --git a/src/main/runtime/rpc/methods/orchestration.test.ts b/src/main/runtime/rpc/methods/orchestration.test.ts index cf0c722c675..fc7536c2e4f 100644 --- a/src/main/runtime/rpc/methods/orchestration.test.ts +++ b/src/main/runtime/rpc/methods/orchestration.test.ts @@ -105,6 +105,100 @@ describe('orchestration RPC methods', () => { expect(db.getMessageById(result.message.id)?.sender_pane_key).toBe('tab_a:leaf_a') }) + it('recovers missing sender pane identity from the resolved handle', async () => { + setup() + vi.spyOn(runtime, 'getTerminalPaneKey').mockReturnValue('tab_worker:leaf_worker') + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + + const result = (await call('orchestration.send', { + from: 'term_worker', + to: 'term_coord', + subject: 'hello' + })) as { message: { id: string } } + + expect(runtime.getTerminalPaneKey).toHaveBeenCalledWith('term_worker') + expect(db.getMessageById(result.message.id)?.sender_pane_key).toBe('tab_worker:leaf_worker') + }) + + it('completes an identity-less injected send through its explicit worker handle', async () => { + setup() + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_worker', 'tab_worker:leaf_worker') + vi.spyOn(runtime, 'getTerminalPaneKey').mockImplementation((handle) => + handle === 'term_worker' ? 'tab_worker:leaf_worker' : null + ) + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + + await call('orchestration.send', { + from: 'term_worker', + to: 'term_coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }) + }) + + expect(db.getTask(task.id)?.status).toBe('completed') + expect(db.getDispatchContextById(dispatch.id)?.status).toBe('completed') + }) + + it('rejects an identity-less lifecycle send resolved through the coordinator handle', async () => { + setup() + const task = db.createTask({ spec: 'work' }) + const dependent = db.createTask({ spec: 'dependent', deps: [task.id] }) + const dispatch = db.createDispatchContext(task.id, 'term_worker', 'tab_worker:leaf_worker') + vi.spyOn(runtime, 'getTerminalPaneKey').mockImplementation((handle) => + handle === 'term_coord' ? 'tab_coord:leaf_coord' : null + ) + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + + await call('orchestration.send', { + from: 'term_coord', + to: 'term_coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }) + }) + + expect(db.getTask(task.id)?.status).toBe('dispatched') + expect(db.getTask(dependent.id)?.status).toBe('pending') + }) + + it('does not replace a foreign sender pane with its claimed assignee handle pane', async () => { + setup() + const task = db.createTask({ spec: 'work' }) + const dispatch = db.createDispatchContext(task.id, 'term_worker', 'tab_worker:leaf_worker') + vi.spyOn(runtime, 'getTerminalPaneKey').mockReturnValue('tab_worker:leaf_worker') + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + + const result = (await call('orchestration.send', { + from: 'term_worker', + to: 'term_coord', + subject: 'Done', + type: 'worker_done', + senderPaneKey: 'tab_foreign:leaf_foreign', + payload: JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }) + })) as { + message: { id: string; type: string; subject: string } + lifecycle: { action: string; code: string; reason: string } + } + + expect(db.getTask(task.id)?.status).toBe('dispatched') + expect(result.lifecycle).toMatchObject({ + action: 'rejected', + code: 'sender_not_assignee', + reason: expect.stringContaining('expected handle term_worker') + }) + expect(result.message).toMatchObject({ + type: 'worker_done', + subject: 'Rejected worker_done: Done' + }) + expect(db.getUnreadMessages('term_coord')).toEqual([ + expect.objectContaining({ id: result.message.id, type: 'worker_done' }) + ]) + expect(runtime.notifyMessageArrived).toHaveBeenCalledWith('term_coord', 'worker_done') + }) + it('does not wake waiters for a heartbeat suppressed at send time', async () => { setup() const task = db.createTask({ spec: 'work' }) @@ -512,9 +606,9 @@ describe('orchestration RPC methods', () => { }) describe('orchestration.check', () => { - function createDispatchedTask(assigneeHandle = 'term_worker') { + function createDispatchedTask(assigneeHandle = 'term_worker', assigneePaneKey?: string) { const task = db.createTask({ spec: 'manual check work' }) - const dispatch = db.createDispatchContext(task.id, assigneeHandle) + const dispatch = db.createDispatchContext(task.id, assigneeHandle, assigneePaneKey) return { task, dispatch } } @@ -524,6 +618,7 @@ describe('orchestration RPC methods', () => { taskId?: string dispatchId?: string filesModified?: string[] + senderPaneKey?: string }): void { const payload: Record = {} if (params.taskId !== undefined) { @@ -541,7 +636,8 @@ describe('orchestration RPC methods', () => { to: params.to ?? 'term_coord', subject: 'Done', type: 'worker_done', - payload: JSON.stringify(payload) + payload: JSON.stringify(payload), + senderPaneKey: params.senderPaneKey }) } @@ -670,11 +766,13 @@ describe('orchestration RPC methods', () => { it('completes worker_done by payload IDs when the sender handle changed', async () => { setup() - const { task, dispatch } = createDispatchedTask('term_owner') + const leafId = '11111111-1111-4111-8111-111111111111' + const { task, dispatch } = createDispatchedTask('term_owner', `tab_before:${leafId}`) insertWorkerDone({ from: 'term_reminted', taskId: task.id, - dispatchId: dispatch.id + dispatchId: dispatch.id, + senderPaneKey: `tab_after:${leafId}` }) const result = (await call('orchestration.check', { @@ -710,6 +808,34 @@ describe('orchestration RPC methods', () => { expect(db.getDispatchContextById(activeDispatch.id)?.status).toBe('dispatched') }) + it('returns a persisted foreign completion as a rejection diagnostic', async () => { + setup() + const { task, dispatch } = createDispatchedTask('term_worker', 'tab_worker:leaf_worker') + insertWorkerDone({ + from: 'term_foreign', + taskId: task.id, + dispatchId: dispatch.id, + senderPaneKey: 'tab_foreign:leaf_foreign' + }) + + const result = (await call('orchestration.check', { + terminal: 'term_coord', + types: 'worker_done' + })) as { count: number; messages: { type: string; subject: string; body: string }[] } + + expect(result).toMatchObject({ + count: 1, + messages: [ + { + type: 'worker_done', + subject: 'Rejected worker_done: Done', + body: expect.stringContaining('expected handle term_worker') + } + ] + }) + expect(db.getTask(task.id)?.status).toBe('dispatched') + }) + it('records heartbeat returned by unread manual check', async () => { setup() const { task, dispatch } = createDispatchedTask() diff --git a/src/main/runtime/rpc/methods/orchestration.ts b/src/main/runtime/rpc/methods/orchestration.ts index 597ae99b7fb..9caaf1ac57a 100644 --- a/src/main/runtime/rpc/methods/orchestration.ts +++ b/src/main/runtime/rpc/methods/orchestration.ts @@ -209,6 +209,9 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ handler: async (params, { runtime }) => { const db = runtime.getOrchestrationDb() const from = params.from ?? 'unknown' + // Why: older live shells may lack ORCA_PANE_KEY, but the runtime still + // knows the pane behind their resolved handle; persist that authority. + const senderPaneKey = params.senderPaneKey ?? runtime.getTerminalPaneKey(from) ?? undefined if (!isGroupAddress(params.to)) { // Point-to-point — existing single-recipient behavior @@ -221,7 +224,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ priority: params.priority as MessagePriority, threadId: params.threadId, payload: params.payload, - senderPaneKey: params.senderPaneKey + senderPaneKey }) // Why: worker_done/heartbeat sent via `send` must release the dispatch // lock before waking recipients — a coordinator woken by delivery may @@ -234,6 +237,12 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ if (reconciled.action === 'suppressed') { return { message: msg } } + if (reconciled.action === 'rejected') { + const rejection = db.getMessageById(msg.id) ?? msg + runtime.deliverPendingMessagesForHandle(params.to) + runtime.notifyMessageArrived(params.to, rejection.type) + return { message: rejection, lifecycle: reconciled } + } } runtime.deliverPendingMessagesForHandle(params.to) runtime.notifyMessageArrived(params.to, msg.type) @@ -263,7 +272,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ priority: params.priority as MessagePriority, threadId, payload: params.payload, - senderPaneKey: params.senderPaneKey + senderPaneKey }) ) for (const message of messages) { @@ -304,22 +313,26 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ ? db.getAllMessagesForHandle(handle, undefined, typeFilter) : db.getUnreadMessages(handle, typeFilter) + let visibleMessages = messages if (consumeUnread && messages.length > 0) { // Why: manual coordinators can consume lifecycle messages before // the coordinator loop sees them, but unread `check` is still an // authoritative read path for worker_done/heartbeat. - for (const message of messages) { - reconcileLifecycleMessage(db, message) - } + visibleMessages = messages.map((message) => { + const reconciled = reconcileLifecycleMessage(db, message) + return reconciled.action === 'rejected' + ? (db.getMessageById(message.id) ?? message) + : message + }) db.markAsRead(messages.map((m) => m.id)) } if (params.inject) { - const formatted = messages.map(formatMessageBanner).join('\n\n') - return { messages, formatted, count: messages.length } + const formatted = visibleMessages.map(formatMessageBanner).join('\n\n') + return { messages: visibleMessages, formatted, count: visibleMessages.length } } - return { messages, count: messages.length } + return { messages: visibleMessages, count: visibleMessages.length } } if (signal?.aborted) { @@ -481,6 +494,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ dispatchId: 'ctx_dryrun', taskSpec: task.spec, coordinatorHandle: params.from ?? 'coordinator', + workerHandle: params.to ?? 'worker', devMode: params.devMode }) return { dispatch: null, injected: false, dryRun: true, preamble } @@ -525,6 +539,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ dispatchId: ctx.id, taskSpec: task.spec, coordinatorHandle: params.from ?? 'coordinator', + workerHandle: to, devMode: params.devMode }) @@ -576,6 +591,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ dispatchId: ctx?.id ?? 'ctx_preview', taskSpec: task.spec, coordinatorHandle: params.from ?? 'coordinator', + workerHandle: ctx?.assignee_handle ?? 'worker', devMode: params.devMode }) return { dispatch: ctx ?? null, preamble } diff --git a/src/main/ssh/ssh-remote-linear-argument-error.ts b/src/main/ssh/ssh-remote-cli-argument-error.ts similarity index 100% rename from src/main/ssh/ssh-remote-linear-argument-error.ts rename to src/main/ssh/ssh-remote-cli-argument-error.ts diff --git a/src/main/ssh/ssh-remote-linear-cli.ts b/src/main/ssh/ssh-remote-linear-cli.ts index 3367c217256..4a5c5c4b416 100644 --- a/src/main/ssh/ssh-remote-linear-cli.ts +++ b/src/main/ssh/ssh-remote-linear-cli.ts @@ -2,14 +2,12 @@ import type { RpcDispatcher } from '../runtime/rpc/dispatcher' import type { RpcResponse } from '../runtime/rpc/core' import { getRemoteLinearReadHelp } from './ssh-remote-linear-read-help' import { tryDispatchRemoteLinearReadCli } from './ssh-remote-linear-read-cli' -import { RemoteCliArgumentError, type ParsedRemoteCli } from './ssh-remote-linear-argument-error' +import type { ParsedRemoteCli } from './ssh-remote-cli-argument-error' import { getRemoteLinearWriteHelp, tryDispatchRemoteLinearWriteCli } from './ssh-remote-linear-write-cli' -export { RemoteCliArgumentError } - export function getRemoteLinearHelp(parsed: ParsedRemoteCli): string | null { const helpPath = remoteLinearHelpPath(parsed) if (!helpPath) { diff --git a/src/main/ssh/ssh-remote-linear-read-cli.ts b/src/main/ssh/ssh-remote-linear-read-cli.ts index 82a6fedb2ad..7e64081c5c0 100644 --- a/src/main/ssh/ssh-remote-linear-read-cli.ts +++ b/src/main/ssh/ssh-remote-linear-read-cli.ts @@ -6,7 +6,7 @@ import { } from '../../shared/linear-agent-access' import type { RpcDispatcher } from '../runtime/rpc/dispatcher' import type { RpcResponse } from '../runtime/rpc/core' -import { RemoteCliArgumentError, type ParsedRemoteCli } from './ssh-remote-linear-argument-error' +import { RemoteCliArgumentError, type ParsedRemoteCli } from './ssh-remote-cli-argument-error' import { LINEAR_ISSUE_FLAGS, diff --git a/src/main/ssh/ssh-remote-orca-cli.test.ts b/src/main/ssh/ssh-remote-orca-cli.test.ts index f54c2157c55..1741772d789 100644 --- a/src/main/ssh/ssh-remote-orca-cli.test.ts +++ b/src/main/ssh/ssh-remote-orca-cli.test.ts @@ -11,7 +11,8 @@ vi.mock('../persistence', () => ({ getCanonicalUserDataPath: () => '/host/user-data' })) -import type { OrcaRuntimeService } from '../runtime/orca-runtime' +import { OrchestrationDb } from '../runtime/orchestration/db' +import { OrcaRuntimeService } from '../runtime/orca-runtime' import type { HostCliPassthroughOptions } from './ssh-remote-cli-host-passthrough' import { runRemoteOrcaCli } from './ssh-remote-orca-cli' @@ -91,6 +92,7 @@ describe('runRemoteOrcaCli', () => { liveLeafCount: 1 }), getOrchestrationDb: () => db, + getTerminalPaneKey: () => null, deliverPendingMessagesForHandle: vi.fn(), notifyMessageArrived: vi.fn(), linearIssueContext: vi.fn(async (request: unknown) => ({ @@ -147,6 +149,191 @@ describe('runRemoteOrcaCli', () => { expect(db.getUnreadMessages('term_windows')[0]?.from_handle).toBe('term_ssh') }) + it('forwards remote pane identity through the legacy orchestration fallback', async () => { + const { runtime, db } = createRuntime() + + const result = await runRemoteOrcaCli( + runtime, + { + argv: ['orchestration', 'send', '--to', 'term_windows', '--subject', 'ping', '--json'], + cwd: '/home/alice/repo', + env: { + ORCA_TERMINAL_HANDLE: 'term_ssh', + ORCA_PANE_KEY: 'tab_ssh:leaf_ssh' + } + }, + LEGACY_FALLBACK_OPTIONS + ) + + expect(result.exitCode).toBe(0) + expect(db.insertMessage).toHaveBeenCalledWith( + expect.objectContaining({ senderPaneKey: 'tab_ssh:leaf_ssh' }) + ) + }) + + it('returns a non-zero status for lifecycle rejection through the legacy fallback', async () => { + const db = new OrchestrationDb(':memory:') + const runtime = new OrcaRuntimeService() + runtime.setOrchestrationDb(db) + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + const task = db.createTask({ spec: 'remote work' }) + const dispatch = db.createDispatchContext(task.id, 'term_ssh', 'tab_owner:leaf_owner') + + try { + const result = await runRemoteOrcaCli( + runtime, + { + argv: [ + 'orchestration', + 'send', + '--from', + 'term_ssh', + '--to', + 'term_coord', + '--subject', + 'Done', + '--type', + 'worker_done', + '--payload', + JSON.stringify({ taskId: task.id, dispatchId: dispatch.id }), + '--json' + ], + cwd: '/home/alice/repo', + env: { ORCA_PANE_KEY: 'tab_foreign:leaf_foreign' } + }, + LEGACY_FALLBACK_OPTIONS + ) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: true, + result: { + message: { type: 'worker_done', subject: 'Rejected worker_done: Done' }, + lifecycle: { action: 'rejected', code: 'sender_not_assignee' } + } + }) + expect(db.getTask(task.id)?.status).toBe('dispatched') + } finally { + db.close() + } + }) + + it('preserves structured lifecycle payload flags through the legacy fallback', async () => { + const db = new OrchestrationDb(':memory:') + const runtime = new OrcaRuntimeService() + runtime.setOrchestrationDb(db) + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + const task = db.createTask({ spec: 'remote work' }) + const dispatch = db.createDispatchContext(task.id, 'term_ssh', 'tab_owner:leaf_owner') + + try { + const result = await runRemoteOrcaCli( + runtime, + { + argv: [ + 'orchestration', + 'send', + '--to', + 'term_coord', + '--subject', + 'Done', + '--type', + 'worker_done', + '--task-id', + task.id, + '--dispatch-id', + dispatch.id, + '--files-modified', + 'src/a.ts, src/b.ts', + '--json' + ], + cwd: '/home/alice/repo', + env: { + ORCA_TERMINAL_HANDLE: 'term_ssh', + ORCA_PANE_KEY: 'tab_owner:leaf_owner' + } + }, + LEGACY_FALLBACK_OPTIONS + ) + + expect(result.exitCode).toBe(0) + expect(db.getTask(task.id)).toMatchObject({ + status: 'completed', + result: expect.stringContaining('src/a.ts') + }) + } finally { + db.close() + } + }) + + it('rejects identity-less lifecycle sends in the legacy fallback', async () => { + const { runtime, db } = createRuntime() + + const result = await runRemoteOrcaCli( + runtime, + { + argv: [ + 'orchestration', + 'send', + '--to', + 'term_coord', + '--subject', + 'Done', + '--type', + 'worker_done', + '--json' + ], + cwd: '/home/alice/repo', + env: {} + }, + LEGACY_FALLBACK_OPTIONS + ) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: false, + error: { code: 'no_active_sender_terminal' } + }) + expect(db.insertMessage).not.toHaveBeenCalled() + }) + + it('rejects mixed raw and structured payload flags in the legacy fallback', async () => { + const { runtime, db } = createRuntime() + + const result = await runRemoteOrcaCli( + runtime, + { + argv: [ + 'orchestration', + 'send', + '--from', + 'term_ssh', + '--to', + 'term_coord', + '--subject', + 'Done', + '--payload', + '{"taskId":"task_1"}', + '--task-id', + 'task_1', + '--json' + ], + cwd: '/home/alice/repo', + env: {} + }, + LEGACY_FALLBACK_OPTIONS + ) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toMatchObject({ + ok: false, + error: { code: 'invalid_argument', message: expect.stringContaining('structured payload') } + }) + expect(db.insertMessage).not.toHaveBeenCalled() + }) + it('accepts equals-style orchestration flags in the remote shim', async () => { const { runtime, db } = createRuntime() diff --git a/src/main/ssh/ssh-remote-orca-cli.ts b/src/main/ssh/ssh-remote-orca-cli.ts index d555b5c2707..205723321da 100644 --- a/src/main/ssh/ssh-remote-orca-cli.ts +++ b/src/main/ssh/ssh-remote-orca-cli.ts @@ -10,19 +10,16 @@ import { type RemoteOrcaCliRequest, type RemoteOrcaCliResult } from './ssh-remote-cli-host-passthrough' +import { RemoteCliArgumentError, type ParsedRemoteCli } from './ssh-remote-cli-argument-error' +import { getRemoteLinearHelp, tryDispatchRemoteLinearCli } from './ssh-remote-linear-cli' import { - RemoteCliArgumentError, - getRemoteLinearHelp, - tryDispatchRemoteLinearCli -} from './ssh-remote-linear-cli' + getRemoteOrchestrationPayload, + hasRemoteLifecycleRejection, + resolveRemoteOrchestrationSender +} from './ssh-remote-orchestration-send' export type { RemoteOrcaCliRequest, RemoteOrcaCliResult } from './ssh-remote-cli-host-passthrough' -type ParsedRemoteCli = { - commandPath: string[] - flags: Map -} - // Why: these commands run a foreground/interactive process attached to the // caller's TTY (or a local tmux pane), which a buffered one-shot relay bridge // cannot host. Everything else routes through the full host CLI. @@ -116,7 +113,9 @@ async function runLegacyRemoteOrcaCli( return { stdout: formatted.stdout, stderr: formatted.stderr, - exitCode: response.ok ? 0 : 1 + // Why: the legacy SSH bridge bypasses the local CLI handler that turns + // a persisted lifecycle rejection into an unsuccessful command. + exitCode: response.ok && !hasRemoteLifecycleRejection(response.result) ? 0 : 1 } } catch (err) { const message = err instanceof Error ? err.message : String(err) @@ -174,17 +173,22 @@ async function dispatchRemoteCli( worktree: optionalString(parsed.flags, 'worktree'), limit: optionalNumber(parsed.flags, 'limit') }) - case 'orchestration send': + case 'orchestration send': { + const type = optionalString(parsed.flags, 'type') return await call(dispatcher, 'orchestration.send', { - from: resolveHandle(parsed.flags, env, 'from'), + from: resolveRemoteOrchestrationSender(parsed.flags, env, type), to: requiredString(parsed.flags, 'to'), subject: requiredString(parsed.flags, 'subject'), body: optionalString(parsed.flags, 'body'), - type: optionalString(parsed.flags, 'type'), + type, priority: optionalString(parsed.flags, 'priority'), threadId: optionalString(parsed.flags, 'thread-id'), - payload: optionalString(parsed.flags, 'payload') + payload: getRemoteOrchestrationPayload(parsed.flags), + // Why: the legacy in-process bridge must preserve the same pane + // authority as the full host CLI passthrough. + senderPaneKey: env.ORCA_PANE_KEY || undefined }) + } case 'orchestration check': return await call(dispatcher, 'orchestration.check', { terminal: resolveHandle(parsed.flags, env, 'terminal'), diff --git a/src/main/ssh/ssh-remote-orchestration-send.test.ts b/src/main/ssh/ssh-remote-orchestration-send.test.ts new file mode 100644 index 00000000000..6efd9a18891 --- /dev/null +++ b/src/main/ssh/ssh-remote-orchestration-send.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' +import { + getRemoteOrchestrationPayload, + hasRemoteLifecycleRejection, + resolveRemoteOrchestrationSender +} from './ssh-remote-orchestration-send' + +describe('remote orchestration send compatibility', () => { + it('recognizes only structured lifecycle rejections', () => { + expect(hasRemoteLifecycleRejection({ lifecycle: { action: 'rejected' } })).toBe(true) + expect(hasRemoteLifecycleRejection({ lifecycle: null })).toBe(false) + expect(hasRemoteLifecycleRejection({ lifecycle: { action: 'completed' } })).toBe(false) + expect(hasRemoteLifecycleRejection(null)).toBe(false) + }) + + it('prefers an explicit sender over the remote terminal environment', () => { + expect( + resolveRemoteOrchestrationSender( + new Map([['from', 'term_explicit']]), + { ORCA_TERMINAL_HANDLE: 'term_env' }, + 'worker_done' + ) + ).toBe('term_explicit') + }) + + it.each(['worker_done', 'heartbeat'])('fails closed for an identity-less %s', (type) => { + expect(() => resolveRemoteOrchestrationSender(new Map(), {}, type)).toThrowError( + expect.objectContaining({ code: 'no_active_sender_terminal' }) + ) + }) + + it('preserves the legacy unknown sender for non-lifecycle messages', () => { + expect(resolveRemoteOrchestrationSender(new Map(), {}, 'status')).toBe('unknown') + }) + + it('serializes every structured payload field with local CLI semantics', () => { + const payload = getRemoteOrchestrationPayload( + new Map([ + ['task-id', 'task_1'], + ['dispatch-id', 'ctx_1'], + ['files-modified', 'src/a.ts, src/b.ts,'], + ['report-path', 'report.md'], + ['phase', 'reviewing'] + ]) + ) + + expect(JSON.parse(payload ?? '{}')).toEqual({ + taskId: 'task_1', + dispatchId: 'ctx_1', + filesModified: ['src/a.ts', 'src/b.ts'], + reportPath: 'report.md', + phase: 'reviewing' + }) + }) + + it('passes through a raw payload when no structured fields are present', () => { + expect(getRemoteOrchestrationPayload(new Map([['payload', '{"ok":true}']]))).toBe('{"ok":true}') + }) + + it('rejects mixed raw and structured payloads', () => { + expect(() => + getRemoteOrchestrationPayload( + new Map([ + ['payload', '{"taskId":"task_1"}'], + ['task-id', 'task_1'] + ]) + ) + ).toThrowError(expect.objectContaining({ code: 'invalid_argument' })) + }) +}) diff --git a/src/main/ssh/ssh-remote-orchestration-send.ts b/src/main/ssh/ssh-remote-orchestration-send.ts new file mode 100644 index 00000000000..5e844a9c586 --- /dev/null +++ b/src/main/ssh/ssh-remote-orchestration-send.ts @@ -0,0 +1,83 @@ +import { RemoteCliArgumentError } from './ssh-remote-cli-argument-error' + +type RemoteFlags = Map + +export function hasRemoteLifecycleRejection(result: unknown): boolean { + if (!result || typeof result !== 'object') { + return false + } + const lifecycle = (result as { lifecycle?: unknown }).lifecycle + return ( + lifecycle !== null && + typeof lifecycle === 'object' && + (lifecycle as { action?: unknown }).action === 'rejected' + ) +} + +export function resolveRemoteOrchestrationSender( + flags: RemoteFlags, + env: Record, + type: string | undefined +): string { + const explicit = optionalString(flags, 'from') + const envHandle = env.ORCA_TERMINAL_HANDLE || undefined + if ((type === 'worker_done' || type === 'heartbeat') && !explicit && !envHandle) { + // Why: the fallback must not turn missing remote identity into the + // synthetic "unknown" handle for lifecycle authority decisions. + throw new RemoteCliArgumentError( + 'no_active_sender_terminal', + 'Could not determine the sender terminal for this orchestration command. ' + + 'Pass --from or run the command inside a live Orca terminal with ORCA_TERMINAL_HANDLE set.' + ) + } + return explicit ?? envHandle ?? 'unknown' +} + +export function getRemoteOrchestrationPayload(flags: RemoteFlags): string | undefined { + const rawPayload = optionalString(flags, 'payload') + const taskId = optionalString(flags, 'task-id') + const dispatchId = optionalString(flags, 'dispatch-id') + const filesModified = optionalString(flags, 'files-modified') + const reportPath = optionalString(flags, 'report-path') + const phase = optionalString(flags, 'phase') + const hasStructuredPayload = [taskId, dispatchId, filesModified, reportPath, phase].some( + (value) => value !== undefined + ) + if (!hasStructuredPayload) { + return rawPayload + } + if (rawPayload !== undefined) { + throw new RemoteCliArgumentError( + 'invalid_argument', + 'Use either --payload or structured payload flags, not both.' + ) + } + + // Why: the fallback receives the same preamble commands as the full CLI; + // preserving these flags keeps lifecycle payloads valid over broken installs. + const payload: Record = {} + if (taskId) { + payload.taskId = taskId + } + if (dispatchId) { + payload.dispatchId = dispatchId + } + if (filesModified) { + payload.filesModified = filesModified + .split(',') + .map((file) => file.trim()) + .filter(Boolean) + } + if (reportPath) { + payload.reportPath = reportPath + } + if (phase) { + payload.phase = phase + } + return JSON.stringify(payload) +} + +function optionalString(flags: RemoteFlags, name: string): string | undefined { + const value = flags.get(name) + return typeof value === 'string' && value.length > 0 ? value : undefined +} From ec7cc2df569b1791614180816c471f5443a0753b Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:48:01 -0700 Subject: [PATCH 3/3] Restyle Open/Start button as primary Button component (#8487) Replace the raw + )} {item.type !== 'pr' ? (