From 836635ebefb7b5bb44d32a884429bd2c223e4b64 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 3 Jul 2026 22:10:58 -0700 Subject: [PATCH] cleanup(main): defer serve-sim materialization, dedupe reload flags, guard gate validators Quality pass on main-process/build PRs merged 2026-07-03: - ios-emulator-backend: resolve the serve-sim executable via a lazily-cached getter instead of eagerly in the constructor. The bridge is built before the main window is shown, so the one-time recursive copy + xattr subprocess (first launch after each version bump) no longer blocks macOS startup for a feature that may go unused (#7174). - index: collapse the two near-identical `{webContentsId, until}` reload flags (expectedRendererReload / recoveryReloadInFlight) into one `createWebContentsTimedFlag` primitive; behavior preserved, including consume-on-read for the recovery reload (#7290). - check-reliability-gates: coerce gate.commands/testFiles/platforms/providers with an `asArray` helper before `.includes`, so a hand-edited manifest with a missing/mistyped field reports a validation failure instead of throwing an uncaught TypeError; extract `hasCompleteRedGreenEvidence` for the duplicated status check (#7295). - claude-pty: derive FABLE_WEEKLY_LABEL_RE from WEEKLY_RE.source so a future weekly- wording change stays in one place and can't reopen the parsing gap it just closed. - macos-tcc-login-shell: trim the 30-line flag-by-flag JSDoc to the two non-obvious whys (TCC identity, env(1) SHELL re-assertion) per the repo comment guidance (#7003). Typecheck, oxlint, oxfmt, `check:reliability-gates`, and touched unit suites all pass. --- config/scripts/check-reliability-gates.mjs | 23 ++++-- .../emulator/backends/ios-emulator-backend.ts | 12 ++- src/main/index.ts | 76 +++++++++++-------- src/main/providers/macos-tcc-login-shell.ts | 34 ++------- src/main/rate-limits/claude-pty.ts | 8 +- 5 files changed, 87 insertions(+), 66 deletions(-) diff --git a/config/scripts/check-reliability-gates.mjs b/config/scripts/check-reliability-gates.mjs index 79ca2e27d8b..8cf61c5cb2b 100644 --- a/config/scripts/check-reliability-gates.mjs +++ b/config/scripts/check-reliability-gates.mjs @@ -153,7 +153,7 @@ function validateProtection(gate, failures) { if (gate.flakeHistory?.status !== 'stable') { failures.push(`${gate.id}: protection active gates must have stable flakeHistory`) } - if (!['complete', 'not-required'].includes(gate.redGreenEvidence?.status)) { + if (!hasCompleteRedGreenEvidence(gate)) { failures.push(`${gate.id}: protection active gates must have complete red/green evidence`) } } @@ -166,6 +166,17 @@ function isIsoDate(value) { return typeof value === 'string' && /^\d{4}-\d{2}-\d{2}$/.test(value) } +// Why: cross-field validators run even after an earlier type check flagged a +// malformed field, so coerce to an array before `.includes` to report the error +// instead of throwing on hand-edited manifests. +function asArray(value) { + return Array.isArray(value) ? value : [] +} + +function hasCompleteRedGreenEvidence(gate) { + return ['complete', 'not-required'].includes(gate.redGreenEvidence?.status) +} + function validateEvidenceRun(gate, run, index, failures) { const owner = `${gate.id}: evidenceRuns[${index}]` if (!isRecord(run)) { @@ -186,7 +197,7 @@ function validateEvidenceRun(gate, run, index, failures) { } if (!isNonEmptyString(run.command)) { failures.push(`${owner}.command must be a non-empty string`) - } else if (!gate.commands.includes(run.command)) { + } else if (!asArray(gate.commands).includes(run.command)) { failures.push(`${owner}.command must match one of the gate commands`) } if (!Number.isFinite(run.durationSeconds) || run.durationSeconds < 0) { @@ -222,7 +233,7 @@ function validateAssertionRef(gate, ref, index, failures) { } if (!isNonEmptyString(ref.file)) { failures.push(`${owner}.file must be a non-empty string`) - } else if (!gate.testFiles.includes(ref.file)) { + } else if (!asArray(gate.testFiles).includes(ref.file)) { failures.push(`${owner}.file must be one of the gate testFiles`) } if (!hasNonEmptyStringArray(ref.assertions)) { @@ -253,12 +264,12 @@ function validateCoveredScope(gate, failures) { return } for (const platform of gate.coveredPlatforms) { - if (!gate.platforms.includes(platform)) { + if (!asArray(gate.platforms).includes(platform)) { failures.push(`${gate.id}: covered platform is outside risk scope: ${platform}`) } } for (const provider of gate.coveredProviders) { - if (!gate.providers.includes(provider)) { + if (!asArray(gate.providers).includes(provider)) { failures.push(`${gate.id}: covered provider is outside risk scope: ${provider}`) } } @@ -343,7 +354,7 @@ async function validateGate(gate, maturities, root) { if (!['soaking', 'stable'].includes(gate.flakeHistory?.status)) { failures.push(`${gate.id}: ${gate.maturity} gates must have soaking or stable flakeHistory`) } - if (!['complete', 'not-required'].includes(gate.redGreenEvidence?.status)) { + if (!hasCompleteRedGreenEvidence(gate)) { failures.push(`${gate.id}: ${gate.maturity} gates must have complete red/green evidence`) } } diff --git a/src/main/emulator/backends/ios-emulator-backend.ts b/src/main/emulator/backends/ios-emulator-backend.ts index 8d3c31604e4..a0828c75898 100644 --- a/src/main/emulator/backends/ios-emulator-backend.ts +++ b/src/main/emulator/backends/ios-emulator-backend.ts @@ -46,14 +46,22 @@ export class IosEmulatorBackend implements EmulatorBackend { logcat: false } - private readonly serveSimExecutable: ServeSimExecutable + private cachedServeSimExecutable: ServeSimExecutable | undefined private readonly waitForEndpointReady: (endpoint: string) => Promise constructor(options: EmulatorBridgeOptions = {}) { - this.serveSimExecutable = resolveServeSimExecutable() this.waitForEndpointReady = options.waitForEndpointReady ?? waitForServeSimEndpointReady } + // Why: resolving the executable can materialize the serve-sim runtime (a one-time + // recursive copy + xattr subprocess on macOS after each version bump). Defer it + // off the startup path — the bridge is constructed before the main window shows — + // so it only runs when an emulator command is actually issued. + private get serveSimExecutable(): ServeSimExecutable { + this.cachedServeSimExecutable ??= resolveServeSimExecutable() + return this.cachedServeSimExecutable + } + isSupportedOnHost(): boolean { return platform() === 'darwin' } diff --git a/src/main/index.ts b/src/main/index.ts index 37e1d6d471e..9537dc98b38 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -209,10 +209,11 @@ let watcherShutdownPromise: Promise | null = null let watcherShutdownDone = false let automations: AutomationService | null = null let keybindings: KeybindingService | null = null -let expectedRendererReload: { webContentsId: number; until: number } | null = null -// Why: the crash/freeze-recovery reload re-fires did-finish-load; flag it so the -// local-PTY orphan sweep spares live sessions across that one reload (#5787). -let recoveryReloadInFlight: { webContentsId: number; until: number } | null = null +// Why: a reload/teardown intent set for one renderer must not leak to a later load. +// The recovery reload re-fires did-finish-load, so its flag lets the local-PTY orphan +// sweep spare live sessions across that one reload (#5787). +const expectedRendererReload = createWebContentsTimedFlag() +const recoveryReloadInFlight = createWebContentsTimedFlag() let firstWindowStartupServicesReady: Promise = Promise.resolve() // Why: GPU child crashes clustered right after launch indicate a broken driver; // track them so Orca can move this build onto software rendering. @@ -435,51 +436,66 @@ function focusExistingWindow(): void { }) } +// Why: a webContents-scoped flag that auto-expires so an intent set for one renderer +// can't leak to a later load. `consume` clears on a positive match for one-shot +// signals (the recovery reload fires exactly one did-finish-load). +function createWebContentsTimedFlag(defaultDurationMs = 10_000): { + mark: (webContentsId: number, durationMs?: number) => void + clear: (webContentsId?: number) => void + matches: (webContentsId: number, options?: { consume?: boolean }) => boolean +} { + let state: { webContentsId: number; until: number } | null = null + return { + mark(webContentsId, durationMs = defaultDurationMs) { + state = { webContentsId, until: Date.now() + durationMs } + }, + clear(webContentsId) { + if (webContentsId === undefined || state?.webContentsId === webContentsId) { + state = null + } + }, + matches(webContentsId, options) { + if (!state || Date.now() > state.until) { + state = null + return false + } + if (state.webContentsId !== webContentsId) { + return false + } + if (options?.consume) { + state = null + } + return true + } + } +} + function markExpectedRendererReload(webContentsId: number, durationMs = 10_000): void { - expectedRendererReload = { webContentsId, until: Date.now() + durationMs } + expectedRendererReload.mark(webContentsId, durationMs) } function clearExpectedRendererReload(webContentsId?: number): void { - if (webContentsId === undefined || expectedRendererReload?.webContentsId === webContentsId) { - expectedRendererReload = null - } + expectedRendererReload.clear(webContentsId) } function getExpectedTeardownScope(webContentsId?: number): ExpectedTeardownScope { if (isQuitting || isQuittingForUpdate()) { return 'app-shutdown' } - if (!expectedRendererReload) { + if (webContentsId === undefined) { return 'none' } - if (Date.now() > expectedRendererReload.until) { - expectedRendererReload = null - return 'none' - } - return webContentsId !== undefined && expectedRendererReload.webContentsId === webContentsId - ? 'renderer-reload' - : 'none' + return expectedRendererReload.matches(webContentsId) ? 'renderer-reload' : 'none' } function markRecoveryReloadInFlight(webContentsId: number, durationMs = 10_000): void { - recoveryReloadInFlight = { webContentsId, until: Date.now() + durationMs } + recoveryReloadInFlight.mark(webContentsId, durationMs) } function isRecoveryReloadInFlight(webContentsId: number): boolean { - if (!recoveryReloadInFlight) { - return false - } - if (Date.now() > recoveryReloadInFlight.until) { - recoveryReloadInFlight = null - return false - } - if (recoveryReloadInFlight.webContentsId !== webContentsId) { - return false - } // Why: consume on read — the recovery reload fires exactly one did-finish-load, - // so clearing here keeps a later genuine reload sweeping orphaned local PTYs. - recoveryReloadInFlight = null - return true + // so a later genuine reload still sweeps orphaned local PTYs. + return recoveryReloadInFlight.matches(webContentsId, { consume: true }) } function recordAgentStateCrashBreadcrumb(agentType: string, state: string): void { diff --git a/src/main/providers/macos-tcc-login-shell.ts b/src/main/providers/macos-tcc-login-shell.ts index 283f8b867c2..806b6669f68 100644 --- a/src/main/providers/macos-tcc-login-shell.ts +++ b/src/main/providers/macos-tcc-login-shell.ts @@ -17,34 +17,16 @@ function isDisabledByEnv(): boolean { } /** - * Wrap a macOS POSIX shell spawn in `/usr/bin/login` so terminal children carry - * their own TCC identity instead of collapsing into Orca's bundle identifier. + * Wrap a macOS shell spawn in `/usr/bin/login -flpq …` so terminal children + * get their own TCC identity instead of collapsing into Orca's bundle id — signed + * CLIs like `op` otherwise re-prompt every launch because tccd attributes the grant + * to Orca and never persists it (#6996). This mirrors how Terminal.app spawns shells. * - * Why: when Orca spawns a shell directly, macOS attributes a spawned CLI's - * "access other apps' data" request (kTCCServiceSystemPolicyAppData) to Orca's - * bundle id and never persists the grant, so signed CLIs like `op` re-prompt on - * every launch (#6996). Native terminals (Terminal.app and others) launch shells - * through login(1), which lets tccd resolve each child's own code identity and - * remember the decision. This matches that spawn shape without a native patch. + * Why the env(1) interposition: login(1) overwrites SHELL from the account DB even + * under -p, so `/usr/bin/env SHELL=` re-asserts the shell Orca actually runs + * without disturbing login's attribution (skipped when the shell path contains `=`). * - * Flags: -f (skip auth; we are the logged-in user relaunching as ourselves), - * -l (do not chdir to home — node-pty already set cwd — and skip the login - * dash-argv0 marker), -p (preserve Orca's env, including its ZDOTDIR shell - * integration), -q (suppress the login banner so wrapped terminals look - * unchanged). The underlying shell keeps its own args (e.g. zsh's `-l`) so - * login-shell behavior is unchanged. - * - * SHELL: even under -p, login(1) overwrites SHELL with the account shell from - * the user database, while Orca terminals deliberately export the shell they - * actually run (fallback shells, custom shell settings). Interposing - * `/usr/bin/env SHELL=` between login and the shell re-asserts the - * intended value — and, as a same-process exec, does not disturb login's TCC - * attribution. Skipped only if the shell path itself contains `=`, which env(1) - * would misparse as an assignment. - * - * No-op off macOS, when already wrapped, when the login binary or username is - * unavailable, or when disabled via {@link DISABLE_ENV_VAR}, so terminal - * spawning never regresses. + * No-op off macOS, when already wrapped, or when disabled via {@link DISABLE_ENV_VAR}. */ export function wrapShellSpawnForMacosTccAttribution( file: string, diff --git a/src/main/rate-limits/claude-pty.ts b/src/main/rate-limits/claude-pty.ts index 419f7a1d200..f2000489275 100644 --- a/src/main/rate-limits/claude-pty.ts +++ b/src/main/rate-limits/claude-pty.ts @@ -24,8 +24,12 @@ const SESSION_RE = /current\s*session/i const WEEKLY_RE = /(?:current\s*week|weekly\s*(?:limits?|usage|rate\s*limits?)|7\s*[- ]?\s*day)/i const FABLE_WORD_RE = /\bfable\b/i const FABLE_LABEL_RE = /^\s*fable\s*$/i -const FABLE_WEEKLY_LABEL_RE = - /(?:current\s*week|weekly\s*(?:limits?|usage|rate\s*limits?)|7\s*[- ]?\s*day)\s*(?:\([^)]*\bfable\b[^)]*\)|[-:]?\s*\bfable\b)/i +// Why: derive from WEEKLY_RE so a future weekly-wording change stays in one place +// instead of silently reopening the Fable-weekly parsing gap this fix closed. +const FABLE_WEEKLY_LABEL_RE = new RegExp( + `${WEEKLY_RE.source}\\s*(?:\\([^)]*\\bfable\\b[^)]*\\)|[-:]?\\s*\\bfable\\b)`, + 'i' +) const PERCENT_RE = /(\d{1,3})(?:\.\d+)?\s*%\s*(used|consumed|left|remaining|available)/i const ESC = String.fromCharCode(27) const BEL = String.fromCharCode(7)