Merge remote-tracking branch 'origin/main' into brennanb2025/bullhead

This commit is contained in:
Brennan Benson
2026-07-30 13:25:46 -07:00
16 changed files with 690 additions and 26 deletions
+6
View File
@@ -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'))
+32
View File
@@ -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) ||
@@ -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
@@ -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
@@ -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()
@@ -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<SkillFreshnessInstallation>): 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]
])
})
})
@@ -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,
@@ -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')
@@ -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))
}
@@ -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)
})
})
@@ -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 &&
+29 -2
View File
@@ -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<SkillInstallationTopology> = 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