From aaba669a30108e79f53bd9ce879f1e0db0ef2bd1 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 14 Jul 2026 19:50:43 -0700 Subject: [PATCH] fix(skills): harden freshness integrity checks --- .../generate-skill-bundle-manifest.mjs | 16 ++- .../generate-skill-bundle-manifest.test.mjs | 7 +- .../scripts/verify-skill-update-roundtrip.mjs | 7 + .../skills/skill-bundle-artifacts.test.ts | 37 +++++ src/main/skills/skill-bundle-artifacts.ts | 132 +++++++++++++----- .../skills/skill-freshness-inventory.test.ts | 2 +- .../skills/skill-package-identity.test.ts | 10 ++ src/main/skills/skill-package-identity.ts | 8 +- .../skills/skill-plugin-cache-scan.test.ts | 14 ++ src/main/skills/skill-plugin-cache-scan.ts | 8 +- .../installed-agent-skills-change-event.ts | 1 + .../src/hooks/useInstalledAgentSkills.ts | 2 +- src/renderer/src/hooks/useSkillFreshness.ts | 6 +- 13 files changed, 201 insertions(+), 49 deletions(-) create mode 100644 src/main/skills/skill-bundle-artifacts.test.ts create mode 100644 src/renderer/src/hooks/installed-agent-skills-change-event.ts diff --git a/config/scripts/generate-skill-bundle-manifest.mjs b/config/scripts/generate-skill-bundle-manifest.mjs index 47a3e5f84a8..bdcc0aecca4 100644 --- a/config/scripts/generate-skill-bundle-manifest.mjs +++ b/config/scripts/generate-skill-bundle-manifest.mjs @@ -18,6 +18,10 @@ function sha256(bytes) { return createHash('sha256').update(bytes).digest('hex') } +function compareCodeUnits(left, right) { + return left === right ? 0 : left < right ? -1 : 1 +} + function gitObjectSha(kind, bytes) { return createHash('sha1').update(`${kind} ${bytes.length}\0`).update(bytes).digest() } @@ -116,7 +120,9 @@ async function collectPackageFiles(packageRoot) { async function visit(directory) { const entries = await readdir(directory, { withFileTypes: true }) - entries.sort((left, right) => left.name.localeCompare(right.name, 'en')) + // Why: build-time Node and packaged Electron may ship different ICU data; + // package identity order must use the same locale-independent comparison. + entries.sort((left, right) => compareCodeUnits(left.name, right.name)) for (const entry of entries) { const absolutePath = path.join(directory, entry.name) const relativePath = path.relative(packageRoot, absolutePath) @@ -243,7 +249,7 @@ function compareManifestPaths(left, right) { const rightParts = right.split('/') const shared = Math.min(leftParts.length, rightParts.length) for (let index = 0; index < shared; index += 1) { - const order = leftParts[index].localeCompare(rightParts[index], 'en') + const order = compareCodeUnits(leftParts[index], rightParts[index]) if (order !== 0) { return order } @@ -331,9 +337,7 @@ function buildReleasedHistory() { if (!packages) { throw new Error(`Missing released skill tree ${skillsTreeSha} at ${tag}`) } - for (const name of [...packages.keys()].sort((left, right) => - left.localeCompare(right, 'en') - )) { + for (const name of [...packages.keys()].sort(compareCodeUnits)) { const entries = packages.get(name) const filesWithGitHashes = collectGitPackageFiles(skillsTreeSha, name, entries, blobs) if (!filesWithGitHashes.some((file) => file.path === 'SKILL.md')) { @@ -369,7 +373,7 @@ async function buildArtifacts(appVersion) { const skillDirectories = (await readdir(SKILLS_ROOT, { withFileTypes: true })) .filter((entry) => entry.isDirectory()) .map((entry) => entry.name) - .sort((left, right) => left.localeCompare(right, 'en')) + .sort(compareCodeUnits) const currentSkills = [] for (const name of skillDirectories) { const filesWithGitHashes = await collectPackageFiles(path.join(SKILLS_ROOT, name)) diff --git a/config/scripts/generate-skill-bundle-manifest.test.mjs b/config/scripts/generate-skill-bundle-manifest.test.mjs index e731fcf10ff..063b30c57f3 100644 --- a/config/scripts/generate-skill-bundle-manifest.test.mjs +++ b/config/scripts/generate-skill-bundle-manifest.test.mjs @@ -44,10 +44,11 @@ describe('skill bundle manifest generator', () => { it('uses normalized text identity but exact executable identity', () => { const skillFile = describeFile('SKILL.md', Buffer.from('line one\r\nline two\r\n'), false) - const executable = describeFile('run.sh', Buffer.from('#!/bin/sh\necho ok\n'), true) + const executable = describeFile('run.sh', Buffer.from('#!/bin/sh\r\necho ok\r\n'), true) expect(skillFile.identitySha256).toBe(skillFile.textNormalizedSha256) expect(skillFile.identitySha256).not.toBe(skillFile.exactSha256) + expect(executable.exactSha256).not.toBe(executable.textNormalizedSha256) expect(executable.identitySha256).toBe(executable.exactSha256) expect(packageDigest([skillFile, executable])).toMatch(/^[a-f0-9]{64}$/) }) @@ -69,10 +70,10 @@ describe('skill bundle manifest generator', () => { expect(sortManifestFiles(gitOrdered)).toEqual(walked) expect(packageDigest(sortManifestFiles(gitOrdered))).toBe(packageDigest(walked)) expect(walked.map((file) => file.path)).toEqual([ + 'Zebra.md', 'apple.md', 'sub/inner.txt', - 'sub.md', - 'Zebra.md' + 'sub.md' ]) }) diff --git a/config/scripts/verify-skill-update-roundtrip.mjs b/config/scripts/verify-skill-update-roundtrip.mjs index ef42d372c63..f6154691af9 100644 --- a/config/scripts/verify-skill-update-roundtrip.mjs +++ b/config/scripts/verify-skill-update-roundtrip.mjs @@ -210,6 +210,13 @@ try { execSkills(['update', targetName, '--global']) await assertCurrentCanonical(targetName) const targetProviderAfter = await packageDigestAt(await realpath(targetProvider)) + const targetProviderStat = await lstat(targetProvider) + if (shape === 'symlink' && !targetProviderStat.isSymbolicLink()) { + throw new Error(`${targetName} provider alias was replaced with an independent copy`) + } + if (shape === 'copy' && targetProviderStat.isSymbolicLink()) { + throw new Error(`${targetName} independent provider copy was replaced with an alias`) + } if (shape === 'symlink' && targetProviderAfter !== currentSkill(targetName).packageDigest) { throw new Error(`${targetName} provider alias did not converge with the canonical update`) } diff --git a/src/main/skills/skill-bundle-artifacts.test.ts b/src/main/skills/skill-bundle-artifacts.test.ts new file mode 100644 index 00000000000..f86d5c382d7 --- /dev/null +++ b/src/main/skills/skill-bundle-artifacts.test.ts @@ -0,0 +1,37 @@ +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { loadSkillBundleArtifacts } from './skill-bundle-artifacts' + +const temporaryDirectories: string[] = [] + +afterEach(async () => { + await Promise.all(temporaryDirectories.splice(0).map((root) => rm(root, { recursive: true }))) +}) + +describe('skill bundle artifacts', () => { + it('rejects malformed nested release entries before building provenance', async () => { + const resourceRoot = await mkdtemp(join(tmpdir(), 'orca-skill-artifacts-')) + temporaryDirectories.push(resourceRoot) + const target = join(resourceRoot, 'skills') + const source = resolve('resources', 'skills') + await mkdir(target, { recursive: true }) + const [manifest, registry, releaseMapping] = await Promise.all( + ['current-manifest.json', 'snapshot-registry.json', 'release-mapping.json'].map((name) => + readFile(join(source, name), 'utf8') + ) + ) + const malformedMapping = JSON.parse(releaseMapping) + malformedMapping.releases[0] = { appVersion: 'invalid' } + await Promise.all([ + writeFile(join(target, 'current-manifest.json'), manifest), + writeFile(join(target, 'snapshot-registry.json'), registry), + writeFile(join(target, 'release-mapping.json'), JSON.stringify(malformedMapping)) + ]) + + await expect(loadSkillBundleArtifacts(resourceRoot)).rejects.toThrow( + 'Invalid skill release mapping' + ) + }) +}) diff --git a/src/main/skills/skill-bundle-artifacts.ts b/src/main/skills/skill-bundle-artifacts.ts index d2a1eca9ff9..e7d403442b5 100644 --- a/src/main/skills/skill-bundle-artifacts.ts +++ b/src/main/skills/skill-bundle-artifacts.ts @@ -1,6 +1,7 @@ import { app } from 'electron' import { readFile } from 'node:fs/promises' import { join, resolve } from 'node:path' +import { z, type ZodType } from 'zod' import type { SkillBundleManifest, SkillKnownSnapshot, @@ -16,18 +17,70 @@ export type SkillBundleArtifacts = { releasedAppVersions: Record> } -function assertSupportedSchema( - value: unknown, - label: string -): asserts value is { schemaVersion: 1 } { - if ( - typeof value !== 'object' || - value === null || - !('schemaVersion' in value) || - value.schemaVersion !== 1 - ) { - throw new Error(`Unsupported ${label} schema`) +const sha256Schema = z.string().regex(/^[a-f0-9]{64}$/) +const snapshotShape = { + releaseRevision: z.number().int().positive(), + packageDigest: sha256Schema, + gitTreeSha: z.string().regex(/^[a-f0-9]{40}$/), + files: z + .array( + z + .object({ + path: z.string().min(1), + size: z.number().int().nonnegative(), + executable: z.boolean(), + classification: z.enum(['text', 'binary']), + exactSha256: sha256Schema, + textNormalizedSha256: sha256Schema.nullable(), + identitySha256: sha256Schema + }) + .strict() + ) + .min(1) +} +const knownSnapshotSchema = z.object(snapshotShape).strict() +const manifestSchema = z + .object({ + schemaVersion: z.literal(1), + appVersion: z.string().min(1), + skills: z.array( + z + .object({ + name: z.string().regex(/^[a-z0-9][a-z0-9._-]*$/), + sourcePath: z.string().min(1), + appVersion: z.string().min(1), + ...snapshotShape + }) + .strict() + ) + }) + .strict() +const registrySchema = z + .object({ + schemaVersion: z.literal(1), + skills: z.record(z.string().min(1), z.array(knownSnapshotSchema).min(1)) + }) + .strict() +const releaseMappingSchema = z + .object({ + schemaVersion: z.literal(1), + releases: z.array( + z + .object({ + appVersion: z.string().min(1), + skills: z.record(z.string().min(1), z.number().int().positive()) + }) + .strict() + ) + }) + .strict() + +function parseArtifact(schema: ZodType, value: unknown, label: string): T { + const result = schema.safeParse(value) + if (!result.success) { + throw new Error(`Invalid ${label}: ${result.error.issues[0]?.message ?? 'schema mismatch'}`) } + return result.data } const artifactsByResourceRoot = new Map>() @@ -51,46 +104,61 @@ export function loadSkillBundleArtifacts( async function readSkillBundleArtifacts(resourceRoot: string): Promise { const bundleRoot = join(resourceRoot, 'skills') - const [manifest, registry, releaseMapping] = await Promise.all([ + const [manifestValue, registryValue, releaseMappingValue] = await Promise.all([ readFile(join(bundleRoot, 'current-manifest.json'), 'utf8').then(JSON.parse), readFile(join(bundleRoot, 'snapshot-registry.json'), 'utf8').then(JSON.parse), readFile(join(bundleRoot, 'release-mapping.json'), 'utf8').then(JSON.parse) ]) - assertSupportedSchema(manifest, 'skill bundle manifest') - assertSupportedSchema(registry, 'skill snapshot registry') - assertSupportedSchema(releaseMapping, 'skill release mapping') - if (!('skills' in manifest) || !Array.isArray(manifest.skills)) { - throw new Error('Invalid skill bundle manifest') - } - if (!('appVersion' in manifest) || typeof manifest.appVersion !== 'string') { - throw new Error('Invalid skill bundle manifest') - } - if (!('skills' in registry) || typeof registry.skills !== 'object' || registry.skills === null) { - throw new Error('Invalid skill snapshot registry') - } - if (!('releases' in releaseMapping) || !Array.isArray(releaseMapping.releases)) { - throw new Error('Invalid skill release mapping') + const manifest: SkillBundleManifest = parseArtifact( + manifestSchema, + manifestValue, + 'skill bundle manifest' + ) + const registry: SkillSnapshotRegistry = parseArtifact( + registrySchema, + registryValue, + 'skill snapshot registry' + ) + const releaseMapping: SkillReleaseMapping = parseArtifact( + releaseMappingSchema, + releaseMappingValue, + 'skill release mapping' + ) + for (const current of manifest.skills) { + if ( + current.appVersion !== manifest.appVersion || + !registry.skills[current.name]?.some( + (snapshot) => + snapshot.releaseRevision === current.releaseRevision && + snapshot.packageDigest === current.packageDigest + ) + ) { + throw new Error(`Inconsistent current skill snapshot: ${current.name}`) + } } const releasedAppVersions: Record> = {} - for (const release of (releaseMapping as SkillReleaseMapping).releases) { + for (const release of releaseMapping.releases) { for (const [name, revision] of Object.entries(release.skills)) { + if (!registry.skills[name]?.some((snapshot) => snapshot.releaseRevision === revision)) { + throw new Error(`Unknown released skill revision: ${name}@${revision}`) + } releasedAppVersions[name] ??= {} releasedAppVersions[name][revision] ??= release.appVersion } } - for (const current of (manifest as SkillBundleManifest).skills) { + for (const current of manifest.skills) { releasedAppVersions[current.name] ??= {} releasedAppVersions[current.name][current.releaseRevision] = current.appVersion } return { - manifest: manifest as SkillBundleManifest, - registry: registry as SkillSnapshotRegistry, - releaseMapping: releaseMapping as SkillReleaseMapping, + manifest, + registry, + releaseMapping, // Why: newer-known classification needs every identity packaged with this // build, while release mapping remains the provenance record for shipped revisions. - knownSnapshots: (registry as SkillSnapshotRegistry).skills, + knownSnapshots: registry.skills, releasedAppVersions } } diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index d80a0307667..fe29f7ce3d2 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -30,7 +30,7 @@ function snapshot(releaseRevision: number, markdown: string): SkillKnownSnapshot return { releaseRevision, packageDigest: skillPackageDigest([file]), - gitTreeSha: `tree-${releaseRevision}`, + gitTreeSha: releaseRevision.toString(16).padStart(40, '0'), files: [file] } } diff --git a/src/main/skills/skill-package-identity.test.ts b/src/main/skills/skill-package-identity.test.ts index e21acbddccb..f88d2692c78 100644 --- a/src/main/skills/skill-package-identity.test.ts +++ b/src/main/skills/skill-package-identity.test.ts @@ -48,6 +48,16 @@ describe('skill package identity', () => { expect(binary.classification).toBe('binary') }) + it('orders package files by locale-independent code units', async () => { + const root = await temporarySkill() + await writeFile(join(root, 'apple.md'), 'apple') + await writeFile(join(root, 'Zebra.md'), 'zebra') + + const observed = await observeSkillPackage(root) + + expect(observed.files.map((file) => file.path)).toEqual(['Zebra.md', 'apple.md']) + }) + it('rejects links and bounded-observation overflows', async () => { const root = await temporarySkill() await writeFile(join(root, 'SKILL.md'), 'skill') diff --git a/src/main/skills/skill-package-identity.ts b/src/main/skills/skill-package-identity.ts index 6e28bb0afe0..e6e06f37327 100644 --- a/src/main/skills/skill-package-identity.ts +++ b/src/main/skills/skill-package-identity.ts @@ -63,6 +63,10 @@ function sha256(bytes: Buffer): string { return createHash('sha256').update(bytes).digest('hex') } +function compareCodeUnits(left: string, right: string): number { + return left === right ? 0 : left < right ? -1 : 1 +} + function normalizedText(bytes: Buffer): Buffer { const text = new TextDecoder('utf-8', { fatal: true }).decode(bytes) return Buffer.from(text.replace(/\r\n/g, '\n').replace(/\r/g, '\n'), 'utf8') @@ -154,7 +158,9 @@ export async function observeSkillPackage( } finally { await directoryHandle.close().catch(() => undefined) } - entries.sort((left, right) => left.name.localeCompare(right.name, 'en')) + // Why: runtime Electron and the build's Node may carry different ICU data; + // identity order must match the generator without locale-sensitive collation. + entries.sort((left, right) => compareCodeUnits(left.name, right.name)) for (const entry of entries) { const absolutePath = join(directory, entry.name) const relativePath = relative(packageRoot, absolutePath) diff --git a/src/main/skills/skill-plugin-cache-scan.test.ts b/src/main/skills/skill-plugin-cache-scan.test.ts index 90c71b2d470..46216ce6c04 100644 --- a/src/main/skills/skill-plugin-cache-scan.test.ts +++ b/src/main/skills/skill-plugin-cache-scan.test.ts @@ -23,4 +23,18 @@ describe('plugin skill candidate scan', () => { expect(result.candidates).toHaveLength(1) expect(result.incompletePaths).toEqual([root]) }) + + it('marks depth-truncated subtrees incomplete so hidden skills poison eligibility', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-plugin-skill-depth-')) + temporaryDirectories.push(root) + const segments = Array.from({ length: 11 }, (_, index) => `level-${index}`) + const hiddenSkill = join(root, ...segments, 'orca-cli') + await mkdir(hiddenSkill, { recursive: true }) + + 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) + }) }) diff --git a/src/main/skills/skill-plugin-cache-scan.ts b/src/main/skills/skill-plugin-cache-scan.ts index 3a8ec05924d..7162cedbc47 100644 --- a/src/main/skills/skill-plugin-cache-scan.ts +++ b/src/main/skills/skill-plugin-cache-scan.ts @@ -50,7 +50,11 @@ export async function scanKnownPluginSkillCandidates( } async function visit(directory: string, depth: number): Promise { - if (limitReached || depth > MAXIMUM_PLUGIN_SCAN_DEPTH) { + if (limitReached) { + return + } + if (depth > MAXIMUM_PLUGIN_SCAN_DEPTH) { + recordIncomplete(directory) return } let resolved: string @@ -95,7 +99,7 @@ export async function scanKnownPluginSkillCandidates( await handle.close().catch(() => undefined) } - entries.sort((left, right) => left.name.localeCompare(right.name, 'en')) + entries.sort((left, right) => (left.name === right.name ? 0 : left.name < right.name ? -1 : 1)) for (const entry of entries) { if (limitReached) { return diff --git a/src/renderer/src/hooks/installed-agent-skills-change-event.ts b/src/renderer/src/hooks/installed-agent-skills-change-event.ts new file mode 100644 index 00000000000..2e11a17ff03 --- /dev/null +++ b/src/renderer/src/hooks/installed-agent-skills-change-event.ts @@ -0,0 +1 @@ +export const INSTALLED_AGENT_SKILLS_CHANGED_EVENT = 'orca:installed-agent-skills-changed' diff --git a/src/renderer/src/hooks/useInstalledAgentSkills.ts b/src/renderer/src/hooks/useInstalledAgentSkills.ts index e06a8a2a2ee..e19bac655b1 100644 --- a/src/renderer/src/hooks/useInstalledAgentSkills.ts +++ b/src/renderer/src/hooks/useInstalledAgentSkills.ts @@ -7,9 +7,9 @@ import type { } from '../../../shared/skills' import { ORCHESTRATION_SKILL_NAME } from '@/lib/agent-feature-install-commands' import { markOrchestrationSetupComplete } from '@/lib/orchestration-setup-state' +import { INSTALLED_AGENT_SKILLS_CHANGED_EVENT } from './installed-agent-skills-change-event' import { useMountedRef } from './useMountedRef' -const INSTALLED_AGENT_SKILLS_CHANGED_EVENT = 'orca:installed-agent-skills-changed' export const GLOBAL_AGENT_SKILL_SOURCE_KINDS = [ 'home' ] as const satisfies readonly SkillSourceKind[] diff --git a/src/renderer/src/hooks/useSkillFreshness.ts b/src/renderer/src/hooks/useSkillFreshness.ts index b09b815eb9c..382787bf258 100644 --- a/src/renderer/src/hooks/useSkillFreshness.ts +++ b/src/renderer/src/hooks/useSkillFreshness.ts @@ -1,8 +1,8 @@ import { useCallback, useEffect, useState } from 'react' import type { SkillFreshnessInventory } from '../../../shared/skill-freshness' +import { INSTALLED_AGENT_SKILLS_CHANGED_EVENT } from './installed-agent-skills-change-event' import { useMountedRef } from './useMountedRef' -const INSTALLED_SKILLS_CHANGED_EVENT = 'orca:installed-agent-skills-changed' // Why: window focus fires on every alt-tab, and each scan re-reads and re-hashes // every installed package; a just-completed scan stays authoritative briefly. const FOCUS_RESCAN_COOLDOWN_MS = 15_000 @@ -100,10 +100,10 @@ export function useSkillFreshness(): SkillFreshnessState { const onFocus = (): void => invalidate(true) const onInstalledSkillsChanged = (): void => invalidate(false) window.addEventListener('focus', onFocus) - window.addEventListener(INSTALLED_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged) + window.addEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged) return () => { window.removeEventListener('focus', onFocus) - window.removeEventListener(INSTALLED_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged) + window.removeEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged) } }, [refresh])