From 49cfbf014c417396aad442e2c3a09ef9d7c79b8c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 30 Jul 2026 13:20:47 -0700 Subject: [PATCH 1/2] fix(skills): stop OS sidecars marking an untouched skill as modified (#11471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): stop OS sidecars marking an untouched skill as modified Package identity compared a live user directory against a tree read from a clean checkout, so anything the OS deposited counted as drift. One Finder visit writes .DS_Store, which sorts before SKILL.md and misaligns the index-aligned snapshot comparison — the copy became 'unrecognized', was reported as "may be modified... Remove it", and left out of the update. Running the update could not clear it either: the updater compares its lock to the source and never reads disk, so it correctly reports "up to date" and writes nothing. Ignore OS-authored names on both sides of the comparison. The generator half is not hypothetical: a stray sidecar in a working tree made the committed artifacts read as stale, failing lint for that developer. Scoped to OS-authored names only. Tolerating unexpected files in general would let an injected payload ride along beside a clean SKILL.md; these are safe because an official SKILL.md never references them, so no agent can be routed into one. Mode bits are deliberately untouched — that would weaken identity for real scripts. * fix(skills): keep guarding a directory or link wearing an OS metadata name The name-only skip dropped any entry matching an OS metadata name, so a directory named .DS_Store or ._scripts took its whole subtree out of identity and a symlink wearing one stopped tripping the link guard — a skill hiding either read as pristine. The OS writes these as plain files only, so the entry type decides, still ahead of the case-fold map. Also compares both walkers over the same fixture: an asymmetric skip is worse than none, since one side would bake in content the other can never observe. * chore: ignore the OS metadata names skill identity already skips Both skill-identity walkers ignore these names, but .gitignore covered only .DS_Store and Thumbs.db — so a stray ._SKILL.md showed as untracked and `git add -A` could commit it. That is the one way the two walkers can disagree: the disk walker skips such a file while the git-tree producer (collectGitPackageFiles, used by the unreferenced --rebuild-from-tags path) does not, so a committed sidecar would make released history and observation describe different content. Ignoring them keeps that asymmetry unreachable rather than adding a second skip to the released-history path, which is load-bearing and provably never sees one today: no committed sidecar exists on any ref. Nothing tracked matches the new patterns. * chore: correct the skill-identity ignore comment The previous wording claimed these names cannot be committed, which overstates what .gitignore provides: `git add -f` and `git apply --index` both bypass it, so a cherry-pick, rebase or fork branch already carrying a sidecar is unaffected. That clause was load-bearing — it was the stated reason for leaving the released-history producer unhardened — so it should not read as a structural guarantee. Also fixes the producer count (three, not two: two disk walkers plus the git-tree producer, which does not skip) and says plain file, since the skip is isFile()-gated so a directory or link wearing the name is still walked. --- .gitignore | 6 ++ .../generate-skill-bundle-manifest.mjs | 21 ++++- .../generate-skill-bundle-manifest.test.mjs | 85 +++++++++++++++++++ .../skills/skill-freshness-inventory.test.ts | 26 ++++++ .../skills/skill-package-identity.test.ts | 79 +++++++++++++++++ src/main/skills/skill-package-identity.ts | 32 +++++++ 6 files changed, 248 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 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) || From 8ea8fe1a6d4da8074eb3716c59ac725428c6ef16 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 30 Jul 2026 13:21:03 -0700 Subject: [PATCH 2/2] fix(skills): stop a project's own skill copy driving global freshness (#11474) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): stop a project's own skill copy driving global freshness A pristine global install plus a drifted copy inside a work directory showed Settings -> Computer Use as amber "Needs attention", with the copy labelled "may be modified ... Remove it if you want Orca to update this skill", while the same page said "Computer Use is ready" and the update command correctly printed "All global skills are up to date". No user action could clear it. Orca's updater only ever passes --global, so a copy a project owns has no remedy by design. Two defects made it drive the global verdict anyway: - locationChip tested byte status before topology, so an unrecognized repo-scope placement returned early and never reached its 'in-a-repo' case. Because SKIPPED_REASON_PRIORITY ranks unrecognized above in-a-repo, the summary sentence was wrong too. - isSkillCopyNeedingAttention excepted plugin-cache but not repo-scope. Stated by scope rather than by byte status: an outdated or unreadable project copy is 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 another. Chip precedence is now explicit -- a read failure outranks ownership so that rule cannot hide a real fault, and ownership outranks byte status. Ownership suppresses the group, never a location's visibility: a project copy is still listed whenever another placement earns the row. The badge predicate deliberately omits the shared helper's outdated carve-out, so a non-eligible outdated copy stays amber. Collapsing the two into one predicate would flip that to green while the dialog still shows its reinstall row, so the distinction is preserved and pinned by a regression guard. Eligibility needed no change: it already filters to convergent placements. * fix(skills): keep a project copy from explaining a global skill's skip Review follow-up. The chip and the group no longer treat a project-owned copy as global drift, but three surfaces still read it as one: - skippedReason derived its one sentence from the highest-priority chip among a group's locations, with 'in-a-repo' ranked above 'duplicate', 'external-link', 'broken-link' and above the no-chip case that hands over the reinstall command. A repo-scope copy can no longer earn a group, so whenever it won it explained a skip it had no part in — and swallowed the one runnable remedy. SkillLocationRow now carries whether the update judged it, and only judged rows explain. That also covers the scan-limit sentinel, which is repo-scope and chips 'inaccessible'. - hasSkillCopyNeedingAttention counted project copies as the presence that makes a plugin-cache read failure a skill's problem, while the status function skipped them — the disagreement the two exist to prevent. - The nudge mixed project copies into its dismissal fingerprint, so re-checking out a repo re-raised a nudge the user had already dismissed. plugin-cache is untouched: it stays on the judged side everywhere, because updating the plugin is a remedy a project copy does not have. --- .../skills/SkillFreshnessNudge.test.tsx | 53 ++++++++ .../components/skills/SkillFreshnessNudge.tsx | 5 + .../SkillFreshnessUpdateDialog.test.tsx | 101 ++++++++++++++- .../skills/skill-freshness-grouping.test.ts | 121 ++++++++++++++++-- .../skills/skill-freshness-grouping.ts | 36 ++++-- .../skill-freshness-skipped-reason.test.ts | 39 +++++- .../skills/skill-freshness-skipped-reason.ts | 6 +- .../skill-freshness-display-status.test.ts | 62 +++++++++ .../src/lib/skill-freshness-display-status.ts | 13 +- src/shared/skill-freshness.ts | 31 ++++- 10 files changed, 442 insertions(+), 25 deletions(-) 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