fix(skills): match official skill files despite local sidecars (#12812)

* fix(skills): match official skill files despite local sidecars

Scope known-snapshot matching to manifest-listed files so agent-written
sidecars (e.g. agents/openai.yaml) no longer mark a package unrecognized
and block updates when official bytes still match.

Preserves fail-closed detection when a listed file's content drifts.

Fixes #12694

* fix(skills): scope lock trust and convergence to official files too

Sidecar tolerance stopped at the snapshot match, leaving three disk-vs-official
comparisons still judging the whole folder.

The lock-comparable hash covered every observed file, so a clean update beside
agents/openai.yaml reported as failed and read 'may be modified'. It is now
carried both whole and scoped to the current bundle's paths, and either may
satisfy the lock: the sidecar case only ever matches scoped, while an upstream
revision that ADDS a file only ever matches whole, so publishing one alone
would trade this bug for #11220.

Convergence re-derived the disk revision from that same whole-folder digest,
which no revision matches once a sidecar lands, retiring the stuck-lock gate
and arming an update the command provably cannot perform; it now honours the
revision observation already resolved.

Subset matching also let an older revision launder drift on a file the current
bundle lists, since that revision does not list it and so read it as a
neighbour. Identity now keys tolerance on what the current bundle owns.

---------

Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com>
This commit is contained in:
Wooseong Kim 2026-08-06 14:43:22 +09:00 committed by GitHub
parent ab1bf1aff5
commit 12472b8a63
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 477 additions and 126 deletions

View File

@ -274,6 +274,139 @@ describe('read-only skill freshness inventory', () => {
expect(inventory.eligibleUpdateNames).toEqual([])
})
it('scopes the lock hash to the current bundle, not every path a revision ever shipped', async () => {
// A file revision 1 shipped and the current revision dropped is a stale leftover, not
// part of what the updater installed. Scoping to the union of all revisions would drag
// it back into the hash on the accident of its name, and the copy the CLI just wrote
// would read "may be modified".
const test = await fixture()
const legacy = describeObservedSkillFile(
'references/legacy.md',
Buffer.from('dropped after rev 1\n'),
false
)
const registryPath = join(test.resourceRoot, 'skills', 'snapshot-registry.json')
const registry = JSON.parse(await readFile(registryPath, 'utf8'))
registry.skills['orca-cli'][0].files.push(legacy)
await writeFile(registryPath, `${JSON.stringify(registry, null, 2)}\n`)
const upstreamMarkdown = `${test.newerMarkdown}\nUpstream edit no bundle has shipped.\n`
const canonical = await test.writeSkill(
join(test.homeDir, '.agents', 'skills'),
upstreamMarkdown
)
// The lock records the source tree — SKILL.md alone, no leftover.
const sourceTree = await test.writeSkill(join(test.root, 'source'), upstreamMarkdown)
await writeSkillLockHash(test.homeDir, await gitTreeShaOf(sourceTree))
await mkdir(join(canonical, 'references'), { recursive: true })
await writeFile(join(canonical, 'references', 'legacy.md'), 'dropped after rev 1\n')
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations[0]).toMatchObject({
topology: 'canonical-copy',
status: 'newer-known'
})
expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('up-to-date')
})
it('trusts the updater lock for upstream bytes beside an agent CLI sidecar (#12694)', async () => {
// The reported folder shape after a successful update: `skills update` wrote
// source-repo HEAD no bundle knows yet, and Codex's own agents/openai.yaml sits
// beside it. The lock records the source tree — SKILL.md alone — so a folder hash
// taken over the sidecar too can never match it, and the copy the command just
// wrote would be reported as a failed update.
const test = await fixture()
const upstreamMarkdown = `${test.newerMarkdown}\nUpstream edit no bundle has shipped.\n`
const canonical = await test.writeSkill(
join(test.homeDir, '.agents', 'skills'),
upstreamMarkdown
)
const sourceTree = await test.writeSkill(join(test.root, 'source'), upstreamMarkdown)
await writeSkillLockHash(test.homeDir, await gitTreeShaOf(sourceTree))
await mkdir(join(canonical, 'agents'), { recursive: true })
await writeFile(join(canonical, 'agents', 'openai.yaml'), 'display_name: test\n')
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations[0]).toMatchObject({
topology: 'canonical-copy',
status: 'newer-known'
})
expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('up-to-date')
})
it('still trusts the lock when the upstream revision added a file (#11220 guard)', async () => {
// The other half of the sidecar fix: scoping the lock hash to official paths alone
// would drop a file upstream genuinely shipped, and that file IS in the lock's tree.
// A clean install would then read "may be modified" and its update run report failure.
const test = await fixture()
const upstreamMarkdown = `${test.newerMarkdown}\nUpstream edit no bundle has shipped.\n`
const canonical = await test.writeSkill(
join(test.homeDir, '.agents', 'skills'),
upstreamMarkdown
)
await mkdir(join(canonical, 'references'), { recursive: true })
await writeFile(
join(canonical, 'references', 'new.md'),
'Shipped upstream, no bundle knows it\n'
)
await writeSkillLockHash(test.homeDir, await gitTreeShaOf(canonical))
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations[0]).toMatchObject({
topology: 'canonical-copy',
status: 'newer-known'
})
expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('up-to-date')
})
it('still withholds an unwinnable update when a sidecar sits beside the stale copy', async () => {
// Recognising the copy is only half the job: the lock already names the revision the
// source has, so `skills update` would compare lock to source, see no work and write
// nothing. Offering it promises a button that can never finish. A sidecar must not
// make the placement unidentifiable and retire that guard.
const test = await fixture()
const canonical = await test.writeSkill(
join(test.homeDir, '.agents', 'skills'),
test.oldMarkdown
)
await mkdir(join(canonical, 'agents'), { recursive: true })
await writeFile(join(canonical, 'agents', 'openai.yaml'), 'display_name: test\n')
// The fixture's synthetic tree sha for revision 2 — the revision on disk is 1.
await writeSkillLockHash(test.homeDir, (2).toString(16).padStart(40, '0'))
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations[0]).toMatchObject({
topology: 'canonical-copy',
status: 'outdated',
installedReleaseRevision: 1
})
expect(inventory.eligibleUpdateNames).toEqual([])
})
it('still flags canonical bytes that do not match what the lock says was installed', async () => {
const test = await fixture()
const editedMarkdown = `${test.currentMarkdown}\nLocal edit the updater never wrote.\n`
@ -522,10 +655,14 @@ describe('read-only skill freshness inventory', () => {
expect(inventory.eligibleUpdateNames).toEqual([])
})
it('keeps a plugin-cache copy with known official files unrecognized', async () => {
it('reads a plugin-cache copy with untouched official files as current', async () => {
// The deliberate posture change behind #12694: an unlisted neighbour is not evidence
// of an edit, so the bytes Orca owns decide alone — here and in every scope, not just
// the canonical copy the updater writes. The drifted-SKILL.md case above still fails
// closed, which is what keeps "unrecognized" meaningful.
const test = await fixture()
await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown)
const modifiedRoot = join(
const withSidecarRoot = join(
test.homeDir,
'.codex',
'plugins',
@ -534,9 +671,9 @@ describe('read-only skill freshness inventory', () => {
'modified',
'orca-cli'
)
await mkdir(modifiedRoot, { recursive: true })
await writeFile(join(modifiedRoot, 'SKILL.md'), test.currentMarkdown)
await writeFile(join(modifiedRoot, 'README.md'), 'Modified official package\n')
await mkdir(withSidecarRoot, { recursive: true })
await writeFile(join(withSidecarRoot, 'SKILL.md'), test.currentMarkdown)
await writeFile(join(withSidecarRoot, 'README.md'), 'Neighbouring file Orca never shipped\n')
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
@ -548,8 +685,8 @@ describe('read-only skill freshness inventory', () => {
expect(inventory.installations).toEqual(
expect.arrayContaining([
expect.objectContaining({
unresolvedPath: modifiedRoot,
status: 'unrecognized'
unresolvedPath: withSidecarRoot,
status: 'current'
})
])
)

View File

@ -19,7 +19,7 @@ import {
} from './skill-freshness-placement-observation'
import { scanKnownPluginSkillCandidates } from './skill-plugin-cache-scan'
import { convergableSkillNames } from './skill-update-convergence'
import { readGloballyUpdatableSkillLocks } from './skill-update-registration'
import { matchesUpdaterLock, readGloballyUpdatableSkillLocks } from './skill-update-registration'
export const MAXIMUM_REPOSITORY_SKILL_ROOTS = 128
@ -35,8 +35,7 @@ function trustLockInstalledRevision(
): SkillFreshnessInstallation {
return installation.status === 'unrecognized' &&
SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) &&
installation.observedGitTreeSha != null &&
installation.observedGitTreeSha === globalSkillLocks.get(installation.name)
matchesUpdaterLock(installation, globalSkillLocks.get(installation.name))
? { ...installation, status: 'newer-known' }
: installation
}

View File

@ -8,7 +8,11 @@ import type {
} from '../../shared/skill-freshness'
import type { SkillScanRoot } from './skill-discovery-sources'
import type { SkillBundleArtifacts } from './skill-bundle-artifacts'
import { matchingKnownSnapshot, observeSkillPackage } from './skill-package-identity'
import {
matchingKnownSnapshot,
observeSkillPackage,
officialPathsGitTreeSha
} from './skill-package-identity'
import {
classifyHomeSkillTopology,
classifyUnsupportedSkillTopology,
@ -95,10 +99,9 @@ export async function observeSkillFreshnessInstallation(args: {
try {
const observed = await observeSkillPackage(args.topology.resolvedPath)
const matchedSnapshot = matchingKnownSnapshot(
observed,
knownSnapshots(args.artifacts, args.current)
)
const snapshots = knownSnapshots(args.artifacts, args.current)
const officialPaths = new Set(args.current.files.map((file) => file.path))
const matchedSnapshot = matchingKnownSnapshot(observed, snapshots, officialPaths)
// Why: a later release can reintroduce identical bytes. Exact current
// identity is still current, and cannot honestly be attributed to the later tag.
const snapshot =
@ -117,7 +120,8 @@ export async function observeSkillFreshnessInstallation(args: {
null)
: null,
observedPackageDigest: observed.observedDigest,
observedGitTreeSha: observed.observedGitTreeSha
observedGitTreeSha: observed.observedGitTreeSha,
observedOfficialGitTreeSha: officialPathsGitTreeSha(observed, officialPaths)
}
} catch (error) {
return {

View File

@ -1,4 +1,4 @@
import { chmod, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'
import { chmod, cp, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { afterEach, describe, expect, it } from 'vitest'
@ -6,6 +6,7 @@ import {
describeObservedSkillFile,
matchingKnownSnapshot,
observeSkillPackage,
officialPathsGitTreeSha,
skillPackageDigest
} from './skill-package-identity'
@ -17,6 +18,13 @@ async function temporarySkill(): Promise<string> {
return root
}
/** A byte-identical folder elsewhere, so a hash can be derived without the original. */
async function copyOf(source: string): Promise<string> {
const target = await temporarySkill()
await cp(source, target, { recursive: true })
return target
}
afterEach(async () => {
await Promise.all(temporaryDirectories.splice(0).map((root) => rm(root, { recursive: true })))
})
@ -32,14 +40,18 @@ describe('skill package identity', () => {
// hashes, not raw file buffers, should survive each file's identity pass.
expect(observed.files[0]).not.toHaveProperty('bytes')
expect(
matchingKnownSnapshot(observed, [
{
releaseRevision: 1,
packageDigest: skillPackageDigest([expected]),
gitTreeSha: 'tree',
files: [expected]
}
])?.releaseRevision
matchingKnownSnapshot(
observed,
[
{
releaseRevision: 1,
packageDigest: skillPackageDigest([expected]),
gitTreeSha: 'tree',
files: [expected]
}
],
new Set(['SKILL.md'])
)?.releaseRevision
).toBe(1)
})
@ -102,14 +114,18 @@ describe('skill package identity', () => {
// has to come out clean too, not just the digest.
expect(observed.observedGitTreeSha).toBe(official.observedGitTreeSha)
expect(
matchingKnownSnapshot(observed, [
{
releaseRevision: 1,
packageDigest: official.observedDigest,
gitTreeSha: official.observedGitTreeSha,
files: official.files
}
])?.releaseRevision
matchingKnownSnapshot(
observed,
[
{
releaseRevision: 1,
packageDigest: official.observedDigest,
gitTreeSha: official.observedGitTreeSha,
files: official.files
}
],
new Set(['SKILL.md'])
)?.releaseRevision
).toBe(1)
})
@ -141,11 +157,13 @@ describe('skill package identity', () => {
const edited = await temporarySkill()
await writeFile(join(edited, 'SKILL.md'), 'skill\nlocal tweak\n')
await writeFile(join(edited, '.DS_Store'), Buffer.from([0, 1]))
// An unexpected file that is NOT OS metadata must keep failing closed; tolerating
// extras in general would let an injected payload ride along beside a clean SKILL.md.
// Extra sidecar next to an untouched SKILL.md must still match — agent CLIs
// write agents/openai.yaml (and similar) without editing official bytes.
// Content drift on a listed file remains fail-closed.
const withPayload = await temporarySkill()
await writeFile(join(withPayload, 'SKILL.md'), 'skill\n')
await writeFile(join(withPayload, 'payload.sh'), '#!/bin/sh\n')
await mkdir(join(withPayload, 'agents'), { recursive: true })
await writeFile(join(withPayload, 'agents', 'openai.yaml'), 'display_name: test\n')
const snapshot = [
{
@ -155,8 +173,116 @@ describe('skill package identity', () => {
files: official.files
}
]
expect(matchingKnownSnapshot(await observeSkillPackage(edited), snapshot)).toBeNull()
expect(matchingKnownSnapshot(await observeSkillPackage(withPayload), snapshot)).toBeNull()
const official1 = new Set(['SKILL.md'])
expect(matchingKnownSnapshot(await observeSkillPackage(edited), snapshot, official1)).toBeNull()
expect(
matchingKnownSnapshot(await observeSkillPackage(withPayload), snapshot, official1)
?.releaseRevision
).toBe(1)
})
it('does not let an older revision launder drift on a file the current one lists', async () => {
// Subset matching treats a path the tested revision does not list as a neighbour.
// Left unguarded, revision 1 (SKILL.md alone) would match a revision-2 folder whose
// references/x.md had been rewritten — reporting a tampered package as merely
// outdated, with an update offered and no "may be modified" anywhere.
const pristine = await temporarySkill()
await writeFile(join(pristine, 'SKILL.md'), 'skill\n')
const revisionOne = await observeSkillPackage(pristine)
const tampered = await temporarySkill()
await writeFile(join(tampered, 'SKILL.md'), 'skill\n')
await mkdir(join(tampered, 'references'), { recursive: true })
await writeFile(join(tampered, 'references', 'x.md'), 'attacker content\n')
const snapshots = [
{
releaseRevision: 1,
packageDigest: revisionOne.observedDigest,
gitTreeSha: revisionOne.observedGitTreeSha,
files: revisionOne.files
}
]
const observed = await observeSkillPackage(tampered)
expect(
matchingKnownSnapshot(observed, snapshots, new Set(['SKILL.md', 'references/x.md']))
).toBeNull()
// The same folder is a plain sidecar case once the current bundle stops claiming
// that path, so the guard must key on what is official — not on file count.
expect(matchingKnownSnapshot(observed, snapshots, new Set(['SKILL.md']))?.releaseRevision).toBe(
1
)
})
it('hashes official paths for the lock without hiding an upstream file it added', async () => {
const clean = await temporarySkill()
await writeFile(join(clean, 'SKILL.md'), 'skill\n')
const source = await observeSkillPackage(clean)
const withSidecar = await temporarySkill()
await writeFile(join(withSidecar, 'SKILL.md'), 'skill\n')
await mkdir(join(withSidecar, 'agents'), { recursive: true })
await writeFile(join(withSidecar, 'agents', 'openai.yaml'), 'display_name: test\n')
const sidecarObserved = await observeSkillPackage(withSidecar)
const officialPaths = new Set(['SKILL.md'])
// A sidecar folder reaches the lock's source-tree hash only once scoped.
expect(sidecarObserved.observedGitTreeSha).not.toBe(source.observedGitTreeSha)
expect(officialPathsGitTreeSha(sidecarObserved, officialPaths)).toBe(source.observedGitTreeSha)
// An upstream revision that ADDS a file puts it in the lock's own tree, so the
// whole-folder hash has to survive alongside — scoping it away would re-break the
// clean install this check exists to recognise (#11220).
const upstream = await temporarySkill()
await writeFile(join(upstream, 'SKILL.md'), 'skill\n')
await mkdir(join(upstream, 'references'), { recursive: true })
await writeFile(join(upstream, 'references', 'new.md'), 'shipped upstream\n')
const upstreamObserved = await observeSkillPackage(upstream)
// The lock is the source tree the CLI installed, derived independently of the
// observation under test — comparing the observation to itself would assert nothing.
const upstreamLock = (await observeSkillPackage(await copyOf(upstream))).observedGitTreeSha
expect(officialPathsGitTreeSha(upstreamObserved, officialPaths)).not.toBe(upstreamLock)
expect(upstreamObserved.observedGitTreeSha).toBe(upstreamLock)
})
it('scopes the lock hash to the current bundle, not every path ever shipped', async () => {
// A file an older revision shipped and the current one dropped is a stale leftover,
// not part of what the updater installed. Scoping to the union of all revisions would
// drag it back into the hash purely because its name was once official, and the copy
// would read "may be modified" over bytes the CLI itself wrote.
const source = await temporarySkill()
await writeFile(join(source, 'SKILL.md'), 'skill\n')
const lock = (await observeSkillPackage(source)).observedGitTreeSha
const withLeftover = await temporarySkill()
await writeFile(join(withLeftover, 'SKILL.md'), 'skill\n')
await mkdir(join(withLeftover, 'references'), { recursive: true })
await writeFile(join(withLeftover, 'references', 'legacy.md'), 'dropped after rev 1\n')
const observed = await observeSkillPackage(withLeftover)
expect(officialPathsGitTreeSha(observed, new Set(['SKILL.md']))).toBe(lock)
// The union of every revision's paths — what scoping must NOT use.
expect(
officialPathsGitTreeSha(observed, new Set(['SKILL.md', 'references/legacy.md']))
).not.toBe(lock)
})
it('does not hand the lock an empty tree when no official path is present', async () => {
// git's empty-tree sha is a real, matchable value; returning it would let a lock that
// ever recorded an empty source tree vouch for any folder holding nothing official.
const root = await temporarySkill()
await mkdir(join(root, 'agents'), { recursive: true })
await writeFile(join(root, 'agents', 'openai.yaml'), 'display_name: test\n')
const observed = await observeSkillPackage(root)
expect(officialPathsGitTreeSha(observed, new Set(['SKILL.md']))).toBe(
observed.observedGitTreeSha
)
expect(officialPathsGitTreeSha(observed, new Set(['SKILL.md']))).not.toBe(
'4b825dc642cb6eb9a060e54bf8d69288fbee4904'
)
})
it.runIf(process.platform !== 'win32')('tracks executable mode in package identity', async () => {

View File

@ -14,8 +14,13 @@ type ObservedSkillFile = SkillBundleFileIdentity
export type ObservedSkillPackage = {
files: ObservedSkillFile[]
observedDigest: string
/** Git tree sha of the raw bytes — comparable against the updater lock's skillFolderHash. */
/**
* Git tree sha of every observed file's raw bytes one of the two values comparable
* against the updater lock's `skillFolderHash` (see `officialPathsGitTreeSha`).
*/
observedGitTreeSha: string
/** Retained so a subset of the observed paths can be re-hashed the same way. */
treeEntries: SkillGitTreeFileEntry[]
}
// Why: package identity compares a live user directory against a tree the generator read
@ -250,21 +255,40 @@ export async function observeSkillPackage(
return {
files,
observedDigest: skillPackageDigest(files),
observedGitTreeSha: skillPackageGitTreeSha(treeEntries)
observedGitTreeSha: skillPackageGitTreeSha(treeEntries),
treeEntries
}
}
/**
* The newest revision whose every listed file is present and byte-identical.
*
* `officialPaths` is what the CURRENT bundle says this skill owns, and it is what
* separates a tolerable neighbour from drift. Agent CLIs drop their own metadata
* beside an official SKILL.md (Codex writes `agents/openai.yaml` and cannot put it
* anywhere else), and a file no revision claims is not evidence the user edited
* anything so extras alone must not mark the package unrecognized.
*
* A file the current bundle DOES list is never an extra, even when the revision
* being tested predates it: without that guard an older snapshot would happily match
* a folder whose newer official file had been tampered with, laundering real drift
* into a clean "outdated" row. Listed-file drift still fails closed either way.
*/
export function matchingKnownSnapshot(
observed: ObservedSkillPackage,
snapshots: readonly SkillKnownSnapshot[]
snapshots: readonly SkillKnownSnapshot[],
officialPaths: ReadonlySet<string>
): SkillKnownSnapshot | null {
const observedByPath = new Map(observed.files.map((file) => [file.path, file]))
for (const snapshot of snapshots.toReversed()) {
if (snapshot.files.length !== observed.files.length) {
continue
}
const listed = new Set(snapshot.files.map((file) => file.path))
const launders = observed.files.some(
(file) => !listed.has(file.path) && officialPaths.has(file.path)
)
if (
snapshot.files.every((expected, index) => {
const actual = observed.files[index]
!launders &&
snapshot.files.every((expected) => {
const actual = observedByPath.get(expected.path)
return Boolean(actual && matchesFileIdentity(actual, expected))
})
) {
@ -273,3 +297,32 @@ export function matchingKnownSnapshot(
}
return null
}
/**
* The observed folder hashed as if it held only the files the current bundle owns.
*
* Compared against the updater lock's `skillFolderHash` ALONGSIDE the whole-folder
* hash, never instead of it. The two cover different halves and neither subsumes the
* other: the lock records the source tree, so a folder carrying a sidecar only ever
* matches scoped, while an upstream revision that ADDS a file puts that file in the
* lock's own tree and only ever matches whole. Publishing one and dropping the other
* would trade this bug for #11220 a clean install reading "may be modified" and its
* update run reporting failure.
*
* Scoped to the CURRENT entry rather than every revision ever shipped: a leftover from
* an older revision is exactly the stale byte the lock comparison should look past, and
* unioning historical paths would drag it back in on the accident of its name.
*
* A folder holding none of them yields the whole-folder hash rather than git's empty-tree
* sha, which is a real, matchable value: a lock that ever recorded an empty source tree
* would otherwise vouch for every such folder.
*/
export function officialPathsGitTreeSha(
observed: ObservedSkillPackage,
officialPaths: ReadonlySet<string>
): string {
const scoped = observed.treeEntries.filter((entry) => officialPaths.has(entry.path))
return scoped.length === 0 || scoped.length === observed.treeEntries.length
? observed.observedGitTreeSha
: skillPackageGitTreeSha(scoped)
}

View File

@ -2,13 +2,22 @@ import { describe, expect, it } from 'vitest'
import type { SkillFreshnessInstallation, SkillKnownSnapshot } from '../../shared/skill-freshness'
import { convergableSkillNames } from './skill-update-convergence'
// `installedReleaseRevision` is the revision observation resolved this copy to, and
// null is the honest "readable, but matching nothing we know". A null
// `observedPackageDigest` is the different case of a copy that could not be read.
//
// The digest defaults to the resolved revision's own, so these fixtures describe a
// folder holding nothing but official files. That keeps the revert signal honest:
// swapping resolution back to a digest lookup still passes every case below except
// the sidecar one, which is the only case that distinguishes the two mechanisms.
function placement(
name: string,
observedPackageDigest: string | null,
topology: SkillFreshnessInstallation['topology'] = 'canonical-copy'
installedReleaseRevision: number | null,
topology: SkillFreshnessInstallation['topology'] = 'canonical-copy',
observedPackageDigest: string | null = `digest-${installedReleaseRevision}`
): SkillFreshnessInstallation {
return {
id: `${name}:${observedPackageDigest}:${topology}`,
id: `${name}:${installedReleaseRevision}:${topology}`,
name,
rootId: 'home',
providers: [],
@ -19,7 +28,7 @@ function placement(
physicalIdentity: '1:1',
topology,
status: 'outdated',
installedReleaseRevision: null,
installedReleaseRevision,
installedAppVersion: null,
currentReleaseRevision: 8,
currentPackageDigest: 'digest-current',
@ -29,24 +38,22 @@ function placement(
}
}
function revision(packageDigest: string, gitTreeSha: string): SkillKnownSnapshot {
return { releaseRevision: 1, packageDigest, gitTreeSha, files: [] }
function revision(releaseRevision: number, gitTreeSha: string): SkillKnownSnapshot {
return { releaseRevision, packageDigest: `digest-${releaseRevision}`, gitTreeSha, files: [] }
}
const PRE_STUB = revision(1, 'f3727995')
const STUB = revision(2, '091d9bcc')
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')],
[placement('orca-linear', 1)],
new Map([['orca-linear', '091d9bcc']]),
{
'orca-linear': [
revision('digest-pre-stub', 'f3727995'),
revision('digest-stub', '091d9bcc')
]
}
{ 'orca-linear': [PRE_STUB, STUB] }
)
expect([...result]).toEqual([])
})
@ -55,42 +62,50 @@ describe('convergableSkillNames', () => {
// 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')],
[placement('orca-cli', 1)],
new Map([['orca-cli', 'aaaa1111']]),
{ 'orca-cli': [revision('digest-installed', 'aaaa1111')] }
{ 'orca-cli': [revision(1, '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')],
[placement('orca-cli', null)],
new Map([['orca-cli', 'aaaa1111']]),
{ 'orca-cli': [revision('digest-other', 'bbbb2222')] }
{ 'orca-cli': [revision(1, 'bbbb2222')] }
)
expect([...result]).toEqual(['orca-cli'])
})
it('keeps a skill with no observable placement', () => {
const result = convergableSkillNames(
[placement('orca-cli', null)],
[placement('orca-cli', null, 'canonical-copy', null)],
new Map([['orca-cli', 'aaaa1111']]),
{ 'orca-cli': [revision('digest-installed', 'aaaa1111')] }
{ 'orca-cli': [revision(1, 'aaaa1111')] }
)
expect([...result]).toEqual(['orca-cli'])
})
// Why: a sidecar an agent CLI dropped beside the official files makes the folder
// digest match no revision at all, so resolving disk content by that digest would
// read as unidentifiable and quietly re-arm the unwinnable update. Observation
// already placed this copy at the pre-stub revision; the gate must honour that.
it('drops a stale skill whose folder holds files no revision lists', () => {
const result = convergableSkillNames(
[placement('orca-linear', 1, 'canonical-copy', 'digest-with-sidecar')],
new Map([['orca-linear', '091d9bcc']]),
{ 'orca-linear': [PRE_STUB, STUB] }
)
expect([...result]).toEqual([])
})
// 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')],
[placement('orca-cli', 1), placement('orca-cli', 2)],
new Map([['orca-cli', 'aaaa1111']]),
{
'orca-cli': [
revision('digest-installed', 'aaaa1111'),
revision('digest-pre-stub', 'f3727995')
]
}
{ 'orca-cli': [revision(1, 'aaaa1111'), revision(2, 'f3727995')] }
)
expect([...result]).toEqual(['orca-cli'])
})
@ -98,23 +113,21 @@ describe('convergableSkillNames', () => {
// 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')],
[placement('orca-cli', 1)],
new Map([['orca-cli', 'not-a-known-tree']]),
{ 'orca-cli': [revision('digest-pre-stub', 'f3727995')] }
{ 'orca-cli': [revision(1, 'f3727995')] }
)
expect([...result]).toEqual(['orca-cli'])
})
// Why: `diskTreeShas` silently drops digests that match no known revision, so a
// Why: `diskTreeShas` silently drops placements that resolved to nothing, 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')],
[placement('orca-cli', 1), placement('orca-cli', null)],
new Map([['orca-cli', '091d9bcc']]),
{
'orca-cli': [revision('digest-pre-stub', 'f3727995'), revision('digest-stub', '091d9bcc')]
}
{ 'orca-cli': [PRE_STUB, STUB] }
)
expect([...result]).toEqual(['orca-cli'])
})
@ -124,17 +137,9 @@ describe('convergableSkillNames', () => {
// 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')
],
[placement('orca-linear', 1), placement('orca-linear', null, 'plugin-cache')],
new Map([['orca-linear', '091d9bcc']]),
{
'orca-linear': [
revision('digest-pre-stub', 'f3727995'),
revision('digest-stub', '091d9bcc')
]
}
{ 'orca-linear': [PRE_STUB, STUB] }
)
expect([...result]).toEqual([])
})
@ -143,34 +148,23 @@ describe('convergableSkillNames', () => {
// 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')
],
[placement('orca-linear', 1), placement('orca-linear', 2, 'plugin-cache')],
new Map([['orca-linear', '091d9bcc']]),
{
'orca-linear': [
revision('digest-pre-stub', 'f3727995'),
revision('digest-stub', '091d9bcc')
]
}
{ 'orca-linear': [PRE_STUB, STUB] }
)
expect([...result]).toEqual([])
})
it('judges each locked skill independently', () => {
const result = convergableSkillNames(
[placement('orca-linear', 'digest-pre-stub'), placement('orca-cli', 'digest-installed')],
[placement('orca-linear', 1), placement('orca-cli', 1)],
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')]
'orca-linear': [PRE_STUB, STUB],
'orca-cli': [revision(1, 'aaaa1111')]
}
)
expect([...result]).toEqual(['orca-cli'])

View File

@ -34,20 +34,27 @@ export function convergableSkillNames(
// 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) {
const observable = installations.filter(
(entry) =>
entry.name === name &&
SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(entry.topology) &&
entry.observedPackageDigest
)
if (observable.length === 0) {
continue
}
const revisions = knownSnapshots[name] ?? []
const diskTreeShas = digests
.map((digest) => revisions.find((revision) => revision.packageDigest === digest)?.gitTreeSha)
// Why: the revision each placement resolved to during observation, not a fresh
// lookup by whole-folder digest. Identity tolerates files the manifest never
// listed, so a folder holding an agent CLI's sidecar digests to nothing any
// revision knows — re-deriving here would call every such placement
// unidentifiable and quietly retire the gate for the users who most need it.
const diskTreeShas = observable
.map(
(entry) =>
revisions.find((revision) => revision.releaseRevision === entry.installedReleaseRevision)
?.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
@ -55,11 +62,11 @@ export function convergableSkillNames(
// 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
// `diskTreeShas` drops placements that resolved to no known revision, so requiring
// every observable one 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 === observable.length
if (
lockNamesAKnownRevision &&
everyPlacementResolved &&

View File

@ -2,6 +2,7 @@ import {
SUPPORTED_GLOBAL_SKILL_TOPOLOGIES,
type SkillFreshnessInstallation
} from '../../shared/skill-freshness'
import { matchesUpdaterLock } from './skill-update-registration'
/**
* Names that did not land, judged from the post-run inventory.
@ -53,9 +54,5 @@ function skillPlacementLanded(
// yet. Everything else unrecognized (half-written, no lock entry, mismatch)
// still fails, and `outdated` is never forgiven: lock == disk there means the
// command provably wrote nothing.
return (
entry.status === 'unrecognized' &&
lockHash !== undefined &&
entry.observedGitTreeSha === lockHash
)
return entry.status === 'unrecognized' && matchesUpdaterLock(entry, lockHash)
}

View File

@ -1,6 +1,7 @@
import { readFile } from 'node:fs/promises'
import { homedir } from 'node:os'
import { join } from 'node:path'
import type { SkillFreshnessInstallation } from '../../shared/skill-freshness'
const GLOBAL_SKILL_LOCK_SCHEMA_VERSION = 3
@ -21,6 +22,32 @@ function globalSkillLockPath(args: SkillUpdateRegistrationArgs): string {
: join(args.homeDir ?? homedir(), '.agents', '.skill-lock.json')
}
/**
* Whether a placement holds exactly the bytes the updater recorded installing.
*
* Either hash may carry it. The lock is the git tree sha of the source folder, so a
* folder an agent CLI dropped a sidecar into only matches once that sidecar is scoped
* out, while an upstream revision that added a file only matches whole that file is
* in the lock's own tree but in no bundled snapshot. Requiring one specific hash trades
* either #12694 or #11220 for the other.
*
* Not exhaustive, and deliberately so: a folder carrying BOTH a sidecar and an upstream
* file no bundle lists matches neither hash and stays unrecognized, exactly as it does
* without this pair. Closing that needs the sidecar gone before hashing an observe-time
* exclusion like `isOsMetadataSkillEntryName`, which means naming the foreign paths rather
* than tolerating any unlisted one. Editing a listed file still matches neither hash.
*/
export function matchesUpdaterLock(
installation: SkillFreshnessInstallation,
lockHash: string | undefined
): boolean {
return (
lockHash !== undefined &&
(installation.observedGitTreeSha === lockHash ||
installation.observedOfficialGitTreeSha === lockHash)
)
}
export async function readGloballyUpdatableSkillNames(
args: SkillUpdateRegistrationArgs = {}
): Promise<ReadonlySet<string>> {

View File

@ -83,6 +83,13 @@ export type SkillFreshnessInstallation = {
observedPackageDigest: string | null
/** Git tree sha of the observed bytes; lets the post-run verdict match disk against the updater's lock. */
observedGitTreeSha?: string | null
/**
* The same hash over only the files the current bundle lists. Carried beside the
* whole-folder hash, not in place of it, so a folder holding an agent CLI's sidecar
* can still match the lock without blinding the check to an upstream revision that
* added a file. Absent from hosts older than this field.
*/
observedOfficialGitTreeSha?: string | null
errorCategory: string | null
}