From 930ff96152a12de127d7c633d5ab8baae4f970f7 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:41:27 -0700 Subject: [PATCH] fix(skills): stop the scan issue budget evicting a read failure (#11221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-scan issue budget kept an issue only when it explained a candidate or truncated the walk. Neither set intersects the attention set, so 'io-error' — the sole reason a plugin-cache scan can raise "Needs attention" — was droppable. Once 16 ordinary issues filled the budget (16 'outside-root' vendor symlinks is an install shape the scan itself documents as normal), a later read failure was evicted for a generic 'issue-limit' row that raises neither attention nor truncation, and the dialog headline read "All installed Orca skills are up to date" over a path that could be hiding a stale copy. Attention issues now outrank the budget, capped at a small reserve so an adversarial tree of unreadable folders cannot pin one issue per folder. --- .../skills/skill-plugin-cache-scan.test.ts | 69 +++++++++++++++++++ src/main/skills/skill-plugin-cache-scan.ts | 18 ++++- src/shared/skill-freshness.ts | 6 +- 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/main/skills/skill-plugin-cache-scan.test.ts b/src/main/skills/skill-plugin-cache-scan.test.ts index 442af48ce96..17f1c8e1eb9 100644 --- a/src/main/skills/skill-plugin-cache-scan.test.ts +++ b/src/main/skills/skill-plugin-cache-scan.test.ts @@ -4,7 +4,9 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' import { afterEach, describe, expect, it } from 'vitest' +import { isSkillScanIssueNeedingAttention } from '../../shared/skill-freshness' import { + MAXIMUM_PLUGIN_SCAN_ATTENTION_ISSUES, MAXIMUM_PLUGIN_SCAN_ISSUES, scanKnownPluginSkillCandidates } from './skill-plugin-cache-scan' @@ -463,6 +465,73 @@ describe('plugin skill candidate scan', () => { } ) + // Why: linking skills out of the cache is ordinary vendor packaging, so 'outside-root' is + // what fills the display budget on a real install — before any read failure is reached. + async function createCacheBehindSpentBudget(prefix: string, loopCount: number): Promise { + const parent = await mkdtemp(join(tmpdir(), prefix)) + temporaryDirectories.push(parent) + const root = join(parent, 'cache') + const outside = join(parent, 'outside') + await mkdir(outside, { recursive: true }) + await mkdir(root, { recursive: true }) + await Promise.all( + Array.from({ length: MAXIMUM_PLUGIN_SCAN_ISSUES }, (_, index) => + symlink(outside, join(root, `aa-linked-${index.toString().padStart(2, '0')}`), 'dir') + ) + ) + // Sorts after the links, so these are read once the budget is already spent. + await Promise.all( + Array.from({ length: loopCount }, (_, index) => { + const name = `zz-loop-${index.toString().padStart(2, '0')}` + return symlink(name, join(root, name), 'dir') + }) + ) + return root + } + + it.skipIf(process.platform === 'win32')( + 'keeps a read failure that lands after the display budget is spent', + async () => { + const root = await createCacheBehindSpentBudget('orca-plugin-attention-eviction-', 1) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: a read failure is the only issue that can take the headline off "all up to + // date". Evicting it for display budget reports all-clear over a path that could be + // hiding a stale copy — the bounds that filled the budget say nothing about it. + expect(result.issues).toContainEqual({ + path: join(root, 'zz-loop-00'), + reason: 'io-error', + errorCode: 'ELOOP' + }) + const inventoryIssues = result.issues.map((issue) => ({ + rootId: 'plugin-cache', + sourceLabel: 'Plugin cache', + ...issue + })) + expect(inventoryIssues.some(isSkillScanIssueNeedingAttention)).toBe(true) + } + ) + + it.skipIf(process.platform === 'win32')( + 'bounds how many read failures outrank the display budget', + async () => { + const root = await createCacheBehindSpentBudget( + 'orca-plugin-attention-bound-', + MAXIMUM_PLUGIN_SCAN_ATTENTION_ISSUES + 4 + ) + + const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli'])) + + // Why: outranking the budget is what makes this class unbounded, so a tree full of + // unreadable folders must not be able to pin one issue per folder in memory. + expect(result.issues.filter((issue) => issue.reason === 'io-error')).toHaveLength( + MAXIMUM_PLUGIN_SCAN_ATTENTION_ISSUES + ) + expect(result.issues).toContainEqual({ path: root, reason: 'issue-limit', errorCode: null }) + } + ) + 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) diff --git a/src/main/skills/skill-plugin-cache-scan.ts b/src/main/skills/skill-plugin-cache-scan.ts index 78fbc53cd7b..7f151e8475b 100644 --- a/src/main/skills/skill-plugin-cache-scan.ts +++ b/src/main/skills/skill-plugin-cache-scan.ts @@ -2,6 +2,7 @@ import type { Dirent } from 'node:fs' import { opendir, realpath, stat } from 'node:fs/promises' import { basename, join } from 'node:path' import { + isSkillScanAttentionReason, isTruncatingSkillScanReason, type SkillFreshnessScanIssueReason } from '../../shared/skill-freshness' @@ -19,6 +20,11 @@ const MAXIMUM_NESTED_SKILL_DEPTH = 2 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 +// pathological tree can pin in memory. One is all the badge needs to be truthful; a few +// more give the dialog enough distinct paths to read as a pattern, and 'issue-limit' still +// says there are others. +export const MAXIMUM_PLUGIN_SCAN_ATTENTION_ISSUES = 4 const SKILL_FILE_NAME = 'SKILL.md' export type KnownPluginSkillCandidate = { @@ -52,6 +58,7 @@ export async function scanKnownPluginSkillCandidates( const issues: KnownPluginSkillScanIssue[] = [] const issueKeys = new Set() const visited = new Set() + let attentionIssueCount = 0 let resolvedRoot: string | null = null let entryCount = 0 let limitReached = false @@ -70,9 +77,15 @@ export async function scanKnownPluginSkillCandidates( if (issueKeys.has(key)) { return } + // Why: an attention issue is the only thing that can turn the headline off "all up to + // date", so evicting one for display budget makes Orca report all-clear over a read + // failure. Reserving a few keeps that unbounded on a tree full of unreadable folders. + const attention = + isSkillScanAttentionReason(reason) && + attentionIssueCount < MAXIMUM_PLUGIN_SCAN_ATTENTION_ISSUES // 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) + const required = explainsCandidate || attention || 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. @@ -87,6 +100,9 @@ export async function scanKnownPluginSkillCandidates( return } issueKeys.add(key) + if (isSkillScanAttentionReason(reason)) { + attentionIssueCount += 1 + } issues.push({ path, reason, errorCode: code }) } diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 0daddd4ed58..0414a1823c0 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -114,8 +114,12 @@ export type SkillFreshnessScanIssue = { // every installed skill. It is still listed in Details, like the other bounds. const SKILL_SCAN_ATTENTION_REASONS = new Set(['io-error']) +export function isSkillScanAttentionReason(reason: SkillFreshnessScanIssueReason): boolean { + return SKILL_SCAN_ATTENTION_REASONS.has(reason) +} + export function isSkillScanIssueNeedingAttention(issue: SkillFreshnessScanIssue): boolean { - return SKILL_SCAN_ATTENTION_REASONS.has(issue.reason) + return isSkillScanAttentionReason(issue.reason) } // Why: these are the bounds that end the walk rather than skip one folder. They are