diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index 415e052ad..3aeca4bdd 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -15,6 +15,7 @@ import { MAXIMUM_REPOSITORY_SKILL_ROOTS } from './skill-freshness-inventory' import { describeObservedSkillFile, skillPackageDigest } from './skill-package-identity' +import { MAXIMUM_PLUGIN_SCAN_ENTRIES } from './skill-plugin-cache-scan' import { getSkillFreshnessDisplayStatus } from '../../renderer/src/lib/skill-freshness-display-status' const temporaryDirectories: string[] = [] @@ -706,4 +707,42 @@ describe('read-only skill freshness inventory', () => { }) ]) }) + + it('invents no installations when the plugin cache trips the entry budget (#10918)', 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(pluginCache, { recursive: true }) + // Why: the production bound, not an injected one — #10918 is the real constant + // collapsing the scan to the cache root, and only a real cache proves that path. + const entries = Array.from({ length: MAXIMUM_PLUGIN_SCAN_ENTRIES + 1 }, (_, index) => + join(pluginCache, `entry-${index}`) + ) + for (let index = 0; index < entries.length; index += 512) { + await Promise.all(entries.slice(index, index + 512).map((path) => writeFile(path, ''))) + } + + const inventory = await inventorySkillFreshness({ + currentAppVersion: '2.0.0', + homeDir: test.homeDir, + repos: [], + resourceRoot: test.resourceRoot + }) + + // Why: assert the bound actually tripped first — if the fixture stopped reaching it, + // the placement assertion below would still pass and cover nothing. + expect(inventory.scanIssues).toEqual([ + expect.objectContaining({ + rootId: 'codex-plugin-cache', + path: pluginCache, + reason: 'entry-limit', + errorCode: null + }) + ]) + // Why: the truncated root is not evidence of a copy. Fabricating one per manifest name + // is what pinned an unclearable "Needs attention" on every card in #10918. + expect(inventory.installations).toEqual([ + expect.objectContaining({ name: 'orca-cli', status: 'current', topology: 'canonical-copy' }) + ]) + }) }) diff --git a/src/main/skills/skill-plugin-cache-scan.test.ts b/src/main/skills/skill-plugin-cache-scan.test.ts index 17f1c8e1e..ed01d49cc 100644 --- a/src/main/skills/skill-plugin-cache-scan.test.ts +++ b/src/main/skills/skill-plugin-cache-scan.test.ts @@ -7,6 +7,7 @@ import { afterEach, describe, expect, it } from 'vitest' import { isSkillScanIssueNeedingAttention } from '../../shared/skill-freshness' import { MAXIMUM_PLUGIN_SCAN_ATTENTION_ISSUES, + MAXIMUM_PLUGIN_SCAN_DEPTH, MAXIMUM_PLUGIN_SCAN_ISSUES, scanKnownPluginSkillCandidates } from './skill-plugin-cache-scan' @@ -29,12 +30,122 @@ describe('plugin skill candidate scan', () => { }) ) - const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), 1) + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), { + maximumCandidates: 1 + }) expect(result.candidates).toHaveLength(1) expect(result.issues).toEqual([{ path: root, reason: 'candidate-limit', errorCode: null }]) }) + it('stops at the entry budget and reports the truncation at the scan root', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-entry-limit-')) + temporaryDirectories.push(root) + await Promise.all( + ['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') + }) + ) + + // Budget: the root's two vendor dirents, one's, and its skill's — so the count is + // crossed inside 'two', not at the root. That is what pins the issue to the scan + // root the dialog can name rather than whichever directory happened to cross it. + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), { + maximumEntries: 4 + }) + + // Why: the second vendor's skill goes unseen. The issue is all that stops the dialog + // reporting all-clear over a scan that never reached it. + expect(result.candidates).toEqual([{ name: 'orca-cli', path: join(root, 'one', 'orca-cli') }]) + expect(result.issues).toEqual([{ path: root, reason: 'entry-limit', errorCode: null }]) + }) + + it('stops walking the directory whose read crossed the entry budget', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-entry-limit-stop-')) + temporaryDirectories.push(root) + // Why: read off the depth bound rather than hardcoded. The deepest directory has to + // sit exactly at it, so its children are the first thing a walk that failed to stop + // would reject on depth — one level shallower and the mutant walks them silently. + const segments = Array.from( + { length: MAXIMUM_PLUGIN_SCAN_DEPTH }, + (_, index) => `level-${index}` + ) + await Promise.all( + ['a', 'b'].map((name) => mkdir(join(root, ...segments, name), { recursive: true })) + ) + + // Budget: one dirent per level down to the deepest directory, plus the first of its + // two children — so the count is crossed on the second, with the first already read. + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), { + maximumEntries: segments.length + 1 + }) + + // Why: the entries already read before the bound must not still be descended. A scan + // that keeps walking reports the leftovers as depth-truncated, which is a coverage + // failure the walk never observed — exactly the kind of unaccountable claim #10918 was. + expect(result.candidates).toEqual([]) + expect(result.issues).toEqual([{ path: root, reason: 'entry-limit', errorCode: null }]) + }) + + // Why: a declared root costs a resolve before it can be rejected, and a root that does + // not exist reads no dirent at all — so the dirent guard never sees a manifest that + // spends the whole scan on missing paths. Only the resolve guard bounds that, and only + // this shape lets its threshold be read off the entry count instead of assumed. + async function createManifestWithMissingSkillRoots(): Promise<{ + root: string + candidate: string + }> { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-entry-limit-declared-')) + temporaryDirectories.push(root) + // Why: the manifest sits under a vendor directory, not at the scan root, so the + // directory whose declared roots cross the budget is not the root the issue names. + const packageRoot = join(root, 'vendor') + const candidate = join(packageRoot, 'a-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":["./a-skills","./missing-one","./missing-two"]}\n' + ) + await writeFile(join(candidate, 'SKILL.md'), '# Orca CLI\n') + return { root, candidate } + } + + // Why: the scan reads exactly eight entries here — the root's dirent, the vendor's two, + // a-skills' and its skill's, then one resolve per declared root. Both budgets below are + // stated against that count so each guard's threshold is asserted, not just its firing. + const DECLARED_ROOT_SCAN_ENTRIES = 8 + + it('stops at the entry budget while resolving declared skill roots', async () => { + const { root, candidate } = await createManifestWithMissingSkillRoots() + + // Why: one short of the full count, so only the last declared root's resolve crosses + // it. A guard that admitted that root would run the scan to completion and report + // nothing, which is what makes the issue below an assertion on the threshold. + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), { + maximumEntries: DECLARED_ROOT_SCAN_ENTRIES - 1 + }) + + // Why: the declared root that exists was fully walked, so the bound is being crossed by + // a resolve of the roots after it — not by the dirent loop stopping the scan early. + expect(result.candidates).toEqual([{ name: 'orca-cli', path: candidate }]) + expect(result.issues).toEqual([{ path: root, reason: 'entry-limit', errorCode: null }]) + }) + + it('resolves the last declared skill root when the entry budget is exactly spent', async () => { + const { root, candidate } = await createManifestWithMissingSkillRoots() + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), { + maximumEntries: DECLARED_ROOT_SCAN_ENTRIES + }) + + // Why: a budget the scan fits inside is not a truncation. Firing one entry early would + // pin the same unclearable attention #10918 did, on a scan that missed nothing. + expect(result.candidates).toEqual([{ name: 'orca-cli', path: candidate }]) + expect(result.issues).toEqual([]) + }) + 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). @@ -554,7 +665,9 @@ describe('plugin skill candidate scan', () => { }) ) - const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), 1) + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']), { + maximumCandidates: 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 diff --git a/src/main/skills/skill-plugin-cache-scan.ts b/src/main/skills/skill-plugin-cache-scan.ts index 7f151e847..5cc93d473 100644 --- a/src/main/skills/skill-plugin-cache-scan.ts +++ b/src/main/skills/skill-plugin-cache-scan.ts @@ -8,7 +8,7 @@ import { } from '../../shared/skill-freshness' import { declaredPluginSkillRoots, isWithinRoot } from './skill-plugin-manifest-roots' -const MAXIMUM_PLUGIN_SCAN_DEPTH = 9 +export const MAXIMUM_PLUGIN_SCAN_DEPTH = 9 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 @@ -17,7 +17,7 @@ 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_SCAN_ENTRIES = 16_384 export const MAXIMUM_PLUGIN_SKILL_CANDIDATES = 64 export const MAXIMUM_PLUGIN_SCAN_ISSUES = 16 // Why: an attention issue outranks the display budget, so nothing else bounds how many a @@ -49,11 +49,21 @@ function errorCode(error: unknown): string | null { : null } +// Why: overriding a bound is how its truncation path stays executable — reaching the real +// entry budget costs a 16k-dirent fixture per case, and the declared-root guard below it +// needs the running count parked just under that budget. Production passes neither. +export type PluginSkillScanBounds = { + maximumCandidates?: number + maximumEntries?: number +} + export async function scanKnownPluginSkillCandidates( rootPath: string, knownNames: ReadonlySet, - maximumCandidates = MAXIMUM_PLUGIN_SKILL_CANDIDATES + bounds: PluginSkillScanBounds = {} ): Promise { + const maximumCandidates = bounds.maximumCandidates ?? MAXIMUM_PLUGIN_SKILL_CANDIDATES + const maximumEntries = bounds.maximumEntries ?? MAXIMUM_PLUGIN_SCAN_ENTRIES const candidates: KnownPluginSkillCandidate[] = [] const issues: KnownPluginSkillScanIssue[] = [] const issueKeys = new Set() @@ -197,7 +207,7 @@ export async function scanKnownPluginSkillCandidates( break } entryCount += 1 - if (entryCount > MAXIMUM_PLUGIN_SCAN_ENTRIES) { + if (entryCount > maximumEntries) { limitReached = true recordIssue(rootPath, 'entry-limit') break @@ -233,7 +243,7 @@ export async function scanKnownPluginSkillCandidates( const skillRootDepth = withinDeclaredSkillRoot ? depth + 1 : 0 for (const skillRoot of skillRoots.sort()) { entryCount += 1 - if (entryCount > MAXIMUM_PLUGIN_SCAN_ENTRIES) { + if (entryCount > maximumEntries) { limitReached = true recordIssue(rootPath, 'entry-limit') return