diff --git a/.gitignore b/.gitignore index 00b6dd02894..7f954a8df11 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 ee74cc85bcb..899cb6436df 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 3069c1cbb68..e8a88b4636c 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 3aeca4bdd59..2b20055e646 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 648b628e82c..ca9beb9fd3d 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 d518bd05d43..4c883a2d6e6 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) || diff --git a/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx b/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx index ea84e02c1d6..d32f6a017a8 100644 --- a/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx @@ -223,6 +223,59 @@ describe('SkillFreshnessNudge', () => { expect(mocks.toastInfo).not.toHaveBeenCalled() }) + it('keeps a project copy out of the dismissal key it persists', async () => { + // Why: the global update never reaches a project copy, so its identity must not enter + // the dismissal tuple — re-checking out that repo mints a new inode, which would + // re-raise a nudge the user already dismissed for the same global revision. + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement(), + placement({ + id: 'repo-copy', + topology: 'repo-scope', + sourceKind: 'repo', + unresolvedPath: '/home/projects/work/.agents/skills/orca-cli', + resolvedPath: '/home/projects/work/.agents/skills/orca-cli', + physicalIdentity: 'physical-repo-orca-cli' + }) + ], + eligibleUpdateNames: ['orca-cli'], + scanIssues: [], + scannedAt: 1 + } + + await renderNudge() + mocks.toastInfo.mock.calls[0]?.[1].onDismiss() + + expect(mocks.updateSettings).toHaveBeenCalledWith({ + dismissedSkillFreshnessNudges: [DISMISSAL_KEY] + }) + }) + + it('stays dismissed when only a project copy changed identity', async () => { + mocks.dismissed = [DISMISSAL_KEY] + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement(), + placement({ + id: 'repo-copy', + topology: 'repo-scope', + sourceKind: 'repo', + physicalIdentity: 'physical-repo-orca-cli-after-checkout' + }) + ], + eligibleUpdateNames: ['orca-cli'], + scanIssues: [], + scannedAt: 2 + } + + await renderNudge() + + expect(mocks.toastInfo).not.toHaveBeenCalled() + }) + it('waits for persisted settings before deciding whether to nudge', async () => { mocks.settingsLoaded = false diff --git a/src/renderer/src/components/skills/SkillFreshnessNudge.tsx b/src/renderer/src/components/skills/SkillFreshnessNudge.tsx index 135d860656b..eb29629812a 100644 --- a/src/renderer/src/components/skills/SkillFreshnessNudge.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessNudge.tsx @@ -4,6 +4,7 @@ import { toast } from 'sonner' import { useSkillFreshness } from '@/hooks/useSkillFreshness' import { translate } from '@/i18n/i18n' import { useAppStore } from '@/store' +import { skillPlacementParticipatesInGlobalFreshness } from '../../../../shared/skill-freshness' import { requestSkillFreshnessUpdateDialog } from './skill-freshness-update-dialog' const MAX_DISMISSED_FRESHNESS_NUDGES = 512 @@ -52,6 +53,10 @@ export function SkillFreshnessNudge(): null { } const eligibleNames = new Set(inventory.eligibleUpdateNames) const candidates = inventory.installations.flatMap((installation) => + // Why: a project copy the global update never touches must not enter the dismissal + // fingerprint, or re-checking out that repo re-raises a nudge the user already + // dismissed — and spends the bounded dismissal budget on copies Orca cannot update. + skillPlacementParticipatesInGlobalFreshness(installation) && installation.status === 'outdated' && eligibleNames.has(installation.name) && installation.physicalIdentity diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx index 7c00fb26d53..1a53ecdcf10 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx @@ -400,7 +400,9 @@ describe('SkillFreshnessUpdateDialog', () => { it('shows why a skill was skipped without needing the disclosure', async () => { mocks.inventory = { schemaVersion: 1, - installations: [placement('computer-use', { topology: 'repo-scope' })], + // Why: a read-only copy rather than a project one — a project-owned copy raises no + // row at all now, so it cannot carry this assertion about how a row renders. + installations: [placement('computer-use', { topology: 'read-only' })], eligibleUpdateNames: [], scanIssues: [], scannedAt: 3 @@ -415,14 +417,73 @@ describe('SkillFreshnessUpdateDialog', () => { // outside the disclosure — visible with the row still collapsed, and not // dependent on a mount-time `defaultOpen` a later re-scan could never re-fire. expect(row?.getAttribute('data-collapsible-open')).toBe('false') - expect(container?.textContent).toContain('This is a project skill, not a global one') + expect(container?.textContent).toContain('This copy is in a read-only location') // Assert the placement, not just the presence: moving it back inside the // disclosure would hide it in production but still satisfy `textContent`. expect(row?.querySelector('[data-collapsible-content]')?.textContent).not.toContain( - 'This is a project skill, not a global one' + 'This copy is in a read-only location' ) }) + it('raises no row for a skill whose only finding is a project-owned copy', async () => { + // The reported bug: a pristine global install plus a drifted copy inside a work + // directory. Orca only ever updates global skills, so a "Skipped" row here asserts + // it considered an update it could never perform. + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement('computer-use', { status: 'current' }), + placement('computer-use', { + rootId: 'repo-work', + sourceKind: 'repo', + topology: 'repo-scope', + status: 'unrecognized', + unresolvedPath: '/home/projects/work/.agents/skills/computer-use', + resolvedPath: '/home/projects/work/.agents/skills/computer-use', + physicalIdentity: 'physical-repo-computer-use' + }) + ], + eligibleUpdateNames: [], + scanIssues: [], + scannedAt: 4 + } + await renderDialog() + await openViaRequest() + + expect(container?.textContent).toContain('All installed Orca skills are up to date.') + expect(container?.querySelector('[data-skill-row="computer-use"]')).toBeNull() + }) + + it('still lists a project copy when another placement earns the group', async () => { + // The converse: ownership suppresses the group, never the visibility of a location + // that exists. An outdated global copy earns the row, and the project copy has to + // stay listed there so the user can see every place the skill lives. + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement('computer-use', { topology: 'read-only' }), + placement('computer-use', { + rootId: 'repo-work', + sourceKind: 'repo', + topology: 'repo-scope', + status: 'unrecognized', + unresolvedPath: '/home/projects/work/.agents/skills/computer-use', + resolvedPath: '/home/projects/work/.agents/skills/computer-use', + physicalIdentity: 'physical-repo-computer-use' + }) + ], + eligibleUpdateNames: [], + scanIssues: [], + scannedAt: 5 + } + await renderDialog() + await openViaRequest() + + const row = container?.querySelector('[data-skill-row="computer-use"]') + expect(row).not.toBeNull() + expect(row?.textContent).toContain('/home/projects/work/.agents/skills/computer-use') + }) + it('names the skill in the stale-record remedy so the command is runnable as-is', async () => { // Why: the reinstall advice only fires when the row hands its group name through // to skippedReason — dropping that argument silently degrades every stale-record @@ -442,6 +503,40 @@ describe('SkillFreshnessUpdateDialog', () => { ) }) + it('keeps the stale-record remedy when a project copy is listed beside it', async () => { + // The same stale-record row as above, plus the user's own project copy. That copy is + // listed but was never judged, so letting it explain the skip replaced the only + // runnable command with advice about a copy the user never asked Orca to update. + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement('orchestration'), + placement('orchestration', { + rootId: 'repo-work', + sourceKind: 'repo', + topology: 'repo-scope', + status: 'unrecognized', + unresolvedPath: '/home/projects/work/.agents/skills/orchestration', + resolvedPath: '/home/projects/work/.agents/skills/orchestration', + physicalIdentity: 'physical-repo-orchestration' + }) + ], + eligibleUpdateNames: [], + scanIssues: [], + scannedAt: 6 + } + await renderDialog() + await openViaRequest() + + const row = container?.querySelector('[data-skill-row="orchestration"]') + expect(row?.textContent).toContain( + 'npx skills add https://github.com/stablyai/orca --skill orchestration --global' + ) + expect(row?.textContent).not.toContain('This is a project skill, not a global one') + // Still listed, though — ownership silences the explanation, never the location. + expect(row?.textContent).toContain('/home/projects/work/.agents/skills/orchestration') + }) + it('stays coherent while an idle re-scan is in flight', async () => { await renderDialog() await openViaRequest() diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts index 63c2243b71f..4c9afaed59b 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -35,7 +35,12 @@ describe('groupSkillFreshness', () => { expect(groups).toHaveLength(1) expect(groups[0]).toMatchObject({ name: 'orca-cli', status: 'update-available' }) expect(groups[0]?.locations).toEqual([ - { id: expect.any(String), path: '/home/.agents/skills/orca-cli', chip: null } + { + id: expect.any(String), + path: '/home/.agents/skills/orca-cli', + chip: null, + participatesInGlobalFreshness: true + } ]) }) @@ -75,7 +80,12 @@ describe('groupSkillFreshness', () => { name: 'dataviz', status: 'cannot-update', locations: [ - { id: expect.any(String), path: '/home/.agents/skills/dataviz', chip: 'unrecognized' } + { + id: expect.any(String), + path: '/home/.agents/skills/dataviz', + chip: 'unrecognized', + participatesInGlobalFreshness: true + } ] }, { @@ -85,7 +95,8 @@ describe('groupSkillFreshness', () => { { id: expect.any(String), path: '/home/.agents/skills/linear-tickets', - chip: 'inaccessible' + chip: 'inaccessible', + participatesInGlobalFreshness: true } ] } @@ -124,8 +135,18 @@ describe('groupSkillFreshness', () => { expect(groups[0]?.status).toBe('cannot-update') // Why: the out-of-date main copy is bare; only the poisoning copy carries a chip. expect(groups[0]?.locations).toEqual([ - { id: expect.any(String), path: '/home/.agents/skills/orchestration', chip: null }, - { id: expect.any(String), path: '/home/.claude/skills/orchestration', chip: 'unrecognized' } + { + id: expect.any(String), + path: '/home/.agents/skills/orchestration', + chip: null, + participatesInGlobalFreshness: true + }, + { + id: expect.any(String), + path: '/home/.claude/skills/orchestration', + chip: 'unrecognized', + participatesInGlobalFreshness: true + } ]) }) @@ -146,12 +167,22 @@ describe('groupSkillFreshness', () => { expect(groups).toHaveLength(1) expect(groups[0]?.status).toBe('cannot-update') expect(groups[0]?.locations).toEqual([ - { id: expect.any(String), path: '/home/.agents/skills/orchestration', chip: 'current' }, - { id: expect.any(String), path: '/home/.factory/skills/orchestration', chip: 'duplicate' } + { + id: expect.any(String), + path: '/home/.agents/skills/orchestration', + chip: 'current', + participatesInGlobalFreshness: true + }, + { + id: expect.any(String), + path: '/home/.factory/skills/orchestration', + chip: 'duplicate', + participatesInGlobalFreshness: true + } ]) }) - it('prefers a location status over its topology and maps every topology to a chip', () => { + it('ranks a read failure over ownership, ownership over byte status, and maps every topology', () => { const chipFor = (overrides: Partial): string | null => groupSkillFreshness( [placement('s', { status: 'outdated' }), placement('s', overrides)], @@ -178,5 +209,79 @@ describe('groupSkillFreshness', () => { expect(chipFor(at('j', { status: 'unrecognized', topology: 'plugin-cache' }))).toBe( 'plugin-cache' ) + // Ownership outranks byte status: a project copy whose bytes match nothing known is + // the repo's content, not the user's drift, so it must not read "may be modified". + expect(chipFor(at('l', { status: 'unrecognized', topology: 'repo-scope' }))).toBe('in-a-repo') + // But a read failure outranks ownership, or that same rule would hide a real fault. + expect(chipFor(at('m', { status: 'inaccessible', topology: 'repo-scope' }))).toBe( + 'inaccessible' + ) + }) + + it('raises no group when every finding is project-owned', () => { + // Orca's updater only passes --global, so a project copy has no remedy; a row here + // would claim Orca considered an update it could never perform. + expect( + groupSkillFreshness( + [placement('computer-use', { status: 'unrecognized', topology: 'repo-scope' })], + [] + ) + ).toEqual([]) + expect( + groupSkillFreshness( + [placement('computer-use', { status: 'outdated', topology: 'repo-scope' })], + [] + ) + ).toEqual([]) + }) + + it('still lists a project copy inside a group another placement earned', () => { + const groups = groupSkillFreshness( + [ + placement('computer-use', { status: 'outdated', topology: 'read-only' }), + placement('computer-use', { + status: 'unrecognized', + topology: 'repo-scope', + unresolvedPath: '/home/projects/work/.agents/skills/computer-use' + }) + ], + [] + ) + + expect(groups).toHaveLength(1) + expect(groups[0]?.locations.map((location) => location.chip)).toContain('in-a-repo') + }) + + it('marks only the project copy as one the global update never judged', () => { + // Why the flag rather than the chip: the skip sentence reads this to keep a listed-only + // copy from explaining a skip it had no part in, and a plugin copy — which does earn + // groups of its own — has to stay on the judged side of that line. + const groups = groupSkillFreshness( + [ + placement('computer-use', { status: 'outdated', topology: 'read-only' }), + placement('computer-use', { + status: 'outdated', + topology: 'plugin-cache', + unresolvedPath: '/home/.claude/plugins/cache/pack/skills/computer-use' + }), + placement('computer-use', { + status: 'unrecognized', + topology: 'repo-scope', + unresolvedPath: '/home/projects/work/.agents/skills/computer-use' + }) + ], + [] + ) + + expect( + groups[0]?.locations.map((location) => [ + location.chip, + location.participatesInGlobalFreshness + ]) + ).toEqual([ + ['read-only', true], + ['plugin-cache', true], + ['in-a-repo', false] + ]) }) }) diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index 79d39b31aff..08241eda6a4 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.ts @@ -1,5 +1,7 @@ import { + isOwnerManagedSkillScope, isSkillCopyNeedingAttention, + skillPlacementParticipatesInGlobalFreshness, type SkillFreshnessInstallation } from '../../../../shared/skill-freshness' @@ -21,6 +23,9 @@ export type SkillLocationRow = { id: string path: string chip: SkillLocationChip | null + /** False for a project-owned copy: listed for visibility, but never judged, so it + * cannot be the reason a skill was skipped. */ + participatesInGlobalFreshness: boolean } export type SkillFreshnessGroupModel = { @@ -29,16 +34,16 @@ export type SkillFreshnessGroupModel = { locations: SkillLocationRow[] } +// Why: an owner-managed scope outranks 'unrecognized', or a project copy reads "may be +// modified — remove it" over content its own repo owns. A read failure outranks both, so +// that rule cannot hide a genuine fault. export function locationChip(installation: SkillFreshnessInstallation): SkillLocationChip | null { - if (installation.status === 'unrecognized' && installation.topology === 'plugin-cache') { - return 'plugin-cache' - } - if (installation.status === 'unrecognized') { - return 'unrecognized' - } if (installation.status === 'inaccessible') { return 'inaccessible' } + if (!isOwnerManagedSkillScope(installation.topology) && installation.status === 'unrecognized') { + return 'unrecognized' + } switch (installation.topology) { case 'independent-copy': return 'duplicate' @@ -74,7 +79,8 @@ export function locationChip(installation: SkillFreshnessInstallation): SkillLoc * Details, and Details opens this modal. Returning only out-of-date skills left that * modal saying every skill was up to date over an empty list, contradicting the badge * that sent the user there. Skills where every copy is current are still omitted, as - * is a plugin's own copy of a same-named skill, which is not the user's to fix. + * is a plugin's own copy of a same-named skill, and a project's own copy — neither is the + * user's to fix through the global update. * * `alwaysIncludeNames` overrides that filter. A successful update makes every * targeted skill current, which would otherwise drop its row the instant the @@ -96,14 +102,26 @@ export function groupSkillFreshness( } const groups: SkillFreshnessGroupModel[] = [] for (const [name, entries] of byName) { + // Why: the inclusion test only consults placements the global update can reach, so a + // skill whose only finding is a project-owned copy raises no row at all — a "Skipped" + // row would assert Orca considered an update it never could. Locations below are still + // listed in full, so a project copy stays visible whenever some other placement earns + // the group. if ( !pinned.has(name) && - !entries.some((entry) => entry.status === 'outdated' || isSkillCopyNeedingAttention(entry)) + !entries + .filter(skillPlacementParticipatesInGlobalFreshness) + .some((entry) => entry.status === 'outdated' || isSkillCopyNeedingAttention(entry)) ) { continue } const locations = entries - .map((entry) => ({ id: entry.id, path: entry.unresolvedPath, chip: locationChip(entry) })) + .map((entry) => ({ + id: entry.id, + path: entry.unresolvedPath, + chip: locationChip(entry), + participatesInGlobalFreshness: skillPlacementParticipatesInGlobalFreshness(entry) + })) .sort((left, right) => left.path.localeCompare(right.path, 'en')) groups.push({ name, diff --git a/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts index 664a4883f84..5f04d122dd6 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.test.ts @@ -6,7 +6,15 @@ function row( chip: SkillLocationRow['chip'], path = `/home/.agents/skills/${chip}` ): SkillLocationRow { - return { id: `row-${chip}-${path}`, path, chip } + return { id: `row-${chip}-${path}`, path, chip, participatesInGlobalFreshness: true } +} + +/** A project's own copy: listed, but never judged by the global update. */ +function projectRow(chip: SkillLocationRow['chip'] = 'in-a-repo'): SkillLocationRow { + return { + ...row(chip, '/home/projects/work/.agents/skills/orchestration'), + participatesInGlobalFreshness: false + } } describe('skippedReason', () => { @@ -49,6 +57,35 @@ describe('skippedReason', () => { ) }) + it('keeps the reinstall remedy when a project copy sits beside the blocked global copy', () => { + // The reported shape: a bare out-of-date global copy the updater's record cannot + // converge, plus the user's own project copy. Letting the project copy explain the + // skip swapped the one runnable command for advice about a copy Orca never judged. + const reason = skippedReason([row(null), projectRow()], 'orchestration') + expect(reason).toContain('reports the skill as already up to date') + expect(reason).toContain('skills add') + expect(reason).not.toContain('This is a project skill') + }) + + it('does not let a project copy outrank the placement that blocked the update', () => { + // Why by row and not by chip: the scan-limit sentinel is also project-scoped and + // carries 'inaccessible', which would otherwise claim a read failure Orca never hit. + expect(skippedReason([row('duplicate'), projectRow()], 'orchestration')).toContain( + 'separate copy' + ) + expect( + skippedReason([row('duplicate'), projectRow('inaccessible')], 'orchestration') + ).toContain('separate copy') + }) + + it('explains a project copy when that is the only location there is', () => { + // Why the fallback exists: the sentence stays total. With nothing else to point at, + // "Orca only updates your global skills" is the honest reason. + expect(skippedReason([projectRow()], 'orchestration')).toContain( + 'This is a project skill, not a global one' + ) + }) + it('falls back to the generic sentence when no skill name is available', () => { expect(skippedReason([row('current')])).toContain('left this skill out of the update') expect(skippedReason([])).toContain('left this skill out of the update') diff --git a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts index 0e9c01c01f1..84ef6fc3f29 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts @@ -21,8 +21,12 @@ const SKIPPED_REASON_PRIORITY: SkillLocationChip[] = [ 'duplicate' ] +// Why: a location the global update never judged cannot be why the skill was skipped, so it +// must not outrank the placement that was — nor the ABSENCE of a chip, which is how the +// stale-record remedy below is reached. The fallback keeps the sentence total. function blockingChip(locations: readonly SkillLocationRow[]): SkillLocationChip | undefined { - const present = new Set(locations.map((location) => location.chip)) + const judged = locations.filter((location) => location.participatesInGlobalFreshness) + const present = new Set((judged.length > 0 ? judged : locations).map((location) => location.chip)) return SKIPPED_REASON_PRIORITY.find((candidate) => present.has(candidate)) } diff --git a/src/renderer/src/lib/skill-freshness-display-status.test.ts b/src/renderer/src/lib/skill-freshness-display-status.test.ts index 55a68e774e6..66c616a702f 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.test.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.test.ts @@ -57,6 +57,18 @@ function pluginCachePlacement( } } +function repoPlacement(status: SkillFreshnessStatus = 'unrecognized'): SkillFreshnessInstallation { + return { + ...placement(status, 7), + rootId: 'repo-work', + sourceKind: 'repo', + sourceLabel: 'work', + topology: 'repo-scope', + unresolvedPath: `/home/projects/work/.agents/skills/${SKILL_NAME}`, + resolvedPath: `/home/projects/work/.agents/skills/${SKILL_NAME}` + } +} + function inventory( installations: SkillFreshnessInstallation[], eligibleUpdateNames: string[] = [], @@ -209,6 +221,46 @@ describe('hasSkillCopyNeedingAttention', () => { ).toBe(true) }) + // The exact shape of the rc.4 report: a pristine global install alongside a drifted + // copy inside a work directory. Orca only updates global skills, so the project copy + // must not turn the badge amber over drift it has no way to fix. + it('reports up to date for a current global copy beside a drifted project copy', () => { + const value = inventory([placement('current'), repoPlacement('unrecognized')]) + + expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('up-to-date') + expect(hasSkillCopyNeedingAttention(value, SKILL_NAME)).toBe(false) + }) + + // Why by scope and not by status: an outdated or unreadable project copy is just as + // far outside the global updater's reach as an unrecognized one, so pinning only the + // reported status would leave the same bug reachable through a different one. + it.each<[SkillFreshnessStatus]>([['unrecognized'], ['outdated'], ['inaccessible']])( + 'ignores a %s project copy when judging global freshness', + (status) => { + const value = inventory([placement('current'), repoPlacement(status)]) + + expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('up-to-date') + } + ) + + // A project copy is not evidence the skill is installed globally, so it reports + // presence rather than a freshness claim about a copy Orca does not manage. + it('reports presence only when every copy is project-owned', () => { + expect(getSkillFreshnessDisplayStatus(inventory([repoPlacement()]), SKILL_NAME)).toBe( + 'installed' + ) + }) + + // Regression guard for the centralization: the badge predicate deliberately omits the + // shared helper's outdated carve-out, so a non-eligible outdated copy stays amber. + // Collapsing the two predicates would flip this to green while the dialog still shows + // a reinstall row — the badge/dialog contradiction this change exists to avoid. + it('keeps a non-eligible outdated global copy amber', () => { + expect(getSkillFreshnessDisplayStatus(inventory([placement('outdated')]), SKILL_NAME)).toBe( + 'needs-attention' + ) + }) + // Why: an unreadable plugin path could hide a copy of anything, but a skill Orca // never found anywhere is not the one to blame for it — that reads as a problem // with a skill the user has not installed. @@ -218,4 +270,14 @@ describe('hasSkillCopyNeedingAttention', () => { expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('installed') expect(hasSkillCopyNeedingAttention(value, SKILL_NAME)).toBe(false) }) + + // Why: a project copy is not the presence that makes the cache fault this skill's + // problem either. The two helpers have to read the same placements, or the badge calls + // a repo-only skill merely installed while the review affordance flags it amber. + it('does not treat a project copy as the presence that owns a cache fault', () => { + const value = inventory([repoPlacement()], [], [scanIssue('io-error')]) + + expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('installed') + expect(hasSkillCopyNeedingAttention(value, SKILL_NAME)).toBe(false) + }) }) diff --git a/src/renderer/src/lib/skill-freshness-display-status.ts b/src/renderer/src/lib/skill-freshness-display-status.ts index ed983e88c2a..3d6ad527a97 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.ts @@ -1,6 +1,7 @@ import { isSkillCopyNeedingAttention, isSkillScanIssueNeedingAttention, + skillPlacementParticipatesInGlobalFreshness, type SkillFreshnessInventory } from '../../../shared/skill-freshness' @@ -23,6 +24,13 @@ export function getSkillFreshnessDisplayStatus( if (installation.name !== skillName) { continue } + // Why: a project-owned copy is outside the global updater's reach, so it can neither + // stand in as evidence this skill is installed globally nor make the badge amber over + // drift Orca has no way to fix. Skipped before `hasPlacement` so a repo-only skill + // reports presence, not a freshness claim about a copy Orca does not manage. + if (!skillPlacementParticipatesInGlobalFreshness(installation)) { + continue + } hasPlacement = true // Why: 'newer-known' is recognized official content ahead of this build — the // updater's own install or a newer release's bytes. There is nothing to fix and @@ -61,8 +69,11 @@ export function hasSkillCopyNeedingAttention( inventory: SkillFreshnessInventory | null, skillName: string ): boolean { + // Why: the same participation filter the status above applies, or an unreadable plugin + // folder flags a repo-only skill the status calls merely installed. const placements = (inventory?.installations ?? []).filter( - (installation) => installation.name === skillName + (installation) => + installation.name === skillName && skillPlacementParticipatesInGlobalFreshness(installation) ) return ( (placements.length > 0 && diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index fe3c94272f3..24bbcc9df38 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -86,17 +86,44 @@ export type SkillFreshnessInstallation = { errorCategory: string | null } +// A scope whose contents belong to someone other than the user: a project's own checkout, +// or a plugin's bundled copy. Orca's global update never writes here, so the owner's +// content is not the user's drift — which is why ownership outranks byte status when +// labelling a location. +const OWNER_MANAGED_SKILL_SCOPES: ReadonlySet = new Set([ + 'repo-scope', + 'plugin-cache' +]) + +export function isOwnerManagedSkillScope(topology: SkillInstallationTopology): boolean { + return OWNER_MANAGED_SKILL_SCOPES.has(topology) +} + +// Why: Orca's updater only ever passes --global, so a copy a project owns is not +// something Orca can act on — it has no remedy by design. Reporting it as global drift +// produced an amber badge no user action could clear, over a copy their own repo +// legitimately owns. Stated by scope rather than by byte status on purpose: an outdated +// or unreadable project copy is just as far outside the global updater's reach as an +// unrecognized one. +export function skillPlacementParticipatesInGlobalFreshness( + installation: SkillFreshnessInstallation +): boolean { + return installation.topology !== 'repo-scope' +} + /** * Whether a copy is wrong in a way running the update would not resolve. * * Shared so the badge and the review dialog cannot disagree about what counts: the * badge points at the dialog for the explanation, so a copy that turns the badge * amber must also produce a row there. An out-of-date copy the command converges is - * ordinary work, not a problem, and a plugin's own copy of a same-named skill is the - * vendor's business rather than the user's drift. + * ordinary work, not a problem; a plugin's own copy of a same-named skill is the vendor's + * business rather than the user's drift, and a project's own copy is outside the reach of + * the only update Orca runs. */ export function isSkillCopyNeedingAttention(installation: SkillFreshnessInstallation): boolean { return ( + skillPlacementParticipatesInGlobalFreshness(installation) && installation.status !== 'current' && // Why: 'newer-known' is recognized official content ahead of this build — the // updater's own install or a newer release's bytes. There is nothing to fix and