From a0148e74420cfb4bdb5854e8a454dd517bb92691 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 12 Jul 2026 23:44:14 -0700 Subject: [PATCH] fix(skills): harden managed update safety --- config/reliability-gates.jsonc | 7 +- .../generate-skill-bundle-manifest.mjs | 49 ++++++ .../generate-skill-bundle-manifest.test.mjs | 40 +++++ package.json | 2 +- resources/skills/current-manifest.json | 2 +- resources/skills/release-mapping.json | 15 +- resources/skills/snapshot-registry.json | 32 ++++ .../skills/skill-candidate-concurrency.ts | 25 +++ .../skills/skill-management-inventory.test.ts | 31 +++- src/main/skills/skill-management-inventory.ts | 35 ++-- src/main/skills/skill-management-status.ts | 15 ++ .../skills/skill-orphan-transaction-sweep.ts | 49 ++++-- src/main/skills/skill-package-publish.ts | 27 +++- .../skill-package-transaction-context.ts | 6 +- .../skills/skill-transaction-marker-write.ts | 76 +++++++++ src/main/skills/skill-transaction-marker.ts | 34 +--- .../skills/skill-transaction-recovery.test.ts | 52 +++++- src/main/skills/skill-transaction-recovery.ts | 149 +++++++++++++++--- src/main/skills/skill-transaction-restore.ts | 6 +- .../settings/ManagedOrcaSkillsSection.tsx | 12 +- .../managed-skill-status-copy.test.ts | 24 +++ .../settings/managed-skill-status-copy.ts | 20 ++- 22 files changed, 581 insertions(+), 127 deletions(-) create mode 100644 src/main/skills/skill-candidate-concurrency.ts create mode 100644 src/main/skills/skill-management-status.ts create mode 100644 src/main/skills/skill-transaction-marker-write.ts create mode 100644 src/renderer/src/components/settings/managed-skill-status-copy.test.ts diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 670f3435a33..4c2a32c5784 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3702,7 +3702,8 @@ { "file": "src/main/skills/skill-management-inventory.test.ts", "assertions": [ - "oversized and access-denied candidates fail closed without hiding healthy siblings" + "oversized and access-denied candidates fail closed without hiding healthy siblings", + "candidate filesystem classification never exceeds four concurrent tasks" ] } ], @@ -3714,7 +3715,7 @@ "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/skills/skill-package-observation-limits.test.ts src/main/skills/skill-management-inventory.test.ts", "result": "passed", "durationSeconds": 0.183, - "summary": "15 deterministic bounded-observation and sibling-isolation tests passed." + "summary": "16 deterministic bounded-observation, concurrency, and sibling-isolation tests passed." } ], "runtimeBudget": { "p95Seconds": 10, "scope": "local main-process unit tests" }, @@ -3728,7 +3729,7 @@ }, "performanceBudget": { "required": true, - "evidence": "Production observation is capped at depth 16, 2,048 entries, 512 files, 4 MiB per file, and 32 MiB aggregate per candidate with no polling or subprocesses." + "evidence": "Production observation is capped at four concurrent candidates, depth 16, 2,048 entries, 512 files, 4 MiB per file, and 32 MiB aggregate per candidate with no polling or subprocesses." }, "promotionCriteria": [ "Accumulate the required soak runs across macOS, Linux, and Windows.", diff --git a/config/scripts/generate-skill-bundle-manifest.mjs b/config/scripts/generate-skill-bundle-manifest.mjs index 6d7bd631e49..ebf80db113d 100644 --- a/config/scripts/generate-skill-bundle-manifest.mjs +++ b/config/scripts/generate-skill-bundle-manifest.mjs @@ -287,6 +287,49 @@ function readJsonAtRef(ref, relativePath) { } } +function gitRefExists(ref) { + try { + execFileSync('git', ['rev-parse', '--verify', '--quiet', `${ref}^{commit}`], { + stdio: 'ignore' + }) + return true + } catch { + return false + } +} + +function assertReleaseMappingProvenance( + artifacts, + { + allowUnreleasedAppVersion = null, + refExists = gitRefExists, + digestAtRef = (ref, name) => packageDigest(collectGitPackageFiles(ref, name)) + } = {} +) { + for (const release of artifacts.releaseMapping.releases) { + const releaseRef = `v${release.appVersion}` + if (!refExists(releaseRef)) { + if (release.appVersion === allowUnreleasedAppVersion) { + continue + } + throw new Error(`Skill release mapping references missing tag ${releaseRef}`) + } + for (const [name, revision] of Object.entries(release.skills)) { + const snapshot = (artifacts.snapshotRegistry.skills[name] ?? []).find( + (entry) => entry.releaseRevision === revision + ) + if (!snapshot) { + throw new Error(`Skill release mapping references unknown ${name} revision ${revision}`) + } + if (digestAtRef(releaseRef, name) !== snapshot.packageDigest) { + throw new Error( + `Skill release mapping for ${releaseRef} does not match shipped ${name} revision ${revision}` + ) + } + } + } +} + function assertBaseHistoryPreserved(baseRef, artifacts) { if (!baseRef) { return @@ -442,6 +485,11 @@ async function main() { recordRelease, bootstrapHistory }) + // Why: a package version can remain unchanged while main advances; only + // tag bytes prove that a historical revision actually shipped. + assertReleaseMappingProvenance(artifacts, { + allowUnreleasedAppVersion: recordRelease ? packageJson.version : null + }) assertBaseHistoryPreserved(baseRef, artifacts) await (write ? writeArtifacts : verifyArtifacts)(artifacts) } @@ -454,6 +502,7 @@ if (process.argv[1] && path.resolve(process.argv[1]) === import.meta.filename) { } export { + assertReleaseMappingProvenance, buildArtifacts, classifyFile, collectPackageFiles, diff --git a/config/scripts/generate-skill-bundle-manifest.test.mjs b/config/scripts/generate-skill-bundle-manifest.test.mjs index 1c694310084..dfea0a4357d 100644 --- a/config/scripts/generate-skill-bundle-manifest.test.mjs +++ b/config/scripts/generate-skill-bundle-manifest.test.mjs @@ -4,6 +4,7 @@ import { tmpdir } from 'node:os' import path from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import { + assertReleaseMappingProvenance, classifyFile, collectPackageFiles, gitTreeSha, @@ -95,4 +96,43 @@ describe('skill bundle manifest generator', () => { const files = await collectPackageFiles(packageRoot) expect(gitTreeSha(files)).toMatch(/^[a-f0-9]{40}$/) }) + + it('rejects a released revision that differs from the tagged package', () => { + const digest = 'a'.repeat(64) + const artifacts = { + snapshotRegistry: { + skills: { + orchestration: [{ releaseRevision: 2, packageDigest: digest }] + } + }, + releaseMapping: { + releases: [{ appVersion: '1.2.3', skills: { orchestration: 2 } }] + } + } + + expect(() => + assertReleaseMappingProvenance(artifacts, { + refExists: () => true, + digestAtRef: () => 'b'.repeat(64) + }) + ).toThrow('does not match shipped orchestration revision 2') + }) + + it('allows only the in-progress release cut to precede its tag', () => { + const artifacts = { + snapshotRegistry: { skills: { 'orca-cli': [{ releaseRevision: 1, packageDigest: 'a' }] } }, + releaseMapping: { releases: [{ appVersion: '1.2.4', skills: { 'orca-cli': 1 } }] } + } + const options = { refExists: () => false, digestAtRef: () => 'a' } + + expect(() => assertReleaseMappingProvenance(artifacts, options)).toThrow( + 'references missing tag v1.2.4' + ) + expect(() => + assertReleaseMappingProvenance(artifacts, { + ...options, + allowUnreleasedAppVersion: '1.2.4' + }) + ).not.toThrow() + }) }) diff --git a/package.json b/package.json index c811e3f9c30..a2c0d40dda3 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "check:max-lines-ratchet": "node config/scripts/check-max-lines-ratchet.mjs", "check:feature-wall-assets": "node config/scripts/check-feature-wall-assets.mjs", "generate:skill-bundle-manifest": "node config/scripts/generate-skill-bundle-manifest.mjs --write", - "bootstrap:skill-release-history": "node config/scripts/generate-skill-bundle-manifest.mjs --write --record-release --bootstrap-release-history", + "bootstrap:skill-release-history": "node config/scripts/generate-skill-bundle-manifest.mjs --write --bootstrap-release-history", "record-release:skill-bundle-manifest": "node config/scripts/generate-skill-bundle-manifest.mjs --write --record-release", "verify:skill-bundle-manifest": "node config/scripts/generate-skill-bundle-manifest.mjs", "verify:macos-entitlements": "node config/scripts/verify-macos-entitlements.mjs", diff --git a/resources/skills/current-manifest.json b/resources/skills/current-manifest.json index a580f3592a9..d282de48f39 100644 --- a/resources/skills/current-manifest.json +++ b/resources/skills/current-manifest.json @@ -139,7 +139,7 @@ "name": "orchestration", "sourcePath": "skills/orchestration", "appVersion": "1.4.138-rc.2", - "releaseRevision": 23, + "releaseRevision": 25, "packageDigest": "dab5cc1bf87b12838a53dda90f1f4f35d1f77da66bcc63840574531eff600d2b", "gitTreeSha": "f7a017f3e1f14dafed0c59717bec37c022075be2", "files": [ diff --git a/resources/skills/release-mapping.json b/resources/skills/release-mapping.json index 076d6aed0f3..fe1eadee106 100644 --- a/resources/skills/release-mapping.json +++ b/resources/skills/release-mapping.json @@ -459,7 +459,7 @@ } }, { - "appVersion": "1.4.138-rc.2", + "appVersion": "1.4.138-rc.3", "skills": { "computer-use": 4, "linear-tickets": 4, @@ -470,6 +470,19 @@ "orca-per-workspace-env": 2, "orchestration": 23 } + }, + { + "appVersion": "1.4.138-rc.4", + "skills": { + "computer-use": 4, + "linear-tickets": 4, + "orca-cli": 31, + "orca-emulator": 3, + "orca-emulator-android": 1, + "orca-linear": 2, + "orca-per-workspace-env": 2, + "orchestration": 24 + } } ] } diff --git a/resources/skills/snapshot-registry.json b/resources/skills/snapshot-registry.json index 87601a19487..d39f4f1681f 100644 --- a/resources/skills/snapshot-registry.json +++ b/resources/skills/snapshot-registry.json @@ -867,6 +867,38 @@ "identitySha256": "b73c7c665aa233a782ba81982ebc954e220fdf1c1c6ca2b6bf38133c1cd76fc9" } ] + }, + { + "releaseRevision": 24, + "packageDigest": "9fbfa2ae3f3f99441563a4b8b1c6302107944480db8718ddb326a862a51f7ab9", + "gitTreeSha": "086c41e0b353b4908d2963694b4a7c791d4b3982", + "files": [ + { + "path": "SKILL.md", + "size": 22861, + "executable": false, + "classification": "text", + "exactSha256": "0cb898d37560ab7ac4cb477a2d7e8ba3e593d45affd6d7e9f99151879aaeb1a5", + "textNormalizedSha256": "0cb898d37560ab7ac4cb477a2d7e8ba3e593d45affd6d7e9f99151879aaeb1a5", + "identitySha256": "0cb898d37560ab7ac4cb477a2d7e8ba3e593d45affd6d7e9f99151879aaeb1a5" + } + ] + }, + { + "releaseRevision": 25, + "packageDigest": "dab5cc1bf87b12838a53dda90f1f4f35d1f77da66bcc63840574531eff600d2b", + "gitTreeSha": "f7a017f3e1f14dafed0c59717bec37c022075be2", + "files": [ + { + "path": "SKILL.md", + "size": 22850, + "executable": false, + "classification": "text", + "exactSha256": "b73c7c665aa233a782ba81982ebc954e220fdf1c1c6ca2b6bf38133c1cd76fc9", + "textNormalizedSha256": "b73c7c665aa233a782ba81982ebc954e220fdf1c1c6ca2b6bf38133c1cd76fc9", + "identitySha256": "b73c7c665aa233a782ba81982ebc954e220fdf1c1c6ca2b6bf38133c1cd76fc9" + } + ] } ], "mobile-fit-debug": [ diff --git a/src/main/skills/skill-candidate-concurrency.ts b/src/main/skills/skill-candidate-concurrency.ts new file mode 100644 index 00000000000..e3e0d7e876e --- /dev/null +++ b/src/main/skills/skill-candidate-concurrency.ts @@ -0,0 +1,25 @@ +import type { ClassifiedSkillCandidate } from './skill-management-inaccessible-candidate' + +export const SKILL_CANDIDATE_SCAN_CONCURRENCY = 4 + +export async function runSkillCandidateTasks( + tasks: readonly (() => Promise)[] +): Promise<(ClassifiedSkillCandidate | null)[]> { + const results: (ClassifiedSkillCandidate | null)[] = tasks.map(() => null) + let nextIndex = 0 + const workers = Array.from( + { length: Math.min(SKILL_CANDIDATE_SCAN_CONCURRENCY, tasks.length) }, + async () => { + for (;;) { + const index = nextIndex + nextIndex += 1 + if (index >= tasks.length) { + return + } + results[index] = await tasks[index]!() + } + } + ) + await Promise.all(workers) + return results +} diff --git a/src/main/skills/skill-management-inventory.test.ts b/src/main/skills/skill-management-inventory.test.ts index db85b863862..bdede87bf4a 100644 --- a/src/main/skills/skill-management-inventory.test.ts +++ b/src/main/skills/skill-management-inventory.test.ts @@ -68,7 +68,7 @@ describe('managed skill inventory', () => { it('suppresses only the explicitly dismissed destination snapshot tuple', async () => { const home = await createHome() await installSkill(home, '.agents', 'orca-cli') - await installSkill(home, '.agents', 'orchestration') + await installSkill(home, '.agents', 'computer-use') const initial = await inventoryManagedSkills({ ledger: emptySkillManagementLedger(), hostId: 'local', @@ -99,7 +99,7 @@ describe('managed skill inventory', () => { expect(next.installations.find((entry) => entry.name === 'orca-cli')).toMatchObject({ adoptionPromptEligible: false }) - expect(next.installations.find((entry) => entry.name === 'orchestration')).toMatchObject({ + expect(next.installations.find((entry) => entry.name === 'computer-use')).toMatchObject({ adoptionPromptEligible: true }) }) @@ -240,7 +240,7 @@ describe('managed skill inventory', () => { it('bounds one oversized candidate without hiding healthy siblings', async () => { const home = await createHome() const oversized = await installSkill(home, '.agents', 'orca-cli') - await installSkill(home, '.agents', 'orchestration') + await installSkill(home, '.agents', 'computer-use') const oversizedFile = join(oversized, 'oversized.bin') await writeFile(oversizedFile, '') await truncate(oversizedFile, 4 * 1024 * 1024 + 1) @@ -256,7 +256,7 @@ describe('managed skill inventory', () => { status: 'unknown', errorCategory: 'skill-package-file-size-limit' }) - expect(inventory.installations.find((entry) => entry.name === 'orchestration')).toMatchObject({ + expect(inventory.installations.find((entry) => entry.name === 'computer-use')).toMatchObject({ status: 'known-current', eligible: true }) @@ -321,6 +321,29 @@ describe('managed skill inventory', () => { ).toBeTruthy() }) + it('caps concurrent candidate filesystem observations', async () => { + const home = await createHome() + let active = 0 + let peak = 0 + + await inventoryManagedSkills({ + ledger: emptySkillManagementLedger(), + hostId: 'local', + homeDir: home, + resourceRoot, + candidateLstat: async () => { + active += 1 + peak = Math.max(peak, active) + await new Promise((resolve) => queueMicrotask(resolve)) + active -= 1 + throw Object.assign(new Error('missing'), { code: 'ENOENT' }) + } + }) + + expect(peak).toBeGreaterThan(1) + expect(peak).toBeLessThanOrEqual(4) + }) + it.runIf(process.platform !== 'win32')( 'deduplicates a provider alias into its canonical physical copy', async () => { diff --git a/src/main/skills/skill-management-inventory.ts b/src/main/skills/skill-management-inventory.ts index a4f754a7052..5b594264c18 100644 --- a/src/main/skills/skill-management-inventory.ts +++ b/src/main/skills/skill-management-inventory.ts @@ -7,8 +7,7 @@ import type { SkillCurrentBundleEntry, SkillManagementInstallation, SkillManagementInventory, - SkillManagementLedger, - SkillManagementStatus + SkillManagementLedger } from '../../shared/skill-management' import { buildSkillDiscoverySources, type SkillScanRoot } from './skill-discovery-sources' import { loadSkillBundleArtifacts, type SkillBundleArtifacts } from './skill-bundle-artifacts' @@ -26,6 +25,8 @@ import { skillTopologyPriority } from './skill-installation-topology' import { readOfficialSkillsCliLockEntries } from './skills-cli-lock-provenance' +import { runSkillCandidateTasks } from './skill-candidate-concurrency' +import { knownSkillManagementStatus } from './skill-management-status' type CandidateLstat = (path: string) => Promise type CandidateTopology = ( @@ -34,20 +35,6 @@ type CandidateTopology = ( canonicalRootPath: string ) => Promise -function knownStatus( - managed: boolean, - installedRevision: number, - currentRevision: number -): SkillManagementStatus { - if (installedRevision > currentRevision) { - return 'newer-known' - } - if (installedRevision < currentRevision) { - return managed ? 'managed-update-available' : 'known-update-available' - } - return managed ? 'managed-current' : 'known-current' -} - async function classifyCandidate(args: { root: SkillScanRoot name: string @@ -161,7 +148,11 @@ async function classifyCandidate(args: { status: match ? managed && managedRecord?.lastOutcome === 'failed' ? 'update-failed' - : knownStatus(managed, match.releaseRevision, args.current.releaseRevision) + : knownSkillManagementStatus( + managed, + match.releaseRevision, + args.current.releaseRevision + ) : managedRecord ? 'modified' : 'unknown', @@ -222,9 +213,9 @@ export async function inventoryManagedSkills(args: { : null : args.xdgStateHome }) - const candidates = await Promise.all( - artifacts.manifest.skills.flatMap((current) => - roots.map((root) => + const candidateTasks = artifacts.manifest.skills.flatMap((current) => + roots.map( + (root) => () => classifyCandidate({ root, name: current.name, @@ -239,9 +230,11 @@ export async function inventoryManagedSkills(args: { candidateLstat: args.candidateLstat ?? ((path) => lstat(path)), classifyTopology: args.classifyTopology ?? classifySkillInstallationTopology }) - ) ) ) + // Why: each candidate may retain up to the package byte ceiling while it is + // hashed, so mounted/focus inventory must not fan out across every root. + const candidates = await runSkillCandidateTasks(candidateTasks) const deduped = new Map() for (const candidate of candidates) { diff --git a/src/main/skills/skill-management-status.ts b/src/main/skills/skill-management-status.ts new file mode 100644 index 00000000000..9f6d3d1e21d --- /dev/null +++ b/src/main/skills/skill-management-status.ts @@ -0,0 +1,15 @@ +import type { SkillManagementStatus } from '../../shared/skill-management' + +export function knownSkillManagementStatus( + managed: boolean, + installedRevision: number, + currentRevision: number +): SkillManagementStatus { + if (installedRevision > currentRevision) { + return 'newer-known' + } + if (installedRevision < currentRevision) { + return managed ? 'managed-update-available' : 'known-update-available' + } + return managed ? 'managed-current' : 'known-current' +} diff --git a/src/main/skills/skill-orphan-transaction-sweep.ts b/src/main/skills/skill-orphan-transaction-sweep.ts index 6cbc86f01c6..780ec398763 100644 --- a/src/main/skills/skill-orphan-transaction-sweep.ts +++ b/src/main/skills/skill-orphan-transaction-sweep.ts @@ -1,18 +1,19 @@ import { lstat, mkdir, readFile, readdir, realpath, rm } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' import type { SkillManagementLedger } from '../../shared/skill-management' -import { normalizedSkillIdentityPath } from './skill-installation-topology' +import { normalizedSkillIdentityPath, skillPhysicalIdentity } from './skill-installation-topology' import { parseSkillTransactionMarker, type SkillTransactionMarker } from './skill-transaction-marker' -import { RESERVED_SKILL_TRANSACTION_DIRECTORY } from './skill-transaction-paths' import { acquireSkillDestinationLock } from './skill-transaction-recovery' import { recoverTransactionToPrior } from './skill-transaction-restore' +import { skillTransactionReservedRoot } from './skill-transaction-workspace' async function assertApprovedDestinationParent( marker: SkillTransactionMarker, - rootIdentity: string + rootIdentity: string, + rootPhysicalIdentity: string ): Promise { if (normalizedSkillIdentityPath(dirname(resolve(marker.destinationPath))) !== rootIdentity) { throw new Error('skill-transaction-destination-outside-root') @@ -21,7 +22,11 @@ async function assertApprovedDestinationParent( lstat(dirname(marker.destinationPath)), realpath(dirname(marker.destinationPath)).then(normalizedSkillIdentityPath) ]) - if (parentStat.isSymbolicLink() || parentIdentity !== rootIdentity) { + if ( + parentStat.isSymbolicLink() || + parentIdentity !== rootIdentity || + skillPhysicalIdentity(dirname(marker.destinationPath), parentStat) !== rootPhysicalIdentity + ) { throw new Error('skill-transaction-destination-outside-root') } } @@ -30,19 +35,32 @@ export async function sweepOrphanedSkillTransactions( approvedSkillsRoot: string, ledger: SkillManagementLedger ): Promise { - const linkedRoot = await Promise.all([ + const roots = await Promise.all([ lstat(approvedSkillsRoot), - lstat(dirname(approvedSkillsRoot)) - ]) - .then((entries) => entries.some((entry) => entry.isSymbolicLink())) - .catch(() => true) - if (linkedRoot) { + lstat(dirname(approvedSkillsRoot)), + realpath(approvedSkillsRoot), + realpath(dirname(approvedSkillsRoot)) + ]).catch(() => null) + if ( + !roots || + roots[0].isSymbolicLink() || + roots[1].isSymbolicLink() || + normalizedSkillIdentityPath(dirname(roots[2])) !== normalizedSkillIdentityPath(roots[3]) + ) { return } - const rootIdentity = normalizedSkillIdentityPath(await realpath(approvedSkillsRoot)) + const [skillsRootStat, parentRootStat, resolvedSkillsRoot] = roots + const rootIdentity = normalizedSkillIdentityPath(resolvedSkillsRoot) + const rootPhysicalIdentity = skillPhysicalIdentity(approvedSkillsRoot, skillsRootStat) + // Why: mount changes invalidate the old workspace authority; sweeping both + // possible locations could mutate a directory Orca did not reserve now. const reservedRoots = [ - join(dirname(approvedSkillsRoot), RESERVED_SKILL_TRANSACTION_DIRECTORY), - join(approvedSkillsRoot, RESERVED_SKILL_TRANSACTION_DIRECTORY) + skillTransactionReservedRoot({ + skillsRoot: approvedSkillsRoot, + skillsRootDevice: skillsRootStat.dev, + parentRoot: dirname(approvedSkillsRoot), + parentRootDevice: parentRootStat.dev + }) ] for (const reservedRoot of reservedRoots) { const reservedRootSafe = await lstat(reservedRoot) @@ -64,7 +82,7 @@ export async function sweepOrphanedSkillTransactions( continue } try { - await assertApprovedDestinationParent(marker, rootIdentity) + await assertApprovedDestinationParent(marker, rootIdentity, rootPhysicalIdentity) } catch { continue } @@ -90,7 +108,8 @@ export async function sweepOrphanedSkillTransactions( await recoverTransactionToPrior({ transactionRoot, marker, - assertParentAuthority: () => assertApprovedDestinationParent(marker, rootIdentity), + assertParentAuthority: () => + assertApprovedDestinationParent(marker, rootIdentity, rootPhysicalIdentity), currentDisposition: committed ? 'committed' : 'restore' }) await rm(transactionRoot, { recursive: true, force: true }) diff --git a/src/main/skills/skill-package-publish.ts b/src/main/skills/skill-package-publish.ts index ae9ebcff557..2ffab77b818 100644 --- a/src/main/skills/skill-package-publish.ts +++ b/src/main/skills/skill-package-publish.ts @@ -1,4 +1,4 @@ -import { mkdir, rm, stat } from 'node:fs/promises' +import { mkdir, readFile, rm, stat } from 'node:fs/promises' import { dirname, join } from 'node:path' import type { ManagedSkillDestination, @@ -17,6 +17,7 @@ import { import { destinationUsesCrlf, replaceSkillPackage } from './skill-package-publication' import type { SkillRenameRuntime } from './skill-transaction-rename' import { createSkillTransactionContext } from './skill-package-transaction-context' +import { parseSkillTransactionMarker } from './skill-transaction-marker' import { assertManagedDestinationUnchanged, assertPublishedDestinationOwned @@ -58,6 +59,7 @@ export async function publishBundledSkill(args: { ) let retainTransaction = false let publicationStarted = false + let durableCommitCompleted = false try { await transaction.assertAuthority(true) await recoverMarkedSkillTransactions({ @@ -141,6 +143,7 @@ export async function publishBundledSkill(args: { marker: verifiedMarker }) await args.commit(destination) + durableCommitCompleted = true return destination } catch (error) { try { @@ -160,9 +163,27 @@ export async function publishBundledSkill(args: { .then(() => true) .catch(() => false) if (workspaceSafe && !retainTransaction) { - await rm(transaction.paths.transactionRoot, { recursive: true, force: true }).catch( - () => undefined + const cleanupMarker = await readFile( + join(transaction.paths.transactionRoot, 'transaction.json'), + 'utf8' ) + .then(parseSkillTransactionMarker) + .catch(() => null) + if (cleanupMarker?.transactionId === transaction.marker('staging').transactionId) { + try { + // Why: cleanup repeats recovery from the durable marker immediately + // before recursive removal, including after an ambiguous rollback. + await recoverTransactionToPrior({ + transactionRoot: transaction.paths.transactionRoot, + marker: cleanupMarker, + assertParentAuthority: transaction.assertParentAuthority, + currentDisposition: durableCommitCompleted ? 'committed' : 'restore' + }) + await rm(transaction.paths.transactionRoot, { recursive: true, force: true }) + } catch { + retainTransaction = true + } + } } if (workspaceSafe) { await releaseLock().catch(() => undefined) diff --git a/src/main/skills/skill-package-transaction-context.ts b/src/main/skills/skill-package-transaction-context.ts index 1024a67b0c4..0c707b84e5f 100644 --- a/src/main/skills/skill-package-transaction-context.ts +++ b/src/main/skills/skill-package-transaction-context.ts @@ -8,10 +8,8 @@ import type { import { assertManagedSkillParentTopology } from './skill-installation-topology' import type { PublicationCallbacks } from './skill-package-publication' import { assertManagedDestinationUnchanged } from './skill-published-destination-authority' -import { - writeSkillTransactionMarker, - type SkillTransactionMarker -} from './skill-transaction-marker' +import type { SkillTransactionMarker } from './skill-transaction-marker' +import { writeSkillTransactionMarker } from './skill-transaction-marker-write' import { assertTransactionOwnedState } from './skill-transaction-owned-state' import type { SkillPublicationState } from './skill-transaction-publication-state' import { diff --git a/src/main/skills/skill-transaction-marker-write.ts b/src/main/skills/skill-transaction-marker-write.ts new file mode 100644 index 00000000000..de8d5dce1f5 --- /dev/null +++ b/src/main/skills/skill-transaction-marker-write.ts @@ -0,0 +1,76 @@ +import { randomUUID } from 'node:crypto' +import { lstat, readFile, rename, unlink, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { skillPhysicalIdentity } from './skill-installation-topology' +import { + parseSkillTransactionMarker, + type SkillTransactionMarker +} from './skill-transaction-marker' + +export async function writeSkillTransactionMarker( + transactionRoot: string, + marker: SkillTransactionMarker, + beforeMutation: () => Promise = async () => undefined +): Promise { + const markerPath = join(transactionRoot, 'transaction.json') + const temporaryPath = join(transactionRoot, `.transaction-${randomUUID()}.tmp`) + const serialized = JSON.stringify(marker) + if (!parseSkillTransactionMarker(serialized)) { + throw new Error('skill-transaction-marker-invalid') + } + let temporaryPhysicalIdentity: string | null = null + const assertTemporaryOwned = async (): Promise => { + const entry = await lstat(temporaryPath) + if ( + !entry.isFile() || + entry.isSymbolicLink() || + skillPhysicalIdentity(temporaryPath, entry) !== temporaryPhysicalIdentity + ) { + throw new Error('skill-transaction-marker-changed') + } + } + const assertMarkerSlotOwned = async (): Promise => { + const entry = await lstat(markerPath).catch((error) => { + if (error && typeof error === 'object' && 'code' in error && error.code === 'ENOENT') { + return null + } + throw error + }) + if (!entry) { + return + } + const current = + entry.isFile() && !entry.isSymbolicLink() + ? await readFile(markerPath, 'utf8') + .then(parseSkillTransactionMarker) + .catch(() => null) + : null + if (current?.transactionId !== marker.transactionId) { + throw new Error('skill-transaction-marker-changed') + } + } + try { + await beforeMutation() + await writeFile(temporaryPath, serialized, { flag: 'wx' }) + temporaryPhysicalIdentity = skillPhysicalIdentity(temporaryPath, await lstat(temporaryPath)) + // Why: recovery must observe either the old or new phase, never a torn + // marker that could discard the only verified rollback package. + await beforeMutation() + await assertMarkerSlotOwned() + await assertTemporaryOwned() + await rename(temporaryPath, markerPath) + } finally { + if ( + temporaryPhysicalIdentity && + (await beforeMutation() + .then(() => true) + .catch(() => false)) + ) { + // Why: a redirected or independently replaced temporary is not + // transaction-owned cleanup, even though its randomized name matches. + await assertTemporaryOwned() + .then(() => unlink(temporaryPath)) + .catch(() => undefined) + } + } +} diff --git a/src/main/skills/skill-transaction-marker.ts b/src/main/skills/skill-transaction-marker.ts index 99caa7bb16f..754b722cbf3 100644 --- a/src/main/skills/skill-transaction-marker.ts +++ b/src/main/skills/skill-transaction-marker.ts @@ -1,6 +1,4 @@ -import { randomUUID } from 'node:crypto' -import { rename, rm, writeFile } from 'node:fs/promises' -import { basename, isAbsolute, join, posix } from 'node:path' +import { basename, isAbsolute, posix } from 'node:path' import { normalizeExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import type { SkillReleasedSnapshot } from '../../shared/skill-management' import { normalizedSkillIdentityPath } from './skill-installation-topology' @@ -259,33 +257,3 @@ export function parseSkillTransactionMarker(value: string): SkillTransactionMark return null } } - -export async function writeSkillTransactionMarker( - transactionRoot: string, - marker: SkillTransactionMarker, - beforeMutation: () => Promise = async () => undefined -): Promise { - const markerPath = join(transactionRoot, 'transaction.json') - const temporaryPath = join(transactionRoot, `.transaction-${randomUUID()}.tmp`) - const serialized = JSON.stringify(marker) - if (!parseSkillTransactionMarker(serialized)) { - throw new Error('skill-transaction-marker-invalid') - } - try { - await beforeMutation() - await writeFile(temporaryPath, serialized) - // Why: recovery must observe either the old or new phase, never a torn - // marker that could discard the only verified rollback package. - await beforeMutation() - await rename(temporaryPath, markerPath) - } finally { - // Why: a redirected transaction path is no longer Orca-owned cleanup. - if ( - await beforeMutation() - .then(() => true) - .catch(() => false) - ) { - await rm(temporaryPath, { force: true }).catch(() => undefined) - } - } -} diff --git a/src/main/skills/skill-transaction-recovery.test.ts b/src/main/skills/skill-transaction-recovery.test.ts index 00b4aadb2a1..4cdb6f45c01 100644 --- a/src/main/skills/skill-transaction-recovery.test.ts +++ b/src/main/skills/skill-transaction-recovery.test.ts @@ -24,9 +24,9 @@ import { import { sweepOrphanedSkillTransactions } from './skill-orphan-transaction-sweep' import { parseSkillTransactionMarker, - writeSkillTransactionMarker, type SkillTransactionMarker } from './skill-transaction-marker' +import { writeSkillTransactionMarker } from './skill-transaction-marker-write' import { emptySkillManagementLedger } from './skill-management-ledger' const temporaryRoots: string[] = [] @@ -547,7 +547,13 @@ describe('skill transaction recovery', () => { await mkdir(lockRoot) await writeFile( join(lockRoot, 'owner.json'), - JSON.stringify({ schemaVersion: 1, pid: 2_147_483_647 }) + JSON.stringify({ + schemaVersion: 1, + pid: 2_147_483_647, + lockId: '00000000-0000-4000-8000-000000000000', + createdAt: 1, + physicalIdentity: skillPhysicalIdentity(lockRoot, await stat(lockRoot)) + }) ) const release = await acquireSkillDestinationLock(lockRoot) @@ -555,6 +561,37 @@ describe('skill transaction recovery', () => { await release() }) + it('does not release a lock whose exact ownership marker changed', async () => { + const { root } = await fixture() + const lockRoot = join(root, 'lock') + const release = await acquireSkillDestinationLock(lockRoot) + const owner = JSON.parse(await readFile(join(lockRoot, 'owner.json'), 'utf8')) + await writeFile( + join(lockRoot, 'owner.json'), + JSON.stringify({ ...owner, lockId: '11111111-1111-4111-8111-111111111111' }) + ) + + await expect(release()).rejects.toThrow('skill-update-lock-changed') + expect(await lstat(lockRoot)).toBeTruthy() + }) + + it('removes its exact unmarked lock when authority fails during acquisition', async () => { + const { root } = await fixture() + const lockRoot = join(root, 'lock') + let authorityChecks = 0 + + await expect( + acquireSkillDestinationLock(lockRoot, async () => { + authorityChecks += 1 + if (authorityChecks === 2) { + throw new Error('skill-topology-changed') + } + }) + ).rejects.toThrow('skill-topology-changed') + + expect(await lstat(lockRoot).catch(() => null)).toBeNull() + }) + it('restores a self-contained orphan before inventory has loaded a ledger', async () => { const { root, live, record, snapshot } = await fixture() const reservedRoot = join(root, '.orca-skill-transactions') @@ -596,6 +633,17 @@ describe('skill transaction recovery', () => { await release() }) + it('does not sweep the reserved location for a different mount topology', async () => { + const { root, record, snapshot } = await fixture() + const transaction = join(root, 'skills', '.orca-skill-transactions', 'wrong-workspace') + await mkdir(transaction, { recursive: true }) + await writeSkillTransactionMarker(transaction, marker(record, snapshot, 'staging')) + + await sweepOrphanedSkillTransactions(join(root, 'skills'), emptySkillManagementLedger()) + + expect(await readFile(join(transaction, 'transaction.json'), 'utf8')).toBeTruthy() + }) + it('ignores a valid marker whose destination is outside the approved root', async () => { const { root, snapshot } = await fixture() const outside = join(root, 'outside', 'orca-cli') diff --git a/src/main/skills/skill-transaction-recovery.ts b/src/main/skills/skill-transaction-recovery.ts index a3c4e4f0199..879f780be49 100644 --- a/src/main/skills/skill-transaction-recovery.ts +++ b/src/main/skills/skill-transaction-recovery.ts @@ -1,9 +1,11 @@ -import { mkdir, readFile, readdir, rm, writeFile } from 'node:fs/promises' +import { randomUUID } from 'node:crypto' +import { lstat, mkdir, readFile, readdir, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import type { ManagedSkillDestination } from '../../shared/skill-management' import { assertManagedSkillParentTopology, - normalizedSkillIdentityPath + normalizedSkillIdentityPath, + skillPhysicalIdentity } from './skill-installation-topology' import { parseSkillTransactionMarker } from './skill-transaction-marker' import { recoverTransactionToPrior } from './skill-transaction-restore' @@ -59,43 +61,142 @@ function processIsAlive(pid: number): boolean { } } +type SkillDestinationLockOwner = { + schemaVersion: 1 + pid: number + lockId: string + createdAt: number + physicalIdentity: string +} + +function parseSkillDestinationLockOwner(value: unknown): SkillDestinationLockOwner | null { + if ( + !value || + typeof value !== 'object' || + !('schemaVersion' in value) || + value.schemaVersion !== 1 || + !('pid' in value) || + typeof value.pid !== 'number' || + !Number.isSafeInteger(value.pid) || + value.pid <= 0 || + !('lockId' in value) || + typeof value.lockId !== 'string' || + !/^[a-f0-9-]{36}$/.test(value.lockId) || + !('createdAt' in value) || + typeof value.createdAt !== 'number' || + !Number.isFinite(value.createdAt) || + !('physicalIdentity' in value) || + typeof value.physicalIdentity !== 'string' || + value.physicalIdentity.length === 0 + ) { + return null + } + return value as SkillDestinationLockOwner +} + +async function inspectSkillDestinationLock(lockRoot: string): Promise { + const entry = await lstat(lockRoot).catch(() => null) + if (!entry?.isDirectory() || entry.isSymbolicLink()) { + throw new Error('skill-update-busy') + } + const owner = await readFile(join(lockRoot, 'owner.json'), 'utf8') + .then((value) => parseSkillDestinationLockOwner(JSON.parse(value))) + .catch(() => null) + if (!owner || skillPhysicalIdentity(lockRoot, entry) !== owner.physicalIdentity) { + throw new Error('skill-update-busy') + } + return owner +} + +async function assertUnpublishedSkillDestinationLock( + lockRoot: string, + physicalIdentity: string +): Promise { + const [entry, children] = await Promise.all([ + lstat(lockRoot), + readdir(lockRoot, { withFileTypes: true }) + ]) + if ( + !entry.isDirectory() || + entry.isSymbolicLink() || + skillPhysicalIdentity(lockRoot, entry) !== physicalIdentity || + children.some( + (child) => child.name !== 'owner.json' || !child.isFile() || child.isSymbolicLink() + ) + ) { + throw new Error('skill-update-lock-changed') + } +} + +function sameSkillDestinationLockOwner( + left: SkillDestinationLockOwner, + right: SkillDestinationLockOwner +): boolean { + return ( + left.pid === right.pid && + left.lockId === right.lockId && + left.createdAt === right.createdAt && + left.physicalIdentity === right.physicalIdentity + ) +} + export async function acquireSkillDestinationLock( lockRoot: string, beforeMutation: () => Promise = async () => undefined ): Promise<() => Promise> { - const create = async (): Promise => { + const create = async (): Promise => { await beforeMutation() await mkdir(lockRoot) - await beforeMutation() - await writeFile( - join(lockRoot, 'owner.json'), - JSON.stringify({ schemaVersion: 1, pid: process.pid, createdAt: Date.now() }) - ) + const entry = await lstat(lockRoot) + const owner: SkillDestinationLockOwner = { + schemaVersion: 1, + pid: process.pid, + lockId: randomUUID(), + createdAt: Date.now(), + physicalIdentity: skillPhysicalIdentity(lockRoot, entry) + } + try { + await beforeMutation() + if (skillPhysicalIdentity(lockRoot, await lstat(lockRoot)) !== owner.physicalIdentity) { + throw new Error('skill-update-busy') + } + await writeFile(join(lockRoot, 'owner.json'), JSON.stringify(owner), { flag: 'wx' }) + return owner + } catch (error) { + // Why: a failed authority check or partial owner write must not leave an + // unmarked lock that permanently wedges an otherwise retryable update. + await assertUnpublishedSkillDestinationLock(lockRoot, owner.physicalIdentity) + .then(() => rm(lockRoot, { recursive: true })) + .catch(() => undefined) + throw error + } } + let acquired: SkillDestinationLockOwner try { - await create() + acquired = await create() } catch (error) { if (!error || typeof error !== 'object' || !('code' in error) || error.code !== 'EEXIST') { throw error } - const owner = await readFile(join(lockRoot, 'owner.json'), 'utf8') - .then((value) => JSON.parse(value) as unknown) - .catch(() => null) - if ( - typeof owner !== 'object' || - owner === null || - !('schemaVersion' in owner) || - owner.schemaVersion !== 1 || - !('pid' in owner) || - typeof owner.pid !== 'number' || - processIsAlive(owner.pid) - ) { + const owner = await inspectSkillDestinationLock(lockRoot) + if (processIsAlive(owner.pid)) { throw new Error('skill-update-busy') } - // Why: only an Orca-marked lock whose owner is gone is safe to reclaim. await beforeMutation() + const currentOwner = await inspectSkillDestinationLock(lockRoot) + // Why: a second Orca may reclaim and replace the stale lock while this + // process proves destination authority; never remove its new lock. + if (!sameSkillDestinationLockOwner(owner, currentOwner)) { + throw new Error('skill-update-busy') + } + await rm(lockRoot, { recursive: true }) + acquired = await create() + } + return async () => { + const currentOwner = await inspectSkillDestinationLock(lockRoot) + if (!sameSkillDestinationLockOwner(acquired, currentOwner)) { + throw new Error('skill-update-lock-changed') + } await rm(lockRoot, { recursive: true }) - await create() } - return () => rm(lockRoot, { recursive: true, force: true }) } diff --git a/src/main/skills/skill-transaction-restore.ts b/src/main/skills/skill-transaction-restore.ts index 3f4522f6199..6e8eb732790 100644 --- a/src/main/skills/skill-transaction-restore.ts +++ b/src/main/skills/skill-transaction-restore.ts @@ -8,10 +8,8 @@ import { clearSkillPublicationTemporary, rollbackSkillPackageInPlace } from './skill-package-rollback' -import { - writeSkillTransactionMarker, - type SkillTransactionMarker -} from './skill-transaction-marker' +import type { SkillTransactionMarker } from './skill-transaction-marker' +import { writeSkillTransactionMarker } from './skill-transaction-marker-write' import { assertTransactionOwnedState } from './skill-transaction-owned-state' import { assertSkillTransactionPackageBuild } from './skill-transaction-package-build' import { diff --git a/src/renderer/src/components/settings/ManagedOrcaSkillsSection.tsx b/src/renderer/src/components/settings/ManagedOrcaSkillsSection.tsx index c4ccc195bc4..93837d5ae93 100644 --- a/src/renderer/src/components/settings/ManagedOrcaSkillsSection.tsx +++ b/src/renderer/src/components/settings/ManagedOrcaSkillsSection.tsx @@ -19,7 +19,7 @@ import { DialogTitle } from '../ui/dialog' import { SettingsBadge, SettingsSubsectionHeader } from './SettingsFormControls' -import { managedSkillStatusCopy } from './managed-skill-status-copy' +import { managedSkillDiagnosticCopy, managedSkillStatusCopy } from './managed-skill-status-copy' function statusTone( status: SkillManagementStatus @@ -41,14 +41,6 @@ function statusTone( } } -function diagnosticCopy(installation: SkillManagementInstallation): string { - const revision = installation.installedReleaseRevision - ? `r${installation.installedReleaseRevision}` - : translate('auto.components.settings.ManagedOrcaSkills.unverified', 'unverified') - const digest = installation.installedPackageDigest?.slice(0, 8) ?? '—' - return `${revision} · Orca ${installation.installedAppVersion ?? installation.currentAppVersion} · ${digest}` -} - function missingVisibilityCopy(installation: SkillManagementInstallation): string | null { if (!installation.managed) { return null @@ -210,7 +202,7 @@ export function ManagedOrcaSkillsSection(): React.JSX.Element {

- {diagnosticCopy(installation)} + {managedSkillDiagnosticCopy(installation)}

{installation.description ? (

{installation.description}

diff --git a/src/renderer/src/components/settings/managed-skill-status-copy.test.ts b/src/renderer/src/components/settings/managed-skill-status-copy.test.ts new file mode 100644 index 00000000000..ee2d2ffd219 --- /dev/null +++ b/src/renderer/src/components/settings/managed-skill-status-copy.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest' +import { managedSkillDiagnosticCopy } from './managed-skill-status-copy' + +describe('managed skill diagnostic copy', () => { + it('does not attribute unknown bytes to the current Orca release', () => { + expect( + managedSkillDiagnosticCopy({ + installedReleaseRevision: null, + installedAppVersion: null, + installedPackageDigest: 'abcdef0123456789' + }) + ).toBe('unverified · abcdef01') + }) + + it('shows release provenance only for a mapped snapshot', () => { + expect( + managedSkillDiagnosticCopy({ + installedReleaseRevision: 7, + installedAppVersion: '1.2.3', + installedPackageDigest: 'abcdef0123456789' + }) + ).toBe('r7 · Orca 1.2.3 · abcdef01') + }) +}) diff --git a/src/renderer/src/components/settings/managed-skill-status-copy.ts b/src/renderer/src/components/settings/managed-skill-status-copy.ts index 2343ea1102f..7c7a111bb78 100644 --- a/src/renderer/src/components/settings/managed-skill-status-copy.ts +++ b/src/renderer/src/components/settings/managed-skill-status-copy.ts @@ -1,4 +1,7 @@ -import type { SkillManagementStatus } from '../../../../shared/skill-management' +import type { + SkillManagementInstallation, + SkillManagementStatus +} from '../../../../shared/skill-management' import { translate } from '@/i18n/i18n' export function managedSkillStatusCopy(status: SkillManagementStatus): string { @@ -34,3 +37,18 @@ export function managedSkillStatusCopy(status: SkillManagementStatus): string { return translate('auto.components.settings.ManagedOrcaSkills.failed', 'Update failed') } } + +export function managedSkillDiagnosticCopy( + installation: Pick< + SkillManagementInstallation, + 'installedReleaseRevision' | 'installedAppVersion' | 'installedPackageDigest' + > +): string { + const digest = installation.installedPackageDigest?.slice(0, 8) ?? '—' + if (installation.installedReleaseRevision === null || installation.installedAppVersion === null) { + // Why: the current app version says nothing about which release produced + // unknown bytes, so diagnostic copy must not invent provenance. + return `${translate('auto.components.settings.ManagedOrcaSkills.unverified', 'unverified')} · ${digest}` + } + return `r${installation.installedReleaseRevision} · Orca ${installation.installedAppVersion} · ${digest}` +}