From 12472b8a63d640d196396e5d84e7da8c4ccf796a Mon Sep 17 00:00:00 2001 From: Wooseong Kim <2222333+innocarpe@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:43:22 +0900 Subject: [PATCH] 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 --- .../skills/skill-freshness-inventory.test.ts | 151 +++++++++++++++- src/main/skills/skill-freshness-inventory.ts | 5 +- .../skill-freshness-placement-observation.ts | 16 +- .../skills/skill-package-identity.test.ts | 170 +++++++++++++++--- src/main/skills/skill-package-identity.ts | 69 ++++++- .../skills/skill-update-convergence.test.ts | 112 ++++++------ src/main/skills/skill-update-convergence.ts | 39 ++-- src/main/skills/skill-update-outcome.ts | 7 +- src/main/skills/skill-update-registration.ts | 27 +++ src/shared/skill-freshness.ts | 7 + 10 files changed, 477 insertions(+), 126 deletions(-) diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index 2b20055e6..f2cb31e13 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -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' }) ]) ) diff --git a/src/main/skills/skill-freshness-inventory.ts b/src/main/skills/skill-freshness-inventory.ts index a37474518..f50270732 100644 --- a/src/main/skills/skill-freshness-inventory.ts +++ b/src/main/skills/skill-freshness-inventory.ts @@ -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 } diff --git a/src/main/skills/skill-freshness-placement-observation.ts b/src/main/skills/skill-freshness-placement-observation.ts index 87911de48..02c9dcf80 100644 --- a/src/main/skills/skill-freshness-placement-observation.ts +++ b/src/main/skills/skill-freshness-placement-observation.ts @@ -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 { diff --git a/src/main/skills/skill-package-identity.test.ts b/src/main/skills/skill-package-identity.test.ts index ca9beb9fd..7d362be9e 100644 --- a/src/main/skills/skill-package-identity.test.ts +++ b/src/main/skills/skill-package-identity.test.ts @@ -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 { return root } +/** A byte-identical folder elsewhere, so a hash can be derived without the original. */ +async function copyOf(source: string): Promise { + 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 () => { diff --git a/src/main/skills/skill-package-identity.ts b/src/main/skills/skill-package-identity.ts index 4c883a2d6..5610479a4 100644 --- a/src/main/skills/skill-package-identity.ts +++ b/src/main/skills/skill-package-identity.ts @@ -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 ): 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 { + const scoped = observed.treeEntries.filter((entry) => officialPaths.has(entry.path)) + return scoped.length === 0 || scoped.length === observed.treeEntries.length + ? observed.observedGitTreeSha + : skillPackageGitTreeSha(scoped) +} diff --git a/src/main/skills/skill-update-convergence.test.ts b/src/main/skills/skill-update-convergence.test.ts index 86e4f395e..3e8d2633a 100644 --- a/src/main/skills/skill-update-convergence.test.ts +++ b/src/main/skills/skill-update-convergence.test.ts @@ -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']) diff --git a/src/main/skills/skill-update-convergence.ts b/src/main/skills/skill-update-convergence.ts index 41889058f..c318e89d8 100644 --- a/src/main/skills/skill-update-convergence.ts +++ b/src/main/skills/skill-update-convergence.ts @@ -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 && diff --git a/src/main/skills/skill-update-outcome.ts b/src/main/skills/skill-update-outcome.ts index 9a15b8566..9c032cb9f 100644 --- a/src/main/skills/skill-update-outcome.ts +++ b/src/main/skills/skill-update-outcome.ts @@ -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) } diff --git a/src/main/skills/skill-update-registration.ts b/src/main/skills/skill-update-registration.ts index 66231d71f..e80b8dc05 100644 --- a/src/main/skills/skill-update-registration.ts +++ b/src/main/skills/skill-update-registration.ts @@ -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> { diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 24bbcc9df..dafc50472 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -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 }