fix(skills): stop OS sidecars marking an untouched skill as modified (#11471)
* 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.
This commit is contained in:
Brennan Benson
GitHub
parent
d94ed85c24
commit
49cfbf014c
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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}`)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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'))
|
||||
|
|
|
|||
|
|
@ -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) ||
|
||||
|
|
|
|||
Loading…
Reference in New Issue