From af01e09baf1fe40d7ef86cd2ba8e01137bd2fd3c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:19:34 -0700 Subject: [PATCH] fix(skills): correct the headless update run's verdict, cancel path, and stopping copy Review fixes for the headless skill-update runner. Main process: - Judge per-skill outcomes on a positive signal. "Absent from eligibleUpdateNames" is not success: a deleted, half-written, or unreadable skill also leaves that list, so a corrupt update reported a green check. skillUpdateFailedNames now requires every convergent placement to come back current or newer-known. - Retire a child's handlers with a per-run token. A failed spawn emits error *and* close, so the second settle clobbered the real spawn ENOENT; a cancelled child could also settle, or write output into, the run that replaced it. The token guards the rescan's finish closure too. - Hold the run `running` until the killed process tree is actually dead. Releasing on the synchronous path let an immediate re-Update spawn a second npx writing the same bundles, with a watchdog so a sweep that never settles cannot wedge the run. - Kill the tree, not just the npx wrapper, via killWithDescendantSweep. - Publish an error instead of a silent `started: false` when the cmd.exe rail rejects the resolved npx path, which a profile directory containing & or % is enough to trigger on Windows. - Coalesce captured output into one push per tick instead of structured-cloning the whole buffer to every window on each progress frame. Renderer: - Keep rows on screen while the settling re-scan runs. Refreshing the inventory nulls it synchronously, so every row vanished at the moment the result appeared. Rows render off the last good scan; eligibility stays on the live snapshot so nothing is authorized off stale bytes. - Retry the names that failed, not the eligibility list that same re-scan has just emptied. - Add Stop, restoring the escape hatch the embedded terminal used to provide, and say "stopping" on every surface rather than claiming the update keeps running in the background. - Show a skipped skill's reason outside the disclosure, so it no longer depends on a mount-time defaultOpen a later re-scan can never re-fire. - Drop the summary line telling users to open "Update details", a control this PR removes; it was translated into four languages. - Keep the success linger from retiring a result the open dialog is showing. - Delete skill-location-chip-copy.tsx: an unreferenced copy of the old row component, colliding on basename with the module that is actually imported. --- src/main/ipc/skills.ts | 6 +- src/main/skills/skill-update-outcome.test.ts | 96 ++++++++++ src/main/skills/skill-update-outcome.ts | 39 ++++ src/main/skills/skill-update-run.test.ts | 174 +++++++++++++++++- src/main/skills/skill-update-run.ts | 170 +++++++++++++++-- .../SkillFreshnessUpdateDialog.test.tsx | 173 ++++++++++++++++- .../skills/SkillFreshnessUpdateDialog.tsx | 144 ++++++++++++--- .../src/components/skills/SkillUpdateRow.tsx | 14 +- .../skill-freshness-summary-headline.tsx | 22 +-- .../skills/skill-location-chip-copy.tsx | 143 -------------- .../skills/skill-update-run-store.ts | 72 ++++++-- .../status-bar/SkillUpdateStatusSegment.tsx | 19 ++ src/renderer/src/i18n/locales/en.json | 11 +- src/renderer/src/i18n/locales/es.json | 11 +- src/renderer/src/i18n/locales/ja.json | 11 +- src/renderer/src/i18n/locales/ko.json | 11 +- src/renderer/src/i18n/locales/zh.json | 11 +- src/shared/skill-freshness.ts | 7 +- 18 files changed, 886 insertions(+), 248 deletions(-) create mode 100644 src/main/skills/skill-update-outcome.test.ts create mode 100644 src/main/skills/skill-update-outcome.ts delete mode 100644 src/renderer/src/components/skills/skill-location-chip-copy.tsx diff --git a/src/main/ipc/skills.ts b/src/main/ipc/skills.ts index 7adfc296daf..a03bd57288c 100644 --- a/src/main/ipc/skills.ts +++ b/src/main/ipc/skills.ts @@ -12,6 +12,7 @@ import type { } from '../../shared/skill-freshness' import { inventorySkillFreshness } from '../skills/skill-freshness-inventory' import { SkillUpdateRunner } from '../skills/skill-update-run' +import { skillUpdateFailedNames } from '../skills/skill-update-outcome' import { discoverSkillsOnTarget, resolveSkillDiscoveryTarget @@ -28,11 +29,10 @@ export function registerSkillsHandlers(store: Store): void { const runner = new SkillUpdateRunner({ // Why: per-skill outcomes come from re-hashing what is actually on disk, not - // from scraping stdout. Anything still eligible after the run did not land. + // from scraping stdout. rescanOutdatedNames: async (names) => { const inventory = await scanInventory() - const stillEligible = new Set(inventory.eligibleUpdateNames) - return names.filter((name) => stillEligible.has(name)) + return skillUpdateFailedNames(names, inventory.installations) }, onState: (run: SkillUpdateRun) => { for (const window of BrowserWindow.getAllWindows()) { diff --git a/src/main/skills/skill-update-outcome.test.ts b/src/main/skills/skill-update-outcome.test.ts new file mode 100644 index 00000000000..4aa2ce401b2 --- /dev/null +++ b/src/main/skills/skill-update-outcome.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from 'vitest' +import type { + SkillFreshnessInstallation, + SkillFreshnessStatus, + SkillInstallationTopology +} from '../../shared/skill-freshness' +import { skillUpdateFailedNames } from './skill-update-outcome' + +function placement( + name: string, + status: SkillFreshnessStatus, + topology: SkillInstallationTopology = 'canonical-copy' +): SkillFreshnessInstallation { + return { + id: `${name}-${topology}-${status}`, + name, + rootId: 'home-agents', + providers: ['agent-skills'], + sourceKind: 'home', + sourceLabel: 'Agent skills home', + unresolvedPath: `/home/.agents/skills/${name}`, + resolvedPath: `/home/.agents/skills/${name}`, + physicalIdentity: `physical-${name}`, + topology, + status, + installedReleaseRevision: 2, + installedAppVersion: '2.0.0', + currentReleaseRevision: 2, + currentPackageDigest: 'current', + currentAppVersion: '2.0.0', + observedPackageDigest: 'current', + errorCategory: null + } +} + +describe('skillUpdateFailedNames', () => { + it('treats a convergent copy that is now current as landed', () => { + expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'current')])).toEqual([]) + }) + + it('reports a copy the run left outdated', () => { + expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'outdated')])).toEqual([ + 'orca-cli' + ]) + }) + + it('reports a half-written bundle instead of reading it as success', () => { + // The old "still eligible?" test passed here: an unrecognized copy is not + // eligible either, so a corrupt write looked identical to a clean update. + expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'unrecognized')])).toEqual([ + 'orca-cli' + ]) + }) + + it('reports an unreadable copy', () => { + expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'inaccessible')])).toEqual([ + 'orca-cli' + ]) + }) + + it('reports a skill the run removed outright', () => { + expect(skillUpdateFailedNames(['orca-cli'], [])).toEqual(['orca-cli']) + }) + + it('accepts a revision newer than this build ships', () => { + // The CLI pulls from the source repo, which runs ahead of the bundled manifest. + expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'newer-known')])).toEqual([]) + }) + + it('ignores placements the update command never writes to', () => { + expect( + skillUpdateFailedNames( + ['orca-cli'], + [placement('orca-cli', 'current'), placement('orca-cli', 'outdated', 'plugin-cache')] + ) + ).toEqual([]) + }) + + it('fails the name when any convergent alias was left behind', () => { + expect( + skillUpdateFailedNames( + ['orca-cli'], + [placement('orca-cli', 'current'), placement('orca-cli', 'outdated', 'provider-alias')] + ) + ).toEqual(['orca-cli']) + }) + + it('judges each requested name independently', () => { + expect( + skillUpdateFailedNames( + ['orca-cli', 'orchestration'], + [placement('orca-cli', 'current'), placement('orchestration', 'outdated')] + ) + ).toEqual(['orchestration']) + }) +}) diff --git a/src/main/skills/skill-update-outcome.ts b/src/main/skills/skill-update-outcome.ts new file mode 100644 index 00000000000..6d75d4b5efc --- /dev/null +++ b/src/main/skills/skill-update-outcome.ts @@ -0,0 +1,39 @@ +import { + SUPPORTED_GLOBAL_SKILL_TOPOLOGIES, + type SkillFreshnessInstallation +} from '../../shared/skill-freshness' + +/** + * Names that did not land, judged from the post-run inventory. + * + * Why not "whatever is still in `eligibleUpdateNames`": a name leaves that list + * for reasons that are not success. A half-written bundle hashes to + * `unrecognized`, a failed write can leave the directory unreadable, and a + * removed skill has no convergent placement at all — each would read as + * "updated" and show a green check over a broken skill. So demand a positive + * signal instead: every placement the command writes to has to come back as a + * revision we recognise. + * + * Known limit: the topology filter runs before the status check, so a placement + * that degrades OUT of the convergent set is dropped from the judgment rather + * than failing it. A run that rewrites the canonical copy but leaves a + * provider-alias a broken symlink still reports success. Catching that needs the + * pre-run inventory to compare convergent-then against convergent-now; every + * wholly-degraded and removed-entirely case is already reported as a failure. + */ +export function skillUpdateFailedNames( + names: readonly string[], + installations: readonly SkillFreshnessInstallation[] +): string[] { + return names.filter((name) => { + const convergent = installations.filter( + (entry) => entry.name === name && SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(entry.topology) + ) + if (convergent.length === 0) { + return true + } + // `newer-known` counts as landed: the CLI pulls from the source repo, which + // can be ahead of the revision this build ships in its manifest. + return convergent.some((entry) => entry.status !== 'current' && entry.status !== 'newer-known') + }) +} diff --git a/src/main/skills/skill-update-run.test.ts b/src/main/skills/skill-update-run.test.ts index e53f3b1b7c4..0aeb31bf32e 100644 --- a/src/main/skills/skill-update-run.test.ts +++ b/src/main/skills/skill-update-run.test.ts @@ -1,11 +1,12 @@ import { EventEmitter } from 'node:events' import { describe, expect, it, vi } from 'vitest' import type { SkillUpdateRun } from '../../shared/skill-freshness' -import { SkillUpdateRunner } from './skill-update-run' +import { CANCEL_RELEASE_TIMEOUT_MS, SkillUpdateRunner } from './skill-update-run' class FakeChild extends EventEmitter { stdout = new EventEmitter() stderr = new EventEmitter() + pid: number | undefined = 1234 kill = vi.fn() } @@ -13,6 +14,8 @@ function makeRunner( overrides: { rescanOutdatedNames?: (names: string[]) => Promise resolveCommand?: (name: string) => string + killTree?: (pid: number, killRoot: () => void) => Promise + buildSpawnArgs?: (command: string, args: string[]) => { spawnCmd: string; spawnArgs: string[] } } = {} ) { const child = new FakeChild() @@ -22,6 +25,9 @@ function makeRunner( now: () => 1000, resolveCommand: overrides.resolveCommand ?? (() => '/usr/local/bin/npx'), rescanOutdatedNames: overrides.rescanOutdatedNames, + // Default to a no-op sweep so tests never signal a real PID. + killTree: overrides.killTree ?? (async (_pid, killRoot) => killRoot()), + buildSpawnArgs: overrides.buildSpawnArgs, onState: (run) => states.push(run), spawnProcess: ((command: string, args: string[], options: Record) => { spawnCalls.push({ command, args, options }) @@ -129,6 +135,102 @@ describe('SkillUpdateRunner', () => { expect(run.state === 'error' && run.message).toBe('spawn ENOENT') }) + it('keeps the spawn error when the failed child also emits close', async () => { + // A spawn failure emits `error` *then* `close`. Without a latch the second + // settle overwrites `spawn ENOENT` with a useless "exited with code null". + const rescan = vi.fn(async () => ['orca-cli']) + const { runner, child } = makeRunner({ rescanOutdatedNames: rescan }) + runner.start(['orca-cli']) + child.emit('error', new Error('spawn ENOENT')) + child.emit('close', null) + await flush() + + const run = runner.getState() + expect(run.state === 'error' && run.message).toBe('spawn ENOENT') + expect(rescan).toHaveBeenCalledTimes(1) + }) + + it('refuses a new run until the cancelled process tree is actually dead', async () => { + // The sweep waits for a descendant snapshot before it signals anything, so + // releasing the UI synchronously would let a second npx write the same + // bundles as the one still being killed. + let finishKill = (): void => {} + const { runner } = makeRunner({ + killTree: (_pid, killRoot) => + new Promise((resolve) => { + finishKill = () => { + killRoot() + resolve() + } + }) + }) + runner.start(['orca-cli']) + runner.cancel() + await flush() + + expect(runner.getState().state).toBe('running') + expect(runner.start(['orchestration'])).toEqual({ started: false, reason: 'already-running' }) + + finishKill() + await flush() + expect(runner.getState()).toEqual({ state: 'idle' }) + expect(runner.start(['orchestration'])).toEqual({ started: true }) + }) + + it('releases the run even if the kill sweep never settles', async () => { + vi.useFakeTimers() + try { + // Stop is already spent by this point, so a sweep that hangs would leave + // the run wedged in `running` with no way out. + const { runner } = makeRunner({ killTree: () => new Promise(() => {}) }) + runner.start(['orca-cli']) + runner.cancel() + const stopping = runner.getState() + expect(stopping.state).toBe('running') + // Surfaced so the dialog can retire the Stop affordance it already spent. + expect(stopping.state === 'running' && stopping.stopping).toBe(true) + + await vi.advanceTimersByTimeAsync(CANCEL_RELEASE_TIMEOUT_MS) + + expect(runner.getState()).toEqual({ state: 'idle' }) + expect(runner.start(['orchestration'])).toEqual({ started: true }) + } finally { + vi.useRealTimers() + } + }) + + it('does not let a cancelled child settle the run that replaced it', async () => { + const children: FakeChild[] = [] + const states: SkillUpdateRun[] = [] + const runner = new SkillUpdateRunner({ + now: () => 1000, + resolveCommand: () => '/usr/local/bin/npx', + rescanOutdatedNames: async () => [], + // Never let a test reach the real sweep — it would signal live PIDs. + killTree: async (_pid, killRoot) => killRoot(), + onState: (run) => states.push(run), + spawnProcess: (() => { + const child = new FakeChild() + children.push(child) + return child as never + }) as never + }) + runner.start(['orca-cli']) + runner.cancel() + await flush() + runner.start(['orchestration']) + // The killed child's exit lands after the replacement is already in flight. + children[0].stdout.emit('data', Buffer.from('output from the dead run')) + children[0].emit('close', 1) + await flush() + + const run = runner.getState() + expect(run.state).toBe('running') + expect(run.state === 'running' && run.names).toEqual(['orchestration']) + expect(run.state === 'running' && run.output).toBe('') + expect(states.some((state) => state.state === 'error')).toBe(false) + }) + it('returns to idle on cancel and stops reporting output', async () => { const { runner, child, states } = makeRunner({ rescanOutdatedNames: async () => [] }) runner.start(['orca-cli']) @@ -141,6 +243,76 @@ describe('SkillUpdateRunner', () => { expect(states.at(-1)).toEqual({ state: 'idle' }) }) + it('ignores a re-scan that resolves after the run was cancelled', async () => { + let releaseRescan = (): void => {} + const { runner, child } = makeRunner({ + rescanOutdatedNames: () => + new Promise((resolve) => { + releaseRescan = () => resolve([]) + }) + }) + runner.start(['orca-cli']) + child.emit('close', 0) + await flush() + // The re-scan re-hashes every package, so a cancel lands well inside it. + runner.cancel() + releaseRescan() + await flush() + + expect(runner.getState()).toEqual({ state: 'idle' }) + }) + + it('kills the whole npx tree on cancel, not just the wrapper', async () => { + const killTree = vi.fn(async (_pid: number, killRoot: () => void) => { + killRoot() + }) + const { runner, child } = makeRunner({ killTree }) + child.pid = 4242 + runner.start(['orca-cli']) + runner.cancel() + await flush() + + expect(killTree).toHaveBeenCalledWith(4242, expect.any(Function)) + expect(child.kill).toHaveBeenCalled() + }) + + it('surfaces an unspawnable command path instead of silently doing nothing', async () => { + // A Windows profile directory containing `&` makes the cmd.exe rail reject + // the resolved npx path; the names are already canonical by this point. + const { runner, states } = makeRunner({ + resolveCommand: () => 'C:\\Users\\A&B\\AppData\\Roaming\\npm\\npx.cmd', + buildSpawnArgs: () => { + throw new Error('unsafe batch arguments') + } + }) + + const result = runner.start(['orca-cli']) + + expect(result.started).toBe(false) + const run = runner.getState() + expect(run.state).toBe('error') + expect(run.state === 'error' && run.failedNames).toEqual(['orca-cli']) + expect(states.at(-1)?.state).toBe('error') + }) + + it('coalesces progress frames into one push instead of one per chunk', async () => { + const { runner, child, states } = makeRunner({ rescanOutdatedNames: async () => [] }) + runner.start(['orca-cli']) + const pushesAfterStart = states.length + for (let frame = 0; frame < 25; frame += 1) { + child.stdout.emit('data', Buffer.from(`\rfetching ${frame}%`)) + } + expect(states.length).toBe(pushesAfterStart) + + child.emit('close', 0) + await flush() + + const run = runner.getState() + expect(run.state).toBe('success') + // Nothing is dropped — the tail is drained before the run settles. + expect(run.state === 'success' && run.output).toContain('fetching 24%') + }) + it('acknowledge clears a settled run but leaves a live one alone', async () => { const { runner, child } = makeRunner({ rescanOutdatedNames: async () => [] }) runner.start(['orca-cli']) diff --git a/src/main/skills/skill-update-run.ts b/src/main/skills/skill-update-run.ts index 13f8d8f8e6b..9083b24e43f 100644 --- a/src/main/skills/skill-update-run.ts +++ b/src/main/skills/skill-update-run.ts @@ -5,6 +5,7 @@ import { type SkillUpdateStartResult } from '../../shared/skill-freshness' import { resolveCliCommand } from '../codex-cli/command' +import { killWithDescendantSweep } from '../pty-descendant-termination' import { getSpawnArgsForWindows } from '../win32-utils' // Why: `skills update` prints ANSI colour and \r + erase-line progress. We show @@ -16,11 +17,23 @@ const ANSI_RE = /\x1b\[[0-9;?]*[A-Za-z]/g // eslint-disable-line no-control-rege // however much the CLI decides to print. const MAX_OUTPUT_CHARS = 32_000 +// Long enough to swallow a burst of progress frames, short enough that the log +// still reads as live when the user has it expanded. +const OUTPUT_FLUSH_MS = 100 + +// Strictly above the sweep's own transitive worst case (~1s on POSIX; 3s identity +// query + 5s taskkill on Windows). A backstop that ties its own bound would fire +// while a slow-but-healthy sweep is still working. +export const CANCEL_RELEASE_TIMEOUT_MS = 12_000 + export type SkillUpdateRunnerDeps = { spawnProcess?: typeof spawn resolveCommand?: (commandName: string) => string - /** Returns the subset of `names` still outdated after the run. */ + /** Returns the subset of `names` that did not land, re-read from disk. */ rescanOutdatedNames?: (names: string[]) => Promise + killTree?: (pid: number, killRoot: () => void) => Promise + /** Injected so the Windows cmd.exe rail is reachable off Windows. */ + buildSpawnArgs?: typeof getSpawnArgsForWindows now?: () => number onState?: (run: SkillUpdateRun) => void } @@ -45,6 +58,13 @@ function clampOutput(value: string): string { export class SkillUpdateRunner { private run: SkillUpdateRun = { state: 'idle' } private child: ChildProcess | null = null + // Why: a failed spawn emits `error` *and* `close`, and a cancelled child still + // emits `close` after `kill()`. The token retires a child's handlers so a dead + // run can never settle or write output into the run that replaced it; the latch + // keeps the first verdict of a live run while its re-scan is still in flight. + private runToken = 0 + private settling = false + private killing = false private readonly deps: Required> & SkillUpdateRunnerDeps constructor(deps: SkillUpdateRunnerDeps = {}) { @@ -77,12 +97,29 @@ export class SkillUpdateRunner { let spawnCmd: string let spawnArgs: string[] try { - ;({ spawnCmd, spawnArgs } = getSpawnArgsForWindows(npxCommand, npxArgs)) + const buildSpawnArgs = this.deps.buildSpawnArgs ?? getSpawnArgsForWindows + ;({ spawnCmd, spawnArgs } = buildSpawnArgs(npxCommand, npxArgs)) } catch { - return { started: false, reason: 'invalid-names' } + // Why: the names are already canonical here, so this is the cmd.exe rail + // rejecting the resolved npx *path* — a profile directory containing `&`, + // `%` or `!` is enough. Publishing the failure keeps the dialog honest; + // returning a bare `started: false` would leave the button dead and silent. + this.runToken += 1 + this.settling = false + this.publish({ + state: 'error', + names: canonicalNames, + finishedAt: this.deps.now(), + output: '', + failedNames: canonicalNames, + message: `Could not run ${npxCommand} safely from this location.` + }) + return { started: false, reason: 'unsafe-command-path' } } const startedAt = this.deps.now() + const token = ++this.runToken + this.settling = false this.publish({ state: 'running', names: canonicalNames, startedAt, output: '' }) const child = spawnProcess(spawnCmd, spawnArgs, { @@ -94,32 +131,62 @@ export class SkillUpdateRunner { }) this.child = child - const append = (chunk: Buffer): void => { - if (this.run.state !== 'running') { + // Why: `stripAnsi` turns each \r progress frame into its own line, so an npm + // install emits many chunks a second — and every publish structured-clones + // the whole buffer to every window. Coalesce into one push per tick. + let flushTimer: ReturnType | null = null + let pendingOutput = '' + const flush = (): void => { + flushTimer = null + if (token !== this.runToken || this.run.state !== 'running' || !pendingOutput) { return } - this.publish({ - ...this.run, - output: clampOutput(this.run.output + stripAnsi(chunk.toString('utf8'))) - }) + const appended = pendingOutput + pendingOutput = '' + this.publish({ ...this.run, output: clampOutput(this.run.output + appended) }) + } + const append = (chunk: Buffer): void => { + if (token !== this.runToken || this.run.state !== 'running') { + return + } + pendingOutput = clampOutput(pendingOutput + stripAnsi(chunk.toString('utf8'))) + if (!flushTimer) { + flushTimer = setTimeout(flush, OUTPUT_FLUSH_MS) + flushTimer.unref?.() + } } child.stdout?.on('data', append) child.stderr?.on('data', append) + // The tail matters most on failure, so never let a pending chunk die with the + // process — drain it before the exit handlers settle the run. + const drain = (): void => { + if (flushTimer) { + clearTimeout(flushTimer) + } + flush() + } child.on('error', (error) => { - this.settle(canonicalNames, error.message) + drain() + this.settle(token, canonicalNames, error.message) }) child.on('close', (code) => { - this.settle(canonicalNames, code === 0 ? null : `skills update exited with code ${code}`) + drain() + this.settle( + token, + canonicalNames, + code === 0 ? null : `skills update exited with code ${code}` + ) }) return { started: true } } - private settle(names: string[], spawnError: string | null): void { - if (this.run.state !== 'running') { + private settle(token: number, names: string[], spawnError: string | null): void { + if (token !== this.runToken || this.settling || this.run.state !== 'running') { return } + this.settling = true this.child = null const output = this.run.output const finishedAt = this.deps.now() @@ -130,6 +197,12 @@ export class SkillUpdateRunner { // code only decides the outcome when no verdict is available, because // `skills update` reports nothing else we can trust. const finish = (failedNames: string[] | null): void => { + // The re-scan is slow enough that a cancel — or a whole replacement run — + // can land while it is still in flight; its verdict is about a run that no + // longer exists. + if (token !== this.runToken) { + return + } const failed = failedNames ?? (spawnError ? names : []) if (failed.length === 0) { this.publish({ state: 'success', names, finishedAt, output }) @@ -156,11 +229,74 @@ export class SkillUpdateRunner { } cancel(): void { - this.child?.kill() - this.child = null - if (this.run.state === 'running') { - this.publish({ state: 'idle' }) + if (this.killing) { + return } + // Retire the child's handlers now so its exit settles nothing. + this.runToken += 1 + this.settling = false + const child = this.child + this.child = null + if (!child) { + if (this.run.state === 'running') { + this.publish({ state: 'idle' }) + } + return + } + + // Why: `npx` is a wrapper — on POSIX the `skills` process it execs is a + // child, and on Windows the shim runs under cmd.exe. Killing only the direct + // child leaves the process that is actually writing to the global skill + // homes alive. + this.killing = true + let hasReleased = false + let releaseTimer: ReturnType | null = null + const release = (): void => { + if (hasReleased) { + return + } + hasReleased = true + if (releaseTimer) { + clearTimeout(releaseTimer) + } + this.killing = false + // Why: stay `running` until the tree is actually dead. The sweep waits for + // a descendant snapshot before it signals anything, so releasing on the + // synchronous path would let an immediate re-Update spawn a second npx + // writing the same bundles — the corruption the post-run verdict exists to + // catch. `start()` already refuses while running, so holding the state is + // the whole guard. + if (this.run.state === 'running') { + this.publish({ state: 'idle' }) + } + } + // Every layer of the sweep is individually bounded, but this is the recovery + // path: if one ever fails to settle, the run would be stuck `running` with + // Stop already spent. Cap it rather than depend on that transitively. + releaseTimer = setTimeout(release, CANCEL_RELEASE_TIMEOUT_MS) + releaseTimer.unref?.() + if (this.run.state === 'running') { + this.publish({ ...this.run, stopping: true }) + } + + const kill = this.deps.killTree ?? killWithDescendantSweep + const pid = child.pid + if (typeof pid !== 'number') { + // Same contract as the sweep path below: a throwing kill must not escape + // and leave `killing` latched with the run stuck `running`. + try { + child.kill() + } catch { + /* already gone, or not ours to signal */ + } + release() + return + } + // Why `release` on both paths and no retry: the sweep runs `killRoot()` in its + // own `finally`, so the only way it rejects is that kill throwing (EPERM) — + // calling it again would throw straight back out of the rejection handler, + // leaving an unhandled rejection and no release at all. + void kill(pid, () => child.kill()).then(release, release) } /** Clears a settled run so the status-bar segment can retire itself. */ diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx index e9d2ef9ad58..875fc3974bf 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx @@ -13,7 +13,7 @@ import { consumeSkillFreshnessUpdateDialogRequest, requestSkillFreshnessUpdateDialog } from './skill-freshness-update-dialog' -import { _resetSkillUpdateRunStore } from './skill-update-run-store' +import { _resetSkillUpdateRunStore, SKILL_UPDATE_SUCCESS_LINGER_MS } from './skill-update-run-store' const mocks = vi.hoisted(() => ({ inventory: null as SkillFreshnessInventory | null, @@ -67,7 +67,11 @@ vi.mock('@/components/ui/collapsible', () => ({ ) }, CollapsibleTrigger: ({ children }: { children?: ReactNode }) =>
{children}
, - CollapsibleContent: ({ children }: { children?: ReactNode }) =>
{children}
+ // Tagged so tests can prove WHERE content sits: this mock renders it whether + // or not the disclosure is open, so presence in the DOM alone proves nothing. + CollapsibleContent: ({ children }: { children?: ReactNode }) => ( +
{children}
+ ) })) const skillsApi = { @@ -333,6 +337,32 @@ describe('SkillFreshnessUpdateDialog', () => { expect(container?.textContent).toContain('2 locations') }) + it('does not let the status-bar linger retire the result being read here', async () => { + vi.useFakeTimers() + try { + await renderDialog() + await openViaRequest() + await emitRun({ state: 'success', names: ['orca-cli'], finishedAt: 2, output: 'done' }) + + await act(async () => { + vi.advanceTimersByTime(SKILL_UPDATE_SUCCESS_LINGER_MS * 3) + }) + + expect(skillsApi.acknowledgeUpdateRun).not.toHaveBeenCalled() + expect(container?.textContent).toContain('Updated 1 skill') + expect(findButton('Done')).toBeDefined() + expect( + container?.querySelector('[data-skill-row="orca-cli"]')?.getAttribute('data-state-label') + ).toBe('done') + + // Closing is what hands the run back — it must not be left stuck. + await clickButton('Done') + expect(skillsApi.acknowledgeUpdateRun).toHaveBeenCalledTimes(1) + } finally { + vi.useRealTimers() + } + }) + it('shows the captured log verbatim without parsing it', async () => { await renderDialog() await openViaRequest() @@ -362,7 +392,7 @@ describe('SkillFreshnessUpdateDialog', () => { expect(findButton('Update 1 skill')).toBeUndefined() }) - it('keeps skills it cannot update visible with their reason', async () => { + it('shows why a skill was skipped without needing the disclosure', async () => { mocks.inventory = { schemaVersion: 1, installations: [placement('computer-use', { topology: 'repo-scope' })], @@ -372,18 +402,151 @@ describe('SkillFreshnessUpdateDialog', () => { await renderDialog() await openViaRequest() + const row = container?.querySelector('[data-skill-row="computer-use"]') expect(container?.textContent).toContain('computer-use') expect(container?.textContent).toContain('Skipped') + // The reason is the one thing a skipped row exists to say, so it lives + // outside the disclosure — visible with the row still collapsed, and not + // dependent on a mount-time `defaultOpen` a later re-scan could never re-fire. + expect(row?.getAttribute('data-collapsible-open')).toBe('false') + expect(container?.textContent).toContain('This is a project skill, not a global one') + // Assert the placement, not just the presence: moving it back inside the + // disclosure would hide it in production but still satisfy `textContent`. + expect(row?.querySelector('[data-collapsible-content]')?.textContent).not.toContain( + 'This is a project skill, not a global one' + ) + }) + + it('stays coherent while an idle re-scan is in flight', async () => { + await renderDialog() + await openViaRequest() + // Re-check publishes {inventory: null, loading: true} synchronously. The rows + // are retained, so the headline and the count must not be read off the live + // snapshot and the retained one at the same time — that pairs a "0 updates + // available" headline with rows badged "Update available". + mocks.inventory = null + mocks.loading = true + await rerender() + + expect(container?.textContent).not.toContain('0 updates available') + expect(container?.textContent).toContain('Checking installed Orca skills…') + // The action keeps its place rather than reflowing the footer, but cannot + // fire against bytes that are being re-read. + const update = findButton('Update 1 skill') + expect(update).toBeDefined() + expect(update?.disabled).toBe(true) + expect( + container?.querySelector('[data-skill-row="orca-cli"]')?.getAttribute('data-state-label') + ).toBe('available') + }) + + it('says it is stopping while the process tree is still being killed', async () => { + await renderDialog() + await openViaRequest() + await emitRun({ state: 'running', names: ['orca-cli'], startedAt: 1, output: '' }) + await clickButton('Stop') + // Main holds the run `running` until the kill lands — that is what blocks a + // second writer — so the button must not sit enabled and inert meanwhile. + await emitRun({ + state: 'running', + names: ['orca-cli'], + startedAt: 1, + output: '', + stopping: true + }) + + const stopping = findButton('Stopping…') + expect(stopping).toBeDefined() + expect(stopping?.disabled).toBe(true) + expect(findButton('Stop')).toBeUndefined() + // The headline must not contradict the button — telling someone the update + // "keeps running in the background" is the opposite of what Stop just did. + expect(container?.textContent).toContain('Stopping the update…') + expect(container?.textContent).not.toContain('keeps running in the background') + expect(container?.textContent).not.toContain('Updating 1 skill…') + // The primary button sits right next to "Stopping…" — it must not still be + // announcing "Updating…", visually or to a screen reader. + expect(findButton('Updating…')).toBeUndefined() + expect(container?.querySelector('[role="progressbar"]')?.getAttribute('aria-label')).toBe( + 'Stopping the update…' + ) + }) + + it('re-reads the inventory after a run is stopped', async () => { + await renderDialog() + await openViaRequest() + await emitRun({ state: 'running', names: ['orca-cli'], startedAt: 1, output: '' }) + mocks.notifyChanged.mockClear() + // A killed run may already have written several skills; leaving the pre-run + // scan on screen would re-offer skills that are now current. + await emitRun({ state: 'idle' }) + + expect(mocks.notifyChanged).toHaveBeenCalled() + }) + + it('keeps the rows on screen while the settling re-scan is in flight', async () => { + await renderDialog() + await openViaRequest() + await emitRun({ state: 'success', names: ['orca-cli'], finishedAt: 2, output: 'done' }) + + // Settling notifies every skills surface, and that refresh nulls the + // inventory synchronously while it re-hashes every package on disk. + mocks.inventory = null + mocks.loading = true + await rerender() + + expect( + container?.querySelector('[data-skill-row="orca-cli"]')?.getAttribute('data-state-label') + ).toBe('done') + expect(container?.textContent).toContain('Updated 1 skill') + }) + + it('retries what failed rather than the emptied eligibility list', async () => { + await renderDialog() + await openViaRequest() + await emitRun({ + state: 'error', + names: ['orca-cli'], + failedNames: ['orca-cli'], + finishedAt: 3, + output: '', + message: 'skills update exited with code 1' + }) + // The settling re-scan has emptied `eligibleUpdateNames` by the time Retry + // is on screen; retrying that list would spawn nothing at all. + mocks.inventory = null + mocks.loading = true + await rerender() + await clickButton('Retry') + + expect(skillsApi.startUpdateRun).toHaveBeenCalledWith(['orca-cli']) + }) + + it('offers a way out of a run that never finishes', async () => { + await renderDialog() + await openViaRequest() + await emitRun({ state: 'running', names: ['orca-cli'], startedAt: 1, output: '' }) + await clickButton('Stop') + + expect(skillsApi.cancelUpdateRun).toHaveBeenCalledTimes(1) }) it('surfaces a scan error instead of a stale summary', async () => { - mocks.inventory = null - mocks.error = 'Missing canonical agent skills root' await renderDialog() await openViaRequest() + // Prime a good scan first: the retained inventory must not survive into the + // error state. That invariant lives in useSkillFreshness (every publish that + // sets `error` also clears `loading`), so pin it from the side that depends + // on it — otherwise a later "keep spinning while retrying" change would + // silently start showing stale rows under an error. + expect(container?.querySelector('[data-skill-row="orca-cli"]')).not.toBeNull() + + mocks.inventory = null + mocks.error = 'Missing canonical agent skills root' await rerender() expect(container?.textContent).toContain('Missing canonical agent skills root') + expect(container?.querySelector('[data-skill-row="orca-cli"]')).toBeNull() expect(findButton('Update 1 skill')).toBeUndefined() }) }) diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx index b6892a6904f..e29d82d07b0 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx @@ -1,6 +1,9 @@ -import { useMemo, useState, useSyncExternalStore } from 'react' +import { useMemo, useRef, useState, useSyncExternalStore } from 'react' import { AlertTriangle, CheckCircle2, ChevronDown, Copy, Loader2, RefreshCw } from 'lucide-react' -import { buildTargetedSkillUpdateCommand } from '../../../../shared/skill-freshness' +import { + buildTargetedSkillUpdateCommand, + type SkillFreshnessInventory +} from '../../../../shared/skill-freshness' import { useSkillFreshness } from '@/hooks/useSkillFreshness' import { notifyInstalledAgentSkillsChanged } from '@/hooks/useInstalledAgentSkills' import { translate } from '@/i18n/i18n' @@ -19,6 +22,7 @@ import { SkillUpdateRow } from './SkillUpdateRow' import { SummaryHeadline, summarizeInventory } from './skill-freshness-summary-headline' import { acknowledgeSkillUpdateRun, + cancelSkillUpdateRun, startSkillUpdateRun, useSkillUpdateRun } from './skill-update-run-store' @@ -64,11 +68,31 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { getSkillFreshnessUpdateDialogRequest ) const [copied, setCopied] = useState(false) - const inventory = state.inventory - const eligibleNames = useMemo(() => inventory?.eligibleUpdateNames ?? [], [inventory]) + + // Why: settling a run notifies every skills surface, and that refresh nulls the + // inventory *synchronously* while it re-hashes each package on disk. Rendering + // rows off the last good scan keeps them on screen through that window instead + // of blanking the dialog at the exact moment the result appears. Eligibility + // below still reads the live snapshot, so nothing is authorized off stale bytes. + const lastInventoryRef = useRef(null) + if (state.inventory) { + lastInventoryRef.current = state.inventory + } + const inventory = state.inventory ?? (state.loading ? lastInventoryRef.current : null) + const eligibleNames = useMemo(() => state.inventory?.eligibleUpdateNames ?? [], [state.inventory]) + // Display only. The action still fires `eligibleNames`, so a re-scan in flight + // can never authorize work — but the button keeps its place and its label + // instead of vanishing and reflowing the footer every time one runs. + const displayEligibleCount = inventory?.eligibleUpdateNames.length ?? 0 const isRunning = run.state === 'running' + // The kill sweep can take seconds; without this the Stop button sits enabled + // and inert, which reads as broken. + const isStopping = run.state === 'running' && run.stopping === true const showResult = run.state === 'success' || run.state === 'error' - const runNames = useMemo(() => (run.state === 'idle' ? [] : run.names), [run]) + // Keyed on the names themselves: every captured output chunk republishes the + // run, and regrouping the whole inventory per chunk would re-render each row. + const runNamesKey = run.state === 'idle' ? '' : run.names.join('\n') + const runNames = useMemo(() => (runNamesKey ? runNamesKey.split('\n') : []), [runNamesKey]) const groups = useMemo( () => inventory @@ -78,13 +102,18 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { ) const hasBlockedGroup = groups.some((group) => group.status === 'cannot-update') const blockedCount = groups.filter((group) => group.status === 'cannot-update').length - const summaryKind = summarizeInventory(inventory, hasBlockedGroup) + // Why: the headline reads the LIVE snapshot, not the retained one — the two + // disagree for the whole loading window, and pairing a retained "eligible" with + // a live count of 0 renders "0 updates available" over rows badged "Update + // available". Live means it says "Checking…" over the rows it kept on screen. + const summaryKind = summarizeInventory(state.inventory, hasBlockedGroup) // Why: one row list for every state. The rows are identical objects across the // transition, so pressing Update changes each row's leading icon in place // instead of swapping the dialog's body for a different component. + const failedNamesKey = run.state === 'error' ? run.failedNames.join('\n') : '' const rows = useMemo(() => { - const failed = new Set(run.state === 'error' ? run.failedNames : []) + const failed = new Set(failedNamesKey ? failedNamesKey.split('\n') : []) const inRun = new Set(runNames) return groups.map((group) => { if (inRun.has(group.name)) { @@ -98,7 +127,7 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { state: group.status === 'cannot-update' ? ('blocked' as const) : ('available' as const) } }) - }, [groups, isRunning, run, runNames]) + }, [groups, isRunning, failedNamesKey, runNames]) const handleOpenChange = (next: boolean): void => { if (next) { @@ -108,14 +137,20 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { // status-bar segment carries it from here. consumeSkillFreshnessUpdateDialogRequest() setCopied(false) + // Don't carry a finished session's rows into the next open — but a live run + // keeps its own, or reopening from the status segment mid-run would land on + // an empty list while the close's own re-scan is still reading disk. + if (run.state === 'idle') { + lastInventoryRef.current = null + } if (showResult) { void acknowledgeSkillUpdateRun() } notifyInstalledAgentSkillsChanged() } - const handleUpdate = (): void => { - void startSkillUpdateRun(eligibleNames) + const handleUpdate = (names: readonly string[]): void => { + void startSkillUpdateRun(names) } const handleCopyCommand = (): void => { @@ -125,13 +160,33 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { if (!command) { return } - void navigator.clipboard.writeText(command).then(() => { - setCopied(true) - setTimeout(() => setCopied(false), 2000) - }) + // Clipboard writes reject on a denied permission or an unfocused document; + // without this the button just never flips to "Copied". + void navigator.clipboard + .writeText(command) + .then(() => { + setCopied(true) + setTimeout(() => setCopied(false), 2000) + }) + .catch((error: unknown) => { + console.error('Failed to copy skill update command', error) + }) } const headline = ((): React.JSX.Element => { + if (isStopping) { + // Why: no "keeps running in the background" line here — after Stop that is + // the opposite of what is happening. + return ( +
+ + {translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.stoppingHeadline', + 'Stopping the update…' + )} +
+ ) + } if (isRunning) { return (
@@ -218,10 +273,17 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { // Indeterminate on purpose: the CLI reports no parseable progress.
@@ -250,7 +312,15 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { {run.message}

- + ) : showResult ? ( ) : ( ) : null} diff --git a/src/renderer/src/components/skills/SkillUpdateRow.tsx b/src/renderer/src/components/skills/SkillUpdateRow.tsx index 6b5a77382e0..3f069be5b54 100644 --- a/src/renderer/src/components/skills/SkillUpdateRow.tsx +++ b/src/renderer/src/components/skills/SkillUpdateRow.tsx @@ -113,13 +113,17 @@ export function SkillUpdateRow({

) : null} + {/* Why: outside the disclosure, alongside the failure line. The reason is + the whole point of a skipped row, so it must not depend on a click — + nor on a mount-time `defaultOpen` that a re-scan can't re-fire. */} + {state === 'blocked' ? ( +

+ {skippedReason(group.locations)} +

+ ) : null} +
- {state === 'blocked' ? ( -

- {skippedReason(group.locations)} -

- ) : null} {group.locations.map((location) => (
{/* Why: plugin-cache paths nest arbitrarily deep. Without an explicit diff --git a/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx b/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx index f4c28429a46..0347302c632 100644 --- a/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx +++ b/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx @@ -66,21 +66,15 @@ export function SummaryHeadline({ ) } if (kind === 'attention') { + // No follow-up sentence: skipped rows open themselves, so the reason is + // already on screen directly under this headline. return ( -
-
- - {translate( - 'auto.components.skills.SkillFreshnessUpdateDialog.attention', - 'Some installed Orca skills were left out of the update.' - )} -
-

- {translate( - 'auto.components.skills.SkillFreshnessUpdateDialog.attentionDescription', - 'Open Update details to see why each one was skipped.' - )} -

+
+ + {translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.attention', + 'Some installed Orca skills were left out of the update.' + )}
) } diff --git a/src/renderer/src/components/skills/skill-location-chip-copy.tsx b/src/renderer/src/components/skills/skill-location-chip-copy.tsx deleted file mode 100644 index 2d130d3b33a..00000000000 --- a/src/renderer/src/components/skills/skill-location-chip-copy.tsx +++ /dev/null @@ -1,143 +0,0 @@ -import type { SkillFreshnessGroupModel, SkillLocationChip } from './skill-freshness-grouping' -import { translate } from '@/i18n/i18n' -import { Badge } from '@/components/ui/badge' -import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' -import { skippedReason } from './skill-freshness-skipped-reason' - -function chipLabel(chip: SkillLocationChip): string { - switch (chip) { - case 'current': - return translate('auto.components.skills.SkillFreshnessRow.chipCurrent', 'Current') - case 'unrecognized': - return translate('auto.components.skills.SkillFreshnessRow.chipUnrecognized', 'Unrecognized') - case 'inaccessible': - return translate('auto.components.skills.SkillFreshnessRow.chipInaccessible', 'Inaccessible') - case 'duplicate': - return translate('auto.components.skills.SkillFreshnessRow.chipDuplicate', 'Duplicate') - case 'external-link': - return translate('auto.components.skills.SkillFreshnessRow.chipExternalLink', 'External link') - case 'broken-link': - return translate('auto.components.skills.SkillFreshnessRow.chipBrokenLink', 'Broken link') - case 'read-only': - return translate('auto.components.skills.SkillFreshnessRow.chipReadOnly', 'Read only') - case 'in-a-repo': - return translate('auto.components.skills.SkillFreshnessRow.chipInRepo', 'In a repo') - case 'plugin-cache': - return translate('auto.components.skills.SkillFreshnessRow.chipPluginCache', 'Plugin cache') - } -} - -// Why: chips describe only what a location *is*; the effect on the update -// command lives in the per-skill sentence, so the two never say it twice. -function chipTooltip(chip: SkillLocationChip): string { - switch (chip) { - case 'current': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipCurrent', - 'This copy matches the current official version.' - ) - case 'unrecognized': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipUnrecognized', - 'This copy doesn’t match any official version — it may be modified, or a different skill with the same name.' - ) - case 'inaccessible': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipInaccessible', - 'Orca couldn’t read this copy (a permissions or file error).' - ) - case 'duplicate': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipDuplicate', - 'A separate copy of this skill, installed apart from the main one.' - ) - case 'external-link': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipExternalLink', - 'A shortcut pointing outside Orca’s skill folders.' - ) - case 'broken-link': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipBrokenLink', - 'A shortcut to something that no longer exists.' - ) - case 'read-only': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipReadOnly', - 'This copy is in a read-only location.' - ) - case 'in-a-repo': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipInRepo', - 'This copy lives inside a project, not your global skills.' - ) - case 'plugin-cache': - return translate( - 'auto.components.skills.SkillFreshnessRow.tipPluginCache', - 'This copy is managed by a plugin.' - ) - } -} - -export function SkillFreshnessGroup({ - group -}: { - group: SkillFreshnessGroupModel -}): React.JSX.Element { - const isBlocked = group.status === 'cannot-update' - return ( -
-
- {group.name} - {isBlocked ? ( - - {translate('auto.components.skills.SkillFreshnessRow.statusCantUpdate', 'Skipped')} - - ) : ( - - {translate( - 'auto.components.skills.SkillFreshnessRow.statusUpdateAvailable', - 'Update available' - )} - - )} -
- {isBlocked ? ( -

{skippedReason(group.locations)}

- ) : null} -
- {group.locations.map((location) => ( -
- {/* Why: plugin-cache paths nest arbitrarily deep. Without an explicit - shrink basis the unbreakable string sets the dialog's width and - pushes the footer actions off-screen. */} - - {location.path} - - {location.chip ? ( - - - - {chipLabel(location.chip)} - - - - {chipTooltip(location.chip)} - - - ) : null} -
- ))} -
-
- ) -} diff --git a/src/renderer/src/components/skills/skill-update-run-store.ts b/src/renderer/src/components/skills/skill-update-run-store.ts index 6d5f32ecc57..601fda7da3f 100644 --- a/src/renderer/src/components/skills/skill-update-run-store.ts +++ b/src/renderer/src/components/skills/skill-update-run-store.ts @@ -1,6 +1,10 @@ import { useSyncExternalStore } from 'react' import type { SkillUpdateRun } from '../../../../shared/skill-freshness' import { notifyInstalledAgentSkillsChanged } from '@/hooks/useInstalledAgentSkills' +import { + getSkillFreshnessUpdateDialogRequest, + subscribeSkillFreshnessUpdateDialog +} from './skill-freshness-update-dialog' // Why: the run outlives the dialog — closing the window must not cancel it, and // the status-bar segment needs the same snapshot. Keeping it outside React means @@ -19,22 +23,42 @@ function emit(): void { } } -function setRun(next: SkillUpdateRun): void { - run = next +function clearSuccessTimer(): void { if (successTimer) { clearTimeout(successTimer) successTimer = null } - if (next.state === 'success') { - // Why: a success needs to be *seen*, then get out of the way. Errors stay - // until the user acts on them. - successTimer = setTimeout(() => { - successTimer = null - void window.api.skills.acknowledgeUpdateRun() - }, SKILL_UPDATE_SUCCESS_LINGER_MS) +} + +/** + * Why: a success needs to be *seen*, then get out of the way. Errors stay until + * the user acts on them. The dialog renders this same run, so retiring it while + * the dialog is open would yank the result rows out from under someone reading + * them — there, closing the dialog is what acknowledges the run. + */ +function scheduleSuccessLinger(): void { + clearSuccessTimer() + if (run.state !== 'success' || getSkillFreshnessUpdateDialogRequest()) { + return } - if (next.state === 'success' || next.state === 'error') { - // A finished run changes what's on disk; let every skills surface re-read. + successTimer = setTimeout(() => { + successTimer = null + void acknowledgeSkillUpdateRun() + }, SKILL_UPDATE_SUCCESS_LINGER_MS) +} + +// Opening the dialog on a lingering success hands ownership back to it. +subscribeSkillFreshnessUpdateDialog(scheduleSuccessLinger) + +function setRun(next: SkillUpdateRun): void { + const wasRunning = run.state === 'running' + run = next + scheduleSuccessLinger() + // A run that stopped changes what's on disk — including a cancelled one, which + // may have written several skills before the kill landed. Without this a Stop + // leaves the rows and the count describing the pre-run scan, so the Update + // button re-offers skills that already updated. + if (next.state === 'success' || next.state === 'error' || (wasRunning && next.state === 'idle')) { notifyInstalledAgentSkillsChanged() } emit() @@ -68,26 +92,38 @@ export function useSkillUpdateRun(): SkillUpdateRun { return useSyncExternalStore(subscribeSkillUpdateRun, getSkillUpdateRun, getSkillUpdateRun) } +// Why: every caller fires these from an event handler with `void`. Swallowing +// here rather than at each call site keeps a dropped IPC from surfacing as an +// unhandled rejection; the run state itself is pushed from main either way. export async function startSkillUpdateRun(names: readonly string[]): Promise { ensureSubscribed() - await window.api.skills.startUpdateRun([...names]) + try { + await window.api.skills.startUpdateRun([...names]) + } catch (error) { + console.error('Failed to start skill update run', error) + } } export async function cancelSkillUpdateRun(): Promise { - await window.api.skills.cancelUpdateRun() + try { + await window.api.skills.cancelUpdateRun() + } catch (error) { + console.error('Failed to cancel skill update run', error) + } } export async function acknowledgeSkillUpdateRun(): Promise { - await window.api.skills.acknowledgeUpdateRun() + try { + await window.api.skills.acknowledgeUpdateRun() + } catch (error) { + console.error('Failed to acknowledge skill update run', error) + } } /** @internal - tests need a clean module between cases. */ export function _resetSkillUpdateRunStore(): void { run = { state: 'idle' } subscribed = false - if (successTimer) { - clearTimeout(successTimer) - successTimer = null - } + clearSuccessTimer() listeners.clear() } diff --git a/src/renderer/src/components/status-bar/SkillUpdateStatusSegment.tsx b/src/renderer/src/components/status-bar/SkillUpdateStatusSegment.tsx index c7add3eff98..08e06083525 100644 --- a/src/renderer/src/components/status-bar/SkillUpdateStatusSegment.tsx +++ b/src/renderer/src/components/status-bar/SkillUpdateStatusSegment.tsx @@ -18,6 +18,25 @@ export function SkillUpdateStatusSegment({ } const segment = (() => { + if (run.state === 'running' && run.stopping) { + // The dialog can be closed while a Stop is still killing the tree; this is + // then the only surface reporting it, and "Updating skills" would be wrong. + return { + icon: , + label: translate( + 'auto.components.status.bar.SkillUpdateStatusSegment.stoppingLabel', + 'Stopping update' + ), + tooltip: translate( + 'auto.components.status.bar.SkillUpdateStatusSegment.stoppingTooltip', + 'Stopping the skill update…' + ), + ariaLabel: translate( + 'auto.components.status.bar.SkillUpdateStatusSegment.stoppingAria', + 'Stopping the skill update. Click to open details.' + ) + } + } if (run.state === 'running') { return { icon: , diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index c40e23dae27..d7f44831dbb 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3177,7 +3177,10 @@ "successAria": "Skills updated. Click to open details.", "errorLabel": "Update failed", "errorTooltip": "Skill update failed — click to see details", - "errorAria": "Skill update failed. Click to open details." + "errorAria": "Skill update failed. Click to open details.", + "stoppingLabel": "Stopping update", + "stoppingTooltip": "Stopping the skill update…", + "stoppingAria": "Stopping the skill update. Click to open details." }, "UpdateStatusSegment": { "5cd13105a3": "Update failed. Click to expand.", @@ -3792,7 +3795,6 @@ "blockedMany": "{{value0}} skills can't be updated automatically.", "success": "All installed Orca skills are up to date.", "attention": "Some installed Orca skills were left out of the update.", - "attentionDescription": "Open Update details to see why each one was skipped.", "runningOne": "Updating 1 skill…", "runningMany": "Updating {{value0}} skills…", "runningDescription": "You can close this window — it keeps running in the background.", @@ -3810,7 +3812,10 @@ "updateActionMany": "Update {{value0}} skills", "checkNow": "Re-check", "done": "Done", - "close": "Close" + "close": "Close", + "stop": "Stop", + "stopping": "Stopping…", + "stoppingHeadline": "Stopping the update…" }, "SkillUpdateResultRows": { "stillOutdated": "Still out of date after the update ran." diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 663635cb5e4..e0b9682ef8f 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -3356,7 +3356,10 @@ "successAria": "Skills updated. Click to open details.", "errorLabel": "Update failed", "errorTooltip": "Skill update failed — click to see details", - "errorAria": "Skill update failed. Click to open details." + "errorAria": "Skill update failed. Click to open details.", + "stoppingLabel": "Stopping update", + "stoppingTooltip": "Stopping the skill update…", + "stoppingAria": "Stopping the skill update. Click to open details." } } }, @@ -3767,7 +3770,6 @@ "updateMany": "Se pueden actualizar {{value0}} skills de forma segura", "success": "Todas las skills de Orca instaladas están actualizadas.", "attention": "Algunas skills de Orca instaladas no se pueden actualizar automáticamente.", - "attentionDescription": "Abre Detalles de la actualización para ver por qué no se puede actualizar cada una.", "checkNow": "Comprobar ahora", "close": "Cerrar", "blockedOne": "1 skill can't be updated automatically.", @@ -3787,7 +3789,10 @@ "updating": "Updating…", "updateActionOne": "Update 1 skill", "updateActionMany": "Update {{value0}} skills", - "done": "Done" + "done": "Done", + "stop": "Stop", + "stopping": "Stopping…", + "stoppingHeadline": "Stopping the update…" }, "SkillFreshnessStatusPill": { "updateAvailable": "Actualización disponible", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 5fd59120510..3786ee0ecb9 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -3356,7 +3356,10 @@ "successAria": "Skills updated. Click to open details.", "errorLabel": "Update failed", "errorTooltip": "Skill update failed — click to see details", - "errorAria": "Skill update failed. Click to open details." + "errorAria": "Skill update failed. Click to open details.", + "stoppingLabel": "Stopping update", + "stoppingTooltip": "Stopping the skill update…", + "stoppingAria": "Stopping the skill update. Click to open details." } } }, @@ -3767,7 +3770,6 @@ "updateMany": "{{value0}} 件のスキルを安全に更新できます", "success": "インストール済みの Orca スキルはすべて最新です。", "attention": "一部のインストール済み Orca スキルは自動的に更新できません。", - "attentionDescription": "更新できない理由は、更新の詳細を開いて確認してください。", "checkNow": "今すぐ確認", "close": "閉じる", "blockedOne": "1 skill can't be updated automatically.", @@ -3787,7 +3789,10 @@ "updating": "Updating…", "updateActionOne": "Update 1 skill", "updateActionMany": "Update {{value0}} skills", - "done": "Done" + "done": "Done", + "stop": "Stop", + "stopping": "Stopping…", + "stoppingHeadline": "Stopping the update…" }, "SkillFreshnessStatusPill": { "updateAvailable": "更新があります", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index b0b0e04ddff..2e91cbf3b50 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -3356,7 +3356,10 @@ "successAria": "Skills updated. Click to open details.", "errorLabel": "Update failed", "errorTooltip": "Skill update failed — click to see details", - "errorAria": "Skill update failed. Click to open details." + "errorAria": "Skill update failed. Click to open details.", + "stoppingLabel": "Stopping update", + "stoppingTooltip": "Stopping the skill update…", + "stoppingAria": "Stopping the skill update. Click to open details." } } }, @@ -3767,7 +3770,6 @@ "updateMany": "스킬 {{value0}}개를 안전하게 업데이트할 수 있습니다", "success": "설치된 Orca 스킬이 모두 최신 상태입니다.", "attention": "일부 설치된 Orca 스킬은 자동으로 업데이트할 수 없습니다.", - "attentionDescription": "각 항목을 업데이트할 수 없는 이유는 업데이트 세부 정보를 열어 확인하세요.", "checkNow": "지금 확인", "close": "닫기", "blockedOne": "1 skill can't be updated automatically.", @@ -3787,7 +3789,10 @@ "updating": "Updating…", "updateActionOne": "Update 1 skill", "updateActionMany": "Update {{value0}} skills", - "done": "Done" + "done": "Done", + "stop": "Stop", + "stopping": "Stopping…", + "stoppingHeadline": "Stopping the update…" }, "SkillFreshnessStatusPill": { "updateAvailable": "업데이트 가능", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 66aac7cd6c3..cce35de5de6 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -3356,7 +3356,10 @@ "successAria": "Skills updated. Click to open details.", "errorLabel": "Update failed", "errorTooltip": "Skill update failed — click to see details", - "errorAria": "Skill update failed. Click to open details." + "errorAria": "Skill update failed. Click to open details.", + "stoppingLabel": "Stopping update", + "stoppingTooltip": "Stopping the skill update…", + "stoppingAria": "Stopping the skill update. Click to open details." } } }, @@ -3767,7 +3770,6 @@ "updateMany": "可以安全更新 {{value0}} 个技能", "success": "所有已安装的 Orca 技能均为最新。", "attention": "部分已安装的 Orca 技能无法自动更新。", - "attentionDescription": "打开“更新详情”查看为何每一项都无法更新。", "checkNow": "立即检查", "close": "关闭", "blockedOne": "1 skill can't be updated automatically.", @@ -3787,7 +3789,10 @@ "updating": "Updating…", "updateActionOne": "Update 1 skill", "updateActionMany": "Update {{value0}} skills", - "done": "Done" + "done": "Done", + "stop": "Stop", + "stopping": "Stopping…", + "stoppingHeadline": "Stopping the update…" }, "SkillFreshnessStatusPill": { "updateAvailable": "有可用更新", diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 76e6562cc22..0de5570b4f5 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -111,7 +111,10 @@ export function buildTargetedSkillUpdateCommand(names: readonly string[]): strin // the inventory after exit, never from parsing stdout. export type SkillUpdateRun = | { state: 'idle' } - | { state: 'running'; names: string[]; startedAt: number; output: string } + // `stopping` covers the window between Stop and the process tree actually + // dying — the run is still `running` (that is what blocks a second writer), + // but the Stop affordance has already been spent. + | { state: 'running'; names: string[]; startedAt: number; output: string; stopping?: boolean } | { state: 'success'; names: string[]; finishedAt: number; output: string } | { state: 'error' @@ -125,4 +128,4 @@ export type SkillUpdateRun = export type SkillUpdateStartResult = | { started: true } - | { started: false; reason: 'already-running' | 'invalid-names' } + | { started: false; reason: 'already-running' | 'invalid-names' | 'unsafe-command-path' }