fix(skills): stop offering an update the CLI provably cannot perform (#11110)
* Revert "fix(skills): stop reporting a failed update when the CLI succeeded (#11105)" This reverts commita8660839ee. * 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 revertsa8660839ee, 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.
This commit is contained in:
Brennan Benson
GitHub
parent
48e31b3fc0
commit
7a3df87994
|
|
@ -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<SkillFreshnessInventory> {
|
||||
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()
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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'])
|
||||
})
|
||||
})
|
||||
|
|
@ -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<string, string>,
|
||||
knownSnapshots: Readonly<Record<string, SkillKnownSnapshot[]>>
|
||||
): ReadonlySet<string> {
|
||||
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
|
||||
}
|
||||
|
|
@ -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'])
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -24,6 +24,20 @@ function globalSkillLockPath(args: SkillUpdateRegistrationArgs): string {
|
|||
export async function readGloballyUpdatableSkillNames(
|
||||
args: SkillUpdateRegistrationArgs = {}
|
||||
): Promise<ReadonlySet<string>> {
|
||||
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<ReadonlyMap<string, string>> {
|
||||
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()
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue