From 2d1ad808968787ccb9a935e1d8091c1df13b6d7b Mon Sep 17 00:00:00 2001
From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
Date: Wed, 15 Jul 2026 01:12:14 -0700
Subject: [PATCH] fix(skills): fail closed on stale freshness scans
---
.../generate-skill-bundle-manifest.mjs | 13 +-
.../generate-skill-bundle-manifest.test.mjs | 22 +++
.../skills/SkillFreshnessNudge.test.tsx | 19 ++-
.../components/skills/SkillFreshnessNudge.tsx | 15 +-
.../SkillFreshnessUpdateDialog.test.tsx | 37 ++++-
.../skills/SkillFreshnessUpdateDialog.tsx | 18 +-
.../src/hooks/useSkillFreshness.test.tsx | 98 ++++++++++-
src/renderer/src/hooks/useSkillFreshness.ts | 157 +++++++++++-------
8 files changed, 300 insertions(+), 79 deletions(-)
diff --git a/config/scripts/generate-skill-bundle-manifest.mjs b/config/scripts/generate-skill-bundle-manifest.mjs
index bdcc0aecca4..c3542d6f0e2 100644
--- a/config/scripts/generate-skill-bundle-manifest.mjs
+++ b/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.'
diff --git a/config/scripts/generate-skill-bundle-manifest.test.mjs b/config/scripts/generate-skill-bundle-manifest.test.mjs
index 063b30c57f3..cf4a8a100e5 100644
--- a/config/scripts/generate-skill-bundle-manifest.test.mjs
+++ b/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()
})
diff --git a/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx b/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx
index d6dd054e453..ca0090a6c68 100644
--- a/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx
+++ b/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()
diff --git a/src/renderer/src/components/skills/SkillFreshnessNudge.tsx b/src/renderer/src/components/skills/SkillFreshnessNudge.tsx
index c4ea551a5c4..1d1b4fd668b 100644
--- a/src/renderer/src/components/skills/SkillFreshnessNudge.tsx
+++ b/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
}
diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx
index d9af5daf44a..5c14b94182d 100644
--- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx
+++ b/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()
diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx
index 2b0a4ec97ce..61b56fee950 100644
--- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx
+++ b/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 {
- {state.error ? {state.error}
: null}
-
-
+ {state.error ? (
+ {state.error}
+ ) : (
+
+ )}
{terminalCommand ? (
(): {
promise: Promise
resolve: (value: T) => void
+ reject: (cause: unknown) => void
} {
let resolve!: (value: T) => void
- const promise = new Promise((complete) => {
+ let reject!: (cause: unknown) => void
+ const promise = new Promise((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()
-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()
+ const freshnessInventory = vi
+ .fn()
+ .mockResolvedValueOnce(inventory(1))
+ .mockReturnValueOnce(second.promise)
+ window.api = { skills: { freshnessInventory } } as never
+
+ await act(async () =>
+ root?.render(
+ <>
+
+
+
+ )
+ )
+ expect(states.get('one')?.inventory?.scannedAt).toBe(1)
+ expect(states.get('two')?.inventory?.scannedAt).toBe(1)
+
+ let refresh: Promise | 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()
+ const freshnessInventory = vi
+ .fn()
+ .mockResolvedValueOnce(inventory(1))
+ .mockReturnValueOnce(second.promise)
+ window.api = { skills: { freshnessInventory } } as never
+
+ await act(async () => root?.render())
+ 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(
+ <>
+
+
+
+ )
+ )
+
+ 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)
+ })
})
diff --git a/src/renderer/src/hooks/useSkillFreshness.ts b/src/renderer/src/hooks/useSkillFreshness.ts
index 382787bf258..ad2ad5c088f 100644
--- a/src/renderer/src/hooks/useSkillFreshness.ts
+++ b/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 | 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 {
if (force) {
@@ -43,71 +69,77 @@ async function loadInventory(force: boolean): Promise {
}
}
-export type SkillFreshnessState = {
- inventory: SkillFreshnessInventory | null
- loading: boolean
- error: string | null
+async function refreshSkillFreshness(force = true): Promise {
+ 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
}
export function useSkillFreshness(): SkillFreshnessState {
- const [inventory, setInventory] = useState(cachedInventory)
- const [loading, setLoading] = useState(cachedInventory === null)
- const [error, setError] = useState(null)
- const mountedRef = useMountedRef()
-
- const refresh = useCallback(
- async (force = true): Promise => {
- 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 }
}
}