fix(skills): fail closed on stale freshness scans

config/scripts/generate-skill-bundle-manifest.mjs

@@ -4,6 +4,7 @@ import { constants } from 'node:fs'
 import { access, lstat, mkdir, readFile, readdir, writeFile } from 'node:fs/promises'
 import path from 'node:path'
 import process from 'node:process'
+import { isDeepStrictEqual } from 'node:util'
 
 const SCHEMA_VERSION = 1
 const SCRIPT_DIR = import.meta.dirname
@@ -424,15 +425,17 @@ function assertReleasedHistoryPreserved(committedRegistry, artifacts) {
   for (const [name, committedSnapshots] of Object.entries(committedRegistry.skills ?? {})) {
     const releasedCount = artifacts.releasedSnapshotCounts[name] ?? 0
     const regenerated = artifacts.snapshotRegistry.skills[name] ?? []
+    if (releasedCount < Math.max(0, committedSnapshots.length - 1)) {
+      throw new Error(
+        `Released snapshot history is incomplete for ${name}. ` +
+          'Fetch all release tags before regenerating skill artifacts.'
+      )
+    }
     const protectedCount = Math.min(committedSnapshots.length, releasedCount)
     for (let index = 0; index < protectedCount; index += 1) {
       const committed = committedSnapshots[index]
       const rebuilt = regenerated[index]
-      if (
-        !rebuilt ||
-        rebuilt.releaseRevision !== committed.releaseRevision ||
-        rebuilt.packageDigest !== committed.packageDigest
-      ) {
+      if (!rebuilt || !isDeepStrictEqual(rebuilt, committed)) {
         throw new Error(
           `Released snapshot history changed for ${name} at revision ${committed.releaseRevision}. ` +
             'Released snapshots are append-only; a deliberate identity migration must update this check.'

config/scripts/generate-skill-bundle-manifest.test.mjs

@@ -111,6 +111,28 @@ describe('skill bundle manifest generator', () => {
         artifacts
       )
     ).toThrow('Released snapshot history changed for orca-cli at revision 2')
+    expect(() =>
+      assertReleasedHistoryPreserved(
+        {
+          schemaVersion: 1,
+          skills: {
+            'orca-cli': [snapshot(1, 'aaa'), { ...snapshot(2, 'bbb'), gitTreeSha: 'rewritten' }]
+          }
+        },
+        artifacts
+      )
+    ).toThrow('Released snapshot history changed for orca-cli at revision 2')
+    expect(() =>
+      assertReleasedHistoryPreserved(
+        {
+          schemaVersion: 1,
+          skills: {
+            'orca-cli': [snapshot(1, 'aaa'), snapshot(2, 'bbb'), snapshot(3, 'stale')]
+          }
+        },
+        { ...artifacts, releasedSnapshotCounts: { 'orca-cli': 1 } }
+      )
+    ).toThrow('Released snapshot history is incomplete for orca-cli')
     expect(() => assertReleasedHistoryPreserved(null, artifacts)).not.toThrow()
   })
 

src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx

@@ -16,7 +16,8 @@ const mocks = vi.hoisted(() => ({
   toastDismiss: vi.fn(),
   requestDialog: vi.fn(),
   settingsLoaded: true,
-  inventory: null as SkillFreshnessInventory | null
+  inventory: null as SkillFreshnessInventory | null,
+  error: null as string | null
 }))
 
 function placement(
@@ -58,7 +59,7 @@ vi.mock('@/hooks/useSkillFreshness', () => ({
   useSkillFreshness: () => ({
     inventory: mocks.inventory,
     loading: false,
-    error: null,
+    error: mocks.error,
     refresh: vi.fn()
   })
 }))
@@ -106,6 +107,7 @@ describe('SkillFreshnessNudge', () => {
     mocks.dismissed = []
     mocks.settingsLoaded = true
     mocks.inventory = eligibleInventory()
+    mocks.error = null
     mocks.updateSettings.mockReset()
     mocks.updateSettings.mockResolvedValue(undefined)
     mocks.toastInfo.mockReset()
@@ -162,6 +164,19 @@ describe('SkillFreshnessNudge', () => {
     expect(mocks.updateSettings).not.toHaveBeenCalled()
   })
 
+  it('retracts a stale nudge when re-inventory fails', async () => {
+    await renderNudge()
+    const options = mocks.toastInfo.mock.calls[0]?.[1]
+
+    mocks.inventory = null
+    mocks.error = 'scan failed'
+    await rerenderNudge()
+    options.onDismiss()
+
+    expect(mocks.toastDismiss).toHaveBeenCalledWith('freshness-toast')
+    expect(mocks.updateSettings).not.toHaveBeenCalled()
+  })
+
   it('persists the exact placement/revision key once on explicit dismissal', async () => {
     await renderNudge()
 

src/renderer/src/components/skills/SkillFreshnessNudge.tsx

@@ -35,7 +35,18 @@ export function SkillFreshnessNudge(): null {
 
   useEffect(() => {
     const inventory = state.inventory
-    if (!settingsLoaded || !inventory) {
+    if (!settingsLoaded) {
+      return
+    }
+    if (!inventory) {
+      const active = activeNudgeRef.current
+      if (state.error && active) {
+        // Why: a failed re-check cannot keep advertising authority derived from
+        // old bytes; retract without turning the scan failure into a dismissal.
+        active.persistDismissal = false
+        activeNudgeRef.current = null
+        toast.dismiss(active.id)
+      }
       return
     }
     const eligibleNames = new Set(inventory.eligibleUpdateNames)
@@ -152,7 +163,7 @@ export function SkillFreshnessNudge(): null {
       }
     )
     activeNudgeRef.current = nextActive
-  }, [dismissed, settingsLoaded, state.inventory, updateSettings])
+  }, [dismissed, settingsLoaded, state.error, state.inventory, updateSettings])
 
   return null
 }

src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx

@@ -16,6 +16,7 @@ import {
 const mocks = vi.hoisted(() => ({
   inventory: null as SkillFreshnessInventory | null,
   loading: false,
+  error: null as string | null,
   refresh: vi.fn(),
   terminalProps: [] as {
     command: string
@@ -30,7 +31,7 @@ vi.mock('@/hooks/useSkillFreshness', () => ({
   useSkillFreshness: () => ({
     inventory: mocks.inventory,
     loading: mocks.loading,
-    error: null,
+    error: mocks.error,
     refresh: mocks.refresh
   })
 }))
@@ -139,6 +140,7 @@ describe('SkillFreshnessUpdateDialog', () => {
     consumeSkillFreshnessUpdateDialogRequest()
     mocks.inventory = eligibleInventory()
     mocks.loading = false
+    mocks.error = null
     mocks.refresh.mockReset()
     mocks.notifyChanged.mockReset()
     mocks.terminalProps.length = 0
@@ -210,6 +212,14 @@ describe('SkillFreshnessUpdateDialog', () => {
     await act(async () => {
       mocks.terminalProps.at(-1)?.onInteracted?.('keyboard', { key: 'Enter' })
     })
+    mocks.inventory = null
+    mocks.loading = true
+    await rerender()
+
+    expect(container?.querySelector('[data-testid="update-terminal"]')?.textContent).toBe(
+      'npx skills update orca-cli --global'
+    )
+
     mocks.inventory = {
       schemaVersion: 1,
       installations: [
@@ -218,6 +228,7 @@ describe('SkillFreshnessUpdateDialog', () => {
       eligibleUpdateNames: [],
       scannedAt: 2
     }
+    mocks.loading = false
     await rerender()
 
     expect(container?.textContent).toContain('All installed Orca skills are up to date.')
@@ -226,6 +237,30 @@ describe('SkillFreshnessUpdateDialog', () => {
     )
   })
 
+  it('removes an unsubmitted draft as soon as its inventory is invalidated', async () => {
+    await renderDialog()
+    await openViaRequest()
+    expect(container?.querySelector('[data-testid="update-terminal"]')).not.toBeNull()
+
+    mocks.inventory = null
+    mocks.loading = true
+    await rerender()
+
+    expect(container?.textContent).toContain('Checking installed Orca skills')
+    expect(container?.querySelector('[data-testid="update-terminal"]')).toBeNull()
+  })
+
+  it('shows a failed scan as an error instead of indefinite progress', async () => {
+    mocks.inventory = null
+    mocks.error = 'Could not inspect installed skills.'
+    await renderDialog()
+    await openViaRequest()
+
+    expect(container?.textContent).toContain('Could not inspect installed skills.')
+    expect(container?.textContent).not.toContain('Checking installed Orca skills')
+    expect(container?.querySelector('[data-testid="update-terminal"]')).toBeNull()
+  })
+
   it('creates a fresh draft after a terminal exits and the rescan still finds an update', async () => {
     await renderDialog()
     await openViaRequest()

src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx

@@ -149,7 +149,15 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
   }, [])
 
   useEffect(() => {
-    if (!open || state.loading || state.error) {
+    if (!open) {
+      return
+    }
+    if (state.loading || state.error || !inventory) {
+      // Why: a scan invalidates the authorization behind an unsubmitted draft.
+      // A running command keeps its PTY until exit, but stale drafts fail closed.
+      if (!terminalSubmittedRef.current && terminalCommand !== null) {
+        setTerminalCommand(null)
+      }
       return
     }
     if (awaitingExitRefresh) {
@@ -226,9 +234,11 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
           </DialogDescription>
         </DialogHeader>
 
-        {state.error ? <p className="text-xs text-destructive">{state.error}</p> : null}
-
-        <SummaryHeadline kind={summaryKind} eligibleCount={eligibleNames.length} />
+        {state.error ? (
+          <p className="text-xs text-destructive">{state.error}</p>
+        ) : (
+          <SummaryHeadline kind={summaryKind} eligibleCount={eligibleNames.length} />
+        )}
 
         {terminalCommand ? (
           <OnboardingInlineCommandTerminal

src/renderer/src/hooks/useSkillFreshness.test.tsx

@@ -13,12 +13,15 @@ import {
 function deferred<T>(): {
   promise: Promise<T>
   resolve: (value: T) => void
+  reject: (cause: unknown) => void
 } {
   let resolve!: (value: T) => void
-  const promise = new Promise<T>((complete) => {
+  let reject!: (cause: unknown) => void
+  const promise = new Promise<T>((complete, fail) => {
     resolve = complete
+    reject = fail
   })
-  return { promise, resolve }
+  return { promise, resolve, reject }
 }
 
 function inventory(scannedAt: number): SkillFreshnessInventory {
@@ -28,9 +31,11 @@ function inventory(scannedAt: number): SkillFreshnessInventory {
 let root: Root | null = null
 let container: HTMLDivElement | null = null
 let state: SkillFreshnessState | null = null
+const states = new Map<string, SkillFreshnessState>()
 
-function Probe(): null {
+function Probe({ id = 'default' }: { id?: string }): null {
   state = useSkillFreshness()
+  states.set(id, state)
   return null
 }
 
@@ -38,6 +43,7 @@ describe('useSkillFreshness', () => {
   beforeEach(() => {
     _skillFreshnessCacheForTests.reset()
     state = null
+    states.clear()
     container = document.createElement('div')
     document.body.appendChild(container)
     root = createRoot(container)
@@ -118,4 +124,90 @@ describe('useSkillFreshness', () => {
     await act(async () => second.resolve(inventory(2)))
     expect(freshnessInventory).toHaveBeenCalledTimes(2)
   })
+
+  it('publishes a manual refresh to every consumer', async () => {
+    const second = deferred<SkillFreshnessInventory>()
+    const freshnessInventory = vi
+      .fn()
+      .mockResolvedValueOnce(inventory(1))
+      .mockReturnValueOnce(second.promise)
+    window.api = { skills: { freshnessInventory } } as never
+
+    await act(async () =>
+      root?.render(
+        <>
+          <Probe id="one" />
+          <Probe id="two" />
+        </>
+      )
+    )
+    expect(states.get('one')?.inventory?.scannedAt).toBe(1)
+    expect(states.get('two')?.inventory?.scannedAt).toBe(1)
+
+    let refresh: Promise<void> | undefined
+    await act(async () => {
+      refresh = states.get('one')?.refresh()
+      await Promise.resolve()
+    })
+    expect(states.get('one')?.inventory).toBeNull()
+    expect(states.get('two')?.inventory).toBeNull()
+
+    await act(async () => second.resolve(inventory(2)))
+    await refresh
+    expect(states.get('one')?.inventory?.scannedAt).toBe(2)
+    expect(states.get('two')?.inventory?.scannedAt).toBe(2)
+    expect(freshnessInventory).toHaveBeenCalledTimes(2)
+  })
+
+  it('fails closed when an invalidation scan rejects', async () => {
+    const second = deferred<SkillFreshnessInventory>()
+    const freshnessInventory = vi
+      .fn()
+      .mockResolvedValueOnce(inventory(1))
+      .mockReturnValueOnce(second.promise)
+    window.api = { skills: { freshnessInventory } } as never
+
+    await act(async () => root?.render(<Probe />))
+    expect(state?.inventory?.scannedAt).toBe(1)
+
+    await act(async () => window.dispatchEvent(new Event('orca:installed-agent-skills-changed')))
+    expect(state?.inventory).toBeNull()
+    expect(state?.loading).toBe(true)
+
+    await act(async () => second.reject(new Error('scan failed')))
+    expect(state?.inventory).toBeNull()
+    expect(state?.loading).toBe(false)
+    expect(state?.error).toBe('scan failed')
+  })
+
+  it('installs one event-listener pair for multiple consumers and cleans it up', async () => {
+    const addEventListener = vi.spyOn(window, 'addEventListener')
+    const removeEventListener = vi.spyOn(window, 'removeEventListener')
+    window.api = {
+      skills: { freshnessInventory: vi.fn().mockResolvedValue(inventory(1)) }
+    } as never
+
+    await act(async () =>
+      root?.render(
+        <>
+          <Probe id="one" />
+          <Probe id="two" />
+        </>
+      )
+    )
+
+    expect(addEventListener.mock.calls.filter(([name]) => name === 'focus')).toHaveLength(1)
+    expect(
+      addEventListener.mock.calls.filter(([name]) => name === 'orca:installed-agent-skills-changed')
+    ).toHaveLength(1)
+
+    await act(async () => root?.unmount())
+    root = null
+    expect(removeEventListener.mock.calls.filter(([name]) => name === 'focus')).toHaveLength(1)
+    expect(
+      removeEventListener.mock.calls.filter(
+        ([name]) => name === 'orca:installed-agent-skills-changed'
+      )
+    ).toHaveLength(1)
+  })
 })

src/renderer/src/hooks/useSkillFreshness.ts

@@ -1,7 +1,6 @@
-import { useCallback, useEffect, useState } from 'react'
+import { useEffect, useSyncExternalStore } from 'react'
 import type { SkillFreshnessInventory } from '../../../shared/skill-freshness'
 import { INSTALLED_AGENT_SKILLS_CHANGED_EVENT } from './installed-agent-skills-change-event'
-import { useMountedRef } from './useMountedRef'
 
 // Why: window focus fires on every alt-tab, and each scan re-reads and re-hashes
 // every installed package; a just-completed scan stays authoritative briefly.
@@ -11,7 +10,34 @@ let pendingInventory: Promise<SkillFreshnessInventory> | null = null
 let invalidationRevision = 0
 let completedRevision = -1
 let lastCompletedScanAt = 0
-let sharedInvalidationPending = false
+let refreshSequence = 0
+
+type SkillFreshnessSnapshot = {
+  inventory: SkillFreshnessInventory | null
+  loading: boolean
+  error: string | null
+}
+
+let snapshot: SkillFreshnessSnapshot = {
+  inventory: null,
+  loading: false,
+  error: null
+}
+const subscribers = new Set<() => void>()
+
+function publishSnapshot(next: SkillFreshnessSnapshot): void {
+  if (
+    snapshot.inventory === next.inventory &&
+    snapshot.loading === next.loading &&
+    snapshot.error === next.error
+  ) {
+    return
+  }
+  snapshot = next
+  for (const subscriber of subscribers) {
+    subscriber()
+  }
+}
 
 async function loadInventory(force: boolean): Promise<SkillFreshnessInventory> {
   if (force) {
@@ -43,71 +69,77 @@ async function loadInventory(force: boolean): Promise<SkillFreshnessInventory> {
   }
 }
 
-export type SkillFreshnessState = {
-  inventory: SkillFreshnessInventory | null
-  loading: boolean
-  error: string | null
+async function refreshSkillFreshness(force = true): Promise<void> {
+  const sequence = ++refreshSequence
+  // Why: eligibility is write authority for the draft command. Once invalidated,
+  // stale bytes must stop authorizing UI even if the replacement scan fails.
+  publishSnapshot({ inventory: null, loading: true, error: null })
+  try {
+    const inventory = await loadInventory(force)
+    if (sequence === refreshSequence) {
+      publishSnapshot({ inventory, loading: false, error: null })
+    }
+  } catch (cause) {
+    if (sequence === refreshSequence) {
+      publishSnapshot({
+        inventory: null,
+        loading: false,
+        error: cause instanceof Error ? cause.message : 'Could not inspect Orca skills.'
+      })
+    }
+  }
+}
+
+function onWindowFocus(): void {
+  if (Date.now() - lastCompletedScanAt < FOCUS_RESCAN_COOLDOWN_MS) {
+    return
+  }
+  void refreshSkillFreshness(true)
+}
+
+function onInstalledSkillsChanged(): void {
+  void refreshSkillFreshness(true)
+}
+
+function subscribe(subscriber: () => void): () => void {
+  subscribers.add(subscriber)
+  if (subscribers.size === 1) {
+    // Why: every consumer reads one external snapshot, so focus/install events
+    // install one listener and trigger one shared IPC scan regardless of UI count.
+    window.addEventListener('focus', onWindowFocus)
+    window.addEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
+  }
+  return () => {
+    subscribers.delete(subscriber)
+    if (subscribers.size === 0) {
+      window.removeEventListener('focus', onWindowFocus)
+      window.removeEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
+    }
+  }
+}
+
+function getSnapshot(): SkillFreshnessSnapshot {
+  return snapshot
+}
+
+function ensureInventoryLoaded(): void {
+  if (!snapshot.inventory && !snapshot.loading) {
+    void refreshSkillFreshness(false)
+  }
+}
+
+export type SkillFreshnessState = SkillFreshnessSnapshot & {
   refresh: () => Promise<void>
 }
 
 export function useSkillFreshness(): SkillFreshnessState {
-  const [inventory, setInventory] = useState<SkillFreshnessInventory | null>(cachedInventory)
-  const [loading, setLoading] = useState(cachedInventory === null)
-  const [error, setError] = useState<string | null>(null)
-  const mountedRef = useMountedRef()
-
-  const refresh = useCallback(
-    async (force = true): Promise<void> => {
-      setLoading(true)
-      try {
-        const next = await loadInventory(force)
-        if (mountedRef.current) {
-          setInventory(next)
-          setError(null)
-        }
-      } catch (cause) {
-        if (mountedRef.current) {
-          setError(cause instanceof Error ? cause.message : 'Could not inspect Orca skills.')
-        }
-      } finally {
-        if (mountedRef.current) {
-          setLoading(false)
-        }
-      }
-    },
-    [mountedRef]
-  )
+  const current = useSyncExternalStore(subscribe, getSnapshot, getSnapshot)
 
   useEffect(() => {
-    void refresh(false)
-  }, [refresh])
+    ensureInventoryLoaded()
+  }, [])
 
-  useEffect(() => {
-    const invalidate = (respectCooldown: boolean): void => {
-      if (respectCooldown && Date.now() - lastCompletedScanAt < FOCUS_RESCAN_COOLDOWN_MS) {
-        return
-      }
-      // Why: the nudge and the panel both listen for the same events; one forced
-      // rescan per event refreshes every consumer through the shared request.
-      const alreadyInvalidated = sharedInvalidationPending
-      sharedInvalidationPending = true
-      queueMicrotask(() => {
-        sharedInvalidationPending = false
-      })
-      cachedInventory = null
-      void refresh(!alreadyInvalidated)
-    }
-    const onFocus = (): void => invalidate(true)
-    const onInstalledSkillsChanged = (): void => invalidate(false)
-    window.addEventListener('focus', onFocus)
-    window.addEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
-    return () => {
-      window.removeEventListener('focus', onFocus)
-      window.removeEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
-    }
-  }, [refresh])
-
-  return { inventory, loading, error, refresh: () => refresh(true) }
+  return { ...current, refresh: refreshSkillFreshness }
 }
 
 export const _skillFreshnessCacheForTests = {
@@ -117,6 +149,7 @@ export const _skillFreshnessCacheForTests = {
     invalidationRevision = 0
     completedRevision = -1
     lastCompletedScanAt = 0
-    sharedInvalidationPending = false
+    refreshSequence = 0
+    snapshot = { inventory: null, loading: false, error: null }
   }
 }