test(skills): execute the plugin-cache entry limit instead of reasoning about it (#11255)
* test(skills): execute the plugin-cache entry limit instead of reasoning about it The entry bound that ended a plugin-cache scan was never reached by any test. Every existing `entry-limit` case builds a synthetic issue object at the display layer, so both emission sites in scanKnownPluginSkillCandidates — the dirent read loop and the declared-skill-root resolve — were verified by reading the code. The real bound is 16,384 dirents, and the declared-root guard only fires when the count lands in a one-entry window below it, which is why neither was ever fixtured. Makes the entry bound injectable the way the candidate bound already was, folding both into a `PluginSkillScanBounds` bag so a fourth positional number is not needed, and adds: - a case that truncates the dirent read and asserts the scan drops both real skills and reports `entry-limit` at the root; - a case that truncates while resolving declared roots, asserting the declared root that exists was still walked so the guard under test is the resolve one; - an inventorySkillFreshness case at the production 16,384 bound asserting a truncated scan produces zero fabricated placements — the #10918 regression. No bound value or scan behavior changes. Refs #10918. * docs(skills): state the declared-root guard's real window in the bounds comment * test(skills): pin entry-limit to the scan root, not the crossing directory Both entry-budget fixtures crossed the bound in the same directory they named, so swapping recordIssue(rootPath) for recordIssue(directory) at either guard passed the whole suite — the dialog would surface a nested path and no test would say so. Cross the budget below the root instead. * test(skills): assert the declared-root guard's threshold, not just its firing The declared-root entry guard's ±1 boundary was left unasserted on the claim that it is unkillable: admitting one more declared root was said to always cost a dirent the read-loop guard then catches identically. It does not. A declared root that does not exist reads no dirent, and when it is the last one the loop simply ends — so the scan completes and reports nothing. Restates both budgets against the fixture's exact entry count: one short of it, where only the last root's resolve can cross, and exactly at it, where nothing should be reported. `>` -> `>=` and `>` -> `> max + 1` now each fail a test. * test(skills): assert the entry bound stops the walk, not just what it reports Dropping `limitReached = true` at the dirent guard survived every case: the already-read entries of the crossing directory are still descended, and the scan reports a depth-limit for a path it never reached. A nine-level chain whose deepest directory sits at the depth bound and crosses the budget on the second of its two children kills that, with no symlink and no Windows skip. * test(skills): derive the walk-stop fixture from the depth bound it depends on The walk-stop case detects a dropped `limitReached` only because its chain is exactly MAXIMUM_PLUGIN_SCAN_DEPTH deep, so the entries already read when the entry bound trips are rejected on depth if the walk keeps going. That coupling was a hardcoded 9. Raising the depth bound left the test passing while it stopped killing the mutant it is the only cover for — verified by A/B: at depth 12 the hardcoded fixture lets the mutant survive, the derived one does not. Other tests in the file fail loudly on that change, which is exactly what makes the silent one dangerous. Exports the bound the way the file already exports its four siblings for the same reason. No behavior change.
This commit is contained in:
Brennan Benson
GitHub
parent
5c8013abaa
commit
493f403ef4
|
|
@ -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' })
|
||||
])
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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: <vendor>/<plugin>/<version>/.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
|
||||
|
|
|
|||
|
|
@ -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<string>,
|
||||
maximumCandidates = MAXIMUM_PLUGIN_SKILL_CANDIDATES
|
||||
bounds: PluginSkillScanBounds = {}
|
||||
): Promise<KnownPluginSkillScan> {
|
||||
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<string>()
|
||||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue