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() } }