From dcfc5d403ac997c2b08bee4753e57a1a4a8a8538 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 12 Oct 2026 05:56:26 +0800 Subject: [PATCH] fix(setup): retain regular-file cleanup ownership through workflows Thread the existing resource scope through skill and client configuration reads. Keep failed-close handles owned, wait for admitted reads, and surface unresolved cleanup alongside doctor, uninstall and update results. Validated focused setup/doctor/update and filesystem boundary checks, temporary-file hygiene, packaged setup/update/uninstall workflows, and static checks. No external provider installation was performed. --- src/application/ClientConfigurationFiles.ts | 19 +- src/application/ClientRegistrationStatus.ts | 14 +- src/application/Doctor.ts | 79 +++++- src/application/GeminiClientSettings.ts | 14 +- src/application/RegularFileRead.ts | 26 +- src/application/SetupHost.ts | 29 ++- src/application/SetupSkill.ts | 84 +++++-- src/application/Uninstall.ts | 111 +++++--- src/application/Update.test.ts | 18 ++ src/application/Update.ts | 39 ++- src/application/UpdateFailure.ts | 1 + src/application/UpdateMaintenance.ts | 23 +- src/application/UpdateRuntime.ts | 145 ++++++----- src/cli/setupCommands.ts | 11 +- .../setupSkillCleanupOwnership.test.ts | 237 ++++++++++++++++++ 15 files changed, 707 insertions(+), 143 deletions(-) create mode 100644 tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts diff --git a/src/application/ClientConfigurationFiles.ts b/src/application/ClientConfigurationFiles.ts index 5a665b770..fc3c2d4ea 100644 --- a/src/application/ClientConfigurationFiles.ts +++ b/src/application/ClientConfigurationFiles.ts @@ -10,6 +10,7 @@ import { } from "./ClientConfigurationDocument.js"; import type { SetupClient } from "./SupportedClients.js"; import { clientRegistrationEnvironment } from "./ClientRegistrationEnvironment.js"; +import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; /** A named source failure that setup can explain without losing the bad path. */ export interface ClientConfigurationFileError { @@ -22,9 +23,15 @@ export interface ClientConfigurationFileError { /** Preserve named read failures and distinguish a missing source from an empty file. */ export const readConfigurationText = async ( path: string, + resources?: ArtifactResourceScope, ): Promise> => { try { - return ok(await readRegularFileText(path)); + return ok( + await readRegularFileText( + path, + resources === undefined ? {} : { resources }, + ), + ); } catch (cause: unknown) { if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") return ok(undefined); @@ -43,10 +50,11 @@ export const readConfigurationText = async ( export const readConfigurationFile = async ( path: string, format: SetupClient["format"], + resources?: ArtifactResourceScope, ): Promise< Result > => { - const source = await readConfigurationText(path); + const source = await readConfigurationText(path, resources); if (!source.ok) return source; const content = source.value; if (content === undefined) return ok(undefined); @@ -64,12 +72,17 @@ export const readConfigurationFile = async ( /** Read every applicable user file; malformed lower-priority files also stop setup. */ export const readClientConfigurationFiles = async ( client: SetupClient, + resources?: ArtifactResourceScope, ): Promise< Result > => { const documents: ClientConfigurationDocument[] = []; for (const path of client.configPaths ?? [client.configPath]) { - const document = await readConfigurationFile(path, client.format); + const document = await readConfigurationFile( + path, + client.format, + resources, + ); if (!document.ok) return document; if (document.value !== undefined) { const environment = clientRegistrationEnvironment(document.value); diff --git a/src/application/ClientRegistrationStatus.ts b/src/application/ClientRegistrationStatus.ts index b40079c45..3b649c132 100644 --- a/src/application/ClientRegistrationStatus.ts +++ b/src/application/ClientRegistrationStatus.ts @@ -26,6 +26,7 @@ import { } from "./SupportedClients.js"; import type { SetupClient } from "./SupportedClients.js"; import { compareUnicodeCodePoints } from "../domain/unicodeCodePointOrder.js"; +import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; interface ClientRegistrationStatusBase { readonly client: string; @@ -78,6 +79,7 @@ export const readClientRegistrationStatuses = async ( options: { readonly platform?: NodeJS.Platform; readonly environment?: NodeJS.ProcessEnv; + readonly resources?: ArtifactResourceScope; } = {}, ): Promise => { const statuses: ClientRegistrationStatus[] = []; @@ -110,7 +112,10 @@ export const readClientRegistrationStatuses = async ( continue; } try { - const files = await readClientConfigurationFiles(client); + const files = await readClientConfigurationFiles( + client, + options.resources, + ); if (!files.ok) { statuses.push({ client: client.name, @@ -128,7 +133,12 @@ export const readClientRegistrationStatuses = async ( ); continue; } - const policy = await readClientPolicyBlock(client, parsed); + const policy = await readClientPolicyBlock( + client, + parsed, + undefined, + options.resources, + ); if (!policy.ok) { statuses.push({ client: client.name, diff --git a/src/application/Doctor.ts b/src/application/Doctor.ts index 0f4247421..5ffc9f340 100644 --- a/src/application/Doctor.ts +++ b/src/application/Doctor.ts @@ -1,5 +1,6 @@ import { constants } from "node:fs"; import { access, readdir, stat } from "node:fs/promises"; +import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { homeDirectoryFromEnvironment } from "../config/homeDirectory.js"; import { join } from "node:path"; @@ -100,6 +101,7 @@ export interface DoctorHost { installedSkillIdentity?( clientIds: readonly string[], ): Promise; + close?(): Promise; clientRegistrations?(): Promise; ilspyCmdVersion?(path: string): Promise; } @@ -155,7 +157,41 @@ interface DoctorIdentity { */ export const runDoctor = async ( target?: string, - host: DoctorHost = systemDoctorHost(), + selectedHost?: DoctorHost, + requestedScope?: DoctorScope, +): Promise => + selectedHost === undefined + ? runDoctorOwned(target, systemDoctorHost(), requestedScope) + : inspectDoctor(target, selectedHost, requestedScope); + +/** Inspect with a host whose lifetime belongs to this operation. */ +export const runDoctorOwned = async ( + target: string | undefined, + host: DoctorHost, + requestedScope?: DoctorScope, +): Promise => { + let report: DoctorReport; + try { + report = await inspectDoctor(target, host, requestedScope); + } catch (cause: unknown) { + const cleanup = await closeDoctorHost(host); + if (cleanup !== undefined) + throw new AggregateError( + [cause, new Error(cleanup)], + "Doctor failed and its resource cleanup remains incomplete.", + { cause }, + ); + throw cause; + } + const cleanup = await closeDoctorHost(host); + return cleanup === undefined + ? report + : doctorReportWithCleanup(report, cleanup); +}; + +const inspectDoctor = async ( + target: string | undefined, + host: DoctorHost, requestedScope?: DoctorScope, ): Promise => { const scope = normalizeDoctorScope(requestedScope, target); @@ -197,6 +233,36 @@ export const runDoctor = async ( }; }; +const closeDoctorHost = async ( + host: DoctorHost, +): Promise => { + try { + return await host.close?.(); + } catch (cause: unknown) { + return `Doctor host cleanup failed: ${cause instanceof Error ? cause.message : String(cause)}`; + } +}; + +const doctorReportWithCleanup = ( + report: DoctorReport, + detail: string, +): DoctorReport => { + const check: DoctorCheck = { + name: "doctor-resource-cleanup", + ok: false, + classification: "config_drift", + detail, + remediation: "Resolve the reported cleanup failure, then rerun rea doctor.", + }; + return { + ...report, + healthy: false, + environment_healthy: false, + checks: [...report.checks, check], + scope_checks: [...report.scope_checks, check], + }; +}; + const collectDoctorIdentity = async ( host: DoctorHost, scope: DoctorScopeReport, @@ -307,6 +373,7 @@ export interface SystemDoctorHostOptions { export const systemDoctorHost = ( options: SystemDoctorHostOptions = {}, ): DoctorHost => { + const resources = new ArtifactResourceScope(); const platform = options.platform ?? process.platform; const architecture = options.architecture ?? process.arch; const environment = options.environment ?? process.env; @@ -446,11 +513,21 @@ export const systemDoctorHost = ( clientIds, environment, platform, + resources, ), + close: async () => { + try { + await resources.close(); + return undefined; + } catch (cause: unknown) { + return `Doctor resource cleanup failed: ${cause instanceof Error ? cause.message : String(cause)}`; + } + }, clientRegistrations: () => readClientRegistrationStatuses(homeDirectory, undefined, { platform, environment, + resources, }), }; }; diff --git a/src/application/GeminiClientSettings.ts b/src/application/GeminiClientSettings.ts index eecb33b53..440ccf9d6 100644 --- a/src/application/GeminiClientSettings.ts +++ b/src/application/GeminiClientSettings.ts @@ -4,6 +4,7 @@ import { stripComments } from "jsonc-parser"; import { z } from "zod"; import { err, ok, type Result } from "../domain/result.js"; import { PRODUCT_IDENTITY } from "../identity.js"; +import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { npxRegistrationCommand } from "./ClientRegistrationIdentity.js"; import { clientConfigurationValuesEqual, @@ -61,6 +62,7 @@ const normalizedPath = async ( const workspaceTrusted = async ( settings: NonNullable, sources: readonly SettingsSource[], + resources?: ArtifactResourceScope, ): Promise> => { let enabled = true; for (const { path, document } of sources) { @@ -70,7 +72,10 @@ const workspaceTrusted = async ( } if (settings.trustOverride !== undefined) return ok(settings.trustOverride); if (!enabled) return ok(true); - const text = await readConfigurationText(settings.trustedFoldersPath); + const text = await readConfigurationText( + settings.trustedFoldersPath, + resources, + ); if (!text.ok) return text; if (text.value === undefined) return ok(false); let rules: z.output; @@ -110,6 +115,7 @@ export const readClientPolicyBlock = async ( client: SetupClient, userDocument?: ClientConfigurationDocument, desiredRegistration?: unknown, + resources?: ArtifactResourceScope, ): Promise> => { if (client.name !== "gemini_cli" || client.geminiSettings === undefined) return ok(undefined); @@ -124,7 +130,7 @@ export const readClientPolicyBlock = async ( const parsed = path === client.configPath && userDocument !== undefined ? ok(userDocument) - : await readConfigurationFile(path, "json"); + : await readConfigurationFile(path, "json", resources); if (!parsed.ok) return parsed; if (parsed.value !== undefined) sources.push({ path, document: parsed.value }); @@ -143,10 +149,10 @@ export const readClientPolicyBlock = async ( ".gemini", "settings.json", ); - const document = await readConfigurationFile(path, "json"); + const document = await readConfigurationFile(path, "json", resources); if (!document.ok) return document; if (document.value !== undefined) { - const trusted = await workspaceTrusted(settings, sources); + const trusted = await workspaceTrusted(settings, sources, resources); if (!trusted.ok) return trusted; if (trusted.value) { const source = { path, document: document.value }; diff --git a/src/application/RegularFileRead.ts b/src/application/RegularFileRead.ts index 1c3852326..9183c9f8a 100644 --- a/src/application/RegularFileRead.ts +++ b/src/application/RegularFileRead.ts @@ -16,6 +16,7 @@ import type { AnalysisCleanupObservation } from "../domain/analysisErrorBase.js" export interface RegularFileReadOptions { readonly signal?: AbortSignal | undefined; readonly symlinks?: "follow" | "reject"; + readonly resources?: ArtifactResourceScope; } export type RegularFileReadOutcome = @@ -68,6 +69,15 @@ export const withRegularFile = async ( path: string, read: (handle: FileHandle, stats: Stats) => Promise, options: RegularFileReadOptions = {}, +): Promise => + options.resources === undefined + ? withRegularFileOwned(path, read, options) + : options.resources.run(() => withRegularFileOwned(path, read, options)); + +const withRegularFileOwned = async ( + path: string, + read: (handle: FileHandle, stats: Stats) => Promise, + options: RegularFileReadOptions, ): Promise => { const { signal } = options; let handle: FileHandle; @@ -78,12 +88,15 @@ export const withRegularFile = async ( }); } catch (cause: unknown) { if (!(cause instanceof RegularFileAdmissionFailure)) throw cause; - throw new RegularFileCleanupFailure( + const failure = new RegularFileCleanupFailure( path, cause.owner, { kind: "failed", cause: cause.cause }, cause.cleanupCause, ); + if (options.resources === undefined) throw failure; + await retryRegularFileCleanup(failure, options.resources); + throw cause.cause; } const owner = new OwnedFileHandle(handle); let outcome: RegularFileReadOutcome; @@ -108,7 +121,16 @@ export const withRegularFile = async ( try { await owner.close(); } catch (cleanupCause: unknown) { - throw new RegularFileCleanupFailure(path, owner, outcome, cleanupCause); + const failure = new RegularFileCleanupFailure( + path, + owner, + outcome, + cleanupCause, + ); + if (options.resources === undefined) throw failure; + const cleanup = await retryRegularFileCleanup(failure, options.resources); + if (cleanup.outcome.kind === "failed") throw cleanup.outcome.cause; + return cleanup.outcome.value; } if (outcome.kind === "failed") throw outcome.cause; return outcome.value; diff --git a/src/application/SetupHost.ts b/src/application/SetupHost.ts index 9f44b8d63..9e2ae95f1 100644 --- a/src/application/SetupHost.ts +++ b/src/application/SetupHost.ts @@ -122,7 +122,7 @@ export const systemSetupHost = ( nodeVersion: process.versions.node, macosVersion: () => doctorHost.macosVersion(), linuxDistribution: readLinuxDistribution, - close: () => closeSetupResources(resources, macHopperHost), + close: () => closeSetupResources(resources, macHopperHost, doctorHost), initialSetupState: async ( scope?: DoctorScope, ): Promise => { @@ -181,9 +181,17 @@ export const systemSetupHost = ( clientIds, environment, platform, + resources, ), installSkill: (clientIds) => - installCanonicalSkill(homeDirectory, clientIds, environment, platform), + installCanonicalSkill( + homeDirectory, + clientIds, + environment, + platform, + undefined, + resources, + ), doctor: (scope) => runDoctor(undefined, doctorHost, scope), }; }; @@ -226,11 +234,14 @@ const clientConfigurationOperations = (resources: ArtifactResourceScope) => ({ const closeSetupResources = async ( resources: ArtifactResourceScope, macHopperHost: ReturnType | undefined, + doctorHost: DoctorHost, ): Promise => { - const [resourceCleanup, hopperCleanup] = await Promise.allSettled([ - resources.close(), - Promise.resolve().then(() => macHopperHost?.close()), - ]); + const [resourceCleanup, hopperCleanup, doctorCleanup] = + await Promise.allSettled([ + resources.close(), + Promise.resolve().then(() => macHopperHost?.close()), + Promise.resolve().then(() => doctorHost.close?.()), + ]); const failures: string[] = []; if (hopperCleanup.status === "rejected") failures.push( @@ -238,6 +249,12 @@ const closeSetupResources = async ( ); else if (hopperCleanup.value !== undefined) failures.push(hopperCleanup.value); + if (doctorCleanup.status === "rejected") + failures.push( + `Doctor host cleanup failed: ${errorMessage(doctorCleanup.reason)}`, + ); + else if (doctorCleanup.value !== undefined) + failures.push(doctorCleanup.value); if (resourceCleanup.status === "rejected") failures.push(errorMessage(resourceCleanup.reason)); return failures.length === 0 ? undefined : failures.join("; "); diff --git a/src/application/SetupSkill.ts b/src/application/SetupSkill.ts index 6bbcdd0c7..83ba96c5a 100644 --- a/src/application/SetupSkill.ts +++ b/src/application/SetupSkill.ts @@ -4,13 +4,18 @@ import { dirname, join } from "node:path"; import writeFileAtomic from "write-file-atomic"; import { err, ok, type Result } from "../domain/result.js"; import { readBoundedFileBytes } from "../process/BoundedFileBytes.js"; +import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; +import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; import { PRODUCT_IDENTITY } from "../identity.js"; import { NonRegularFileReadError, RegularFileChangedError, } from "../filesystem/RegularFile.js"; -import { withRegularFile } from "./RegularFileRead.js"; +import { + RegularFileCleanupFailure, + withRegularFile, +} from "./RegularFileRead.js"; import { clientSkillDirectories } from "./SupportedClients.js"; export const MANAGED_SKILL_FILES = [ @@ -27,15 +32,21 @@ export const isManagedSkillManifest = (content: string): boolean => content.startsWith(`---\nname: ${PRODUCT_IDENTITY.skillName}\n`) || content.startsWith(`---\r\nname: ${PRODUCT_IDENTITY.skillName}\r\n`); -export const readSkillFile = (path: string): Promise => - readSkillFileState(path).then(({ content }) => content); +export const readSkillFile = ( + path: string, + resources?: ArtifactResourceScope, +): Promise => + readSkillFileState(path, resources).then(({ content }) => content); interface SkillFileState { readonly content: string; readonly mode: number; } -const readSkillFileState = async (path: string): Promise => +const readSkillFileState = async ( + path: string, + resources?: ArtifactResourceScope, +): Promise => withRegularFile( path, async (handle, stats) => { @@ -49,7 +60,10 @@ const readSkillFileState = async (path: string): Promise => throw new RegularFileChangedError(path); return { content: bytes.toString("utf8"), mode: stats.mode & 0o7777 }; }, - { symlinks: "reject" }, + { + symlinks: "reject", + ...(resources === undefined ? {} : { resources }), + }, ); interface CanonicalSkillFile { @@ -173,6 +187,7 @@ export const existingSkillDestinations = async ( home: string, environment: Readonly = {}, platform: NodeJS.Platform = process.platform, + resources?: ArtifactResourceScope, ): Promise => { const existing: SkillDestination[] = []; for (const destination of skillDestinations( @@ -185,11 +200,16 @@ export const existingSkillDestinations = async ( const root = await lstat(destination.path); if (root.isSymbolicLink() || !root.isDirectory()) continue; const manifest = join(destination.path, "SKILL.md"); - const content = await readSkillFile(manifest); + const content = await readSkillFile(manifest, resources); if (isManagedSkillManifest(content)) existing.push(destination); } catch (cause: unknown) { if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") continue; + if ( + cause instanceof RegularFileCleanupFailure || + cause instanceof ArtifactReaderFailure + ) + throw cause; if (cause instanceof NonRegularFileReadError) continue; throw cause; } @@ -199,9 +219,10 @@ export const existingSkillDestinations = async ( const readOptionalSkillState = async ( path: string, + resources?: ArtifactResourceScope, ): Promise => { try { - return await readSkillFileState(path); + return await readSkillFileState(path, resources); } catch (cause: unknown) { if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") return undefined; @@ -214,6 +235,7 @@ const canonicalSkillFiles = async ( clientIds: readonly string[], environment: Readonly = {}, platform: NodeJS.Platform = process.platform, + resources?: ArtifactResourceScope, ): Promise => Promise.all( skillDestinations(home, clientIds, environment, platform).flatMap( @@ -229,7 +251,7 @@ const canonicalSkillFiles = async ( ), "utf8", ), - original: await readOptionalSkillState(destination), + original: await readOptionalSkillState(destination, resources), }; }), ), @@ -241,12 +263,24 @@ export const canonicalSkillNeedsInstall = async ( clientIds: readonly string[] = [], environment: Readonly = {}, platform: NodeJS.Platform = process.platform, + resources?: ArtifactResourceScope, ): Promise => { try { return ( - await canonicalSkillFiles(home, clientIds, environment, platform) + await canonicalSkillFiles( + home, + clientIds, + environment, + platform, + resources, + ) ).some(({ content, original }) => original?.content !== content); } catch (cause: unknown) { + if ( + cause instanceof RegularFileCleanupFailure || + cause instanceof ArtifactReaderFailure + ) + throw cause; // Unreadable skill state fails open to install so setup can repair it. void cause; return true; @@ -277,7 +311,6 @@ const restoreSkillFiles = async ( return failures; }; -/** Transactionally install or upgrade the canonical REA skill and references. */ /** Transactionally install the canonical skill bundle and optional references. */ export const installCanonicalSkill = async ( home: string, @@ -285,6 +318,7 @@ export const installCanonicalSkill = async ( environment: Readonly = {}, platform: NodeJS.Platform = process.platform, operations: SkillInstallFileOperations = systemSkillInstallFileOperations, + resources?: ArtifactResourceScope, ): Promise => { let attempted: CanonicalSkillFile[] = []; let stage: "prepare" | "backup" | "write" | "readback" = "prepare"; @@ -295,6 +329,7 @@ export const installCanonicalSkill = async ( clientIds, environment, platform, + resources, ); const changed = canonical.filter( ({ content, original }) => original?.content !== content, @@ -323,7 +358,7 @@ export const installCanonicalSkill = async ( stage = "readback"; for (const { destination, content } of changed) { failingPath = destination; - if ((await readSkillFile(destination)) !== content) + if ((await readSkillFile(destination, resources)) !== content) throw new Error(`skill readback mismatch: ${destination}`); } return ok("installed"); @@ -351,6 +386,7 @@ export const readInstalledSkillIdentity = async ( clientIds: readonly string[], environment: Readonly = {}, platform: NodeJS.Platform = process.platform, + resources?: ArtifactResourceScope, ): Promise => { const destinations = skillDestinations( home, @@ -361,22 +397,38 @@ export const readInstalledSkillIdentity = async ( let content: string | undefined; for (const destination of destinations) { try { - content = await readSkillFile(join(destination.path, "SKILL.md")); + content = await readSkillFile( + join(destination.path, "SKILL.md"), + resources, + ); break; - } catch { + } catch (cause: unknown) { + if ( + cause instanceof RegularFileCleanupFailure || + cause instanceof ArtifactReaderFailure + ) + throw cause; // Another selected copy may provide metadata; canonical validation below // still marks missing or unreadable copies as stale. } } if (content === undefined) return undefined; const toolCount = /^\s{2}tool_count:\s*(\d+)\s*$/mu.exec(content)?.[1]; - return { - canonical: !(await canonicalSkillNeedsInstall( + let canonical: boolean; + try { + canonical = !(await canonicalSkillNeedsInstall( home, clientIds, environment, platform, - )), + resources, + )); + } catch (cause: unknown) { + if (!(cause instanceof ArtifactReaderFailure)) throw cause; + canonical = false; + } + return { + canonical, version: /^\s{2}version:\s*"([^"]+)"\s*$/mu.exec(content)?.[1] ?? null, toolCount: toolCount === undefined ? null : Number.parseInt(toolCount, 10), }; diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index d363f950d..abcb61ac3 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -24,6 +24,7 @@ import { import { isOwnedClientRegistrationCommand } from "./ClientRegistrationIdentity.js"; import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; import { readRegularFileText } from "./RegularFileRead.js"; +import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { manualRegistrationRemediation, supportedClients, @@ -50,9 +51,11 @@ export interface UninstallFileSystem { remove(path: string, recursive?: boolean): Promise; } -const systemFileSystem: UninstallFileSystem = { - readText: (path) => readRegularFileText(path), - readRegularText: readSkillFile, +const systemFileSystem = ( + resources: ArtifactResourceScope, +): UninstallFileSystem => ({ + readText: (path) => readRegularFileText(path, { resources }), + readRegularText: (path) => readSkillFile(path, resources), copy: (source, destination) => copyFile(source, destination, fsConstants.COPYFILE_EXCL), writeText: (path, contents) => @@ -60,7 +63,7 @@ const systemFileSystem: UninstallFileSystem = { stat: (path) => lstat(path), realpath: (path) => realpath(path), remove: (path, recursive = true) => rm(path, { recursive }), -}; +}); /** One explicitly classified uninstall action. */ export interface UninstallItem { @@ -83,6 +86,7 @@ export interface UninstallHost { removeClient(client: SetupClient): Promise; removeSkill(): Promise; purgeData(): Promise; + close?(): Promise; } /** @@ -95,35 +99,66 @@ export const runUninstall = async ( purgeData: boolean, host: UninstallHost = systemUninstallHost(), ): Promise => { - const clients = await host.clients(); - const blocked: UninstallItem[] = []; - for (const client of clients) { - const blocking = await host.inspectClient(client); - if (blocking !== undefined) blocked.push(blocking); + let result: UninstallResult | undefined; + let primaryFailure: unknown; + let failed = false; + try { + const clients = await host.clients(); + const blocked: UninstallItem[] = []; + for (const client of clients) { + const blocking = await host.inspectClient(client); + if (blocking !== undefined) blocked.push(blocking); + } + const items: UninstallItem[] = + blocked.length > 0 + ? [ + ...blocked, + item( + "uninstall", + "skipped", + "No REA registration, skill, or data was removed. Repair each failed configuration, then rerun uninstall.", + ), + ] + : await removeAll(clients, purgeData, host); + items.push({ + name: "analysis_engine", + status: "retained", + detail: "Hopper is not owned by REA uninstall.", + }); + const reported = collapseUninstallItems(items); + result = { + status: reported.some(({ status }) => status === "failed") + ? "failed" + : "complete", + items: reported, + }; + } catch (cause: unknown) { + failed = true; + primaryFailure = cause; } - const items: UninstallItem[] = - blocked.length > 0 - ? [ - ...blocked, - item( - "uninstall", - "skipped", - "No REA registration, skill, or data was removed. Repair each failed configuration, then rerun uninstall.", - ), - ] - : await removeAll(clients, purgeData, host); - items.push({ - name: "analysis_engine", - status: "retained", - detail: "Hopper is not owned by REA uninstall.", - }); - const reported = collapseUninstallItems(items); - return { - status: reported.some(({ status }) => status === "failed") - ? "failed" - : "complete", - items: reported, - }; + let cleanupFailure: string | undefined; + try { + cleanupFailure = await host.close?.(); + } catch (cause: unknown) { + cleanupFailure = `Uninstall resource cleanup failed: ${cause instanceof Error ? cause.message : String(cause)}`; + } + if (failed) { + if (cleanupFailure !== undefined) + throw new AggregateError( + [primaryFailure, new Error(cleanupFailure)], + "Uninstall failed and its resource cleanup remains incomplete.", + { cause: primaryFailure }, + ); + throw primaryFailure; + } + if (result === undefined) + throw new Error("Uninstall result was not produced."); + if (cleanupFailure === undefined) return result; + const items = collapseUninstallItems([ + ...result.items, + item("uninstall_cleanup", "failed", cleanupFailure), + ]); + return { status: "failed", items }; }; /** @@ -157,12 +192,14 @@ const removeAll = async ( /** Create uninstall effects contained to detected client configs and REA-owned paths. */ export const systemUninstallHost = ( selectedHome: string | undefined = undefined, - fileSystem: UninstallFileSystem = systemFileSystem, + selectedFileSystem: UninstallFileSystem | undefined = undefined, environment: Readonly = process.env, platform: NodeJS.Platform = process.platform, ): UninstallHost => { const home = selectedHome ?? homeDirectoryFromEnvironment(environment, platform); + const resources = new ArtifactResourceScope(); + const fileSystem = selectedFileSystem ?? systemFileSystem(resources); return { clients: () => Promise.resolve( @@ -185,6 +222,14 @@ export const systemUninstallHost = ( removeSkill: () => removeManagedSkills(home, fileSystem, environment, platform), purgeData: () => purgeManagedData(join(home, ".rea"), fileSystem), + close: async () => { + try { + await resources.close(); + return undefined; + } catch (cause: unknown) { + return `Uninstall resource cleanup failed: ${cause instanceof Error ? cause.message : String(cause)}`; + } + }, }; }; diff --git a/src/application/Update.test.ts b/src/application/Update.test.ts index 26c580ab0..241cb7cd0 100644 --- a/src/application/Update.test.ts +++ b/src/application/Update.test.ts @@ -18,6 +18,7 @@ import { type NpmInstallationHost, } from "./UpdateRuntime.js"; import type { IntegrationMaintenance } from "./UpdateMaintenance.js"; +import { isUpdateFailure } from "./UpdateFailure.js"; class FakeUpdateHost implements UpdateHost { release: Result = ok("3.3.0"); @@ -37,6 +38,7 @@ class FakeUpdateHost implements UpdateHost { verifiedInstallation: UpdateInstallation | undefined; maintenanceInspected = false; output: UpdateOutput | undefined; + cleanupFailure: string | undefined; installation = (): Promise => { this.installationInspected = true; @@ -63,6 +65,8 @@ class FakeUpdateHost implements UpdateHost { this.maintenanceInspected = true; return Promise.resolve(this.maintenance); }; + close = (): Promise => + Promise.resolve(this.cleanupFailure); } describe("REA update", () => { @@ -79,6 +83,20 @@ describe("REA update", () => { expect(host.maintenanceInspected).toBe(false); }); + it("preserves update status while marking incomplete resource cleanup as failure", async () => { + const host = new FakeUpdateHost(); + host.cleanupFailure = "Regular file remains open: /fixture/skill/SKILL.md"; + + const result = await runUpdate("3.3.0", host); + + expect(result).toMatchObject({ + status: "current", + currentVersion: "3.3.0", + cleanupFailure: host.cleanupFailure, + }); + expect(isUpdateFailure(result)).toBe(true); + }); + it("reports an unknown current installation honestly", async () => { const host = new FakeUpdateHost(); host.installationResult = undefined; diff --git a/src/application/Update.ts b/src/application/Update.ts index 6a92b46a5..b9344bfc1 100644 --- a/src/application/Update.ts +++ b/src/application/Update.ts @@ -49,10 +49,11 @@ export interface UpdateHost { planMaintenance( installation: UpdateInstallation, ): Promise; + close?(): Promise; } /** Verified update outcome, separate from the unapplied integration plan. */ -export type UpdateResult = +export type UpdateResult = ( | { readonly status: "current"; readonly currentVersion: string; @@ -80,7 +81,8 @@ export type UpdateResult = | "verification"; readonly installedVersion?: string; readonly remediation: string; - }; + } +) & { readonly cleanupFailure?: string }; /** Resolve the exact package-manager command for the identified installation and release. */ export const updateInstallCommand = ( @@ -141,6 +143,39 @@ export const runUpdate = async ( currentVersion: string, host: UpdateHost, output: UpdateOutput = "human", +): Promise => { + let result: UpdateResult | undefined; + let primaryFailure: unknown; + let failed = false; + try { + result = await runUpdateWorkflow(currentVersion, host, output); + } catch (cause: unknown) { + failed = true; + primaryFailure = cause; + } + let cleanupFailure: string | undefined; + try { + cleanupFailure = await host.close?.(); + } catch (cause: unknown) { + cleanupFailure = `Update resource cleanup failed: ${cause instanceof Error ? cause.message : String(cause)}`; + } + if (failed) { + if (cleanupFailure !== undefined) + throw new AggregateError( + [primaryFailure, new Error(cleanupFailure)], + "Update failed and its resource cleanup remains incomplete.", + { cause: primaryFailure }, + ); + throw primaryFailure; + } + if (result === undefined) throw new Error("Update result was not produced."); + return cleanupFailure === undefined ? result : { ...result, cleanupFailure }; +}; + +const runUpdateWorkflow = async ( + currentVersion: string, + host: UpdateHost, + output: UpdateOutput, ): Promise => { const installation = await host.installation(); const release = await host.latestVersion(installation); diff --git a/src/application/UpdateFailure.ts b/src/application/UpdateFailure.ts index d7791eaab..85503a3c3 100644 --- a/src/application/UpdateFailure.ts +++ b/src/application/UpdateFailure.ts @@ -2,6 +2,7 @@ import type { UpdateResult } from "./Update.js"; /** Whether the update command left the requested release unapplied. */ export const isUpdateFailure = (result: UpdateResult): boolean => { + if (result.cleanupFailure !== undefined) return true; const status = result.status; switch (status) { case "current": diff --git a/src/application/UpdateMaintenance.ts b/src/application/UpdateMaintenance.ts index 2a5a286d5..06ac1b512 100644 --- a/src/application/UpdateMaintenance.ts +++ b/src/application/UpdateMaintenance.ts @@ -14,6 +14,7 @@ import { existingSkillDestinations } from "./SetupSkill.js"; import type { SetupAction } from "./SetupTypes.js"; import type { Result } from "../domain/result.js"; import { readRegularFileText } from "./RegularFileRead.js"; +import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; /** Existing REA integrations selected for maintenance, without discovering new targets. */ export interface MaintenanceScope { @@ -58,9 +59,11 @@ export const existingMaintenanceScope = async ( home: string, entryPoint: string, environment: Readonly, + resources?: ArtifactResourceScope, ): Promise => { const registrations = await readClientRegistrationStatuses(home, entryPoint, { environment, + ...(resources === undefined ? {} : { resources }), }); const supported = supportedClients(home, process.platform, environment); const clients: string[] = []; @@ -73,7 +76,10 @@ export const existingMaintenanceScope = async ( const client = supported.find(({ name }) => name === registration.client); if (client === undefined) continue; const parsed = parseClientConfiguration( - await readRegularFileText(client.configPath), + await readRegularFileText( + client.configPath, + resources === undefined ? {} : { resources }, + ), client.format, ); const enabled = z @@ -91,7 +97,12 @@ export const existingMaintenanceScope = async ( ) clients.push(client.name); } - const skillDestinations = await existingSkillDestinations(home, environment); + const skillDestinations = await existingSkillDestinations( + home, + environment, + process.platform, + resources, + ); return { clients, skill: skillDestinations.length > 0, @@ -105,9 +116,15 @@ export const planIntegrationMaintenance = async ( entryPoint: string, environment: Readonly, execute: (command: readonly string[]) => Promise>, + resources?: ArtifactResourceScope, ): Promise => { try { - const scope = await existingMaintenanceScope(home, entryPoint, environment); + const scope = await existingMaintenanceScope( + home, + entryPoint, + environment, + resources, + ); const skillDestinations = scope.skillDestinations ?? []; if (scope.clients.length === 0 && !scope.skill) return { status: "current", plannedActions: [] }; diff --git a/src/application/UpdateRuntime.ts b/src/application/UpdateRuntime.ts index e2e919404..b4986cbcf 100644 --- a/src/application/UpdateRuntime.ts +++ b/src/application/UpdateRuntime.ts @@ -6,6 +6,7 @@ import spawn from "cross-spawn"; import { lt, valid } from "semver"; import { PRODUCT_IDENTITY } from "../identity.js"; +import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { err, ok, type Result } from "../domain/result.js"; import { safeParseJson } from "../domain/safeJson.js"; import { @@ -292,74 +293,86 @@ export const systemUpdateHost = ( packageRoot = fileURLToPath(new URL("../..", import.meta.url)), home: string | undefined = undefined, environment: Readonly = process.env, -): UpdateHost => ({ - installation: async () => - (await detectBunInstallation( - packageRoot, - bunGlobalDirectory(environment, process.platform), - { canonicalPath: realpath, isFile }, - )) ?? - detectNpmInstallation(packageRoot, { - canonicalPath: realpath, - globalRoot: () => - requireCommandOutput(["npm", "root", "--global"], environment), - globalPrefix: () => - requireCommandOutput(["npm", "prefix", "--global"], environment), - }), - latestVersion: async (installation) => { - const context = packageManagerContext(installation, environment); - if (installation?.manager === "bun") { - const version = await runUpdateCommand( - ["bun", "--version"], +): UpdateHost => { + const resources = new ArtifactResourceScope(); + return { + installation: async () => + (await detectBunInstallation( + packageRoot, + bunGlobalDirectory(environment, process.platform), + { canonicalPath: realpath, isFile }, + )) ?? + detectNpmInstallation(packageRoot, { + canonicalPath: realpath, + globalRoot: () => + requireCommandOutput(["npm", "root", "--global"], environment), + globalPrefix: () => + requireCommandOutput(["npm", "prefix", "--global"], environment), + }), + latestVersion: async (installation) => { + const context = packageManagerContext(installation, environment); + if (installation?.manager === "bun") { + const version = await runUpdateCommand( + ["bun", "--version"], + context.environment, + "structured", + context.workingDirectory, + ); + if (!version.ok) + return err({ kind: "unavailable", detail: version.error }); + const bunVersion = valid(version.value); + // bun info was introduced in 1.2.17; older releases reserve the command. + if (bunVersion === null || lt(bunVersion, "1.2.17")) + return err({ + kind: "unsupported-package-manager", + detail: `REA updates for Bun global installations require Bun 1.2.17 or later; bun --version returned ${JSON.stringify(version.value)}. Upgrade Bun manually, then rerun rea update.`, + }); + } + return npmReleaseLookup( + await runUpdateCommand( + installation?.manager === "bun" + ? BUN_LATEST_VERSION_COMMAND + : npmLatestVersionCommand(installation), + context.environment, + "structured", + context.workingDirectory, + ), + ); + }, + installVersion: async (installation, version, output) => { + const context = packageManagerContext(installation, environment); + const result = await runUpdateCommand( + updateInstallCommand(installation, version), context.environment, - "structured", + output, context.workingDirectory, ); - if (!version.ok) - return err({ kind: "unavailable", detail: version.error }); - const bunVersion = valid(version.value); - // bun info was introduced in 1.2.17; older releases reserve the command. - if (bunVersion === null || lt(bunVersion, "1.2.17")) - return err({ - kind: "unsupported-package-manager", - detail: `REA updates for Bun global installations require Bun 1.2.17 or later; bun --version returned ${JSON.stringify(version.value)}. Upgrade Bun manually, then rerun rea update.`, - }); - } - return npmReleaseLookup( - await runUpdateCommand( - installation?.manager === "bun" - ? BUN_LATEST_VERSION_COMMAND - : npmLatestVersionCommand(installation), - context.environment, - "structured", - context.workingDirectory, + return result.ok ? ok(undefined) : result; + }, + installedVersion: (installation) => + runUpdateCommand( + [ + process.execPath, + join(installation.packageRoot, "scripts", "rea.mjs"), + "--version", + ], + environment, ), - ); - }, - installVersion: async (installation, version, output) => { - const context = packageManagerContext(installation, environment); - const result = await runUpdateCommand( - updateInstallCommand(installation, version), - context.environment, - output, - context.workingDirectory, - ); - return result.ok ? ok(undefined) : result; - }, - installedVersion: (installation) => - runUpdateCommand( - [ - process.execPath, + planMaintenance: (installation) => + planIntegrationMaintenance( + home ?? homeDirectoryFromEnvironment(environment, process.platform), join(installation.packageRoot, "scripts", "rea.mjs"), - "--version", - ], - environment, - ), - planMaintenance: (installation) => - planIntegrationMaintenance( - home ?? homeDirectoryFromEnvironment(environment, process.platform), - join(installation.packageRoot, "scripts", "rea.mjs"), - environment, - (command) => runUpdateCommand(command, environment), - ), -}); + environment, + (command) => runUpdateCommand(command, environment), + resources, + ), + close: async () => { + try { + await resources.close(); + return undefined; + } catch (cause: unknown) { + return `Update resource cleanup failed: ${cause instanceof Error ? cause.message : String(cause)}`; + } + }, + }; +}; diff --git a/src/cli/setupCommands.ts b/src/cli/setupCommands.ts index d21be1c08..15caac321 100644 --- a/src/cli/setupCommands.ts +++ b/src/cli/setupCommands.ts @@ -120,11 +120,12 @@ const registerDoctorCommand = ( }), run: ({ options }) => logCliCommand(logger, "doctor", async () => { - const [{ runDoctor }, { createSystemDoctorHost }] = await Promise.all([ - import("../application/Doctor.js"), - import("../doctorRuntime.js"), - ]); - return runDoctor( + const [{ runDoctorOwned }, { createSystemDoctorHost }] = + await Promise.all([ + import("../application/Doctor.js"), + import("../doctorRuntime.js"), + ]); + return runDoctorOwned( options.target, createSystemDoctorHost(environment), doctorScope(options), diff --git a/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts b/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts new file mode 100644 index 000000000..18003b039 --- /dev/null +++ b/tests/boundary/filesystem/setupSkillCleanupOwnership.test.ts @@ -0,0 +1,237 @@ +import { + mkdir, + open as fsOpen, + writeFile, + type FileHandle, +} from "node:fs/promises"; +import { dirname, join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js"; +import { PRODUCT_IDENTITY } from "../../../src/identity.js"; +import { createDoctorHostFixture } from "../../../src/application/Doctor.fixture.js"; +import { + runDoctorOwned, + type DoctorHost, +} from "../../../src/application/Doctor.js"; +import { + readSkillFile, + skillDestinations, +} from "../../../src/application/SetupSkill.js"; +import { systemSetupHost } from "../../../src/application/SetupHost.js"; +import { + runUninstall, + systemUninstallHost, +} from "../../../src/application/Uninstall.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, open: vi.fn(actual.open) }; +}); + +const openMock = vi.mocked(fsOpen); +let actualFs: typeof import("node:fs/promises"); + +beforeEach(async () => { + actualFs = + await vi.importActual( + "node:fs/promises", + ); + openMock.mockImplementation((...args) => actualFs.open(...args)); +}); + +afterEach(() => openMock.mockReset()); + +const failFirstCloses = (path: string) => { + const attempts: FileHandle[] = []; + let allowClose = false; + openMock.mockImplementation(async (...args) => { + const handle = await actualFs.open(...args); + if (String(args[0]) !== path) return handle; + const nativeClose = handle.close.bind(handle); + vi.spyOn(handle, "close").mockImplementation(async () => { + attempts.push(handle); + if (!allowClose) throw new Error("test holds the skill handle open"); + await nativeClose(); + }); + return handle; + }); + return { + attempts, + release: () => { + allowClose = true; + }, + }; +}; + +describe("skill regular-file cleanup ownership", () => { + it("keeps setup skill reads owned until setup host disposal", async () => { + const home = await createTestTempDirectory("rea-setup-skill-close-"); + const [destination] = skillDestinations(home, []); + if (destination === undefined) throw new Error("Shared skill root missing"); + const manifest = join(destination.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + await writeFile(manifest, "stale managed skill\n"); + const close = failFirstCloses(manifest); + const host = systemSetupHost( + { platform: "linux", homeDirectory: home } as DoctorHost, + { HOME: home }, + ); + + try { + expect(await host.skillNeedsInstall([])).toBe(true); + expect(close.attempts.length).toBeGreaterThan(0); + expect(new Set(close.attempts).size).toBe(1); + const attemptsBeforeClose = close.attempts.length; + close.release(); + expect(await host.close?.()).toBeUndefined(); + expect(close.attempts.length).toBeGreaterThan(attemptsBeforeClose); + expect(new Set(close.attempts).size).toBe(1); + expect(close.attempts[0]?.fd).toBe(-1); + } finally { + close.release(); + await host.close?.(); + } + }); + + it("waits for an admitted skill read before its resource scope closes", async () => { + const home = await createTestTempDirectory("rea-setup-skill-inflight-"); + const [destination] = skillDestinations(home, []); + if (destination === undefined) throw new Error("Shared skill root missing"); + const manifest = join(destination.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + await writeFile(manifest, "stale managed skill\n"); + const resources = new ArtifactResourceScope(); + let beginRead!: () => void; + const readStarted = new Promise((resolve) => { + beginRead = resolve; + }); + let unblockRead!: () => void; + const readGate = new Promise((resolve) => { + unblockRead = resolve; + }); + let heldRead = false; + openMock.mockImplementation(async (...args) => { + const handle = await actualFs.open(...args); + if (String(args[0]) !== manifest || heldRead) return handle; + heldRead = true; + const nativeRead = handle.read.bind(handle); + vi.spyOn(handle, "read").mockImplementation(async (...readArgs) => { + beginRead(); + await readGate; + return nativeRead(...readArgs); + }); + return handle; + }); + + try { + const reading = readSkillFile(manifest, resources); + await readStarted; + let closed = false; + const closing = resources.close().then(() => { + closed = true; + }); + await Promise.resolve(); + expect(closed).toBe(false); + unblockRead(); + expect(await reading).toBe("stale managed skill\n"); + await closing; + expect(closed).toBe(true); + } finally { + unblockRead(); + await resources.close(); + } + }); +}); + +describe("skill cleanup diagnostics", () => { + it("preserves a doctor report and cleanup diagnostic when host close still fails", async () => { + const home = await createTestTempDirectory("rea-doctor-skill-close-"); + const [destination] = skillDestinations(home, []); + if (destination === undefined) throw new Error("Shared skill root missing"); + const manifest = join(destination.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + await writeFile(manifest, "managed skill fixture\n"); + const close = failFirstCloses(manifest); + const resources = new ArtifactResourceScope(); + const host = createDoctorHostFixture({ + installedSkillIdentity: async () => { + await readSkillFile(manifest, resources); + return { version: null, toolCount: null, canonical: true }; + }, + close: async () => { + try { + await resources.close(); + return undefined; + } catch (cause: unknown) { + return cause instanceof Error ? cause.message : String(cause); + } + }, + }); + + try { + const report = await runDoctorOwned(undefined, host, { skill: true }); + + expect(report.healthy).toBe(false); + expect(report.identity?.skill.installed_version).toBeNull(); + expect(report.checks).toContainEqual( + expect.objectContaining({ + name: "doctor-resource-cleanup", + ok: false, + detail: expect.stringContaining(manifest), + }), + ); + expect(close.attempts.length).toBeGreaterThan(0); + expect(new Set(close.attempts).size).toBe(1); + const attemptsBeforeClose = close.attempts.length; + close.release(); + expect(await host.close?.()).toBeUndefined(); + expect(close.attempts.length).toBeGreaterThan(attemptsBeforeClose); + expect(new Set(close.attempts).size).toBe(1); + expect(close.attempts[0]?.fd).toBe(-1); + } finally { + close.release(); + await host.close?.(); + } + }); + + it("returns uninstall changes with a failed cleanup item for a retained handle", async () => { + const home = await createTestTempDirectory("rea-uninstall-skill-close-"); + const [destination] = skillDestinations(home, []); + if (destination === undefined) throw new Error("Shared skill root missing"); + const manifest = join(destination.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + await writeFile( + manifest, + `---\nname: ${PRODUCT_IDENTITY.skillName}\n---\nmanaged fixture\n`, + ); + const close = failFirstCloses(manifest); + const host = systemUninstallHost(home, undefined, { HOME: home }); + + try { + const result = await runUninstall(false, host); + + expect(result.status).toBe("failed"); + expect(result.items).toContainEqual( + expect.objectContaining({ + name: "uninstall_cleanup", + status: "failed", + detail: expect.stringContaining(manifest), + }), + ); + expect(close.attempts.length).toBeGreaterThan(0); + expect(new Set(close.attempts).size).toBe(1); + const attemptsBeforeClose = close.attempts.length; + close.release(); + expect(await host.close?.()).toBeUndefined(); + expect(close.attempts.length).toBeGreaterThan(attemptsBeforeClose); + expect(new Set(close.attempts).size).toBe(1); + expect(close.attempts[0]?.fd).toBe(-1); + } finally { + close.release(); + await host.close?.(); + } + }); +});