From 801ff57e83f8af774490443a8ad446102d258a29 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Thu, 23 Jul 2026 14:23:44 -0700 Subject: [PATCH 01/10] fix(mobile): unblock iOS releases (#10224) Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .github/workflows/mobile-ios-release.yml | 2 +- .github/workflows/mobile.yml | 8 +++ mobile/fastlane/Fastfile | 50 +++++++++---------- mobile/fastlane/ios_release_version.rb | 24 +++++++++ mobile/fastlane/ios_release_version_test.rb | 48 ++++++++++++++++++ ...le-native-chat-image-transcript-markers.ts | 6 +-- .../native-chat-pending-occurrence.ts | 2 +- .../native-chat-session-assembler.ts | 2 +- ...tive-chat-image-transcript-markers.test.ts | 2 +- .../native-chat-image-transcript-markers.ts | 6 +-- 10 files changed, 111 insertions(+), 39 deletions(-) create mode 100644 mobile/fastlane/ios_release_version.rb create mode 100644 mobile/fastlane/ios_release_version_test.rb rename src/{renderer/src/components/native-chat => shared}/native-chat-image-transcript-markers.test.ts (95%) rename src/{renderer/src/components/native-chat => shared}/native-chat-image-transcript-markers.ts (95%) diff --git a/.github/workflows/mobile-ios-release.yml b/.github/workflows/mobile-ios-release.yml index b38a79b9267..f759bd3efee 100644 --- a/.github/workflows/mobile-ios-release.yml +++ b/.github/workflows/mobile-ios-release.yml @@ -10,7 +10,7 @@ on: workflow_dispatch: inputs: bump_patch_version: - description: 'Bump the iOS marketing version patch number before release. Tick this after a version has shipped to the App Store (the release fails fast if the current version''s train is already closed).' + description: 'Use the first open iOS patch version after the checked-in version, skipping versions already closed on the App Store.' required: false default: false type: boolean diff --git a/.github/workflows/mobile.yml b/.github/workflows/mobile.yml index 2998d1dcce7..e9683904be2 100644 --- a/.github/workflows/mobile.yml +++ b/.github/workflows/mobile.yml @@ -30,6 +30,11 @@ jobs: with: node-version-file: package.json + - name: Setup Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.3' + - name: Setup pnpm uses: pnpm/action-setup@v6 with: @@ -56,6 +61,9 @@ jobs: - name: Test run: pnpm test + - name: Test iOS release version resolution + run: ruby fastlane/ios_release_version_test.rb + - name: Lint run: pnpm lint diff --git a/mobile/fastlane/Fastfile b/mobile/fastlane/Fastfile index a8ccf9b8c49..3ceb91c0d97 100644 --- a/mobile/fastlane/Fastfile +++ b/mobile/fastlane/Fastfile @@ -16,6 +16,7 @@ require "base64" require "json" +require_relative "ios_release_version" default_platform(:ios) @@ -66,25 +67,6 @@ def current_mobile_version(config) config.fetch("expo").fetch("version") end -def truthy_option?(value) - %w[1 true yes on].include?(value.to_s.strip.downcase) -end - -def bump_patch_version(version) - match = version.match(/\A(\d+)\.(\d+)\.(\d+)\z/) - UI.user_error!("Cannot bump non-semver mobile version '#{version}'") unless match - - "#{match[1]}.#{match[2]}.#{match[3].to_i + 1}" -end - -def resolve_requested_version(options, config) - requested = options[:version].to_s.strip - return requested unless requested.empty? - - current_version = current_mobile_version(config) - truthy_option?(options[:bump_patch]) ? bump_patch_version(current_version) : current_version -end - # Returns true when `version`'s App Store train is closed to new build uploads. # `app` is a Spaceship::ConnectAPI::App the caller looks up (nil if lookup # failed). On a nil app or any API error, degrade to "open": we then proceed as @@ -113,13 +95,7 @@ platform :ios do lane :prepare_release_version do |options| api_key = app_store_connect_api_key_from_env config = load_mobile_app_config - version = resolve_requested_version(options, config) - # Fail fast (seconds) if the resolved version's App Store train is already - # closed: Apple would otherwise reject the upload ~20 min later with 90186. - # Bumping the version is left to the human (the bump_patch input or a - # "Prepare mobile X" commit) so the marketing version stays a deliberate, - # release-notes-bearing decision rather than something CI invents. app = begin Spaceship::ConnectAPI::App.find(BUNDLE_ID) @@ -127,11 +103,31 @@ platform :ios do UI.error("Could not look up App Store app #{BUNDLE_ID} (#{error.message}); skipping closed-train check.") nil end + version = + begin + IosReleaseVersion.resolve( + requested: options[:version], + bump_patch: options[:bump_patch], + current_version: current_mobile_version(config), + train_closed: ->(candidate) { version_train_closed?(app, candidate) }, + ) + rescue ArgumentError => error + UI.user_error!(error.message) + end + + # Explicit and checked-in versions still fail fast when closed. Patch bumps + # skip closed trains above because workflow-only releases can outpace Git. if version_train_closed?(app, version) + retry_guidance = + if IosReleaseVersion.truthy?(options[:bump_patch]) + "Use a higher release_version or land a \"Prepare mobile \" commit." + else + "Re-dispatch with bump_patch_version: true (or a higher release_version), " \ + "or land a \"Prepare mobile \" commit." + end UI.user_error!( "iOS version #{version} is already submitted/released on the App Store and cannot accept " \ - "new builds. Re-dispatch with bump_patch_version: true (or a higher release_version), " \ - "or land a \"Prepare mobile \" commit.", + "new builds. #{retry_guidance}", ) end diff --git a/mobile/fastlane/ios_release_version.rb b/mobile/fastlane/ios_release_version.rb new file mode 100644 index 00000000000..2b4fda40e06 --- /dev/null +++ b/mobile/fastlane/ios_release_version.rb @@ -0,0 +1,24 @@ +module IosReleaseVersion + module_function + + def truthy?(value) + %w[1 true yes on].include?(value.to_s.strip.downcase) + end + + def bump_patch(version) + match = version.match(/\A(\d+)\.(\d+)\.(\d+)\z/) + raise ArgumentError, "Cannot bump non-semver mobile version '#{version}'" unless match + + "#{match[1]}.#{match[2]}.#{match[3].to_i + 1}" + end + + def resolve(requested:, bump_patch:, current_version:, train_closed:) + exact_version = requested.to_s.strip + return exact_version unless exact_version.empty? + return current_version unless truthy?(bump_patch) + + candidate = bump_patch(current_version) + candidate = bump_patch(candidate) while train_closed.call(candidate) + candidate + end +end diff --git a/mobile/fastlane/ios_release_version_test.rb b/mobile/fastlane/ios_release_version_test.rb new file mode 100644 index 00000000000..795e6748275 --- /dev/null +++ b/mobile/fastlane/ios_release_version_test.rb @@ -0,0 +1,48 @@ +require "minitest/autorun" +require_relative "ios_release_version" + +class IosReleaseVersionTest < Minitest::Test + def test_exact_version_wins_without_checking_trains + checked_versions = [] + + version = IosReleaseVersion.resolve( + requested: " 0.0.40 ", + bump_patch: true, + current_version: "0.0.32", + train_closed: ->(candidate) { checked_versions << candidate }, + ) + + assert_equal("0.0.40", version) + assert_empty(checked_versions) + end + + def test_uses_current_version_without_a_patch_bump + version = IosReleaseVersion.resolve( + requested: "", + bump_patch: false, + current_version: "0.0.32", + train_closed: ->(_) { flunk("should not check trains") }, + ) + + assert_equal("0.0.32", version) + end + + def test_skips_closed_patch_versions_from_a_stale_repo_version + closed_versions = %w[0.0.33 0.0.34] + + version = IosReleaseVersion.resolve( + requested: "", + bump_patch: true, + current_version: "0.0.32", + train_closed: ->(candidate) { closed_versions.include?(candidate) }, + ) + + assert_equal("0.0.35", version) + end + + def test_rejects_non_semver_versions + error = assert_raises(ArgumentError) { IosReleaseVersion.bump_patch("0.0") } + + assert_equal("Cannot bump non-semver mobile version '0.0'", error.message) + end +end diff --git a/mobile/src/session/mobile-native-chat-image-transcript-markers.ts b/mobile/src/session/mobile-native-chat-image-transcript-markers.ts index f8f0ed6b82b..2c1ecc291ff 100644 --- a/mobile/src/session/mobile-native-chat-image-transcript-markers.ts +++ b/mobile/src/session/mobile-native-chat-image-transcript-markers.ts @@ -1,4 +1,4 @@ -// Single-sources the desktop marker logic (pure functions over shared types): +// Single-sources the marker logic (pure functions over shared types): // Claude records an attached image as `[Image: source: /path]` (+ `[Image #N]` // prefix on the caption turn), and both render and echo reconciliation must // agree with desktop on how those marker turns are interpreted. @@ -6,8 +6,8 @@ export { imageSourcePathFromText, normalizeImageTranscriptMessages, stripImagePromptMarker -} from '../../../src/renderer/src/components/native-chat/native-chat-image-transcript-markers' -import { imageSourcePathFromText } from '../../../src/renderer/src/components/native-chat/native-chat-image-transcript-markers' +} from '../../../src/shared/native-chat-image-transcript-markers' +import { imageSourcePathFromText } from '../../../src/shared/native-chat-image-transcript-markers' import { isTextBlock, type NativeChatMessage } from '../../../src/shared/native-chat-types' /** A raw (un-normalized) transcript user turn that is an image-source marker — diff --git a/src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts b/src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts index d552747efb0..7185b6ad1d3 100644 --- a/src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts +++ b/src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts @@ -1,4 +1,4 @@ -import { stripImagePromptMarker } from './native-chat-image-transcript-markers' +import { stripImagePromptMarker } from '../../../../shared/native-chat-image-transcript-markers' import { isImageRefBlock, isTextBlock, diff --git a/src/renderer/src/components/native-chat/native-chat-session-assembler.ts b/src/renderer/src/components/native-chat/native-chat-session-assembler.ts index 53ebbd0084a..2b318554fc7 100644 --- a/src/renderer/src/components/native-chat/native-chat-session-assembler.ts +++ b/src/renderer/src/components/native-chat/native-chat-session-assembler.ts @@ -7,7 +7,7 @@ import { type NativeChatSessionStatus } from '../../../../shared/native-chat-types' import { NATIVE_CHAT_STREAMING_ID } from '../../../../shared/native-chat-streaming' -import { normalizeImageTranscriptMessages } from './native-chat-image-transcript-markers' +import { normalizeImageTranscriptMessages } from '../../../../shared/native-chat-image-transcript-markers' import { isLaunchPromptMessageId, isPendingMessageId } from './native-chat-pending' /** Messages grouped by source. Higher-priority sources (transcript > hook > diff --git a/src/renderer/src/components/native-chat/native-chat-image-transcript-markers.test.ts b/src/shared/native-chat-image-transcript-markers.test.ts similarity index 95% rename from src/renderer/src/components/native-chat/native-chat-image-transcript-markers.test.ts rename to src/shared/native-chat-image-transcript-markers.test.ts index 6d6a15917c3..58d6d0d16c5 100644 --- a/src/renderer/src/components/native-chat/native-chat-image-transcript-markers.test.ts +++ b/src/shared/native-chat-image-transcript-markers.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import type { NativeChatMessage } from '../../../../shared/native-chat-types' +import type { NativeChatMessage } from './native-chat-types' import { normalizeImageTranscriptMessages } from './native-chat-image-transcript-markers' function userText(id: string, text: string): NativeChatMessage { diff --git a/src/renderer/src/components/native-chat/native-chat-image-transcript-markers.ts b/src/shared/native-chat-image-transcript-markers.ts similarity index 95% rename from src/renderer/src/components/native-chat/native-chat-image-transcript-markers.ts rename to src/shared/native-chat-image-transcript-markers.ts index 031d6980cab..3f8f90f8cdb 100644 --- a/src/renderer/src/components/native-chat/native-chat-image-transcript-markers.ts +++ b/src/shared/native-chat-image-transcript-markers.ts @@ -1,8 +1,4 @@ -import { - isTextBlock, - type NativeChatBlock, - type NativeChatMessage -} from '../../../../shared/native-chat-types' +import { isTextBlock, type NativeChatBlock, type NativeChatMessage } from './native-chat-types' const IMAGE_SOURCE_MARKER = /^\[Image:\s*source:\s*(.+?)\]\s*$/ const IMAGE_PROMPT_MARKER = /^\[Image #\d+\]\s*/ From 0da5380a49879160459f2199ac9cefa1566c4bcf Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Thu, 23 Jul 2026 14:31:15 -0700 Subject: [PATCH 02/10] fix: recover Relay broker startup after transient failures (#10147) --- .../relay-auth-coordinator-recovery.test.ts | 237 ++++++++++++++++++ .../runtime/relay/relay-auth-coordinator.ts | 68 ++++- src/main/runtime/relay/relay-http-client.ts | 12 + 3 files changed, 314 insertions(+), 3 deletions(-) create mode 100644 src/main/runtime/relay/relay-auth-coordinator-recovery.test.ts diff --git a/src/main/runtime/relay/relay-auth-coordinator-recovery.test.ts b/src/main/runtime/relay/relay-auth-coordinator-recovery.test.ts new file mode 100644 index 00000000000..21e7f9aadec --- /dev/null +++ b/src/main/runtime/relay/relay-auth-coordinator-recovery.test.ts @@ -0,0 +1,237 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { RelayAuthCoordinator, type RelayAuthContext } from './relay-auth-coordinator' +import { RelayHttpError } from './relay-http-client' + +const context: RelayAuthContext = { + identity: { userId: 'user-1', profileId: 'profile-1', organizationId: 'org-1' }, + accessToken: 'access-1', + relayEntitled: true +} + +afterEach(() => { + vi.useRealTimers() +}) + +describe('RelayAuthCoordinator transient recovery', () => { + it('retries a transient assignment failure and activates without an external event', async () => { + vi.useFakeTimers() + const broker = { closeNow: vi.fn() } + const openBroker = vi + .fn() + .mockRejectedValueOnce(new RelayHttpError('assignment', 500)) + .mockResolvedValueOnce(broker) + const statuses: string[] = [] + const coordinator = new RelayAuthCoordinator({ + readContext: async () => context, + openBroker, + onStatus: (status) => statuses.push(status), + random: () => 0.5 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + expect(statuses.at(-1)).toBe('offline') + + await vi.advanceTimersByTimeAsync(501) + expect(openBroker).toHaveBeenCalledTimes(2) + expect(coordinator.getActiveBroker()).toBe(broker) + expect(statuses.at(-1)).toBe('registered') + }) + + it('retries when cloud-session refresh fails before identity can be read', async () => { + vi.useFakeTimers() + const broker = { closeNow: vi.fn() } + const readContext = vi + .fn() + .mockRejectedValueOnce(new Error('temporary cloud session refresh failure')) + .mockResolvedValueOnce(context) + const openBroker = vi.fn().mockResolvedValue(broker) + const coordinator = new RelayAuthCoordinator({ + readContext, + openBroker, + onStatus: vi.fn(), + random: () => 0.5 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(readContext).toHaveBeenCalledOnce() + expect(openBroker).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(501) + expect(readContext).toHaveBeenCalledTimes(2) + expect(openBroker).toHaveBeenCalledOnce() + expect(coordinator.getActiveBroker()).toBe(broker) + }) + + it('backs a sustained outage off to the five-minute jitter cap', async () => { + vi.useFakeTimers() + const openBroker = vi.fn().mockRejectedValue(new Error('temporary control open failure')) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => context, + openBroker, + onStatus: vi.fn(), + random: () => 0.5 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + + for (const delayMs of [500, 1_000, 2_000, 4_000, 8_000, 16_000, 32_000, 64_000, 128_000]) { + await vi.advanceTimersByTimeAsync(delayMs) + } + expect(openBroker).toHaveBeenCalledTimes(10) + + await vi.advanceTimersByTimeAsync(149_999) + expect(openBroker).toHaveBeenCalledTimes(10) + await vi.advanceTimersByTimeAsync(1) + expect(openBroker).toHaveBeenCalledTimes(11) + + await vi.advanceTimersByTimeAsync(150_000) + expect(openBroker).toHaveBeenCalledTimes(12) + }) + + it('does not retry a permanent authorization response', async () => { + vi.useFakeTimers() + const openBroker = vi.fn().mockRejectedValue(new RelayHttpError('token-exchange', 403)) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => context, + openBroker, + onStatus: vi.fn(), + random: () => 0 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(120_000) + + expect(openBroker).toHaveBeenCalledOnce() + expect(coordinator.getActiveBroker()).toBeNull() + }) + + it('cancels a pending retry as soon as demand disappears', async () => { + vi.useFakeTimers() + let demanded = true + const openBroker = vi.fn().mockRejectedValue(new Error('temporary control open failure')) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => context, + hasDemand: () => demanded, + openBroker, + onStatus: vi.fn(), + random: () => 0.75 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + + demanded = false + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(120_000) + + expect(openBroker).toHaveBeenCalledOnce() + expect(coordinator.getActiveBroker()).toBeNull() + }) + + it('re-reads demand when the retry fires and stops without opening again', async () => { + vi.useFakeTimers() + let demanded = true + const statuses: string[] = [] + const openBroker = vi.fn().mockRejectedValue(new Error('temporary control open failure')) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => context, + hasDemand: () => demanded, + openBroker, + onStatus: (status) => statuses.push(status), + random: () => 0.5 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + + demanded = false + await vi.advanceTimersByTimeAsync(501) + await vi.advanceTimersByTimeAsync(120_000) + + expect(openBroker).toHaveBeenCalledOnce() + expect(statuses.at(-1)).toBe('standby') + }) + + it('re-reads entitlement when the retry fires and stops after removal', async () => { + vi.useFakeTimers() + let current = context + const openBroker = vi.fn().mockRejectedValue(new RelayHttpError('assignment', 500)) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => current, + openBroker, + onStatus: vi.fn(), + random: () => 0.5 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + + current = { ...context, relayEntitled: false } + await vi.advanceTimersByTimeAsync(501) + await vi.advanceTimersByTimeAsync(120_000) + + expect(openBroker).toHaveBeenCalledOnce() + expect(coordinator.getActiveBroker()).toBeNull() + }) + + it('cancels a pending retry immediately when the coordinator is fenced', async () => { + vi.useFakeTimers() + const openBroker = vi.fn().mockRejectedValue(new Error('temporary control open failure')) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => context, + openBroker, + onStatus: vi.fn(), + random: () => 0.5 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + + coordinator.fenceAndCloseNow() + await vi.advanceTimersByTimeAsync(120_000) + + expect(openBroker).toHaveBeenCalledOnce() + expect(coordinator.getActiveBroker()).toBeNull() + }) + + it('does not carry a pending retry across an identity switch', async () => { + vi.useFakeTimers() + let current = context + const broker = { closeNow: vi.fn() } + const openBroker = vi + .fn() + .mockRejectedValueOnce(new Error('temporary control open failure')) + .mockResolvedValueOnce(broker) + const coordinator = new RelayAuthCoordinator({ + readContext: async () => current, + openBroker, + onStatus: vi.fn(), + random: () => 0.75 + }) + + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledOnce() + + current = { + ...context, + identity: { ...context.identity, profileId: 'profile-2' } + } + coordinator.reconcile() + await vi.advanceTimersByTimeAsync(0) + expect(openBroker).toHaveBeenCalledTimes(2) + expect(coordinator.getActiveBroker()).toBe(broker) + await vi.advanceTimersByTimeAsync(120_000) + + expect(openBroker).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/runtime/relay/relay-auth-coordinator.ts b/src/main/runtime/relay/relay-auth-coordinator.ts index f197edcc2b1..7f97e33dc41 100644 --- a/src/main/runtime/relay/relay-auth-coordinator.ts +++ b/src/main/runtime/relay/relay-auth-coordinator.ts @@ -1,4 +1,5 @@ import type { RelayBrokerStatus } from './relay-session-broker' +import { shouldRetryRelayConnectionError } from './relay-http-client' export type RelayAuthIdentity = { userId: string @@ -26,6 +27,7 @@ type RelayAuthCoordinatorOptions = { }) => Promise onStatus: (status: RelayBrokerStatus) => void lingerMs?: number + random?: () => number } type BrokerOwnership = { @@ -39,12 +41,17 @@ function identityKey(identity: RelayAuthIdentity): string { } export class RelayAuthCoordinator { + // Why: recover brief failures quickly without turning a sustained outage into auth/director load. + private static readonly RETRY_BASE_MS = 1_000 + private static readonly RETRY_MAX_MS = 5 * 60_000 private readonly options: RelayAuthCoordinatorOptions private authEpoch = 0 private ownership: BrokerOwnership | null = null private readonly pendingOwnerships = new Set() private latestReconcile: Promise = Promise.resolve() private lingerTimer: ReturnType | null = null + private retryTimer: ReturnType | null = null + private retryAttempt = 0 private stopped = false constructor(options: RelayAuthCoordinatorOptions) { @@ -52,12 +59,20 @@ export class RelayAuthCoordinator { } reconcile(): void { + this.beginReconcile(true) + } + + private beginReconcile(resetRetry: boolean, expectedIdentityKey?: string): void { if (this.stopped) { return } + this.cancelRetry() + if (resetRetry) { + this.retryAttempt = 0 + } const epoch = ++this.authEpoch this.invalidatePendingOwnerships() - const reconcile = this.reconcileEpoch(epoch) + const reconcile = this.reconcileEpoch(epoch, expectedIdentityKey) this.latestReconcile = reconcile void reconcile } @@ -65,6 +80,8 @@ export class RelayAuthCoordinator { fenceAndCloseNow(): void { ++this.authEpoch this.cancelLinger() + this.cancelRetry() + this.retryAttempt = 0 this.invalidatePendingOwnerships() this.invalidateOwnership() this.options.onStatus('offline') @@ -94,7 +111,8 @@ export class RelayAuthCoordinator { this.fenceAndCloseNow() } - private async reconcileEpoch(epoch: number): Promise { + private async reconcileEpoch(epoch: number, expectedIdentityKey?: string): Promise { + let retryIdentityKey: string | undefined try { const context = await this.options.readContext() if (!this.isEpochCurrent(epoch)) { @@ -102,12 +120,19 @@ export class RelayAuthCoordinator { } if (!context || !context.relayEntitled) { this.cancelLinger() + this.retryAttempt = 0 this.invalidateOwnership() this.options.onStatus('offline') return } const nextIdentityKey = identityKey(context.identity) + if (expectedIdentityKey && nextIdentityKey !== expectedIdentityKey) { + this.retryAttempt = 0 + this.options.onStatus('offline') + return + } if (!(this.options.hasDemand?.(context) ?? true)) { + this.retryAttempt = 0 if (this.ownership?.valid && this.ownership.identityKey !== nextIdentityKey) { this.cancelLinger() this.invalidateOwnership() @@ -119,9 +144,11 @@ export class RelayAuthCoordinator { } this.cancelLinger() if (this.ownership?.valid && this.ownership.identityKey === nextIdentityKey) { + this.retryAttempt = 0 this.options.onStatus('registered') return } + retryIdentityKey = nextIdentityKey this.invalidateOwnership() this.options.onStatus('connecting') const ownership: BrokerOwnership = { @@ -150,14 +177,42 @@ export class RelayAuthCoordinator { return } this.ownership = ownership + this.retryAttempt = 0 this.options.onStatus('registered') - } catch { + } catch (error) { if (this.isEpochCurrent(epoch)) { this.options.onStatus('offline') + if (shouldRetryRelayConnectionError(error)) { + this.scheduleRetry(epoch, retryIdentityKey) + } } } } + private scheduleRetry(epoch: number, expectedIdentityKey?: string): void { + if (this.retryTimer || !this.isEpochCurrent(epoch)) { + return + } + const exponent = Math.min( + this.retryAttempt, + Math.ceil(Math.log2(RelayAuthCoordinator.RETRY_MAX_MS / RelayAuthCoordinator.RETRY_BASE_MS)) + ) + const capMs = Math.min( + RelayAuthCoordinator.RETRY_MAX_MS, + RelayAuthCoordinator.RETRY_BASE_MS * 2 ** exponent + ) + this.retryAttempt++ + const random = this.options.random ?? Math.random + const delayMs = Math.floor(random() * (capMs + 1)) + this.retryTimer = setTimeout(() => { + this.retryTimer = null + if (this.isEpochCurrent(epoch)) { + // Retry still re-reads entitlement and demand; the timer grants no authority. + this.beginReconcile(false, expectedIdentityKey) + } + }, delayMs) + } + private async refreshAccessToken( ownership: { valid: boolean }, expectedIdentityKey: string @@ -212,6 +267,13 @@ export class RelayAuthCoordinator { } } + private cancelRetry(): void { + if (this.retryTimer) { + clearTimeout(this.retryTimer) + this.retryTimer = null + } + } + private invalidatePendingOwnerships(): void { for (const ownership of this.pendingOwnerships) { ownership.valid = false diff --git a/src/main/runtime/relay/relay-http-client.ts b/src/main/runtime/relay/relay-http-client.ts index 1906381ceeb..6862f26ce6c 100644 --- a/src/main/runtime/relay/relay-http-client.ts +++ b/src/main/runtime/relay/relay-http-client.ts @@ -38,6 +38,18 @@ export class RelayHttpError extends Error { } } +export function shouldRetryRelayConnectionError(error: unknown): boolean { + if (!(error instanceof RelayHttpError)) { + return true + } + return ( + error.statusCode >= 500 || + error.statusCode === 408 || + error.statusCode === 425 || + error.statusCode === 429 + ) +} + export function deriveRelayHostId(publicKey: Uint8Array): string { return createHash('sha256').update(publicKey).digest('base64url').slice(0, 16) } From 94d3db4a2467bb8f78ce814362f84275a7758bd6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 14:35:58 -0700 Subject: [PATCH 03/10] feat(source-control): show current branch without evicting Create PR (#10215) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(source-control): show current branch without evicting Create PR #9787 added the current-branch identity to the Source Control header but did it by replacing the Create PR button's toolbar slot, so #10032 reverted the whole thing. Create PR is the primary entry point into the stage→commit→push→generate-PR flow, so it can't be traded away. Restore the branch identity as its own row above the toolbar so it coexists with the Create PR button (Option 2 layout). Detached HEAD renders in the same identity row via DetachedHeadBadge (re-adds its tabIndex/aria-label), replacing the separate below-toolbar badge row. * style(source-control): match branch identity text to the 'vs main' base ref Same font-mono / 10.5px / foreground-90 / underline treatment so the current branch name reads visually consistent with the base ref in the context row. * style(source-control): drop branch identity underline Keeps the 'vs main' font/size/color match but no underline — the label isn't clickable, so the underline read as a false affordance. * test(source-control): harden identity-row detached + no-identity contracts Add a stable data-testid to the identity row so the no-identity case proves no row renders, and assert the detached badge's accessible label + focusability. Addresses CodeRabbit review on #10215. --- .../src/components/DetachedHeadBadge.tsx | 8 +- .../right-sidebar/SourceControl.tsx | 9 +- ...e-control-header-toolbar-identity.test.tsx | 115 ++++++++++++++++++ .../source-control-header-toolbar.tsx | 57 ++++++++- src/renderer/src/i18n/locales/en.json | 3 +- src/renderer/src/i18n/locales/es.json | 3 +- src/renderer/src/i18n/locales/ja.json | 3 +- src/renderer/src/i18n/locales/ko.json | 3 +- src/renderer/src/i18n/locales/zh.json | 3 +- 9 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx diff --git a/src/renderer/src/components/DetachedHeadBadge.tsx b/src/renderer/src/components/DetachedHeadBadge.tsx index 58a12e6395e..bc03ce2ab52 100644 --- a/src/renderer/src/components/DetachedHeadBadge.tsx +++ b/src/renderer/src/components/DetachedHeadBadge.tsx @@ -12,13 +12,15 @@ type DetachedHeadBadgeProps = { label?: 'sidebar' | 'source-control' side?: React.ComponentProps['side'] className?: string + tabIndex?: number } export function DetachedHeadBadge({ display, label = 'source-control', side = 'right', - className + className, + tabIndex }: DetachedHeadBadgeProps): React.JSX.Element { const visibleLabel = label === 'sidebar' ? display.sidebarLabel : display.sourceControlLabel @@ -27,6 +29,8 @@ export function DetachedHeadBadge({ - {visibleLabel} + {visibleLabel} diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 36553cbc924..16b6cf95d66 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -46,7 +46,6 @@ import { isFolderRepo } from '../../../../shared/repo-kind' import { mapSettledWithConcurrency } from '../../../../shared/map-with-concurrency' import { Tooltip, TooltipTrigger, TooltipContent, TooltipProvider } from '@/components/ui/tooltip' import { Button } from '@/components/ui/button' -import { DetachedHeadBadge } from '@/components/DetachedHeadBadge' import { DropdownMenu, DropdownMenuContent, @@ -811,7 +810,6 @@ function SourceControlInner(): React.JSX.Element { const activeRepoConnectionId = activeRepo?.connectionId ?? null const activeRepoExecutionHostId = activeRepo?.executionHostId ?? null const gitIdentityDisplay = activeWorktree ? getWorktreeGitIdentityDisplay(activeWorktree) : null - const detachedHeadDisplay = gitIdentityDisplay?.kind === 'detached' ? gitIdentityDisplay : null const branchName = gitIdentityDisplay?.kind === 'branch' ? gitIdentityDisplay.branchName : '' const entries = useAppStore((s) => activeWorktreeId @@ -5461,6 +5459,7 @@ function SourceControlInner(): React.JSX.Element { <>
- {detachedHeadDisplay && ( -
- -
- )} - {/* Why: hidden when count is 0 — notes are created from the diff view, so an empty Notes shelf here is pure chrome. */} {activeWorktreeId && worktreePath && diffCommentCount > 0 && (
diff --git a/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx b/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx new file mode 100644 index 00000000000..a7039795932 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx @@ -0,0 +1,115 @@ +import { renderToStaticMarkup } from 'react-dom/server' +import type { ReactNode } from 'react' +import { describe, expect, it, vi } from 'vitest' +import type { WorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display' +import type { PrimaryAction } from './source-control-primary-action' +import { SourceControlHeaderToolbar } from './source-control-header-toolbar' + +vi.mock('@/components/ui/tooltip', () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + +vi.mock('./source-control-header-overflow-menu', () => ({ + SourceControlHeaderOverflowMenu: () => +})) + +vi.mock('./source-control-branch-context-row', () => ({ + shouldShowSourceControlBranchContextRow: () => false, + SourceControlBranchContextRow: () => null +})) + +const CREATE_PR_ACTION: PrimaryAction = { + kind: 'create_pr', + label: 'Create PR', + title: 'Create a pull request', + disabled: false +} + +function renderToolbar( + overrides: { + gitIdentityDisplay?: WorktreeGitIdentityDisplay | null + createPrAction?: PrimaryAction | null + } = {} +): string { + const gitIdentityDisplay = + overrides.gitIdentityDisplay === undefined + ? ({ kind: 'branch', branchName: 'brennanb2025/source-control-branch-name' } as const) + : overrides.gitIdentityDisplay + const createPrAction = + overrides.createPrAction === undefined ? CREATE_PR_ACTION : overrides.createPrAction + + return renderToStaticMarkup( + + ) +} + +describe('SourceControlHeaderToolbar branch identity', () => { + it('keeps the Create PR button while showing the branch identity above it', () => { + const markup = renderToolbar() + const branchIndex = markup.indexOf('brennanb2025/source-control-branch-name') + const createPrIndex = markup.indexOf('Create PR') + + // Why: the #9787 revert regression — identity must not evict Create PR. + expect(markup).toContain('data-testid="source-control-git-identity-row"') + expect(branchIndex).toBeGreaterThan(-1) + expect(createPrIndex).toBeGreaterThan(-1) + // Identity row renders above the toolbar row that hosts Create PR. + expect(branchIndex).toBeLessThan(createPrIndex) + expect(markup).toContain('aria-label="Current branch: brennanb2025/source-control-branch-name"') + expect(markup).toContain('min-w-0 truncate') + }) + + it('renders detached HEAD identity alongside the Create PR button', () => { + const markup = renderToolbar({ + gitIdentityDisplay: { + kind: 'detached', + shortHead: '8cec248', + sidebarLabel: 'Detached HEAD @ 8cec248', + sourceControlLabel: 'Detached HEAD · 8cec248', + tooltip: 'Detached HEAD at 8cec248. You are viewing a commit, not a branch.' + } + }) + + expect(markup).not.toContain('aria-label="Current branch:') + expect(markup).toContain('data-testid="source-control-git-identity-row"') + expect(markup).toContain('Detached HEAD · 8cec248') + // Detached badge stays keyboard-reachable and exposes the full tooltip as its label. + expect(markup).toContain( + 'aria-label="Detached HEAD at 8cec248. You are viewing a commit, not a branch."' + ) + expect(markup).toContain('tabindex="0"') + expect(markup).toContain('Create PR') + }) + + it('omits the identity row when there is no git identity', () => { + const markup = renderToolbar({ gitIdentityDisplay: null }) + + expect(markup).not.toContain('data-testid="source-control-git-identity-row"') + expect(markup).not.toContain('aria-label="Current branch:') + expect(markup).toContain('Create PR') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-header-toolbar.tsx b/src/renderer/src/components/right-sidebar/source-control-header-toolbar.tsx index 3cac51e1e9f..d56d56b0cb6 100644 --- a/src/renderer/src/components/right-sidebar/source-control-header-toolbar.tsx +++ b/src/renderer/src/components/right-sidebar/source-control-header-toolbar.tsx @@ -1,5 +1,5 @@ import React, { useCallback, useEffect, useRef } from 'react' -import { GitPullRequestArrow, Loader2, Search, X } from 'lucide-react' +import { GitBranch, GitPullRequestArrow, Loader2, Search, X } from 'lucide-react' import type { GitBranchCompareSummary, GitUpstreamStatus, @@ -11,6 +11,8 @@ import { Button } from '@/components/ui/button' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { cn } from '@/lib/utils' import { translate } from '@/i18n/i18n' +import { DetachedHeadBadge } from '@/components/DetachedHeadBadge' +import type { WorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display' import { HostedReviewHeaderLink, HostedReviewIcon } from './hosted-review-header-chrome' import { shouldShowSourceControlBranchContextRow, @@ -19,6 +21,7 @@ import { import { SourceControlHeaderOverflowMenu } from './source-control-header-overflow-menu' type SourceControlHeaderToolbarProps = { + gitIdentityDisplay: WorktreeGitIdentityDisplay | null filterQuery: string filterExpanded: boolean onFilterQueryChange: (value: string) => void @@ -43,6 +46,56 @@ type SourceControlHeaderToolbarProps = { manualReviewUrl?: string | null } +// Why: its own row above the toolbar so branch identity coexists with the Create PR +// button instead of competing for the single toolbar slot (#9787 restore). +function SourceControlGitIdentityRow({ + display +}: { + display: WorktreeGitIdentityDisplay +}): React.JSX.Element { + if (display.kind === 'detached') { + return ( +
+ +
+ ) + } + + const branchName = display.branchName + const label = translate( + 'auto.components.right.sidebar.SourceControl.a4e93c21d7', + 'Current branch: {{value0}}', + { value0: branchName } + ) + + return ( +
+ + + + + + + {branchName} + + +
+ ) +} + function HostedReviewToolbarLink({ review, onOpenHostedReviewInChecks, @@ -124,6 +177,7 @@ function renderOverflowMenu( } export function SourceControlHeaderToolbar({ + gitIdentityDisplay, filterQuery, filterExpanded, onFilterQueryChange, @@ -190,6 +244,7 @@ export function SourceControlHeaderToolbar({ return (
+ {gitIdentityDisplay ? : null}
Date: Thu, 23 Jul 2026 15:54:02 -0700 Subject: [PATCH 04/10] fix(codex-accounts): stop account mutations blocking on the quota refresh (#10225) Switching, adding, re-authing, or removing a Codex account awaited refreshForCodexAccountChange before resolving the IPC call. Since the per-account CODEX_HOME rollout (#9501) that probe runs against a cold home (10s RPC + 15s PTY fallback, 25s WSL) and can queue behind an in-flight global usage fetch, so the switcher sat unresponsive for tens of seconds and a fresh login looked stuck on the loading screen. Worse, in addAccount the awaited refresh sat inside the login cleanup try/catch: a refresh rejection after the account was durably committed deleted the just-created managed home, leaving a registered account with no home ("account never connects"). Run the refresh as best-effort background work instead. Its synchronous prefix still flips usage to "fetching" before the first await, so the switcher updates instantly and usage fills in via the normal rate-limit state pushes; a probe failure is logged and can never trigger managed-home cleanup. Fixes #10141 --- src/main/codex-accounts/service.test.ts | 184 ++++++++++++++++++++++++ src/main/codex-accounts/service.ts | 22 ++- 2 files changed, 202 insertions(+), 4 deletions(-) diff --git a/src/main/codex-accounts/service.test.ts b/src/main/codex-accounts/service.test.ts index c0617eaea50..435a187fe4a 100644 --- a/src/main/codex-accounts/service.test.ts +++ b/src/main/codex-accounts/service.test.ts @@ -2586,4 +2586,188 @@ describe('CodexAccountService config sync', () => { expect(state.systemDefault?.email).toBe('real@home.dev') }) }) + + // Why: quota probes against a cold per-account CODEX_HOME can take 10–25s + // (RPC + PTY fallback) and queue behind an in-flight global usage refresh; + // account mutations must never block on — or fail because of — that probe. + describe('quota refresh decoupling', () => { + function createAccountOneSettings(): GlobalSettings { + const managedHomePath = createManagedHome( + testState.userDataDir, + 'account-1', + '', + '{"account":"managed"}\n' + ) + return createSettings({ + codexManagedAccounts: [ + { + id: 'account-1', + email: 'user@example.com', + managedHomePath, + providerAccountId: null, + workspaceLabel: null, + workspaceAccountId: null, + createdAt: 1, + updatedAt: 1, + lastAuthenticatedAt: 1 + } + ] + }) + } + + async function expectResolvesPromptly(promise: Promise, label: string): Promise { + let timer: NodeJS.Timeout | undefined + try { + return await Promise.race([ + promise, + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error(`${label} blocked on the quota refresh`)), + 2_000 + ) + }) + ]) + } finally { + clearTimeout(timer) + } + } + + function createLoginSpawnMock() { + return vi.fn((_command: string, _args: string[], options: { env: NodeJS.ProcessEnv }) => { + const child = new EventEmitter() as EventEmitter & { + stdout: PassThrough + stderr: PassThrough + kill: () => void + } + child.stdout = new PassThrough() + child.stderr = new PassThrough() + child.kill = vi.fn() + writeFileSync( + join(options.env.CODEX_HOME!, 'auth.json'), + createCodexAuthJson('user@example.com', 'provider-account-1', 'refresh-token'), + 'utf-8' + ) + queueMicrotask(() => child.emit('close', 0)) + return child + }) + } + + it('resolves selectAccount while the quota refresh never settles', async () => { + const store = createStore(createAccountOneSettings()) + const rateLimits = { + refreshForCodexAccountChange: vi.fn(() => new Promise(() => {})), + evictInactiveCodexCache: vi.fn() + } + const runtimeHome = createRuntimeHome() + + const { CodexAccountService } = await import('./service') + const service = new CodexAccountService( + store as never, + rateLimits as never, + runtimeHome as never + ) + + const state = await expectResolvesPromptly( + service.selectAccount('account-1'), + 'selectAccount' + ) + + expect(state.activeAccountId).toBe('account-1') + expect(rateLimits.refreshForCodexAccountChange).toHaveBeenCalledTimes(1) + }) + + it('resolves selectAccount when the quota refresh rejects', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const store = createStore(createAccountOneSettings()) + const rateLimits = { + refreshForCodexAccountChange: vi.fn().mockRejectedValue(new Error('cold probe failed')), + evictInactiveCodexCache: vi.fn() + } + const runtimeHome = createRuntimeHome() + + const { CodexAccountService } = await import('./service') + const service = new CodexAccountService( + store as never, + rateLimits as never, + runtimeHome as never + ) + + const state = await service.selectAccount('account-1') + + expect(state.activeAccountId).toBe('account-1') + await vi.waitFor(() => expect(errorSpy).toHaveBeenCalled()) + errorSpy.mockRestore() + }) + + it('resolves addAccount while the post-login quota refresh never settles', async () => { + vi.resetModules() + writeFileSync( + join(testState.fakeHomeDir, '.codex', 'config.toml'), + 'approval_policy = "never"\n', + 'utf-8' + ) + const spawnMock = createLoginSpawnMock() + vi.doMock('node:child_process', () => ({ execFileSync: vi.fn(), spawn: spawnMock })) + vi.doMock('../codex-cli/command', () => ({ resolveCodexCommand: () => 'codex' })) + + const store = createStore(createSettings()) + const rateLimits = { + refreshForCodexAccountChange: vi.fn(() => new Promise(() => {})), + evictInactiveCodexCache: vi.fn() + } + const runtimeHome = createRuntimeHome() + + const { CodexAccountService } = await import('./service') + const service = new CodexAccountService( + store as never, + rateLimits as never, + runtimeHome as never + ) + + const state = await expectResolvesPromptly(service.addAccount(), 'addAccount') + + expect(state.accounts).toHaveLength(1) + expect(state.accounts[0].email).toBe('user@example.com') + expect(rateLimits.refreshForCodexAccountChange).toHaveBeenCalledTimes(1) + }) + + it('keeps the new account and its managed home when the post-login quota refresh rejects', async () => { + vi.resetModules() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + writeFileSync( + join(testState.fakeHomeDir, '.codex', 'config.toml'), + 'approval_policy = "never"\n', + 'utf-8' + ) + const spawnMock = createLoginSpawnMock() + vi.doMock('node:child_process', () => ({ execFileSync: vi.fn(), spawn: spawnMock })) + vi.doMock('../codex-cli/command', () => ({ resolveCodexCommand: () => 'codex' })) + + const store = createStore(createSettings()) + const rateLimits = { + refreshForCodexAccountChange: vi.fn().mockRejectedValue(new Error('cold probe failed')), + evictInactiveCodexCache: vi.fn() + } + const runtimeHome = createRuntimeHome() + + const { CodexAccountService } = await import('./service') + const service = new CodexAccountService( + store as never, + rateLimits as never, + runtimeHome as never + ) + + const state = await service.addAccount() + + expect(state.accounts).toHaveLength(1) + const account = store.getSettings().codexManagedAccounts[0] + expect(account.email).toBe('user@example.com') + // The durable mutation must survive a failed usage probe — previously the + // rejection fell into login cleanup and deleted the just-created home. + expect(existsSync(account.managedHomePath)).toBe(true) + expect(existsSync(join(account.managedHomePath, 'auth.json'))).toBe(true) + await vi.waitFor(() => expect(errorSpy).toHaveBeenCalled()) + errorSpy.mockRestore() + }) + }) }) diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index a4733767740..a7448e855f2 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -204,6 +204,20 @@ export class CodexAccountService { return this.serializeMutation(() => this.doSelectAccount(accountId, target)) } + // Why: quota probes against a cold per-account CODEX_HOME can take 10–25s + // (RPC + PTY fallback) and queue behind an in-flight global usage refresh. + // The refresh synchronously flips usage to "fetching" before its first await, + // so the switcher updates immediately; the probe itself must never block or + // fail the already-durable account mutation. + private startQuotaRefreshInBackground( + outgoingAccountId: string | null | undefined, + target: CodexAccountSelectionTarget | undefined + ): void { + void this.rateLimits.refreshForCodexAccountChange(outgoingAccountId, target).catch((error) => { + console.error('[codex-accounts] Quota refresh after account change failed:', error) + }) + } + private async doAddAccount(target?: CodexAccountAddTarget): Promise { const accountId = randomUUID() const managedHome = this.createManagedHome(accountId, target) @@ -254,7 +268,7 @@ export class CodexAccountService { // Why: switching activates the new account, so cache the outgoing account's usage for the switcher. const outgoingAccountId = getSelectedCodexAccountIdForTarget(settings, targetSelection) - await this.rateLimits.refreshForCodexAccountChange(outgoingAccountId, targetSelection) + this.startQuotaRefreshInBackground(outgoingAccountId, targetSelection) return this.getSnapshot() } catch (error) { this.safeRemoveManagedHome(managedHomePath, accountId) @@ -310,7 +324,7 @@ export class CodexAccountService { this.runtimeHome.syncForCurrentSelection(accountTarget) // Why: re-auth can change the underlying Codex identity, so force a fresh read to avoid showing stale quota. - await this.rateLimits.refreshForCodexAccountChange(undefined, accountTarget) + this.startQuotaRefreshInBackground(undefined, accountTarget) return this.getSnapshot() } @@ -339,7 +353,7 @@ export class CodexAccountService { // Why: a removed account can no longer appear in the switcher dropdown, // so purge its cached usage to avoid stale entries. this.rateLimits.evictInactiveCodexCache(accountId) - await this.rateLimits.refreshForCodexAccountChange( + this.startQuotaRefreshInBackground( getSelectedCodexAccountIdForTarget(settings, getCodexSelectionTargetForAccount(account)) === accountId ? accountId @@ -388,7 +402,7 @@ export class CodexAccountService { this.lifecycle.onHostSystemDefaultSelected?.() } - await this.rateLimits.refreshForCodexAccountChange(outgoingAccountId, effectiveTarget) + this.startQuotaRefreshInBackground(outgoingAccountId, effectiveTarget) return this.getSnapshot() } From 4a09ede8b1ba16322e6a95d9a88d3b8a0c429948 Mon Sep 17 00:00:00 2001 From: kazu-42 <91968271+kazu-42@users.noreply.github.com> Date: Fri, 24 Jul 2026 07:58:01 +0900 Subject: [PATCH 05/10] fix(codex): classify rate-limit windows by duration (#10136) Co-authored-by: kazu-42 <91968271+kazu-42@users.noreply.github.com> --- src/main/rate-limits/codex-fetcher.test.ts | 95 +++++++++---- src/main/rate-limits/codex-fetcher.ts | 29 ++-- ...x-rate-limit-window-classification.test.ts | 132 ++++++++++++++++++ .../codex-rate-limit-window-classification.ts | 73 ++++++++++ 4 files changed, 283 insertions(+), 46 deletions(-) create mode 100644 src/main/rate-limits/codex-rate-limit-window-classification.test.ts create mode 100644 src/main/rate-limits/codex-rate-limit-window-classification.ts diff --git a/src/main/rate-limits/codex-fetcher.test.ts b/src/main/rate-limits/codex-fetcher.test.ts index 0ef24af3695..237909137b2 100644 --- a/src/main/rate-limits/codex-fetcher.test.ts +++ b/src/main/rate-limits/codex-fetcher.test.ts @@ -53,6 +53,31 @@ function makeRpcChild() { return child } +function respondToRpcRateLimitRead( + rpcChild: ReturnType, + rateLimits: unknown +): void { + rpcChild.stdin.write.mockImplementation((line: string) => { + const msg = JSON.parse(line) as { id?: number; method?: string } + if (msg.method === 'initialize') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: {} })}\n`) + ) + }, 0) + } + if (msg.method === 'account/rateLimits/read') { + setTimeout(() => { + rpcChild.stdout.emit( + 'data', + Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: { rateLimits } })}\n`) + ) + }, 0) + } + }) +} + function makePtyTerm() { let dataHandler: ((data: string) => void) | null = null let exitHandler: (() => void) | null = null @@ -325,38 +350,12 @@ describe('fetchCodexRateLimits', () => { expect(ptySpawnMock).not.toHaveBeenCalled() }) - it('normalizes Codex RPC remaining-minute windows to fixed display durations', async () => { + it('normalizes near-canonical Codex RPC windows to fixed display durations', async () => { const rpcChild = makeRpcChild() childSpawnMock.mockReturnValue(rpcChild) - rpcChild.stdin.write.mockImplementation((line: string) => { - const msg = JSON.parse(line) as { id?: number; method?: string } - if (msg.method === 'initialize') { - setTimeout(() => { - rpcChild.stdout.emit( - 'data', - Buffer.from(`${JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: {} })}\n`) - ) - }, 0) - } - if (msg.method === 'account/rateLimits/read') { - setTimeout(() => { - rpcChild.stdout.emit( - 'data', - Buffer.from( - `${JSON.stringify({ - jsonrpc: '2.0', - id: msg.id, - result: { - rateLimits: { - primary: { usedPercent: 0, windowDurationMins: 299 }, - secondary: { usedPercent: 0, windowDurationMins: 10079 } - } - } - })}\n` - ) - ) - }, 0) - } + respondToRpcRateLimitRead(rpcChild, { + primary: { usedPercent: 0, windowDurationMins: 299 }, + secondary: { usedPercent: 0, windowDurationMins: 10079 } }) const resultPromise = fetchCodexRateLimits() @@ -368,6 +367,42 @@ describe('fetchCodexRateLimits', () => { expect(result.weekly?.windowMinutes).toBe(10080) }) + it('keeps a weekly-only Codex primary window out of the 5-hour slot', async () => { + const rpcChild = makeRpcChild() + childSpawnMock.mockReturnValue(rpcChild) + respondToRpcRateLimitRead(rpcChild, { + primary: { usedPercent: 22, windowDurationMins: 10080 }, + secondary: null + }) + + const resultPromise = fetchCodexRateLimits() + await vi.advanceTimersByTimeAsync(1) + await vi.advanceTimersByTimeAsync(1) + + await expect(resultPromise).resolves.toMatchObject({ + session: null, + weekly: { usedPercent: 22, windowMinutes: 10080 } + }) + }) + + it('does not map a duplicate session-duration window into the weekly slot', async () => { + const rpcChild = makeRpcChild() + childSpawnMock.mockReturnValue(rpcChild) + respondToRpcRateLimitRead(rpcChild, { + primary: { usedPercent: 11, windowDurationMins: 300 }, + secondary: { usedPercent: 12, windowDurationMins: 300 } + }) + + const resultPromise = fetchCodexRateLimits() + await vi.advanceTimersByTimeAsync(1) + await vi.advanceTimersByTimeAsync(1) + + await expect(resultPromise).resolves.toMatchObject({ + session: { usedPercent: 11, windowMinutes: 300 }, + weekly: null + }) + }) + it('fills reset-credit count from the backend when the installed app-server omits it', async () => { const rpcChild = makeRpcChild() childSpawnMock.mockReturnValue(rpcChild) diff --git a/src/main/rate-limits/codex-fetcher.ts b/src/main/rate-limits/codex-fetcher.ts index 18042db9c80..7d9037e3eed 100644 --- a/src/main/rate-limits/codex-fetcher.ts +++ b/src/main/rate-limits/codex-fetcher.ts @@ -11,6 +11,13 @@ import { readFetchResponseJsonWithinLimit } from '../lib/fetch-response-body' import { readIntegrationCredentialFileText } from '../integration-credential-file' import { join } from 'node:path' import { probeCodexAuthPresence } from './codex-auth-presence' +import { + classifyCodexRateLimitWindows, + CODEX_SESSION_WINDOW_MINUTES, + CODEX_WEEKLY_WINDOW_MINUTES, + type CodexRpcRateLimits, + type CodexRpcRateWindow +} from './codex-rate-limit-window-classification' import { resolveCodexCommand } from '../codex-cli/command' import { withMacTailscaleDnsHint } from '../network/macos-tailscale-dns-diagnostic' import { getCmdExePath, getSpawnArgsForWindows } from '../win32-utils' @@ -58,12 +65,6 @@ type RpcResponse = { error?: { code: number; message: string } } -type RpcRateWindow = { - usedPercent?: number - windowDurationMins?: number - resetsAt?: number // Unix seconds -} - type RateLimitResetCredits = { availableCount: number totalEarnedCount?: number @@ -75,14 +76,9 @@ type RateLimitResetCredits = { }[] } -type RpcRateLimitsResult = { - primary?: RpcRateWindow - secondary?: RpcRateWindow -} - // Why: the Codex app-server wraps rate limit data as { rateLimits: { primary, secondary, ... } }. type RpcRateLimitsResponse = { - rateLimits?: RpcRateLimitsResult + rateLimits?: CodexRpcRateLimits | null rateLimitResetCredits?: { availableCount?: number totalEarnedCount?: number @@ -444,7 +440,7 @@ export async function consumeCodexRateLimitResetCredit(options: { } function mapRpcWindow( - raw: RpcRateWindow | undefined, + raw: CodexRpcRateWindow | null | undefined, expectedWindowMinutes: number ): RateLimitWindow | null { if (!raw || typeof raw.usedPercent !== 'number' || !Number.isFinite(raw.usedPercent)) { @@ -472,7 +468,7 @@ function mapRpcWindow( return { usedPercent: Math.min(100, Math.max(0, raw.usedPercent)), - // Why: windowDurationMins reports remaining minutes, but the UI needs the fixed bucket duration for "5h"/"wk" labels. + // Why: older app-server builds can report canonical bucket lengths off by one minute. windowMinutes: expectedWindowMinutes, resetsAt, resetDescription @@ -720,8 +716,9 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise { + it.each([ + { + name: 'null windows', + result: null, + expected: { session: null, weekly: null } + }, + { + name: 'reordered known windows', + result: { + primary: { usedPercent: 81, windowDurationMins: 10080 }, + secondary: { usedPercent: 21, windowDurationMins: 300 } + }, + expected: { session: 21, weekly: 81 } + }, + { + name: 'weekly-only primary window', + result: { + primary: { usedPercent: 22, windowDurationMins: 10080 }, + secondary: null + }, + expected: { session: null, weekly: 22 } + }, + { + name: 'session-only secondary window', + result: { + primary: null, + secondary: { usedPercent: 31, windowDurationMins: 300 } + }, + expected: { session: 31, weekly: null } + }, + { + name: 'duplicate session windows', + result: { + primary: { usedPercent: 41, windowDurationMins: 300 }, + secondary: { usedPercent: 42, windowDurationMins: 300 } + }, + expected: { session: 41, weekly: null } + }, + { + name: 'duplicate weekly windows', + result: { + primary: { usedPercent: 51, windowDurationMins: 10080 }, + secondary: { usedPercent: 52, windowDurationMins: 10080 } + }, + expected: { session: null, weekly: 51 } + }, + { + name: 'malformed usage', + result: { + primary: { usedPercent: Number.NaN, windowDurationMins: 300 }, + secondary: { usedPercent: 61, windowDurationMins: 10080 } + }, + expected: { session: null, weekly: 61 } + }, + { + name: 'malformed duration with positional fallback', + result: { + primary: { usedPercent: 71, windowDurationMins: '300' }, + secondary: null + }, + expected: { session: 71, weekly: null } + }, + { + name: 'reordered near-canonical windows', + result: { + primary: { usedPercent: 81, windowDurationMins: 10081 }, + secondary: { usedPercent: 82, windowDurationMins: 299 } + }, + expected: { session: 82, weekly: 81 } + }, + { + name: 'opposite near-canonical boundaries', + result: { + primary: { usedPercent: 83, windowDurationMins: 10079 }, + secondary: { usedPercent: 84, windowDurationMins: 301 } + }, + expected: { session: 84, weekly: 83 } + }, + { + name: 'outside-tolerance unknown windows', + result: { + primary: { usedPercent: 91, windowDurationMins: 302 }, + secondary: { usedPercent: 92, windowDurationMins: 10082 } + }, + expected: { session: 91, weekly: 92 } + }, + { + name: 'unknown windows without durations', + result: { + primary: { usedPercent: 101 }, + secondary: { usedPercent: 102 } + }, + expected: { session: 101, weekly: 102 } + }, + { + name: 'known session wins over unknown primary fallback', + result: { + primary: { usedPercent: 111, windowDurationMins: 60 }, + secondary: { usedPercent: 112, windowDurationMins: 300 } + }, + expected: { session: 112, weekly: null } + }, + { + name: 'known weekly wins over unknown secondary fallback', + result: { + primary: { usedPercent: 121, windowDurationMins: 10080 }, + secondary: { usedPercent: 122, windowDurationMins: 60 } + }, + expected: { session: null, weekly: 121 } + } + ] as const)('$name', ({ result, expected }) => { + expect(usedPercentByWindow(result)).toEqual(expected) + }) +}) diff --git a/src/main/rate-limits/codex-rate-limit-window-classification.ts b/src/main/rate-limits/codex-rate-limit-window-classification.ts new file mode 100644 index 00000000000..9c260cc52ae --- /dev/null +++ b/src/main/rate-limits/codex-rate-limit-window-classification.ts @@ -0,0 +1,73 @@ +export const CODEX_SESSION_WINDOW_MINUTES = 300 +export const CODEX_WEEKLY_WINDOW_MINUTES = 10080 + +// Why: tolerate the one-minute drift seen in older Codex bucket lengths without absorbing other durations. +const CODEX_WINDOW_DURATION_TOLERANCE_MINUTES = 1 + +export type CodexRpcRateWindow = { + usedPercent?: unknown + windowDurationMins?: unknown + resetsAt?: unknown +} + +export type CodexRpcRateLimits = { + primary?: CodexRpcRateWindow | null + secondary?: CodexRpcRateWindow | null +} + +type MappableCodexRpcRateWindow = CodexRpcRateWindow & { usedPercent: number } +type CodexRateLimitWindowKind = 'session' | 'weekly' | null + +function isMappableCodexRpcRateWindow( + raw: CodexRpcRateWindow | null | undefined +): raw is MappableCodexRpcRateWindow { + return typeof raw?.usedPercent === 'number' && Number.isFinite(raw.usedPercent) +} + +function classifyWindowDuration(raw: MappableCodexRpcRateWindow): CodexRateLimitWindowKind { + const duration = raw.windowDurationMins + if (typeof duration !== 'number' || !Number.isFinite(duration)) { + return null + } + if ( + Math.abs(duration - CODEX_SESSION_WINDOW_MINUTES) <= CODEX_WINDOW_DURATION_TOLERANCE_MINUTES + ) { + return 'session' + } + if (Math.abs(duration - CODEX_WEEKLY_WINDOW_MINUTES) <= CODEX_WINDOW_DURATION_TOLERANCE_MINUTES) { + return 'weekly' + } + return null +} + +export function classifyCodexRateLimitWindows(result: CodexRpcRateLimits | null | undefined): { + session: MappableCodexRpcRateWindow | null + weekly: MappableCodexRpcRateWindow | null +} { + const primary = isMappableCodexRpcRateWindow(result?.primary) ? result.primary : null + const secondary = isMappableCodexRpcRateWindow(result?.secondary) ? result.secondary : null + let session: MappableCodexRpcRateWindow | null = null + let weekly: MappableCodexRpcRateWindow | null = null + + for (const window of [primary, secondary]) { + if (!window) { + continue + } + const kind = classifyWindowDuration(window) + if (kind === 'session' && !session) { + session = window + } else if (kind === 'weekly' && !weekly) { + weekly = window + } + } + + // Why: unknown app-server durations retain Orca's legacy primary/session and secondary/weekly mapping. + if (!session && primary && classifyWindowDuration(primary) === null) { + session = primary + } + if (!weekly && secondary && classifyWindowDuration(secondary) === null) { + weekly = secondary + } + + return { session, weekly } +} From 06f6e3bed242df95ea0a7f953ec6dd9c41bcf18d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:04:05 -0700 Subject: [PATCH 06/10] fix(mobile): make image send retries safe (#10228) --- ...bile-native-chat-image-attachments.test.ts | 167 +++++++++++++- ...se-mobile-native-chat-image-attachments.ts | 203 ++++++++++-------- 2 files changed, 284 insertions(+), 86 deletions(-) diff --git a/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts b/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts index 7c91bc4cb78..6018f353854 100644 --- a/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts +++ b/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts @@ -31,7 +31,10 @@ function sendResult(accepted: boolean): RpcSuccess { return { id: 'send', ok: true, result: { send: { accepted } }, _meta: { runtimeId: 'r' } } } -function makeClient(responses: RpcResponse[]): Pick & { +function makeClient(responses: (RpcResponse | Promise)[]): Pick< + RpcClient, + 'sendRequest' +> & { calls: { method: string; params: Record }[] } { const calls: { method: string; params: Record }[] = [] @@ -52,6 +55,7 @@ type HookArgs = Parameters[0] type Hook = ReturnType const SCOPE_A = 'h\0w\0tab-a' +const SCOPE_B = 'h\0w\0tab-b' function baseArgs(overrides: Partial & Pick): HookArgs { return { @@ -403,6 +407,14 @@ describe('useMobileNativeChatImageAttachments', () => { await act(async () => { await hook!.attachImage('library') }) + let overlappingAccepted = true + await act(async () => { + overlappingAccepted = await hook!.sendNativeChat('too soon') + }) + expect(overlappingAccepted).toBe(false) + expect(baseSend).not.toHaveBeenCalled() + expect(client.calls.filter((call) => call.method === 'terminal.send')).toHaveLength(2) + await act(async () => { releaseSettle!() await sendPromise @@ -498,6 +510,159 @@ describe('useMobileNativeChatImageAttachments', () => { expect(baseSend).toHaveBeenCalledWith('hi again') }) + it('retains the stale marker when a rejected healing clear blocks text-only send', async () => { + pick.mockResolvedValue({ base64: 'AAAA', uri: 'file:///a.jpg' }) + const client = makeClient([ + methodNotFound('start'), + ok('save', '/tmp/a.png'), + sendResult(true), // Ctrl+U clear + sendResult(true), // image paste accepted + sendResult(false), // first healing Ctrl+U rejected + sendResult(true) // retry healing Ctrl+U accepted + ]) + const baseSend = vi.fn().mockResolvedValueOnce(false).mockResolvedValueOnce(true) + mount(baseArgs({ client: client as unknown as RpcClient, baseSend })) + await act(async () => { + await hook!.attachImage('library') + }) + await act(async () => { + await hook!.sendNativeChat('hi') + }) + expect(hook!.attachments).toHaveLength(1) + + await act(async () => { + hook!.removeAttachment('img-1') + }) + let accepted = true + await act(async () => { + accepted = await hook!.sendNativeChat('hi again') + }) + expect(accepted).toBe(false) + expect(baseSend).toHaveBeenCalledTimes(1) + + await act(async () => { + accepted = await hook!.sendNativeChat('hi again') + }) + expect(accepted).toBe(true) + const sendCalls = client.calls.filter((c) => c.method === 'terminal.send') + expect(sendCalls).toHaveLength(4) + expect(sendCalls[2]?.params).toMatchObject({ text: '\x15', enter: false }) + expect(sendCalls[3]?.params).toMatchObject({ text: '\x15', enter: false }) + expect(baseSend).toHaveBeenNthCalledWith(1, 'hi', ['file:///a.jpg']) + expect(baseSend).toHaveBeenNthCalledWith(2, 'hi again') + }) + + it('does not reroute text when the active terminal changes during a healing clear', async () => { + pick.mockResolvedValue({ base64: 'AAAA', uri: 'file:///a.jpg' }) + let releaseClear: ((response: RpcResponse) => void) | null = null + const deferredClear = new Promise((resolve) => { + releaseClear = resolve + }) + const client = makeClient([ + methodNotFound('start'), + ok('save', '/tmp/a.png'), + sendResult(true), + sendResult(true), + deferredClear + ]) + const baseSend = vi.fn().mockResolvedValueOnce(false) + const activeHandleRef = { current: 'term-1' } + mount(baseArgs({ client: client as unknown as RpcClient, activeHandleRef, baseSend })) + await act(async () => { + await hook!.attachImage('library') + }) + await act(async () => { + await hook!.sendNativeChat('hi') + }) + await act(async () => { + hook!.removeAttachment('img-1') + }) + + let retry: Promise | null = null + await act(async () => { + retry = hook!.sendNativeChat('hi again') + await Promise.resolve() + }) + activeHandleRef.current = 'term-2' + let accepted = true + await act(async () => { + releaseClear!(sendResult(true)) + accepted = await retry! + }) + + expect(accepted).toBe(false) + expect(baseSend).toHaveBeenCalledTimes(1) + const sendCalls = client.calls.filter((c) => c.method === 'terminal.send') + expect(sendCalls[2]?.params).toMatchObject({ terminal: 'term-1', text: '\x15', enter: false }) + }) + + it('heals rejected image submits independently across terminals', async () => { + pick + .mockResolvedValueOnce({ base64: 'AAAA', uri: 'file:///a.jpg' }) + .mockResolvedValueOnce({ base64: 'BBBB', uri: 'file:///b.jpg' }) + const client = makeClient([ + methodNotFound('start-a'), + ok('save-a', '/tmp/a.png'), + sendResult(true), + sendResult(true), + methodNotFound('start-b'), + ok('save-b', '/tmp/b.png'), + sendResult(true), + sendResult(true), + sendResult(true), + sendResult(true) + ]) + const baseSend = vi + .fn() + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(true) + const activeHandleRef = { current: 'term-1' } + const args = baseArgs({ client: client as unknown as RpcClient, activeHandleRef, baseSend }) + mount(args) + await act(async () => { + await hook!.attachImage('library') + }) + await act(async () => { + await hook!.sendNativeChat('first') + }) + + activeHandleRef.current = 'term-2' + update({ ...args, scopeKey: SCOPE_B }) + await act(async () => { + await hook!.attachImage('library') + }) + await act(async () => { + await hook!.sendNativeChat('second') + }) + await act(async () => { + hook!.removeAttachment('img-2') + }) + + activeHandleRef.current = 'term-1' + update(args) + await act(async () => { + hook!.removeAttachment('img-1') + }) + await act(async () => { + expect(await hook!.sendNativeChat('retry first')).toBe(true) + }) + + activeHandleRef.current = 'term-2' + update({ ...args, scopeKey: SCOPE_B }) + await act(async () => { + expect(await hook!.sendNativeChat('retry second')).toBe(true) + }) + + const sendCalls = client.calls.filter((c) => c.method === 'terminal.send') + expect(sendCalls.slice(4).map((call) => call.params)).toMatchObject([ + { terminal: 'term-1', text: '\x15', enter: false }, + { terminal: 'term-2', text: '\x15', enter: false } + ]) + expect(baseSend).toHaveBeenCalledTimes(4) + }) + it('reports a disconnected attach failure via the live connection state', async () => { const client = makeClient([]) const showToast = vi.fn() diff --git a/mobile/src/session/use-mobile-native-chat-image-attachments.ts b/mobile/src/session/use-mobile-native-chat-image-attachments.ts index fded5e2f547..4d8ea9d8eb6 100644 --- a/mobile/src/session/use-mobile-native-chat-image-attachments.ts +++ b/mobile/src/session/use-mobile-native-chat-image-attachments.ts @@ -71,6 +71,10 @@ function withScopeAttachments( return remaining } +function markTerminalInputStale(staleInputs: Set, terminal: string): void { + staleInputs.add(terminal) +} + const defaultSleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)) @@ -99,9 +103,10 @@ export function useMobileNativeChatImageAttachments({ // checked 'connected' at entry, so only a ref can see a mid-upload disconnect. const connStateRef = useRef(connState) connStateRef.current = connState - // Terminal whose input line may hold a partial paste from a failed send; the - // next send TO THAT terminal must lead with Ctrl+U even if it has no images. - const staleInputTerminalRef = useRef(null) + // Terminals whose input may hold a failed paste; each must heal independently. + const staleInputTerminalsRef = useRef(new Set()) + // Serialize clear/paste/submit ownership per terminal while allowing other tabs to send. + const sendInFlightTerminalsRef = useRef(new Set()) const attachments = (scopeKey ? attachmentsByScope[scopeKey] : undefined) ?? NO_ATTACHMENTS @@ -192,92 +197,120 @@ export function useMobileNativeChatImageAttachments({ const sendNativeChat = useCallback( async (text: string): Promise => { - const scope = scopeKey - const pendingImages = (scope ? attachmentsByScope[scope] : undefined) ?? NO_ATTACHMENTS - if (pendingImages.length === 0 || !scope) { - // Heal a previously failed paste: a text-only send to that terminal would - // otherwise glue the stale image paste onto this message. Best-effort — - // on failure the marker stays set and the send proceeds as before. - const staleTerminal = staleInputTerminalRef.current - if (staleTerminal && staleTerminal === activeHandleRef.current && client) { - try { - await pasteMobileNativeChatImagePaths({ - client, - terminal: staleTerminal, - deviceToken: deviceTokenRef.current, - imagePaths: [] - }) - staleInputTerminalRef.current = null - } catch { - // Leave marked for the next attempt. - } - } - return baseSend(text) - } - const handle = activeHandleRef.current - if (!client || !handle || !enabled || connState !== 'connected') { - onError?.() - // Mirror the text path's failure surface (the base send is never reached). - showToast('Message not sent (disconnected)', 1500) - return false - } - try { - const pasted = await pasteMobileNativeChatImagePaths({ - client, - terminal: handle, - deviceToken: deviceTokenRef.current, - imagePaths: pendingImages.map((attachment) => attachment.path) - }) - if (!pasted) { - // Keep the chips so the user can retry; the failed paste never submitted. - staleInputTerminalRef.current = handle - onError?.() - showToast('Message not sent', 1500) - return false - } - // The paste's leading Ctrl+U cleared any earlier stale input in `handle`. - if (staleInputTerminalRef.current === handle) { - staleInputTerminalRef.current = null - } - // Let the TUI absorb the image paste before the text + Enter follow. The - // preview URIs ride along to baseSend so the sent bubble shows the photo - // immediately (empty text still submits a bare Enter through baseSend). - await sleep(MOBILE_NATIVE_CHAT_IMAGE_SETTLE_MS) - // The paste above targeted `handle`; a tab switch during the settle would - // route the text + Enter to a different terminal than the images. Abort — - // the chips keep their scope and a retry's Ctrl+U clears the stale paste. - if (activeHandleRef.current !== handle) { - staleInputTerminalRef.current = handle - onError?.() - showToast('Message not sent', 1500) - return false - } - const accepted = await baseSend( - text, - pendingImages.map((attachment) => attachment.previewUri) - ) - if (accepted) { - // Drop only what rode along — a chip attached while this send was in - // flight keeps waiting for its own send. - const sentIds = new Set(pendingImages.map((attachment) => attachment.id)) - setAttachmentsByScope((prev) => - withScopeAttachments( - prev, - scope, - (prev[scope] ?? []).filter((attachment) => !sentIds.has(attachment.id)) - ) - ) - } - return accepted - } catch { - // A thrown paste/send (network/RPC) keeps the chips and honors the - // Promise contract instead of rejecting. Retry-safe: the next - // attempt's leading Ctrl+U clears whatever fraction of the paste landed. - staleInputTerminalRef.current = handle + const operationTerminal = activeHandleRef.current + if (operationTerminal && sendInFlightTerminalsRef.current.has(operationTerminal)) { onError?.() showToast('Message not sent', 1500) return false } + if (operationTerminal) { + sendInFlightTerminalsRef.current.add(operationTerminal) + } + try { + const scope = scopeKey + const pendingImages = (scope ? attachmentsByScope[scope] : undefined) ?? NO_ATTACHMENTS + if (pendingImages.length === 0 || !scope) { + // Heal a previously failed paste: a text-only send to that terminal would + // otherwise glue the stale image paste onto this message. Best-effort — + // on failure the marker stays set and the text must not be submitted. + const staleTerminal = activeHandleRef.current + if (staleTerminal && staleInputTerminalsRef.current.has(staleTerminal) && client) { + let cleared = false + try { + cleared = await pasteMobileNativeChatImagePaths({ + client, + terminal: staleTerminal, + deviceToken: deviceTokenRef.current, + imagePaths: [] + }) + } catch { + // Leave marked for the next attempt. + } + if (!cleared) { + onError?.() + showToast('Message not sent', 1500) + return false + } + staleInputTerminalsRef.current.delete(staleTerminal) + if (activeHandleRef.current !== staleTerminal) { + onError?.() + showToast('Message not sent', 1500) + return false + } + } + return baseSend(text) + } + const handle = activeHandleRef.current + if (!client || !handle || !enabled || connState !== 'connected') { + onError?.() + // Mirror the text path's failure surface (the base send is never reached). + showToast('Message not sent (disconnected)', 1500) + return false + } + try { + const pasted = await pasteMobileNativeChatImagePaths({ + client, + terminal: handle, + deviceToken: deviceTokenRef.current, + imagePaths: pendingImages.map((attachment) => attachment.path) + }) + if (!pasted) { + // Keep the chips so the user can retry; the failed paste never submitted. + markTerminalInputStale(staleInputTerminalsRef.current, handle) + onError?.() + showToast('Message not sent', 1500) + return false + } + // The paste's leading Ctrl+U cleared any earlier stale input in `handle`. + staleInputTerminalsRef.current.delete(handle) + // Let the TUI absorb the image paste before the text + Enter follow. The + // preview URIs ride along to baseSend so the sent bubble shows the photo + // immediately (empty text still submits a bare Enter through baseSend). + await sleep(MOBILE_NATIVE_CHAT_IMAGE_SETTLE_MS) + // The paste above targeted `handle`; a tab switch during the settle would + // route the text + Enter to a different terminal than the images. Abort — + // the chips keep their scope and a retry's Ctrl+U clears the stale paste. + if (activeHandleRef.current !== handle) { + markTerminalInputStale(staleInputTerminalsRef.current, handle) + onError?.() + showToast('Message not sent', 1500) + return false + } + const accepted = await baseSend( + text, + pendingImages.map((attachment) => attachment.previewUri) + ) + if (!accepted) { + // A rejected submit leaves the successfully pasted image path on this input line. + markTerminalInputStale(staleInputTerminalsRef.current, handle) + } + if (accepted) { + // Drop only what rode along — a chip attached while this send was in + // flight keeps waiting for its own send. + const sentIds = new Set(pendingImages.map((attachment) => attachment.id)) + setAttachmentsByScope((prev) => + withScopeAttachments( + prev, + scope, + (prev[scope] ?? []).filter((attachment) => !sentIds.has(attachment.id)) + ) + ) + } + return accepted + } catch { + // A thrown paste/send (network/RPC) keeps the chips and honors the + // Promise contract instead of rejecting. Retry-safe: the next + // attempt's leading Ctrl+U clears whatever fraction of the paste landed. + markTerminalInputStale(staleInputTerminalsRef.current, handle) + onError?.() + showToast('Message not sent', 1500) + return false + } + } finally { + if (operationTerminal) { + sendInFlightTerminalsRef.current.delete(operationTerminal) + } + } }, [ activeHandleRef, From d4387b17d95371321a40fec6222b9016281e727d Mon Sep 17 00:00:00 2001 From: gatsby74 <166927047+gatsby74@users.noreply.github.com> Date: Fri, 24 Jul 2026 01:07:59 +0200 Subject: [PATCH 07/10] fix(status-bar): Resource Manager closed badge terminal count + RAM seed (#9387) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(status-bar): seed Resource Manager closed badge from daemon inventory The closed chip counted tab/layout PTY wake hints (inflating terminal count) and never fetched memory until the popover opened (showing "—"). Cache listSessions for the badge, seed memory on session ready, and drop the wake-hint closed selector. * fix(status-bar): update session inventory ref in an effect CodeRabbit/React Doctor flagged mutating sessionInventoryRef during render; keep the write in useEffect after commit. * fix(status-bar): follow daemon session lifecycle events * perf(status-bar): skip inventory scans for known PTYs * perf(status-bar): bound resource inventory refreshes --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> --- config/reliability-gates.jsonc | 81 +++++- src/main/ipc/pty.test.ts | 56 +++- src/main/ipc/pty.ts | 10 + src/preload/api-types.ts | 1 + src/preload/index.ts | 6 + ...UsageStatusSegment.session-polling.test.ts | 35 ++- .../status-bar/ResourceUsageStatusSegment.tsx | 61 ++-- .../resource-session-count-selector.test.ts | 219 -------------- .../resource-session-count-selector.ts | 102 ------- .../resource-session-inventory.test.ts | 45 +++ .../status-bar/resource-session-inventory.ts | 43 +++ .../use-resource-session-inventory.test.tsx | 266 ++++++++++++++++++ .../use-resource-session-inventory.ts | 196 +++++++++++++ src/renderer/src/web/web-preload-api.ts | 1 + 14 files changed, 754 insertions(+), 368 deletions(-) delete mode 100644 src/renderer/src/components/status-bar/resource-session-count-selector.test.ts delete mode 100644 src/renderer/src/components/status-bar/resource-session-count-selector.ts create mode 100644 src/renderer/src/components/status-bar/resource-session-inventory.test.ts create mode 100644 src/renderer/src/components/status-bar/resource-session-inventory.ts create mode 100644 src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx create mode 100644 src/renderer/src/components/status-bar/use-resource-session-inventory.ts diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 7ec61073b60..9ac29a28a4f 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -1,6 +1,6 @@ { "schemaVersion": 1, - "updatedAt": "2026-07-20", + "updatedAt": "2026-07-22", "policy": { "maturityLevels": [ "experimental", @@ -2556,7 +2556,7 @@ "id": "terminal-performance.no-hot-list-sessions", "title": "Hot terminal interactions do not call global PTY session listing", "maturity": "experimental", - "protection": "none", + "protection": "partial", "owner": "terminal-performance", "layer": "ipc-count-contract", "surfaces": [ @@ -2580,19 +2580,74 @@ "wsl", "remote-runtime" ], - "coveredPlatforms": [], + "coveredPlatforms": [ + "macos" + ], "coveredProviders": [], - "coverageNotes": "Registered gap on main. The targeted-hasPty product hardening and no-hot count assertions exist only on the pending reliability stack. It registers here with its owning split PR.", + "coverageNotes": "Platform-neutral unit coverage proves the Resource Manager closed badge performs one readiness seed, coalesces unknown spawn signals, skips known-session reattach signals, and installs no interval. Broader terminal interaction coverage remains on the pending reliability stack.", "motivatingLinks": [ "https://github.com/stablyai/orca/pull/7002", - "https://github.com/stablyai/orca/pull/6858" + "https://github.com/stablyai/orca/pull/6858", + "https://github.com/stablyai/orca/issues/9386", + "https://github.com/stablyai/orca/pull/9387" ], "invariant": "Typing, focus, terminal switch, workspace switch, visibility resume, resize, render, and per-pane liveness paths must not call global pty:listSessions; they must use targeted per-PTY APIs or cached provider-owned state.", - "oracle": "The current executable slice asserts targeted visibility/first-input liveness, resize re-assertion after visibility resume, light tab/active-state resume, SSH/remote skip behavior, and closed Resource Manager status badges avoid pty:listSessions; targeted hasPty/getSize calls are allowed for liveness/resize slices and forbidden for light tab/active-state resume. The full hot-path oracle still needs instrumentation around raw focus, split focus, workspace switch, render ticks, and high-session local/daemon/SSH fixtures.", - "commands": [], - "testFiles": [], - "assertionRefs": [], - "evidenceRuns": [], + "oracle": "The current executable slice asserts targeted visibility/first-input liveness, resize re-assertion after visibility resume, light tab/active-state resume, SSH/remote skip behavior, and a closed Resource Manager budget of one readiness seed plus one coalesced inventory read only for unknown spawn IDs; known-session reattach signals and steady closed time perform zero reads. Targeted hasPty/getSize calls are allowed for liveness/resize slices and forbidden for light tab/active-state resume. The full hot-path oracle still needs instrumentation around raw focus, split focus, workspace switch, render ticks, and high-session local/daemon/SSH fixtures.", + "commands": [ + "pnpm exec vitest run --config config/vitest.config.ts src/main/ipc/pty.test.ts src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx src/renderer/src/components/status-bar/resource-session-inventory.test.ts src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts" + ], + "testFiles": [ + "src/main/ipc/pty.test.ts", + "src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx", + "src/renderer/src/components/status-bar/resource-session-inventory.test.ts", + "src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts" + ], + "assertionRefs": [ + { + "file": "src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx", + "assertions": [ + "the false-to-true workspace readiness transition performs one daemon inventory seed", + "a failed readiness seed surfaces an error and a later inventory refresh recovers", + "known-session reattach signals perform zero additional inventory reads", + "multiple unknown background spawn signals coalesce to one inventory read", + "spawn signals during a slow inventory read never overlap provider-wide scans and cause at most one required follow-up", + "unknown sessions that exit before reconciliation cancel their queued inventory read", + "unmount during a slow inventory read cannot schedule follow-up work", + "exit and out-of-order refresh races cannot resurrect stale sessions" + ] + }, + { + "file": "src/main/ipc/pty.test.ts", + "assertions": [ + "global inventory starts local and SSH provider listings concurrently" + ] + }, + { + "file": "src/renderer/src/components/status-bar/resource-session-inventory.test.ts", + "assertions": [ + "daemon inventory construction copies its source and preserves count parity", + "single and batch removals preserve unrelated sessions and no-op references" + ] + }, + { + "file": "src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts", + "assertions": [ + "the closed inventory hook installs no interval", + "the badge count comes from cached daemon inventory rather than wake-hint bindings" + ] + } + ], + "evidenceRuns": [ + { + "date": "2026-07-22", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/ipc/pty.test.ts src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx src/renderer/src/components/status-bar/resource-session-inventory.test.ts src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts", + "result": "passed", + "durationSeconds": 4.3, + "summary": "4 files and 358 tests passed, covering readiness seed/recovery, zero interval polling, bounded unknown-spawn reconciliation, concurrent provider starts, exit fencing, cleanup, and out-of-order refresh fencing." + } + ], "runtimeBudget": { "p95Seconds": 20, "scope": "unit or focused Electron count gate" @@ -2603,7 +2658,7 @@ }, "redGreenEvidence": { "status": "partial", - "evidence": "Tests assert visibility resume prefers targeted hasPty over listSessions, first input after visibility resume calls targeted hasPty once, resize re-assertion after visibility resume uses getSize/resize without listSessions, light tab switches and visible active-state resume avoid listSessions/hasPty/getSize fanout while still allowing the active PTY scheduler hint, SSH/remote broad listing is skipped, Resource Manager broad session inventory polling is scoped to the open popover rather than its closed badge, and panes close only on authoritative false. Needs broader raw focus/workspace-switch/render/high-session count coverage before promotion." + "evidence": "Tests assert visibility resume prefers targeted hasPty over listSessions, first input after visibility resume calls targeted hasPty once, resize re-assertion after visibility resume uses getSize/resize without listSessions, light tab switches and visible active-state resume avoid listSessions/hasPty/getSize fanout while still allowing the active PTY scheduler hint, SSH/remote broad listing is skipped, and the closed Resource Manager performs one readiness seed while known reattach signals and steady time perform no additional reads. Needs broader raw focus/workspace-switch/render/high-session count coverage before promotion." }, "performanceBudget": { "required": true, @@ -2615,8 +2670,8 @@ "Run with enough preserved sessions/providers to make a broad listing observable." ], "knownGaps": [ - "No executable coverage on main yet; the slice lives on the pending fix-terminal-reliability stack.", - "Current command covers targeted visibility/first-input liveness, resize re-assertion on visibility resume, light tab/active-state resume, SSH/remote skip behavior, and closed Resource Manager session-poll avoidance, but not every hot interaction listed in the invariant.", + "Current commands cover Resource Manager readiness/lifecycle inventory counts; the broader targeted-liveness slice still lives on the pending fix-terminal-reliability stack.", + "Current coverage includes the closed Resource Manager's no-interval and known-reattach budgets, but not every hot interaction listed in the invariant.", "No Electron or IPC-level high-session counter gate yet proves raw focus, workspace switch, render, or high-session typing stay at zero global listSessions calls." ], "demotionRule": "Cannot promote if the test allows broad listing in any hot interaction path." diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 0645cc0ffad..54a1851d30d 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -1674,6 +1674,14 @@ describe('registerPtyHandlers', () => { } describe('spawn environment', () => { + it('publishes a lifecycle signal after a successful renderer spawn', async () => { + await spawnAndGetEnv() + + expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:spawned', { + id: expect.any(String) + }) + }) + it('marks local Claude launches live until the PTY is killed', async () => { let exitCb: ((info: { exitCode: number }) => void) | undefined spawnMock.mockReturnValue({ @@ -5472,6 +5480,48 @@ describe('registerPtyHandlers', () => { }) }) + it('starts local and SSH session inventories concurrently', async () => { + let resolveLocal!: (sessions: { id: string; cwd: string; title: string }[]) => void + const localSessions = new Promise<{ id: string; cwd: string; title: string }[]>((resolve) => { + resolveLocal = resolve + }) + vi.spyOn(getLocalPtyProvider(), 'listProcesses').mockReturnValue(localSessions) + registerPtyHandlers(mainWindow as never) + + let resolveSsh!: (sessions: { id: string; cwd: string; title: string }[]) => void + const sshSessions = new Promise<{ id: string; cwd: string; title: string }[]>((resolve) => { + resolveSsh = resolve + }) + const sshListProcesses = vi.fn(() => sshSessions) + registerSshPtyProvider('ssh-1', { + spawn: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + shutdown: vi.fn(), + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + onData: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses: sshListProcesses, + hasChildProcesses: vi.fn(), + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + + const pendingInventory = handlers.get('pty:listSessions')!(null, undefined) + + expect(sshListProcesses).toHaveBeenCalledTimes(1) + resolveLocal([]) + resolveSsh([]) + await pendingInventory + }) + it('reports authoritative snapshot capability with the owning provider context', () => { const capabilityProvider = { authoritativeIds: new Set(['current-pty']), @@ -6479,7 +6529,7 @@ describe('registerPtyHandlers', () => { registerPtyHandlers(mainWindow as never, runtime as never) expect(controller).not.toBeNull() const spawnController = controller as unknown as RuntimeSpawnController - await spawnController.spawn({ + const spawned = await spawnController.spawn({ cols: 80, rows: 24, worktreeId: 'wt-1', @@ -6494,6 +6544,9 @@ describe('registerPtyHandlers', () => { expect.any(String), 'term_expected' ) + expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:spawned', { + id: spawned.id + }) }) it('does not update cached PTY size when runtime controller resize fails', async () => { @@ -10265,6 +10318,7 @@ describe('registerPtyHandlers', () => { cwd: '/tmp' })) as { id: string } const writeListener = getPtyWriteListener() + mainWindow.webContents.send.mockClear() const pendingOutput = 'x'.repeat(1020) mockProc.emitData(pendingOutput) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 95f14c1125a..ea528aca371 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -2709,6 +2709,12 @@ export function registerPtyHandlers( }) } + function sendPtySpawnedToRenderer(id: string): void { + if (!mainWindow.isDestroyed()) { + mainWindow.webContents.send('pty:spawned', { id }) + } + } + function sendPtyReplayToRenderer(payload: { id: string; data: string }): void { const pending = pendingData.get(payload.id) if (pending) { @@ -3771,6 +3777,8 @@ export function registerPtyHandlers( : null }) } + // Why: runtime-owned/background spawns bypass mounted-pane state, so inventory consumers need an explicit signal. + sendPtySpawnedToRenderer(result.id) const response = { id: result.id, ...(result.incarnationId ? { incarnationId: result.incarnationId } : {}), @@ -4943,6 +4951,8 @@ export function registerPtyHandlers( // Why: a daemon-retry race can surface isReattach even for a minted session id, and a reattach must never claim its cwd was remapped. ...(startupCwdFallback && !result.isReattach ? { startupCwdFallback } : {}) } + // Why: renderer tab state cannot reliably infer background and reattached PTYs in the daemon inventory. + sendPtySpawnedToRenderer(result.id) return resolvePaneSpawnReservation(reservationPaneKey, paneSpawnReservation, response) } catch (err) { if (pendingRegistrationPtyId) { diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index ce2a7eb08b1..265fa659a0c 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1458,6 +1458,7 @@ export type PreloadApi = { onExit: ( callback: (data: { id: string; code: number; preserveRendererBinding?: boolean }) => void ) => () => void + onSpawned: (callback: (data: { id: string }) => void) => () => void onSerializeBufferRequest: ( callback: (data: { requestId: string diff --git a/src/preload/index.ts b/src/preload/index.ts index 1de8efbfed4..b6244cc4e44 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1064,6 +1064,12 @@ const api = { return () => ipcRenderer.removeListener('pty:exit', listener) }, + onSpawned: (callback: (data: { id: string }) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, data: { id: string }) => callback(data) + ipcRenderer.on('pty:spawned', listener) + return () => ipcRenderer.removeListener('pty:spawned', listener) + }, + onSerializeBufferRequest: ( callback: (data: { requestId: string diff --git a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts index e29871f0a17..8dc4a515a23 100644 --- a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts +++ b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.session-polling.test.ts @@ -3,21 +3,50 @@ import { resolve } from 'node:path' import { describe, expect, it } from 'vitest' const SOURCE_PATH = resolve(__dirname, 'ResourceUsageStatusSegment.tsx') +const INVENTORY_HOOK_PATH = resolve(__dirname, 'use-resource-session-inventory.ts') -describe('ResourceUsageStatusSegment session polling', () => { +describe('ResourceUsageStatusSegment session inventory', () => { it('does not poll global terminal sessions while the popover is closed', () => { const source = readFileSync(SOURCE_PATH, 'utf8') + const inventoryHookSource = readFileSync(INVENTORY_HOOK_PATH, 'utf8') expect(source).not.toContain('installWindowVisibilityInterval') expect(source).not.toContain('SESSIONS_POLL_MS') - expect(source.match(/window\.api\.pty\.listSessions\(\)/g) ?? []).toHaveLength(1) + expect(inventoryHookSource).not.toContain('setInterval') + // Why: every seed/action/lifecycle refresh shares one guarded inventory + // read, and the closed path never installs a polling interval. + expect(inventoryHookSource.match(/window\.api\.pty\.listSessions\(\)/g) ?? []).toHaveLength(1) const openEffectIndex = source.indexOf('if (!open)') const refreshIndex = source.indexOf('void refreshSessions()', openEffectIndex) // Why: pty.listSessions() is a global daemon inventory and can pause input - // with large preserved-session sets. Keep it on explicit Resource Manager use. + // with large preserved-session sets. Keep continuous use off the closed path. expect(openEffectIndex).toBeGreaterThanOrEqual(0) expect(refreshIndex).toBeGreaterThan(openEffectIndex) }) + + it('seeds the closed badge from daemon inventory instead of wake-hint bound PTYs', () => { + const source = readFileSync(SOURCE_PATH, 'utf8') + const inventoryHookSource = readFileSync(INVENTORY_HOOK_PATH, 'utf8') + + expect(source).toContain('useResourceSessionInventory') + expect(source).toContain('sessionInventory.count') + expect(inventoryHookSource).toContain('window.api.pty.onSpawned') + expect(inventoryHookSource).toContain('window.api.pty.onExit') + expect(source).not.toContain('createClosedResourceSessionCountSelector') + expect(source).not.toContain('boundPtyIds.size') + expect(source).not.toContain('closedSessionCount') + expect(source).not.toContain('livePtyIdsByTabId') + }) + + it('seeds memory snapshot for the closed badge without requiring a click', () => { + const source = readFileSync(SOURCE_PATH, 'utf8') + + // Why: the ready-seed effect must call fetchSnapshot so RAM is not "—" + // until the user opens Resource Manager. + const readySeedBlock = source.slice(source.indexOf('// Why: seed RAM after session restore')) + expect(readySeedBlock).toContain('void fetchSnapshot()') + expect(readySeedBlock).toContain('workspaceSessionReady') + }) }) diff --git a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx index 6f29ed85a66..8b57c00651c 100644 --- a/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx +++ b/src/renderer/src/components/status-bar/ResourceUsageStatusSegment.tsx @@ -38,7 +38,6 @@ import { isFolderRepo } from '../../../../shared/repo-kind' import { isWorkspaceOldForCleanup } from '../../../../shared/workspace-cleanup' import { mergeSnapshotAndSessions, UNATTRIBUTED_REPO_ID } from './mergeSnapshotAndSessions' import type { - DaemonSession, Metric, UnifiedProjectGroup, UnifiedSessionRow, @@ -72,11 +71,10 @@ import { countUnboundDaemonSessions, type ResourceSessionBindingInputs } from './resource-session-bindings' -import { createClosedResourceSessionCountSelector } from './resource-session-count-selector' +import { useResourceSessionInventory } from './use-resource-session-inventory' import { translate } from '@/i18n/i18n' const POLL_MS = 2_000 -const selectClosedResourceSessionCount = createClosedResourceSessionCountSelector() type SortOption = 'memory' | 'cpu' | 'name' @@ -733,7 +731,6 @@ export function ResourceUsageStatusSegment({ const memorySnapshotError = useAppStore((s) => s.memorySnapshotError) const fetchSnapshot = useAppStore((s) => s.fetchMemorySnapshot) const workspaceSessionReady = useAppStore((s) => s.workspaceSessionReady) - const closedSessionCount = useAppStore(selectClosedResourceSessionCount) const setActiveView = useAppStore((s) => s.setActiveView) const openModal = useAppStore((s) => s.openModal) const openSpacePage = useAppStore((s) => s.openSpacePage) @@ -748,8 +745,15 @@ export function ResourceUsageStatusSegment({ const [collapsedRepos, setCollapsedRepos] = useState>(new Set()) const [collapsedWorktrees, setCollapsedWorktrees] = useState>(new Set()) const [appCollapsed, setAppCollapsed] = useState(true) - const [sessions, setSessions] = useState([]) - const [sessionsError, setSessionsError] = useState(false) + const { + sessionInventory, + sessionsError, + refreshSessions, + clearSessionsError, + removeSession, + removeSessions + } = useResourceSessionInventory(workspaceSessionReady) + const sessions = sessionInventory.sessions const [killConfirm, setKillConfirm] = useState(null) const [killing, setKilling] = useState(false) const [spaceScanSnapshot, setSpaceScanSnapshot] = useState( @@ -806,24 +810,9 @@ export function ResourceUsageStatusSegment({ [cancelPopoverBodyFocusFrame] ) - const refreshSessions = useCallback(async () => { - try { - const result = await window.api.pty.listSessions() - if (!mountedRef.current) { - return - } - setSessions(result) - setSessionsError(false) - } catch { - if (mountedRef.current) { - setSessionsError(true) - } - } - }, [mountedRef]) - const daemonActions = useDaemonActions({ onRestartSettled: () => { - setSessionsError(false) + clearSessionsError() void fetchSnapshot() void refreshSessions() }, @@ -850,7 +839,17 @@ export function ResourceUsageStatusSegment({ } const spaceScanReady = nextSpaceScanSnapshot.ready - // Poll memory only while open; a closed badge must not inventory daemon PTYs (large preserved-session sets stall typing). + // Why: seed RAM after session restore so the closed chip does not require a + // click; the session-inventory hook independently seeds daemon PTYs. + useEffect(() => { + if (workspaceSessionReady) { + void fetchSnapshot() + } + }, [workspaceSessionReady, fetchSnapshot]) + + // Poll memory only while the popover is open. Session inventory is still + // explicit-on-open/action/seed (not a closed interval) because full + // listSessions can pause input with large preserved-session sets. useEffect(() => { if (!open) { return @@ -959,7 +958,9 @@ export function ResourceUsageStatusSegment({ return countUnboundDaemonSessions(sessions, resourceSessionBindings) }, [open, sessions, resourceSessionBindings, workspaceSessionReady]) - const triggerSessionCount = open ? sessions.length : closedSessionCount + // Why: open and closed badges share the same daemon inventory cache. The old + // closed path used boundPtyIds (wake hints) and inflated the chip to 60+. + const triggerSessionCount = sessionInventory.count const { totalMemory, totalCpu, hostShare, memBadgeLabel } = useMemo(() => { const memory = resourceSnapshot?.totalMemory ?? 0 @@ -1046,7 +1047,7 @@ export function ResourceUsageStatusSegment({ (session: UnifiedSessionRow): void => { // Why: orphan sessions have no tab here (no unsaved work to lose), so skip the confirm dialog; bound sessions still confirm. if (!session.bound) { - setSessions((prev) => prev.filter((s) => s.id !== session.sessionId)) + removeSession(session.sessionId) // Why: await the kill before refreshing, else the refresh re-reads the daemon list before the kill lands and re-adds the row. void (async () => { try { @@ -1060,7 +1061,7 @@ export function ResourceUsageStatusSegment({ } setKillConfirm(session) }, - [refreshSessions] + [refreshSessions, removeSession] ) const handleKillOrphans = useCallback(async () => { @@ -1074,10 +1075,10 @@ export function ResourceUsageStatusSegment({ } // Why: optimistic removal so rows disappear immediately instead of waiting for the next daemon-side list refresh. const orphanIds = new Set(orphans.map((s) => s.id)) - setSessions((prev) => prev.filter((s) => !orphanIds.has(s.id))) + removeSessions(orphanIds) await Promise.allSettled(orphans.map((s) => window.api.pty.kill(s.id))) void refreshSessions() - }, [sessions, resourceSessionBindings, workspaceSessionReady, refreshSessions]) + }, [sessions, resourceSessionBindings, workspaceSessionReady, refreshSessions, removeSessions]) const runKillConfirmed = useCallback(async () => { if (!killConfirm) { @@ -1086,7 +1087,7 @@ export function ResourceUsageStatusSegment({ const target = killConfirm setKilling(true) // Why: optimistic removal avoids a flash where the dialog closes but the killed row lingers until the next list refresh. - setSessions((prev) => prev.filter((s) => s.id !== target.sessionId)) + removeSession(target.sessionId) try { await window.api.pty.kill(target.sessionId) } catch { @@ -1106,7 +1107,7 @@ export function ResourceUsageStatusSegment({ void refreshSessions() } } - }, [cancelPopoverBodyFocusFrame, killConfirm, mountedRef, refreshSessions]) + }, [cancelPopoverBodyFocusFrame, killConfirm, mountedRef, refreshSessions, removeSession]) const openSpaceResults = useCallback((): void => { setOpen(false) diff --git a/src/renderer/src/components/status-bar/resource-session-count-selector.test.ts b/src/renderer/src/components/status-bar/resource-session-count-selector.test.ts deleted file mode 100644 index 7d416ff856b..00000000000 --- a/src/renderer/src/components/status-bar/resource-session-count-selector.test.ts +++ /dev/null @@ -1,219 +0,0 @@ -import { describe, expect, it, vi } from 'vitest' -import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/types' -import { - buildResourceSessionBindingIndex, - type ResourceSessionBindingInputs -} from './resource-session-bindings' -import { - createClosedResourceSessionCountSelector, - type ClosedResourceSessionCountState -} from './resource-session-count-selector' - -const TAB_COUNT = 100 -const TITLE_WRITES = 600 - -function makeTab(id: string, ptyId: string | null = null, title = 'Terminal'): TerminalTab { - return { - id, - ptyId, - worktreeId: `wt-${id}`, - title, - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 0 - } -} - -function makeLayout(ptyIdsByLeafId: Record): TerminalLayoutSnapshot { - return { - root: { type: 'leaf', leafId: 'leaf-1' }, - activeLeafId: 'leaf-1', - expandedLeafId: null, - ptyIdsByLeafId - } -} - -function makeState( - overrides: Partial = {} -): ClosedResourceSessionCountState { - return { - tabsByWorktree: {}, - ptyIdsByTabId: {}, - terminalLayoutsByTabId: {}, - workspaceSessionReady: true, - ...overrides - } -} - -function withCountedIteration(tabs: TerminalTab[], onVisit: () => void): TerminalTab[] { - return new Proxy(tabs, { - get(target, property, receiver) { - if (property !== Symbol.iterator) { - return Reflect.get(target, property, receiver) - } - return function* countedIterator(): Generator { - for (const tab of target) { - onVisit() - yield tab - } - } - } - }) -} - -function makeScaleTabs(onVisit: () => void): Record { - return Object.fromEntries( - Array.from({ length: TAB_COUNT }, (_, index) => { - const worktreeId = `wt-${index}` - return [worktreeId, withCountedIteration([makeTab(`tab-${index}`, `pty-${index}`)], onVisit)] - }) - ) -} - -function countNextTabComparison( - tab: TerminalTab, - onComparison: () => void -): [TerminalTab[], () => void] { - let counting = true - const tabs = new Proxy([tab], { - get(target, property, receiver) { - if (counting && property === '0') { - onComparison() - } - return Reflect.get(target, property, receiver) - } - }) - return [ - tabs, - () => { - counting = false - } - ] -} - -describe('closed resource session count selector', () => { - it('removes full binding-index rebuilds from title-only tab churn at scale', () => { - let oldPathTabVisits = 0 - const initialTabs = makeScaleTabs(() => { - oldPathTabVisits += 1 - }) - const inputs: ResourceSessionBindingInputs = { - tabsByWorktree: initialTabs, - ptyIdsByTabId: {}, - terminalLayoutsByTabId: {}, - workspaceSessionReady: true - } - - // Why: this models the previous useMemo dependency on tabsByWorktree: - // every title write replaces that map and rebuilds both tab passes. - for (let index = 0; index < TITLE_WRITES; index += 1) { - buildResourceSessionBindingIndex({ ...inputs, tabsByWorktree: { ...initialTabs } }) - } - expect(oldPathTabVisits).toBe(120_000) - - let optimizedTabVisits = 0 - const optimizedTabs = makeScaleTabs(() => { - optimizedTabVisits += 1 - }) - const buildIndex = vi.fn(buildResourceSessionBindingIndex) - const selectCount = createClosedResourceSessionCountSelector(buildIndex) - let state = makeState({ tabsByWorktree: optimizedTabs }) - const currentTabByWorktree = Object.fromEntries( - Object.entries(optimizedTabs).map(([worktreeId, tabs]) => [worktreeId, tabs[0]]) - ) - let changedArrayTabComparisons = 0 - - expect(selectCount(state)).toBe(TAB_COUNT) - buildIndex.mockClear() - optimizedTabVisits = 0 - - for (let index = 0; index < TITLE_WRITES; index += 1) { - const worktreeId = `wt-${index % TAB_COUNT}` - const nextTab = { ...currentTabByWorktree[worktreeId], title: `Terminal ${index}` } - const [nextTabs, stopCounting] = countNextTabComparison(nextTab, () => { - changedArrayTabComparisons += 1 - }) - state = { - ...state, - tabsByWorktree: { - ...state.tabsByWorktree, - [worktreeId]: nextTabs - } - } - expect(selectCount(state)).toBe(TAB_COUNT) - stopCounting() - currentTabByWorktree[worktreeId] = nextTab - } - - expect(buildIndex).not.toHaveBeenCalled() - expect(optimizedTabVisits).toBe(0) - expect(changedArrayTabComparisons).toBe(TITLE_WRITES) - }) - - it('reacts to every binding and readiness input while ignoring display-only tab fields', () => { - const buildIndex = vi.fn(buildResourceSessionBindingIndex) - const selectCount = createClosedResourceSessionCountSelector(buildIndex) - let state = makeState({ - workspaceSessionReady: false, - tabsByWorktree: { - 'wt-1': [makeTab('tab-1', 'pty-wake')] - }, - ptyIdsByTabId: { - 'tab-1': ['pty-live'] - }, - terminalLayoutsByTabId: { - 'tab-1': makeLayout({ 'leaf-1': 'pty-layout' }) - } - }) - - expect(selectCount(state)).toBe(0) - expect(buildIndex).not.toHaveBeenCalled() - - state = { ...state, workspaceSessionReady: true } - expect(selectCount(state)).toBe(3) - expect(buildIndex).toHaveBeenCalledTimes(1) - - state = { - ...state, - tabsByWorktree: { - 'wt-1': [{ ...state.tabsByWorktree['wt-1'][0], title: 'Working' }] - } - } - expect(selectCount(state)).toBe(3) - expect(buildIndex).toHaveBeenCalledTimes(1) - - state = { - ...state, - ptyIdsByTabId: { 'tab-1': ['pty-live', 'pty-live-2'] } - } - expect(selectCount(state)).toBe(4) - expect(buildIndex).toHaveBeenCalledTimes(2) - - state = { - ...state, - terminalLayoutsByTabId: { - 'tab-1': makeLayout({ 'leaf-1': 'pty-layout', 'leaf-2': 'pty-layout-2' }) - } - } - expect(selectCount(state)).toBe(5) - expect(buildIndex).toHaveBeenCalledTimes(3) - - state = { - ...state, - tabsByWorktree: { - 'wt-1': [{ ...state.tabsByWorktree['wt-1'][0], ptyId: null }] - } - } - expect(selectCount(state)).toBe(4) - expect(buildIndex).toHaveBeenCalledTimes(4) - - state = { ...state, tabsByWorktree: {} } - expect(selectCount(state)).toBe(2) - expect(buildIndex).toHaveBeenCalledTimes(5) - - state = { ...state, workspaceSessionReady: false } - expect(selectCount(state)).toBe(0) - expect(buildIndex).toHaveBeenCalledTimes(5) - }) -}) diff --git a/src/renderer/src/components/status-bar/resource-session-count-selector.ts b/src/renderer/src/components/status-bar/resource-session-count-selector.ts deleted file mode 100644 index 4fb924f4529..00000000000 --- a/src/renderer/src/components/status-bar/resource-session-count-selector.ts +++ /dev/null @@ -1,102 +0,0 @@ -import type { AppState } from '../../store' -import { - buildResourceSessionBindingIndex, - type ResourceSessionBindingIndex, - type ResourceSessionBindingInputs -} from './resource-session-bindings' - -export type ClosedResourceSessionCountState = Pick< - AppState, - 'tabsByWorktree' | 'ptyIdsByTabId' | 'terminalLayoutsByTabId' | 'workspaceSessionReady' -> - -type BuildResourceSessionBindingIndex = ( - inputs: ResourceSessionBindingInputs -) => ResourceSessionBindingIndex - -export type ClosedResourceSessionCountSelector = (state: ClosedResourceSessionCountState) => number - -function haveSameTabBindings( - previous: AppState['tabsByWorktree'], - next: AppState['tabsByWorktree'] -): boolean { - if (previous === next) { - return true - } - - const previousWorktreeIds = Object.keys(previous) - const nextWorktreeIds = Object.keys(next) - if (previousWorktreeIds.length !== nextWorktreeIds.length) { - return false - } - - for (const worktreeId of nextWorktreeIds) { - const previousTabs = previous[worktreeId] - const nextTabs = next[worktreeId] - if (previousTabs === nextTabs) { - continue - } - if (!previousTabs || previousTabs.length !== nextTabs.length) { - return false - } - for (let index = 0; index < nextTabs.length; index += 1) { - const previousTab = previousTabs[index] - const nextTab = nextTabs[index] - // Why: the closed badge counts PTY ownership only. Titles and other - // display fields can churn per terminal frame without changing it. - if (previousTab.id !== nextTab.id || previousTab.ptyId !== nextTab.ptyId) { - return false - } - } - } - - return true -} - -export function createClosedResourceSessionCountSelector( - buildBindingIndex: BuildResourceSessionBindingIndex = buildResourceSessionBindingIndex -): ClosedResourceSessionCountSelector { - // Why: Zustand runs selectors for every store notification. Keep the last - // liveness inputs here so unrelated and title-only writes stay scalar-cheap. - let initialized = false - let previousTabsByWorktree: AppState['tabsByWorktree'] = {} - let previousPtyIdsByTabId: AppState['ptyIdsByTabId'] = {} - let previousTerminalLayoutsByTabId: AppState['terminalLayoutsByTabId'] = {} - let previousWorkspaceSessionReady = false - let count = 0 - - return (state): number => { - const bindingMapChanged = - state.ptyIdsByTabId !== previousPtyIdsByTabId || - state.terminalLayoutsByTabId !== previousTerminalLayoutsByTabId - const readinessChanged = state.workspaceSessionReady !== previousWorkspaceSessionReady - const tabsReferenceChanged = state.tabsByWorktree !== previousTabsByWorktree - let tabBindingsChanged = tabsReferenceChanged - if ( - initialized && - state.workspaceSessionReady && - !bindingMapChanged && - !readinessChanged && - tabsReferenceChanged - ) { - tabBindingsChanged = !haveSameTabBindings(previousTabsByWorktree, state.tabsByWorktree) - } - - const shouldRebuild = - state.workspaceSessionReady && - (!initialized || bindingMapChanged || readinessChanged || tabBindingsChanged) - - if (shouldRebuild) { - count = buildBindingIndex(state).boundPtyIds.size - } else if (!state.workspaceSessionReady) { - count = 0 - } - - previousTabsByWorktree = state.tabsByWorktree - previousPtyIdsByTabId = state.ptyIdsByTabId - previousTerminalLayoutsByTabId = state.terminalLayoutsByTabId - previousWorkspaceSessionReady = state.workspaceSessionReady - initialized = true - return count - } -} diff --git a/src/renderer/src/components/status-bar/resource-session-inventory.test.ts b/src/renderer/src/components/status-bar/resource-session-inventory.test.ts new file mode 100644 index 00000000000..a26371f040f --- /dev/null +++ b/src/renderer/src/components/status-bar/resource-session-inventory.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' +import { + EMPTY_DAEMON_SESSION_INVENTORY, + inventoryFromSessions, + removeSessionFromInventory, + removeSessionsFromInventory +} from './resource-session-inventory' +import type { DaemonSession } from './resource-usage-merge-types' + +function session(id: string): DaemonSession { + return { id, cwd: '/workspace', title: id } +} + +describe('resource session inventory', () => { + it('builds count from daemon listSessions payloads', () => { + const inventory = inventoryFromSessions([session('a'), session('b')]) + expect(inventory.count).toBe(2) + expect(inventory.sessions.map((entry) => entry.id)).toEqual(['a', 'b']) + }) + + it('returns a detached sessions array so callers can mutate safely', () => { + const source = [session('a')] + const inventory = inventoryFromSessions(source) + source.pop() + expect(inventory.sessions).toEqual([session('a')]) + expect(inventory.count).toBe(1) + }) + + it('removes killed or exited sessions without inventing wake-hint ids', () => { + const start = inventoryFromSessions([session('live'), session('orphan'), session('other')]) + const afterOne = removeSessionFromInventory(start, 'orphan') + expect(afterOne.count).toBe(2) + expect(afterOne.sessions.map((entry) => entry.id)).toEqual(['live', 'other']) + + const afterMany = removeSessionsFromInventory(start, new Set(['live', 'missing', 'other'])) + expect(afterMany).toEqual(inventoryFromSessions([session('orphan')])) + }) + + it('is a no-op when removed ids are absent', () => { + const start = inventoryFromSessions([session('live')]) + expect(removeSessionFromInventory(start, 'gone')).toBe(start) + expect(removeSessionsFromInventory(start, new Set())).toBe(start) + expect(EMPTY_DAEMON_SESSION_INVENTORY.count).toBe(0) + }) +}) diff --git a/src/renderer/src/components/status-bar/resource-session-inventory.ts b/src/renderer/src/components/status-bar/resource-session-inventory.ts new file mode 100644 index 00000000000..fda71c1beae --- /dev/null +++ b/src/renderer/src/components/status-bar/resource-session-inventory.ts @@ -0,0 +1,43 @@ +import type { DaemonSession } from './resource-usage-merge-types' + +/** Last-known daemon terminal inventory for the Resource Manager badge. */ +export type DaemonSessionInventory = { + sessions: DaemonSession[] + count: number +} + +export const EMPTY_DAEMON_SESSION_INVENTORY: DaemonSessionInventory = { + sessions: [], + count: 0 +} + +export function inventoryFromSessions(sessions: readonly DaemonSession[]): DaemonSessionInventory { + return { + sessions: sessions.slice(), + count: sessions.length + } +} + +export function removeSessionsFromInventory( + inventory: DaemonSessionInventory, + sessionIds: ReadonlySet +): DaemonSessionInventory { + if (sessionIds.size === 0 || inventory.sessions.length === 0) { + return inventory + } + const sessions = inventory.sessions.filter((session) => !sessionIds.has(session.id)) + if (sessions.length === inventory.sessions.length) { + return inventory + } + return { + sessions, + count: sessions.length + } +} + +export function removeSessionFromInventory( + inventory: DaemonSessionInventory, + sessionId: string +): DaemonSessionInventory { + return removeSessionsFromInventory(inventory, new Set([sessionId])) +} diff --git a/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx b/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx new file mode 100644 index 00000000000..124a5052221 --- /dev/null +++ b/src/renderer/src/components/status-bar/use-resource-session-inventory.test.tsx @@ -0,0 +1,266 @@ +// @vitest-environment happy-dom +import { act, renderHook, waitFor } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { DaemonSession } from './resource-usage-merge-types' +import { useResourceSessionInventory } from './use-resource-session-inventory' + +function session(id: string): DaemonSession { + return { id, cwd: '/workspace', title: id } +} + +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +describe('useResourceSessionInventory', () => { + const listSessions = vi.fn<() => Promise>() + const unsubscribeSpawned = vi.fn() + const unsubscribeExit = vi.fn() + let spawnedCallback: ((data: { id: string }) => void) | null = null + let exitCallback: ((data: { id: string; code: number }) => void) | null = null + + beforeEach(() => { + spawnedCallback = null + exitCallback = null + listSessions.mockReset() + unsubscribeSpawned.mockReset() + unsubscribeExit.mockReset() + ;(window as unknown as { api: unknown }).api = { + pty: { + listSessions, + onSpawned: (callback: (data: { id: string }) => void) => { + spawnedCallback = callback + return unsubscribeSpawned + }, + onExit: (callback: (data: { id: string; code: number }) => void) => { + exitCallback = callback + return unsubscribeExit + } + } + } + }) + + afterEach(() => { + delete (window as unknown as { api?: unknown }).api + }) + + it('seeds from the daemon inventory and resets when session restore is not ready', async () => { + listSessions.mockResolvedValue([session('one'), session('two')]) + const { result, rerender } = renderHook(({ ready }) => useResourceSessionInventory(ready), { + initialProps: { ready: false } + }) + + expect(result.current.sessionInventory.count).toBe(0) + expect(listSessions).not.toHaveBeenCalled() + + rerender({ ready: true }) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(2)) + expect(listSessions).toHaveBeenCalledTimes(1) + + rerender({ ready: false }) + expect(result.current.sessionInventory.count).toBe(0) + expect(result.current.sessionsError).toBe(false) + }) + + it('recovers inventory and clears the error after a failed readiness seed', async () => { + listSessions + .mockRejectedValueOnce(new Error('daemon unavailable')) + .mockResolvedValueOnce([session('recovered')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + + await waitFor(() => expect(result.current.sessionsError).toBe(true)) + await act(async () => { + await result.current.refreshSessions() + }) + + expect(result.current.sessionInventory.sessions).toEqual([session('recovered')]) + expect(result.current.sessionsError).toBe(false) + }) + + it('refreshes for background spawns without depending on mounted pane state', async () => { + listSessions + .mockResolvedValueOnce([session('one')]) + .mockResolvedValue([session('one'), session('background')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + await act(async () => { + spawnedCallback?.({ id: 'one' }) + spawnedCallback?.({ id: 'background' }) + spawnedCallback?.({ id: 'background-2' }) + await new Promise((resolve) => window.setTimeout(resolve, 0)) + }) + + await waitFor(() => expect(result.current.sessionInventory.count).toBe(2)) + expect(listSessions).toHaveBeenCalledTimes(2) + }) + + it('does not inventory again when an existing session reattaches', async () => { + listSessions.mockResolvedValue([session('one')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + act(() => { + spawnedCallback?.({ id: 'one' }) + }) + + expect(listSessions).toHaveBeenCalledTimes(1) + }) + + it('does not overlap provider-wide inventory reads for spawns during a slow refresh', async () => { + listSessions.mockResolvedValueOnce([session('one')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + const inFlight = deferred() + listSessions.mockReturnValueOnce(inFlight.promise) + await act(async () => { + spawnedCallback?.({ id: 'background-one' }) + await new Promise((resolve) => window.setTimeout(resolve, 0)) + }) + expect(listSessions).toHaveBeenCalledTimes(2) + + await act(async () => { + spawnedCallback?.({ id: 'background-two' }) + await new Promise((resolve) => window.setTimeout(resolve, 0)) + }) + expect(listSessions).toHaveBeenCalledTimes(2) + + await act(async () => { + inFlight.resolve([session('one'), session('background-one'), session('background-two')]) + await inFlight.promise + }) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(3)) + expect(listSessions).toHaveBeenCalledTimes(2) + }) + + it('reconciles once when an in-flight inventory misses a later spawn', async () => { + listSessions.mockResolvedValueOnce([session('one')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + const inFlight = deferred() + listSessions + .mockReturnValueOnce(inFlight.promise) + .mockResolvedValueOnce([session('one'), session('background-one'), session('background-two')]) + await act(async () => { + spawnedCallback?.({ id: 'background-one' }) + await new Promise((resolve) => window.setTimeout(resolve, 0)) + }) + act(() => { + spawnedCallback?.({ id: 'background-two' }) + }) + + await act(async () => { + inFlight.resolve([session('one'), session('background-one')]) + await inFlight.promise + }) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(3)) + expect(listSessions).toHaveBeenCalledTimes(3) + }) + + it('cancels a queued inventory read when the unknown session exits first', async () => { + listSessions.mockResolvedValueOnce([session('one')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + act(() => { + spawnedCallback?.({ id: 'short-lived' }) + exitCallback?.({ id: 'short-lived', code: 0 }) + }) + await new Promise((resolve) => window.setTimeout(resolve, 0)) + + expect(listSessions).toHaveBeenCalledTimes(1) + expect(result.current.sessionInventory.count).toBe(1) + }) + + it('does not schedule follow-up inventory after unmount', async () => { + listSessions.mockResolvedValueOnce([session('one')]) + const { result, unmount } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + const inFlight = deferred() + listSessions.mockReturnValueOnce(inFlight.promise) + await act(async () => { + spawnedCallback?.({ id: 'background-one' }) + await new Promise((resolve) => window.setTimeout(resolve, 0)) + }) + act(() => { + spawnedCallback?.({ id: 'background-two' }) + }) + unmount() + + inFlight.resolve([session('one'), session('background-one')]) + await inFlight.promise + await new Promise((resolve) => window.setTimeout(resolve, 0)) + + expect(listSessions).toHaveBeenCalledTimes(2) + }) + + it('filters an exit from an in-flight list without losing other new sessions', async () => { + listSessions.mockResolvedValueOnce([session('one'), session('exited')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(2)) + + const stale = deferred() + listSessions.mockReturnValueOnce(stale.promise) + let refresh!: Promise + act(() => { + refresh = result.current.refreshSessions() + }) + act(() => { + exitCallback?.({ id: 'exited', code: 0 }) + }) + expect(result.current.sessionInventory.sessions.map(({ id }) => id)).toEqual(['one']) + + await act(async () => { + stale.resolve([session('one'), session('exited'), session('background')]) + await refresh + }) + expect(result.current.sessionInventory.sessions.map(({ id }) => id)).toEqual([ + 'one', + 'background' + ]) + }) + + it('keeps the newest result when refreshes resolve out of order', async () => { + listSessions.mockResolvedValueOnce([session('one')]) + const { result } = renderHook(() => useResourceSessionInventory(true)) + await waitFor(() => expect(result.current.sessionInventory.count).toBe(1)) + + const older = deferred() + const newer = deferred() + listSessions.mockReturnValueOnce(older.promise).mockReturnValueOnce(newer.promise) + let olderRefresh!: Promise + let newerRefresh!: Promise + act(() => { + olderRefresh = result.current.refreshSessions() + newerRefresh = result.current.refreshSessions() + }) + + await act(async () => { + newer.resolve([session('one'), session('two')]) + await newerRefresh + }) + await act(async () => { + older.resolve([session('one')]) + await olderRefresh + }) + + expect(result.current.sessionInventory.count).toBe(2) + }) + + it('unsubscribes from lifecycle events on unmount', () => { + listSessions.mockResolvedValue([]) + const { unmount } = renderHook(() => useResourceSessionInventory(true)) + + unmount() + + expect(unsubscribeSpawned).toHaveBeenCalledTimes(1) + expect(unsubscribeExit).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/components/status-bar/use-resource-session-inventory.ts b/src/renderer/src/components/status-bar/use-resource-session-inventory.ts new file mode 100644 index 00000000000..dc0b5febc1f --- /dev/null +++ b/src/renderer/src/components/status-bar/use-resource-session-inventory.ts @@ -0,0 +1,196 @@ +import { useCallback, useEffect, useRef, useState } from 'react' +import { useMountedRef } from '@/hooks/useMountedRef' +import { + EMPTY_DAEMON_SESSION_INVENTORY, + inventoryFromSessions, + removeSessionFromInventory, + removeSessionsFromInventory, + type DaemonSessionInventory +} from './resource-session-inventory' + +type ResourceSessionInventory = { + sessionInventory: DaemonSessionInventory + sessionsError: boolean + refreshSessions: () => Promise + clearSessionsError: () => void + removeSession: (sessionId: string) => void + removeSessions: (sessionIds: ReadonlySet) => void +} + +type ResourceSessionInventoryState = { + ready: boolean + sessionInventory: DaemonSessionInventory + sessionsError: boolean +} + +export function useResourceSessionInventory(ready: boolean): ResourceSessionInventory { + const mountedRef = useMountedRef() + const refreshGenerationRef = useRef(0) + const lifecycleRevisionRef = useRef(0) + const removedAtRevisionRef = useRef(new Map()) + const knownSessionIdsRef = useRef(new Set()) + const [storedState, setStoredState] = useState(() => ({ + ready, + sessionInventory: EMPTY_DAEMON_SESSION_INVENTORY, + sessionsError: false + })) + const state = + storedState.ready === ready + ? storedState + : { + ready, + sessionInventory: EMPTY_DAEMON_SESSION_INVENTORY, + sessionsError: false + } + if (state !== storedState) { + // Why: readiness changes define a new inventory epoch. Reset during render + // so an old workspace count is never exposed for one committed frame. + setStoredState(state) + } + + const refreshSessions = useCallback(async (): Promise => { + if (!ready) { + return + } + const generation = ++refreshGenerationRef.current + const lifecycleRevision = lifecycleRevisionRef.current + try { + const sessions = await window.api.pty.listSessions() + // Why: an exit or newer refresh can land while the global provider list + // is in flight; stale results must not resurrect dead sessions. + if (!mountedRef.current || generation !== refreshGenerationRef.current) { + return + } + const currentRemovedAtRevision = removedAtRevisionRef.current + const liveSessions = sessions.filter( + ({ id }) => (currentRemovedAtRevision.get(id) ?? 0) <= lifecycleRevision + ) + // Tombstones at or before this request cannot suppress later ID reuse; + // only exits that raced this request must survive to the next refresh. + for (const [id, removedAtRevision] of currentRemovedAtRevision) { + if (removedAtRevision <= lifecycleRevision) { + currentRemovedAtRevision.delete(id) + } + } + knownSessionIdsRef.current = new Set(liveSessions.map(({ id }) => id)) + setStoredState({ + ready: true, + sessionInventory: inventoryFromSessions(liveSessions), + sessionsError: false + }) + } catch { + if (mountedRef.current && generation === refreshGenerationRef.current) { + setStoredState((current) => ({ ...current, sessionsError: true })) + } + } + }, [mountedRef, ready]) + + const clearSessionsError = useCallback((): void => { + setStoredState((current) => ({ ...current, sessionsError: false })) + }, []) + + const removeSession = useCallback((sessionId: string): void => { + // Why: mark the exact PTY removed while a list may be in flight; filtering + // only that id preserves unrelated sessions discovered by the same list. + const lifecycleRevision = ++lifecycleRevisionRef.current + removedAtRevisionRef.current.set(sessionId, lifecycleRevision) + knownSessionIdsRef.current.delete(sessionId) + setStoredState((current) => ({ + ...current, + sessionInventory: removeSessionFromInventory(current.sessionInventory, sessionId) + })) + }, []) + + const removeSessions = useCallback((sessionIds: ReadonlySet): void => { + const lifecycleRevision = ++lifecycleRevisionRef.current + for (const sessionId of sessionIds) { + removedAtRevisionRef.current.set(sessionId, lifecycleRevision) + knownSessionIdsRef.current.delete(sessionId) + } + setStoredState((current) => ({ + ...current, + sessionInventory: removeSessionsFromInventory(current.sessionInventory, sessionIds) + })) + }, []) + + useEffect(() => { + refreshGenerationRef.current += 1 + if (!ready) { + removedAtRevisionRef.current.clear() + knownSessionIdsRef.current.clear() + return + } + void refreshSessions() + }, [ready, refreshSessions]) + + useEffect(() => { + if (!ready) { + return + } + let disposed = false + let refreshTimer: number | null = null + let lifecycleRefresh: Promise | null = null + const pendingSpawnIds = new Set() + const scheduleLifecycleRefresh = (): void => { + if (disposed || refreshTimer !== null || lifecycleRefresh !== null) { + return + } + refreshTimer = window.setTimeout(() => { + refreshTimer = null + if (pendingSpawnIds.size === 0) { + return + } + pendingSpawnIds.clear() + const refresh = refreshSessions() + lifecycleRefresh = refresh + void refresh.finally(() => { + if (disposed || lifecycleRefresh !== refresh) { + return + } + lifecycleRefresh = null + for (const id of pendingSpawnIds) { + if (knownSessionIdsRef.current.has(id)) { + pendingSpawnIds.delete(id) + } + } + scheduleLifecycleRefresh() + }) + }, 0) + } + const unsubscribeSpawned = window.api.pty.onSpawned(({ id }) => { + // Why: reattach emits the same lifecycle signal; known IDs must not turn remounts into global inventory scans. + if (knownSessionIdsRef.current.has(id)) { + return + } + pendingSpawnIds.add(id) + // Why: serialize slow provider-wide lists; retry once only when a result missed a later spawn. + scheduleLifecycleRefresh() + }) + const unsubscribeExit = window.api.pty.onExit(({ id }) => { + pendingSpawnIds.delete(id) + if (pendingSpawnIds.size === 0 && refreshTimer !== null) { + window.clearTimeout(refreshTimer) + refreshTimer = null + } + removeSession(id) + }) + return () => { + disposed = true + pendingSpawnIds.clear() + if (refreshTimer !== null) { + window.clearTimeout(refreshTimer) + } + unsubscribeSpawned() + unsubscribeExit() + } + }, [ready, refreshSessions, removeSession]) + + return { + sessionInventory: state.sessionInventory, + sessionsError: state.sessionsError, + refreshSessions, + clearSessionsError, + removeSession, + removeSessions + } +} diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index 97009ce2890..122535e751e 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -3012,6 +3012,7 @@ function createPtyApi(): NonNullable['pty']> { onReplay: () => noopUnsubscribe, onModelRestoreNeeded: () => noopUnsubscribe, onExit: () => noopUnsubscribe, + onSpawned: () => noopUnsubscribe, onSerializeBufferRequest: () => noopUnsubscribe, onClearBufferRequest: () => noopUnsubscribe, sendSerializedBuffer: () => {}, From ef985ed8002c1e8cf9a7ce4e18727e5d270722cd Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:08:46 -0700 Subject: [PATCH 08/10] fix(codex): re-land TUI settings promotion with anchored baseline (#10213) * Reapply "Preserve Codex [tui] settings across managed CODEX_HOME remirrors (#9475)" (#10085) This reverts commit 09756dfaff0d652dfe7a3c9cc737d1a02c89312b. * fix(codex): anchor settings baseline migration --- config/tsconfig.cli.json | 6 + src/main/codex/codex-config-mirror.ts | 37 +- .../codex-config-settings-preservation.ts | 22 + .../codex-config-settings-removal.test.ts | 41 ++ .../codex/codex-config-settings-removal.ts | 73 ++++ .../codex/codex-config-settings-upsert.ts | 322 +++++++++++++++ .../config-settings-baseline-upgrade.test.ts | 263 ++++++++++++ src/main/codex/config-settings-baseline.ts | 89 ++++ .../config-settings-conflict-resolution.ts | 35 ++ .../codex/config-settings-promotion.test.ts | 386 +++++++++++++++++- src/main/codex/config-settings-promotion.ts | 333 ++++++++------- src/main/codex/config-toml-key-path.ts | 67 +++ 12 files changed, 1518 insertions(+), 156 deletions(-) create mode 100644 src/main/codex/codex-config-settings-preservation.ts create mode 100644 src/main/codex/codex-config-settings-removal.test.ts create mode 100644 src/main/codex/codex-config-settings-removal.ts create mode 100644 src/main/codex/codex-config-settings-upsert.ts create mode 100644 src/main/codex/config-settings-baseline-upgrade.test.ts create mode 100644 src/main/codex/config-settings-baseline.ts create mode 100644 src/main/codex/config-settings-conflict-resolution.ts create mode 100644 src/main/codex/config-toml-key-path.ts diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 53859a16a32..6b31708de90 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -27,6 +27,9 @@ "../src/main/codex/codex-app-server-session.ts", "../src/main/codex/codex-config-mirror.ts", "../src/main/codex/codex-config-path-reference-rewrite.ts", + "../src/main/codex/codex-config-settings-preservation.ts", + "../src/main/codex/codex-config-settings-removal.ts", + "../src/main/codex/codex-config-settings-upsert.ts", "../src/main/codex/codex-home-paths.ts", "../src/main/codex/codex-host-retry-deadlines.ts", "../src/main/codex/codex-hook-identity.ts", @@ -41,7 +44,10 @@ "../src/main/codex/codex-user-hook-trust-rebase.ts", "../src/main/codex/codex-wsl-hook-install-plan.ts", "../src/main/codex/codex-wsl-reconciliation-generations.ts", + "../src/main/codex/config-settings-baseline.ts", + "../src/main/codex/config-settings-conflict-resolution.ts", "../src/main/codex/config-settings-promotion.ts", + "../src/main/codex/config-toml-key-path.ts", "../src/main/codex/config-toml-line-scan.ts", "../src/main/codex/config-toml-trust.ts", "../src/main/codex/hook-service.ts", diff --git a/src/main/codex/codex-config-mirror.ts b/src/main/codex/codex-config-mirror.ts index 5869d362397..150ef31840d 100644 --- a/src/main/codex/codex-config-mirror.ts +++ b/src/main/codex/codex-config-mirror.ts @@ -8,8 +8,10 @@ import { parseWslUncPath } from '../../shared/wsl-paths' import { promoteCodexRuntimeSettingsToSystem, snapshotCodexRuntimeSettingsBaseline, - type CodexSettingsPromotionHomes + type CodexSettingsPromotionHomes, + type CodexSettingsPromotionPlan } from './config-settings-promotion' +import { preserveRuntimeConflictValues } from './codex-config-settings-preservation' import { createTomlLineScanState, getTomlTableHeader, @@ -31,32 +33,37 @@ export function syncSystemConfigIntoManagedCodexHome( // Why: the mirror overwrites runtime settings from ~/.codex, so changes the // user made inside Orca-launched Codex (/model, /approvals) must be written // back to ~/.codex first or this very pass silently reverts them. - if (!promoteCodexRuntimeSettingsToSystem(homes)) { + const promotionPlan = promoteCodexRuntimeSettingsToSystem(homes) + if (!promotionPlan) { // Why: mirroring after a failed write-back would erase the runtime change; // leave both runtime and its old baseline intact so the next launch retries. return } + let preservedConflictKeys: ReadonlySet try { - syncSystemConfigIntoManagedCodexHomeUnsafe(homes) + preservedConflictKeys = syncSystemConfigIntoManagedCodexHomeUnsafe(homes, promotionPlan) } catch (error) { console.warn('[codex-config] Failed to mirror system Codex config:', error) return } // Why: the baseline advances only after a successful mirror; recording an // unpromoted runtime change as Orca-written would strand it forever. - snapshotCodexRuntimeSettingsBaseline(homes.runtimeHomePath) + snapshotCodexRuntimeSettingsBaseline( + homes.runtimeHomePath, + new Map([...promotionPlan.conflicts].filter(([key]) => preservedConflictKeys.has(key))) + ) } -function syncSystemConfigIntoManagedCodexHomeUnsafe({ - runtimeHomePath, - systemHomePath -}: CodexSettingsPromotionHomes): void { +function syncSystemConfigIntoManagedCodexHomeUnsafe( + { runtimeHomePath, systemHomePath }: CodexSettingsPromotionHomes, + promotionPlan: CodexSettingsPromotionPlan +): ReadonlySet { const systemConfigPath = join(systemHomePath, 'config.toml') const runtimeConfigPath = join(runtimeHomePath, 'config.toml') const systemConfigExists = existsSync(systemConfigPath) const runtimeConfigExists = existsSync(runtimeConfigPath) if (!systemConfigExists && !runtimeConfigExists) { - return + return new Set() } const rawSystemConfig = systemConfigExists ? readAgentStateFileSync(systemConfigPath) : '' @@ -66,15 +73,19 @@ function syncSystemConfigIntoManagedCodexHomeUnsafe({ runtimeConfigPath, prepareSystemConfigForFreshRuntimeMirror(rawSystemConfig, sourceConfigDir) ) - return + return new Set() } const systemConfig = prepareSystemConfigForRuntimeMirror(rawSystemConfig, sourceConfigDir) const runtimeConfig = readAgentStateFileSync(runtimeConfigPath) - const mergedConfig = mergeSystemCodexConfigIntoRuntime(runtimeConfig, systemConfig) - if (mergedConfig !== runtimeConfig) { - writeFileAtomically(runtimeConfigPath, mergedConfig) + const preserved = preserveRuntimeConflictValues( + mergeSystemCodexConfigIntoRuntime(runtimeConfig, systemConfig), + promotionPlan.runtimeValuesToPreserve + ) + if (preserved.content !== runtimeConfig) { + writeFileAtomically(runtimeConfigPath, preserved.content) } + return preserved.keys } export function resolveCodexConfigMirrorSourceDirectory(systemHomePath: string): string { diff --git a/src/main/codex/codex-config-settings-preservation.ts b/src/main/codex/codex-config-settings-preservation.ts new file mode 100644 index 00000000000..38bd81e0295 --- /dev/null +++ b/src/main/codex/codex-config-settings-preservation.ts @@ -0,0 +1,22 @@ +import { removePromotedSettingsFromContent } from './codex-config-settings-removal' +import { upsertPromotedSettingsInContent } from './codex-config-settings-upsert' + +export function preserveRuntimeConflictValues( + content: string, + values: ReadonlyMap +): { content: string; keys: ReadonlySet } { + let result = content + const keys = new Set() + for (const [key, raw] of values) { + const previous = result + result = + raw === null + ? removePromotedSettingsFromContent(result, new Set([key])) + : upsertPromotedSettingsInContent(result, new Map([[key, raw]])) + if (result !== previous) { + keys.add(key) + } + } + // Why: only schema-new ambiguous keys stay runtime-local; every unrelated setting still mirrors. + return { content: result, keys } +} diff --git a/src/main/codex/codex-config-settings-removal.test.ts b/src/main/codex/codex-config-settings-removal.test.ts new file mode 100644 index 00000000000..1009886eb1c --- /dev/null +++ b/src/main/codex/codex-config-settings-removal.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from 'vitest' +import { removePromotedSettingsFromContent } from './codex-config-settings-removal' + +describe('removePromotedSettingsFromContent', () => { + it('removes a top-level preamble key without touching nested copies', () => { + expect( + removePromotedSettingsFromContent( + 'model = "root"\n\n[profiles.dev]\nmodel = "nested"\n', + new Set(['model']) + ) + ).toBe('\n[profiles.dev]\nmodel = "nested"\n') + }) + + it('removes a bare key from the first tui table body', () => { + expect( + removePromotedSettingsFromContent( + '[tui]\ntheme = "dark"\nanimations = true\n\n[tui.notifications]\ntheme = "nested"\n', + new Set(['tui.theme']) + ) + ).toBe('[tui]\nanimations = true\n\n[tui.notifications]\ntheme = "nested"\n') + }) + + it('removes dotted and quoted dotted tui keys from the preamble', () => { + expect( + removePromotedSettingsFromContent( + 'tui.theme = "dark"\n"tui" . "status_line" = ["model"]\n', + new Set(['tui.theme', 'tui.status_line']) + ) + ).toBe('') + }) + + it('does not remove a tui-shaped key inside another table', () => { + const content = '[[profiles]]\ntui.theme = "profile-theme"\n' + expect(removePromotedSettingsFromContent(content, new Set(['tui.theme']))).toBe(content) + }) + + it('does not remove a nested tui descendant with the same first segment', () => { + const content = '[tui]\ntheme.variant = "dark"\n' + expect(removePromotedSettingsFromContent(content, new Set(['tui.theme']))).toBe(content) + }) +}) diff --git a/src/main/codex/codex-config-settings-removal.ts b/src/main/codex/codex-config-settings-removal.ts new file mode 100644 index 00000000000..7210d921ebe --- /dev/null +++ b/src/main/codex/codex-config-settings-removal.ts @@ -0,0 +1,73 @@ +import { + createTomlLineScanState, + getTomlTableHeader, + isTomlStructuralLine, + updateTomlLineScanState +} from './config-toml-line-scan' +import { parseTomlKeyPath, parseTomlTableHeaderPath } from './config-toml-key-path' +import { tuiStructuredKey } from './codex-config-settings-upsert' + +export function removePromotedSettingsFromContent( + content: string, + removals: ReadonlySet +): string { + if (removals.size === 0) { + return content + } + const lines = content.split('\n') + const indexes: number[] = [] + let state = createTomlLineScanState() + let inPreamble = true + let tuiTableSeen = false + let tuiBodyActive = false + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? '' + if (isTomlStructuralLine(state)) { + const header = getTomlTableHeader(line) + if (header) { + const table = parseTomlTableHeaderPath(header) + tuiBodyActive = + table !== null && + !table.isArray && + table.segments.length === 1 && + table.segments[0] === 'tui' && + !tuiTableSeen + tuiTableSeen ||= tuiBodyActive + inPreamble = false + state = updateTomlLineScanState(state, line) + continue + } + const parsed = parseTomlKeyPath(line) + if (parsed && line[parsed.end] === '=') { + const structuredKey = getStructuredKey(parsed.segments, inPreamble, tuiBodyActive) + if (structuredKey && removals.has(structuredKey)) { + indexes.push(index) + } + } + } + state = updateTomlLineScanState(state, line) + } + + for (const index of indexes.toReversed()) { + lines.splice(index, 1) + } + return lines.join('\n') +} + +function getStructuredKey( + segments: string[], + inPreamble: boolean, + tuiBodyActive: boolean +): string | null { + if (inPreamble && segments.length === 1) { + return segments[0] ?? null + } + if (inPreamble && segments.length === 2 && segments[0] === 'tui') { + return segments[1] ? tuiStructuredKey(segments[1]) : null + } + if (tuiBodyActive && segments.length === 1) { + return segments[0] ? tuiStructuredKey(segments[0]) : null + } + return null +} diff --git a/src/main/codex/codex-config-settings-upsert.ts b/src/main/codex/codex-config-settings-upsert.ts new file mode 100644 index 00000000000..963d46d239a --- /dev/null +++ b/src/main/codex/codex-config-settings-upsert.ts @@ -0,0 +1,322 @@ +import { + createTomlLineScanState, + getTomlTableHeader, + isTomlStructuralLine, + updateTomlLineScanState +} from './config-toml-line-scan' +import { parseTomlKeyPath, parseTomlTableHeaderPath } from './config-toml-key-path' + +const TUI_STRUCTURED_PREFIX = 'tui.' + +// Why: promoted [tui] settings are keyed by structured path (tui.) so their +// baseline/update entries can never collide with a top-level key of the same name. +export function tuiStructuredKey(key: string): string { + return `${TUI_STRUCTURED_PREFIX}${key}` +} + +export function isTuiStructuredKey(structuredKey: string): boolean { + return structuredKey.startsWith(TUI_STRUCTURED_PREFIX) +} + +export function tuiKeyFromStructuredKey(structuredKey: string): string { + return structuredKey.slice(TUI_STRUCTURED_PREFIX.length) +} + +// Why: promoted updates arrive keyed by structured path; the preamble and [tui] +// regions are disjoint, so a mixed batch (e.g. /model + a status-line change) +// composes in one rewrite — top-level keys land in the preamble, tui. +// entries wherever the [tui] placement rule puts them. +export function upsertPromotedSettingsInContent( + content: string, + updates: Map +): string { + const topLevelUpdates = new Map() + const tuiUpdates = new Map() + for (const [key, raw] of updates) { + if (isTuiStructuredKey(key)) { + tuiUpdates.set(tuiKeyFromStructuredKey(key), raw) + } else { + topLevelUpdates.set(key, raw) + } + } + let result = content + if (topLevelUpdates.size > 0) { + result = upsertTopLevelSettingsInContent(result, topLevelUpdates) + } + if (tuiUpdates.size > 0) { + result = upsertTuiSettingsInContent(result, tuiUpdates) + } + return result +} + +export function upsertTopLevelSettingsInContent( + content: string, + updates: Map +): string { + const lines = content.split('\n') + let state = createTomlLineScanState() + let preambleEnd = lines.length + const keyLineIndexes = new Map() + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? '' + if (isTomlStructuralLine(state)) { + if (getTomlTableHeader(line)) { + preambleEnd = index + break + } + const parsed = parseTomlKeyPath(line) + const key = parsed?.segments.length === 1 ? parsed.segments[0] : null + if (parsed && line[parsed.end] === '=' && key && updates.has(key)) { + keyLineIndexes.set(key, index) + } + } + state = updateTomlLineScanState(state, line) + } + + // Why: CRLF configs keep a trailing \r after the split; new lines must use + // the file's existing endings or a Windows-owned config becomes mixed-EOL. + const usesCrlf = content.includes('\r\n') + const insertions: string[] = [] + for (const [key, raw] of updates) { + const existingIndex = keyLineIndexes.get(key) + const rendered = `${key} = ${raw}` + if (existingIndex !== undefined) { + lines[existingIndex] = lines[existingIndex]?.endsWith('\r') ? `${rendered}\r` : rendered + } else { + insertions.push(usesCrlf ? `${rendered}\r` : rendered) + } + } + if (insertions.length > 0) { + let insertAt = preambleEnd + while (insertAt > 0 && (lines[insertAt - 1] ?? '').trim() === '') { + insertAt -= 1 + } + if (insertAt === preambleEnd && preambleEnd < lines.length) { + insertions.push(usesCrlf ? '\r' : '') + } + lines.splice(insertAt, 0, ...insertions) + } + return joinPreservingTrailingNewline(lines, usesCrlf) +} + +type TuiPlacementScan = { + bareKeyIndexes: Map + dottedKeyIndexes: Map + hasBareTuiTable: boolean + hasDottedTuiKey: boolean + blocksNewTuiTable: boolean + blockedAbsentKeys: Set + bareBodyInsertIndex: number + lastDottedTuiIndex: number +} + +/** + * Upserts promoted `[tui]` keys (keyed by bare name) into the system config, + * placing each per the design's total placement rule: replace an existing key + * in place keeping its form; else insert bare into the first `[tui]` body; else + * dotted in the preamble beside existing dotted `tui.*` keys; else create one + * `[tui]` table at EOF for every key that reaches that branch. Rendering follows + * the destination — bare inside a table, dotted in the preamble — so no `tui` + * table is ever defined twice. + */ +export function upsertTuiSettingsInContent(content: string, updates: Map): string { + const lines = content.split('\n') + const scan = scanTuiPlacement(lines, updates) + const usesCrlf = content.includes('\r\n') + const bareBodyInserts: string[] = [] + const dottedPreambleInserts: string[] = [] + const newTableKeys: string[] = [] + + for (const [key, raw] of updates) { + const dottedIndex = scan.dottedKeyIndexes.get(key) + if (dottedIndex !== undefined) { + lines[dottedIndex] = withTrailingCr(lines[dottedIndex]!, `${tuiStructuredKey(key)} = ${raw}`) + continue + } + const bareIndex = scan.bareKeyIndexes.get(key) + if (bareIndex !== undefined) { + lines[bareIndex] = withTrailingCr(lines[bareIndex]!, `${key} = ${raw}`) + continue + } + // Why: adding a scalar beside an existing tui. descendant would turn valid TOML invalid. + if (scan.blockedAbsentKeys.has(key)) { + continue + } + if (scan.hasBareTuiTable) { + bareBodyInserts.push(`${key} = ${raw}`) + } else if (scan.hasDottedTuiKey) { + dottedPreambleInserts.push(`${tuiStructuredKey(key)} = ${raw}`) + } else if (!scan.blocksNewTuiTable) { + // Why: inline/array tui definitions block this branch because adding a + // plain [tui] beside either would make the config invalid. + newTableKeys.push(`${key} = ${raw}`) + } + } + + // Why: the config shape routes every absent key to the same branch, so at most + // one insert group is non-empty; still apply EOF→body→preamble so a splice + // never shifts a lower index a later splice depends on. + if (newTableKeys.length > 0) { + appendNewTuiTable(lines, newTableKeys, usesCrlf) + } + if (bareBodyInserts.length > 0) { + lines.splice( + scan.bareBodyInsertIndex, + 0, + ...bareBodyInserts.map((line) => withCrLine(line, usesCrlf)) + ) + } + if (dottedPreambleInserts.length > 0) { + lines.splice( + scan.lastDottedTuiIndex + 1, + 0, + ...dottedPreambleInserts.map((line) => withCrLine(line, usesCrlf)) + ) + } + return joinPreservingTrailingNewline(lines, usesCrlf) +} + +function scanTuiPlacement(lines: string[], updates: Map): TuiPlacementScan { + let state = createTomlLineScanState() + let inPreamble = true + let tuiTableSeen = false + let tuiBodyActive = false + let tuiBodyHeaderIndex = -1 + let tuiBodyEndIndex = -1 + let hasDottedTuiKey = false + let blocksNewTuiTable = false + let lastDottedTuiIndex = -1 + const bareKeyIndexes = new Map() + const dottedKeyIndexes = new Map() + const blockedAbsentKeys = new Set() + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? '' + if (isTomlStructuralLine(state)) { + const header = getTomlTableHeader(line) + if (header) { + if (tuiBodyActive) { + tuiBodyEndIndex = index + tuiBodyActive = false + } + const table = parseTomlTableHeaderPath(header) + if ( + table && + !table.isArray && + table.segments.length === 1 && + table.segments[0] === 'tui' && + !tuiTableSeen + ) { + tuiTableSeen = true + tuiBodyActive = true + tuiBodyHeaderIndex = index + } + // Why: a root [[tui]] is already an array, so appending [tui] would + // redefine it and make an otherwise valid config unparseable. + if (table?.isArray && table.segments.length === 1 && table.segments[0] === 'tui') { + blocksNewTuiTable = true + } + const descendantKey = + table?.segments[0] === 'tui' && table.segments.length > 1 ? table.segments[1] : null + if (descendantKey && updates.has(descendantKey)) { + blockedAbsentKeys.add(descendantKey) + } + inPreamble = false + state = updateTomlLineScanState(state, line) + continue + } + if (inPreamble) { + // Why: any dotted `tui.*` key (allowlisted or not) already defines the + // implicit tui table, so a new `[tui]` table at EOF would duplicate it. + const parsed = parseTomlKeyPath(line) + const isAssignment = parsed && line[parsed.end] === '=' + if (isAssignment && parsed.segments[0] === 'tui' && parsed.segments.length > 1) { + hasDottedTuiKey = true + lastDottedTuiIndex = index + const promotedKey = parsed.segments.length === 2 ? parsed.segments[1] : null + if (promotedKey && updates.has(promotedKey)) { + dottedKeyIndexes.set(promotedKey, index) + } + const descendantKey = parsed.segments.length > 2 ? parsed.segments[1] : null + if (descendantKey && updates.has(descendantKey)) { + blockedAbsentKeys.add(descendantKey) + } + } else if (isAssignment && parsed.segments.length === 1 && parsed.segments[0] === 'tui') { + blocksNewTuiTable = true + } + } else if (tuiBodyActive) { + const parsed = parseTomlKeyPath(line) + const key = parsed?.segments.length === 1 ? parsed.segments[0] : null + if (parsed && line[parsed.end] === '=' && key && updates.has(key)) { + bareKeyIndexes.set(key, index) + } + const descendantKey = parsed && parsed.segments.length > 1 ? parsed.segments[0] : null + if (descendantKey && updates.has(descendantKey)) { + blockedAbsentKeys.add(descendantKey) + } + } + } + state = updateTomlLineScanState(state, line) + } + if (tuiBodyActive) { + tuiBodyEndIndex = lines.length + } + + return { + bareKeyIndexes, + dottedKeyIndexes, + hasBareTuiTable: tuiTableSeen, + hasDottedTuiKey, + blocksNewTuiTable, + blockedAbsentKeys, + bareBodyInsertIndex: computeBareBodyInsertIndex(lines, tuiBodyHeaderIndex, tuiBodyEndIndex), + lastDottedTuiIndex + } +} + +// Why: TOML forbids adding bare keys to `[tui]` after a `[tui.*]` subtable opens, +// so absent keys land at the body's end — before trailing blanks and before the +// next header — which is the only valid spot. +function computeBareBodyInsertIndex( + lines: string[], + headerIndex: number, + endIndex: number +): number { + if (headerIndex === -1) { + return -1 + } + let insertAt = endIndex + while (insertAt > headerIndex + 1 && (lines[insertAt - 1] ?? '').trim() === '') { + insertAt -= 1 + } + return insertAt +} + +function appendNewTuiTable(lines: string[], keyRenders: string[], usesCrlf: boolean): void { + let appendAt = lines.length + while (appendAt > 0 && (lines[appendAt - 1] ?? '').trim() === '') { + appendAt -= 1 + } + // Why: separate the new table from prior content with a blank line, unless the + // file was empty/blank, where a leading blank would be spurious. + const block = appendAt > 0 ? ['', '[tui]', ...keyRenders] : ['[tui]', ...keyRenders] + lines.splice(appendAt, 0, ...block.map((line) => withCrLine(line, usesCrlf))) +} + +function withTrailingCr(originalLine: string, rendered: string): string { + return originalLine.endsWith('\r') ? `${rendered}\r` : rendered +} + +function withCrLine(rendered: string, usesCrlf: boolean): string { + return usesCrlf ? `${rendered}\r` : rendered +} + +// Why: a missing trailing newline is restored in the file's own EOL so a +// preamble-only or table-appended rewrite matches the source's newline behavior. +function joinPreservingTrailingNewline(lines: string[], usesCrlf: boolean): string { + const result = lines.join('\n') + if (result.endsWith('\n') || result.length === 0) { + return result + } + return result.endsWith('\r') ? `${result}\n` : `${result}${usesCrlf ? '\r\n' : '\n'}` +} diff --git a/src/main/codex/config-settings-baseline-upgrade.test.ts b/src/main/codex/config-settings-baseline-upgrade.test.ts new file mode 100644 index 00000000000..443f5f429ec --- /dev/null +++ b/src/main/codex/config-settings-baseline-upgrade.test.ts @@ -0,0 +1,263 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { homedir, tmpdir } from 'node:os' +import type * as Os from 'node:os' +import { join } from 'node:path' + +const { homedirMock } = vi.hoisted(() => ({ + homedirMock: vi.fn<() => string>() +})) + +vi.mock('node:os', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, homedir: homedirMock } +}) + +import { syncSystemConfigIntoManagedCodexHome } from './codex-config-mirror' + +let tmpHome: string +let userDataDir: string +let previousUserDataPath: string | undefined + +beforeEach(() => { + tmpHome = mkdtempSync(join(tmpdir(), 'orca-codex-settings-upgrade-home-')) + userDataDir = mkdtempSync(join(tmpdir(), 'orca-codex-settings-upgrade-data-')) + previousUserDataPath = process.env.ORCA_USER_DATA_PATH + process.env.ORCA_USER_DATA_PATH = userDataDir + homedirMock.mockReturnValue(tmpHome) + if (homedir() !== tmpHome) { + throw new Error('node:os homedir mock is not active; refusing to touch the real ~/.codex') + } +}) + +afterEach(() => { + rmSync(tmpHome, { recursive: true, force: true }) + rmSync(userDataDir, { recursive: true, force: true }) + if (previousUserDataPath === undefined) { + delete process.env.ORCA_USER_DATA_PATH + } else { + process.env.ORCA_USER_DATA_PATH = previousUserDataPath + } + vi.clearAllMocks() +}) + +function systemConfigPath(): string { + return join(tmpHome, '.codex', 'config.toml') +} + +function runtimeHomePath(): string { + return join(userDataDir, 'codex-runtime-home', 'home') +} + +function runtimeConfigPath(): string { + return join(runtimeHomePath(), 'config.toml') +} + +function baselinePath(): string { + return join(runtimeHomePath(), '.orca-config-settings-baseline.json') +} + +function prepareLegacyState(systemConfig: string, runtimeConfig: string): void { + mkdirSync(join(tmpHome, '.codex'), { recursive: true }) + mkdirSync(runtimeHomePath(), { recursive: true }) + writeFileSync(systemConfigPath(), systemConfig, 'utf-8') + writeFileSync(runtimeConfigPath(), runtimeConfig, 'utf-8') + writeFileSync( + baselinePath(), + `${JSON.stringify({ version: 1, settings: { model: '"gpt-5"' } }, null, 2)}\n`, + 'utf-8' + ) +} + +function readBaseline(): { + version: number + settings: Record + conflicts?: Record +} { + return JSON.parse(readFileSync(baselinePath(), 'utf-8')) +} + +describe('Codex settings baseline schema upgrade', () => { + it('upgrades an aligned legacy baseline without creating a conflict', () => { + const config = 'model = "gpt-5"\n\n[tui]\ntheme = "dark"\n' + prepareLegacyState(config, config) + + syncSystemConfigIntoManagedCodexHome() + + expect(readBaseline()).toMatchObject({ + version: 2, + settings: { model: '"gpt-5"', 'tui.theme': '"dark"' } + }) + expect(readBaseline().conflicts).toBeUndefined() + }) + + it('anchors a schema-new conflict while promoting an unrelated known key', () => { + prepareLegacyState( + 'model = "gpt-5"\n\n[tui]\ntheme = "system"\n', + 'model = "o4"\n\n[tui]\ntheme = "runtime"\n' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('model = "o4"') + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('theme = "system"') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('theme = "runtime"') + expect(readBaseline().conflicts).toEqual({ + 'tui.theme': { runtime: '"runtime"', system: '"system"' } + }) + }) + + it('promotes the runtime side when its anchored value changes', () => { + prepareLegacyState( + 'model = "gpt-5"\n\n[tui]\ntheme = "system"\n', + 'model = "gpt-5"\n\n[tui]\ntheme = "runtime"\n' + ) + syncSystemConfigIntoManagedCodexHome() + + writeFileSync( + runtimeConfigPath(), + readFileSync(runtimeConfigPath(), 'utf-8').replace('theme = "runtime"', 'theme = "chosen"'), + 'utf-8' + ) + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('theme = "chosen"') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('theme = "chosen"') + expect(readBaseline().conflicts).toBeUndefined() + expect(readBaseline().settings['tui.theme']).toBe('"chosen"') + }) + + it('accepts the system side when its anchored value changes', () => { + prepareLegacyState( + 'model = "gpt-5"\n\n[tui]\ntheme = "system"\n', + 'model = "gpt-5"\n\n[tui]\ntheme = "runtime"\n' + ) + syncSystemConfigIntoManagedCodexHome() + + writeFileSync( + systemConfigPath(), + readFileSync(systemConfigPath(), 'utf-8').replace('theme = "system"', 'theme = "outside"'), + 'utf-8' + ) + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('theme = "outside"') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('theme = "outside"') + expect(readBaseline().conflicts).toBeUndefined() + }) + + it('ignores unrelated config writes while a value pair is anchored', () => { + prepareLegacyState( + 'model = "gpt-5"\n\n[tui]\ntheme = "system"\n', + 'model = "gpt-5"\n\n[tui]\ntheme = "runtime"\n' + ) + syncSystemConfigIntoManagedCodexHome() + + writeFileSync( + runtimeConfigPath(), + `${readFileSync(runtimeConfigPath(), 'utf-8')}\n[projects."/tmp/repo"]\ntrust_level = "trusted"\n`, + 'utf-8' + ) + writeFileSync( + systemConfigPath(), + `${readFileSync(systemConfigPath(), 'utf-8')}\n[features]\nhooks = true\n`, + 'utf-8' + ) + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('theme = "system"') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('theme = "runtime"') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('[projects."/tmp/repo"]') + expect(readBaseline().conflicts).toEqual({ + 'tui.theme': { runtime: '"runtime"', system: '"system"' } + }) + }) + + it('re-anchors two new divergent values until one side changes again', () => { + prepareLegacyState( + 'model = "gpt-5"\n\n[tui]\ntheme = "system"\n', + 'model = "gpt-5"\n\n[tui]\ntheme = "runtime"\n' + ) + syncSystemConfigIntoManagedCodexHome() + + writeFileSync( + runtimeConfigPath(), + readFileSync(runtimeConfigPath(), 'utf-8').replace( + 'theme = "runtime"', + 'theme = "runtime-2"' + ), + 'utf-8' + ) + writeFileSync( + systemConfigPath(), + readFileSync(systemConfigPath(), 'utf-8').replace('theme = "system"', 'theme = "system-2"'), + 'utf-8' + ) + syncSystemConfigIntoManagedCodexHome() + + expect(readBaseline().conflicts).toEqual({ + 'tui.theme': { runtime: '"runtime-2"', system: '"system-2"' } + }) + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('theme = "runtime-2"') + }) + + it('preserves an absent runtime value until the user chooses one', () => { + prepareLegacyState('model = "gpt-5"\n\n[tui]\ntheme = "system"\n', 'model = "gpt-5"\n') + + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(runtimeConfigPath(), 'utf-8')).not.toContain('theme =') + expect(readBaseline().conflicts).toEqual({ + 'tui.theme': { runtime: null, system: '"system"' } + }) + + writeFileSync(runtimeConfigPath(), 'model = "gpt-5"\n\n[tui]\ntheme = "chosen"\n', 'utf-8') + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('theme = "chosen"') + expect(readBaseline().conflicts).toBeUndefined() + }) + + it('applies the migration rule to future top-level schema additions', () => { + prepareLegacyState( + 'model = "gpt-5"\napproval_policy = "never"\n', + 'model = "gpt-5"\napproval_policy = "on-request"\n' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('approval_policy = "never"') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).toContain('approval_policy = "on-request"') + expect(readBaseline().conflicts).toEqual({ + approval_policy: { runtime: '"on-request"', system: '"never"' } + }) + }) + + it('lets an incompatible system TOML shape win instead of stranding a conflict', () => { + prepareLegacyState( + 'model = "gpt-5"\ntui = { animations = false }\n', + 'model = "gpt-5"\n\n[tui]\ntheme = "runtime"\n' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(readFileSync(systemConfigPath(), 'utf-8')).toContain('tui = { animations = false }') + expect(readFileSync(runtimeConfigPath(), 'utf-8')).not.toContain('theme = "runtime"') + expect(readBaseline().conflicts).toBeUndefined() + expect(readBaseline().settings['tui.theme']).toBeNull() + }) + + it('does not require filesystem timestamp mutation during migration', () => { + prepareLegacyState( + 'model = "gpt-5"\n\n[tui]\ntheme = "system"\n', + 'model = "gpt-5"\n\n[tui]\ntheme = "runtime"\n' + ) + const baselineBefore = readFileSync(baselinePath(), 'utf-8') + + syncSystemConfigIntoManagedCodexHome() + + expect(existsSync(baselinePath())).toBe(true) + expect(readFileSync(baselinePath(), 'utf-8')).not.toBe(baselineBefore) + expect(readBaseline().conflicts?.['tui.theme']).toBeDefined() + }) +}) diff --git a/src/main/codex/config-settings-baseline.ts b/src/main/codex/config-settings-baseline.ts new file mode 100644 index 00000000000..6959219c2d2 --- /dev/null +++ b/src/main/codex/config-settings-baseline.ts @@ -0,0 +1,89 @@ +import { existsSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import { readAgentStateFileSync, readAgentStateJsonFileSync } from '../agent-state-file-reader' + +const SETTINGS_BASELINE_FILE = '.orca-config-settings-baseline.json' + +export type CodexSettingsConflict = { + runtime: string | null + system: string | null +} + +export type CodexSettingsBaseline = { + settings: ReadonlyMap + conflicts: ReadonlyMap +} + +type StoredSettingsBaseline = { + version: 1 | 2 + settings: Record + conflicts?: Record +} + +export function readCodexSettingsBaseline(runtimeHomePath: string): CodexSettingsBaseline | null { + const baselinePath = getCodexSettingsBaselinePath(runtimeHomePath) + if (!existsSync(baselinePath)) { + return null + } + try { + const parsed: unknown = readAgentStateJsonFileSync(baselinePath) + if (!isStoredSettingsBaseline(parsed)) { + return null + } + const settings = new Map( + Object.entries(parsed.settings).filter((entry): entry is [string, string | null] => { + return typeof entry[1] === 'string' || entry[1] === null + }) + ) + const conflicts = new Map() + for (const [key, conflict] of Object.entries(parsed.conflicts ?? {})) { + if ( + conflict && + (typeof conflict.runtime === 'string' || conflict.runtime === null) && + (typeof conflict.system === 'string' || conflict.system === null) + ) { + conflicts.set(key, conflict) + } + } + return { settings, conflicts } + } catch { + return null + } +} + +export function writeCodexSettingsBaseline( + runtimeHomePath: string, + baseline: CodexSettingsBaseline +): void { + const file: StoredSettingsBaseline = { + version: 2, + settings: Object.fromEntries(baseline.settings) + } + if (baseline.conflicts.size > 0) { + file.conflicts = Object.fromEntries(baseline.conflicts) + } + const baselinePath = getCodexSettingsBaselinePath(runtimeHomePath) + const serialized = `${JSON.stringify(file, null, 2)}\n` + // Why: launch prep runs repeatedly; byte-identical baselines should not churn disk metadata. + if (existsSync(baselinePath) && readAgentStateFileSync(baselinePath) === serialized) { + return + } + writeFileSync(baselinePath, serialized, { encoding: 'utf-8', mode: 0o600 }) +} + +function getCodexSettingsBaselinePath(runtimeHomePath: string): string { + return join(runtimeHomePath, SETTINGS_BASELINE_FILE) +} + +function isStoredSettingsBaseline(value: unknown): value is StoredSettingsBaseline { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + return false + } + const candidate = value as Partial + return ( + (candidate.version === 1 || candidate.version === 2) && + !!candidate.settings && + typeof candidate.settings === 'object' && + !Array.isArray(candidate.settings) + ) +} diff --git a/src/main/codex/config-settings-conflict-resolution.ts b/src/main/codex/config-settings-conflict-resolution.ts new file mode 100644 index 00000000000..1fc147f4829 --- /dev/null +++ b/src/main/codex/config-settings-conflict-resolution.ts @@ -0,0 +1,35 @@ +import type { CodexSettingsConflict } from './config-settings-baseline' + +export type CodexSettingsConflictResolution = + | { action: 'aligned' } + | { action: 'preserve'; conflict: CodexSettingsConflict } + | { action: 'promote-runtime'; raw: string } + | { action: 'use-system' } + +export function resolveUntrackedCodexSetting( + runtime: string | null, + system: string | null, + existingConflict?: CodexSettingsConflict +): CodexSettingsConflictResolution { + if (runtime === system) { + return { action: 'aligned' } + } + if (!existingConflict) { + return { action: 'preserve', conflict: { runtime, system } } + } + + const runtimeChanged = runtime !== existingConflict.runtime + const systemChanged = system !== existingConflict.system + if (runtimeChanged && !systemChanged) { + // Why: steady-state promotion intentionally does not propagate deletions. + return runtime === null ? { action: 'use-system' } : { action: 'promote-runtime', raw: runtime } + } + if (!runtimeChanged && systemChanged) { + return { action: 'use-system' } + } + if (runtimeChanged && systemChanged) { + // Why: two new divergent values remain ambiguous; re-anchor their content without blocking other keys. + return { action: 'preserve', conflict: { runtime, system } } + } + return { action: 'preserve', conflict: existingConflict } +} diff --git a/src/main/codex/config-settings-promotion.test.ts b/src/main/codex/config-settings-promotion.test.ts index f6ceabc362f..0855a0c8c5d 100644 --- a/src/main/codex/config-settings-promotion.test.ts +++ b/src/main/codex/config-settings-promotion.test.ts @@ -19,7 +19,7 @@ import type * as CodexFsUtils from '../codex-accounts/fs-utils' const { homedirMock, promotionTestState } = vi.hoisted(() => ({ homedirMock: vi.fn<() => string>(), - promotionTestState: { failAtomicWrite: false } + promotionTestState: { failAtomicWrite: false, atomicWritePaths: [] as string[] } })) vi.mock('node:os', async (importOriginal) => { @@ -35,6 +35,7 @@ vi.mock('../codex-accounts/fs-utils', async (importOriginal) => { return { ...actual, writeFileAtomically: (...args: Parameters) => { + promotionTestState.atomicWritePaths.push(args[0]) if (promotionTestState.failAtomicWrite) { throw new Error('injected atomic write failure') } @@ -44,7 +45,15 @@ vi.mock('../codex-accounts/fs-utils', async (importOriginal) => { }) import { syncSystemConfigIntoManagedCodexHome } from './codex-config-mirror' -import { upsertTopLevelSettingsInContent } from './config-settings-promotion' +import { + upsertPromotedSettingsInContent, + upsertTopLevelSettingsInContent +} from './codex-config-settings-upsert' + +// The exact [tui] block codex 0.144.6 writes via config/batchWrite (all four +// promoted keys single-line, theme a string). +const CODEX_TUI_BLOCK = + '[tui]\nstatus_line = ["model-with-reasoning", "task-progress"]\nstatus_line_use_colors = true\nterminal_title = ["model"]\ntheme = "dark-photon"\n' let tmpHome: string let userDataDir: string @@ -57,6 +66,7 @@ beforeEach(() => { process.env.ORCA_USER_DATA_PATH = userDataDir homedirMock.mockReturnValue(tmpHome) promotionTestState.failAtomicWrite = false + promotionTestState.atomicWritePaths.length = 0 // Why: promotion writes into homedir()/.codex — if the mock ever fails to // intercept, these tests would rewrite the developer's real Codex config. if (homedir() !== tmpHome) { @@ -117,6 +127,13 @@ function simulateCodexSettingWrite(key: string, rawValue: string): void { writeFileSync(runtimeConfigPath(), next, 'utf-8') } +// Codex reads then rewrites the whole runtime config; simulate that by writing +// a known runtime config directly (its EOL is normalized by the mirror anyway). +function setRuntimeConfig(content: string): void { + mkdirSync(runtimeHomeDir(), { recursive: true }) + writeFileSync(runtimeConfigPath(), content, 'utf-8') +} + function simulateCodexSettingRemoval(key: string): void { const existing = readFileSync(runtimeConfigPath(), 'utf-8') const linePattern = new RegExp(`^${key}[ \\t]*=.*\\n?`, 'm') @@ -184,7 +201,7 @@ describe('codex settings write-back promotion', () => { simulateCodexSettingWrite('model', '"o4"') syncSystemConfigIntoManagedCodexHome() expect(readSystemConfig()).toBe('model = "gpt-5"\n') - expect(JSON.parse(readFileSync(baselinePath(), 'utf-8'))).toMatchObject({ version: 1 }) + expect(JSON.parse(readFileSync(baselinePath(), 'utf-8'))).toMatchObject({ version: 2 }) simulateCodexSettingWrite('model', '"o4"') syncSystemConfigIntoManagedCodexHome() @@ -402,6 +419,363 @@ describe('codex settings write-back promotion', () => { }) }) +describe('codex [tui] settings write-back promotion', () => { + it('promotes a runtime [tui] block (codex 0.144.6 shape) into ~/.codex and survives the remirror', () => { + writeSystemConfig('model = "gpt-5"\n') + syncSystemConfigIntoManagedCodexHome() + + // The user customizes the status line/theme inside Orca-launched Codex. + writeFileSync(runtimeConfigPath(), `${readRuntimeConfig()}\n${CODEX_TUI_BLOCK}`, 'utf-8') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe(`model = "gpt-5"\n\n${CODEX_TUI_BLOCK}`) + const runtime = readRuntimeConfig() + expect(runtime).toContain('status_line = ["model-with-reasoning", "task-progress"]') + expect(runtime).toContain('status_line_use_colors = true') + expect(runtime).toContain('terminal_title = ["model"]') + expect(runtime).toContain('theme = "dark-photon"') + + const settledSystem = readSystemConfig() + const settledRuntime = readRuntimeConfig() + syncSystemConfigIntoManagedCodexHome() + expect(readSystemConfig()).toBe(settledSystem) + expect(readRuntimeConfig()).toBe(settledRuntime) + }) + + it('replaces a promoted key in an existing [tui] table, leaving non-promoted neighbors untouched', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui]\nanimations = true\ntheme = "dark"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n[tui]\nanimations = true\ntheme = "light"\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe( + 'model = "gpt-5"\n\n[tui]\nanimations = true\ntheme = "light"\n' + ) + }) + + it('promotes a changed status_line array value', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui]\nstatus_line = ["model"]\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig( + 'model = "gpt-5"\n\n[tui]\nstatus_line = ["model-with-reasoning", "task-progress"]\n' + ) + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe( + 'model = "gpt-5"\n\n[tui]\nstatus_line = ["model-with-reasoning", "task-progress"]\n' + ) + }) + + it('promotes a model change and a status-line change in one pass into their regions', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui]\ntheme = "dark-photon"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "o4"\n\n[tui]\ntheme = "dark-photon"\nstatus_line = ["model"]\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe( + 'model = "o4"\n\n[tui]\ntheme = "dark-photon"\nstatus_line = ["model"]\n' + ) + }) + + it('detects and replaces a dotted-form system tui key without creating a [tui] table', () => { + writeSystemConfig('model = "gpt-5"\ntui.theme = "dark"\n') + syncSystemConfigIntoManagedCodexHome() + + // toml_edit preserves the dotted form when codex rewrites the value. + setRuntimeConfig('model = "gpt-5"\ntui.theme = "light"\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe('model = "gpt-5"\ntui.theme = "light"\n') + expect(readSystemConfig()).not.toContain('[tui]') + }) + + it('promotes through a quoted tui table without creating a duplicate table', () => { + writeSystemConfig('model = "gpt-5"\n\n["tui"]\ntheme = "dark"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n["tui"]\ntheme = "light"\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe('model = "gpt-5"\n\n["tui"]\ntheme = "light"\n') + expect(readSystemConfig()).not.toContain('\n[tui]\n') + }) + + it('inserts a second dotted tui key beside an existing dotted-only tui config', () => { + writeSystemConfig('model = "gpt-5"\ntui.theme = "dark"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\ntui.theme = "dark"\ntui.status_line = ["model"]\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe( + 'model = "gpt-5"\ntui.theme = "dark"\ntui.status_line = ["model"]\n' + ) + expect(readSystemConfig()).not.toContain('[tui]') + }) + + it('inserts dotted beside a non-promoted dotted tui key instead of creating a [tui] table', () => { + // Why: any dotted tui.* key already defines the implicit tui table, so a + // fresh [tui] table at EOF would be a duplicate-definition parse error. + writeSystemConfig('model = "gpt-5"\ntui.pet = "cat"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\ntui.pet = "cat"\ntui.theme = "dark-photon"\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe('model = "gpt-5"\ntui.pet = "cat"\ntui.theme = "dark-photon"\n') + expect(readSystemConfig()).not.toContain('[tui]') + }) + + it('creates a [tui] table at EOF when the only tui presence is a subtable', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui.notifications]\nenabled = true\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig( + 'model = "gpt-5"\n\n[tui.notifications]\nenabled = true\n\n[tui]\nstatus_line = ["model"]\n' + ) + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe( + 'model = "gpt-5"\n\n[tui.notifications]\nenabled = true\n\n[tui]\nstatus_line = ["model"]\n' + ) + }) + + it('creates exactly one [tui] table for two keys promoted in one pass', () => { + writeSystemConfig('model = "gpt-5"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n[tui]\ntheme = "dark-photon"\nstatus_line = ["model"]\n') + syncSystemConfigIntoManagedCodexHome() + + const system = readSystemConfig() + expect(system.match(/^\[tui\]$/gm)?.length).toBe(1) + expect(system).toBe( + 'model = "gpt-5"\n\n[tui]\nstatus_line = ["model"]\ntheme = "dark-photon"\n' + ) + }) + + it('lets an outside ~/.codex [tui] edit win over a conflicting in-Codex tui change', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui]\ntheme = "dark"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n[tui]\ntheme = "in-codex"\n') + writeSystemConfig('model = "gpt-5"\n\n[tui]\ntheme = "outside-edit"\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe('model = "gpt-5"\n\n[tui]\ntheme = "outside-edit"\n') + expect(readRuntimeConfig()).toContain('theme = "outside-edit"') + }) + + it('does not promote a [tui] key deletion', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui]\ntheme = "dark"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n[tui]\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toContain('theme = "dark"') + }) + + it('inserts a promoted key into a CRLF system [tui] table preserving CRLF', () => { + writeSystemConfig('model = "gpt-5"\r\n\r\n[tui]\r\ntheme = "dark"\r\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n[tui]\ntheme = "dark"\nstatus_line = ["model"]\n') + syncSystemConfigIntoManagedCodexHome() + + const system = readSystemConfig() + expect(system).toContain('status_line = ["model"]\r\n') + expect(system).toBe( + 'model = "gpt-5"\r\n\r\n[tui]\r\ntheme = "dark"\r\nstatus_line = ["model"]\r\n' + ) + }) + + it('never appends a [tui] table when the system config defines tui inline', () => { + writeSystemConfig('model = "gpt-5"\n') + syncSystemConfigIntoManagedCodexHome() + + // In-Codex tui change racing an outside edit that adds an inline tui table: + // appending [tui] would make the system config unparseable, so the change + // is dropped instead. + setRuntimeConfig('model = "gpt-5"\n\n[tui]\ntheme = "dark-photon"\n') + writeSystemConfig('model = "gpt-5"\ntui = { animations = false }\n') + promotionTestState.atomicWritePaths.length = 0 + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe('model = "gpt-5"\ntui = { animations = false }\n') + expect(promotionTestState.atomicWritePaths).not.toContain(systemConfigPath()) + }) + + it('ignores an allowlisted key nested under a [tui.*] subtable', () => { + writeSystemConfig('model = "gpt-5"\n\n[tui.notifications]\ntheme = "should-not-promote"\n') + syncSystemConfigIntoManagedCodexHome() + + setRuntimeConfig('model = "gpt-5"\n\n[tui.notifications]\ntheme = "changed-in-subtable"\n') + syncSystemConfigIntoManagedCodexHome() + + expect(readSystemConfig()).toBe( + 'model = "gpt-5"\n\n[tui.notifications]\ntheme = "should-not-promote"\n' + ) + }) +}) + +describe('upsertPromotedSettingsInContent', () => { + it('replaces a bare key in place inside the [tui] table', () => { + expect( + upsertPromotedSettingsInContent( + '[tui]\ntheme = "dark"\n', + new Map([['tui.theme', '"light"']]) + ) + ).toBe('[tui]\ntheme = "light"\n') + }) + + it('inserts a bare key at the end of the [tui] body, before a subtable', () => { + expect( + upsertPromotedSettingsInContent( + '[tui]\ntheme = "dark"\n\n[tui.notifications]\nenabled = true\n', + new Map([['tui.status_line', '["model"]']]) + ) + ).toBe( + '[tui]\ntheme = "dark"\nstatus_line = ["model"]\n\n[tui.notifications]\nenabled = true\n' + ) + }) + + it('replaces a dotted preamble tui key in place, keeping the dotted form', () => { + expect( + upsertPromotedSettingsInContent('tui.theme = "dark"\n', new Map([['tui.theme', '"light"']])) + ).toBe('tui.theme = "light"\n') + }) + + it('inserts a dotted tui key beside an existing dotted tui key', () => { + expect( + upsertPromotedSettingsInContent( + 'tui.theme = "dark"\n\n[features]\nx = 1\n', + new Map([['tui.status_line', '["model"]']]) + ) + ).toBe('tui.theme = "dark"\ntui.status_line = ["model"]\n\n[features]\nx = 1\n') + }) + + it('creates a [tui] table from empty content', () => { + expect(upsertPromotedSettingsInContent('', new Map([['tui.theme', '"dark"']]))).toBe( + '[tui]\ntheme = "dark"\n' + ) + }) + + it('drops an absent key instead of appending [tui] beside an inline tui table', () => { + expect( + upsertPromotedSettingsInContent( + 'tui = { animations = false }\n', + new Map([['tui.theme', '"dark"']]) + ) + ).toBe('tui = { animations = false }\n') + }) + + it('drops an absent key beside a quoted inline tui table', () => { + expect( + upsertPromotedSettingsInContent( + '"tui" = { animations = false }\n', + new Map([['tui.theme', '"dark"']]) + ) + ).toBe('"tui" = { animations = false }\n') + }) + + it('inserts beside a quoted dotted tui key instead of appending a table', () => { + expect( + upsertPromotedSettingsInContent('"tui" . "pet" = "cat"\n', new Map([['tui.theme', '"dark"']])) + ).toBe('"tui" . "pet" = "cat"\ntui.theme = "dark"\n') + }) + + it('creates a [tui] super-table at EOF after a [tui.*] subtable', () => { + expect( + upsertPromotedSettingsInContent( + '[tui.notifications]\nenabled = true\n', + new Map([['tui.theme', '"dark"']]) + ) + ).toBe('[tui.notifications]\nenabled = true\n\n[tui]\ntheme = "dark"\n') + }) + + it('drops an absent scalar that would redefine an existing tui key table', () => { + expect( + upsertPromotedSettingsInContent( + '[tui."theme"]\nvariant = "dark"\n', + new Map([['tui.theme', '"light"']]) + ) + ).toBe('[tui."theme"]\nvariant = "dark"\n') + }) + + it('drops an absent scalar that would redefine a dotted tui key table', () => { + expect( + upsertPromotedSettingsInContent( + 'tui.theme.variant = "dark"\n', + new Map([['tui.theme', '"light"']]) + ) + ).toBe('tui.theme.variant = "dark"\n') + }) + + it('does not mistake a dotted tui key inside an array table for a root key', () => { + expect( + upsertPromotedSettingsInContent( + '[[profiles]]\ntui.theme = "profile-theme"\n', + new Map([['tui.theme', '"root-theme"']]) + ) + ).toBe('[[profiles]]\ntui.theme = "profile-theme"\n\n[tui]\ntheme = "root-theme"\n') + }) + + it('does not append a table beside a root tui array-of-tables', () => { + expect( + upsertPromotedSettingsInContent( + '[[tui]]\ntheme = "array-theme"\n', + new Map([['tui.theme', '"root-theme"']]) + ) + ).toBe('[[tui]]\ntheme = "array-theme"\n') + }) + + it('does not append a table beside a quoted root tui array-of-tables', () => { + expect( + upsertPromotedSettingsInContent( + '[["tui"]]\ntheme = "array-theme"\n', + new Map([['tui.theme', '"root-theme"']]) + ) + ).toBe('[["tui"]]\ntheme = "array-theme"\n') + }) + + it('creates one [tui] table for multiple keys reaching the new-table branch', () => { + expect( + upsertPromotedSettingsInContent( + '', + new Map([ + ['tui.status_line', '["model"]'], + ['tui.theme', '"dark"'] + ]) + ) + ).toBe('[tui]\nstatus_line = ["model"]\ntheme = "dark"\n') + }) + + it('routes a mixed top-level + tui batch to its two regions in one rewrite', () => { + expect( + upsertPromotedSettingsInContent( + 'model = "gpt-5"\n\n[tui]\ntheme = "dark"\n', + new Map([ + ['model', '"o4"'], + ['tui.theme', '"light"'] + ]) + ) + ).toBe('model = "o4"\n\n[tui]\ntheme = "light"\n') + }) + + it('inserts into a CRLF [tui] table with CRLF endings', () => { + expect( + upsertPromotedSettingsInContent( + '[tui]\r\ntheme = "dark"\r\n', + new Map([['tui.status_line', '["model"]']]) + ) + ).toBe('[tui]\r\ntheme = "dark"\r\nstatus_line = ["model"]\r\n') + }) +}) + describe('upsertTopLevelSettingsInContent', () => { it('writes into empty content', () => { expect(upsertTopLevelSettingsInContent('', new Map([['model', '"x"']]))).toBe('model = "x"\n') @@ -428,6 +802,12 @@ describe('upsertTopLevelSettingsInContent', () => { ).toBe('# keep\nmodel = "new"\n\n[t]\nk = 1\n') }) + it('replaces a quoted top-level key instead of adding its bare equivalent', () => { + expect( + upsertTopLevelSettingsInContent('"model" = "old"\n', new Map([['model', '"new"']])) + ).toBe('model = "new"\n') + }) + it('inserts with CRLF endings into CRLF content', () => { expect( upsertTopLevelSettingsInContent('[features]\r\nhooks = true\r\n', new Map([['model', '"x"']])) diff --git a/src/main/codex/config-settings-promotion.ts b/src/main/codex/config-settings-promotion.ts index 372f1806c19..bc0a5421eff 100644 --- a/src/main/codex/config-settings-promotion.ts +++ b/src/main/codex/config-settings-promotion.ts @@ -8,7 +8,7 @@ import { writeFileSync } from 'node:fs' import { dirname, join, resolve } from 'node:path' -import { readAgentStateFileSync, readAgentStateJsonFileSync } from '../agent-state-file-reader' +import { readAgentStateFileSync } from '../agent-state-file-reader' import { writeFileAtomically } from '../codex-accounts/fs-utils' import { parseWslUncPath } from '../../shared/wsl-paths' import { getOrcaManagedCodexHomePath, getSystemCodexHomePath } from './codex-home-paths' @@ -18,6 +18,15 @@ import { isTomlStructuralLine, updateTomlLineScanState } from './config-toml-line-scan' +import { parseTomlKeyPath, parseTomlTableHeaderPath } from './config-toml-key-path' +import { tuiStructuredKey, upsertPromotedSettingsInContent } from './codex-config-settings-upsert' +import { + readCodexSettingsBaseline, + writeCodexSettingsBaseline, + type CodexSettingsBaseline, + type CodexSettingsConflict +} from './config-settings-baseline' +import { resolveUntrackedCodexSetting } from './config-settings-conflict-resolution' // Why: the mirror reverts in-Codex config changes each launch; promotion salvages them by diffing the last baseline. @@ -29,65 +38,112 @@ export const PROMOTED_CODEX_SETTING_KEYS = [ 'sandbox_mode' ] as const +// Why: the [tui] keys the Codex TUI's user-facing pickers persist (status line, +// terminal title, theme). Like the top-level list, every key here gets written +// into the user's real ~/.codex/config.toml on promotion — grow it deliberately. +export const PROMOTED_CODEX_TUI_SETTING_KEYS = [ + 'status_line', + 'status_line_use_colors', + 'terminal_title', + 'theme' +] as const + +// Why: promotion diffs and upserts operate on structured keys — top-level keys +// keep their bare name, [tui] keys are namespaced tui. so their baseline +// entries cannot collide with a top-level key of the same name. +const PROMOTED_STRUCTURED_KEYS: readonly string[] = [ + ...PROMOTED_CODEX_SETTING_KEYS, + ...PROMOTED_CODEX_TUI_SETTING_KEYS.map(tuiStructuredKey) +] + +function isPromotedTuiKey(key: string): boolean { + return (PROMOTED_CODEX_TUI_SETTING_KEYS as readonly string[]).includes(key) +} + +// Returns the structured tui key a scanned line's key represents, or null. In +// the preamble it recognizes the dotted `tui.` form a user may hand-author; +// inside the first `[tui]` table body it recognizes the bare `` form Codex +// writes. Both map to the same structured key so either config shape promotes. +function matchTuiStructuredKey( + keyPath: string[], + inPreamble: boolean, + tuiBodyActive: boolean +): string | null { + if (inPreamble) { + const tuiKey = keyPath.length === 2 && keyPath[0] === 'tui' ? keyPath[1] : null + return tuiKey && isPromotedTuiKey(tuiKey) ? tuiStructuredKey(tuiKey) : null + } + const tuiKey = keyPath.length === 1 ? keyPath[0] : null + return tuiBodyActive && tuiKey && isPromotedTuiKey(tuiKey) ? tuiStructuredKey(tuiKey) : null +} + type TopLevelSettingValue = { raw: string // Why: a multiline string/array value can't be replaced line-by-line, so it's excluded from promotion. multiline: boolean } -type SettingsBaselineFile = { - version: 1 - settings: Record -} - -function getSettingsBaselinePath(runtimeHomePath: string): string { - return join(runtimeHomePath, '.orca-config-settings-baseline.json') -} - -function readSettingsBaseline(runtimeHomePath: string): Map | null { - const baselinePath = getSettingsBaselinePath(runtimeHomePath) - if (!existsSync(baselinePath)) { +function matchPromotedStructuredKey( + line: string, + inPreamble: boolean, + tuiBodyActive: boolean +): { structuredKey: string; raw: string } | null { + const parsed = parseTomlKeyPath(line) + if (!parsed || line[parsed.end] !== '=') { return null } - try { - const parsed = readAgentStateJsonFileSync(baselinePath) - if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { - return null - } - const settings = (parsed as SettingsBaselineFile).settings - if (!settings || typeof settings !== 'object' || Array.isArray(settings)) { - return null - } - const result = new Map() - for (const [key, value] of Object.entries(settings)) { - if (typeof value === 'string') { - result.set(key, value) - } - } - return result - } catch { - return null + const raw = line.slice(parsed.end + 1).trim() + const topLevelKey = parsed.segments.length === 1 ? parsed.segments[0] : null + if ( + inPreamble && + topLevelKey && + (PROMOTED_CODEX_SETTING_KEYS as readonly string[]).includes(topLevelKey) + ) { + return { structuredKey: topLevelKey, raw } } + const tuiKey = matchTuiStructuredKey(parsed.segments, inPreamble, tuiBodyActive) + return tuiKey ? { structuredKey: tuiKey, raw } : null } -// Why: only top-level preamble keys are scanned; rewriting nested [profiles.*] tables isn't worth the risk here. -function readTopLevelSettingValues(configPath: string): Map { +// Why: top-level preamble scalars keep the historical behavior; [tui] keys are +// collected from the first bare [tui] table body or the dotted preamble form, +// keyed by structured path. Any table header (including [tui.*] subtables) ends +// the [tui] body, and [profiles.*]/other tables are still ignored. +function readPromotedSettingValues(configPath: string): Map { const result = new Map() if (!existsSync(configPath)) { return result } const lines = readAgentStateFileSync(configPath).split('\n') let state = createTomlLineScanState() + let inPreamble = true + let tuiTableSeen = false + let tuiBodyActive = false for (const line of lines) { if (isTomlStructuralLine(state)) { - if (getTomlTableHeader(line)) { - break + const header = getTomlTableHeader(line) + if (header) { + const table = parseTomlTableHeaderPath(header) + tuiBodyActive = + table !== null && + !table.isArray && + table.segments.length === 1 && + table.segments[0] === 'tui' && + !tuiTableSeen + if (tuiBodyActive) { + tuiTableSeen = true + } + inPreamble = false + state = updateTomlLineScanState(state, line) + continue } - const match = /^[ \t]*([A-Za-z0-9_-]+)[ \t]*=[ \t]*(.*?)[ \t\r]*$/.exec(line) - const key = match?.[1] - if (key && (PROMOTED_CODEX_SETTING_KEYS as readonly string[]).includes(key)) { + const matched = matchPromotedStructuredKey(line, inPreamble, tuiBodyActive) + if (matched) { const nextState = updateTomlLineScanState(state, line) - result.set(key, { raw: match?.[2] ?? '', multiline: !isTomlStructuralLine(nextState) }) + result.set(matched.structuredKey, { + raw: matched.raw, + multiline: !isTomlStructuralLine(nextState) + }) state = nextState continue } @@ -103,28 +159,22 @@ function readTopLevelSettingValues(configPath: string): Map = new Map() ): void { try { const runtimeTomlPath = join(runtimeHomePath, 'config.toml') // Why: record an empty baseline even for a missing runtime config, so Codex's first write still diffs and promotes. - const settings: Record = {} - for (const [key, value] of readTopLevelSettingValues(runtimeTomlPath)) { - if (!value.multiline) { - settings[key] = value.raw + const runtimeValues = readPromotedSettingValues(runtimeTomlPath) + const settings = new Map() + for (const key of PROMOTED_STRUCTURED_KEYS) { + const value = runtimeValues.get(key) + if (!conflicts.has(key) && !value?.multiline) { + // Why: explicit nulls distinguish a schema-aware absence from a key added by a later schema. + settings.set(key, value?.raw ?? null) } } - const file: SettingsBaselineFile = { version: 1, settings } - const baselinePath = getSettingsBaselinePath(runtimeHomePath) - const serialized = `${JSON.stringify(file, null, 2)}\n` - // Why: launch prep runs repeatedly; skip byte-identical rewrites to avoid needless disk writes. - if (existsSync(baselinePath) && readAgentStateFileSync(baselinePath) === serialized) { - return - } - writeFileSync(baselinePath, serialized, { - encoding: 'utf-8', - mode: 0o600 - }) + writeCodexSettingsBaseline(runtimeHomePath, { settings, conflicts }) } catch (error) { console.warn('[codex-settings-promotion] failed to snapshot settings baseline', error) } @@ -135,6 +185,11 @@ export type CodexSettingsPromotionHomes = { systemHomePath: string } +export type CodexSettingsPromotionPlan = { + conflicts: ReadonlyMap + runtimeValuesToPreserve: ReadonlyMap +} + function getHostPromotionHomes(): CodexSettingsPromotionHomes { return { runtimeHomePath: getOrcaManagedCodexHomePath(), @@ -147,56 +202,50 @@ function getHostPromotionHomes(): CodexSettingsPromotionHomes { * Runs before the config mirror so promoted values survive it instead of reverting. * WSL callers pass explicit per-distro homes; default is the host runtime home and ~/.codex. */ -export function promoteCodexRuntimeSettingsToSystem(homes?: CodexSettingsPromotionHomes): boolean { +export function promoteCodexRuntimeSettingsToSystem( + homes?: CodexSettingsPromotionHomes +): CodexSettingsPromotionPlan | null { try { - promoteCodexRuntimeSettingsToSystemUnsafe(homes ?? getHostPromotionHomes()) - return true + return promoteCodexRuntimeSettingsToSystemUnsafe(homes ?? getHostPromotionHomes()) } catch (error) { // Why: promotion is best-effort launch prep; a malformed file must not block Codex launch. console.warn('[codex-settings-promotion] failed to promote runtime settings', error) - return false + return null } } -function promoteCodexRuntimeSettingsToSystemUnsafe(homes: CodexSettingsPromotionHomes): void { +function promoteCodexRuntimeSettingsToSystemUnsafe( + homes: CodexSettingsPromotionHomes +): CodexSettingsPromotionPlan { const { runtimeHomePath, systemHomePath } = homes const runtimeTomlPath = join(runtimeHomePath, 'config.toml') const systemTomlPath = join(systemHomePath, 'config.toml') if (resolve(runtimeTomlPath) === resolve(systemTomlPath)) { - return + return emptyPromotionPlan() } if (!existsSync(runtimeTomlPath)) { - return + return emptyPromotionPlan() } // Why: without a baseline, a stale runtime value looks like a fresh in-Codex change; skip until the mirror writes one. - const baseline = readSettingsBaseline(runtimeHomePath) + const baseline = readCodexSettingsBaseline(runtimeHomePath) if (!baseline) { - return + return emptyPromotionPlan() } - const runtimeValues = readTopLevelSettingValues(runtimeTomlPath) - const systemValues = readTopLevelSettingValues(systemTomlPath) + const runtimeValues = readPromotedSettingValues(runtimeTomlPath) + const systemValues = readPromotedSettingValues(systemTomlPath) const updates = new Map() - for (const key of PROMOTED_CODEX_SETTING_KEYS) { - const runtime = runtimeValues.get(key) - if (!runtime || runtime.multiline) { - continue - } - if (runtime.raw === baseline.get(key)) { - // Orca mirrored this value and nothing touched it since — not a change. - continue - } - const system = systemValues.get(key) - if (system?.multiline) { - continue - } - // Why: ~/.codex is source of truth — an outside edit since the baseline wins over the in-Codex change. - if (system?.raw !== baseline.get(key)) { - continue - } - updates.set(key, runtime.raw) - } + const conflicts = new Map() + const runtimeValuesToPreserve = new Map() + collectPromotionChanges({ + baseline, + runtimeValues, + systemValues, + updates, + conflicts, + runtimeValuesToPreserve + }) if (updates.size === 0) { - return + return { conflicts, runtimeValuesToPreserve } } // Why: a fresh host has no ~/.codex; create it owner-only (holds auth.json) or the atomic write ENOENTs and the mirror wipes it. mkdirSync(systemHomePath, { recursive: true, mode: 0o700 }) @@ -205,15 +254,71 @@ function promoteCodexRuntimeSettingsToSystemUnsafe(homes: CodexSettingsPromotion mkdirSync(dirname(writeTarget.path), { recursive: true, mode: 0o700 }) const targetExists = existsSync(writeTarget.path) const systemContent = targetExists ? readAgentStateFileSync(writeTarget.path) : '' - const nextContent = upsertTopLevelSettingsInContent(systemContent, updates) + const nextContent = upsertPromotedSettingsInContent(systemContent, updates) + if (nextContent === systemContent) { + return { conflicts, runtimeValuesToPreserve } + } if (targetExists && parseWslUncPath(writeTarget.path)) { // Why: \\wsl$ 9P symlink metadata is unreliable; write through the existing file to preserve the WSL-side inode. writeFileSync(writeTarget.path, nextContent, 'utf-8') - return + return { conflicts, runtimeValuesToPreserve } } writeFileAtomically(writeTarget.path, nextContent, { mode: writeTarget.mode }) + return { conflicts, runtimeValuesToPreserve } +} + +type PromotionCollectionContext = { + baseline: CodexSettingsBaseline + runtimeValues: ReadonlyMap + systemValues: ReadonlyMap + updates: Map + conflicts: Map + runtimeValuesToPreserve: Map +} + +function collectPromotionChanges(context: PromotionCollectionContext): void { + for (const key of PROMOTED_STRUCTURED_KEYS) { + const runtimeRaw = getComparableRaw(context.runtimeValues.get(key)) + const systemRaw = getComparableRaw(context.systemValues.get(key)) + if (runtimeRaw === undefined || systemRaw === undefined) { + continue + } + + const existingConflict = context.baseline.conflicts.get(key) + if (existingConflict || !context.baseline.settings.has(key)) { + const resolution = resolveUntrackedCodexSetting(runtimeRaw, systemRaw, existingConflict) + if (resolution.action === 'promote-runtime') { + context.updates.set(key, resolution.raw) + } else if (resolution.action === 'preserve') { + // Why: a schema-new key has no three-way ancestor; preserve both values until content changes one side. + context.conflicts.set(key, resolution.conflict) + context.runtimeValuesToPreserve.set(key, runtimeRaw) + } + continue + } + + if (runtimeRaw === null || runtimeRaw === context.baseline.settings.get(key)) { + continue + } + // Why: ~/.codex remains source of truth when both sides changed from a known baseline. + if (systemRaw !== context.baseline.settings.get(key)) { + continue + } + context.updates.set(key, runtimeRaw) + } +} + +function getComparableRaw(value: TopLevelSettingValue | undefined): string | null | undefined { + if (!value) { + return null + } + return value.multiline ? undefined : value.raw +} + +function emptyPromotionPlan(): CodexSettingsPromotionPlan { + return { conflicts: new Map(), runtimeValuesToPreserve: new Map() } } // Why: follow an existing dotfile-manager symlink and carry its mode forward so an atomic write can't widen a 0600 config. @@ -252,55 +357,3 @@ function resolveDanglingSymlinkTarget(linkPath: string): string { // Why: replacing any link in a cycle would destroy dotfile-manager state; abort instead. throw new Error(`Codex config symlink cycle at ${linkPath}`) } - -export function upsertTopLevelSettingsInContent( - content: string, - updates: Map -): string { - const lines = content.split('\n') - let state = createTomlLineScanState() - let preambleEnd = lines.length - const keyLineIndexes = new Map() - for (let index = 0; index < lines.length; index += 1) { - const line = lines[index] ?? '' - if (isTomlStructuralLine(state)) { - if (getTomlTableHeader(line)) { - preambleEnd = index - break - } - const match = /^[ \t]*([A-Za-z0-9_-]+)[ \t]*=/.exec(line) - if (match?.[1] && updates.has(match[1])) { - keyLineIndexes.set(match[1], index) - } - } - state = updateTomlLineScanState(state, line) - } - - // Why: match the file's existing EOL (CRLF split leaves a trailing \r) so a Windows config doesn't go mixed-EOL. - const usesCrlf = content.includes('\r\n') - const insertions: string[] = [] - for (const [key, raw] of updates) { - const existingIndex = keyLineIndexes.get(key) - const rendered = `${key} = ${raw}` - if (existingIndex !== undefined) { - lines[existingIndex] = lines[existingIndex]?.endsWith('\r') ? `${rendered}\r` : rendered - } else { - insertions.push(usesCrlf ? `${rendered}\r` : rendered) - } - } - if (insertions.length > 0) { - let insertAt = preambleEnd - while (insertAt > 0 && (lines[insertAt - 1] ?? '').trim() === '') { - insertAt -= 1 - } - if (insertAt === preambleEnd && preambleEnd < lines.length) { - insertions.push(usesCrlf ? '\r' : '') - } - lines.splice(insertAt, 0, ...insertions) - } - const result = lines.join('\n') - if (result.endsWith('\n') || result.length === 0) { - return result - } - return result.endsWith('\r') ? `${result}\n` : `${result}${usesCrlf ? '\r\n' : '\n'}` -} diff --git a/src/main/codex/config-toml-key-path.ts b/src/main/codex/config-toml-key-path.ts new file mode 100644 index 00000000000..25eaa4c98c6 --- /dev/null +++ b/src/main/codex/config-toml-key-path.ts @@ -0,0 +1,67 @@ +import { parseTomlSingleLineStringValue } from './config-toml-line-scan' + +export type ParsedTomlKeyPath = { + segments: string[] + end: number +} + +export type ParsedTomlTableHeaderPath = ParsedTomlKeyPath & { + isArray: boolean +} + +export function parseTomlTableHeaderPath(header: string): ParsedTomlTableHeaderPath | null { + const trimmed = header.trim() + let source: string + let isArray: boolean + if (trimmed.startsWith('[[')) { + if (!trimmed.endsWith(']]')) { + return null + } + source = trimmed.slice(2, -2) + isArray = true + } else { + if (!trimmed.startsWith('[') || !trimmed.endsWith(']') || trimmed.endsWith(']]')) { + return null + } + source = trimmed.slice(1, -1) + isArray = false + } + const parsed = parseTomlKeyPath(source) + if (!parsed || parsed.end !== source.length) { + return null + } + return { ...parsed, isArray } +} + +export function parseTomlKeyPath(source: string, offset = 0): ParsedTomlKeyPath | null { + const segments: string[] = [] + let index = skipTomlKeyWhitespace(source, offset) + while (index < source.length) { + const quoted = parseTomlSingleLineStringValue(source, index) + if (quoted) { + segments.push(quoted.value) + index = quoted.end + } else { + const bare = /^[A-Za-z0-9_-]+/.exec(source.slice(index)) + if (!bare) { + return null + } + segments.push(bare[0]) + index += bare[0].length + } + index = skipTomlKeyWhitespace(source, index) + if (source[index] !== '.') { + return { segments, end: index } + } + index = skipTomlKeyWhitespace(source, index + 1) + } + return null +} + +function skipTomlKeyWhitespace(source: string, offset: number): number { + let index = offset + while (source[index] === ' ' || source[index] === '\t') { + index += 1 + } + return index +} From 90255ab112b3e8d9ec74421edc7c68e9fd159224 Mon Sep 17 00:00:00 2001 From: Crosery <130495739+Crosery@users.noreply.github.com> Date: Fri, 24 Jul 2026 07:29:59 +0800 Subject: [PATCH 09/10] [Bug]: Defer full worktree scan until after session restore (#9446) * perf(startup): defer full worktree scan until after session restore * fix(startup): normalize worktree recovery session keys * fix(startup): preserve persisted workspace hydration * fix(startup): keep SSH repos in pre-hydration worktree fetch Round 1 review fixes for the selective pre-hydration fetch: - The prefetch filter gated on LOCAL_EXECUTION_HOST_ID, which excluded SSH repos. Old full-scan code populated worktreesByRepo for disconnected SSH repos via the local metadata fallback (no network), so hydrateTabsSession / editor / browser preserved their chrome. Excluding SSH dropped that chrome at hydration and setHydrationSucceeded made the loss durable. Include SSH by excluding only runtime-owned repos (which hydrate via placeholders instead). - collectWorktreeHydrationRepoIdsFromSession never collected activeRepoId, so a repo referenced only by active-repo state (no active worktree, no tabs) was not prefetched, breaking hydrateWorkspaceSession's main-worktree fallback. * fix(startup): re-prune visit timestamps after deferred full worktree scan Round 2 review fix. The selective pre-hydration fetch only populates session-referenced repos, so the one-shot pruneLastVisitedTimestamps at startup no longer sees non-session repos and keeps their deleted-worktree entries (the prune defers unloaded repos to preserve SSH focus recency). The old full pre-hydration scan had populated all local repos before the prune. Re-run the prune after the deferred fetchAllWorktrees so stale lastVisitedAtByWorktreeId entries are dropped once every repo is authoritative; disconnected SSH stays non-authoritative and is kept. * fix(startup): exclude unbounded history maps from hydration repo enumeration Round 3 review fix. collectWorktreeHydrationRepoIdsFromSession collected repo ids from lastVisitedAtByWorktreeId and defaultTerminalTabsAppliedByWorktreeId, which accumulate one entry per worktree ever focused / given default tabs. On the long-lived large profile this fix targets (#9441), those maps reference nearly every repo, so the 'selective' pre-hydration fetch degenerated back toward the full all-repos scan the fix removed. Split the keyed-field list: repo enumeration now scans only chrome-bearing fields (open tabs/editors/browser), while folder-key detection keeps scanning all fields. The history maps hydrate unfiltered regardless, and the deferred re-prune reaps their stale entries, so no data or recency is lost. Also pin the round-2 re-prune ordering in the routing test. * fix(startup): prevent competing full worktree scans * fix(startup): keep selective hydration scans bounded * fix(startup): ignore stale tab selection markers * fix(startup): keep deferred worktree recovery selective * test(windows): dismiss startup overlays before terminal setup --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> --- src/renderer/src/App.tsx | 97 ++++---- src/renderer/src/app-startup-routing.test.ts | 110 ++++++--- .../automations/AutomationsPage.tsx | 15 +- .../src/components/sidebar/Sidebar.test.tsx | 76 ++++++- src/renderer/src/components/sidebar/index.tsx | 31 +-- .../workspace-session-hydration-keys.test.ts | 208 ++++++++++++++++++ .../lib/workspace-session-hydration-keys.ts | 130 +++++++++-- .../src/store/slices/worktree-helpers.ts | 9 +- .../src/store/slices/worktrees.test.ts | 39 ++++ src/renderer/src/store/slices/worktrees.ts | 5 +- tools/win-update-e2e/app-driver.mjs | 1 + 11 files changed, 613 insertions(+), 108 deletions(-) create mode 100644 src/renderer/src/lib/workspace-session-hydration-keys.test.ts diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 869ebda72e8..adc405a4219 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -42,6 +42,7 @@ import { ContextMenuTrigger } from '@/components/ui/context-menu' import { useAppStore } from './store' +import { WORKTREE_REFRESH_CONCURRENCY } from './store/slices/worktrees' import { useShallow } from 'zustand/react/shallow' import { isRemoteWorkspaceSnapshotApplyInProgress, useIpcEvents } from './hooks/useIpcEvents' import { useAutomationDispatchEvents } from './hooks/useAutomationDispatchEvents' @@ -131,7 +132,10 @@ import { createShutdownCheckpointBeforeUnloadHandler, createShutdownCheckpointGuard } from './lib/shutdown-checkpoint-guard' -import { collectFolderWorkspaceKeysFromSession } from './lib/workspace-session-hydration-keys' +import { + collectFolderWorkspaceKeysFromSession, + collectWorktreeHydrationRepoIdsFromSession +} from './lib/workspace-session-hydration-keys' import { getStartupErrorFallbackUI, hydratePersistedUIAfterStartupRead @@ -178,10 +182,13 @@ import { type PhysicalModifierToken } from '../../shared/keybindings' import { + getRepoExecutionHostId, isRuntimeOwnedSshTargetId, + parseExecutionHostId, toRuntimeExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import { mapWithConcurrency } from '../../shared/map-with-concurrency' import { ModifierDoubleTapDetector, toModifierDoubleTapEvent @@ -430,6 +437,7 @@ function App(): React.JSX.Element { fetchFolderWorkspaces: s.fetchFolderWorkspaces, fetchFolderWorkspacesForAllHosts: s.fetchFolderWorkspacesForAllHosts, fetchAllWorktrees: s.fetchAllWorktrees, + fetchWorktrees: s.fetchWorktrees, fetchWorktreeLineage: s.fetchWorktreeLineage, fetchOrcaProfiles: s.fetchOrcaProfiles, fetchSettings: s.fetchSettings, @@ -872,8 +880,7 @@ function App(): React.JSX.Element { actions.fetchReposForAllHosts({ remoteHosts: 'skip' }) ) // Why: folder workspaces merge against projectGroups (repos.ts fetchFolderWorkspacesForAllHosts), - // so keep this two-step catalog chain internally ordered; it is otherwise independent of - // repos/worktrees/session and overlaps the worktree scan below. + // so keep this chain ordered while overlapping it with session-scoped hydration. const localCatalogChain = (async () => { await timeRendererStartupStep('fetch-project-groups-local', () => actions.fetchProjectGroupsForAllHosts({ remoteHosts: 'skip' }) @@ -882,10 +889,6 @@ function App(): React.JSX.Element { actions.fetchFolderWorkspacesForAllHosts({ remoteHosts: 'skip' }) ) })() - // Why: chain session-get off runtimeHostsPromise instead of awaiting the host ids here, so - // fetch-worktrees and the catalog chain start immediately (neither needs the ids) — awaiting - // the host-list IPC first would re-serialize the worktree scan behind host discovery when the - // IPC is the slower of the two. Only session-get waits on the ids. const sessionReadPromise = runtimeHostsPromise.then((startupRuntimeHostIds) => // Why: include saved runtime host ids so per-host worktree session slices restore from local settings without waiting on network reachability; unreadable partitions skip. timeRendererStartupStep('session-get', () => @@ -896,27 +899,30 @@ function App(): React.JSX.Element { ) ) ) - // Why: once repos is loaded, fetch-worktrees (snapshots repos), session-get (repos-independent - // local disk read), and the local catalog chain are mutually independent — run them concurrently - // so the two disk reads hide behind the O(repos) worktree git scan (the startup long pole). - // fetchAllWorktrees({hydrationPurge:'defer'}) returns before its folderWorkspaces read (the purge - // guard in worktrees.ts), so it needs no catalog ordering here. session-get is a pure read; - // hydrate-session-stores below still runs only after all three settle. See #18. - // Why (#18 review): join on allSettled, NOT fail-fast Promise.all. A fast rejection from one branch - // would drop into the catch/recovery path (which reconnects terminals and flips readiness) while a - // sibling hydration task is still in flight and mutating catalog/worktree state — the old serial flow - // guaranteed no hydration step ran during recovery. Wait for all three to settle, then surface the - // first rejection so recovery still triggers, but only once nothing is left writing to the store. - const [worktreesOutcome, sessionOutcome, catalogOutcome] = await Promise.allSettled([ - timeRendererStartupStep('fetch-worktrees', () => - actions.fetchAllWorktrees({ hydrationPurge: 'defer' }) - ), - sessionReadPromise, + const hydrationSessionChain = sessionReadPromise.then(async (sessionRead) => { + const hydrationRepoIds = collectWorktreeHydrationRepoIdsFromSession( + sessionRead.session, + sessionRead.runtimeHostIdByWorkspaceSessionKey + ) + const hydrationRepoIdSet = new Set(hydrationRepoIds) + const hydrationRepos = useAppStore.getState().repos.filter( + (repo) => + hydrationRepoIdSet.has(repo.id) && + // Why: disconnected SSH repos hydrate from local metadata; only runtime-owned repos use placeholders. + parseExecutionHostId(getRepoExecutionHostId(repo))?.kind !== 'runtime' + ) + await timeRendererStartupStep('fetch-hydration-worktrees', () => + mapWithConcurrency(hydrationRepos, WORKTREE_REFRESH_CONCURRENCY, (repo) => + actions.fetchWorktrees(repo.id, { executionHostId: getRepoExecutionHostId(repo) }) + ) + ) + return sessionRead + }) + // Why: wait for both writers to settle before recovery so neither can mutate hydrated state afterward. + const [sessionOutcome, catalogOutcome] = await Promise.allSettled([ + hydrationSessionChain, localCatalogChain ]) - if (worktreesOutcome.status === 'rejected') { - throw worktreesOutcome.reason - } if (sessionOutcome.status === 'rejected') { throw sessionOutcome.reason } @@ -1042,19 +1048,34 @@ function App(): React.JSX.Element { }) void (async () => { try { - await timeRendererStartupStep('remote-catalog-refresh', async () => { - await actions.fetchReposForAllHosts() - await actions.fetchProjectGroupsForAllHosts() - await actions.fetchFolderWorkspacesForAllHosts() - }) - if (!cancelled) { - await timeRendererStartupStep('remote-worktree-refresh', async () => { - await actions.fetchAllWorktrees() - await actions.fetchWorktreeLineage() + try { + await timeRendererStartupStep('remote-catalog-refresh', async () => { + await actions.fetchReposForAllHosts() + await actions.fetchProjectGroupsForAllHosts() + await actions.fetchFolderWorkspacesForAllHosts() }) + } catch (err) { + console.warn('Remote startup catalog refresh failed:', err) + } + if (!cancelled) { + try { + await timeRendererStartupStep('remote-worktree-refresh', async () => { + // Why: the full scan is not required for session recovery, so keep it off the startup-critical path. + await actions.fetchAllWorktrees() + // Why: the startup prune only saw session-referenced repos; use the deferred scan's + // authoritative results to drop deleted-worktree visit timestamps that would + // otherwise accumulate unbounded (disconnected SSH stays non-authoritative and is kept). + actions.pruneLastVisitedTimestamps() + await actions.fetchWorktreeLineage() + }) + } catch (err) { + console.warn('Deferred startup worktree refresh failed:', err) + } + } + } finally { + if (!cancelled) { + useAppStore.setState({ startupWorktreeRefreshCompleted: true }) } - } catch (err) { - console.warn('Remote startup catalog refresh failed:', err) } })() } @@ -1067,6 +1088,8 @@ function App(): React.JSX.Element { error ) if (!cancelled) { + // Why: degraded mode stays interactive; later repo/runtime changes must not remain gated forever. + useAppStore.setState({ startupWorktreeRefreshCompleted: true }) // Why (issue #1158): only apply default UI if ui.get() never hydrated; otherwise defaults would clobber ui.json via the debounced writer. const fallbackUI = getStartupErrorFallbackUI(uiHydrated) if (fallbackUI) { diff --git a/src/renderer/src/app-startup-routing.test.ts b/src/renderer/src/app-startup-routing.test.ts index ad0097bff03..05f5837e7c8 100644 --- a/src/renderer/src/app-startup-routing.test.ts +++ b/src/renderer/src/app-startup-routing.test.ts @@ -6,30 +6,31 @@ describe('renderer startup runtime routing', () => { it('hydrates persisted UI before local catalog and worktree hydration', () => { const source = readFileSync(join(process.cwd(), 'src/renderer/src/App.tsx'), 'utf8') const startupBlockStart = source.indexOf('void (async () => {') - // Why (#18): worktrees/session-get/catalog now run concurrently, so session-get is no longer the - // block terminator. End the slice at hydrate-session-stores (runs after the Promise.all settles) so - // every concurrent fetch falls inside the analyzed block. + // Why: concurrent startup branches all settle before hydrate-session-stores. const startupBlockEnd = source.indexOf("timeRendererStartupSyncStep('hydrate-session-stores'") const startupBlock = source.slice(startupBlockStart, startupBlockEnd) - const settingsIndex = startupBlock.indexOf('actions.fetchSettings()') - const uiGetIndex = startupBlock.indexOf("timeRendererStartupStep('ui-get'") - const hydrateUiIndex = startupBlock.indexOf( - "timeRendererStartupSyncStep('hydrate-persisted-ui'" - ) - const localReposIndex = startupBlock.indexOf( + const indexInStartupBlock = (needle: string): number => { + const relativeIndex = startupBlock.indexOf(needle) + return relativeIndex === -1 ? -1 : startupBlockStart + relativeIndex + } + const settingsIndex = indexInStartupBlock('actions.fetchSettings()') + const uiGetIndex = indexInStartupBlock("timeRendererStartupStep('ui-get'") + const hydrateUiIndex = indexInStartupBlock("timeRendererStartupSyncStep('hydrate-persisted-ui'") + const localReposIndex = indexInStartupBlock( "actions.fetchReposForAllHosts({ remoteHosts: 'skip' })" ) - const localGroupsIndex = startupBlock.indexOf( + const localGroupsIndex = indexInStartupBlock( "actions.fetchProjectGroupsForAllHosts({ remoteHosts: 'skip' })" ) - const localFoldersIndex = startupBlock.indexOf( + const localFoldersIndex = indexInStartupBlock( "actions.fetchFolderWorkspacesForAllHosts({ remoteHosts: 'skip' })" ) - const sessionGetIndex = startupBlock.indexOf("timeRendererStartupStep('session-get'") - const localWorktreesIndex = startupBlock.indexOf( - "actions.fetchAllWorktrees({ hydrationPurge: 'defer' })" + const sessionIndex = indexInStartupBlock("timeRendererStartupStep('session-get'") + const hydrationWorktreesIndex = source.indexOf( + "timeRendererStartupStep('fetch-hydration-worktrees'" ) + const fullWorktreesIndex = source.indexOf('await actions.fetchAllWorktrees()') const lineageIndex = startupBlock.indexOf('actions.fetchWorktreeLineage()') expect(settingsIndex).toBeGreaterThanOrEqual(0) @@ -41,26 +42,44 @@ describe('renderer startup runtime routing', () => { // The local catalog chain stays internally ordered (folders merge against project groups). expect(localReposIndex).toBeLessThan(localGroupsIndex) expect(localGroupsIndex).toBeLessThan(localFoldersIndex) - // Worktree scan and session read both start only after repos is loaded (they snapshot/route on it), - // but run concurrently with the catalog chain — see the Promise.all assertion below. - expect(localReposIndex).toBeLessThan(sessionGetIndex) - expect(localReposIndex).toBeLessThan(localWorktreesIndex) - // Lineage is deferred to post-hydration remote refresh, not part of the local hydration block. + expect(localReposIndex).toBeLessThan(sessionIndex) + expect(sessionIndex).toBeLessThan(hydrationWorktreesIndex) + const hydrationWorktreeBlock = source.slice( + hydrationWorktreesIndex, + source.indexOf('await keybindingsPromise') + ) + expect(hydrationWorktreeBlock).toContain( + 'mapWithConcurrency(hydrationRepos, WORKTREE_REFRESH_CONCURRENCY' + ) + expect(hydrationWorktreeBlock).toContain('executionHostId: getRepoExecutionHostId(repo)') + // Why: the pre-hydration fetch must include SSH repos (only runtime-owned repos are + // excluded); gating on local-only drops SSH tab/editor/browser chrome at hydration. + const hydrationFilterBlock = source.slice( + source.indexOf('const hydrationRepos'), + hydrationWorktreesIndex + ) + expect(hydrationFilterBlock).toContain( + "parseExecutionHostId(getRepoExecutionHostId(repo))?.kind !== 'runtime'" + ) + expect(hydrationFilterBlock).not.toContain('=== LOCAL_EXECUTION_HOST_ID') + expect(fullWorktreesIndex).toBeGreaterThan( + source.indexOf("logRendererStartupDiagnostic('startup-hydration-done'") + ) + // Why: the deferred full scan must be followed by a re-prune so deleted-worktree visit + // timestamps for non-session repos are dropped once every repo is authoritative. + expect( + source.indexOf('actions.pruneLastVisitedTimestamps()', fullWorktreesIndex) + ).toBeGreaterThan(fullWorktreesIndex) expect(lineageIndex).toBe(-1) - // Guard the concurrency itself: fetch-worktrees, the session read, and the local catalog chain must be - // joined in a single allSettled so the two disk reads hide behind the O(repos) worktree scan — AND so a - // fast rejection can't enter recovery while a sibling is still mutating the store (allSettled, not - // fail-fast Promise.all). - const joinStart = startupBlock.indexOf('await Promise.allSettled([') + // The catalog and selective hydration chains overlap, but both settle before recovery or hydration. + const joinStart = indexInStartupBlock('await Promise.allSettled([') expect(joinStart).toBeGreaterThan(hydrateUiIndex) - const joinBlock = startupBlock.slice(joinStart) - expect(joinBlock).toContain("timeRendererStartupStep('fetch-worktrees'") - expect(joinBlock).toContain('sessionReadPromise') + const joinBlock = source.slice(joinStart, startupBlockEnd) + expect(joinBlock).toContain('hydrationSessionChain') expect(joinBlock).toContain('localCatalogChain') - // The join must not be fail-fast: a bare `Promise.all([` on these branches would re-introduce the - // recovery-during-hydration race. expect(startupBlock).not.toContain('await Promise.all([') + expect(startupBlock).not.toContain("actions.fetchAllWorktrees({ hydrationPurge: 'defer' })") }) it('refreshes remote catalogs after startup hydration succeeds', () => { @@ -70,15 +89,30 @@ describe('renderer startup runtime routing', () => { ) const remoteCatalogIndex = source.indexOf("timeRendererStartupStep('remote-catalog-refresh'") const remoteWorktreeIndex = source.indexOf("timeRendererStartupStep('remote-worktree-refresh'") + const remoteCatalogFailureIndex = source.indexOf( + "console.warn('Remote startup catalog refresh failed:'" + ) const lineageIndex = source.indexOf('actions.fetchWorktreeLineage()') + const startupRefreshCompletedIndex = source.indexOf('startupWorktreeRefreshCompleted: true') expect(hydrationDoneIndex).toBeGreaterThanOrEqual(0) expect(hydrationDoneIndex).toBeLessThan(remoteCatalogIndex) - expect(remoteCatalogIndex).toBeLessThan(remoteWorktreeIndex) + expect(remoteCatalogIndex).toBeLessThan(remoteCatalogFailureIndex) + // Why: a project-group/folder catalog failure must not suppress the independent full worktree scan. + expect(remoteCatalogFailureIndex).toBeLessThan(remoteWorktreeIndex) expect(remoteWorktreeIndex).toBeLessThan(lineageIndex) + expect(lineageIndex).toBeLessThan(startupRefreshCompletedIndex) expect(source.slice(remoteCatalogIndex, remoteWorktreeIndex)).toContain( 'actions.fetchReposForAllHosts()' ) + + const startupFailureIndex = source.indexOf( + '[startup] Workspace session hydration failed; leaving disk state untouched:' + ) + expect(startupFailureIndex).toBeGreaterThanOrEqual(0) + expect( + source.indexOf('startupWorktreeRefreshCompleted: true', startupFailureIndex) + ).toBeGreaterThan(startupFailureIndex) expect(source.slice(remoteCatalogIndex, remoteWorktreeIndex)).toContain( 'actions.fetchProjectGroupsForAllHosts()' ) @@ -96,6 +130,22 @@ describe('renderer startup runtime routing', () => { expect(servicesIndex).toBeLessThan(reconnectIndex) }) + it('keeps the persisted Automations view from starting its own bootstrap worktree scan', () => { + const source = readFileSync( + join(process.cwd(), 'src/renderer/src/components/automations/AutomationsPage.tsx'), + 'utf8' + ) + const fullRefreshStart = source.indexOf('const mountedBeforeStartupWorktreeRefreshRef') + const fullRefreshEffect = source.slice( + fullRefreshStart, + source.indexOf('void refresh()', fullRefreshStart) + ) + + expect(fullRefreshEffect).toContain('if (!startupWorktreeRefreshCompleted)') + expect(fullRefreshEffect).toContain('mountedBeforeStartupWorktreeRefreshRef.current') + expect(fullRefreshEffect).toContain('void fetchAllWorktrees()') + }) + it('does not eagerly import the floating terminal panel on startup', () => { const source = readFileSync(join(process.cwd(), 'src/renderer/src/App.tsx'), 'utf8') diff --git a/src/renderer/src/components/automations/AutomationsPage.tsx b/src/renderer/src/components/automations/AutomationsPage.tsx index 34a8a66eedb..a9ecba818fe 100644 --- a/src/renderer/src/components/automations/AutomationsPage.tsx +++ b/src/renderer/src/components/automations/AutomationsPage.tsx @@ -368,6 +368,7 @@ export default function AutomationsPage(): React.JSX.Element { const activeWorktreeId = useAppStore((s) => s.activeWorktreeId) const fetchWorktrees = useAppStore((s) => s.fetchWorktrees) const fetchAllWorktrees = useAppStore((s) => s.fetchAllWorktrees) + const startupWorktreeRefreshCompleted = useAppStore((s) => s.startupWorktreeRefreshCompleted) const updateSettings = useAppStore((s) => s.updateSettings) const openSettingsPage = useAppStore((s) => s.openSettingsPage) const openSettingsTarget = useAppStore((s) => s.openSettingsTarget) @@ -1051,10 +1052,22 @@ export default function AutomationsPage(): React.JSX.Element { useAppStore.getState().hydratePersistedUI(await window.api.ui.get(), 'sync') }, []) + const mountedBeforeStartupWorktreeRefreshRef = useRef(!startupWorktreeRefreshCompleted) useEffect(() => { + if (!startupWorktreeRefreshCompleted) { + return + } + if (mountedBeforeStartupWorktreeRefreshRef.current) { + // Why: App just supplied this mount's initial worktrees; a second full scan would duplicate every repo probe. + mountedBeforeStartupWorktreeRefreshRef.current = false + return + } void fetchAllWorktrees() + }, [fetchAllWorktrees, startupWorktreeRefreshCompleted]) + + useEffect(() => { void refresh() - }, [fetchAllWorktrees, refresh]) + }, [refresh]) useEffect(() => { // Pause the relative-time clock while the window is hidden. diff --git a/src/renderer/src/components/sidebar/Sidebar.test.tsx b/src/renderer/src/components/sidebar/Sidebar.test.tsx index cb47521f571..210e4f76cb6 100644 --- a/src/renderer/src/components/sidebar/Sidebar.test.tsx +++ b/src/renderer/src/components/sidebar/Sidebar.test.tsx @@ -1,6 +1,10 @@ +// @vitest-environment happy-dom + import type { CSSProperties, ReactNode } from 'react' import { renderToStaticMarkup } from 'react-dom/server' -import { describe, expect, it, vi } from 'vitest' +import { tmpdir } from 'node:os' +import { cleanup, render, waitFor } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' import { getDefaultSettings } from '../../../../shared/constants' import type { GlobalSettings } from '../../../../shared/types' @@ -105,10 +109,18 @@ function renderSidebar(): string { ) } +function sidebarElement(): ReactNode { + return ( + + ) +} + +afterEach(cleanup) + describe('Sidebar', () => { it('applies left sidebar appearance variables to the workspace sidebar surface', () => { setSidebarState({ - ...getDefaultSettings('/tmp'), + ...getDefaultSettings(tmpdir()), leftSidebarAppearanceMode: 'match-terminal', terminalColorOverrides: { background: '#101820', @@ -125,11 +137,69 @@ describe('Sidebar', () => { }) it('passes status bar visibility into the workspace board drawer', () => { - setSidebarState(getDefaultSettings('/tmp'), false) + setSidebarState(getDefaultSettings(tmpdir()), false) const markup = renderSidebar() expect(markup).toContain('data-testid="workspace-kanban-drawer"') expect(markup).toContain('data-status-bar-visible="false"') }) + + it('does not start a full worktree scan while the startup session is hydrating', () => { + setSidebarState(getDefaultSettings(tmpdir())) + const fetchAllWorktrees = vi.fn().mockResolvedValue(undefined) + mocks.state = { + ...mocks.state, + fetchAllWorktrees, + repos: [], + startupWorktreeRefreshCompleted: false + } + const view = render(sidebarElement()) + + mocks.state = { ...mocks.state, repos: [{ id: 'repo-a' }] } + view.rerender(sidebarElement()) + expect(fetchAllWorktrees).not.toHaveBeenCalled() + + mocks.state = { ...mocks.state, startupWorktreeRefreshCompleted: true } + view.rerender(sidebarElement()) + expect(fetchAllWorktrees).not.toHaveBeenCalled() + + mocks.state = { ...mocks.state, repos: [{ id: 'repo-a' }, { id: 'repo-b' }] } + view.rerender(sidebarElement()) + expect(fetchAllWorktrees).toHaveBeenCalledTimes(1) + }) + + it('does not scan when runtime hosts come online during the startup refresh', async () => { + setSidebarState(getDefaultSettings(tmpdir())) + const fetchAllWorktrees = vi.fn().mockResolvedValue(undefined) + mocks.state = { + ...mocks.state, + fetchAllWorktrees, + fetchWorktreeLineage: vi.fn().mockResolvedValue(undefined), + runtimeStatusByEnvironmentId: new Map(), + startupWorktreeRefreshCompleted: false + } + const view = render(sidebarElement()) + + mocks.state = { + ...mocks.state, + runtimeStatusByEnvironmentId: new Map([['runtime-a', { status: 'connected' }]]) + } + view.rerender(sidebarElement()) + expect(fetchAllWorktrees).not.toHaveBeenCalled() + + mocks.state = { ...mocks.state, startupWorktreeRefreshCompleted: true } + view.rerender(sidebarElement()) + expect(fetchAllWorktrees).not.toHaveBeenCalled() + + mocks.state = { + ...mocks.state, + runtimeStatusByEnvironmentId: new Map([ + ['runtime-a', { status: 'connected' }], + ['runtime-b', { status: 'connected' }] + ]) + } + view.rerender(sidebarElement()) + await waitFor(() => expect(fetchAllWorktrees).toHaveBeenCalledTimes(1)) + }) }) diff --git a/src/renderer/src/components/sidebar/index.tsx b/src/renderer/src/components/sidebar/index.tsx index 30135806467..cddf00d01f9 100644 --- a/src/renderer/src/components/sidebar/index.tsx +++ b/src/renderer/src/components/sidebar/index.tsx @@ -46,6 +46,7 @@ function Sidebar({ const sidebarWidth = useAppStore((s) => s.sidebarWidth) const setSidebarWidth = useAppStore((s) => s.setSidebarWidth) const repos = useAppStore((s) => s.repos) + const startupWorktreeRefreshCompleted = useAppStore((s) => s.startupWorktreeRefreshCompleted) const settings = useAppStore((s) => s.settings) const fetchAllWorktrees = useAppStore((s) => s.fetchAllWorktrees) const activeModal = useAppStore((s) => s.activeModal) @@ -74,13 +75,16 @@ function Sidebar({ document.documentElement.style.setProperty('--workspace-sidebar-live-width', `${width}px`) }, []) - // Fetch worktrees when repos are added/removed const repoCount = repos.length + const previousRepoCountRef = React.useRef(repoCount) useEffect(() => { - if (repoCount > 0) { - fetchAllWorktrees() + const repoCountChanged = previousRepoCountRef.current !== repoCount + previousRepoCountRef.current = repoCount + // Why: App owns the initial all-host scan; partial startup catalogs must not trigger broad scans or stale-state purges. + if (startupWorktreeRefreshCompleted && repoCountChanged && repoCount > 0) { + void fetchAllWorktrees() } - }, [repoCount, fetchAllWorktrees]) + }, [repoCount, startupWorktreeRefreshCompleted, fetchAllWorktrees]) // Why: a runtime host coming online/offline must refresh the sidebar so its // worktrees appear/drop, the same way SSH state changes already refetch. Only @@ -109,18 +113,17 @@ function Sidebar({ ) } useEffect(() => { - // Skip the initial value — startup/repoCount effects already fetch. Only - // refetch when the online-host set actually changes. - if (previousOnlineRuntimeEnvKeyRef.current === null) { - previousOnlineRuntimeEnvKeyRef.current = onlineRuntimeEnvKey - return - } - if (previousOnlineRuntimeEnvKeyRef.current === onlineRuntimeEnvKey) { - return - } + const previousOnlineRuntimeEnvKey = previousOnlineRuntimeEnvKeyRef.current previousOnlineRuntimeEnvKeyRef.current = onlineRuntimeEnvKey + if ( + previousOnlineRuntimeEnvKey === null || + previousOnlineRuntimeEnvKey === onlineRuntimeEnvKey || + !startupWorktreeRefreshCompleted + ) { + return + } reconnectRefreshRef.current?.request() - }, [onlineRuntimeEnvKey]) + }, [onlineRuntimeEnvKey, startupWorktreeRefreshCompleted]) useEffect(() => { if (!sidebarOpen && workspaceBoardRenderedOpen) { diff --git a/src/renderer/src/lib/workspace-session-hydration-keys.test.ts b/src/renderer/src/lib/workspace-session-hydration-keys.test.ts new file mode 100644 index 00000000000..62f97f7b4c2 --- /dev/null +++ b/src/renderer/src/lib/workspace-session-hydration-keys.test.ts @@ -0,0 +1,208 @@ +import { describe, expect, it } from 'vitest' +import type { WorkspaceSessionState } from '../../../shared/types' +import { + collectFolderWorkspaceKeysFromSession, + collectWorktreeHydrationRepoIdsFromSession +} from './workspace-session-hydration-keys' + +describe('collectFolderWorkspaceKeysFromSession', () => { + it('keeps folder workspace selection markers valid without scheduling a Git scan', () => { + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: {}, + activeTabTypeByWorktree: { 'folder:folder-1': 'terminal' } + } as unknown as WorkspaceSessionState + + expect(collectFolderWorkspaceKeysFromSession(session)).toEqual(['folder:folder-1']) + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual([]) + }) +}) + +describe('collectWorktreeHydrationRepoIdsFromSession', () => { + it('includes persisted terminal tabs and ignores folder workspaces', () => { + const session = { + activeWorktreeIdsOnShutdown: [ + 'repo-a::/worktree-a', + 'repo-b::/worktree-b', + 'folder:folder-1' + ], + tabsByWorktree: { + 'repo-a::/worktree-a': [{ ptyId: 'pty-a' }], + 'repo-b::/worktree-b': [{ ptyId: null }], + 'folder:folder-1': [{ ptyId: 'pty-folder' }] + } + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-a', 'repo-b']) + }) + + it('recognizes split-pane and remote persisted sessions', () => { + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + terminalLayoutsByTabId: { + 'tab-a': { ptyIdsByLeafId: { 'pane:1': 'pty-a' } } + }, + tabsByWorktree: { + 'repo-a::/worktree-a': [{ id: 'tab-a', ptyId: null }], + 'repo-b::/worktree-b': [{ id: 'tab-b', ptyId: null }] + }, + remoteSessionIdsByTabId: { 'tab-b': 'remote-session' } + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-a', 'repo-b']) + }) + + it('matches canonical session keys against raw shutdown worktree IDs', () => { + const rawWorktreeId = 'repo-a::/worktree-a' + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + activeWorktreeIdsOnShutdown: [rawWorktreeId], + tabsByWorktree: { + [`worktree:${rawWorktreeId}`]: [{ ptyId: 'pty-a' }] + } + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-a']) + }) + + it('excludes runtime-owned session worktrees for raw and canonical owner keys', () => { + const rawWorktreeId = 'repo-a::/remote/worktree' + const canonicalWorktreeKey = `worktree:${rawWorktreeId}` + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: { + [canonicalWorktreeKey]: [{ ptyId: 'pty-a' }] + } + } as unknown as WorkspaceSessionState + + expect( + collectWorktreeHydrationRepoIdsFromSession(session, { + [rawWorktreeId]: 'runtime:env-1' + }) + ).toEqual([]) + expect( + collectWorktreeHydrationRepoIdsFromSession(session, { + [canonicalWorktreeKey]: 'runtime:env-1' + }) + ).toEqual([]) + }) + + it('keeps SSH-owned session worktrees eligible for local recovery routing', () => { + const rawWorktreeId = 'repo-a::/ssh/worktree' + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: { + [rawWorktreeId]: [{ ptyId: 'ssh:ssh-target@@pty-a' }] + } + } as unknown as WorkspaceSessionState + + expect( + collectWorktreeHydrationRepoIdsFromSession(session, { + [rawWorktreeId]: 'ssh:ssh-target' + }) + ).toEqual(['repo-a']) + }) + + it('returns repository IDs in deterministic order', () => { + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: { + 'repo-b::/worktree-b': [{ ptyId: 'pty-b' }], + 'repo-a::/worktree-a': [{ ptyId: 'pty-a' }] + } + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-a', 'repo-b']) + }) + + it('excludes repositories referenced only by unbounded history maps (visit recency, default tabs)', () => { + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: { 'repo-chrome::/wt': [{ ptyId: 'pty-a' }] }, + lastVisitedAtByWorktreeId: { + 'repo-chrome::/wt': 10, + 'repo-history-only::/visited': 20 + }, + defaultTerminalTabsAppliedByWorktreeId: { + 'repo-default-only::/applied': true + } + } as unknown as WorkspaceSessionState + + // Why: history-only repos hydrate their maps unfiltered and would bloat the selective fetch. + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-chrome']) + }) + + it('does not scan repositories retained only by empty chrome maps or selection markers', () => { + const emptyTabsByWorktree = Object.fromEntries( + Array.from({ length: 326 }, (_, index) => [`repo-empty-${index}::/wt`, []]) + ) + const staleActiveTabTypes = Object.fromEntries( + Array.from({ length: 326 }, (_, index) => [`repo-empty-${index}::/wt`, 'terminal']) + ) + const session = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: { + ...emptyTabsByWorktree, + 'repo-live::/wt': [{ ptyId: 'pty-a' }] + }, + openFilesByWorktree: { 'repo-empty-editor::/wt': [] }, + browserTabsByWorktree: { 'repo-empty-browser::/wt': [] }, + activeFileIdByWorktree: { 'repo-empty-active-file::/wt': '/stale/file.ts' }, + activeBrowserTabIdByWorktree: { 'repo-empty-active-browser::/wt': 'stale-browser' }, + activeTabIdByWorktree: { 'repo-empty-active-terminal::/wt': 'stale-terminal' }, + activeGroupIdByWorktree: { 'repo-empty-active-group::/wt': 'stale-group' }, + activeTabTypeByWorktree: staleActiveTabTypes + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-live']) + }) + + it('includes a repository referenced only by activeRepoId (no active worktree, no tabs)', () => { + const session = { + activeRepoId: 'repo-active-only', + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: {} + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual(['repo-active-only']) + }) + + it('includes repositories referenced only by active, editor, browser, or sleeping-agent state', () => { + const session = { + activeRepoId: null, + activeWorktreeId: 'repo-active::/active', + activeTabId: null, + tabsByWorktree: {}, + terminalLayoutsByTabId: {}, + openFilesByWorktree: { 'repo-editor::/editor': [{ filePath: '/editor/file.ts' }] }, + browserTabsByWorktree: { 'repo-browser::/browser': [{ id: 'browser-a' }] }, + sleepingAgentSessionsByPaneKey: { + 'tab:leaf': { worktreeId: 'repo-agent::/agent' } + } + } as unknown as WorkspaceSessionState + + expect(collectWorktreeHydrationRepoIdsFromSession(session)).toEqual([ + 'repo-active', + 'repo-agent', + 'repo-browser', + 'repo-editor' + ]) + }) +}) diff --git a/src/renderer/src/lib/workspace-session-hydration-keys.ts b/src/renderer/src/lib/workspace-session-hydration-keys.ts index 15513b458d5..c4e4ac9d00b 100644 --- a/src/renderer/src/lib/workspace-session-hydration-keys.ts +++ b/src/renderer/src/lib/workspace-session-hydration-keys.ts @@ -1,26 +1,48 @@ +import type { ExecutionHostId } from '../../../shared/execution-host' +import { parseExecutionHostId } from '../../../shared/execution-host' import type { WorkspaceKey, WorkspaceSessionState } from '../../../shared/types' import { parseWorkspaceKey } from '../../../shared/workspace-scope' +import { getRepoIdFromWorktreeId } from '../../../shared/worktree-id' export type WorkspaceSessionHydrationOptions = { additionalValidWorkspaceKeys?: readonly WorkspaceKey[] } -const WORKSPACE_KEYED_SESSION_FIELDS = [ +// Worktree-keyed fields carrying restorable chrome — a repo appears here only if it has live +// session state (open tabs, editors, browser) to restore. +const WORKSPACE_CHROME_SESSION_FIELDS = [ 'tabsByWorktree', 'openFilesByWorktree', - 'activeFileIdByWorktree', - 'activeBrowserTabIdByWorktree', - 'activeTabTypeByWorktree', - 'activeTabIdByWorktree', 'browserTabsByWorktree', 'unifiedTabs', 'tabGroups', - 'tabGroupLayouts', - 'activeGroupIdByWorktree', + 'tabGroupLayouts' +] as const satisfies readonly (keyof WorkspaceSessionState)[] + +// Why: unbounded per-worktree history — one entry per worktree ever focused / given default tabs. +// Folder-key detection still scans them, but repo-enumeration for pre-hydration must NOT: they'd +// pull in ~every repo the user ever touched and defeat the selective fetch, and they hydrate +// unfiltered regardless (the post-scan re-prune reaps stale entries). +const WORKSPACE_HISTORY_SESSION_FIELDS = [ 'lastVisitedAtByWorktreeId', 'defaultTerminalTabsAppliedByWorktreeId' ] as const satisfies readonly (keyof WorkspaceSessionState)[] +// Why: selection markers can outlive their content and cannot restore anything by themselves. +const WORKSPACE_SELECTION_SESSION_FIELDS = [ + 'activeFileIdByWorktree', + 'activeBrowserTabIdByWorktree', + 'activeTabIdByWorktree', + 'activeGroupIdByWorktree', + 'activeTabTypeByWorktree' +] as const satisfies readonly (keyof WorkspaceSessionState)[] + +const WORKSPACE_KEYED_SESSION_FIELDS = [ + ...WORKSPACE_CHROME_SESSION_FIELDS, + ...WORKSPACE_SELECTION_SESSION_FIELDS, + ...WORKSPACE_HISTORY_SESSION_FIELDS +] as const satisfies readonly (keyof WorkspaceSessionState)[] + function isPlainRecord(value: unknown): value is Record { return Boolean(value) && typeof value === 'object' && !Array.isArray(value) } @@ -35,42 +57,108 @@ function addFolderWorkspaceKey(keys: Set, value: unknown): void { } } -export function collectFolderWorkspaceKeysFromSession( - session: WorkspaceSessionState -): WorkspaceKey[] { - const keys = new Set() +function collectWorkspaceSessionKeys( + session: WorkspaceSessionState, + fields: readonly (keyof WorkspaceSessionState)[], + includeEntry: (value: unknown) => boolean = () => true +): string[] { + const keys = new Set() + const addKey = (value: unknown): void => { + if (typeof value === 'string') { + keys.add(value) + } + } - addFolderWorkspaceKey(keys, session.activeWorkspaceKey) - addFolderWorkspaceKey(keys, session.activeWorktreeId) - - for (const field of WORKSPACE_KEYED_SESSION_FIELDS) { + addKey(session.activeWorkspaceKey) + addKey(session.activeWorktreeId) + for (const field of fields) { const value = session[field] if (!isPlainRecord(value)) { continue } - for (const key of Object.keys(value)) { - addFolderWorkspaceKey(keys, key) + for (const [key, entry] of Object.entries(value)) { + if (includeEntry(entry)) { + addKey(key) + } } } - for (const worktreeId of session.activeWorktreeIdsOnShutdown ?? []) { - addFolderWorkspaceKey(keys, worktreeId) + addKey(worktreeId) } for (const pages of Object.values(session.browserPagesByWorkspace ?? {})) { if (!Array.isArray(pages)) { continue } for (const page of pages) { - addFolderWorkspaceKey(keys, page.worktreeId) + addKey(page.worktreeId) } } for (const record of Object.values(session.sleepingAgentSessionsByPaneKey ?? {})) { - addFolderWorkspaceKey(keys, record.worktreeId) + addKey(record.worktreeId) } return [...keys] } +function hasRestorableWorkspaceChrome(value: unknown): boolean { + if (Array.isArray(value)) { + return value.length > 0 + } + if (isPlainRecord(value)) { + return Object.keys(value).length > 0 + } + return value !== null && value !== undefined && value !== '' +} + +export function collectFolderWorkspaceKeysFromSession( + session: WorkspaceSessionState +): WorkspaceKey[] { + const keys = new Set() + for (const key of collectWorkspaceSessionKeys(session, WORKSPACE_KEYED_SESSION_FIELDS)) { + addFolderWorkspaceKey(keys, key) + } + + return [...keys] +} + +export function collectWorktreeHydrationRepoIdsFromSession( + session: WorkspaceSessionState, + runtimeHostIdByWorkspaceSessionKey?: Record +): string[] { + const repoIds = new Set() + const addWorktreeRepoId = (value: unknown): void => { + if (typeof value !== 'string') { + return + } + const scope = parseWorkspaceKey(value) + if (scope?.type === 'folder') { + return + } + const rawWorktreeId = scope?.type === 'worktree' ? scope.worktreeId : value + const isRuntimeOwned = [value, rawWorktreeId].some( + (key) => parseExecutionHostId(runtimeHostIdByWorkspaceSessionKey?.[key])?.kind === 'runtime' + ) + if (!isRuntimeOwned) { + repoIds.add(getRepoIdFromWorktreeId(rawWorktreeId)) + } + } + + // Why: only chrome-bearing fields — enumerating the unbounded history maps would pull in ~every + // repo ever touched and defeat the selective pre-hydration fetch (they hydrate unfiltered). + for (const key of collectWorkspaceSessionKeys( + session, + WORKSPACE_CHROME_SESSION_FIELDS, + hasRestorableWorkspaceChrome + )) { + addWorktreeRepoId(key) + } + // Why: a repo referenced only by activeRepoId (no active worktree, no tabs) still needs + // enumeration so hydrateWorkspaceSession can restore its main worktree from worktreesByRepo. + addWorktreeRepoId(session.activeRepoId) + + return [...repoIds].filter(Boolean).sort() +} + export function addAdditionalValidWorkspaceKeys( validWorkspaceIds: Set, options?: WorkspaceSessionHydrationOptions diff --git a/src/renderer/src/store/slices/worktree-helpers.ts b/src/renderer/src/store/slices/worktree-helpers.ts index ccf38b5ed60..d1d60ed2ed9 100644 --- a/src/renderer/src/store/slices/worktree-helpers.ts +++ b/src/renderer/src/store/slices/worktree-helpers.ts @@ -22,6 +22,7 @@ import type { } from '../../../../shared/types' import type { WorktreeForceDeleteReason } from '../../../../shared/worktree-removal' import type { TerminalGitHubPRLink } from '../../../../shared/terminal-github-pr-link-detector' +import type { ExecutionHostId } from '../../../../shared/execution-host' import type { PendingWorktreeCreation, WorktreeCreationPhase @@ -116,10 +117,16 @@ export type WorktreeSlice = { * sessions (design §4.4). Session-only; never persisted. */ hasHydratedWorktreePurge: boolean + /** Startup owns the initial all-host refresh; sidebar repo-change refreshes stay gated until it finishes. */ + startupWorktreeRefreshCompleted: boolean fetchDetectedWorktrees: (repoId: string) => Promise fetchWorktrees: ( repoId: string, - options?: { requireAuthoritative?: boolean; forceLocalOwner?: boolean } + options?: { + requireAuthoritative?: boolean + executionHostId?: ExecutionHostId + forceLocalOwner?: boolean + } ) => Promise fetchAllWorktrees: (options?: { hydrationPurge?: 'allow' | 'defer' }) => Promise fetchWorktreeLineage: (options?: { forceLocalOwner?: boolean }) => Promise diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index d436baddf3b..ae5a9fe62ce 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -1350,6 +1350,45 @@ describe('fetchWorktrees', () => { ]) }) + it('fetches the requested host when duplicate repo ids exist', async () => { + const store = createTestStore() + const localWorktree = makeWorktree({ + id: 'same-repo::/local/wt', + repoId: 'same-repo', + path: '/local/wt' + }) + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as never, + repos: [ + { + id: 'same-repo', + path: '/local/repo', + displayName: 'Local', + badgeColor: '#000', + addedAt: 0, + executionHostId: 'local' + }, + { + id: 'same-repo', + path: '/remote/repo', + displayName: 'Runtime', + badgeColor: '#111', + addedAt: 1, + executionHostId: 'runtime:env-1' + } + ] + } as Partial) + mockApi.worktrees.listDetected.mockResolvedValueOnce( + makeDetectedResult('same-repo', [localWorktree]) + ) + + await store.getState().fetchWorktrees('same-repo', { executionHostId: 'local' }) + + expect(mockApi.worktrees.listDetected).toHaveBeenCalledWith({ repoId: 'same-repo' }) + expect(runtimeEnvironmentCall).not.toHaveBeenCalled() + expect(store.getState().worktreesByRepo['same-repo']).toEqual([localWorktree]) + }) + it('stamps remote runtime worktrees with the owning repo runtime host', async () => { const store = createTestStore() // Why: a remote runtime returns worktrees from its own perspective, so their hostId arrives as the default "local". diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index b6609feef82..946955acf9a 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -2287,6 +2287,7 @@ export const createWorktreeSlice: StateCreator everActivatedWorktreeIds: new Set(), lastVisitedAtByWorktreeId: {}, hasHydratedWorktreePurge: false, + startupWorktreeRefreshCompleted: false, fetchDetectedWorktrees: async (repoId) => { try { @@ -2337,7 +2338,9 @@ export const createWorktreeSlice: StateCreator // the local owner without redirecting runtime/SSH-only repos. const useLocalOwner = options?.forceLocalOwner === true && (hasLocalOwner || repoOwners.length === 0) - const hostId = useLocalOwner ? LOCAL_EXECUTION_HOST_ID : repoHostId(ownerState, repoId) + const hostId = useLocalOwner + ? LOCAL_EXECUTION_HOST_ID + : repoHostId(ownerState, repoId, options?.executionHostId) const ownerWasMissingAtStart = repoOwners.length === 0 const setup = getProjectHostSetupForRepoHost(ownerState, repoId, hostId) const ownerSettings = settingsForRepoOwner(ownerState, repoId, hostId) diff --git a/tools/win-update-e2e/app-driver.mjs b/tools/win-update-e2e/app-driver.mjs index 076aa2ceaa5..f730964ad99 100644 --- a/tools/win-update-e2e/app-driver.mjs +++ b/tools/win-update-e2e/app-driver.mjs @@ -237,6 +237,7 @@ export async function waitForTerminalReady(page, timeoutMs = 60_000, terminalTab * workspace (which would mask a broken restore). */ export async function ensureTerminal(page, { allowCreate = true, timeoutMs = 60_000 } = {}) { + await dismissOverlays(page, 1) const visibleTerminal = page.locator(TERMINAL_SURFACE_VISIBLE).first() if (await visibleTerminal.isVisible().catch(() => false)) { await waitForTerminalReady(page, timeoutMs) From ee87bb38dd070cf064d2bc65328fa14354233b66 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:30:49 -0700 Subject: [PATCH 10/10] fix(session): preserve tab/editor/browser chrome when a repo's startup scan degrades (#9991) * fix(session): preserve chrome after degraded repo scan * fix(session): retain terminal-free degraded workspaces --- src/renderer/src/store/slices/browser.ts | 15 +- .../slices/degraded-repo-hydration.test.ts | 211 ++++++++++++++++++ .../slices/degraded-repo-worktree-validity.ts | 75 +++++++ src/renderer/src/store/slices/editor.ts | 9 +- src/renderer/src/store/slices/tabs.ts | 11 +- src/renderer/src/store/slices/terminals.ts | 42 +--- 6 files changed, 314 insertions(+), 49 deletions(-) create mode 100644 src/renderer/src/store/slices/degraded-repo-hydration.test.ts create mode 100644 src/renderer/src/store/slices/degraded-repo-worktree-validity.ts diff --git a/src/renderer/src/store/slices/browser.ts b/src/renderer/src/store/slices/browser.ts index 6afcb24a840..60a71a3eb47 100644 --- a/src/renderer/src/store/slices/browser.ts +++ b/src/renderer/src/store/slices/browser.ts @@ -52,6 +52,7 @@ import { addAdditionalValidWorkspaceKeys, type WorkspaceSessionHydrationOptions } from '@/lib/workspace-session-hydration-keys' +import { buildValidWorktreeIdsForSessionHydration } from './degraded-repo-worktree-validity' type CreateBrowserTabOptions = { activate?: boolean @@ -1582,10 +1583,9 @@ export const createBrowserSlice: StateCreator = hydrateBrowserSession: (session, options) => { const persistedTabsByWorktree = session.browserTabsByWorktree ?? {} const currentState = get() - const validWorktreeIdsForCleanup = new Set( - Object.values(currentState.worktreesByRepo) - .flat() - .map((worktree) => worktree.id) + const validWorktreeIdsForCleanup = buildValidWorktreeIdsForSessionHydration( + currentState, + Object.keys(persistedTabsByWorktree) ) validWorktreeIdsForCleanup.add(FLOATING_TERMINAL_WORKTREE_ID) for (const workspace of currentState.folderWorkspaces) { @@ -1610,10 +1610,9 @@ export const createBrowserSlice: StateCreator = const persistedPagesByWorkspace = session.browserPagesByWorkspace ?? {} const persistedActiveBrowserTabIdByWorktree = session.activeBrowserTabIdByWorktree ?? {} const persistedActiveTabTypeByWorktree = session.activeTabTypeByWorktree ?? {} - const validWorktreeIds = new Set( - Object.values(s.worktreesByRepo) - .flat() - .map((worktree) => worktree.id) + const validWorktreeIds = buildValidWorktreeIdsForSessionHydration( + s, + Object.keys(persistedTabsByWorktree) ) validWorktreeIds.add(FLOATING_TERMINAL_WORKTREE_ID) for (const workspace of s.folderWorkspaces) { diff --git a/src/renderer/src/store/slices/degraded-repo-hydration.test.ts b/src/renderer/src/store/slices/degraded-repo-hydration.test.ts new file mode 100644 index 00000000000..93902bb9d49 --- /dev/null +++ b/src/renderer/src/store/slices/degraded-repo-hydration.test.ts @@ -0,0 +1,211 @@ +import { expect, it, vi } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import type { BrowserTab, WorkspaceSessionState } from '../../../../shared/types' +import { getDefaultWorkspaceSession } from '../../../../shared/constants' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) +vi.mock('@/lib/agent-status', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) } +}) + +// @ts-expect-error -- mocked browser preload API +globalThis.window = { api: {} } + +import { + buildWorkspaceSessionPayload, + shouldPersistWorkspaceSession +} from '@/lib/workspace-session' +import { createTestStore, makeTab } from './store-test-helpers' + +const WORKTREE_ID = 'repo1::/path/degraded' +const TERMINAL_ID = 'terminal-degraded' +const EDITOR_FILE_ID = '/path/degraded/src/App.tsx' +const BROWSER_ID = 'browser-degraded' +const GROUP_ID = 'group-degraded' + +function makeBrowserTab(): BrowserTab { + return { + id: BROWSER_ID, + worktreeId: WORKTREE_ID, + url: 'https://example.com', + title: 'Example', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 3 + } +} + +function makeDegradedRepoSession(): WorkspaceSessionState { + return { + ...getDefaultWorkspaceSession(), + activeRepoId: 'repo1', + activeWorktreeId: WORKTREE_ID, + activeTabId: TERMINAL_ID, + tabsByWorktree: { + [WORKTREE_ID]: [makeTab({ id: TERMINAL_ID, worktreeId: WORKTREE_ID })] + }, + openFilesByWorktree: { + [WORKTREE_ID]: [ + { + filePath: EDITOR_FILE_ID, + relativePath: 'src/App.tsx', + worktreeId: WORKTREE_ID, + language: 'typescript' + } + ] + }, + activeFileIdByWorktree: { [WORKTREE_ID]: EDITOR_FILE_ID }, + browserTabsByWorktree: { [WORKTREE_ID]: [makeBrowserTab()] }, + activeBrowserTabIdByWorktree: { [WORKTREE_ID]: BROWSER_ID }, + activeTabTypeByWorktree: { [WORKTREE_ID]: 'browser' }, + unifiedTabs: { + [WORKTREE_ID]: [ + { + id: TERMINAL_ID, + entityId: TERMINAL_ID, + groupId: GROUP_ID, + worktreeId: WORKTREE_ID, + contentType: 'terminal', + label: 'Terminal', + customLabel: null, + color: null, + sortOrder: 0, + createdAt: 1 + }, + { + id: EDITOR_FILE_ID, + entityId: EDITOR_FILE_ID, + groupId: GROUP_ID, + worktreeId: WORKTREE_ID, + contentType: 'editor', + label: 'App.tsx', + customLabel: null, + color: null, + sortOrder: 1, + createdAt: 2 + }, + { + id: BROWSER_ID, + entityId: BROWSER_ID, + groupId: GROUP_ID, + worktreeId: WORKTREE_ID, + contentType: 'browser', + label: 'Example', + customLabel: null, + color: null, + sortOrder: 2, + createdAt: 3 + } + ] + }, + tabGroups: { + [WORKTREE_ID]: [ + { + id: GROUP_ID, + worktreeId: WORKTREE_ID, + activeTabId: BROWSER_ID, + tabOrder: [TERMINAL_ID, EDITOR_FILE_ID, BROWSER_ID], + recentTabIds: [TERMINAL_ID, EDITOR_FILE_ID, BROWSER_ID] + } + ] + }, + activeGroupIdByWorktree: { [WORKTREE_ID]: GROUP_ID } + } +} + +function makeTerminalFreeDegradedRepoSession(): WorkspaceSessionState { + const session = makeDegradedRepoSession() + session.activeTabId = null + session.tabsByWorktree = {} + session.unifiedTabs![WORKTREE_ID] = session.unifiedTabs![WORKTREE_ID].filter( + (tab) => tab.contentType !== 'terminal' + ) + session.tabGroups![WORKTREE_ID] = session.tabGroups![WORKTREE_ID].map((group) => ({ + ...group, + tabOrder: [EDITOR_FILE_ID, BROWSER_ID], + recentTabIds: [EDITOR_FILE_ID, BROWSER_ID] + })) + return session +} + +function hydrateWithRepoScan( + store: ReturnType, + session: WorkspaceSessionState, + authoritative = false +): void { + store.setState({ + repos: [{ id: 'repo1', path: '/repo1', displayName: 'Repo 1', badgeColor: '#000', addedAt: 0 }], + worktreesByRepo: { repo1: [] }, + detectedWorktreesByRepo: { + repo1: { + repoId: 'repo1', + authoritative, + source: authoritative ? 'git' : 'metadata-fallback', + worktrees: [] + } + } + }) + store.getState().hydrateWorkspaceSession(session) + store.getState().hydrateTabsSession(session) + store.getState().hydrateEditorSession(session) + store.getState().hydrateBrowserSession(session) +} + +it('keeps tab, editor, and browser chrome through degraded hydration and persistence', () => { + const firstStore = createTestStore() + hydrateWithRepoScan(firstStore, makeDegradedRepoSession()) + firstStore.setState({ workspaceSessionReady: true }) + firstStore.getState().setHydrationSucceeded(true) + expect(shouldPersistWorkspaceSession(firstStore.getState())).toBe(true) + + const persisted = buildWorkspaceSessionPayload(firstStore.getState()) + const restoredStore = createTestStore() + hydrateWithRepoScan(restoredStore, persisted) + + const restored = restoredStore.getState() + expect(restored.unifiedTabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + TERMINAL_ID, + EDITOR_FILE_ID, + BROWSER_ID + ]) + expect(restored.openFiles.map((file) => file.id)).toEqual([EDITOR_FILE_ID]) + expect(restored.browserTabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([BROWSER_ID]) +}) + +it('keeps a terminal-free degraded workspace selected through hydration and persistence', () => { + const firstStore = createTestStore() + hydrateWithRepoScan(firstStore, makeTerminalFreeDegradedRepoSession()) + + const first = firstStore.getState() + expect(first.activeWorktreeId).toBe(WORKTREE_ID) + expect(first.activeTabType).toBe('browser') + + firstStore.setState({ workspaceSessionReady: true }) + firstStore.getState().setHydrationSucceeded(true) + const persisted = buildWorkspaceSessionPayload(firstStore.getState()) + const restoredStore = createTestStore() + hydrateWithRepoScan(restoredStore, persisted) + + const restored = restoredStore.getState() + expect(restored.activeWorktreeId).toBe(WORKTREE_ID) + expect(restored.unifiedTabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + EDITOR_FILE_ID, + BROWSER_ID + ]) + expect(restored.activeTabType).toBe('browser') +}) + +it('drops terminal-free chrome when an authoritative scan proves deletion', () => { + const store = createTestStore() + hydrateWithRepoScan(store, makeTerminalFreeDegradedRepoSession(), true) + + const state = store.getState() + expect(state.activeWorktreeId).toBeNull() + expect(state.unifiedTabsByWorktree[WORKTREE_ID]).toBeUndefined() + expect(state.openFiles).toEqual([]) + expect(state.browserTabsByWorktree[WORKTREE_ID]).toBeUndefined() +}) diff --git a/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts b/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts new file mode 100644 index 00000000000..0fb4e42fd19 --- /dev/null +++ b/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts @@ -0,0 +1,75 @@ +import type { + DetectedWorktreeListResult, + Repo, + WorkspaceSessionState, + Worktree +} from '../../../../shared/types' +import { parseWorkspaceKey } from '../../../../shared/workspace-scope' +import { getRepoIdFromWorktreeId } from '../../../../shared/worktree-id' + +type WorktreeValidityCatalog = { + repos: readonly Pick[] + worktreesByRepo: Readonly[]>> + detectedWorktreesByRepo?: Readonly< + Record | undefined> + > +} + +export function collectPersistedWorktreeIdsForSessionHydration( + session: WorkspaceSessionState +): Set { + const persistedWorktreeIds = new Set() + for (const worktreeId of Object.keys(session.tabsByWorktree)) { + persistedWorktreeIds.add(worktreeId) + } + for (const worktreeId of Object.keys(session.unifiedTabs ?? {})) { + persistedWorktreeIds.add(worktreeId) + } + for (const worktreeId of Object.keys(session.openFilesByWorktree ?? {})) { + persistedWorktreeIds.add(worktreeId) + } + for (const worktreeId of Object.keys(session.browserTabsByWorktree ?? {})) { + persistedWorktreeIds.add(worktreeId) + } + return persistedWorktreeIds +} + +export function buildValidWorktreeIdsForSessionHydration( + catalog: WorktreeValidityCatalog, + persistedWorktreeIds: Iterable +): Set { + const worktreesByRepo = catalog.worktreesByRepo + const validWorktreeIds = new Set( + Object.values(worktreesByRepo) + .flat() + .map((worktree) => worktree.id) + ) + const knownRepoIds = new Set(catalog.repos.map((repo) => repo.id)) + const repoIdsWithLoadedWorktrees = new Set( + Object.entries(worktreesByRepo) + .filter(([, worktrees]) => worktrees.length > 0) + .map(([repoId]) => repoId) + ) + const repoIdsWithAuthoritativeDetectedWorktrees = new Set( + Object.entries(catalog.detectedWorktreesByRepo ?? {}) + .filter(([, detected]) => detected?.authoritative) + .map(([repoId]) => repoId) + ) + + for (const worktreeId of persistedWorktreeIds) { + if (validWorktreeIds.has(worktreeId) || parseWorkspaceKey(worktreeId)?.type === 'folder') { + continue + } + const repoId = getRepoIdFromWorktreeId(worktreeId) + // Why (#1158): a failed scan cannot prove deletion, while loaded worktrees or an authoritative scan can. + if ( + knownRepoIds.has(repoId) && + !repoIdsWithLoadedWorktrees.has(repoId) && + !repoIdsWithAuthoritativeDetectedWorktrees.has(repoId) + ) { + validWorktreeIds.add(worktreeId) + } + } + + return validWorktreeIds +} diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index b7ca9685716..86887623f61 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -69,6 +69,7 @@ import { addAdditionalValidWorkspaceKeys, type WorkspaceSessionHydrationOptions } from '@/lib/workspace-session-hydration-keys' +import { buildValidWorktreeIdsForSessionHydration } from './degraded-repo-worktree-validity' import { createUntitledMarkdownFileWithTemplateSelection } from '@/lib/create-untitled-markdown' import { extractIpcErrorMessage } from '@/lib/ipc-error' import { translate } from '@/i18n/i18n' @@ -4407,11 +4408,9 @@ export const createEditorSlice: StateCreator = (s const persistedActiveTabTypeByWorktree = session.activeTabTypeByWorktree ?? {} const persistedMarkdownFrontmatterVisible = session.markdownFrontmatterVisible ?? {} - // Why: worktrees may have been deleted between sessions; drop files for worktrees that no longer exist. - const validWorktreeIds = new Set( - Object.values(s.worktreesByRepo) - .flat() - .map((w) => w.id) + const validWorktreeIds = buildValidWorktreeIdsForSessionHydration( + s, + Object.keys(openFilesByWorktree) ) validWorktreeIds.add(FLOATING_TERMINAL_WORKTREE_ID) for (const workspace of s.folderWorkspaces) { diff --git a/src/renderer/src/store/slices/tabs.ts b/src/renderer/src/store/slices/tabs.ts index 337f45515df..fefbecd5363 100644 --- a/src/renderer/src/store/slices/tabs.ts +++ b/src/renderer/src/store/slices/tabs.ts @@ -40,6 +40,10 @@ import { addAdditionalValidWorkspaceKeys, type WorkspaceSessionHydrationOptions } from '@/lib/workspace-session-hydration-keys' +import { + buildValidWorktreeIdsForSessionHydration, + collectPersistedWorktreeIdsForSessionHydration +} from './degraded-repo-worktree-validity' export type TabSplitDirection = 'left' | 'right' | 'up' | 'down' @@ -1971,11 +1975,8 @@ export const createTabsSlice: StateCreator = (set, hydrateTabsSession: (session, options) => { const state = get() - const validWorktreeIds = new Set( - Object.values(state.worktreesByRepo) - .flat() - .map((w) => w.id) - ) + const persistedWorktreeIds = collectPersistedWorktreeIdsForSessionHydration(session) + const validWorktreeIds = buildValidWorktreeIdsForSessionHydration(state, persistedWorktreeIds) validWorktreeIds.add(FLOATING_TERMINAL_WORKTREE_ID) for (const workspace of state.folderWorkspaces) { validWorktreeIds.add(folderWorkspaceKey(workspace.id)) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 97d56c5a9db..4e857129adf 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -104,6 +104,10 @@ import { addAdditionalValidWorkspaceKeys, type WorkspaceSessionHydrationOptions } from '@/lib/workspace-session-hydration-keys' +import { + buildValidWorktreeIdsForSessionHydration, + collectPersistedWorktreeIdsForSessionHydration +} from './degraded-repo-worktree-validity' import { collectHibernatedCompletionEvidenceForWorktree, collectSleepingAgentSessionRecordsForWorktree, @@ -3074,45 +3078,21 @@ export const createTerminalSlice: StateCreator runtimeHostIdByWorkspaceSessionKey: options?.runtimeHostIdByWorkspaceSessionKey ?? {}, worktreesByRepo: s.worktreesByRepo }) - const validWorktreeIds = new Set( - Object.values(runtimeSessionPlaceholders.worktreesByRepo) - .flat() - .map((worktree) => worktree.id) + const validWorktreeIds = buildValidWorktreeIdsForSessionHydration( + { + repos: runtimeSessionPlaceholders.repos, + worktreesByRepo: runtimeSessionPlaceholders.worktreesByRepo, + detectedWorktreesByRepo: s.detectedWorktreesByRepo + }, + collectPersistedWorktreeIdsForSessionHydration(session) ) const knownRepoIds = new Set(runtimeSessionPlaceholders.repos.map((r) => r.id)) - const repoIdsWithLoadedWorktrees = new Set( - Object.entries(runtimeSessionPlaceholders.worktreesByRepo) - .filter(([, worktrees]) => worktrees.length > 0) - .map(([repoId]) => repoId) - ) - const repoIdsWithAuthoritativeDetectedWorktrees = new Set( - Object.entries(s.detectedWorktreesByRepo) - .filter(([, detected]) => detected.authoritative) - .map(([repoId]) => repoId) - ) // Why: the Floating Workspace isn't a repo worktree, but its tabs use the normal session pipeline so daemon PTYs survive app restart. validWorktreeIds.add(FLOATING_TERMINAL_WORKTREE_ID) for (const workspace of s.folderWorkspaces) { validWorktreeIds.add(folderWorkspaceKey(workspace.id)) } addAdditionalValidWorkspaceKeys(validWorktreeIds, options) - for (const worktreeId of Object.keys(session.tabsByWorktree)) { - const parsedWorkspaceKey = parseWorkspaceKey(worktreeId) - if (parsedWorkspaceKey?.type === 'folder') { - continue - } - if (!validWorktreeIds.has(worktreeId)) { - const repoId = getRepoIdFromWorktreeId(worktreeId) - // Why (#1158): an empty/missing list can mean degraded hydration; a non-empty repo list is authoritative for deleted-worktree cleanup. - if ( - knownRepoIds.has(repoId) && - !repoIdsWithLoadedWorktrees.has(repoId) && - !repoIdsWithAuthoritativeDetectedWorktrees.has(repoId) - ) { - validWorktreeIds.add(worktreeId) - } - } - } // Why pendingActivationSpawn: a restored worktree's first mount calls updateTabPtyId, which would bump lastActivityAt and bounce it to the top of Recent; the tag (consumed on the first pty update) suppresses that so only real activity bumps. const tabsByWorktree: Record = Object.fromEntries( Object.entries(session.tabsByWorktree)