fix(skills): stop reporting a failed update when the CLI succeeded (#11105)

The Update skills dialog showed "The update didn't finish" / "Some skills
could not be updated" with an armed Retry, directly above the runner's own
log line saying "All global skills are up to date" — on a clean exit 0.

`skills update` compares its lock's recorded hash against the source and
never reads disk (dist/cli.mjs: `latestHash !== entry.skillFolderHash`).
Once the lock has advanced past the installed bytes it prints up-to-date,
exits 0 and writes nothing. The copy left behind is a recognised older
revision, so the post-run re-scan sees `outdated`, skillUpdateFailedNames
counted that as a failed run, and skill-update-run settled to state 'error'.
Retry re-ran the same command, which no-op'd again — a closed loop.

Reclassify `outdated` as "the command did not converge this", not "the run
failed". The freshness badge still marks the copy not-current, so nothing is
hidden; the run just stops being blamed for it.

A 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 still surfaces via the spawn error.

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.
This commit is contained in:
Brennan Benson 2026-07-27 23:26:54 -07:00 committed by GitHub
parent 700cde83e0
commit a8660839ee
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 38 additions and 8 deletions

View File

@ -38,10 +38,13 @@ describe('skillUpdateFailedNames', () => {
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'
])
// 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 half-written bundle instead of reading it as success', () => {
@ -76,20 +79,31 @@ describe('skillUpdateFailedNames', () => {
).toEqual([])
})
it('fails the name when any convergent alias was left behind', () => {
// 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', () => {
expect(
skillUpdateFailedNames(
['orca-cli'],
[placement('orca-cli', 'current'), placement('orca-cli', 'outdated', 'provider-alias')]
)
).toEqual(['orca-cli'])
).toEqual([])
})
it('judges each requested name independently', () => {
expect(
skillUpdateFailedNames(
['orca-cli', 'orchestration'],
[placement('orca-cli', 'current'), placement('orchestration', 'outdated')]
[placement('orca-cli', 'current'), placement('orchestration', 'unrecognized')]
)
).toEqual(['orchestration'])
})

View File

@ -34,6 +34,22 @@ 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.
return convergent.some((entry) => entry.status !== 'current' && entry.status !== 'newer-known')
//
// `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'
)
})
}