From cf20e089d20fd59a29479fac42dc45eabb666de3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 13 Sep 2026 14:26:27 -0700 Subject: [PATCH] fix(native-chat): wait for the runtime capability probe before resolving the creation launch route (#19819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(native-chat): wait for the runtime capability probe before resolving the launch route A worktree created before the renderer's hydration-gated capability refresh runs read the local capability set as null, which resolveStructuredNativeChatSupport treats as a blocker, silently degrading structured native chat to the legacy terminal-backed route. Creation submits now await ensureLocalRuntimeCapabilities(), which probes the local runtime when no answer has landed yet, so the route resolves on an actual answer. Fixes #19154 * fix(native-chat): await the capability probe in the work-item direct launch route too prepareDirectWorkItemAgentLaunch is the fourth creation-flow route owner and already async; a pre-hydration submit-after-ready launch (fix-checks) read the unprobed cache as unsupported and silently degraded to legacy. Draft-delivery launches were unaffected (draft-prompt blocks structured before the capability check). Same shape as the three creation-submit sites. * fix(native-chat): keep the capability probe starting synchronously The broken-bridge hardening wrapped the probe in Promise.resolve().then(...), which deferred window.api.runtime.getStatus() by a microtask. The session-tabs restore deliberately overlaps its inventory RPC with this refresh and relies on the probe already being in flight when refresh returns, so the deferral broke it. The bridge call is synchronous again; a synchronous throw becomes a rejection instead, which is what the wrapper was actually for. * fix(native-chat): hydrate local runtime capabilities at renderer boot The capability cache's only writer was `useLocalStructuredSessionTabsSync`, gated on workspaceSessionReady + terminalStartupRestorationReady + the experimental flag. Every `resolveAgentLaunchRoute` reader treats an unanswered cache as "unsupported", so the answer arriving seconds late is what produces the bare-terminal create in #19154 — awaiting the probe at a route decision guards four call sites but leaves the window open for the three readers that are synchronous and cannot await. Start the probe from the renderer boot chain, ungated, so the answer is cached before any launch route is resolved. The per-call-site awaits stay as the backstop for the residual window and for re-probing after a failed probe. Also: hoist the full-creation probe above its cancel gate so the gate stays adjacent to createWorktree; pin the retry-after-failure, concurrent-ensure and missing-bridge contracts; drop a stale microtask tick and correct two comments that no longer described the code. * test(native-chat): pin the cancel gate around the capability probe The probe added an await to two composer creation paths. Full creation had no gate between the route decision and createWorktree, so the earlier revision opened a window where a dismissed composer still created a worktree; the hoist that closed it was unpinned. Quick creation already gated immediately before runBackgroundWorktreeCreation, so its inline await is safe — pin that too, since nothing asserted it. Both tests fail against origin/main (no probe) and the full-creation one fails against the pre-hoist revision. * fix(native-chat): close the folder-create cancel window the probe opened The probe added the first `await` inside `submitFolderWorkspaceCreate`. On `main` that function ran straight through to `createFolderWorkspace` with no suspension of its own, so its caller's `isSubmissionCancelled()` gate and the create call sat in the same turn. With the probe inline, a composer dismissed while the probe is in flight still creates the folder workspace and launches an agent — the same defect the full-creation hoist fixed on the git path. Resolve capabilities in `folder-submit-orchestration` above its existing gate and hand them down, so the create path's prefix is synchronous again. The parameter stays optional: a caller without a cancel gate keeps the probe. Both new tests fail against `origin/main` and against this branch's previous head; the cancel-window one still fails with its probe-pending assertion removed, so it pins the create, not just the probe. * refactor(native-chat): require pre-resolved capabilities on the folder create path The cancel-window fix in f492064432 left its invariant -- a caller that gates on cancellation must resolve capabilities above its gate -- enforced only by a comment, because `hostCapabilities` stayed optional with an inline probe as the fallback. A future caller that owns a cancel gate and forgets the parameter would silently reopen the window twice fixed already, and nothing would catch it: the caller census test pins `resolveAgentLaunchRoute` callers, not this function's, and `exactOptionalPropertyTypes` is off so even an explicit `undefined` is legal. Make it required and drop the now-unreachable inline probe. The sole production caller already passes it, so runtime behaviour is unchanged: the old ternary never evaluated its `await` when a value was supplied. `null` keeps its meaning -- probed, genuinely unknown -- and still degrades to the legacy route; only absence becomes impossible. The launch-route test that covered the removed probe is replaced by one pinning that `null` contract with the cache and the bridge both holding the structured capability, so only the handed-in value can produce the legacy outcome. The cases in the sibling suite are not about the route, so they go through one typed wrapper that supplies the unknown answer rather than repeating it 21 times. --------- Co-authored-by: Merge Sim --- .../app-shell/use-app-startup-hydration.ts | 7 +++ src/renderer/src/app-startup-routing.test.ts | 18 +++++++ .../local-runtime-capabilities.test.ts | 51 +++++++++++++++++++ .../src/runtime/local-runtime-capabilities.ts | 36 +++++++++++-- 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/renderer/src/app-shell/use-app-startup-hydration.ts b/src/renderer/src/app-shell/use-app-startup-hydration.ts index 77da19ffd20..a2fd133ae32 100644 --- a/src/renderer/src/app-shell/use-app-startup-hydration.ts +++ b/src/renderer/src/app-shell/use-app-startup-hydration.ts @@ -36,6 +36,7 @@ import { import { mapWithConcurrency } from '../../../shared/map-with-concurrency' import type { OnboardingState } from '../../../shared/onboarding-state-types' import { restoreLocalStructuredSessionTabsOnce } from '../runtime/local-structured-session-tabs-sync' +import { ensureLocalRuntimeCapabilities } from '../runtime/local-runtime-capabilities' async function listRuntimeSessionHostIdsForStartup(): Promise { try { @@ -67,6 +68,12 @@ export function useAppStartupHydration(onOnboardingLoaded: (state: OnboardingSta // Fetch initial data + hydrate GitHub cache from disk useEffect(() => { + // Why first and ungated: the local capability set is a static fact the main process can answer + // immediately, but its only other writer is the structured-session-tabs sync, which waits for + // workspaceSessionReady + terminalStartupRestorationReady + the experimental flag. Every + // `resolveAgentLaunchRoute` reader treats "not asked yet" as "unsupported", so leaving the + // answer behind those gates degrades a pre-hydration create to a bare terminal (#19154). + void ensureLocalRuntimeCapabilities() let cancelled = false // Why: declared outside the async block so cleanup can abort it — under StrictMode the first (unmounted) pass would otherwise keep spawning PTYs. const abortController = new AbortController() diff --git a/src/renderer/src/app-startup-routing.test.ts b/src/renderer/src/app-startup-routing.test.ts index fead2f6c7bb..4c6ab94f010 100644 --- a/src/renderer/src/app-startup-routing.test.ts +++ b/src/renderer/src/app-startup-routing.test.ts @@ -367,6 +367,24 @@ describe('renderer startup runtime routing', () => { ) }) + it('probes local runtime capabilities before any startup gate can hold the answer back', () => { + const source = readSource(STARTUP_HYDRATION_PATH) + const probeIndex = source.indexOf('void ensureLocalRuntimeCapabilities()') + const chainStart = source.indexOf('void (async () => {') + const effectStart = source.lastIndexOf('useEffect(() => {', probeIndex) + + expect(probeIndex).toBeGreaterThanOrEqual(0) + // Why pinned here: the structured-session-tabs sync is the cache's only other writer and it + // waits for workspaceSessionReady + terminalStartupRestorationReady + the experimental flag. + // Every resolveAgentLaunchRoute reader — including the three that cannot await — reads an + // unanswered cache as "unsupported", so a create in that window degrades to a bare + // terminal (#19154). The probe must therefore start before the chain and outside its gates. + expect(probeIndex).toBeLessThan(chainStart) + expect(probeIndex).toBeLessThan(source.indexOf('await ', effectStart)) + expect(source.slice(effectStart, probeIndex)).not.toContain('if (') + expect(source.slice(effectStart, probeIndex)).not.toContain('experimentalStructuredNativeChat') + }) + it('orders packaged restoration before adoption, projection, and default creation', () => { // Why this file: the startup sequence moved out of App.tsx into the hydration hook; // the ordering it asserts is unchanged, only the module that now spells it out. diff --git a/src/renderer/src/runtime/local-runtime-capabilities.test.ts b/src/renderer/src/runtime/local-runtime-capabilities.test.ts index eedd31a748a..d205e712867 100644 --- a/src/renderer/src/runtime/local-runtime-capabilities.test.ts +++ b/src/renderer/src/runtime/local-runtime-capabilities.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { + ensureLocalRuntimeCapabilities, readLocalRuntimeCapabilities, readLocalRuntimeCapabilitiesOrUnknown, refreshLocalRuntimeCapabilities, @@ -82,4 +83,54 @@ describe('local runtime capabilities', () => { await refreshLocalRuntimeCapabilities() expect(readLocalRuntimeCapabilitiesOrUnknown()).toEqual(['agent-session.structured.v1']) }) + it('ensure probes the runtime when no answer has landed yet', async () => { + setLocalRuntimeCapabilitiesForTests(null) + const getStatus = vi.fn(async () => ({ capabilities: ['agent-session.structured.v1'] })) + Object.assign(window, { api: { runtime: { getStatus } } }) + + await expect(ensureLocalRuntimeCapabilities()).resolves.toEqual(['agent-session.structured.v1']) + expect(getStatus).toHaveBeenCalledOnce() + }) + + it('ensure returns the cached answer without probing again', async () => { + setLocalRuntimeCapabilitiesForTests(['agent-session.structured.v1']) + const getStatus = vi.fn(async () => ({ capabilities: [] })) + Object.assign(window, { api: { runtime: { getStatus } } }) + + await expect(ensureLocalRuntimeCapabilities()).resolves.toEqual(['agent-session.structured.v1']) + expect(getStatus).not.toHaveBeenCalled() + }) + + it('ensure stays unknown after a failed probe and re-probes on the next call', async () => { + setLocalRuntimeCapabilitiesForTests(null) + const getStatus = vi + .fn() + .mockRejectedValueOnce(new Error('offline')) + .mockResolvedValueOnce({ capabilities: ['agent-session.structured.v1'] }) + Object.assign(window, { api: { runtime: { getStatus } } }) + + // A failed probe is not evidence about the host, so it must not latch as a denial: + // the answer stays unknown and the next caller pays for a fresh probe. + await expect(ensureLocalRuntimeCapabilities()).resolves.toBeNull() + await expect(ensureLocalRuntimeCapabilities()).resolves.toEqual(['agent-session.structured.v1']) + expect(getStatus).toHaveBeenCalledTimes(2) + }) + + it('ensure never rejects when the preload bridge is missing', async () => { + setLocalRuntimeCapabilitiesForTests(null) + Reflect.deleteProperty(window, 'api') + + await expect(ensureLocalRuntimeCapabilities()).resolves.toBeNull() + }) + + it('coalesces concurrent ensure callers onto one probe', async () => { + setLocalRuntimeCapabilitiesForTests(null) + const getStatus = vi.fn(async () => ({ capabilities: ['agent-session.structured.v1'] })) + Object.assign(window, { api: { runtime: { getStatus } } }) + + await expect( + Promise.all([ensureLocalRuntimeCapabilities(), ensureLocalRuntimeCapabilities()]) + ).resolves.toEqual([['agent-session.structured.v1'], ['agent-session.structured.v1']]) + expect(getStatus).toHaveBeenCalledOnce() + }) }) diff --git a/src/renderer/src/runtime/local-runtime-capabilities.ts b/src/renderer/src/runtime/local-runtime-capabilities.ts index 2bb0e1916d3..f4c83880503 100644 --- a/src/renderer/src/runtime/local-runtime-capabilities.ts +++ b/src/renderer/src/runtime/local-runtime-capabilities.ts @@ -15,9 +15,37 @@ export function readLocalRuntimeCapabilitiesOrUnknown(): readonly RuntimeCapabil return localRuntimeCapabilities } +/** Like `readLocalRuntimeCapabilitiesOrUnknown`, but probes the local runtime when no answer + * has landed yet, so a caller that can wait never reads "not asked yet" as "unsupported" + * (#19154: that reads a structured-native-chat create as a bare terminal). + * + * The renderer boot chain calls this once, ungated, so the answer is normally already cached + * by the time any launch route is resolved — including for the readers that are synchronous + * and cannot await. Awaiting it at a route decision is the backstop for the residual window + * and for re-probing after a failed one. Still `null` after an actually failed probe, and + * never rejects. */ +export async function ensureLocalRuntimeCapabilities(): Promise< + readonly RuntimeCapability[] | null +> { + if (localRuntimeCapabilities !== null) { + return localRuntimeCapabilities + } + await refreshLocalRuntimeCapabilities() + return localRuntimeCapabilities +} + +/** `refreshLocalRuntimeCapabilities` is not `async`, so a missing or broken preload bridge would + * throw synchronously out of it instead of settling into the unknown state its catch owns. */ +function startLocalRuntimeCapabilityProbe(): ReturnType { + try { + return window.api.runtime.getStatus() + } catch (error) { + return Promise.reject(error) + } +} + export function refreshLocalRuntimeCapabilities(): Promise { - refreshPromise ??= window.api.runtime - .getStatus() + refreshPromise ??= startLocalRuntimeCapabilityProbe() .then((status) => { localRuntimeCapabilities = [...(status.capabilities ?? [])] return localRuntimeCapabilities @@ -35,8 +63,8 @@ export function refreshLocalRuntimeCapabilities(): Promise