From 7ce58ec47e304ea51197f7ab3efc03cfbcd7930e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:50:57 -0700 Subject: [PATCH] refactor(terminal): one PTY attribution for teardown, close and identity; delete the deprecated owner resolver (#30626) * refactor(terminal): one PTY attribution for teardown, close and identity; delete the deprecated resolver pair - strict ssh: route: a proven direct listing beats a same-id server proxy; other contradictions refuse - terminal spawn's git branch takes the strict route when a host is named (rivals refuse) - resolveTerminalPtyOwner + isUnattributedTwinWorktree replace resolveRuntimeOwnerForWorktree in shutdown, pane and agent hibernation, and PTY identity; close logs unroutable runtime ids - delete getRuntimeEnvironmentIdForWorktree/resolveRuntimeOwnerForWorktree (legacy branch moved into the adapter), the alias golden test, the importer ratchet, its baseline and pr.yml step - hasHostEvidence reads the owner indexes instead of scanning repos and groups * fix(terminal): close and identity read spawn's attribution for unscoped remote ids (Opus P2) - an unattributed twin's unscoped remote: id is skipped and logged on close, never closed locally - setup terminals: identity and close follow spawn's runtime; raw-id teardown stays local - drop the last stale getRuntimeEnvironmentIdForWorktree mock (tests/e2e unit suite) * refactor(terminal): one ownership read in resolveTerminalPtyOwner * test(terminal): pin the close-path twin skip and the two-proxy direct listing (Opus P3) * test(terminal): keep the close-provider owner fixture's full union type (Codex P1) --- .github/workflows/pr.yml | 4 - ...cated-runtime-owner-importers-baseline.txt | 9 -- config/focus-setting-read-baseline.txt | 9 +- ...check-deprecated-runtime-owner-ratchet.mjs | 70 ----------- ...-deprecated-runtime-owner-ratchet.test.mjs | 101 ---------------- config/scripts/pr-preflight-gates.test.mjs | 1 - package.json | 3 +- .../components/terminal-workspace-model.ts | 4 +- .../src/lib/agent-hibernation-coordinator.ts | 4 +- .../lib/agent-hibernation-runtime-owner.ts | 4 +- ...acy-runtime-owner-answers.test-fixture.ts} | 4 +- .../src/lib/paired-web-client-host.test.ts | 14 +-- .../src/lib/resolve-owner-acceptance.test.ts | 14 +-- .../lib/runtime-owner-alias-golden.test.ts | 60 ---------- .../lib/terminal-pty-owner-acceptance.test.ts | 109 +++++++++++++++++ .../src/lib/terminal-worktree-route.ts | 91 +++++++++++++- .../src/lib/worktree-operation-route.ts | 27 ++++- ...ivation-preserved-pane-replacement.test.ts | 8 +- .../src/lib/worktree-runtime-owner.test.ts | 111 ++++++++++++------ .../src/lib/worktree-runtime-owner.ts | 91 +++++--------- ...terminal-pane-retirement-ownership.test.ts | 24 ++++ .../store/slices/terminal-tab-retirement.ts | 12 +- .../terminals/terminal-pane-hibernation.ts | 4 +- .../src/store/terminals/terminal-shutdown.ts | 4 +- .../terminal-tab-close-providers.test.ts | 71 +++++++++++ .../terminals/terminal-tab-close-providers.ts | 22 +++- .../terminals/terminal-teardown-owner.ts | 4 +- ...x-child-approval-activity-row.unit.test.ts | 8 -- 28 files changed, 475 insertions(+), 412 deletions(-) delete mode 100644 config/deprecated-runtime-owner-importers-baseline.txt delete mode 100644 config/scripts/check-deprecated-runtime-owner-ratchet.mjs delete mode 100644 config/scripts/check-deprecated-runtime-owner-ratchet.test.mjs rename src/renderer/src/lib/{runtime-owner-alias-golden.test-fixture.ts => legacy-runtime-owner-answers.test-fixture.ts} (95%) delete mode 100644 src/renderer/src/lib/runtime-owner-alias-golden.test.ts create mode 100644 src/renderer/src/lib/terminal-pty-owner-acceptance.test.ts create mode 100644 src/renderer/src/store/terminals/terminal-tab-close-providers.test.ts diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b173efb769d..33f8d94e0b6 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -393,10 +393,6 @@ jobs: if: needs.code_paths.outputs.static_analysis == 'true' run: bun config/scripts/check-layout-writer-ratchets.mjs - - name: Enforce deprecated runtime-owner importer ratchet - if: needs.code_paths.outputs.static_analysis == 'true' - run: node config/scripts/check-deprecated-runtime-owner-ratchet.mjs - - name: Enforce path-literal ratchet if: needs.code_paths.outputs.static_analysis == 'true' run: node config/scripts/check-path-literal-ratchet.mjs diff --git a/config/deprecated-runtime-owner-importers-baseline.txt b/config/deprecated-runtime-owner-importers-baseline.txt deleted file mode 100644 index 37adb8baff4..00000000000 --- a/config/deprecated-runtime-owner-importers-baseline.txt +++ /dev/null @@ -1,9 +0,0 @@ -# Renderer files that import getRuntimeEnvironmentIdForWorktree / resolveRuntimeOwnerForWorktree. -# This is a RATCHET: the list may only SHRINK. Both resolvers read an ambiguous (two-host) -# worktree as "not a runtime", which callers treat as local. -# Prune after migrating a file: pnpm check:deprecated-runtime-owner-ratchet --prune -src/renderer/src/components/terminal-workspace-model.ts -src/renderer/src/lib/agent-hibernation-coordinator.ts -src/renderer/src/store/slices/terminal-tab-retirement.ts -src/renderer/src/store/terminals/terminal-pane-hibernation.ts -src/renderer/src/store/terminals/terminal-shutdown.ts diff --git a/config/focus-setting-read-baseline.txt b/config/focus-setting-read-baseline.txt index d75bb384ccc..8b10bcf2eec 100644 --- a/config/focus-setting-read-baseline.txt +++ b/config/focus-setting-read-baseline.txt @@ -304,7 +304,6 @@ 1 src/renderer/src/lib/agent-background-session-launch-host.ts 1 src/renderer/src/lib/agent-detection-target-inventory.ts 1 src/renderer/src/lib/agent-hibernation-coordinator-test-fixture.ts -1 src/renderer/src/lib/agent-hibernation-coordinator.ts 2 src/renderer/src/lib/agent-launch-route-input.ts 1 src/renderer/src/lib/agent-message-send.ts 1 src/renderer/src/lib/ai-vault-resume-platform.ts @@ -346,7 +345,7 @@ 1 src/renderer/src/lib/tab-number-shortcuts.ts 1 src/renderer/src/lib/terminal-close-route.ts 1 src/renderer/src/lib/terminal-tab-owner-activation.ts -5 src/renderer/src/lib/terminal-worktree-route.ts +6 src/renderer/src/lib/terminal-worktree-route.ts # resolveTerminalPtyOwner: teardown, close and PTY identity read spawn's attribution (round 3 F8), which retired the deprecated resolver's readers 3 src/renderer/src/lib/unread-badge-count-selector.ts 2 src/renderer/src/lib/web-runtime-worktree-terminal-after-wake.ts 5 src/renderer/src/lib/workspace-browser-tab-open.ts @@ -359,7 +358,7 @@ 1 src/renderer/src/lib/worktree-jump-navigation.ts 2 src/renderer/src/lib/worktree-operation-route.ts 1 src/renderer/src/lib/worktree-row-open-in-owner.ts # reads focus through resolveOwnerWithLegacyFallback, newly counted (round 3 F2); F8 nets it down -9 src/renderer/src/lib/worktree-runtime-owner.ts # typed owner resolver, its null-collapsing wrapper, and the legacy-fallback adapter the folds move to (round 3 F0); deleting the deprecated pair (F8) nets it down +8 src/renderer/src/lib/worktree-runtime-owner.ts # typed owner resolver and the legacy-fallback adapter the folds moved to (round 3 F0); F8 deleted the deprecated pair 1 src/renderer/src/lib/worktree-sort-order-host-split.ts 1 src/renderer/src/lib/worktree-sort-order-persistence.ts # its split helper calls resolveOwnerWithLegacyFallback directly (round 3 F4); F8 nets it down 1 src/renderer/src/runtime/local-structured-session-tabs-sync/snapshot-apply.ts @@ -451,7 +450,7 @@ 8 src/renderer/src/store/slices/store-test-helpers.ts 1 src/renderer/src/store/slices/tabs/tabs-close-actions.ts 2 src/renderer/src/store/slices/tabs/tabs-host-mirroring.ts -2 src/renderer/src/store/slices/terminal-tab-retirement.ts +1 src/renderer/src/store/slices/terminal-tab-retirement.ts 3 src/renderer/src/store/slices/workspace-attention/workspace-unread-characterization-fixture.ts 1 src/renderer/src/store/slices/worktree-visibility-owner-settings.ts 3 src/renderer/src/store/slices/worktree-visibility-settings-write.ts @@ -468,8 +467,6 @@ 3 src/renderer/src/store/slices/worktrees/session/worktree-terminal-reference.ts 1 src/renderer/src/store/slices/worktrees/teardown/purged-structured-launch-retirement.ts 2 src/renderer/src/store/terminals/terminal-active-workspace-creation.ts -1 src/renderer/src/store/terminals/terminal-pane-hibernation.ts -1 src/renderer/src/store/terminals/terminal-shutdown.ts 1 src/renderer/src/store/terminals/terminal-tab-appearance.ts 1 src/renderer/src/store/terminals/terminal-tab-close-providers.ts 1 src/renderer/src/web/preload-api/web-app-api.ts diff --git a/config/scripts/check-deprecated-runtime-owner-ratchet.mjs b/config/scripts/check-deprecated-runtime-owner-ratchet.mjs deleted file mode 100644 index a2854403737..00000000000 --- a/config/scripts/check-deprecated-runtime-owner-ratchet.mjs +++ /dev/null @@ -1,70 +0,0 @@ -import process from 'node:process' -import { pathToFileURL } from 'node:url' -import { readTrackedSources, runEntryRatchet } from './ratchet-baseline.mjs' - -// Ratchet on renderer files that import the deprecated runtime-owner resolvers. Both read a -// worktree that two hosts publish as "not a runtime", which callers then treat as local, so a stop, -// sleep or kill can land on the wrong machine. New code resolves the owner with its host and -// ambiguity kept; the importer list may only shrink. - -const BASELINE_PATH = 'config/deprecated-runtime-owner-importers-baseline.txt' -const SCAN_ROOT = 'src/renderer/src' -const DEPRECATED_NAMES = ['getRuntimeEnvironmentIdForWorktree', 'resolveRuntimeOwnerForWorktree'] -// The defining module and its own tests are not importers. -const DEFINING_FILE = 'src/renderer/src/lib/worktree-runtime-owner.ts' - -// Any reference to a deprecated name in a file that loads the defining module, whatever the import -// form (named, aliased, namespace, dynamic, `.then` destructuring, re-export). Simpler than tracking -// bindings, and in production code a mention of these names is a use. -const DEPRECATED_NAME = new RegExp(`\\b(?:${DEPRECATED_NAMES.join('|')})\\b`) -const DEFINING_MODULE_SPECIFIER = /['"`][^'"`]*\/worktree-runtime-owner['"`]/ -// Block and line comments; `://` inside a string (a URL) is not a line comment. -const COMMENT = /\/\*[\s\S]*?\*\/|(? importsDeprecatedRuntimeOwner(source)) - .map(([rel]) => rel) - .sort() -} - -const RATCHET = { - name: 'deprecated-runtime-owner', - baselinePath: BASELINE_PATH, - header: [ - '# Renderer files that import getRuntimeEnvironmentIdForWorktree / resolveRuntimeOwnerForWorktree.', - '# This is a RATCHET: the list may only SHRINK. Both resolvers read an ambiguous (two-host)', - '# worktree as "not a runtime", which callers treat as local.', - '# Prune after migrating a file: pnpm check:deprecated-runtime-owner-ratchet --prune' - ], - collect: collectDeprecatedRuntimeOwnerImporters, - fix: [ - 'getRuntimeEnvironmentIdForWorktree / resolveRuntimeOwnerForWorktree collapse an owner that two', - 'hosts publish into "local". Resolve the owner with its host kept and refuse on ambiguity', - 'instead (see src/renderer/src/lib/resolve-owner.ts).' - ] -} - -export function main(root = process.cwd(), arg = undefined) { - return runEntryRatchet(RATCHET, root, arg) -} - -if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { - process.exit(main(process.cwd(), process.argv[2])) -} diff --git a/config/scripts/check-deprecated-runtime-owner-ratchet.test.mjs b/config/scripts/check-deprecated-runtime-owner-ratchet.test.mjs deleted file mode 100644 index 8de50f66890..00000000000 --- a/config/scripts/check-deprecated-runtime-owner-ratchet.test.mjs +++ /dev/null @@ -1,101 +0,0 @@ -import { describe, expect, it } from 'vitest' - -import { - importsDeprecatedRuntimeOwner, - isScannedPath -} from './check-deprecated-runtime-owner-ratchet.mjs' - -describe('importsDeprecatedRuntimeOwner', () => { - it('finds a named import of either deprecated resolver', () => { - expect( - importsDeprecatedRuntimeOwner( - "import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner'\n" - ) - ).toBe(true) - expect( - importsDeprecatedRuntimeOwner( - "import {\n foo,\n resolveRuntimeOwnerForWorktree\n} from './worktree-runtime-owner'\n" - ) - ).toBe(true) - }) - - it('finds an aliased import, a re-export and a dynamic import', () => { - expect( - importsDeprecatedRuntimeOwner( - "import { getRuntimeEnvironmentIdForWorktree as envFor } from '@/lib/worktree-runtime-owner'" - ) - ).toBe(true) - expect( - importsDeprecatedRuntimeOwner( - "export { resolveRuntimeOwnerForWorktree } from './worktree-runtime-owner'" - ) - ).toBe(true) - expect( - importsDeprecatedRuntimeOwner( - "const { getRuntimeEnvironmentIdForWorktree } = await import('@/lib/worktree-runtime-owner')" - ) - ).toBe(true) - }) - - it('finds a use through a namespace import or a module-valued dynamic import', () => { - expect( - importsDeprecatedRuntimeOwner( - "import * as owner from '@/lib/worktree-runtime-owner'\nowner.getRuntimeEnvironmentIdForWorktree(s, id)" - ) - ).toBe(true) - expect( - importsDeprecatedRuntimeOwner( - "const mod = await import('@/lib/worktree-runtime-owner')\nmod.resolveRuntimeOwnerForWorktree(s, id)" - ) - ).toBe(true) - }) - - it('finds destructuring in a promise-based dynamic import', () => { - expect( - importsDeprecatedRuntimeOwner( - "import('@/lib/worktree-runtime-owner').then(({ resolveRuntimeOwnerForWorktree }) => resolveRuntimeOwnerForWorktree(s, id))" - ) - ).toBe(true) - }) - - it('reads through comments inside an import list, and ignores a commented-out import', () => { - expect( - importsDeprecatedRuntimeOwner( - "import { /* legacy */ getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner'" - ) - ).toBe(true) - expect( - importsDeprecatedRuntimeOwner( - "import {\n a, // keep\n resolveRuntimeOwnerForWorktree\n} from './worktree-runtime-owner'" - ) - ).toBe(true) - expect( - importsDeprecatedRuntimeOwner( - "// import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner'\n/* owner.resolveRuntimeOwnerForWorktree() */" - ) - ).toBe(false) - }) - - it('ignores other names from the module, and same-named functions from other modules', () => { - expect( - importsDeprecatedRuntimeOwner( - "import { getExecutionHostIdForWorktree } from '@/lib/worktree-runtime-owner'" - ) - ).toBe(false) - expect( - importsDeprecatedRuntimeOwner( - "import { getRuntimeEnvironmentIdForWorktree } from '@/lib/other-owner'" - ) - ).toBe(false) - }) -}) - -describe('isScannedPath', () => { - it('scans renderer sources only, not tests or the defining module', () => { - expect(isScannedPath('src/renderer/src/store/terminals/terminal-shutdown.ts')).toBe(true) - expect(isScannedPath('src/renderer/src/store/terminals/terminal-shutdown.test.ts')).toBe(false) - expect(isScannedPath('src/renderer/src/store/host-twin.test-fixture.ts')).toBe(false) - expect(isScannedPath('src/renderer/src/lib/worktree-runtime-owner.ts')).toBe(false) - expect(isScannedPath('src/main/runtime/foo.ts')).toBe(false) - }) -}) diff --git a/config/scripts/pr-preflight-gates.test.mjs b/config/scripts/pr-preflight-gates.test.mjs index dad604c88b6..9d6032231e6 100644 --- a/config/scripts/pr-preflight-gates.test.mjs +++ b/config/scripts/pr-preflight-gates.test.mjs @@ -243,7 +243,6 @@ it('pins every foreground and background step to its selected phase', () => { ['Enforce runtime Electron-import ratchet', staticPhase], ['Enforce focus-read ratchet', staticPhase], ['Enforce layout-writer ratchets', staticPhase], - ['Enforce deprecated runtime-owner importer ratchet', staticPhase], ['Enforce path-literal ratchet', staticPhase], ['Enforce worktree-id composer ratchet', staticPhase], ['Enforce e2e typecheck ratchet', staticPhase], diff --git a/package.json b/package.json index 1447e4b5514..005052c5983 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "audit:perf": "oxlint --config config/oxlint-performance-audit.json --format json src", "test:perf:contracts": "vitest run --config config/vitest.performance.config.ts", "format": "oxfmt --write .", - "lint": "oxlint && pnpm run audit:anti-slop && pnpm run audit:code-quality:native && pnpm run audit:code-quality:type-aware && pnpm run check:reliability-gates && pnpm run check:dead-classes && pnpm run check:max-lines-ratchet && pnpm run check:ts-nocheck-ratchet && pnpm run check:runtime-electron-ratchet && pnpm run check:focus-read-ratchet && pnpm run check:layout-writer-ratchets && pnpm run check:deprecated-runtime-owner-ratchet && pnpm run check:path-literal-ratchet && pnpm run check:legacy-relay-fence-ratchet && pnpm run check:readme-local-links && pnpm run check:node-runtime-pin && pnpm run verify:rpc-params-catalog && pnpm run verify:acp-protocol && pnpm run verify:bundled-skill-guides && pnpm run verify:skill-bundle-manifest && pnpm run verify:localization-catalogs && pnpm run verify:localization-extraction && pnpm run verify:localization-coverage && pnpm run check:process-host-imports && pnpm run check:main-sync-spawn-ratchet && pnpm run check:compat-boundary-imports", + "lint": "oxlint && pnpm run audit:anti-slop && pnpm run audit:code-quality:native && pnpm run audit:code-quality:type-aware && pnpm run check:reliability-gates && pnpm run check:dead-classes && pnpm run check:max-lines-ratchet && pnpm run check:ts-nocheck-ratchet && pnpm run check:runtime-electron-ratchet && pnpm run check:focus-read-ratchet && pnpm run check:layout-writer-ratchets && pnpm run check:path-literal-ratchet && pnpm run check:legacy-relay-fence-ratchet && pnpm run check:readme-local-links && pnpm run check:node-runtime-pin && pnpm run verify:rpc-params-catalog && pnpm run verify:acp-protocol && pnpm run verify:bundled-skill-guides && pnpm run verify:skill-bundle-manifest && pnpm run verify:localization-catalogs && pnpm run verify:localization-extraction && pnpm run verify:localization-coverage && pnpm run check:process-host-imports && pnpm run check:main-sync-spawn-ratchet && pnpm run check:compat-boundary-imports", "audit:code-quality": "pnpm run audit:code-quality:native && pnpm run audit:code-quality:type-aware && pnpm run audit:react-doctor", "audit:code-quality:native": "oxlint --config config/oxlint-code-quality-native-plugins.json src config tests mobile --deny-warnings", "audit:code-quality:type-aware": "oxlint --type-aware --config config/oxlint-code-quality-type-aware.json src config tests --deny-warnings", @@ -38,7 +38,6 @@ "check:reliability-gates": "node config/scripts/check-reliability-gates.mjs", "check:max-lines-ratchet": "node config/scripts/check-max-lines-ratchet.mjs", "check:ts-nocheck-ratchet": "node config/scripts/check-ts-nocheck-ratchet.mjs", - "check:deprecated-runtime-owner-ratchet": "node config/scripts/check-deprecated-runtime-owner-ratchet.mjs", "check:path-literal-ratchet": "node config/scripts/check-path-literal-ratchet.mjs", "check:legacy-relay-fence-ratchet": "node config/scripts/check-legacy-relay-fence-ratchet.mjs", "check:worktree-id-composer-ratchet": "node config/scripts/check-worktree-id-composer-ratchet.mjs", diff --git a/src/renderer/src/components/terminal-workspace-model.ts b/src/renderer/src/components/terminal-workspace-model.ts index 44e732fe0da..360ae69ced8 100644 --- a/src/renderer/src/components/terminal-workspace-model.ts +++ b/src/renderer/src/components/terminal-workspace-model.ts @@ -1,7 +1,7 @@ import type { Tab } from '../../../shared/tab-types' import type { KeybindingContext } from '../../../shared/keybindings' import { useAppStore } from '../store' -import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' +import { runtimeEnvironmentIdWithLegacyFallbackUnlessTwin } from '@/lib/worktree-runtime-owner' import { isEditorTabContentType } from '@/store/slices/editor/tabs/editor-tab-content-type' export const CLOSE_DIALOG_DEBOUNCE_MS = 200 @@ -56,7 +56,7 @@ export function isPinnedVisibleTab( } export function getActiveWorktreeRuntimeEnvironmentId(worktreeId: string | null): string | null { - return getRuntimeEnvironmentIdForWorktree(useAppStore.getState(), worktreeId) + return runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(useAppStore.getState(), worktreeId) } export function isPinnedActiveEditorTab( diff --git a/src/renderer/src/lib/agent-hibernation-coordinator.ts b/src/renderer/src/lib/agent-hibernation-coordinator.ts index c2f0996d04b..b56ba6a1383 100644 --- a/src/renderer/src/lib/agent-hibernation-coordinator.ts +++ b/src/renderer/src/lib/agent-hibernation-coordinator.ts @@ -22,7 +22,7 @@ import { observeHibernationPtyBindings } from './agent-hibernation-pane-age' import { mergePendingTerminalInputActivity } from './terminal-input-activity-coalescing' -import { resolveRuntimeOwnerForWorktree } from './worktree-runtime-owner' +import { resolveTerminalPtyOwner } from './terminal-worktree-route' import { toAgentHibernationRuntimeOwner } from './agent-hibernation-runtime-owner' import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import { toRuntimeWorktreeSelector } from '@/runtime/runtime-worktree-selector' @@ -111,7 +111,7 @@ function snapshotFromState( function resolveAgentHibernationRuntimeOwner(state: AppState, worktreeId: string) { const tabs = state.tabsByWorktree[worktreeId] ?? [] const ptyIds = tabs.flatMap((tab) => state.ptyIdsByTabId[tab.id] ?? []) - return toAgentHibernationRuntimeOwner(resolveRuntimeOwnerForWorktree(state, worktreeId), ptyIds) + return toAgentHibernationRuntimeOwner(resolveTerminalPtyOwner(state, worktreeId), ptyIds) } /** Worktree → owning runtime; `null` means liveness is required but no host can be asked. */ diff --git a/src/renderer/src/lib/agent-hibernation-runtime-owner.ts b/src/renderer/src/lib/agent-hibernation-runtime-owner.ts index d6bb16f4aee..f4989f76332 100644 --- a/src/renderer/src/lib/agent-hibernation-runtime-owner.ts +++ b/src/renderer/src/lib/agent-hibernation-runtime-owner.ts @@ -1,5 +1,5 @@ import { resolveTerminalTeardownOwner } from '@/store/terminals/terminal-teardown-owner' -import type { WorktreeRuntimeOwner } from './worktree-runtime-owner' +import type { TerminalPtyOwner } from '@/lib/terminal-worktree-route' export type AgentHibernationRuntimeOwner = | { kind: 'runtime'; environmentId: string } @@ -9,7 +9,7 @@ export type AgentHibernationRuntimeOwner = /** `ptyIds` are the worktree's bound PTYs; they only decide the owner when two hosts publish it. */ export function toAgentHibernationRuntimeOwner( - worktreeOwner: WorktreeRuntimeOwner, + worktreeOwner: TerminalPtyOwner, ptyIds: readonly string[] ): AgentHibernationRuntimeOwner { if (worktreeOwner.kind !== 'ambiguous') { diff --git a/src/renderer/src/lib/runtime-owner-alias-golden.test-fixture.ts b/src/renderer/src/lib/legacy-runtime-owner-answers.test-fixture.ts similarity index 95% rename from src/renderer/src/lib/runtime-owner-alias-golden.test-fixture.ts rename to src/renderer/src/lib/legacy-runtime-owner-answers.test-fixture.ts index 1b8077a7f5d..ae9b11b9592 100644 --- a/src/renderer/src/lib/runtime-owner-alias-golden.test-fixture.ts +++ b/src/renderer/src/lib/legacy-runtime-owner-answers.test-fixture.ts @@ -1,6 +1,6 @@ /** - * The deprecated `resolveRuntimeOwnerForWorktree` over the alias matrix; must not move. - * Captured by running main @ 37c314bcc960 over the whole matrix. + * What the deleted `resolveRuntimeOwnerForWorktree` answered over the owner matrix (main @ + * 37c314bcc960), so its replacements can list every deliberate difference. */ export const LEGACY_RUNTIME_OWNER_GOLDEN: Readonly> = { 'focused | null': null, diff --git a/src/renderer/src/lib/paired-web-client-host.test.ts b/src/renderer/src/lib/paired-web-client-host.test.ts index 2cc371aa3af..8ccabd33fc3 100644 --- a/src/renderer/src/lib/paired-web-client-host.test.ts +++ b/src/renderer/src/lib/paired-web-client-host.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -import { getRuntimeEnvironmentIdForWorktree } from './worktree-runtime-owner' +import { runtimeEnvironmentIdWithLegacyFallbackUnlessTwin } from './worktree-runtime-owner' import type { WorktreeRuntimeOwnerState } from './worktree-runtime-owner-state' import { resolveWorktreeOperationRoute } from './worktree-operation-route' import { resolveTerminalHostOwnership } from './terminal-worktree-route' @@ -42,7 +42,7 @@ describe('a paired web client routes every owner through its server (#9047)', () webState({ activeWorktreeId: WT, activeWorkspaceExecutionHostId: 'local' }), webState(UNSTAMPED_LOCAL_ROW) ]) { - expect(getRuntimeEnvironmentIdForWorktree(state, WT)).toBe(ENV) + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, WT)).toBe(ENV) expect(resolveTerminalHostOwnership(state, WT, 'spawn')).toEqual({ kind: 'runtime', runtimeEnvironmentId: ENV @@ -64,9 +64,9 @@ describe('a paired web client routes every owner through its server (#9047)', () { id: 'folder-1', projectGroupId: 'group-1', executionHostId: 'local', connectionId: null } ] }) - expect(getRuntimeEnvironmentIdForWorktree(folderState, folderWorkspaceKey('folder-1'))).toBe( - ENV - ) + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(folderState, folderWorkspaceKey('folder-1')) + ).toBe(ENV) }) it('keeps an SSH target as the place and the server as the transport', () => { @@ -88,12 +88,12 @@ describe('a paired web client routes every owner through its server (#9047)', () ] } }) - expect(getRuntimeEnvironmentIdForWorktree(state, WT)).toBeNull() + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, WT)).toBeNull() }) it('leaves a desktop window local workspace on this computer', () => { const state = webState(UNSTAMPED_LOCAL_ROW) - expect(getRuntimeEnvironmentIdForWorktree(state, WT)).toBeNull() + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, WT)).toBeNull() expect(resolveTerminalHostOwnership(state, WT, 'spawn').kind).toBe('local-or-ssh') }) }) diff --git a/src/renderer/src/lib/resolve-owner-acceptance.test.ts b/src/renderer/src/lib/resolve-owner-acceptance.test.ts index 138644ee62b..aa0233914ca 100644 --- a/src/renderer/src/lib/resolve-owner-acceptance.test.ts +++ b/src/renderer/src/lib/resolve-owner-acceptance.test.ts @@ -1,12 +1,10 @@ import { describe, expect, it } from 'vitest' import type { ExecutionHostOwnerMatch } from '../../../shared/worktree-execution-host-resolution' import type { HostAuthority } from '../../../shared/host-authority' -import { - getRuntimeEnvironmentIdForWorktree, - resolveOwnerWithLegacyFallback -} from './worktree-runtime-owner' +import { resolveOwnerWithLegacyFallback } from './worktree-runtime-owner' import { OWNER_RESOLUTION_CASES } from './runtime-owner-alias-matrix.test-fixture' import { LEGACY_FALLBACK_OWNER_ACCEPTANCE } from './resolve-owner-acceptance.test-fixture' +import { LEGACY_RUNTIME_OWNER_GOLDEN } from './legacy-runtime-owner-answers.test-fixture' import type { WorktreeRuntimeOwnerState } from './worktree-runtime-owner-state' function describeMatch(match: ExecutionHostOwnerMatch): string { @@ -50,10 +48,9 @@ describe('resolveOwnerWithLegacyFallback acceptance (round-3 design §5)', () => owner.kind === 'resolved' && owner.owner.endpoint.kind === 'environment' ? owner.owner.endpoint.environmentId : null - return ( - owner.kind !== 'resolved' || - environmentId !== getRuntimeEnvironmentIdForWorktree(c.state, c.worktreeId) - ) + const legacy = LEGACY_RUNTIME_OWNER_GOLDEN[c.name] + // Why: the deprecated environment-id reader collapsed `ambiguous` to `null`. + return owner.kind !== 'resolved' || environmentId !== (legacy === 'ambiguous' ? null : legacy) }) for (const c of changed) { expect(DELIBERATE_CHANGES[c.worktreeId], c.name).toBeDefined() @@ -117,6 +114,5 @@ describe('resolveOwnerWithLegacyFallback probes', () => { } } expect(ownerOf(withDetected, 'repo-twin-only::wt')).toBe('self@local') - expect(getRuntimeEnvironmentIdForWorktree(withDetected, 'repo-twin-only::wt')).toBeNull() }) }) diff --git a/src/renderer/src/lib/runtime-owner-alias-golden.test.ts b/src/renderer/src/lib/runtime-owner-alias-golden.test.ts deleted file mode 100644 index 4d5ff92a619..00000000000 --- a/src/renderer/src/lib/runtime-owner-alias-golden.test.ts +++ /dev/null @@ -1,60 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { - getRuntimeEnvironmentIdForWorktree, - resolveRuntimeOwnerForWorktree -} from './worktree-runtime-owner' -import { resolveOwner } from './resolve-owner' -import { OWNER_RESOLUTION_CASES } from './runtime-owner-alias-matrix.test-fixture' -import { LEGACY_RUNTIME_OWNER_GOLDEN } from './runtime-owner-alias-golden.test-fixture' - -describe('the deprecated runtime-owner resolvers', () => { - it('map every case exactly as on main before resolveOwner replaced them', () => { - const actual = Object.fromEntries( - OWNER_RESOLUTION_CASES.map((c) => { - const owner = resolveRuntimeOwnerForWorktree(c.state, c.worktreeId) - const environmentId = getRuntimeEnvironmentIdForWorktree(c.state, c.worktreeId) - expect(environmentId, c.name).toBe(owner.kind === 'runtime' ? owner.environmentId : null) - const value = - owner.kind === 'runtime' - ? owner.environmentId - : owner.kind === 'ambiguous' - ? 'ambiguous' - : null - return [c.name, value] - }) - ) - expect(actual).toEqual(LEGACY_RUNTIME_OWNER_GOLDEN) - }) -}) - -describe('resolveOwner on the deprecated resolvers’ blind spots', () => { - const byName = new Map(OWNER_RESOLUTION_CASES.map((c) => [c.name, c])) - const ownerOf = (name: string) => { - const c = byName.get(name) - if (!c?.worktreeId) { - throw new Error(`no case ${name}`) - } - return resolveOwner(c.state, { workspaceId: c.worktreeId }) - } - - it('refuses folder and project-group twins the deprecated resolver sends to the focused runtime', () => { - expect(LEGACY_RUNTIME_OWNER_GOLDEN['single focus | folder:twin-folder']).toBe('focused-env') - expect(ownerOf('single focus | folder:twin-folder')).toEqual({ kind: 'ambiguous' }) - expect(LEGACY_RUNTIME_OWNER_GOLDEN['single focus | folder:group-twin-folder']).toBe( - 'focused-env' - ) - expect(ownerOf('single focus | folder:group-twin-folder')).toEqual({ kind: 'ambiguous' }) - }) - - it('keeps the SSH target behind a server the deprecated resolver reduces to an environment id', () => { - expect(LEGACY_RUNTIME_OWNER_GOLDEN['focused | hub-repo::wt']).toBe('hub-env') - expect(ownerOf('focused | hub-repo::wt')).toEqual({ - kind: 'resolved', - owner: { endpoint: { kind: 'environment', environmentId: 'hub-env' }, at: 'ssh:box' } - }) - }) - - it('keeps git-worktree twins ambiguous, as the deprecated resolver already does', () => { - expect(ownerOf('focused | twin-repo::wt')).toEqual({ kind: 'ambiguous' }) - }) -}) diff --git a/src/renderer/src/lib/terminal-pty-owner-acceptance.test.ts b/src/renderer/src/lib/terminal-pty-owner-acceptance.test.ts new file mode 100644 index 00000000000..9d5d261ecac --- /dev/null +++ b/src/renderer/src/lib/terminal-pty-owner-acceptance.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from 'vitest' +import { resolveTerminalHostOwnership, resolveTerminalPtyOwner } from './terminal-worktree-route' +import { resolveOwner } from './resolve-owner' +import { OWNER_RESOLUTION_CASES } from './runtime-owner-alias-matrix.test-fixture' +import { LEGACY_RUNTIME_OWNER_GOLDEN } from './legacy-runtime-owner-answers.test-fixture' +import type { WorktreeRuntimeOwnerState } from './worktree-runtime-owner-state' + +const describeOwner = (owner: ReturnType) => + owner.kind === 'runtime' ? owner.environmentId : owner.kind === 'ambiguous' ? 'ambiguous' : null + +// Each deliberate change from the deleted resolver's teardown answer (round-3 design §3, §4, §7). +const DELIBERATE_CHANGES: Record = { + // C: teardown follows spawn, which already starts these PTYs on the server. + 'focused | stale-hub-repo::wt': 'hub-env', + 'no focus | stale-hub-repo::wt': 'hub-env', + 'single focus | stale-hub-repo::wt': 'hub-env', + // D77: a folder twin names no host; only each PTY's own `remote:` id may. + 'focused | folder:twin-folder': 'ambiguous', + 'no focus | folder:twin-folder': 'ambiguous', + 'single focus | folder:twin-folder': 'ambiguous', + 'focused | folder:group-twin-folder': 'ambiguous', + 'no focus | folder:group-twin-folder': 'ambiguous', + 'single focus | folder:group-twin-folder': 'ambiguous', + // B: two servers contest the selected SSH host, so spawn refuses and teardown must too. + 'active ssh:box | rival-repo::wt': 'ambiguous', + 'active ssh:box | conflict-repo::wt': 'ambiguous' +} + +describe('resolveTerminalPtyOwner acceptance', () => { + it('answers as the deleted resolver did, except the deliberate cases', () => { + const actual = Object.fromEntries( + OWNER_RESOLUTION_CASES.map((c) => [ + c.name, + describeOwner(resolveTerminalPtyOwner(c.state, c.worktreeId)) + ]) + ) + expect(actual).toEqual({ ...LEGACY_RUNTIME_OWNER_GOLDEN, ...DELIBERATE_CHANGES }) + for (const name of Object.keys(DELIBERATE_CHANGES)) { + expect(LEGACY_RUNTIME_OWNER_GOLDEN[name], name).not.toBe(DELIBERATE_CHANGES[name]) + } + }) + + it('keeps a setup terminal and a row-less worktree local under single focus', () => { + const single = OWNER_RESOLUTION_CASES.find((c) => c.name === 'single focus | legacy-repo::wt')! + expect(resolveTerminalPtyOwner(single.state, 'ephemeral-setup-terminal:skill-panel')).toEqual({ + kind: 'non-runtime' + }) + expect(resolveTerminalPtyOwner(single.state, 'unknown-repo::wt')).toEqual({ + kind: 'non-runtime' + }) + }) +}) + +describe('the contested-target rule for a selected ssh: host', () => { + const directAndProxied: WorktreeRuntimeOwnerState = { + settings: { activeRuntimeEnvironmentId: null }, + runtimeEnvironments: [{ id: 'hub-env' }], + runtimeEnvironmentCatalogHydrated: true, + repos: [ + { id: 'repo', connectionId: 'box', executionHostId: 'ssh:box' }, + { id: 'repo', connectionId: 'box', executionHostId: 'runtime:hub-env' } + ], + worktreesByRepo: { repo: [{ id: 'repo::wt', repoId: 'repo', hostId: 'ssh:box' }] }, + activeWorktreeId: 'repo::wt', + activeWorkspaceExecutionHostId: 'ssh:box' + } + + it('dials a proven direct listing directly although a server proxies the same target id', () => { + expect(resolveOwner(directAndProxied, { workspaceId: 'repo::wt' })).toEqual({ + kind: 'resolved', + owner: { endpoint: { kind: 'self' }, at: 'ssh:box' } + }) + expect(resolveTerminalHostOwnership(directAndProxied, 'repo::wt', 'spawn').kind).toBe( + 'local-or-ssh' + ) + expect(resolveTerminalPtyOwner(directAndProxied, 'repo::wt')).toEqual({ kind: 'non-runtime' }) + }) + + it('refuses when no direct repo row backs the listing', () => { + const proxiedTwice: WorktreeRuntimeOwnerState = { + ...directAndProxied, + repos: [ + { id: 'repo', connectionId: 'box', executionHostId: 'runtime:hub-a' }, + { id: 'repo', connectionId: 'box', executionHostId: 'runtime:hub-b' } + ] + } + expect(resolveOwner(proxiedTwice, { workspaceId: 'repo::wt' })).toEqual({ kind: 'ambiguous' }) + expect(resolveTerminalHostOwnership(proxiedTwice, 'repo::wt', 'spawn').kind).toBe('unresolved') + expect(resolveTerminalPtyOwner(proxiedTwice, 'repo::wt')).toEqual({ kind: 'ambiguous' }) + }) + + it('still dials a proven direct listing when two servers proxy the same target id', () => { + const directAndTwoProxies: WorktreeRuntimeOwnerState = { + ...directAndProxied, + repos: [ + { id: 'repo', connectionId: 'box', executionHostId: 'ssh:box' }, + { id: 'repo', connectionId: 'box', executionHostId: 'runtime:hub-a' }, + { id: 'repo', connectionId: 'box', executionHostId: 'runtime:hub-b' } + ] + } + expect(resolveOwner(directAndTwoProxies, { workspaceId: 'repo::wt' })).toEqual({ + kind: 'resolved', + owner: { endpoint: { kind: 'self' }, at: 'ssh:box' } + }) + expect(resolveTerminalPtyOwner(directAndTwoProxies, 'repo::wt')).toEqual({ + kind: 'non-runtime' + }) + }) +}) diff --git a/src/renderer/src/lib/terminal-worktree-route.ts b/src/renderer/src/lib/terminal-worktree-route.ts index de71d393163..948b8aaefb7 100644 --- a/src/renderer/src/lib/terminal-worktree-route.ts +++ b/src/renderer/src/lib/terminal-worktree-route.ts @@ -4,10 +4,19 @@ import { parseExecutionHostId } from '../../../shared/execution-host' import { parseWorkspaceKey } from '../../../shared/workspace-scope' import type { AppState } from '@/store/types' import { resolveOwnerWithLegacyFallback } from './worktree-runtime-owner' -import { runtimeEnvironmentIdUnlessTwin } from './resolve-owner' +import { namedHostForOwnerRef, runtimeEnvironmentIdUnlessTwin } from './resolve-owner' import type { WorktreeRuntimeOwnerState } from './worktree-runtime-owner-state' -import { resolveWorktreeOperationRouteResult } from './worktree-operation-route' +import { + resolveStrictWorktreeOperationRouteResultForHost, + resolveWorktreeOperationRouteResult, + type WorktreeOperationRouteResolution +} from './worktree-operation-route' import { getSingleFocusedRuntimeEnvironmentId } from './single-runtime-legacy-owner' +import { + hasIndexedDetectedWorktree, + resolveIndexedRepoOwner, + resolveIndexedWorktreeOwner +} from './worktree-runtime-owner-index' export type TerminalWorktreeRoute = { runtimeEnvironmentId: string | null @@ -86,7 +95,7 @@ export function resolveTerminalHostOwnership( getSingleFocusedRuntimeEnvironmentId(state) ) } - const resolution = resolveWorktreeOperationRouteResult(state, worktreeId) + const resolution = resolveGitWorktreeRoute(state, worktreeId) if (resolution.kind === 'resolved') { if (resolution.route.runtimeEnvironmentId) { // Why: a real worktree row keeps its runtime owner on teardown. Unlike the host-agnostic @@ -106,12 +115,86 @@ export function resolveTerminalHostOwnership( state.repos === undefined ) { // Why: narrow unit/legacy adapters can omit all owner catalogs; production stores always provide - // them and still fail closed above. Teardown never takes this fail-open — it would kill on a guess. + // them and still fail closed above. The `teardown` purpose never takes this fail-open. return ownershipForRuntimeEnvironmentId(getSingleFocusedRuntimeEnvironmentId(state)) } return UNRESOLVED_TERMINAL_HOST } +/** A named host takes the strict route, so rival servers for one `ssh:` target never read as direct. */ +function resolveGitWorktreeRoute( + state: WorktreeRuntimeOwnerState, + worktreeId: string +): WorktreeOperationRouteResolution { + const hostId = namedHostForOwnerRef(state, { workspaceId: worktreeId }) + return hostId + ? resolveStrictWorktreeOperationRouteResultForHost(state, worktreeId, hostId) + : resolveWorktreeOperationRouteResult(state, worktreeId) +} + +export type TerminalPtyOwner = + | { kind: 'runtime'; environmentId: string } + | { kind: 'non-runtime' } + | { kind: 'ambiguous' } + +const NON_RUNTIME_PTY_OWNER: TerminalPtyOwner = { kind: 'non-runtime' } +const AMBIGUOUS_PTY_OWNER: TerminalPtyOwner = { kind: 'ambiguous' } + +/** + * Where spawn started a worktree's PTYs, for teardown, close and PTY identity. A contested or + * unattributed-twin owner is `ambiguous`; an id no row places stays this app's, as it always has. + * `spawn` is for unscoped `remote:` ids, which live where spawn put them; `teardown` keeps a setup + * terminal's raw ids local, as they always have been. + */ +export function resolveTerminalPtyOwner( + state: WorktreeRuntimeOwnerState, + worktreeId: string | null | undefined, + purpose: TerminalHostOwnershipPurpose = 'teardown' +): TerminalPtyOwner { + if (!worktreeId) { + return NON_RUNTIME_PTY_OWNER + } + const setup = isEphemeralSetupTerminalWorktreeId(worktreeId) + if ( + !setup && + !namedHostForOwnerRef(state, { workspaceId: worktreeId }) && + isUnattributedTwinWorktree(state, worktreeId) + ) { + return AMBIGUOUS_PTY_OWNER + } + const ownership = resolveTerminalHostOwnership(state, worktreeId, setup ? purpose : 'spawn') + switch (ownership.kind) { + case 'runtime': + return { kind: 'runtime', environmentId: ownership.runtimeEnvironmentId } + case 'local-or-ssh': + return NON_RUNTIME_PTY_OWNER + case 'unresolved': + return setup || + (parseWorkspaceKey(worktreeId)?.type !== 'folder' && + resolveGitWorktreeRoute(state, worktreeId).kind === 'missing') + ? NON_RUNTIME_PTY_OWNER + : AMBIGUOUS_PTY_OWNER + } +} + +/** + * An unstamped worktree row with no detected copy, in a project two hosts publish: no row says + * which host started its PTYs, so only each PTY's own `remote:` id may. + */ +export function isUnattributedTwinWorktree( + state: WorktreeRuntimeOwnerState, + worktreeId: string +): boolean { + const worktree = resolveIndexedWorktreeOwner(state.worktreesByRepo, worktreeId) + return ( + worktree.kind === 'resolved' && + !worktree.owner.runtimeOwnerEnvironmentId?.trim() && + !parseExecutionHostId(worktree.owner.hostId) && + !hasIndexedDetectedWorktree(state.detectedWorktreesByRepo, worktreeId) && + resolveIndexedRepoOwner(state.repos, worktree.owner.repoId).kind === 'ambiguous' + ) +} + /** * Host ownership for the host-agnostic surfaces — floating, folder workspace, ephemeral setup. * Safe to downgrade on teardown because these never hold an `ssh:`-shaped HUB wake hint: a diff --git a/src/renderer/src/lib/worktree-operation-route.ts b/src/renderer/src/lib/worktree-operation-route.ts index 6cb3c3675e2..e6b0e434aed 100644 --- a/src/renderer/src/lib/worktree-operation-route.ts +++ b/src/renderer/src/lib/worktree-operation-route.ts @@ -1,5 +1,6 @@ import type { AppState } from '@/store/types' import { + getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID, parseExecutionHostId, type ExecutionHostId @@ -136,7 +137,8 @@ export function resolveWorktreeOperationRouteForHost( /** * Strict variant of the host-qualified resolver for callers that treat a null runtime as local: - * rival HUBs projecting the same host report `ambiguous` instead of a transport-less route. + * rival HUBs projecting the same host, or rows that contradict each other about its transport, + * report `ambiguous` instead of a transport-less route. */ export function resolveStrictWorktreeOperationRouteResultForHost( state: WorktreeOperationRouteState, @@ -149,13 +151,34 @@ export function resolveStrictWorktreeOperationRouteResultForHost( } if (host.kind === 'ssh') { const hubOwners = collectHubOwnerEnvironmentIds(state, worktreeId, host.id) - if (hubOwners.hasAmbiguousOwner || hubOwners.environmentIds.size > 1) { + if ( + hubOwners.environmentIds.size > 1 || + (hubOwners.hasAmbiguousOwner && !isDirectListing(state, worktreeId, host.id, hubOwners)) + ) { return { kind: 'ambiguous' } } } return { kind: 'resolved', route: resolveSelectedHostRoute(state, worktreeId, host) } } +/** A direct repo row backs a listing no row ties to a server, so a same-id proxy cannot claim it. */ +function isDirectListing( + state: WorktreeOperationRouteState, + worktreeId: string, + executionHostId: ExecutionHostId, + hubOwners: { environmentIds: Set } +): boolean { + const repoId = getRepoIdFromWorktreeId(worktreeId) + return ( + hubOwners.environmentIds.size === 0 && + Boolean( + state.repos?.some( + (repo) => repo.id === repoId && getRepoExecutionHostId(repo) === executionHostId + ) + ) + ) +} + function collectHubOwnerEnvironmentIds( state: WorktreeOperationRouteState, worktreeId: string, diff --git a/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts b/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts index 351d001cf04..6332bd39b68 100644 --- a/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts +++ b/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts @@ -7,7 +7,7 @@ import { waitForWorktreeAgentActivationGateForTests } from './worktree-agent-act import { makeCreatedAgentWorktree as makeWorktree } from '@/lib/worktree-activation-created-agent.test-fixture' import { makePaneKey } from '../../../shared/stable-pane-id' import type { ExecutionHostId } from '../../../shared/execution-host' -import { getRuntimeEnvironmentIdForWorktree } from './worktree-runtime-owner' +import { runtimeEnvironmentIdWithLegacyFallbackUnlessTwin } from './worktree-runtime-owner' import { markHostSessionMirrorHydrated, resetHostSessionMirrorHydrationForTests @@ -324,9 +324,9 @@ describe('preserved-pane replacement contract on workspace activation', () => { } })) - expect(getRuntimeEnvironmentIdForWorktree(useAppStore.getState(), worktree.id)).toBe( - RUNTIME_ENV_ID - ) + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(useAppStore.getState(), worktree.id) + ).toBe(RUNTIME_ENV_ID) activateAndRevealWorktree(worktree.id, { notifyHostRuntime: false }) await waitForWorktreeAgentActivationGateForTests(worktree.id) diff --git a/src/renderer/src/lib/worktree-runtime-owner.test.ts b/src/renderer/src/lib/worktree-runtime-owner.test.ts index 0f785d82799..296e9417166 100644 --- a/src/renderer/src/lib/worktree-runtime-owner.test.ts +++ b/src/renderer/src/lib/worktree-runtime-owner.test.ts @@ -3,9 +3,9 @@ import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' import { getExecutionHostIdForWorktree, getKnownExecutionHostIdForWorktree, - getRuntimeEnvironmentIdForWorktree, - resolveRuntimeOwnerForWorktree + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin } from './worktree-runtime-owner' +import { resolveTerminalPtyOwner } from './terminal-worktree-route' import { getRuntimeSessionMirrorEnvironmentIds } from './runtime-session-mirror-owners' import type { WorktreeRuntimeOwnerState } from './worktree-runtime-owner-state' import { runtimeEnvironmentIdForWorkspaceOwner } from './resolve-owner' @@ -44,13 +44,15 @@ const state: WorktreeRuntimeOwnerState = { ] } -describe('getRuntimeEnvironmentIdForWorktree', () => { +describe('runtimeEnvironmentIdWithLegacyFallbackUnlessTwin', () => { it('routes to the runtime owner of the worktree', () => { - expect(getRuntimeEnvironmentIdForWorktree(state, 'runtime-repo::wt-b')).toBe('owner-env') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, 'runtime-repo::wt-b')).toBe( + 'owner-env' + ) }) it('keeps explicit-local worktrees local even while a runtime is focused', () => { - expect(getRuntimeEnvironmentIdForWorktree(state, 'local-repo::wt-a')).toBeNull() + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, 'local-repo::wt-a')).toBeNull() }) it('keeps the synthetic floating workspace local while a runtime is focused', () => { @@ -60,7 +62,7 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { activeWorkspaceExecutionHostId: 'runtime:focused-env' } expect( - getRuntimeEnvironmentIdForWorktree(floatingState, FLOATING_TERMINAL_WORKTREE_ID) + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(floatingState, FLOATING_TERMINAL_WORKTREE_ID) ).toBeNull() expect(getKnownExecutionHostIdForWorktree(floatingState, FLOATING_TERMINAL_WORKTREE_ID)).toBe( 'local' @@ -71,7 +73,9 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { }) it('routes folder workspaces to their project group runtime owner', () => { - expect(getRuntimeEnvironmentIdForWorktree(state, 'folder:runtime-folder')).toBe('folder-env') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, 'folder:runtime-folder')).toBe( + 'folder-env' + ) expect(getExecutionHostIdForWorktree(state, 'folder:runtime-folder')).toBe('runtime:folder-env') }) @@ -106,9 +110,12 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { } } - expect(getRuntimeEnvironmentIdForWorktree(restoredFolderState, 'folder:restored-folder')).toBe( - 'restored-env' - ) + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin( + restoredFolderState, + 'folder:restored-folder' + ) + ).toBe('restored-env') expect(ownerEnvironmentId(restoredFolderState, 'folder:restored-folder')).toBe('restored-env') expect(getExecutionHostIdForWorktree(restoredFolderState, 'folder:restored-folder')).toBe( 'runtime:restored-env' @@ -116,7 +123,9 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { }) it('keeps explicit-local folder workspaces local even while a runtime is focused', () => { - expect(getRuntimeEnvironmentIdForWorktree(state, 'folder:local-folder')).toBeNull() + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, 'folder:local-folder') + ).toBeNull() expect(getExecutionHostIdForWorktree(state, 'folder:local-folder')).toBe('local') const restoredOwnerState: WorktreeRuntimeOwnerState = { @@ -125,7 +134,9 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { 'folder:local-folder': 'runtime:stale-env' } } - expect(getRuntimeEnvironmentIdForWorktree(restoredOwnerState, 'folder:local-folder')).toBeNull() + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(restoredOwnerState, 'folder:local-folder') + ).toBeNull() expect(getExecutionHostIdForWorktree(restoredOwnerState, 'folder:local-folder')).toBe('local') }) @@ -139,7 +150,7 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { } expect( - getRuntimeEnvironmentIdForWorktree(folderConnectionState, 'folder:folder-ssh') + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(folderConnectionState, 'folder:folder-ssh') ).toBeNull() expect(getExecutionHostIdForWorktree(folderConnectionState, 'folder:folder-ssh')).toBe( 'ssh:folder-remote' @@ -155,7 +166,10 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { } expect( - getRuntimeEnvironmentIdForWorktree(staleFolderConnectionState, 'folder:runtime-folder') + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin( + staleFolderConnectionState, + 'folder:runtime-folder' + ) ).toBe('folder-env') expect(getExecutionHostIdForWorktree(staleFolderConnectionState, 'folder:runtime-folder')).toBe( 'runtime:folder-env' @@ -163,7 +177,7 @@ describe('getRuntimeEnvironmentIdForWorktree', () => { }) }) -describe('resolveRuntimeOwnerForWorktree', () => { +describe('resolveTerminalPtyOwner', () => { it('names the runtime owner, a non-runtime owner, and keeps a twin ambiguous', () => { const twinState: WorktreeRuntimeOwnerState = { ...state, @@ -175,18 +189,18 @@ describe('resolveRuntimeOwnerForWorktree', () => { ] } } - expect(resolveRuntimeOwnerForWorktree(twinState, 'runtime-repo::wt-b')).toEqual({ + expect(resolveTerminalPtyOwner(twinState, 'runtime-repo::wt-b')).toEqual({ kind: 'runtime', environmentId: 'owner-env' }) - expect(resolveRuntimeOwnerForWorktree(twinState, 'local-repo::wt-a')).toEqual({ + expect(resolveTerminalPtyOwner(twinState, 'local-repo::wt-a')).toEqual({ kind: 'non-runtime' }) - expect(resolveRuntimeOwnerForWorktree(twinState, 'twin-repo::wt')).toEqual({ + expect(resolveTerminalPtyOwner(twinState, 'twin-repo::wt')).toEqual({ kind: 'ambiguous' }) - // Why: read-only callers keep the old collapse to "local". - expect(getRuntimeEnvironmentIdForWorktree(twinState, 'twin-repo::wt')).toBeNull() + // Why: read-only callers keep the old collapse to "no server". + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(twinState, 'twin-repo::wt')).toBeNull() }) }) @@ -214,9 +228,9 @@ describe('the explicit owner environment (resolveOwner)', () => { } expect(ownerEnvironmentId(nestedState, 'nested-repo::remote-worktree')).toBe('owner-hub') - expect(getRuntimeEnvironmentIdForWorktree(nestedState, 'nested-repo::remote-worktree')).toBe( - 'owner-hub' - ) + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(nestedState, 'nested-repo::remote-worktree') + ).toBe('owner-hub') expect(getExecutionHostIdForWorktree(nestedState, 'nested-repo::remote-worktree')).toBe( 'ssh:hub-private-ssh' ) @@ -335,8 +349,12 @@ describe('the explicit owner environment (resolveOwner)', () => { } } - expect(getRuntimeEnvironmentIdForWorktree(multiHubState, 'repo-a::ssh-worktree')).toBe('hub-a') - expect(getRuntimeEnvironmentIdForWorktree(multiHubState, 'repo-b::ssh-worktree')).toBe('hub-b') + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(multiHubState, 'repo-a::ssh-worktree') + ).toBe('hub-a') + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(multiHubState, 'repo-b::ssh-worktree') + ).toBe('hub-b') expect(getExecutionHostIdForWorktree(multiHubState, 'repo-a::ssh-worktree')).toBe( 'ssh:private-a' ) @@ -346,7 +364,9 @@ describe('the explicit owner environment (resolveOwner)', () => { }) it('does not treat the focused runtime as ownership for legacy-local worktrees', () => { - expect(getRuntimeEnvironmentIdForWorktree(state, 'legacy-repo::wt-legacy')).toBe('focused-env') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(state, 'legacy-repo::wt-legacy')).toBe( + 'focused-env' + ) expect(ownerEnvironmentId(state, 'legacy-repo::wt-legacy')).toBeNull() }) @@ -373,7 +393,10 @@ describe('the explicit owner environment (resolveOwner)', () => { expect(ownerEnvironmentId(hostOverrideState, 'runtime-repo::wt-local-override')).toBeNull() expect( - getRuntimeEnvironmentIdForWorktree(hostOverrideState, 'runtime-repo::wt-local-override') + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin( + hostOverrideState, + 'runtime-repo::wt-local-override' + ) ).toBeNull() expect( getExecutionHostIdForWorktree(hostOverrideState, 'runtime-repo::wt-local-override') @@ -382,7 +405,10 @@ describe('the explicit owner environment (resolveOwner)', () => { 'worktree-env' ) expect( - getRuntimeEnvironmentIdForWorktree(hostOverrideState, 'runtime-repo::wt-runtime-override') + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin( + hostOverrideState, + 'runtime-repo::wt-runtime-override' + ) ).toBe('worktree-env') expect( getExecutionHostIdForWorktree(hostOverrideState, 'runtime-repo::wt-runtime-override') @@ -469,10 +495,13 @@ describe('runtime owner identity indexes', () => { } for (let lookup = 0; lookup < 200; lookup += 1) { - getRuntimeEnvironmentIdForWorktree(indexedState, targetWorktreeId) + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(indexedState, targetWorktreeId) ownerEnvironmentId(indexedState, targetWorktreeId) getExecutionHostIdForWorktree(indexedState, targetWorktreeId) - getRuntimeEnvironmentIdForWorktree(indexedState, `folder:${targetFolderWorkspaceId}`) + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin( + indexedState, + `folder:${targetFolderWorkspaceId}` + ) ownerEnvironmentId(indexedState, `folder:${targetFolderWorkspaceId}`) getExecutionHostIdForWorktree(indexedState, `folder:${targetFolderWorkspaceId}`) } @@ -482,7 +511,9 @@ describe('runtime owner identity indexes', () => { expect(repoIdReads).toBe(200) expect(folderWorkspaceIdReads).toBe(1_000) expect(projectGroupIdReads).toBe(1_000) - expect(getRuntimeEnvironmentIdForWorktree(indexedState, targetWorktreeId)).toBe('repo-owner') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(indexedState, targetWorktreeId)).toBe( + 'repo-owner' + ) expect(getExecutionHostIdForWorktree(indexedState, `folder:${targetFolderWorkspaceId}`)).toBe( 'runtime:folder-owner' ) @@ -495,8 +526,10 @@ describe('runtime owner identity indexes', () => { folderWorkspaces: [{ id: 'folder', projectGroupId: 'group' }], projectGroups: [{ id: 'group', connectionId: null, executionHostId: 'runtime:folder-one' }] } - expect(getRuntimeEnvironmentIdForWorktree(first, 'worktree')).toBe('repo-one') - expect(getRuntimeEnvironmentIdForWorktree(first, 'folder:folder')).toBe('folder-one') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(first, 'worktree')).toBe('repo-one') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(first, 'folder:folder')).toBe( + 'folder-one' + ) const second: WorktreeRuntimeOwnerState = { repos: [{ id: 'repo', connectionId: null, executionHostId: 'runtime:repo-two' }], @@ -506,8 +539,12 @@ describe('runtime owner identity indexes', () => { folderWorkspaces: [{ id: 'folder', projectGroupId: 'group' }], projectGroups: [{ id: 'group', connectionId: null, executionHostId: 'runtime:folder-two' }] } - expect(getRuntimeEnvironmentIdForWorktree(second, 'worktree')).toBe('worktree-two') - expect(getRuntimeEnvironmentIdForWorktree(second, 'folder:folder')).toBe('folder-two') + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(second, 'worktree')).toBe( + 'worktree-two' + ) + expect(runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(second, 'folder:folder')).toBe( + 'folder-two' + ) }) }) @@ -609,7 +646,9 @@ describe('active workspace host selection', () => { } it('keeps the HUB transport for the active paired SSH worktree', () => { - expect(getRuntimeEnvironmentIdForWorktree(pairedHubState, PAIRED_HUB_WORKTREE_ID)).toBe('hub-a') + expect( + runtimeEnvironmentIdWithLegacyFallbackUnlessTwin(pairedHubState, PAIRED_HUB_WORKTREE_ID) + ).toBe('hub-a') expect(ownerEnvironmentId(pairedHubState, PAIRED_HUB_WORKTREE_ID)).toBe('hub-a') }) diff --git a/src/renderer/src/lib/worktree-runtime-owner.ts b/src/renderer/src/lib/worktree-runtime-owner.ts index f7b6bba946e..5b0ab0329b6 100644 --- a/src/renderer/src/lib/worktree-runtime-owner.ts +++ b/src/renderer/src/lib/worktree-runtime-owner.ts @@ -12,6 +12,7 @@ import { findIndexedRepoOwner as findRepoRecord, findIndexedWorktreeOwner as findWorktreeRecord, hasIndexedDetectedWorktree, + resolveIndexedProjectGroupOwner, resolveIndexedRepoOwner, resolveIndexedWorktreeOwner } from './worktree-runtime-owner-index' @@ -32,7 +33,6 @@ import { SELF_LOCAL_AUTHORITY, type HostAuthority } from '../../../shared/host-a import type { ExecutionHostOwnerMatch } from '../../../shared/worktree-execution-host-resolution' import { getFloatingWorkspaceOperationRoute, - resolveActiveWorkspaceRoute, resolveWorktreeOperationRouteResult } from './worktree-operation-route' import { resolveExplicitWorktreeOperationRouteResult } from './worktree-operation-catalog-route' @@ -59,59 +59,22 @@ function getActiveWorkspaceExecutionHostId( : null } -export type WorktreeRuntimeOwner = - | { kind: 'runtime'; environmentId: string } - | { kind: 'non-runtime' } - // Why: the same repo id and path is published by two hosts (e.g. mid ssh-to-runtime migration). - | { kind: 'ambiguous' } - -const NON_RUNTIME_OWNER: WorktreeRuntimeOwner = { kind: 'non-runtime' } -const AMBIGUOUS_OWNER: WorktreeRuntimeOwner = { kind: 'ambiguous' } - -function toRuntimeOwner(environmentId: string | null | undefined): WorktreeRuntimeOwner { - return environmentId ? { kind: 'runtime', environmentId } : NON_RUNTIME_OWNER -} - /** - * @deprecated Use `resolveOwner` (`./resolve-owner`), which keeps the host and reports twins as - * `ambiguous`. Here ambiguous collapses to `null`, which callers read as local; read-only use only. + * The server the pre-`resolveOwner` resolver picked for a row that names no host (`null`: this + * app or a twin). Only {@link resolveOwnerWithLegacyFallback} reads it, after a named host, + * a twin and every stamped row have already been answered. */ -export function getRuntimeEnvironmentIdForWorktree( +function legacyRuntimeEnvironmentId( state: WorktreeRuntimeOwnerState, - worktreeId: string | null | undefined + worktreeId: string ): string | null { - const owner = resolveRuntimeOwnerForWorktree(state, worktreeId) - return owner.kind === 'runtime' ? owner.environmentId : null -} - -/** - * @deprecated Use `resolveOwner` (`./resolve-owner`). This drops the host, reads local, SSH and - * unknown alike as `non-runtime`, and sends folder twins to the focused runtime. - */ -export function resolveRuntimeOwnerForWorktree( - state: WorktreeRuntimeOwnerState, - worktreeId: string | null | undefined -): WorktreeRuntimeOwner { - if (!worktreeId) { - return NON_RUNTIME_OWNER - } - const floatingRoute = getFloatingWorkspaceOperationRoute(worktreeId) - if (floatingRoute) { - return toRuntimeOwner(floatingRoute.runtimeEnvironmentId) - } - const activeRoute = resolveActiveWorkspaceRoute(state, worktreeId) - if (activeRoute) { - return toRuntimeOwner(activeRoute.runtimeEnvironmentId) - } const workspaceScope = parseWorkspaceKey(worktreeId) if (workspaceScope?.type === 'folder') { - return toRuntimeOwner( - getRuntimeEnvironmentIdForFolderWorkspace(state, workspaceScope.folderWorkspaceId) - ) + return getRuntimeEnvironmentIdForFolderWorkspace(state, workspaceScope.folderWorkspaceId) } const indexedOwner = resolveIndexedWorktreeOwner(state.worktreesByRepo, worktreeId) if (indexedOwner.kind === 'ambiguous') { - return AMBIGUOUS_OWNER + return null } if (indexedOwner.kind === 'resolved') { const owner = indexedOwner.owner @@ -121,15 +84,16 @@ export function resolveRuntimeOwnerForWorktree( // Why: a row on a web client that names no server is still reached through its only one. const onlyHost = getPairedWebClientEnvironmentId(state) if (!hasDetectedOwner && (projectedRuntimeOwner || parsedHost)) { - return toRuntimeOwner( + return ( projectedRuntimeOwner || - (parsedHost?.kind === 'runtime' ? parsedHost.environmentId : onlyHost) + (parsedHost?.kind === 'runtime' ? parsedHost.environmentId : onlyHost) || + null ) } if (!hasDetectedOwner) { const repoResolution = resolveIndexedRepoOwner(state.repos, owner.repoId) if (repoResolution.kind === 'ambiguous') { - return AMBIGUOUS_OWNER + return null } if ( repoResolution.kind === 'resolved' && @@ -137,18 +101,13 @@ export function resolveRuntimeOwnerForWorktree( ) { const repoHost = parseExecutionHostId(getRepoExecutionHostId(repoResolution.owner)) if (repoHost) { - return toRuntimeOwner(repoHost.kind === 'runtime' ? repoHost.environmentId : onlyHost) + return (repoHost.kind === 'runtime' ? repoHost.environmentId : onlyHost) || null } } } } const resolution = resolveWorktreeOperationRouteResult(state, worktreeId) - if (resolution.kind === 'ambiguous') { - return AMBIGUOUS_OWNER - } - return resolution.kind === 'resolved' - ? toRuntimeOwner(resolution.route.runtimeEnvironmentId) - : NON_RUNTIME_OWNER + return resolution.kind === 'resolved' ? resolution.route.runtimeEnvironmentId || null : null } function getFocusedRuntimeOrLocalExecutionHostId( @@ -237,6 +196,12 @@ function hasRowHost(row: { executionHostId?: string | null; connectionId?: strin return Boolean(row.executionHostId?.trim() || row.connectionId?.trim()) } +/** Through the owner index: rows on two hosts always include a stamped one. */ +function hasStampedRepoRow(state: WorktreeRuntimeOwnerState, repoId: string): boolean { + const repo = resolveIndexedRepoOwner(state.repos, repoId) + return repo.kind === 'ambiguous' || (repo.kind === 'resolved' && hasRowHost(repo.owner)) +} + /** A row that names its host; without one the deprecated resolver's guess still applies. */ function hasHostEvidence(state: WorktreeRuntimeOwnerState, workspaceId: string): boolean { if (getFloatingWorkspaceOperationRoute(workspaceId)) { @@ -245,19 +210,17 @@ function hasHostEvidence(state: WorktreeRuntimeOwnerState, workspaceId: string): const scope = parseWorkspaceKey(workspaceId) if (scope?.type === 'folder') { const folder = findFolderWorkspaceOwner(state, scope.folderWorkspaceId) - return Boolean( - folder && - (hasRowHost(folder) || - state.projectGroups?.some( - (group) => group.id === folder.projectGroupId && hasRowHost(group) - )) - ) + if (!folder || hasRowHost(folder)) { + return Boolean(folder) + } + const group = resolveIndexedProjectGroupOwner(state.projectGroups, folder.projectGroupId) + return group.kind === 'ambiguous' || (group.kind === 'resolved' && hasRowHost(group.owner)) } const repoId = getRepoIdFromWorktreeId(workspaceId) return ( someWorktreeCopy(state, workspaceId, (row) => Boolean(row.hostId?.trim() || row.runtimeOwnerEnvironmentId?.trim()) - ) || Boolean(state.repos?.some((repo) => repo.id === repoId && hasRowHost(repo))) + ) || hasStampedRepoRow(state, repoId) ) } @@ -278,7 +241,7 @@ export function resolveOwnerWithLegacyFallback( ) { return match } - const legacyEnvironmentId = getRuntimeEnvironmentIdForWorktree(state, ref.workspaceId) + const legacyEnvironmentId = legacyRuntimeEnvironmentId(state, ref.workspaceId) if (legacyEnvironmentId) { return { kind: 'resolved', diff --git a/src/renderer/src/store/slices/terminal-pane-retirement-ownership.test.ts b/src/renderer/src/store/slices/terminal-pane-retirement-ownership.test.ts index ee21460d8c7..55b15c2be23 100644 --- a/src/renderer/src/store/slices/terminal-pane-retirement-ownership.test.ts +++ b/src/renderer/src/store/slices/terminal-pane-retirement-ownership.test.ts @@ -83,3 +83,27 @@ it.each(['row', 'layout', 'aggregate', 'relay', 'deferred', 'reconnect'] as cons expect(hasTerminalPtyOwnerOutsidePane(current, identity, 'closing', 'original')).toBe(false) } ) + +it('keys a legacy id where spawn put it: exact for an unattributed twin, the runtime for setup', () => { + const twin: TerminalTabRetirementState = { + ...state(), + repos: [ + { id: 'repo', connectionId: null, executionHostId: 'local' }, + { id: 'repo', connectionId: null, executionHostId: 'runtime:env-1' } + ], + worktreesByRepo: { repo: [{ id: 'wt', repoId: 'repo' }] } + } + expect(getTerminalPtyOwnershipIdentity(twin, 'remote:session', 'wt')).toBe('pty:remote:session') + const focused: TerminalTabRetirementState = { + ...state(), + settings: { activeRuntimeEnvironmentId: 'env-1' }, + runtimeEnvironments: [{ id: 'env-1' }] + } + expect( + getTerminalPtyOwnershipIdentity( + focused, + 'remote:session', + 'ephemeral-setup-terminal:skill-panel' + ) + ).toBe(JSON.stringify(['runtime', 'env-1', 'session'])) +}) diff --git a/src/renderer/src/store/slices/terminal-tab-retirement.ts b/src/renderer/src/store/slices/terminal-tab-retirement.ts index 65f79614784..2df722c78a2 100644 --- a/src/renderer/src/store/slices/terminal-tab-retirement.ts +++ b/src/renderer/src/store/slices/terminal-tab-retirement.ts @@ -4,9 +4,11 @@ import { } from '../../../../shared/remote-runtime-pty-id' import type { SleepingAgentSessionRecord } from '../../../../shared/agent-session-resume' import type { AppState } from '../types' -import { resolveRuntimeOwnerForWorktree } from '@/lib/worktree-runtime-owner' import type { WorktreeRuntimeOwnerState } from '@/lib/worktree-runtime-owner-state' -import { resolveTerminalHostOwnership } from '@/lib/terminal-worktree-route' +import { + resolveTerminalHostOwnership, + resolveTerminalPtyOwner +} from '@/lib/terminal-worktree-route' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' import { isEphemeralSetupTerminalWorktreeId } from '../../../../shared/ephemeral-setup-terminal-worktree-id' import { parseWorkspaceKey } from '../../../../shared/workspace-scope' @@ -61,9 +63,9 @@ export function getTerminalPtyOwnershipIdentity( return JSON.stringify(['runtime', scopedEnvironmentId, remote.handle]) } // Why: hydrated legacy runtime IDs omit their owner, but still refer to the same provider - // terminal as a scoped ID in the owning worktree. This reads the owner the way teardown's - // focus-aware fallback route does, so identity never disagrees with what a close retires. - const owner = resolveRuntimeOwnerForWorktree(state, worktreeId) + // terminal as a scoped ID in the owning worktree. This reads the owner spawn started it on, as + // close does, so identity never disagrees with what a close retires. + const owner = resolveTerminalPtyOwner(state, worktreeId, 'spawn') // Why: a twin's legacy id can't be pinned to either host, so it matches only its own exact id. if (owner.kind === 'ambiguous') { return `pty:${ptyId}` diff --git a/src/renderer/src/store/terminals/terminal-pane-hibernation.ts b/src/renderer/src/store/terminals/terminal-pane-hibernation.ts index 39bf2fc372c..dc052b80a5e 100644 --- a/src/renderer/src/store/terminals/terminal-pane-hibernation.ts +++ b/src/renderer/src/store/terminals/terminal-pane-hibernation.ts @@ -13,7 +13,7 @@ import { } from '../slices/agent-status' import type { TerminalSlice, TerminalStoreGet, TerminalStoreSet } from './terminal-state' import { equalStringSets, sortedUniquePtyIds } from './terminal-pty-identities' -import { resolveRuntimeOwnerForWorktree } from '@/lib/worktree-runtime-owner' +import { resolveTerminalPtyOwner } from '@/lib/terminal-worktree-route' import { resolveTerminalTeardownOwner } from './terminal-teardown-owner' import { removePanesAttention } from '../slices/workspace-attention/subject-attention-transitions' import { settleAttentionRemovedBy } from '../slices/workspace-attention/attention-removal-follow-up' @@ -32,7 +32,7 @@ export function createTerminalPaneHibernationActions( ) const rendererShutdownPtyIds = [opts.ptyId] const state = get() - const worktreeOwner = resolveRuntimeOwnerForWorktree(state, worktreeId) + const worktreeOwner = resolveTerminalPtyOwner(state, worktreeId) const teardownOwner = resolveTerminalTeardownOwner(worktreeOwner, [opts.ptyId]) // Why: a twin-published worktree must route by the PTY's own owner; a remote PTY with no // host stop to run would otherwise lose its binding while the agent keeps running. diff --git a/src/renderer/src/store/terminals/terminal-shutdown.ts b/src/renderer/src/store/terminals/terminal-shutdown.ts index 926170fe5db..68bfd8488e8 100644 --- a/src/renderer/src/store/terminals/terminal-shutdown.ts +++ b/src/renderer/src/store/terminals/terminal-shutdown.ts @@ -9,7 +9,7 @@ import { } from '../slices/agent-status' import type { TerminalSlice, TerminalStoreGet, TerminalStoreSet } from './terminal-state' import { equalStringSets, sortedUniquePtyIds } from './terminal-pty-identities' -import { resolveRuntimeOwnerForWorktree } from '@/lib/worktree-runtime-owner' +import { resolveTerminalPtyOwner } from '@/lib/terminal-worktree-route' import { resolveTerminalTeardownOwner } from './terminal-teardown-owner' import { createTerminalShutdownGuardController } from './terminal-shutdown-guards' import { commitTerminalShutdownState } from './terminal-shutdown-state' @@ -39,7 +39,7 @@ export function createTerminalShutdownActions( const expectedRuntimePtyIds = sortedUniquePtyIds(opts?.expectedRuntimePtyIds) const backendOwnsPtyTeardown = opts?.backendOwnsPtyTeardown === true const teardownOwner = resolveTerminalTeardownOwner( - resolveRuntimeOwnerForWorktree(get(), worktreeId), + resolveTerminalPtyOwner(get(), worktreeId), rendererShutdownPtyIds ) // Why: treating an unattributable owner as local skips the host stop yet still clears diff --git a/src/renderer/src/store/terminals/terminal-tab-close-providers.test.ts b/src/renderer/src/store/terminals/terminal-tab-close-providers.test.ts new file mode 100644 index 00000000000..82c778efa13 --- /dev/null +++ b/src/renderer/src/store/terminals/terminal-tab-close-providers.test.ts @@ -0,0 +1,71 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { TerminalPtyOwner } from '@/lib/terminal-worktree-route' +import { createTestStore } from '../slices/store-test-helpers' +import { startTerminalTabProviderRetirement } from './terminal-tab-close-providers' + +const mocks = vi.hoisted(() => { + const ptyOwner: { current: TerminalPtyOwner } = { current: { kind: 'non-runtime' } } + return { ptyOwner, callRuntimeRpc: vi.fn(async () => undefined) } +}) + +vi.mock('@/lib/terminal-worktree-route', () => ({ + resolveTerminalPtyOwner: () => mocks.ptyOwner.current, + resolveTerminalWorktreeRoute: () => ({ runtimeEnvironmentId: 'env-a' }) +})) + +vi.mock('@/runtime/runtime-rpc-client', () => ({ + callRuntimeRpc: mocks.callRuntimeRpc, + ensureRuntimeEnvironmentCompatible: vi.fn() +})) + +function retire(): void { + startTerminalTabProviderRetirement({ + localPtyTeardownOwnedExternally: false, + remoteCloseOwnedByHost: false, + state: createTestStore().getState(), + tabId: 'tab-1', + retirementPlan: { + tabId: 'tab-1', + worktreeId: 'repo::wt', + ptyIds: ['remote:handle-1'], + localOrSshPtyIds: [], + runtimeTerminals: [{ ptyId: 'remote:handle-1', environmentId: null, handle: 'handle-1' }], + cleanupOnlyPtyIds: [], + sharedPtyIds: [], + unroutablePtyIds: [] + } + }) +} + +describe('closing an unscoped remote terminal id', () => { + let warn: ReturnType + beforeEach(() => { + mocks.callRuntimeRpc.mockClear() + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => warn.mockRestore()) + + it('closes it where spawn started it', () => { + mocks.ptyOwner.current = { kind: 'runtime', environmentId: 'env-a' } + retire() + expect(mocks.callRuntimeRpc).toHaveBeenCalledWith( + { kind: 'environment', environmentId: 'env-a' }, + 'terminal.close', + { terminal: 'handle-1' } + ) + }) + + it('skips and logs it, never closing on this app, when the owner is ambiguous', () => { + mocks.ptyOwner.current = { kind: 'ambiguous' } + retire() + expect(mocks.callRuntimeRpc).not.toHaveBeenCalled() + expect(warn).toHaveBeenCalledWith( + '[terminal-retirement] skipped PTYs with no resolvable owner', + { + tabId: 'tab-1', + worktreeKind: expect.any(String), + count: 1 + } + ) + }) +}) diff --git a/src/renderer/src/store/terminals/terminal-tab-close-providers.ts b/src/renderer/src/store/terminals/terminal-tab-close-providers.ts index 4ba425d95d2..03f4f7ddac5 100644 --- a/src/renderer/src/store/terminals/terminal-tab-close-providers.ts +++ b/src/renderer/src/store/terminals/terminal-tab-close-providers.ts @@ -4,7 +4,10 @@ import { } from '@/runtime/runtime-environment-revision' import type { AppState } from '../types' import { callRuntimeRpc, ensureRuntimeEnvironmentCompatible } from '@/runtime/runtime-rpc-client' -import { resolveTerminalWorktreeRoute } from '@/lib/terminal-worktree-route' +import { + resolveTerminalPtyOwner, + resolveTerminalWorktreeRoute +} from '@/lib/terminal-worktree-route' import { classifyTerminalRetirementWorktree, type TerminalTabRetirementPlan @@ -25,16 +28,22 @@ export function startTerminalTabProviderRetirement({ tabId: string canRetireRuntimeTerminal?: () => boolean }): void { - const fallbackWorktreeRoute = retirementPlan.worktreeId - ? resolveTerminalWorktreeRoute(state, retirementPlan.worktreeId) - : { runtimeEnvironmentId: null } + const { worktreeId } = retirementPlan + // Why: an unscoped `remote:` id of an unattributable twin must not be closed on this app. + const fallbackWorktreeRoute = !worktreeId + ? { runtimeEnvironmentId: null } + : resolveTerminalPtyOwner(state, worktreeId, 'spawn').kind === 'ambiguous' + ? null + : resolveTerminalWorktreeRoute(state, worktreeId) const retirementTasks: Promise[] = localPtyTeardownOwnedExternally ? [] : retirementPlan.localOrSshPtyIds.map(async (ptyId) => window.api.pty.kill(ptyId)) const localOrSshTaskCount = retirementTasks.length + let unroutableRuntimeTerminalCount = 0 if (!remoteCloseOwnedByHost) { for (const terminal of retirementPlan.runtimeTerminals) { if (!terminal.environmentId && !fallbackWorktreeRoute) { + unroutableRuntimeTerminalCount += 1 continue } const environmentId = terminal.environmentId ?? fallbackWorktreeRoute?.runtimeEnvironmentId @@ -43,12 +52,13 @@ export function startTerminalTabProviderRetirement({ ) } } - if (retirementPlan.unroutablePtyIds.length > 0) { + const unroutableCount = retirementPlan.unroutablePtyIds.length + unroutableRuntimeTerminalCount + if (unroutableCount > 0) { // Log the worktree shape, never its id, because worktree ids embed absolute paths. console.warn('[terminal-retirement] skipped PTYs with no resolvable owner', { tabId, worktreeKind: classifyTerminalRetirementWorktree(retirementPlan.worktreeId), - count: retirementPlan.unroutablePtyIds.length + count: unroutableCount }) } // Close remains synchronous; provider failures cannot block ownership revocation. diff --git a/src/renderer/src/store/terminals/terminal-teardown-owner.ts b/src/renderer/src/store/terminals/terminal-teardown-owner.ts index b2c1c9fc49a..21d3f32682e 100644 --- a/src/renderer/src/store/terminals/terminal-teardown-owner.ts +++ b/src/renderer/src/store/terminals/terminal-teardown-owner.ts @@ -2,7 +2,7 @@ import { isRemoteRuntimePtyId, parseRemoteRuntimePtyId } from '../../../../shared/remote-runtime-pty-id' -import type { WorktreeRuntimeOwner } from '@/lib/worktree-runtime-owner' +import type { TerminalPtyOwner } from '@/lib/terminal-worktree-route' export type TerminalTeardownOwner = | { kind: 'resolved'; runtimeEnvironmentId: string | null } @@ -14,7 +14,7 @@ export type TerminalTeardownOwner = * attributed, so teardown is refused rather than treated as local. */ export function resolveTerminalTeardownOwner( - worktreeOwner: WorktreeRuntimeOwner, + worktreeOwner: TerminalPtyOwner, ptyIds: readonly string[] ): TerminalTeardownOwner { if (worktreeOwner.kind === 'runtime') { diff --git a/tests/e2e/codex-child-approval-activity-row.unit.test.ts b/tests/e2e/codex-child-approval-activity-row.unit.test.ts index 9b1dc2306e2..ad56b6c76cc 100644 --- a/tests/e2e/codex-child-approval-activity-row.unit.test.ts +++ b/tests/e2e/codex-child-approval-activity-row.unit.test.ts @@ -20,7 +20,6 @@ import type { AgentJournalItemBody } from '../../src/shared/agent-session-journa import { parseAgentJournalItemKey } from '../../src/shared/agent-session-journal-item-key' import type { Tab } from '../../src/shared/tab-types' import type { AppState } from '../../src/renderer/src/store/types' -import type * as WorktreeRuntimeOwnerModule from '../../src/renderer/src/lib/worktree-runtime-owner' import { createCodexJournalTranslator } from '../../src/main/codex/codex-structured-journal-translation' import { CODEX_COMMAND_APPROVAL_METHOD } from '../../src/main/codex/codex-structured-prompt-replies' import { createTrackedJournalOpener } from '../../src/main/native-chat/agent-session-journal/journal-host-database-test-support' @@ -59,13 +58,6 @@ vi.mock('@/store', async () => { return { useAppStore } }) -// Partial: the status projection also resolves each chat's owner from the worktree. -vi.mock('@/lib/worktree-runtime-owner', async (importOriginal) => ({ - ...(await importOriginal()), - getRuntimeEnvironmentIdForWorktree: (state: { testRuntimeOwner?: string | null }) => - state.testRuntimeOwner ?? null -})) - vi.mock('@/runtime/structured-agent-session-client', () => ({ callStructuredAgentSession: vi.fn(), subscribeStructuredAgentSession: vi.fn(),