From 7a3df879941b2b264121e411274ba393f88ac0fa Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 00:13:34 -0700 Subject: [PATCH] fix(skills): stop offering an update the CLI provably cannot perform (#11110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert "fix(skills): stop reporting a failed update when the CLI succeeded (#11105)" This reverts commit a8660839ee734714d4236fd00d960cfde9d97353. * fix(skills): stop offering an update the CLI provably cannot perform The Update skills dialog reported "The update didn't finish" / "Some skills could not be updated" with an armed Retry, directly above the runner's own "All global skills are up to date" log line, on a clean exit 0. `skills update` decides what to do by comparing its lock's `skillFolderHash` against the source tree and never reads disk (published CLI, dist/cli.mjs: `latestHash !== entry.skillFolderHash`). So once the lock records a revision the filesystem does not actually have, it reports up-to-date, exits 0 and writes nothing. No retry converges it. Gate eligibility on that instead of blaming the run afterwards: a name stays updatable only while the lock's hash and the revision the DISK hashes to agree. Those that disagree fall through to `needs-attention`, which skill-freshness-display-status.ts already documents as the state for a copy "out of date somewhere the update command cannot reach". The dialog no longer offers them, so it can neither claim failure nor claim success. Deliberately compared against disk, not the bundled manifest: the updater pulls from the source repo, which legitimately runs ahead of what a build ships, and gating on the bundle would withhold real updates. Both sides must also be positively identified — an unplaceable lock hash is not evidence. Also reverts a8660839ee, which forgave `outdated` post-run. That turned the false failure into a false success ("Updated 2 skills" over copies nothing wrote) and let an empty verdict swallow spawnError, so an offline run published green. Reachable by anyone who updated during the stub-conversion window — every bundled skill has a stub -> full -> stub oscillation in its last three registry revisions. * fix(skills): do not gate a skill when a placement is unidentifiable `diskTreeShas` drops digests matching no known revision, so `every` ran over only the resolved half — one stale copy beside one unidentifiable copy gated the name, contradicting the unknown-stays-eligible rule the function documents. Require every observed digest to resolve before treating all placements as mismatched. Caught by CodeRabbit on #11110. * fix(skills): judge convergence only over placements the update command writes A same-name plugin-cache or repo copy could defeat the gate two ways: an unidentifiable repack read as an unresolved placement, and a cache copy parked at the lock's own revision read as an anchor — both re-arming the unwinnable update on a drifted canonical. Filter to SUPPORTED_GLOBAL_SKILL_TOPOLOGIES, matching eligibility and outcome. --- src/main/skills/skill-freshness-inventory.ts | 12 +- .../skills/skill-update-convergence.test.ts | 178 ++++++++++++++++++ src/main/skills/skill-update-convergence.ts | 73 +++++++ src/main/skills/skill-update-outcome.test.ts | 28 +-- src/main/skills/skill-update-outcome.ts | 18 +- src/main/skills/skill-update-registration.ts | 24 ++- 6 files changed, 287 insertions(+), 46 deletions(-) create mode 100644 src/main/skills/skill-update-convergence.test.ts create mode 100644 src/main/skills/skill-update-convergence.ts diff --git a/src/main/skills/skill-freshness-inventory.ts b/src/main/skills/skill-freshness-inventory.ts index 5982c878f..41467b11e 100644 --- a/src/main/skills/skill-freshness-inventory.ts +++ b/src/main/skills/skill-freshness-inventory.ts @@ -17,7 +17,8 @@ import { type CandidateLstat } from './skill-freshness-placement-observation' import { scanKnownPluginSkillCandidates } from './skill-plugin-cache-scan' -import { readGloballyUpdatableSkillNames } from './skill-update-registration' +import { convergableSkillNames } from './skill-update-convergence' +import { readGloballyUpdatableSkillLocks } from './skill-update-registration' export const MAXIMUM_REPOSITORY_SKILL_ROOTS = 128 @@ -42,9 +43,9 @@ export async function inventorySkillFreshness(args: { candidateLstat?: CandidateLstat stateHome?: string | null }): Promise { - const [artifacts, globallyUpdatableNames] = await Promise.all([ + const [artifacts, globalSkillLocks] = await Promise.all([ loadSkillBundleArtifacts(args.resourceRoot), - readGloballyUpdatableSkillNames({ homeDir: args.homeDir, stateHome: args.stateHome }) + readGloballyUpdatableSkillLocks({ homeDir: args.homeDir, stateHome: args.stateHome }) ]) const currentByName = new Map(artifacts.manifest.skills.map((skill) => [skill.name, skill])) const discoveryArgs = { @@ -168,7 +169,10 @@ export async function inventorySkillFreshness(args: { return { schemaVersion: 1, installations, - eligibleUpdateNames: eligibleSkillUpdateNames(installations, globallyUpdatableNames), + eligibleUpdateNames: eligibleSkillUpdateNames( + installations, + convergableSkillNames(installations, globalSkillLocks, artifacts.knownSnapshots) + ), scanIssues, scannedAt: Date.now() } diff --git a/src/main/skills/skill-update-convergence.test.ts b/src/main/skills/skill-update-convergence.test.ts new file mode 100644 index 000000000..86e4f395e --- /dev/null +++ b/src/main/skills/skill-update-convergence.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it } from 'vitest' +import type { SkillFreshnessInstallation, SkillKnownSnapshot } from '../../shared/skill-freshness' +import { convergableSkillNames } from './skill-update-convergence' + +function placement( + name: string, + observedPackageDigest: string | null, + topology: SkillFreshnessInstallation['topology'] = 'canonical-copy' +): SkillFreshnessInstallation { + return { + id: `${name}:${observedPackageDigest}:${topology}`, + name, + rootId: 'home', + providers: [], + sourceKind: 'home', + sourceLabel: 'home', + unresolvedPath: `~/.agents/skills/${name}`, + resolvedPath: `/home/u/.agents/skills/${name}`, + physicalIdentity: '1:1', + topology, + status: 'outdated', + installedReleaseRevision: null, + installedAppVersion: null, + currentReleaseRevision: 8, + currentPackageDigest: 'digest-current', + currentAppVersion: '1.4.160', + observedPackageDigest, + errorCategory: null + } +} + +function revision(packageDigest: string, gitTreeSha: string): SkillKnownSnapshot { + return { releaseRevision: 1, packageDigest, gitTreeSha, files: [] } +} + +describe('convergableSkillNames', () => { + // The real reported case: the lock records the stub tree (091d9bcc) while disk + // still holds the pre-stub revision (f3727995). `skills update` compares lock to + // source, sees no work, exits 0 and writes nothing — forever. + it('drops a skill whose lock records a revision the disk does not have', () => { + const result = convergableSkillNames( + [placement('orca-linear', 'digest-pre-stub')], + new Map([['orca-linear', '091d9bcc']]), + { + 'orca-linear': [ + revision('digest-pre-stub', 'f3727995'), + revision('digest-stub', '091d9bcc') + ] + } + ) + expect([...result]).toEqual([]) + }) + + // The legitimate case that must NOT be gated: lock and disk agree, and the source + // has simply moved ahead of what this build bundles. The update really can converge. + it('keeps a skill whose lock matches disk even when it is outdated', () => { + const result = convergableSkillNames( + [placement('orca-cli', 'digest-installed')], + new Map([['orca-cli', 'aaaa1111']]), + { 'orca-cli': [revision('digest-installed', 'aaaa1111')] } + ) + expect([...result]).toEqual(['orca-cli']) + }) + + it('keeps a skill whose disk content matches no known revision', () => { + const result = convergableSkillNames( + [placement('orca-cli', 'digest-unknown')], + new Map([['orca-cli', 'aaaa1111']]), + { 'orca-cli': [revision('digest-other', 'bbbb2222')] } + ) + expect([...result]).toEqual(['orca-cli']) + }) + + it('keeps a skill with no observable placement', () => { + const result = convergableSkillNames( + [placement('orca-cli', null)], + new Map([['orca-cli', 'aaaa1111']]), + { 'orca-cli': [revision('digest-installed', 'aaaa1111')] } + ) + expect([...result]).toEqual(['orca-cli']) + }) + + // One placement still matching the lock means the command has an anchor to write. + it('keeps a skill when any placement still matches the lock', () => { + const result = convergableSkillNames( + [placement('orca-cli', 'digest-installed'), placement('orca-cli', 'digest-pre-stub')], + new Map([['orca-cli', 'aaaa1111']]), + { + 'orca-cli': [ + revision('digest-installed', 'aaaa1111'), + revision('digest-pre-stub', 'f3727995') + ] + } + ) + expect([...result]).toEqual(['orca-cli']) + }) + + // A lock hash we cannot place is not evidence the command is stuck. + it('keeps a skill whose lock names no revision we know', () => { + const result = convergableSkillNames( + [placement('orca-cli', 'digest-pre-stub')], + new Map([['orca-cli', 'not-a-known-tree']]), + { 'orca-cli': [revision('digest-pre-stub', 'f3727995')] } + ) + expect([...result]).toEqual(['orca-cli']) + }) + + // Why: `diskTreeShas` silently drops digests that match no known revision, so a + // stale copy sitting beside an unidentifiable one must NOT gate the name — the + // unknown half could be anything, including a copy the command would converge. + it('keeps a skill when one placement is stale but another is unidentifiable', () => { + const result = convergableSkillNames( + [placement('orca-cli', 'digest-pre-stub'), placement('orca-cli', 'digest-unknown')], + new Map([['orca-cli', '091d9bcc']]), + { + 'orca-cli': [revision('digest-pre-stub', 'f3727995'), revision('digest-stub', '091d9bcc')] + } + ) + expect([...result]).toEqual(['orca-cli']) + }) + + // Why: copies the command never writes must not defeat the gate. An + // unidentifiable plugin-cache repack would otherwise read as an unresolved + // placement and re-arm the unwinnable update on the drifted canonical. + it('ignores an unidentifiable plugin-cache copy when judging the canonical', () => { + const result = convergableSkillNames( + [ + placement('orca-linear', 'digest-pre-stub'), + placement('orca-linear', 'digest-cache-repack', 'plugin-cache') + ], + new Map([['orca-linear', '091d9bcc']]), + { + 'orca-linear': [ + revision('digest-pre-stub', 'f3727995'), + revision('digest-stub', '091d9bcc') + ] + } + ) + expect([...result]).toEqual([]) + }) + + // A cache copy parked at the lock's own revision is not an anchor either — + // the command only writes the canonical, which is still drifted. + it('ignores a plugin-cache copy that matches the lock', () => { + const result = convergableSkillNames( + [ + placement('orca-linear', 'digest-pre-stub'), + placement('orca-linear', 'digest-stub', 'plugin-cache') + ], + new Map([['orca-linear', '091d9bcc']]), + { + 'orca-linear': [ + revision('digest-pre-stub', 'f3727995'), + revision('digest-stub', '091d9bcc') + ] + } + ) + expect([...result]).toEqual([]) + }) + + it('judges each locked skill independently', () => { + const result = convergableSkillNames( + [placement('orca-linear', 'digest-pre-stub'), placement('orca-cli', 'digest-installed')], + new Map([ + ['orca-linear', '091d9bcc'], + ['orca-cli', 'aaaa1111'] + ]), + { + 'orca-linear': [ + revision('digest-pre-stub', 'f3727995'), + revision('digest-stub', '091d9bcc') + ], + 'orca-cli': [revision('digest-installed', 'aaaa1111')] + } + ) + expect([...result]).toEqual(['orca-cli']) + }) +}) diff --git a/src/main/skills/skill-update-convergence.ts b/src/main/skills/skill-update-convergence.ts new file mode 100644 index 000000000..41889058f --- /dev/null +++ b/src/main/skills/skill-update-convergence.ts @@ -0,0 +1,73 @@ +import { + SUPPORTED_GLOBAL_SKILL_TOPOLOGIES, + type SkillFreshnessInstallation, + type SkillKnownSnapshot +} from '../../shared/skill-freshness' + +/** + * Names `skills update` can still move, judged against what it believes it installed. + * + * The updater decides what to do by comparing its lock's `skillFolderHash` against + * the source tree; it never reads disk. So once the lock records a revision the + * filesystem does not actually have, the command reports "up to date", exits 0 and + * writes nothing — no retry converges it. Offering an update there promises work the + * command cannot do, which is exactly what `eligibleSkillUpdateNames` exists to avoid. + * + * The honest signal is the lock's hash versus the `gitTreeSha` of the revision the + * DISK hashes to. Deliberately not versus the bundled manifest: the updater pulls from + * the source repo, which legitimately runs ahead of what this build ships, and gating + * on the bundle would withhold real updates. When lock and disk agree, an update is + * still genuinely available and stays on offer. + * + * Unknown either way (no lock entry, or disk content matching no known revision) is + * left eligible — silence is not evidence the command is stuck. + */ +export function convergableSkillNames( + installations: readonly SkillFreshnessInstallation[], + globalSkillLocks: ReadonlyMap, + knownSnapshots: Readonly> +): ReadonlySet { + const convergable = new Set(globalSkillLocks.keys()) + for (const [name, lockHash] of globalSkillLocks) { + // Why: judged only over the placements the command writes, like eligibility + // itself. A plugin-cache or repo copy is never the command's to converge, so + // it must neither gate the name nor rescue it — an unidentifiable cache copy + // (or one parked at the lock's own revision) would otherwise defeat the gate + // and re-arm the unwinnable update. + const digests = installations + .filter( + (entry) => + entry.name === name && + SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(entry.topology) && + entry.observedPackageDigest + ) + .map((entry) => entry.observedPackageDigest) + if (digests.length === 0) { + continue + } + const revisions = knownSnapshots[name] ?? [] + const diskTreeShas = digests + .map((digest) => revisions.find((revision) => revision.packageDigest === digest)?.gitTreeSha) + .filter((sha): sha is string => Boolean(sha)) + // Why: only claim the lock is stale when BOTH sides are positively identified — + // the lock names a revision we know, and every placement resolves to a different + // known revision. A lock hash we cannot place (a source we do not bundle, a + // revision older than the registry) is not evidence of anything, and gating on it + // would withhold updates that would have worked. + const lockNamesAKnownRevision = revisions.some((entry) => entry.gitTreeSha === lockHash) + // `diskTreeShas` drops digests that match no known revision, so requiring every + // observed digest to resolve is what keeps `every` honest: without it, one stale + // copy beside one unidentifiable copy would gate the name off the resolved half + // alone, contradicting the unknown-stays-eligible rule above. + const everyPlacementResolved = diskTreeShas.length === digests.length + if ( + lockNamesAKnownRevision && + everyPlacementResolved && + diskTreeShas.length > 0 && + diskTreeShas.every((sha) => sha !== lockHash) + ) { + convergable.delete(name) + } + } + return convergable +} diff --git a/src/main/skills/skill-update-outcome.test.ts b/src/main/skills/skill-update-outcome.test.ts index 9f7d809da..4aa2ce401 100644 --- a/src/main/skills/skill-update-outcome.test.ts +++ b/src/main/skills/skill-update-outcome.test.ts @@ -38,13 +38,10 @@ describe('skillUpdateFailedNames', () => { expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'current')])).toEqual([]) }) - // Reversed deliberately. `skills update` compares its lock against the source - // and never reads disk, so once the lock has advanced past the installed bytes - // it prints "up to date", exits 0 and writes nothing — leaving a recognised - // older revision that no retry converges. Blaming the run for that invented a - // failure the CLI never reported and armed a Retry that could not succeed. - it('does not blame the run for a copy the update command cannot converge', () => { - expect(skillUpdateFailedNames(['orca-cli'], [placement('orca-cli', 'outdated')])).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', () => { @@ -79,31 +76,20 @@ describe('skillUpdateFailedNames', () => { ).toEqual([]) }) - // Still fails on a DEGRADED alias — only `outdated` was reclassified, so a - // half-written alias beside a good canonical copy must not read as success. - it('fails the name when any convergent alias was left broken', () => { - expect( - skillUpdateFailedNames( - ['orca-cli'], - [placement('orca-cli', 'current'), placement('orca-cli', 'unrecognized', 'provider-alias')] - ) - ).toEqual(['orca-cli']) - }) - - it('does not fail the name for an alias the command merely left old', () => { + 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([]) + ).toEqual(['orca-cli']) }) it('judges each requested name independently', () => { expect( skillUpdateFailedNames( ['orca-cli', 'orchestration'], - [placement('orca-cli', 'current'), placement('orchestration', 'unrecognized')] + [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 index 77a005446..6d75d4b5e 100644 --- a/src/main/skills/skill-update-outcome.ts +++ b/src/main/skills/skill-update-outcome.ts @@ -34,22 +34,6 @@ export function skillUpdateFailedNames( } // `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. - // - // `outdated` is not a failed run either. `skills update` compares its lock's - // recorded hash against the source and never reads disk, so when the lock has - // advanced past the installed bytes it reports "up to date", exits 0, and - // writes nothing. The copy is left intact — a recognised older revision, not - // a broken one — and no amount of retrying converges it. Calling that an - // error invented a failure the CLI never reported and armed a Retry that - // provably could not succeed. The freshness badge still marks the copy - // not-current, so this only stops the run being blamed for it. - // - // A genuinely botched write is still caught: a half-written bundle hashes to - // `unrecognized`, a wholly-degraded or removed copy leaves no convergent - // placement, and process-level failure surfaces through the spawn error. - return convergent.some( - (entry) => - entry.status !== 'current' && entry.status !== 'newer-known' && entry.status !== 'outdated' - ) + return convergent.some((entry) => entry.status !== 'current' && entry.status !== 'newer-known') }) } diff --git a/src/main/skills/skill-update-registration.ts b/src/main/skills/skill-update-registration.ts index 658679dac..66231d71f 100644 --- a/src/main/skills/skill-update-registration.ts +++ b/src/main/skills/skill-update-registration.ts @@ -24,6 +24,20 @@ function globalSkillLockPath(args: SkillUpdateRegistrationArgs): string { export async function readGloballyUpdatableSkillNames( args: SkillUpdateRegistrationArgs = {} ): Promise> { + return new Set((await readGloballyUpdatableSkillLocks(args)).keys()) +} + +/** + * Each updatable skill's recorded `skillFolderHash`, keyed by name. + * + * The hash is what the updater believes it installed. It is deliberately + * exposed alongside the names because `skills update` decides what to do by + * comparing this against the source and never reads disk — so when it disagrees + * with the bytes actually on disk, the command can only no-op. + */ +export async function readGloballyUpdatableSkillLocks( + args: SkillUpdateRegistrationArgs = {} +): Promise> { try { const parsed = JSON.parse(await readFile(globalSkillLockPath(args), 'utf8')) as { version?: unknown @@ -36,10 +50,10 @@ export async function readGloballyUpdatableSkillNames( typeof parsed.skills !== 'object' || Array.isArray(parsed.skills) ) { - return new Set() + return new Map() } - return new Set( + return new Map( Object.entries(parsed.skills) .filter(([, value]) => { if (!value || typeof value !== 'object' || Array.isArray(value)) { @@ -59,9 +73,11 @@ export async function readGloballyUpdatableSkillNames( entry.source.length > 0 ) }) - .map(([name]) => name) + .map( + ([name, value]) => [name, (value as { skillFolderHash: string }).skillFolderHash] as const + ) ) } catch { - return new Set() + return new Map() } }