From 2cf91b8e690a463491be3acd0f0a5ad107e04b41 Mon Sep 17 00:00:00 2001 From: Hansss <79178335+5Hyeons@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:11:57 +0900 Subject: [PATCH] fix(skills): complete plugin-cache scans without false attention (#10865) Fixes the P0 where skill cards showed an unclearable amber "Needs attention" while the Details dialog reported everything up to date. Root cause: when the plugin-cache scan tripped one of its own bounds it recorded an incomplete path, and inventorySkillFreshness expanded that into one fabricated placement per manifest skill at a path it never stat'ed. Those synthetic "inaccessible" copies lit the pill, were filtered out of the dialog, and could never be cleared because plugin-cache is not an updatable topology. - Removes the fabrication; reports typed scan issues instead. - Requires readable SKILL.md evidence before promoting a directory to a candidate, so a same-named foreign plugin (Codex's own computer-use) no longer flags. - Prunes skill payloads and node_modules so ordinary vendor caches stop tripping the depth and entry bounds. - Partitions scan reasons: only a real read failure raises a pill; bounds that ended the walk block an all-clear claim; the rest are Details-only. Fixes #10633. Refs #10659, #10904, #10918, #10775, #10791, #10813. --- src/main/ipc/skills.test.ts | 1 + .../skills/skill-freshness-inventory.test.ts | 180 +++++ src/main/skills/skill-freshness-inventory.ts | 39 +- .../skills/skill-plugin-cache-scan.test.ts | 645 +++++++++++++++++- src/main/skills/skill-plugin-cache-scan.ts | 218 ++++-- .../skills/skill-plugin-manifest-roots.ts | 145 ++++ .../src/components/codex-restart-chip.test.ts | 16 +- .../skills/SkillFreshnessNudge.test.tsx | 3 + .../skills/SkillFreshnessStatusPill.test.tsx | 1 + .../SkillFreshnessUpdateDialog.test.tsx | 106 +++ .../skills/SkillFreshnessUpdateDialog.tsx | 22 +- .../skills/skill-freshness-grouping.test.ts | 3 + .../skills/skill-freshness-grouping.ts | 5 +- .../skills/skill-freshness-scan-issues.tsx | 74 ++ .../skills/skill-freshness-skipped-reason.ts | 2 - .../skill-freshness-summary-headline.tsx | 48 +- .../src/hooks/useSkillFreshness.test.tsx | 2 +- src/renderer/src/i18n/locales/en.json | 11 +- src/renderer/src/i18n/locales/es.json | 11 +- src/renderer/src/i18n/locales/ja.json | 11 +- src/renderer/src/i18n/locales/ko.json | 11 +- src/renderer/src/i18n/locales/zh.json | 11 +- .../agent-skill-nav-install-status.test.ts | 2 +- .../skill-freshness-display-status.test.ts | 122 +++- .../src/lib/skill-freshness-display-status.ts | 38 +- src/renderer/src/web/web-preload-api.ts | 1 + src/shared/skill-freshness.ts | 49 ++ 27 files changed, 1657 insertions(+), 120 deletions(-) create mode 100644 src/main/skills/skill-plugin-manifest-roots.ts create mode 100644 src/renderer/src/components/skills/skill-freshness-scan-issues.tsx diff --git a/src/main/ipc/skills.test.ts b/src/main/ipc/skills.test.ts index 4a412f249..e6f88345f 100644 --- a/src/main/ipc/skills.test.ts +++ b/src/main/ipc/skills.test.ts @@ -77,6 +77,7 @@ describe('registerSkillsHandlers', () => { schemaVersion: 1, installations: [], eligibleUpdateNames: [], + scanIssues: [], scannedAt: 1 }) getWslHomeMock.mockReturnValue('\\\\wsl.localhost\\Ubuntu\\home\\alice') diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index 2b010639e..c04616abb 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -311,6 +311,111 @@ describe('read-only skill freshness inventory', () => { } ) + it('keeps another ecosystem’s same-name plugin skill unrecognized', async () => { + // Why: Codex ships its own `computer-use` plugin. Reported in #10633 — the copy is + // not ours, not the user's to delete, and left amber with no action available. + const test = await fixture() + await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown) + const pluginRoot = join( + test.homeDir, + '.codex', + 'plugins', + 'cache', + 'openai-bundled', + 'orca-cli' + ) + await mkdir(pluginRoot, { recursive: true }) + await writeFile(join(pluginRoot, 'SKILL.md'), '---\nname: orca-cli\n---\n\nAnother tool.\n') + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + expect(inventory.installations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + unresolvedPath: pluginRoot, + topology: 'plugin-cache', + status: 'unrecognized' + }) + ]) + ) + expect(inventory.eligibleUpdateNames).toEqual([]) + }) + + it('keeps a plugin-cache copy with known official files unrecognized', async () => { + const test = await fixture() + await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown) + const modifiedRoot = join( + test.homeDir, + '.codex', + 'plugins', + 'cache', + 'openai-bundled', + 'modified', + 'orca-cli' + ) + await mkdir(modifiedRoot, { recursive: true }) + await writeFile(join(modifiedRoot, 'SKILL.md'), test.currentMarkdown) + await writeFile(join(modifiedRoot, 'README.md'), 'Modified official package\n') + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + expect(inventory.installations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + unresolvedPath: modifiedRoot, + status: 'unrecognized' + }) + ]) + ) + }) + + it.each([ + ['.claude', 'skills'], + ['.agents', 'skills'] + ])('keeps an unrecognized copy under %s/%s the user’s to review', async (...segments) => { + const test = await fixture() + await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown) + await test.writeSkill( + join(test.homeDir, ...segments), + '---\nname: orca-cli\n---\n\nAnother tool.\n' + ) + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + expect(inventory.installations.some((entry) => entry.status === 'unrecognized')).toBe(true) + }) + + it('does not classify an empty plugin-cache directory as a skill', async () => { + const test = await fixture() + await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown) + const emptyRoot = join(test.homeDir, '.codex', 'plugins', 'cache', 'vendor', 'orca-cli') + await mkdir(emptyRoot, { recursive: true }) + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + expect(inventory.installations.some((entry) => entry.unresolvedPath === emptyRoot)).toBe(false) + }) + it('accepts CRLF as the same official text identity', async () => { const test = await fixture() await test.writeSkill( @@ -377,4 +482,79 @@ describe('read-only skill freshness inventory', () => { // command does not touch, so the limit is reported without blocking the update. expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) }) + + it('scans a real-shaped plugin cache completely and leaves eligibility unchanged', async () => { + const test = await fixture() + await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.oldMarkdown) + const packageRoot = join( + test.homeDir, + '.codex', + 'plugins', + 'cache', + 'openai-bundled', + 'orca-cli', + '1.0.0' + ) + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills/"}\n') + const pluginSkill = await test.writeSkill(join(packageRoot, 'skills'), test.currentMarkdown) + await mkdir(join(pluginSkill, 'templates', 'starter', 'examples', 'd1', 'app', 'api'), { + recursive: true + }) + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + // The plugin copy is real and reported at its own path — never at a joined path, + // and never at the same-named plugin directory two levels above it. + expect(inventory.scanIssues).toEqual([]) + expect(inventory.installations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ topology: 'plugin-cache', unresolvedPath: pluginSkill }) + ]) + ) + // Why: a plugin-cache copy is not convergent, so it neither grants nor withholds + // the update. The outdated canonical copy alone decides, exactly as before. + expect(inventory.eligibleUpdateNames).toEqual(['orca-cli']) + }) + + it('reports incomplete plugin coverage without inventing per-skill installations', async () => { + const test = await fixture() + await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown) + const pluginCache = join(test.homeDir, '.codex', 'plugins', 'cache') + await mkdir(join(pluginCache, ...Array.from({ length: 11 }, (_, index) => `level-${index}`)), { + recursive: true + }) + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + expect(inventory.installations).toHaveLength(1) + expect(inventory.installations[0]).toMatchObject({ + name: 'orca-cli', + status: 'current', + topology: 'canonical-copy' + }) + expect(inventory.installations).not.toEqual( + expect.arrayContaining([ + expect.objectContaining({ errorCategory: 'plugin-cache-scan-incomplete' }) + ]) + ) + expect(inventory.scanIssues).toEqual([ + expect.objectContaining({ + rootId: 'codex-plugin-cache', + sourceLabel: 'Codex plugin cache', + reason: 'depth-limit', + errorCode: null + }) + ]) + }) }) diff --git a/src/main/skills/skill-freshness-inventory.ts b/src/main/skills/skill-freshness-inventory.ts index 8b1c5c190..97527906d 100644 --- a/src/main/skills/skill-freshness-inventory.ts +++ b/src/main/skills/skill-freshness-inventory.ts @@ -123,8 +123,8 @@ export async function inventorySkillFreshness(args: { scan: await scanKnownPluginSkillCandidates(root.path, new Set(currentByName.keys())) })) ) - const pluginTasks = pluginScans.flatMap(({ root, scan }) => [ - ...scan.candidates.flatMap((candidate) => { + const pluginTasks = pluginScans.flatMap(({ root, scan }) => + scan.candidates.flatMap((candidate) => { const current = currentByName.get(candidate.name) return current ? [ @@ -139,31 +139,15 @@ export async function inventorySkillFreshness(args: { }) ] : [] - }), - // Why: unreadable plugin subtrees could hide any official name. An - // incomplete scan must conservatively poison every name rather than imply absence. - ...scan.incompletePaths.flatMap((incompletePath) => - artifacts.manifest.skills.map( - (current) => () => - observeSkillFreshnessInstallation({ - current, - currentAppVersion: args.currentAppVersion, - artifacts, - rootId: root.id, - providers: root.providers, - sourceKind: 'plugin', - sourceLabel: root.label, - unresolvedPath: join(incompletePath, current.name), - topology: { - topology: 'plugin-cache', - resolvedPath: null, - identity: null, - errorCategory: 'plugin-cache-scan-incomplete' - } - }) - ) - ) - ]) + }) + ) + const scanIssues = pluginScans.flatMap(({ root, scan }) => + scan.issues.map((issue) => ({ + rootId: root.id, + sourceLabel: root.label, + ...issue + })) + ) const unsupportedInstallations = ( await runSkillCandidateTasks([...repoTasks, ...omittedRepoTasks, ...pluginTasks]) ).filter((installation): installation is SkillFreshnessInstallation => installation !== null) @@ -180,6 +164,7 @@ export async function inventorySkillFreshness(args: { schemaVersion: 1, installations, eligibleUpdateNames: eligibleSkillUpdateNames(installations), + scanIssues, scannedAt: Date.now() } } diff --git a/src/main/skills/skill-plugin-cache-scan.test.ts b/src/main/skills/skill-plugin-cache-scan.test.ts index 46216ce6c..442af48ce 100644 --- a/src/main/skills/skill-plugin-cache-scan.test.ts +++ b/src/main/skills/skill-plugin-cache-scan.test.ts @@ -1,10 +1,16 @@ -import { mkdir, mkdtemp, rm } from 'node:fs/promises' +import { execFile } from 'node:child_process' +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { promisify } from 'node:util' import { afterEach, describe, expect, it } from 'vitest' -import { scanKnownPluginSkillCandidates } from './skill-plugin-cache-scan' +import { + MAXIMUM_PLUGIN_SCAN_ISSUES, + scanKnownPluginSkillCandidates +} from './skill-plugin-cache-scan' const temporaryDirectories: string[] = [] +const execFileAsync = promisify(execFile) afterEach(async () => { await Promise.all(temporaryDirectories.splice(0).map((root) => rm(root, { recursive: true }))) @@ -15,16 +21,84 @@ describe('plugin skill candidate scan', () => { const root = await mkdtemp(join(tmpdir(), 'orca-plugin-skill-scan-')) temporaryDirectories.push(root) await Promise.all( - ['one', 'two'].map((vendor) => mkdir(join(root, vendor, 'orca-cli'), { recursive: true })) + ['one', 'two'].map(async (vendor) => { + await mkdir(join(root, vendor, 'orca-cli'), { recursive: true }) + await writeFile(join(root, vendor, 'orca-cli', 'SKILL.md'), '# Orca CLI\n') + }) ) const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), 1) expect(result.candidates).toHaveLength(1) - expect(result.incompletePaths).toEqual([root]) + expect(result.issues).toEqual([{ path: root, reason: 'candidate-limit', errorCode: null }]) }) - it('marks depth-truncated subtrees incomplete so hidden skills poison eligibility', async () => { + it('completes a real-shaped Codex cache without reporting coverage issues', async () => { + // Mirrors ~/.codex/plugins/cache: ///.codex-plugin, with the + // skill's own payload nesting well past the raw traversal depth (issue #10659). + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-real-shape-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'openai-bundled', 'sites', '0.1.31') + const skill = join(packageRoot, 'skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir( + join(skill, 'templates', 'vinext-starter', 'examples', 'd1', 'app', 'api', 'deep'), + { recursive: true } + ) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills/"}\n') + await writeFile(join(skill, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [{ name: 'orca-cli', path: skill }], issues: [] }) + }) + + it('does not emit a plugin directory that only shares a skill name', async () => { + // The cached plugin is itself called orca-cli. Only the SKILL.md below it is a skill. + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-name-collision-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'openai-bundled', 'orca-cli', '1.0.0') + const skill = join(packageRoot, 'skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(skill, { recursive: true }) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills/"}\n') + await writeFile(join(skill, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [{ name: 'orca-cli', path: skill }], issues: [] }) + }) + + it('does not emit a bare known-name directory that carries no SKILL.md', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-bare-name-')) + temporaryDirectories.push(root) + await mkdir(join(root, 'vendor', 'orca-cli', 'assets'), { recursive: true }) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [], issues: [] }) + }) + + it('stops descending once a skill package payload exceeds the nested skill budget', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-payload-prune-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const skill = join(packageRoot, 'skills', 'sites-building') + const buried = join(skill, 'templates', 'starter', 'examples', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(buried, { recursive: true }) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills"}\n') + await writeFile(join(skill, 'SKILL.md'), '# Sites building\n') + await writeFile(join(buried, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: pruning payload is a topology decision, so it must stay silent rather than + // surface as a coverage issue the user is asked to act on. + expect(result).toEqual({ candidates: [], issues: [] }) + }) + + it('reports a depth-truncated subtree as scan coverage instead of a skill candidate', async () => { const root = await mkdtemp(join(tmpdir(), 'orca-plugin-skill-depth-')) temporaryDirectories.push(root) const segments = Array.from({ length: 11 }, (_, index) => `level-${index}`) @@ -34,7 +108,564 @@ describe('plugin skill candidate scan', () => { const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) expect(result.candidates).toEqual([]) - expect(result.incompletePaths).toHaveLength(1) - expect(hiddenSkill.startsWith(result.incompletePaths[0] ?? '')).toBe(true) + expect(result.issues).toHaveLength(1) + expect(result.issues[0]).toMatchObject({ reason: 'depth-limit', errorCode: null }) + expect(hiddenSkill.startsWith(result.issues[0]?.path ?? '')).toBe(true) }) + + it('does not scan dependency packages for plugin skill entrypoints', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-dependencies-')) + temporaryDirectories.push(root) + await mkdir( + join( + root, + 'vendor', + 'plugin', + 'scripts', + 'node_modules', + ...Array.from({ length: 12 }, (_, index) => `level-${index}`) + ), + { recursive: true } + ) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [], issues: [] }) + }) + + it('scans only declared Codex plugin skill roots', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-manifest-roots-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const candidate = join(packageRoot, 'custom-skills', 'group', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await mkdir( + join(packageRoot, 'payload', ...Array.from({ length: 12 }, (_, index) => `level-${index}`)), + { recursive: true } + ) + await writeFile( + join(packageRoot, '.codex-plugin', 'plugin.json'), + '{"skills":["./custom-skills","./skills"]}\n' + ) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [] + }) + }) + + it('uses the default skills root for compatible manifests without a skills field', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-default-root-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const candidate = join(packageRoot, 'skills', 'nested', 'orca-cli') + await mkdir(join(packageRoot, '.claude-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await writeFile(join(packageRoot, '.claude-plugin', 'plugin.json'), '{"name":"plugin"}\n') + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [] + }) + }) + + it('falls back to traversal when a manifest skills path is invalid', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-invalid-declared-root-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const candidate = join(packageRoot, 'custom-skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await writeFile( + join(packageRoot, '.codex-plugin', 'plugin.json'), + '{"skills":"custom-skills"}\n' + ) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [] + }) + }) + + it('does not traverse plugin payload when the default skills root is missing', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-missing-default-root-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir( + join(packageRoot, 'commands', ...Array.from({ length: 11 }, (_, index) => `level-${index}`)), + { recursive: true } + ) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"name":"plugin"}\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [], issues: [] }) + }) + + it('does not traverse plugin payload when the manifest declares no skill roots', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-empty-skill-roots-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir( + join(packageRoot, 'commands', ...Array.from({ length: 11 }, (_, index) => `level-${index}`)), + { recursive: true } + ) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":[]}\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [], issues: [] }) + }) + + it('discovers nested skill packages recursively within declared roots', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-skill-boundary-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const skillRoot = join(packageRoot, 'skills', 'sites-building') + await mkdir(join(packageRoot, '.claude-plugin'), { recursive: true }) + await mkdir(join(skillRoot, 'templates', 'orca-cli'), { recursive: true }) + await writeFile(join(packageRoot, '.claude-plugin', 'plugin.json'), '{"skills":"./skills"}\n') + await writeFile(join(skillRoot, 'SKILL.md'), '# Sites building\n') + await writeFile(join(skillRoot, 'templates', 'orca-cli', 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: join(skillRoot, 'templates', 'orca-cli') }], + issues: [] + }) + }) + + it('rejects Windows parent traversal without hiding the default skills root', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-windows-parent-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const candidate = join(packageRoot, 'skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await writeFile( + join(packageRoot, '.codex-plugin', 'plugin.json'), + '{"skills":"./..\\\\outside"}\n' + ) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [] + }) + }) + + it('falls through empty manifest directories to the first manifest file', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-manifest-precedence-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const candidate = join(packageRoot, 'custom-skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(join(packageRoot, '.claude-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await mkdir( + join(packageRoot, 'payload', ...Array.from({ length: 12 }, (_, index) => `level-${index}`)), + { recursive: true } + ) + await writeFile( + join(packageRoot, '.claude-plugin', 'plugin.json'), + '{"skills":"./custom-skills"}\n' + ) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [] + }) + }) + + it('bounds how many skill roots one manifest can declare', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-manifest-budget-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const manifestPath = join(packageRoot, '.codex-plugin', 'plugin.json') + const candidate = join(packageRoot, 'r0000', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + await writeFile( + manifestPath, + JSON.stringify({ + skills: Array.from({ length: 4096 }, (_, index) => `./r${String(index).padStart(4, '0')}`) + }) + ) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: resolving declared roots bypasses the entry budget, so without this cap the + // manifest alone decides how long the scan runs. Falling back to the bounded walk + // still finds the skill, so the cap costs coverage nothing. + expect(result.candidates).toEqual([{ name: 'orca-cli', path: candidate }]) + expect(result.issues).toEqual([ + { path: manifestPath, reason: 'manifest-limit', errorCode: null } + ]) + }) + + it('keeps valid roots when a skills array contains an invalid value', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-invalid-root-array-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const defaultCandidate = join(packageRoot, 'skills', 'orca-cli') + const declaredCandidate = join(packageRoot, 'custom-skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(defaultCandidate, { recursive: true }) + await mkdir(declaredCandidate, { recursive: true }) + await writeFile( + join(packageRoot, '.codex-plugin', 'plugin.json'), + '{"skills":["./custom-skills",7]}\n' + ) + await writeFile(join(defaultCandidate, 'SKILL.md'), '# Wrong root\n') + await writeFile(join(declaredCandidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: declaredCandidate }], + issues: [] + }) + }) + + it('does not reset the depth budget across nested plugin manifests', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-nested-manifests-')) + temporaryDirectories.push(root) + let pluginRoot = join(root, 'vendor', 'plugin', '1.0.0') + for (let index = 0; index < 8; index += 1) { + await mkdir(join(pluginRoot, '.codex-plugin'), { recursive: true }) + await writeFile(join(pluginRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills"}\n') + pluginRoot = join(pluginRoot, 'skills', `nested-${index}`) + } + const hiddenCandidate = join(pluginRoot, 'orca-cli') + await mkdir(hiddenCandidate, { recursive: true }) + await writeFile(join(hiddenCandidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result.candidates).toEqual([]) + expect(result.issues).toContainEqual( + expect.objectContaining({ reason: 'depth-limit', errorCode: null }) + ) + }) + + it('ignores non-directory manifest markers when selecting precedence', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-manifest-marker-file-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin', '1.0.0') + const candidate = join(packageRoot, 'custom-skills', 'orca-cli') + await mkdir(packageRoot, { recursive: true }) + await mkdir(join(packageRoot, '.claude-plugin'), { recursive: true }) + await mkdir(candidate, { recursive: true }) + await mkdir( + join(packageRoot, 'payload', ...Array.from({ length: 12 }, (_, index) => `level-${index}`)), + { recursive: true } + ) + await writeFile(join(packageRoot, '.codex-plugin'), 'not a directory\n') + await writeFile( + join(packageRoot, '.claude-plugin', 'plugin.json'), + '{"skills":"./custom-skills"}\n' + ) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [] + }) + }) + + it('reports manifests that exceed the bounded read limit', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-large-manifest-')) + temporaryDirectories.push(root) + const manifestPath = join(root, 'vendor', 'plugin', '.codex-plugin', 'plugin.json') + await mkdir(join(root, 'vendor', 'plugin', '.codex-plugin'), { recursive: true }) + await writeFile(manifestPath, ' '.repeat(256 * 1024 + 1)) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result.issues).toContainEqual({ + path: manifestPath, + reason: 'manifest-limit', + errorCode: null + }) + }) + + // Why (this and every other skipIf below): creating a symlink on Windows needs + // elevation or Developer Mode, so these would fail EPERM in setup rather than + // exercise the behavior under test. The non-symlink cases still run there. + it.skipIf(process.platform === 'win32')( + 'reports a symlink target that cannot be inspected', + async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-symlink-error-')) + temporaryDirectories.push(root) + const linkPath = join(root, 'loop') + await symlink('loop', linkPath, 'dir') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result.issues).toContainEqual({ + path: linkPath, + reason: 'io-error', + errorCode: 'ELOOP' + }) + } + ) + + it.skipIf(process.platform === 'win32')( + 'preserves read failures when the scan issue limit is reached', + async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-issue-limit-')) + temporaryDirectories.push(root) + await Promise.all( + Array.from({ length: 16 }, async (_, index) => { + const name = `loop-${index.toString().padStart(2, '0')}` + await symlink(name, join(root, name), 'dir') + }) + ) + const packageRoot = join(root, 'zz-package') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await symlink('SKILL.md', join(packageRoot, 'SKILL.md'), 'file') + await symlink('plugin.json', join(packageRoot, '.codex-plugin', 'plugin.json'), 'file') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result.issues).toContainEqual({ + path: join(root, 'loop-00'), + reason: 'io-error', + errorCode: 'ELOOP' + }) + expect(result.issues).toContainEqual({ + path: root, + reason: 'issue-limit', + errorCode: null + }) + expect(result.issues.filter((issue) => issue.reason === 'issue-limit')).toHaveLength(1) + } + ) + + it('reports the bound that ended the walk even with the issue budget spent', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-truncating-issue-')) + temporaryDirectories.push(root) + await Promise.all( + Array.from({ length: 16 }, (_, index) => + mkdir( + join( + root, + `deep-${index.toString().padStart(2, '0')}`, + ...Array.from({ length: 11 }, (_, level) => `level-${level}`) + ), + { recursive: true } + ) + ) + ) + await Promise.all( + ['zz-one', 'zz-two'].map(async (vendor) => { + await mkdir(join(root, vendor, 'orca-cli'), { recursive: true }) + await writeFile(join(root, vendor, 'orca-cli', 'SKILL.md'), '# Orca CLI\n') + }) + ) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), 1) + + // Why: the deep trees above exist to spend the display budget, so assert it is full — + // otherwise this passes with budget to spare and stops covering the case it is named + // for. Not 'issue-limit': that only appears when a non-required issue is dropped, and + // the truncating bound below bypasses the budget instead of being dropped by it. + expect(result.issues.filter((issue) => issue.reason === 'depth-limit')).toHaveLength( + MAXIMUM_PLUGIN_SCAN_ISSUES + ) + // Why: losing this one to the display budget is what lets a scan that stopped early + // report all-clear — the bounds that merely skipped a folder say nothing about it. + expect(result.issues).toContainEqual({ path: root, reason: 'candidate-limit', errorCode: null }) + }) + + it('keeps scanning past the issue budget', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-issue-budget-coverage-')) + temporaryDirectories.push(root) + const candidate = join(root, 'zz-package', 'skills', 'orca-cli') + await Promise.all( + Array.from({ length: 20 }, (_, index) => + mkdir( + join( + root, + `deep-${index.toString().padStart(2, '0')}`, + ...Array.from({ length: 11 }, (_, level) => `level-${level}`) + ), + { recursive: true } + ) + ) + ) + await mkdir(candidate, { recursive: true }) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: the issue budget bounds what the dialog lists, not how far the walk reaches. + // Ending the scan there would drop real copies and still report all-clear, because + // none of the bounds that filled the budget raise attention. + expect(result.candidates).toEqual([{ name: 'orca-cli', path: candidate }]) + expect(result.issues).toContainEqual({ path: root, reason: 'issue-limit', errorCode: null }) + }) + + it.skipIf(process.platform === 'win32')( + 'still names a fail-closed candidate once the issue budget is spent', + async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-issue-budget-candidate-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'zz-package') + const candidate = join(packageRoot, 'skills', 'orca-cli') + await Promise.all( + Array.from({ length: 16 }, (_, index) => + mkdir( + join( + root, + `deep-${index.toString().padStart(2, '0')}`, + ...Array.from({ length: 11 }, (_, level) => `level-${level}`) + ), + { recursive: true } + ) + ) + ) + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(join(packageRoot, 'skills'), { recursive: true }) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills"}\n') + await symlink('missing-target', candidate, 'dir') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: the depth bounds fill the issue budget first. Dropping this one for budget + // would leave the badge amber over a candidate the dialog never mentions. + expect(result.candidates).toEqual([{ name: 'orca-cli', path: candidate }]) + expect(result.issues).toContainEqual({ + path: candidate, + reason: 'io-error', + errorCode: 'ENOENT' + }) + } + ) + + it.skipIf(process.platform === 'win32')( + 'names the path when a dangling known-name symlink is kept as a candidate', + async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-declared-dangling-symlink-')) + temporaryDirectories.push(root) + const packageRoot = join(root, 'vendor', 'plugin') + const candidate = join(packageRoot, 'skills', 'orca-cli') + await mkdir(join(packageRoot, '.codex-plugin'), { recursive: true }) + await mkdir(join(packageRoot, 'skills'), { recursive: true }) + await writeFile(join(packageRoot, '.codex-plugin', 'plugin.json'), '{"skills":"./skills"}\n') + await symlink('missing-target', candidate, 'dir') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: this candidate resolves to nothing, so it reads as inaccessible and raises + // attention. Without the issue the dialog would report all-clear against a badge + // that says otherwise, and nothing would ever name the broken link. + expect(result).toEqual({ + candidates: [{ name: 'orca-cli', path: candidate }], + issues: [{ path: candidate, reason: 'io-error', errorCode: 'ENOENT' }] + }) + } + ) + + it.skipIf(process.platform === 'win32')( + 'does not follow directory symlinks outside the plugin cache', + async () => { + const parent = await mkdtemp(join(tmpdir(), 'orca-plugin-symlink-outside-')) + temporaryDirectories.push(parent) + const root = join(parent, 'cache') + const outside = join(parent, 'outside') + const linkPath = join(root, 'vendor') + await mkdir(join(outside, 'orca-cli'), { recursive: true }) + await mkdir(root, { recursive: true }) + await writeFile(join(outside, 'orca-cli', 'SKILL.md'), '# Orca CLI\n') + await symlink(outside, linkPath, 'dir') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [], + issues: [{ path: linkPath, reason: 'outside-root', errorCode: null }] + }) + } + ) + + it.skipIf(process.platform === 'win32')( + 'does not follow a SKILL.md symlink outside the plugin cache', + async () => { + const parent = await mkdtemp(join(tmpdir(), 'orca-plugin-skill-file-outside-')) + temporaryDirectories.push(parent) + const root = join(parent, 'cache') + const skill = join(root, 'vendor', 'orca-cli') + const outsideSkillFile = join(parent, 'outside', 'SKILL.md') + await mkdir(skill, { recursive: true }) + await mkdir(join(parent, 'outside'), { recursive: true }) + await writeFile(outsideSkillFile, '# Orca CLI\n') + await symlink(outsideSkillFile, join(skill, 'SKILL.md'), 'file') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [], + issues: [{ path: join(skill, 'SKILL.md'), reason: 'outside-root', errorCode: null }] + }) + } + ) + + it.skipIf(process.platform === 'win32')( + 'does not read manifest symlinks outside the plugin cache', + async () => { + const parent = await mkdtemp(join(tmpdir(), 'orca-plugin-manifest-outside-')) + temporaryDirectories.push(parent) + const root = join(parent, 'cache') + const outsideManifest = join(parent, 'plugin.json') + const manifestPath = join(root, '.codex-plugin', 'plugin.json') + await mkdir(join(root, '.codex-plugin'), { recursive: true }) + await writeFile(outsideManifest, '{"skills":"./outside"}\n') + await symlink(outsideManifest, manifestPath, 'file') + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ + candidates: [], + issues: [{ path: manifestPath, reason: 'outside-root', errorCode: null }] + }) + } + ) + + it.skipIf(process.platform === 'win32')( + 'does not block on a manifest FIFO', + async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-manifest-fifo-')) + temporaryDirectories.push(root) + const manifestPath = join(root, '.codex-plugin', 'plugin.json') + await mkdir(join(root, '.codex-plugin'), { recursive: true }) + await execFileAsync('mkfifo', [manifestPath]) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + expect(result).toEqual({ candidates: [], issues: [] }) + }, + 1_000 + ) }) diff --git a/src/main/skills/skill-plugin-cache-scan.ts b/src/main/skills/skill-plugin-cache-scan.ts index 7162cedbc..78fbc53cd 100644 --- a/src/main/skills/skill-plugin-cache-scan.ts +++ b/src/main/skills/skill-plugin-cache-scan.ts @@ -1,20 +1,40 @@ import type { Dirent } from 'node:fs' import { opendir, realpath, stat } from 'node:fs/promises' -import { join } from 'node:path' +import { basename, join } from 'node:path' +import { + isTruncatingSkillScanReason, + type SkillFreshnessScanIssueReason +} from '../../shared/skill-freshness' +import { declaredPluginSkillRoots, isWithinRoot } from './skill-plugin-manifest-roots' const MAXIMUM_PLUGIN_SCAN_DEPTH = 9 -const MAXIMUM_PLUGIN_SCAN_ENTRIES = 4_096 +const MAXIMUM_DECLARED_SKILL_SCAN_DEPTH = 6 +// Why: a skill package's own payload (templates, fixtures, sample apps) is not a skill +// tree, and it is what drives ordinary caches past the depth and entry bounds. Descend +// far enough to still find a skill grouped under a package, then stop. +const MAXIMUM_NESTED_SKILL_DEPTH = 2 +// Why: sized against a real multi-vendor cache, which reads ~7k entries once payload is +// pruned. The bound still exists to stop a hostile or runaway tree; it is not a budget +// ordinary installs are meant to exhaust. +const MAXIMUM_PLUGIN_SCAN_ENTRIES = 16_384 export const MAXIMUM_PLUGIN_SKILL_CANDIDATES = 64 -const MAXIMUM_PLUGIN_INCOMPLETE_PATHS = 16 +export const MAXIMUM_PLUGIN_SCAN_ISSUES = 16 +const SKILL_FILE_NAME = 'SKILL.md' export type KnownPluginSkillCandidate = { name: string path: string } +export type KnownPluginSkillScanIssue = { + path: string + reason: SkillFreshnessScanIssueReason + errorCode: string | null +} + export type KnownPluginSkillScan = { candidates: KnownPluginSkillCandidate[] - incompletePaths: string[] + issues: KnownPluginSkillScanIssue[] } function errorCode(error: unknown): string | null { @@ -29,32 +49,101 @@ export async function scanKnownPluginSkillCandidates( maximumCandidates = MAXIMUM_PLUGIN_SKILL_CANDIDATES ): Promise { const candidates: KnownPluginSkillCandidate[] = [] - const incompletePaths = new Set() + const issues: KnownPluginSkillScanIssue[] = [] + const issueKeys = new Set() const visited = new Set() + let resolvedRoot: string | null = null let entryCount = 0 let limitReached = false - function recordIncomplete(path: string): void { - if (incompletePaths.has(path)) { + // Why: an issue that explains a candidate is not optional. Dropping it for budget + // leaves the badge reacting to a placement the dialog can't account for, which is + // the split this change exists to remove — so those are charged past the bound, + // itself bounded by the candidate cap. + function recordIssue( + path: string, + reason: KnownPluginSkillScanIssue['reason'], + code: string | null = null, + explainsCandidate = false + ): void { + const key = `${path}\0${reason}\0${code ?? ''}` + if (issueKeys.has(key)) { return } - if (incompletePaths.size >= MAXIMUM_PLUGIN_INCOMPLETE_PATHS) { - // Why: each incomplete path expands to one conservative row per official - // skill. Collapse a hostile cache into one poison sentinel before IPC/render fanout. - incompletePaths.clear() - incompletePaths.add(rootPath) - limitReached = true + // Why: the bound that ended the walk is the one issue the dialog cannot do without + // — dropping it for display budget is what lets a truncated scan report all-clear. + const required = explainsCandidate || isTruncatingSkillScanReason(reason) + // Why: this budget bounds what the dialog lists, not how far the scan reaches. + // Ending the walk here would truncate coverage over a display limit — and since + // Orca's own bounds no longer raise attention, it would do so silently. + if (!required && issues.length >= MAXIMUM_PLUGIN_SCAN_ISSUES) { + if (!issues.some((issue) => issue.reason === 'issue-limit')) { + issues.push({ + path: rootPath, + reason: 'issue-limit', + errorCode: null + }) + } return } - incompletePaths.add(path) + issueKeys.add(key) + issues.push({ path, reason, errorCode: code }) } - async function visit(directory: string, depth: number): Promise { + function recordCandidate(name: string, path: string): void { + if (candidates.length >= maximumCandidates) { + limitReached = true + recordIssue(rootPath, 'candidate-limit') + return + } + candidates.push({ name, path }) + } + + // Why: a directory only proves it is a skill by carrying SKILL.md. Matching a known + // name alone would promote any same-named plugin or vendor folder into an installation + // Orca never verified. + async function hasSkillFile( + directory: string, + entries: readonly Dirent[], + resolvedDirectory: string + ): Promise { + const skillFile = entries.find((entry) => entry.name === SKILL_FILE_NAME) + if (!skillFile) { + return false + } + if (!skillFile.isSymbolicLink()) { + return skillFile.isFile() + } + try { + const skillFilePath = join(directory, skillFile.name) + const resolvedSkillFile = await realpath(skillFilePath) + if (!isWithinRoot(resolvedRoot ?? resolvedDirectory, resolvedSkillFile)) { + recordIssue(skillFilePath, 'outside-root') + return false + } + return (await stat(resolvedSkillFile)).isFile() + } catch (error) { + if (errorCode(error) !== 'ENOENT') { + recordIssue(join(directory, skillFile.name), 'io-error', errorCode(error)) + } + return false + } + } + + async function visit( + directory: string, + depth: number, + withinDeclaredSkillRoot = false, + payloadDepth: number | null = null + ): Promise { if (limitReached) { return } - if (depth > MAXIMUM_PLUGIN_SCAN_DEPTH) { - recordIncomplete(directory) + const maximumDepth = withinDeclaredSkillRoot + ? MAXIMUM_DECLARED_SKILL_SCAN_DEPTH + : MAXIMUM_PLUGIN_SCAN_DEPTH + if (depth > maximumDepth) { + recordIssue(directory, 'depth-limit') return } let resolved: string @@ -62,10 +151,16 @@ export async function scanKnownPluginSkillCandidates( resolved = await realpath(directory) } catch (error) { if (errorCode(error) !== 'ENOENT') { - recordIncomplete(directory) + recordIssue(directory, 'io-error', errorCode(error)) } return } + if (resolvedRoot === null) { + resolvedRoot = resolved + } else if (!isWithinRoot(resolvedRoot, resolved)) { + recordIssue(directory, 'outside-root') + return + } if (visited.has(resolved)) { return } @@ -73,9 +168,9 @@ export async function scanKnownPluginSkillCandidates( let handle: Awaited> try { - handle = await opendir(directory) - } catch { - recordIncomplete(directory) + handle = await opendir(resolved) + } catch (error) { + recordIssue(directory, 'io-error', errorCode(error)) return } const entries: Dirent[] = [] @@ -88,35 +183,83 @@ export async function scanKnownPluginSkillCandidates( entryCount += 1 if (entryCount > MAXIMUM_PLUGIN_SCAN_ENTRIES) { limitReached = true - recordIncomplete(rootPath) + recordIssue(rootPath, 'entry-limit') break } entries.push(entry) } - } catch { - recordIncomplete(directory) + } catch (error) { + recordIssue(directory, 'io-error', errorCode(error)) } finally { await handle.close().catch(() => undefined) } + const isSkillPackage = await hasSkillFile(directory, entries, resolved) + if (isSkillPackage) { + const name = basename(directory) + if (knownNames.has(name)) { + recordCandidate(name, directory) + } + } + + // Why: pruning payload is a topology decision, not a coverage failure, so it stays + // silent — recording it would put Orca's own traversal rules in the user's dialog. + const nextPayloadDepth = isSkillPackage ? 0 : payloadDepth === null ? null : payloadDepth + 1 + if (nextPayloadDepth !== null && nextPayloadDepth > MAXIMUM_NESTED_SKILL_DEPTH) { + return + } + + const skillRoots = await declaredPluginSkillRoots(directory, entries, resolvedRoot, recordIssue) + if (limitReached) { + return + } + if (skillRoots) { + const skillRootDepth = withinDeclaredSkillRoot ? depth + 1 : 0 + for (const skillRoot of skillRoots.sort()) { + entryCount += 1 + if (entryCount > MAXIMUM_PLUGIN_SCAN_ENTRIES) { + limitReached = true + recordIssue(rootPath, 'entry-limit') + return + } + await visit(skillRoot, skillRootDepth, true) + } + return + } + entries.sort((left, right) => (left.name === right.name ? 0 : left.name < right.name ? -1 : 1)) for (const entry of entries) { if (limitReached) { return } + if (entry.name === 'node_modules') { + continue + } const entryPath = join(directory, entry.name) let directoryEntry = entry.isDirectory() if (entry.isSymbolicLink()) { try { directoryEntry = (await stat(entryPath)).isDirectory() - } catch { - if (knownNames.has(entry.name)) { - if (candidates.length >= maximumCandidates) { - limitReached = true - recordIncomplete(rootPath) - return + if (directoryEntry) { + const resolvedEntry = await realpath(entryPath) + if (resolvedRoot !== null && !isWithinRoot(resolvedRoot, resolvedEntry)) { + recordIssue(entryPath, 'outside-root') + continue } - candidates.push({ name: entry.name, path: entryPath }) + } + } catch (error) { + const code = errorCode(error) + // Why: inside a declared skill root the plugin itself claims this name is a + // skill, so an uninspectable link stays fail-closed. Outside one there is no + // such claim and no SKILL.md to read, so inventing a copy would be a guess. + const claimedSkill = withinDeclaredSkillRoot && knownNames.has(entry.name) + // Why: a fail-closed candidate reads as inaccessible and raises attention, so + // the path has to be named even when it is merely absent. + if (claimedSkill) { + recordIssue(entryPath, 'io-error', code, true) + recordCandidate(entry.name, entryPath) + } else if (code !== 'ENOENT') { + recordIssue(entryPath, 'io-error', code) } continue } @@ -124,19 +267,10 @@ export async function scanKnownPluginSkillCandidates( if (!directoryEntry) { continue } - if (knownNames.has(entry.name)) { - if (candidates.length >= maximumCandidates) { - limitReached = true - recordIncomplete(rootPath) - return - } - candidates.push({ name: entry.name, path: entryPath }) - continue - } - await visit(entryPath, depth + 1) + await visit(entryPath, depth + 1, withinDeclaredSkillRoot, nextPayloadDepth) } } await visit(rootPath, 0) - return { candidates, incompletePaths: [...incompletePaths] } + return { candidates, issues } } diff --git a/src/main/skills/skill-plugin-manifest-roots.ts b/src/main/skills/skill-plugin-manifest-roots.ts new file mode 100644 index 000000000..ab2ef7264 --- /dev/null +++ b/src/main/skills/skill-plugin-manifest-roots.ts @@ -0,0 +1,145 @@ +import { constants, type Dirent } from 'node:fs' +import { open, realpath } from 'node:fs/promises' +import { isAbsolute, join, relative, sep } from 'node:path' +import type { SkillFreshnessScanIssueReason } from '../../shared/skill-freshness' + +const MAXIMUM_PLUGIN_MANIFEST_BYTES = 256 * 1024 +// Why: every declared root costs a resolve before it can be rejected, and those resolves +// bypass the dirent walk the entry budget bounds — so one manifest could otherwise spend +// the whole scan on paths that don't exist. No real plugin declares this many. +const MAXIMUM_DECLARED_SKILL_ROOTS = 64 +// Why: only formats whose skill layout is known. Treating an unverified manifest as a +// declaration prunes the rest of that plugin, so a wrong guess hides real skills; with +// no manifest the ordinary walk still finds them. +const PLUGIN_MANIFEST_DIRECTORIES = ['.codex-plugin', '.claude-plugin'] as const +const MANIFEST_OPEN_FLAGS = + constants.O_RDONLY | + (process.platform === 'win32' ? 0 : constants.O_NONBLOCK | constants.O_NOFOLLOW) + +function errorCode(error: unknown): string | null { + return error && typeof error === 'object' && 'code' in error && typeof error.code === 'string' + ? error.code + : null +} + +export function isWithinRoot(root: string, path: string): boolean { + const relativePath = relative(root, path) + return !isAbsolute(relativePath) && relativePath.split(sep)[0] !== '..' +} + +function resolveManifestSkillPath(directory: string, value: unknown): string | null { + if (typeof value !== 'string' || !value.startsWith('./')) { + return null + } + const relativePath = value.slice(2) + if (!relativePath || relativePath.split(/[\\/]/).includes('..')) { + return null + } + return join(directory, relativePath) +} + +export async function declaredPluginSkillRoots( + directory: string, + entries: readonly Dirent[], + resolvedRoot: string, + recordIssue: (path: string, reason: SkillFreshnessScanIssueReason, code?: string | null) => void +): Promise { + for (const manifestDirectory of PLUGIN_MANIFEST_DIRECTORIES) { + if (!entries.some((entry) => entry.name === manifestDirectory)) { + continue + } + const manifestPath = join(directory, manifestDirectory, 'plugin.json') + let resolvedManifestPath: string + try { + resolvedManifestPath = await realpath(manifestPath) + } catch (error) { + const code = errorCode(error) + if (code === 'ENOENT' || code === 'ENOTDIR') { + continue + } + recordIssue(manifestPath, 'io-error', code) + return null + } + if (!isWithinRoot(resolvedRoot, resolvedManifestPath)) { + recordIssue(manifestPath, 'outside-root') + return null + } + let manifestFile: Awaited> + try { + manifestFile = await open(resolvedManifestPath, MANIFEST_OPEN_FLAGS) + } catch (error) { + const code = errorCode(error) + if (code === 'ENOENT' || code === 'ENOTDIR') { + continue + } + recordIssue(manifestPath, 'io-error', code) + return null + } + try { + const manifestStat = await manifestFile.stat() + if (!manifestStat.isFile()) { + continue + } + if (manifestStat.size > MAXIMUM_PLUGIN_MANIFEST_BYTES) { + recordIssue(manifestPath, 'manifest-limit') + return null + } + const content = Buffer.alloc(MAXIMUM_PLUGIN_MANIFEST_BYTES + 1) + let contentLength = 0 + while (contentLength < content.length) { + const { bytesRead } = await manifestFile.read( + content, + contentLength, + content.length - contentLength, + contentLength + ) + if (bytesRead === 0) { + break + } + contentLength += bytesRead + } + if (contentLength > MAXIMUM_PLUGIN_MANIFEST_BYTES) { + recordIssue(manifestPath, 'manifest-limit') + return null + } + const parsed: unknown = JSON.parse(content.toString('utf8', 0, contentLength)) + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + return null + } + const skills = (parsed as Record).skills + if (skills === undefined) { + return [join(directory, 'skills')] + } + if (Array.isArray(skills) && skills.length === 0) { + return [] + } + const values = Array.isArray(skills) ? skills : [skills] + const roots = [ + ...new Set( + values + .map((value) => resolveManifestSkillPath(directory, value)) + .filter((value): value is string => value !== null) + ) + ].sort() + if (roots.length === 0) { + return null + } + // Why: fall back to the ordinary walk rather than a truncated root list. The walk + // is bounded by depth and entries, so it costs less than resolving the declared + // roots one by one and still reaches skills a truncation would have dropped. + if (roots.length > MAXIMUM_DECLARED_SKILL_ROOTS) { + recordIssue(manifestPath, 'manifest-limit') + return null + } + return roots + } catch (error) { + if (!(error instanceof SyntaxError)) { + recordIssue(manifestPath, 'io-error', errorCode(error)) + } + return null + } finally { + await manifestFile.close().catch(() => undefined) + } + } + return null +} diff --git a/src/renderer/src/components/codex-restart-chip.test.ts b/src/renderer/src/components/codex-restart-chip.test.ts index a23ed4d78..2dba0a66c 100644 --- a/src/renderer/src/components/codex-restart-chip.test.ts +++ b/src/renderer/src/components/codex-restart-chip.test.ts @@ -395,15 +395,13 @@ describe('CodexRestartChip focus target', () => { }, ptyIdsByTabId: { 'tab-1': ['pty-1'] } }) - useAppStore - .getState() - .markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - ]) + useAppStore.getState().markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com' + } + ]) return healthyPaneInput } diff --git a/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx b/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx index ca0090a6c..ea84e02c1 100644 --- a/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessNudge.test.tsx @@ -51,6 +51,7 @@ function eligibleInventory(): SkillFreshnessInventory { schemaVersion: 1, installations: [placement()], eligibleUpdateNames: ['orca-cli'], + scanIssues: [], scannedAt: 1 } } @@ -154,6 +155,7 @@ describe('SkillFreshnessNudge', () => { schemaVersion: 1, installations: [placement({ status: 'current', observedPackageDigest: 'current' })], eligibleUpdateNames: [], + scanIssues: [], scannedAt: 2 } await rerenderNudge() @@ -212,6 +214,7 @@ describe('SkillFreshnessNudge', () => { schemaVersion: 1, installations: [placement(), placement({ id: 'repo-copy', topology: 'repo-scope' })], eligibleUpdateNames: [], + scanIssues: [], scannedAt: 1 } diff --git a/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx b/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx index e3b1d1e01..8758a9dfc 100644 --- a/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessStatusPill.test.tsx @@ -55,6 +55,7 @@ function inventory( errorCategory: null })), eligibleUpdateNames, + scanIssues: [], scannedAt: 1 } } diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx index 875fc3974..0733df604 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.test.tsx @@ -118,6 +118,7 @@ function eligibleInventory(): SkillFreshnessInventory { schemaVersion: 1, installations: [placement('orca-cli')], eligibleUpdateNames: ['orca-cli'], + scanIssues: [], scannedAt: 1 } } @@ -263,6 +264,7 @@ describe('SkillFreshnessUpdateDialog', () => { schemaVersion: 1, installations: [placement('orca-cli'), placement('orchestration')], eligibleUpdateNames: ['orca-cli', 'orchestration'], + scanIssues: [], scannedAt: 1 } await renderDialog() @@ -304,6 +306,7 @@ describe('SkillFreshnessUpdateDialog', () => { schemaVersion: 1, installations: [placement('orca-cli', { status: 'current', installedReleaseRevision: 2 })], eligibleUpdateNames: [], + scanIssues: [], scannedAt: 5 } await emitRun({ state: 'success', names: ['orca-cli'], finishedAt: 2, output: 'done' }) @@ -325,6 +328,7 @@ describe('SkillFreshnessUpdateDialog', () => { }) ], eligibleUpdateNames: ['orca-cli'], + scanIssues: [], scannedAt: 1 } await renderDialog() @@ -383,6 +387,7 @@ describe('SkillFreshnessUpdateDialog', () => { schemaVersion: 1, installations: [placement('orca-cli', { status: 'current', installedReleaseRevision: 2 })], eligibleUpdateNames: [], + scanIssues: [], scannedAt: 2 } await renderDialog() @@ -397,6 +402,7 @@ describe('SkillFreshnessUpdateDialog', () => { schemaVersion: 1, installations: [placement('computer-use', { topology: 'repo-scope' })], eligibleUpdateNames: [], + scanIssues: [], scannedAt: 3 } await renderDialog() @@ -549,4 +555,104 @@ describe('SkillFreshnessUpdateDialog', () => { expect(container?.querySelector('[data-skill-row="orca-cli"]')).toBeNull() expect(findButton('Update 1 skill')).toBeUndefined() }) + it('shows incomplete plugin coverage without presenting a fabricated skill copy', async () => { + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement('orca-cli', { status: 'current', observedPackageDigest: 'current' }) + ], + eligibleUpdateNames: [], + scanIssues: [ + { + rootId: 'codex-plugin-cache', + sourceLabel: 'Codex plugin cache', + path: '/home/.codex/plugins/cache/vendor/locked', + reason: 'io-error', + errorCode: 'EACCES' + } + ], + scannedAt: 2 + } + await renderDialog() + await openViaRequest() + + expect(container?.textContent).toContain( + 'Orca could not finish checking plugin-managed skills.' + ) + expect(container?.textContent).toContain('/home/.codex/plugins/cache/vendor/locked') + expect(container?.textContent).toContain('EACCES') + expect(container?.textContent).not.toContain('All installed Orca skills are up to date.') + // Why: the fabricated per-skill path is exactly what this change removed — the + // unreadable folder must never be rendered as a copy of a named skill. + expect(container?.textContent).not.toContain( + '/home/.codex/plugins/cache/vendor/locked/orca-cli' + ) + }) + + // Why: the walk stopped early here, so claiming every copy is up to date would assert + // a completeness the scan did not reach — green dishonesty in place of amber. + it.each(['entry-limit', 'candidate-limit'] as const)( + 'does not report all-clear when %s ended the scan early', + async (reason) => { + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement('orca-cli', { status: 'current', observedPackageDigest: 'current' }) + ], + eligibleUpdateNames: [], + scanIssues: [ + { + rootId: 'codex-plugin-cache', + sourceLabel: 'Codex plugin cache', + path: '/home/.codex/plugins/cache', + reason: reason, + errorCode: null + } + ], + scannedAt: 2 + } + await renderDialog() + await openViaRequest() + + expect(container?.textContent).not.toContain('All installed Orca skills are up to date.') + expect(container?.textContent).toContain( + 'Orca could not finish checking plugin-managed skills.' + ) + // Why: the headline alone would pass with the folder list gone, leaving the user + // told the scan stopped but never told where. Assert the diagnostic renders too. + expect(container?.textContent).toContain('/home/.codex/plugins/cache') + } + ) + + // Why: Orca's own traversal bounds are not the user's to act on. Headlining them + // would put a permanent warning on any ordinary large plugin cache while every + // skill badge stayed green — the same unclearable amber, moved into the dialog. + it('lists a traversal bound without headlining it', async () => { + mocks.inventory = { + schemaVersion: 1, + installations: [ + placement('orca-cli', { status: 'current', observedPackageDigest: 'current' }) + ], + eligibleUpdateNames: [], + scanIssues: [ + { + rootId: 'codex-plugin-cache', + sourceLabel: 'Codex plugin cache', + path: '/home/.codex/plugins/cache/vendor/deep', + reason: 'depth-limit', + errorCode: null + } + ], + scannedAt: 2 + } + await renderDialog() + await openViaRequest() + + expect(container?.textContent).toContain('All installed Orca skills are up to date.') + expect(container?.textContent).not.toContain( + 'Orca could not finish checking plugin-managed skills.' + ) + expect(container?.textContent).toContain('/home/.codex/plugins/cache/vendor/deep') + expect(container?.textContent).toContain('scan depth limit') + }) }) diff --git a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx index e260e6123..8dd89233b 100644 --- a/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx +++ b/src/renderer/src/components/skills/SkillFreshnessUpdateDialog.tsx @@ -2,6 +2,8 @@ import { useMemo, useRef, useState, useSyncExternalStore } from 'react' import { AlertTriangle, CheckCircle2, ChevronDown, Copy, Loader2, RefreshCw } from 'lucide-react' import { buildTargetedSkillUpdateCommand, + isSkillScanIssueNeedingAttention, + isSkillScanIssueTruncatingScan, type SkillFreshnessInventory } from '../../../../shared/skill-freshness' import { useSkillFreshness } from '@/hooks/useSkillFreshness' @@ -18,6 +20,7 @@ import { import { Collapsible, CollapsibleContent, CollapsibleTrigger } from '@/components/ui/collapsible' import { TooltipProvider } from '@/components/ui/tooltip' import { groupSkillFreshness } from './skill-freshness-grouping' +import { SkillFreshnessScanIssues } from './skill-freshness-scan-issues' import { SkillUpdateRow } from './SkillUpdateRow' import { SummaryHeadline, summarizeInventory } from './skill-freshness-summary-headline' import { @@ -102,11 +105,20 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { ) const hasBlockedGroup = groups.some((group) => group.status === 'cannot-update') const blockedCount = groups.filter((group) => group.status === 'cannot-update').length + // Retained: the list keeps the last known folders on screen through a re-scan, the + // same way the rows above stay put rather than blanking. + const scanIssues = inventory?.scanIssues ?? [] // Why: the headline reads the LIVE snapshot, not the retained one — the two // disagree for the whole loading window, and pairing a retained "eligible" with // a live count of 0 renders "0 updates available" over rows badged "Update // available". Live means it says "Checking…" over the rows it kept on screen. - const summaryKind = summarizeInventory(state.inventory, hasBlockedGroup) + const summaryKind = summarizeInventory( + state.inventory, + hasBlockedGroup, + (state.inventory?.scanIssues ?? []).some( + (issue) => isSkillScanIssueNeedingAttention(issue) || isSkillScanIssueTruncatingScan(issue) + ) + ) // Why: one row list for every state. The rows are identical objects across the // transition, so pressing Update changes each row's leading icon in place @@ -300,6 +312,14 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element { ) : null} + {/* Why: folders, not skills — a plugin path Orca could not read says nothing + about which skill lives there, so it cannot be a row above. */} + {scanIssues.length > 0 ? ( +
+ +
+ ) : null} + {run.state === 'error' ? (

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 3a89ae1c2..d305353d9 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -116,5 +116,8 @@ describe('groupSkillFreshness', () => { expect(chipFor(at('g', { topology: 'repo-scope' }))).toBe('in-a-repo') expect(chipFor(at('h', { topology: 'plugin-cache' }))).toBe('plugin-cache') expect(chipFor(at('i', { status: 'current', topology: 'provider-alias' }))).toBe('current') + expect(chipFor(at('j', { status: 'unrecognized', topology: 'plugin-cache' }))).toBe( + 'plugin-cache' + ) }) }) diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.ts b/src/renderer/src/components/skills/skill-freshness-grouping.ts index 8d89c6ddd..ee2ebbbf3 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.ts @@ -26,8 +26,9 @@ export type SkillFreshnessGroupModel = { } export function locationChip(installation: SkillFreshnessInstallation): SkillLocationChip | null { - // Why: a location's own status wins over its topology — "the contents don't - // match" is more useful to the user than "it's a duplicate". + if (installation.status === 'unrecognized' && installation.topology === 'plugin-cache') { + return 'plugin-cache' + } if (installation.status === 'unrecognized') { return 'unrecognized' } diff --git a/src/renderer/src/components/skills/skill-freshness-scan-issues.tsx b/src/renderer/src/components/skills/skill-freshness-scan-issues.tsx new file mode 100644 index 000000000..d8979641a --- /dev/null +++ b/src/renderer/src/components/skills/skill-freshness-scan-issues.tsx @@ -0,0 +1,74 @@ +import type { SkillFreshnessScanIssue } from '../../../../shared/skill-freshness' +import { translate } from '@/i18n/i18n' + +function issueDescription(issue: SkillFreshnessScanIssue): string { + switch (issue.reason) { + case 'depth-limit': + return translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanDepthLimit', + 'Orca reached its plugin scan depth limit before checking this folder.' + ) + case 'entry-limit': + return translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanEntryLimit', + 'Orca reached its plugin scan entry limit before checking the rest of this cache.' + ) + case 'candidate-limit': + return translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanCandidateLimit', + 'Orca found more same-named skill folders than it can safely inspect.' + ) + case 'manifest-limit': + return translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanManifestLimit', + 'Orca skipped this plugin manifest because it exceeded a safe limit.' + ) + case 'outside-root': + return translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanOutsideRoot', + 'Orca skipped this plugin path because it points outside the plugin cache.' + ) + case 'io-error': + return issue.errorCode + ? translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanIoErrorWithCode', + 'Orca could not read this plugin path ({{value0}}).', + { value0: issue.errorCode } + ) + : translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanIoError', + 'Orca could not read this plugin path.' + ) + case 'issue-limit': + return translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanIssueLimit', + 'Orca found too many skipped plugin folders to list individually.' + ) + } +} + +export function SkillFreshnessScanIssues({ + issues +}: { + issues: readonly SkillFreshnessScanIssue[] +}): React.JSX.Element { + return ( + <> + {issues.map((issue) => ( +

+

{issue.sourceLabel}

+

{issueDescription(issue)}

+ + {issue.path} + +
+ ))} + + ) +} 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 2d693b691..05af44e1a 100644 --- a/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts +++ b/src/renderer/src/components/skills/skill-freshness-skipped-reason.ts @@ -73,8 +73,6 @@ export function skippedReason(locations: readonly SkillLocationRow[]): string { 'auto.components.skills.SkillFreshnessRow.skippedReasonDuplicate', 'This is a separate copy, so the update won’t reach it — the command only refreshes the main copy. Remove this copy, then reinstall the skill so this location follows the main one.' ) - // Why: 'current' is non-blocking and an empty priority list is possible; - // both fall through to the generic skipped message. case 'current': case undefined: return translate( diff --git a/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx b/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx index 0347302c6..fec3c7a09 100644 --- a/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx +++ b/src/renderer/src/components/skills/skill-freshness-summary-headline.tsx @@ -2,25 +2,42 @@ import { AlertTriangle, CheckCircle2, Loader2 } from 'lucide-react' import type { SkillFreshnessInventory } from '../../../../shared/skill-freshness' import { translate } from '@/i18n/i18n' -export type FreshnessSummaryKind = 'loading' | 'empty' | 'eligible' | 'current' | 'attention' +export type FreshnessSummaryKind = + | 'loading' + | 'empty' + | 'eligible' + | 'current' + | 'attention' + | 'scan-incomplete' export function summarizeInventory( inventory: SkillFreshnessInventory | null, - hasBlockedGroup: boolean + hasBlockedGroup: boolean, + hasIncompleteScan: boolean ): FreshnessSummaryKind { if (!inventory) { return 'loading' } - if (inventory.installations.length === 0) { - return 'empty' - } if (inventory.eligibleUpdateNames.length > 0) { return 'eligible' } - // Why: with nothing eligible, the modal is either genuinely all-clear or has - // out-of-date skills it can't safely update; the group filter already dropped - // the up-to-date and unrecognized-only noise, so a blocked group is the signal. - return hasBlockedGroup ? 'attention' : 'current' + // Why: a named skill the update can't converge outranks a coverage gap — the gap + // says something might be unchecked, the group says something definitely is wrong. + if (hasBlockedGroup) { + return 'attention' + } + // Why: a fault on the user's disk, or a bound that ended the walk early. Skipping a + // single folder is not either one — headlining that would put a permanent warning on + // any ordinary large plugin cache, the unclearable amber this change removes. + if (hasIncompleteScan) { + return 'scan-incomplete' + } + // Why: ordered after the scan checks so a scan that never completed is not reported + // as an empty machine — "none found" would be a claim the scan cannot support. + if (inventory.installations.length === 0) { + return 'empty' + } + return 'current' } /** Pre-run headline. Once a run starts, the dialog reports the run instead. */ @@ -78,6 +95,19 @@ export function SummaryHeadline({
) } + if (kind === 'scan-incomplete') { + // No follow-up sentence, matching 'attention': the skipped folders are listed + // directly under this headline, so pointing at a details panel would be stale. + return ( +
+ + {translate( + 'auto.components.skills.SkillFreshnessUpdateDialog.scanIncomplete', + 'Orca could not finish checking plugin-managed skills.' + )} +
+ ) + } return (

diff --git a/src/renderer/src/hooks/useSkillFreshness.test.tsx b/src/renderer/src/hooks/useSkillFreshness.test.tsx index 812ad72ff..09c2d6863 100644 --- a/src/renderer/src/hooks/useSkillFreshness.test.tsx +++ b/src/renderer/src/hooks/useSkillFreshness.test.tsx @@ -25,7 +25,7 @@ function deferred(): { } function inventory(scannedAt: number, eligibleUpdateNames: string[] = []): SkillFreshnessInventory { - return { schemaVersion: 1, installations: [], eligibleUpdateNames, scannedAt } + return { schemaVersion: 1, installations: [], eligibleUpdateNames, scanIssues: [], scannedAt } } let root: Root | null = null diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 405cd3e19..8af57bc01 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3852,7 +3852,16 @@ "close": "Close", "stop": "Stop", "stopping": "Stopping…", - "stoppingHeadline": "Stopping the update…" + "stoppingHeadline": "Stopping the update…", + "scanIncomplete": "Orca could not finish checking plugin-managed skills.", + "scanDepthLimit": "Orca reached its plugin scan depth limit before checking this folder.", + "scanEntryLimit": "Orca reached its plugin scan entry limit before checking the rest of this cache.", + "scanCandidateLimit": "Orca found more same-named skill folders than it can safely inspect.", + "scanManifestLimit": "Orca skipped this plugin manifest because it exceeded a safe limit.", + "scanOutsideRoot": "Orca skipped this plugin path because it points outside the plugin cache.", + "scanIoErrorWithCode": "Orca could not read this plugin path ({{value0}}).", + "scanIoError": "Orca could not read this plugin path.", + "scanIssueLimit": "Orca found too many skipped plugin folders to list individually." }, "SkillUpdateResultRows": { "stillOutdated": "Still out of date after the update ran." diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index a80e4da10..d60f8704e 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -3829,7 +3829,16 @@ "done": "Done", "stop": "Stop", "stopping": "Stopping…", - "stoppingHeadline": "Stopping the update…" + "stoppingHeadline": "Stopping the update…", + "scanIncomplete": "Orca no pudo terminar de comprobar los skills gestionados por plugins.", + "scanDepthLimit": "Orca alcanzó el límite de profundidad del análisis antes de comprobar esta carpeta.", + "scanEntryLimit": "Orca alcanzó el límite de entradas del análisis antes de comprobar el resto de esta caché.", + "scanCandidateLimit": "Orca encontró más carpetas de skills con el mismo nombre de las que puede inspeccionar de forma segura.", + "scanManifestLimit": "Orca omitió este manifiesto del plugin porque superaba un límite seguro.", + "scanOutsideRoot": "Orca omitió esta ruta del plugin porque apunta fuera de la caché de plugins.", + "scanIoErrorWithCode": "Orca no pudo leer esta ruta del plugin ({{value0}}).", + "scanIoError": "Orca no pudo leer esta ruta del plugin.", + "scanIssueLimit": "Orca encontró demasiadas carpetas de plugins omitidas para mostrarlas por separado." }, "SkillFreshnessStatusPill": { "updateAvailable": "Actualización disponible", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 7ed9b6dc1..e625872db 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -3829,7 +3829,16 @@ "done": "Done", "stop": "Stop", "stopping": "Stopping…", - "stoppingHeadline": "Stopping the update…" + "stoppingHeadline": "Stopping the update…", + "scanIncomplete": "Orca はプラグインで管理されるスキルの確認を完了できませんでした。", + "scanDepthLimit": "このフォルダーを確認する前に、プラグインスキャンの深度上限に達しました。", + "scanEntryLimit": "キャッシュの残りを確認する前に、プラグインスキャンのエントリ数上限に達しました。", + "scanCandidateLimit": "安全に確認できる数を超える同名のスキルフォルダーが見つかりました。", + "scanManifestLimit": "安全な上限を超えたため、このプラグインマニフェストをスキップしました。", + "scanOutsideRoot": "プラグインキャッシュの外部を指しているため、このプラグインパスをスキップしました。", + "scanIoErrorWithCode": "このプラグインパスを読み取れませんでした（{{value0}}）。", + "scanIoError": "このプラグインパスを読み取れませんでした。", + "scanIssueLimit": "スキップされたプラグインフォルダーが多すぎるため、個別に表示できません。" }, "SkillFreshnessStatusPill": { "updateAvailable": "更新があります", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 52dcc3178..184d1aceb 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -3829,7 +3829,16 @@ "done": "Done", "stop": "Stop", "stopping": "Stopping…", - "stoppingHeadline": "Stopping the update…" + "stoppingHeadline": "Stopping the update…", + "scanIncomplete": "Orca가 플러그인으로 관리되는 스킬 확인을 완료하지 못했습니다.", + "scanDepthLimit": "이 폴더를 확인하기 전에 플러그인 검사 깊이 제한에 도달했습니다.", + "scanEntryLimit": "이 캐시의 나머지 항목을 확인하기 전에 플러그인 검사 항목 수 제한에 도달했습니다.", + "scanCandidateLimit": "안전하게 검사할 수 있는 수보다 많은 동명 스킬 폴더가 발견되었습니다.", + "scanManifestLimit": "안전한 제한을 초과한 플러그인 매니페스트를 건너뛰었습니다.", + "scanOutsideRoot": "플러그인 캐시 외부를 가리키는 플러그인 경로를 건너뛰었습니다.", + "scanIoErrorWithCode": "이 플러그인 경로를 읽지 못했습니다({{value0}}).", + "scanIoError": "이 플러그인 경로를 읽지 못했습니다.", + "scanIssueLimit": "건너뛴 플러그인 폴더가 너무 많아 개별적으로 표시할 수 없습니다." }, "SkillFreshnessStatusPill": { "updateAvailable": "업데이트 가능", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index b9efca149..7f9a1ddfe 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -3829,7 +3829,16 @@ "done": "Done", "stop": "Stop", "stopping": "Stopping…", - "stoppingHeadline": "Stopping the update…" + "stoppingHeadline": "Stopping the update…", + "scanIncomplete": "Orca 无法完成对插件管理技能的检查。", + "scanDepthLimit": "在检查此文件夹之前，Orca 已达到插件扫描深度上限。", + "scanEntryLimit": "在检查此缓存的其余内容之前，Orca 已达到插件扫描条目数上限。", + "scanCandidateLimit": "Orca 发现的同名技能文件夹数量超过了可安全检查的上限。", + "scanManifestLimit": "此插件清单超出安全限制，Orca 已将其跳过。", + "scanOutsideRoot": "此插件路径指向插件缓存之外，Orca 已将其跳过。", + "scanIoErrorWithCode": "Orca 无法读取此插件路径（{{value0}}）。", + "scanIoError": "Orca 无法读取此插件路径。", + "scanIssueLimit": "被跳过的插件文件夹过多，Orca 无法逐一列出。" }, "SkillFreshnessStatusPill": { "updateAvailable": "有可用更新", diff --git a/src/renderer/src/lib/agent-skill-nav-install-status.test.ts b/src/renderer/src/lib/agent-skill-nav-install-status.test.ts index 1960d2b18..70e0030de 100644 --- a/src/renderer/src/lib/agent-skill-nav-install-status.test.ts +++ b/src/renderer/src/lib/agent-skill-nav-install-status.test.ts @@ -57,7 +57,7 @@ function inventory( installations: SkillFreshnessInstallation[], eligibleUpdateNames: string[] = [] ): SkillFreshnessInventory { - return { schemaVersion: 1, installations, eligibleUpdateNames, scannedAt: 1 } + return { schemaVersion: 1, installations, eligibleUpdateNames, scanIssues: [], scannedAt: 1 } } describe('getAgentSkillNavInstallStatus', () => { 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 15b25aa4a..b619e59e8 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.test.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.test.ts @@ -2,12 +2,28 @@ import { describe, expect, it } from 'vitest' import type { SkillFreshnessInstallation, SkillFreshnessInventory, + SkillFreshnessScanIssueReason, SkillFreshnessStatus } from '../../../shared/skill-freshness' -import { getSkillFreshnessDisplayStatus } from './skill-freshness-display-status' +import { + getSkillFreshnessDisplayStatus, + hasSkillCopyNeedingAttention +} from './skill-freshness-display-status' const SKILL_NAME = 'orca-cli' +function scanIssue( + reason: SkillFreshnessScanIssueReason +): SkillFreshnessInventory['scanIssues'][number] { + return { + rootId: 'codex-plugin-cache', + sourceLabel: 'Codex plugin cache', + path: '/home/.codex/plugins/cache', + reason, + errorCode: reason === 'io-error' ? 'EACCES' : null + } +} + function placement(status: SkillFreshnessStatus, index = 0): SkillFreshnessInstallation { return { id: `${SKILL_NAME}-${index}`, @@ -31,11 +47,22 @@ function placement(status: SkillFreshnessStatus, index = 0): SkillFreshnessInsta } } +function pluginCachePlacement( + status: SkillFreshnessStatus = 'unrecognized' +): SkillFreshnessInstallation { + return { + ...placement(status, 9), + topology: 'plugin-cache', + unresolvedPath: `/home/.codex/plugins/cache/openai-bundled/${SKILL_NAME}` + } +} + function inventory( installations: SkillFreshnessInstallation[], - eligibleUpdateNames: string[] = [] + eligibleUpdateNames: string[] = [], + scanIssues: SkillFreshnessInventory['scanIssues'] = [] ): SkillFreshnessInventory { - return { schemaVersion: 1, installations, eligibleUpdateNames, scannedAt: 1 } + return { schemaVersion: 1, installations, eligibleUpdateNames, scanIssues, scannedAt: 1 } } describe('getSkillFreshnessDisplayStatus', () => { @@ -75,4 +102,93 @@ describe('getSkillFreshnessDisplayStatus', () => { // all-clear over drift the update command cannot reach and the user cannot see. expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('needs-attention') }) + + it.each([ + ['depth-limit'], + ['entry-limit'], + ['candidate-limit'], + ['manifest-limit'], + ['issue-limit'], + // Why: a vendor linking its skills out of the cache is a packaging choice, not a + // fault the user can clear — deleting the link only makes the package manager + // recreate it. Amber here is clean-on-main turned permanently amber. + ['outside-root'] + ] as const)('does not report attention for the %s traversal bound', (reason) => { + // Why: these are Orca's own bounds. A large but healthy plugin cache would + // otherwise pin every skill amber with nothing the user could do about it. + expect( + getSkillFreshnessDisplayStatus( + inventory([placement('current')], [], [scanIssue(reason)]), + SKILL_NAME + ) + ).toBe('up-to-date') + }) + + // Why: the only reason left that is a fact about the user's own disk rather than a + // bound Orca chose, so the only one a person can actually clear. + it('reports needs attention for the io-error scan fault', () => { + expect( + getSkillFreshnessDisplayStatus( + inventory([placement('current')], [], [scanIssue('io-error')]), + SKILL_NAME + ) + ).toBe('needs-attention') + }) + + it('stays up to date beside another ecosystem’s same-name skill', () => { + // Why: the copy belongs to another tool, so no user action exists to clear it. + // Amber here is the badge-with-no-exit reported in #10633. + expect( + getSkillFreshnessDisplayStatus( + inventory([placement('current'), pluginCachePlacement()]), + SKILL_NAME + ) + ).toBe('up-to-date') + expect(hasSkillCopyNeedingAttention(inventory([pluginCachePlacement()]), SKILL_NAME)).toBe( + false + ) + }) + + it('still reports drift in our own copy when a plugin-managed one sits alongside', () => { + expect( + getSkillFreshnessDisplayStatus( + inventory([placement('unrecognized'), pluginCachePlacement()]), + SKILL_NAME + ) + ).toBe('needs-attention') + }) + + it('reports attention when a plugin-cache copy is inaccessible', () => { + const value = inventory([pluginCachePlacement('inaccessible')]) + + expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('needs-attention') + expect(hasSkillCopyNeedingAttention(value, SKILL_NAME)).toBe(true) + }) +}) + +describe('hasSkillCopyNeedingAttention', () => { + it('ignores a traversal bound but keeps a real scan fault', () => { + expect( + hasSkillCopyNeedingAttention( + inventory([placement('current')], [], [scanIssue('entry-limit')]), + SKILL_NAME + ) + ).toBe(false) + expect( + hasSkillCopyNeedingAttention( + inventory([placement('current')], [], [scanIssue('io-error')]), + SKILL_NAME + ) + ).toBe(true) + }) + + // 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. + it('does not blame a skill with no placement for a fault elsewhere in the cache', () => { + const value = inventory([], [], [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 581c281a8..0da1e08cb 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.ts @@ -1,4 +1,5 @@ import { + isSkillScanIssueNeedingAttention, SUPPORTED_GLOBAL_SKILL_TOPOLOGIES, type SkillFreshnessInventory } from '../../../shared/skill-freshness' @@ -16,7 +17,6 @@ export function getSkillFreshnessDisplayStatus( if (inventory?.eligibleUpdateNames.includes(skillName)) { return 'update-available' } - let hasPlacement = false let hasBlockedCopy = false for (const installation of inventory?.installations ?? []) { @@ -24,7 +24,10 @@ export function getSkillFreshnessDisplayStatus( continue } hasPlacement = true - if (installation.status !== 'current') { + if ( + installation.status !== 'current' && + !(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') + ) { hasBlockedCopy = true } } @@ -33,6 +36,12 @@ export function getSkillFreshnessDisplayStatus( if (!hasPlacement) { return 'installed' } + // Why: an unreadable plugin path could hide a copy of any known skill, so it stays + // fail-closed — but only for skills Orca actually found somewhere. Flagging a skill + // that isn't installed at all blames it for a fault in someone else's plugin. + if (inventory?.scanIssues.some(isSkillScanIssueNeedingAttention)) { + return 'needs-attention' + } // Why: no eligible update is not proof a copy is fine — it can equally mean a copy // is out of date somewhere the update command cannot reach. Saying "Installed" there // reads as all-clear and hides real drift, so that case gets its own attention state. @@ -48,14 +57,21 @@ export function hasSkillCopyNeedingAttention( inventory: SkillFreshnessInventory | null, skillName: string ): boolean { - return (inventory?.installations ?? []).some( - (installation) => - installation.name === skillName && - installation.status !== 'current' && - // Why: an out-of-date copy the command converges is ordinary work, not a problem. - !( - SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) && - installation.status === 'outdated' - ) + const placements = (inventory?.installations ?? []).filter( + (installation) => installation.name === skillName + ) + return ( + (placements.length > 0 && + Boolean(inventory?.scanIssues.some(isSkillScanIssueNeedingAttention))) || + placements.some( + (installation) => + installation.status !== 'current' && + !(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') && + // Why: an out-of-date copy the command converges is ordinary work, not a problem. + !( + SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) && + installation.status === 'outdated' + ) + ) ) } diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index e20656f94..adf36b191 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -2807,6 +2807,7 @@ function createSkillsApi(): NonNullable['skills']> { schemaVersion: 1, installations: [], eligibleUpdateNames: [], + scanIssues: [], scannedAt: Date.now() }), // Why: with no local skill homes there is nothing to update, so the run rail diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 0de5570b4..99c360a3e 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -84,10 +84,59 @@ export type SkillFreshnessInstallation = { errorCategory: string | null } +export type SkillFreshnessScanIssueReason = + | 'depth-limit' + | 'entry-limit' + | 'candidate-limit' + | 'manifest-limit' + | 'outside-root' + | 'io-error' + | 'issue-limit' + +export type SkillFreshnessScanIssue = { + rootId: string + sourceLabel: string + path: string + reason: SkillFreshnessScanIssueReason + errorCode: string | null +} + +// Why: a real read failure is a fact about the user's disk and stays actionable. Orca's +// own traversal bounds are not — reporting them as attention turns an ordinary large +// plugin cache into a permanent amber pill on every skill. +// +// 'outside-root' is deliberately NOT here. A plugin that links its skills out of the +// cache (a content-addressed store, a dev-linked package) is a packaging choice by the +// vendor, not a fault the user can clear: deleting the link only makes their package +// manager recreate it. Flagging it turned an install that is clean on main into amber on +// every installed skill. It is still listed in Details, like the other bounds. +const SKILL_SCAN_ATTENTION_REASONS = new Set(['io-error']) + +export function isSkillScanIssueNeedingAttention(issue: SkillFreshnessScanIssue): boolean { + return SKILL_SCAN_ATTENTION_REASONS.has(issue.reason) +} + +// Why: these are the bounds that end the walk rather than skip one folder. They are +// still not the user's to act on, so they raise no pill — but a scan that stopped +// early cannot be reported as proof every copy is up to date. +const SKILL_SCAN_TRUNCATING_REASONS = new Set([ + 'entry-limit', + 'candidate-limit' +]) + +export function isTruncatingSkillScanReason(reason: SkillFreshnessScanIssueReason): boolean { + return SKILL_SCAN_TRUNCATING_REASONS.has(reason) +} + +export function isSkillScanIssueTruncatingScan(issue: SkillFreshnessScanIssue): boolean { + return isTruncatingSkillScanReason(issue.reason) +} + export type SkillFreshnessInventory = { schemaVersion: 1 installations: SkillFreshnessInstallation[] eligibleUpdateNames: string[] + scanIssues: SkillFreshnessScanIssue[] scannedAt: number }