From 49cfbf014c417396aad442e2c3a09ef9d7c79b8c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 30 Jul 2026 13:20:47 -0700 Subject: [PATCH] fix(skills): stop OS sidecars marking an untouched skill as modified (#11471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): stop OS sidecars marking an untouched skill as modified Package identity compared a live user directory against a tree read from a clean checkout, so anything the OS deposited counted as drift. One Finder visit writes .DS_Store, which sorts before SKILL.md and misaligns the index-aligned snapshot comparison — the copy became 'unrecognized', was reported as "may be modified... Remove it", and left out of the update. Running the update could not clear it either: the updater compares its lock to the source and never reads disk, so it correctly reports "up to date" and writes nothing. Ignore OS-authored names on both sides of the comparison. The generator half is not hypothetical: a stray sidecar in a working tree made the committed artifacts read as stale, failing lint for that developer. Scoped to OS-authored names only. Tolerating unexpected files in general would let an injected payload ride along beside a clean SKILL.md; these are safe because an official SKILL.md never references them, so no agent can be routed into one. Mode bits are deliberately untouched — that would weaken identity for real scripts. * fix(skills): keep guarding a directory or link wearing an OS metadata name The name-only skip dropped any entry matching an OS metadata name, so a directory named .DS_Store or ._scripts took its whole subtree out of identity and a symlink wearing one stopped tripping the link guard — a skill hiding either read as pristine. The OS writes these as plain files only, so the entry type decides, still ahead of the case-fold map. Also compares both walkers over the same fixture: an asymmetric skip is worse than none, since one side would bake in content the other can never observe. * chore: ignore the OS metadata names skill identity already skips Both skill-identity walkers ignore these names, but .gitignore covered only .DS_Store and Thumbs.db — so a stray ._SKILL.md showed as untracked and `git add -A` could commit it. That is the one way the two walkers can disagree: the disk walker skips such a file while the git-tree producer (collectGitPackageFiles, used by the unreferenced --rebuild-from-tags path) does not, so a committed sidecar would make released history and observation describe different content. Ignoring them keeps that asymmetry unreachable rather than adding a second skip to the released-history path, which is load-bearing and provably never sees one today: no committed sidecar exists on any ref. Nothing tracked matches the new patterns. * chore: correct the skill-identity ignore comment The previous wording claimed these names cannot be committed, which overstates what .gitignore provides: `git add -f` and `git apply --index` both bypass it, so a cherry-pick, rebase or fork branch already carrying a sidecar is unaffected. That clause was load-bearing — it was the stated reason for leaving the released-history producer unhardened — so it should not read as a structural guarantee. Also fixes the producer count (three, not two: two disk walkers plus the git-tree producer, which does not skip) and says plain file, since the skip is isFile()-gated so a directory or link wearing the name is still walked. --- .gitignore | 6 ++ .../generate-skill-bundle-manifest.mjs | 21 ++++- .../generate-skill-bundle-manifest.test.mjs | 85 +++++++++++++++++++ .../skills/skill-freshness-inventory.test.ts | 26 ++++++ .../skills/skill-package-identity.test.ts | 79 +++++++++++++++++ src/main/skills/skill-package-identity.ts | 32 +++++++ 6 files changed, 248 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 00b6dd028..7f954a8df 100644 --- a/.gitignore +++ b/.gitignore @@ -44,9 +44,15 @@ package-lock.json *~ # OS +# Keep these in step with isOsMetadataSkillEntryName (skill-package-identity.ts): both disk +# walkers skip a plain file with one of these names, the git-tree producer does not, and +# ignoring them here is what keeps `git add -A` from committing a stray one. *.stackdump .DS_Store +._* Thumbs.db +ehthumbs.db +desktop.ini # Lint/cache .oxlintcache diff --git a/config/scripts/generate-skill-bundle-manifest.mjs b/config/scripts/generate-skill-bundle-manifest.mjs index ee74cc85b..899cb6436 100644 --- a/config/scripts/generate-skill-bundle-manifest.mjs +++ b/config/scripts/generate-skill-bundle-manifest.mjs @@ -124,6 +124,18 @@ function gitTreeSha(entries) { return hashDirectory(root).toString('hex') } +// Why: kept in step with isOsMetadataSkillEntryName in src/main/skills/skill-package-identity.ts. +// The scanner ignores these because the OS writes them into a live install; the generator +// ignores them so a stray one in a working tree cannot be committed into the manifest as +// content no user could ever match. Skipped rather than rejected: the file is not the +// developer's doing, so failing the build over it would be hostile. +const OS_METADATA_FILE_NAMES = new Set(['.ds_store', 'thumbs.db', 'ehthumbs.db', 'desktop.ini']) + +function isOsMetadataSkillEntryName(name) { + const folded = name.toLocaleLowerCase('en-US') + return OS_METADATA_FILE_NAMES.has(folded) || folded.startsWith('._') +} + async function collectPackageFiles(packageRoot) { const files = [] const caseFoldedPaths = new Map() @@ -135,6 +147,14 @@ async function collectPackageFiles(packageRoot) { entries.sort((left, right) => compareCodeUnits(left.name, right.name)) for (const entry of entries) { const absolutePath = path.join(directory, entry.name) + const fileStat = await lstat(absolutePath) + // Only a plain file is OS-authored, so the type decides and not the name alone: a + // directory or link wearing the name would otherwise drop its subtree out of the + // manifest and skip the guards below. Decided before the case-fold map so two + // spellings of one sidecar cannot collide. + if (isOsMetadataSkillEntryName(entry.name) && fileStat.isFile()) { + continue + } const relativePath = path.relative(packageRoot, absolutePath) assertSafeRelativePath(relativePath) const manifestPath = relativePath.split(path.sep).join('/') @@ -144,7 +164,6 @@ async function collectPackageFiles(packageRoot) { throw new Error(`Case-colliding skill paths: ${collision} and ${manifestPath}`) } caseFoldedPaths.set(foldedPath, manifestPath) - const fileStat = await lstat(absolutePath) if (fileStat.isSymbolicLink()) { throw new Error(`Symlink is not allowed in a shipped skill: ${manifestPath}`) } diff --git a/config/scripts/generate-skill-bundle-manifest.test.mjs b/config/scripts/generate-skill-bundle-manifest.test.mjs index 3069c1cbb..e8a88b463 100644 --- a/config/scripts/generate-skill-bundle-manifest.test.mjs +++ b/config/scripts/generate-skill-bundle-manifest.test.mjs @@ -14,6 +14,7 @@ import { tmpdir } from 'node:os' import path from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import { parse } from 'yaml' +import { observeSkillPackage } from '../../src/main/skills/skill-package-identity' import { appendReleaseRow, assertReleasedHistoryPreserved, @@ -436,6 +437,90 @@ describe('skill bundle manifest generator', () => { ) }) + it('ignores OS-authored sidecars a working tree may carry', async () => { + const packageRoot = await createPackage() + await writeFile(path.join(packageRoot, 'SKILL.md'), 'demo skill\n') + await mkdir(path.join(packageRoot, 'references')) + await writeFile(path.join(packageRoot, 'references', 'guide.md'), 'nested\n') + const pristine = await collectPackageFiles(packageRoot) + expect(pristine.map((file) => file.path)).toEqual(['SKILL.md', 'references/guide.md']) + // Finder writes .DS_Store into any browsed folder, and it is gitignored — so without + // this the committed artifacts read as stale and lint fails for that developer, while + // the scanner would have no snapshot a real install could match. + await writeFile(path.join(packageRoot, '.DS_Store'), Buffer.from([0, 1, 2, 3])) + await writeFile(path.join(packageRoot, '._SKILL.md'), Buffer.from([0, 5])) + await writeFile(path.join(packageRoot, 'Thumbs.db'), Buffer.from([9])) + // Nested folders get browsed too, and a sidecar there shifts the same index-aligned list. + await writeFile(path.join(packageRoot, 'references', '.DS_Store'), Buffer.from([7])) + + expect(await collectPackageFiles(packageRoot)).toEqual(pristine) + }) + + it('still records an unexpected file that is not OS metadata', async () => { + const packageRoot = await createPackage() + await writeFile(path.join(packageRoot, 'SKILL.md'), 'demo skill\n') + await writeFile(path.join(packageRoot, 'payload.sh'), 'echo hi\n') + + expect((await collectPackageFiles(packageRoot)).map((file) => file.path)).toEqual([ + 'SKILL.md', + 'payload.sh' + ]) + }) + + it('keeps guarding a directory or link that only wears an OS metadata name', async () => { + const packageRoot = await createPackage() + await writeFile(path.join(packageRoot, 'SKILL.md'), 'demo skill\n') + // Only plain files are OS-authored, so a subtree behind one of these names is real + // content that must stay in the manifest instead of shipping unrecorded. + await mkdir(path.join(packageRoot, '.DS_Store')) + await writeFile(path.join(packageRoot, '.DS_Store', 'payload.sh'), 'echo hi\n') + + expect((await collectPackageFiles(packageRoot)).map((file) => file.path)).toEqual([ + '.DS_Store/payload.sh', + 'SKILL.md' + ]) + + if (process.platform !== 'win32') { + await rm(path.join(packageRoot, '.DS_Store'), { recursive: true }) + await symlink('SKILL.md', path.join(packageRoot, '._SKILL.md')) + await expect(collectPackageFiles(packageRoot)).rejects.toThrow( + 'Symlink is not allowed in a shipped skill' + ) + } + }) + + // Why: the predicate is hand-copied from the scanner, and an asymmetric skip is worse than + // no skip — one side would bake in content the other can never observe, leaving every + // install permanently unrecognized. Compared through both walkers so ordering and the + // case-fold map are covered too, not just the name test. + it('skips exactly the names the scanner skips', async () => { + const packageRoot = await createPackage() + for (const name of [ + 'SKILL.md', + '.DS_Store', + '.ds_store', + '.DS_STORE', + 'Thumbs.db', + 'THUMBS.DB', + 'ehthumbs.db', + 'desktop.ini', + 'Desktop.INI', + '._SKILL.md', + '._', + // Near misses that both sides must keep. + '.dsstore', + 'ds_store.md', + '_SKILL.md', + '.DS_Store.md' + ]) { + await writeFile(path.join(packageRoot, name), `${name}\n`) + } + + const generated = (await collectPackageFiles(packageRoot)).map((file) => file.path) + expect(generated).toEqual((await observeSkillPackage(packageRoot)).files.map((f) => f.path)) + expect(generated).toEqual(['.DS_Store.md', '.dsstore', 'SKILL.md', '_SKILL.md', 'ds_store.md']) + }) + it('computes the same Git tree identity as Git', async () => { const packageRoot = path.resolve('skills', 'orca-cli') const files = await collectPackageFiles(packageRoot) diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index 3aeca4bdd..2b20055e6 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -248,6 +248,32 @@ describe('read-only skill freshness inventory', () => { expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('up-to-date') }) + it('reads up to date after the OS drops a sidecar into an untouched install', async () => { + const test = await fixture() + const directory = await test.writeSkill( + join(test.homeDir, '.agents', 'skills'), + test.currentMarkdown + ) + // What one Finder visit leaves behind. Sorts before SKILL.md, which is what made the + // index-aligned snapshot comparison miss and report the copy as modified. + await writeFile(join(directory, '.DS_Store'), Buffer.from([0, 1, 2, 3])) + // No lock trust available here: the recorded hash is a different tree, so this proves + // the identity fix alone carries it rather than falling through to newer-known. + await writeSkillLockHash(test.homeDir, 'a'.repeat(40)) + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + expect(inventory.installations.map((entry) => entry.status)).toEqual(['current']) + // The whole point: no amber, and nothing offered to "fix" a copy that is already right. + expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('up-to-date') + 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` diff --git a/src/main/skills/skill-package-identity.test.ts b/src/main/skills/skill-package-identity.test.ts index 648b628e8..ca9beb9fd 100644 --- a/src/main/skills/skill-package-identity.test.ts +++ b/src/main/skills/skill-package-identity.test.ts @@ -80,6 +80,85 @@ describe('skill package identity', () => { ).rejects.toThrow('skill-package-entry-limit') }) + it('ignores OS-authored sidecars so a browsed folder still matches its snapshot', async () => { + const pristine = await temporarySkill() + await writeFile(join(pristine, 'SKILL.md'), 'skill\n') + const official = await observeSkillPackage(pristine) + + const browsed = await temporarySkill() + await writeFile(join(browsed, 'SKILL.md'), 'skill\n') + // Every name the OS writes on its own, including one that sorts BEFORE SKILL.md — + // the index-aligned comparison in matchingKnownSnapshot misaligns on a leading entry, + // so a trailing-name-only fixture would pass while the reported bug survived. + await writeFile(join(browsed, '.DS_Store'), Buffer.from([0, 1, 2, 3])) + await writeFile(join(browsed, '._SKILL.md'), Buffer.from([0, 5])) + await writeFile(join(browsed, 'Thumbs.db'), Buffer.from([9])) + await writeFile(join(browsed, 'desktop.ini'), '[.ShellClassInfo]\n') + + const observed = await observeSkillPackage(browsed) + + expect(observed.files.map((file) => file.path)).toEqual(['SKILL.md']) + // The lock-trust path compares this against the updater's recorded source tree, so it + // 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 + ).toBe(1) + }) + + it('keeps guarding a directory or link that only wears an OS metadata name', async () => { + const root = await temporarySkill() + await writeFile(join(root, 'SKILL.md'), 'skill\n') + // The OS writes these names as plain files only, so a subtree behind one is real content: + // dropping it on the name alone would hide it from identity and read as pristine. + await mkdir(join(root, '._scripts')) + await writeFile(join(root, '._scripts', 'payload.sh'), '#!/bin/sh\n') + + expect((await observeSkillPackage(root)).files.map((file) => file.path)).toEqual([ + '._scripts/payload.sh', + 'SKILL.md' + ]) + + if (process.platform !== 'win32') { + await rm(join(root, '._scripts'), { recursive: true }) + await symlink(join(root, 'SKILL.md'), join(root, '._DS_Store')) + await expect(observeSkillPackage(root)).rejects.toThrow('skill-package-link') + } + }) + + it('still reports a genuinely modified skill as unmatched', async () => { + const pristine = await temporarySkill() + await writeFile(join(pristine, 'SKILL.md'), 'skill\n') + const official = await observeSkillPackage(pristine) + + 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. + const withPayload = await temporarySkill() + await writeFile(join(withPayload, 'SKILL.md'), 'skill\n') + await writeFile(join(withPayload, 'payload.sh'), '#!/bin/sh\n') + + const snapshot = [ + { + releaseRevision: 1, + packageDigest: official.observedDigest, + gitTreeSha: official.observedGitTreeSha, + files: official.files + } + ] + expect(matchingKnownSnapshot(await observeSkillPackage(edited), snapshot)).toBeNull() + expect(matchingKnownSnapshot(await observeSkillPackage(withPayload), snapshot)).toBeNull() + }) + it.runIf(process.platform !== 'win32')('tracks executable mode in package identity', async () => { const root = await temporarySkill() await mkdir(join(root, 'scripts')) diff --git a/src/main/skills/skill-package-identity.ts b/src/main/skills/skill-package-identity.ts index d518bd05d..4c883a2d6 100644 --- a/src/main/skills/skill-package-identity.ts +++ b/src/main/skills/skill-package-identity.ts @@ -18,6 +18,27 @@ export type ObservedSkillPackage = { observedGitTreeSha: string } +// Why: package identity compares a live user directory against a tree the generator read +// from a clean checkout, so anything the OS deposits on its own counts as drift the user +// never caused. One Finder visit writes .DS_Store, and that alone made the copy +// 'unrecognized' — reported as "may be modified", left out of the update, and unfixable by +// running it, since the updater compares its lock to the source and never reads disk. +// +// Only OS-authored names belong here. Tolerating unexpected files in general would let a +// modified skill pass: the entry is safe precisely because an official SKILL.md never +// references these, so no agent can be routed into one. Mirrored in +// config/scripts/generate-skill-bundle-manifest.mjs so neither side of the comparison can +// bake one in. Deliberately NOT extended to mode bits — that would weaken identity for +// real scripts. +const OS_METADATA_FILE_NAMES = new Set(['.ds_store', 'thumbs.db', 'ehthumbs.db', 'desktop.ini']) + +export function isOsMetadataSkillEntryName(name: string): boolean { + const folded = name.toLocaleLowerCase('en-US') + // AppleDouble sidecars ('._SKILL.md') appear whenever a skill is copied through a + // filesystem that cannot hold macOS metadata inline. + return OS_METADATA_FILE_NAMES.has(folded) || folded.startsWith('._') +} + export const SKILL_PACKAGE_OBSERVATION_LIMITS = { maximumDepth: 16, maximumEntries: 2_048, @@ -171,6 +192,17 @@ export async function observeSkillPackage( entries.sort((left, right) => compareCodeUnits(left.name, right.name)) for (const entry of entries) { const absolutePath = join(directory, entry.name) + // Only a plain file is OS-authored, so the type decides and not the name alone: a + // directory or link wearing the name would otherwise hide a subtree from identity and + // slip past the link and special-file guards below. Decided before the case-fold map + // so two spellings of one sidecar cannot collide, and tolerant of a vanished entry so + // an unreadable sidecar cannot fail the whole package. + if (isOsMetadataSkillEntryName(entry.name)) { + const sidecarStat = await lstat(absolutePath).catch(() => null) + if (!sidecarStat || sidecarStat.isFile()) { + continue + } + } const relativePath = relative(packageRoot, absolutePath) if ( isAbsolute(relativePath) ||