diff --git a/docs/installation.md b/docs/installation.md index 80a10fe20b..6e30879d43 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -135,6 +135,15 @@ exits 1. Installing missing Hopper non-interactively additionally requires rea setup --yes --all-detected --install-hopper --json ``` +Client configuration writes detect changes to the file read by setup, including +when a matching recovery backup already exists. A detected conflict reports +`reason: "conflict"` and preserves the newer file; review it and rerun setup. +Rollback likewise requires the file identity and content installed by that +transaction. Requests for the same resolved path are serialized within one REA +process. These checks do not provide an operating-system compare-and-swap or +coordinate arbitrary external writers, so avoid editing client configuration +while setup is applying it. + ### Skill destinations The bundled skill is installed in each selected client's personal skill directory. diff --git a/src/application/ClientConfigPath.ts b/src/application/ClientConfigPath.ts index c28cc800c0..67e634b01b 100644 --- a/src/application/ClientConfigPath.ts +++ b/src/application/ClientConfigPath.ts @@ -1,4 +1,5 @@ import { lstat, realpath } from "node:fs/promises"; +import { basename, dirname, join, resolve } from "node:path"; interface ClientConfigPathStats { readonly uid?: number; @@ -14,21 +15,32 @@ interface ClientConfigPathFileSystem { const systemFileSystem: ClientConfigPathFileSystem = { lstat, realpath }; /** - * Resolve an existing, user-owned config symlink to one stable transaction - * path. Missing regular paths remain eligible for first-run creation. + * Resolve one stable transaction path for a configuration file. + * An existing user-owned config symlink resolves to its target file. + * Parent-directory aliases resolve for regular and missing files, so a + * directory symlink and its real directory share one queue key. Missing + * paths stay eligible for first-run creation. */ export const resolveClientConfigTransactionPath = async ( requestedPath: string, fileSystem: ClientConfigPathFileSystem = systemFileSystem, ): Promise => { - let requestedStats: ClientConfigPathStats; + let requestedStats: ClientConfigPathStats | undefined; try { requestedStats = await fileSystem.lstat(requestedPath); } catch (cause: unknown) { - return isMissing(cause) ? requestedPath : undefined; + if (!isMissing(cause)) return undefined; } - if (!requestedStats.isSymbolicLink()) return requestedPath; + if (requestedStats?.isSymbolicLink() === true) + return resolveOwnedConfigSymlink(requestedPath, requestedStats, fileSystem); + return canonicalRegularConfigPath(requestedPath, fileSystem); +}; +const resolveOwnedConfigSymlink = async ( + requestedPath: string, + requestedStats: ClientConfigPathStats, + fileSystem: ClientConfigPathFileSystem, +): Promise => { const currentUid = typeof process.getuid === "function" ? process.getuid() : undefined; if ( @@ -51,5 +63,33 @@ export const resolveClientConfigTransactionPath = async ( } }; +/** + * Resolve parent-directory aliases, including when the file or an intermediate + * directory is absent. A realpath failure keeps the requested spelling. + */ +const canonicalRegularConfigPath = async ( + requestedPath: string, + fileSystem: ClientConfigPathFileSystem, +): Promise => { + if (fileSystem.realpath === undefined) return requestedPath; + let ancestor = resolve(requestedPath); + const missing: string[] = []; + while (true) { + try { + return join(await fileSystem.realpath(ancestor), ...missing); + } catch (cause: unknown) { + if (!isAbsentPath(cause) || dirname(ancestor) === ancestor) + return requestedPath; + missing.unshift(basename(ancestor)); + ancestor = dirname(ancestor); + } + } +}; + const isMissing = (cause: unknown): boolean => cause instanceof Error && "code" in cause && cause.code === "ENOENT"; + +const isAbsentPath = (cause: unknown): boolean => + cause instanceof Error && + "code" in cause && + (cause.code === "ENOENT" || cause.code === "ENOTDIR"); diff --git a/src/application/ClientConfigurationPublication.ts b/src/application/ClientConfigurationPublication.ts index 4131345b60..230f50d81b 100644 --- a/src/application/ClientConfigurationPublication.ts +++ b/src/application/ClientConfigurationPublication.ts @@ -8,7 +8,7 @@ import { rename, type FileHandle, } from "node:fs/promises"; -import { basename, dirname, join, sep } from "node:path"; +import { basename, dirname, join, resolve, sep } from "node:path"; import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; @@ -20,6 +20,14 @@ import { import type { RegularFileOperationOutcome } from "../filesystem/OwnedRegularFile.js"; import { readRegularFileTextSnapshot } from "./RegularFileRead.js"; import { ClientConfigurationPublicationFailure } from "./ClientConfigurationCleanup.js"; +import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; +import { ConfigurationError } from "../domain/configurationErrors.js"; + +/** The original read owner's selection, before merging and preparing a backup. */ +export interface ClientConfigurationSelection { + readonly requestedPath: string; + readonly state: Stats | undefined; +} /** The exact staged file retained through publication, verification and recovery. */ export class ClientConfigurationPublication { @@ -36,10 +44,20 @@ export class ClientConfigurationPublication { contents: string, previous: string | undefined, mode?: number, + selection?: ClientConfigurationSelection, ): Promise { const parent = await realpath(dirname(path)); const destination = join(parent, basename(path)); const input = await selectedConfiguration(destination, previous); + if (selection !== undefined) { + await assertRequestedConfigurationPath(path, selection.requestedPath); + if ( + selection.state === undefined + ? input !== undefined + : input === undefined || !sameRegularFileState(selection.state, input) + ) + throw new RegularFileChangedError(destination); + } const directory = await mkdtemp(`${parent}${sep}.rea-config-`); const staging = await OwnedStagedOutput.create(directory); const cleanup = new AnalysisCleanupLease( @@ -72,10 +90,23 @@ export class ClientConfigurationPublication { if ((await owner.handle.stat()).ino === 0) throw new RegularFileChangedError(stagedPath); await staging.assertReadyForPublication(stagedPath); + if (selection !== undefined) + await assertRequestedConfigurationPath(path, selection.requestedPath); await assertSelectedConfiguration(destination, input); if (input === undefined) { // An exclusive link cannot overwrite a file created after absence was checked. - await link(stagedPath, destination); + try { + await link(stagedPath, destination); + } catch (cause: unknown) { + if ( + selection !== undefined && + cause instanceof Error && + "code" in cause && + cause.code === "EEXIST" + ) + throw new RegularFileChangedError(destination, { cause }); + throw cause; + } } else { // The destination is explicit: rename never follows its final symlink. await rename(stagedPath, destination); @@ -107,11 +138,12 @@ export class ClientConfigurationPublication { return outcome.value; } - async assertCurrent(): Promise { + async assertCurrent(readback?: string): Promise { const owned = await this.owner.handle.stat(); const named = await lstat(this.path); if ( owned.ino === 0 || + (readback !== undefined && readback !== this.contents) || named.isSymbolicLink() || !sameRegularFileState(owned, named) ) @@ -120,10 +152,6 @@ export class ClientConfigurationPublication { /** State checks do not exclude a concurrent writer to the same inode. */ async restore(original: string | undefined): Promise { - if (original === undefined) - throw new Error( - "First-run configuration recovery requires manual removal; pathname deletion cannot prove ownership", - ); const handle = this.owner.handle; const before = await handle.stat(); const named = await lstat(this.path); @@ -137,6 +165,10 @@ export class ClientConfigurationPublication { !sameRegularFileState(before, await lstat(this.path)) ) throw new RegularFileChangedError(this.path); + if (original === undefined) + throw new Error( + "First-run configuration recovery requires manual removal; pathname deletion cannot prove ownership", + ); const bytes = Buffer.from(original, "utf8"); for (let position = 0; position < bytes.length;) { const { bytesWritten } = await handle.write( @@ -178,6 +210,53 @@ export class ClientConfigurationPublication { } } +const assertRequestedConfigurationPath = async ( + path: string, + requestedPath: string, +): Promise => { + const current = await resolveClientConfigTransactionPath(requestedPath); + if (current === undefined) + throw new ConfigurationError( + `Client configuration path could not be safely verified: ${requestedPath}`, + ); + if (resolve(current) === resolve(path)) return; + // `path` is the canonical file selected for this transaction. A later + // retarget of a parent-directory alias keeps that file. A configuration + // symlink that now names another target, or a path whose parent is already + // canonical, still conflicts. + if (await isParentAliasRetarget(requestedPath, path, current)) return; + throw new RegularFileChangedError(path); +}; + +const isParentAliasRetarget = async ( + requestedPath: string, + path: string, + current: string, +): Promise => { + if (basename(current) !== basename(path)) return false; + let requestedStats: { isSymbolicLink(): boolean }; + try { + requestedStats = await lstat(requestedPath); + } catch (cause: unknown) { + if (isNodeErrorCode(cause, "ENOENT")) return false; + throw new ConfigurationError( + `Client configuration path could not be safely verified: ${requestedPath}`, + ); + } + if (requestedStats.isSymbolicLink()) return false; + const requestedParent = dirname(resolve(requestedPath)); + try { + const canonicalParent = await realpath(requestedParent); + return resolve(canonicalParent) !== requestedParent; + } catch (cause: unknown) { + void cause; + return false; + } +}; + +const isNodeErrorCode = (cause: unknown, code: string): boolean => + cause instanceof Error && "code" in cause && cause.code === code; + const selectedConfiguration = async ( path: string, previous: string | undefined, diff --git a/src/application/ClientConfigurationTransaction.ts b/src/application/ClientConfigurationTransaction.ts new file mode 100644 index 0000000000..4cea576054 --- /dev/null +++ b/src/application/ClientConfigurationTransaction.ts @@ -0,0 +1,27 @@ +import { resolve } from "node:path"; + +const activeTransactions = new Map>(); + +/** + * Serialize the full read/merge/publish operation for one resolved + * configuration path. + */ +export const withClientConfigurationTransaction = async ( + path: string, + operation: () => Promise, +): Promise => { + const key = resolve(path); + const previous = activeTransactions.get(key); + let release!: () => void; + const current = new Promise((complete) => { + release = complete; + }); + activeTransactions.set(key, current); + await previous; + try { + return await operation(); + } finally { + release(); + if (activeTransactions.get(key) === current) activeTransactions.delete(key); + } +}; diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index c6c09a7736..3d4887f2c3 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -23,6 +23,8 @@ import { import { dshDocument, DshCordisDocument } from "./DshCordisPatch.js"; import { mkdir } from "node:fs/promises"; import { dirname } from "node:path"; +import { RegularFileChangedError } from "../filesystem/RegularFile.js"; +import { withClientConfigurationTransaction } from "./ClientConfigurationTransaction.js"; import { isClientConfigurationCleanupFailure } from "./ClientConfigurationCleanup.js"; import { ClientConfigurationPublication } from "./ClientConfigurationPublication.js"; import { @@ -42,7 +44,11 @@ import { import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; import { packageReadTargetChanged } from "./MsixPackageStorage.js"; import { RegularFileCleanupFailure } from "../filesystem/OwnedRegularFile.js"; -import { readRegularFileText } from "./RegularFileRead.js"; +import { + readRegularFileText, + readRegularFileTextSnapshot, + InvalidUtf8FileError, +} from "./RegularFileRead.js"; import { preserveCurrentRecoveryBackup, selectCurrentRecoveryBackup, @@ -63,7 +69,7 @@ export class ClientConfigurationCleanupRollbackFailure extends RegularFileCleanu failure: RegularFileCleanupFailure, readonly configurationFailure: Extract< ClientConfigurationResult, - { readonly status: "failed"; readonly reason: "rollback" } + { readonly status: "failed" } >, ) { super( @@ -71,8 +77,8 @@ export class ClientConfigurationCleanupRollbackFailure extends RegularFileCleanu failure.owner, failure.outcome, new AggregateError( - [failure.cleanupCause, new Error(configurationFailure.failure.detail)], - `Configuration readback cleanup and rollback failed: ${configurationFailure.failure.path}`, + [failure.cleanupCause, configurationFailure], + `Configuration readback cleanup and rollback failed: ${failure.path}`, ), failure.retention, failure.completionCancellation, @@ -81,24 +87,34 @@ export class ClientConfigurationCleanupRollbackFailure extends RegularFileCleanu } /** Configure one supported client's native stdio MCP registration shape. */ -export const configureClientConfiguration = ( +export const configureClientConfiguration = async ( client: SetupClient, environment: SetupProviderEnvironment = {}, command: readonly string[] = npxRegistrationCommand(), resolveProviderRequestBudget?: ProviderRequestBudgetResolver, ): Promise => { if (client.configPathError !== undefined) - return Promise.resolve({ status: "failed", reason: "path" }); - return configureClientDocument( - client, - environment, - command, - resolveProviderRequestBudget, + return { status: "failed", reason: "path" }; + const transactionPath = await resolveClientConfigTransactionPath( + client.configPath, + ); + if (transactionPath === undefined) + return { status: "failed", reason: "path" }; + return withClientConfigurationTransaction(transactionPath, () => + configureClientDocument( + { client, transactionPath }, + environment, + command, + resolveProviderRequestBudget, + ), ); }; const configureClientDocument = async ( - client: SetupClient, + { + client, + transactionPath, + }: { readonly client: SetupClient; readonly transactionPath: string }, environment: SetupProviderEnvironment, command: readonly string[], resolveProviderRequestBudget?: ProviderRequestBudgetResolver, @@ -138,18 +154,18 @@ const configureClientDocument = async ( } if (policy.value !== undefined) return { status: "failed", reason: "readback" }; - const transactionPath = await resolveClientConfigTransactionPath( - client.configPath, - ); - if (transactionPath === undefined) - return { status: "failed", reason: "path" }; - let original: string | undefined; + let snapshot: + | Awaited> + | undefined; try { - original = await readRegularFileText(transactionPath); + snapshot = await readRegularFileTextSnapshot(transactionPath, { + symlinks: "reject", + }); } catch (cause: unknown) { if (cause instanceof RegularFileCleanupFailure) throw cause; if (!isMissing(cause)) return { status: "failed", reason: "readback" }; } + const original = snapshot?.text; let parsed: ClientConfigurationDocument; try { parsed = parseClientConfiguration( @@ -203,6 +219,7 @@ const configureClientDocument = async ( return persistClientConfiguration({ client, transactionPath, + snapshot, format, original, parsed, @@ -214,6 +231,9 @@ const configureClientDocument = async ( interface ClientConfigurationUpdate { readonly client: SetupClient; readonly transactionPath: string; + readonly snapshot: + | Awaited> + | undefined; readonly format: NonNullable; readonly original: string | undefined; readonly parsed: ClientConfigurationDocument; @@ -224,6 +244,7 @@ interface ClientConfigurationUpdate { const persistClientConfiguration = async ({ client, transactionPath, + snapshot, format, original, parsed, @@ -328,20 +349,21 @@ const persistClientConfiguration = async ({ serialized.value, original, 0o600, + { requestedPath: client.configPath, state: snapshot?.state }, ); } catch (cause: unknown) { if (isClientConfigurationCleanupFailure(cause)) throw cause; + if (isConfigurationConflict(cause)) + return conflictFailure(recoveryBackupPath); // Write failure is reported by the status reason. void cause; return { status: "failed", reason: "write" }; } return publication.verify(async () => { try { - const readback = parseClientConfiguration( - await readRegularFileText(publication.path), - format, - ); - await publication.assertCurrent(); + const text = await readRegularFileText(publication.path); + await publication.assertCurrent(text); + const readback = parseClientConfiguration(text, format); if ( !registrationCurrent(readback, desired) || Object.entries(fallbackServers).some( @@ -357,17 +379,15 @@ const persistClientConfiguration = async ({ ); } } catch (cause: unknown) { + if (isConfigurationConflict(cause) || isMissing(cause)) + return conflictFailure(recoveryBackupPath); if (cause instanceof RegularFileCleanupFailure) { const rollback = await restoreConfig(publication, original); if (!rollback.ok) - throw new ClientConfigurationCleanupRollbackFailure(cause, { - status: "failed", - reason: "rollback", - ...(recoveryBackupPath === undefined - ? {} - : { backupPath: recoveryBackupPath }), - failure: rollback.error, - }); + throw new ClientConfigurationCleanupRollbackFailure( + cause, + rollbackFailure(rollback.error, recoveryBackupPath), + ); throw cause; } // Readback failure restores the transaction before reporting. @@ -627,19 +647,24 @@ const readZcodeFallbackServers = async ( } }; +type ConfigurationRestoreFailure = + | { readonly kind: "conflict" } + | { + readonly kind: "restore"; + readonly path: string; + readonly detail: string; + }; + const restoreConfig = async ( publication: ClientConfigurationPublication, original: string | undefined, -): Promise< - Result< - void, - { readonly kind: "restore"; readonly path: string; readonly detail: string } - > -> => { +): Promise> => { try { await publication.restore(original); return ok(undefined); } catch (cause: unknown) { + if (isConfigurationConflict(cause) || isMissing(cause)) + return err({ kind: "conflict" }); return err({ kind: "restore", path: publication.path, @@ -660,13 +685,33 @@ const rollbackOrReadbackFailure = async ( reason: "readback", ...(backupPath === undefined ? {} : { backupPath }), } + : rollbackFailure(rollback.error, backupPath); +}; + +const rollbackFailure = ( + failure: ConfigurationRestoreFailure, + backupPath?: string, +): Extract => + failure.kind === "conflict" + ? conflictFailure(backupPath) : { status: "failed", reason: "rollback", + failure, ...(backupPath === undefined ? {} : { backupPath }), - failure: rollback.error, }; -}; + +const isConfigurationConflict = (cause: unknown): boolean => + cause instanceof RegularFileChangedError || + cause instanceof InvalidUtf8FileError; + +const conflictFailure = ( + backupPath?: string, +): Extract => ({ + status: "failed", + reason: "conflict", + ...(backupPath === undefined ? {} : { backupPath }), +}); const errorDetail = (cause: unknown): string => cause instanceof Error ? cause.message : String(cause); diff --git a/src/application/SetupClients.ts b/src/application/SetupClients.ts index aa5ff67a48..361d03ce83 100644 --- a/src/application/SetupClients.ts +++ b/src/application/SetupClients.ts @@ -10,6 +10,8 @@ export const failedConfigurationMessage = ( result: Extract, ): string => { switch (result.reason) { + case "conflict": + return "Agent configuration changed during setup. Review the current file and rerun setup."; case "target_changed": return "The client began reading a different configuration file after the plan was shown, so no change was made. Rerun setup to review the new target."; case "target_unavailable": diff --git a/src/application/SetupTypes.ts b/src/application/SetupTypes.ts index 8acfbe139b..893825e90a 100644 --- a/src/application/SetupTypes.ts +++ b/src/application/SetupTypes.ts @@ -29,6 +29,11 @@ export interface SetupInitialState { /** Result of one backup/write/readback transaction. */ export type ClientConfigurationResult = + | { + readonly status: "failed"; + readonly reason: "conflict"; + readonly backupPath?: string; + } | { readonly status: "unchanged" | "configured" | "skipped"; readonly backupPath?: string; diff --git a/tests/acceptance/setup/piClientSetup.test.ts b/tests/acceptance/setup/piClientSetup.test.ts index afc24ae7c7..52659a8569 100644 --- a/tests/acceptance/setup/piClientSetup.test.ts +++ b/tests/acceptance/setup/piClientSetup.test.ts @@ -226,6 +226,7 @@ cliTest( environment: { HOME: home, USERPROFILE: home, + HOPPER_LAUNCHER_PATH: join(home, "unconfigured-hopper"), PI_CODING_AGENT_DIR: "custom-agent", OMP_PROFILE: "work", PI_PROFILE: "legacy", @@ -341,6 +342,7 @@ cliTest( environment: { HOME: home, USERPROFILE: home, + HOPPER_LAUNCHER_PATH: join(home, "unconfigured-hopper"), PI_CODING_AGENT_DIR: "custom-agent", }, timeoutMs: 20_000, @@ -449,6 +451,7 @@ cliTest( environment: { HOME: home, USERPROFILE: home, + HOPPER_LAUNCHER_PATH: join(home, "unconfigured-hopper"), PI_CODING_AGENT_DIR: "file:///agent%2fdir", }, timeoutMs: 20_000, diff --git a/tests/boundary/filesystem/setupConfig.test.ts b/tests/boundary/filesystem/setupConfig.test.ts index ada09168ce..1e253a0989 100644 --- a/tests/boundary/filesystem/setupConfig.test.ts +++ b/tests/boundary/filesystem/setupConfig.test.ts @@ -50,6 +50,38 @@ describe("JSON client configuration transaction: writing configuration through s }, ); + it.skipIf(process.platform === "win32")( + "resolves a directory symlink and its real directory to one transaction path", + async () => { + const root = await createTestTempDirectory("rea-config-dir-alias-"); + const realDirectory = join(root, "real"); + const alias = join(root, "alias"); + await mkdir(realDirectory); + await symlink(realDirectory, alias, "dir"); + const realPath = join(realDirectory, "config.json"); + const aliasPath = join(alias, "config.json"); + const nestedRealPath = join(realDirectory, "nested", "config.json"); + const nestedAliasPath = join(alias, "nested", "config.json"); + + expect(await resolveClientConfigTransactionPath(aliasPath)).toBe( + realPath, + ); + expect(await resolveClientConfigTransactionPath(realPath)).toBe(realPath); + expect(await resolveClientConfigTransactionPath(nestedAliasPath)).toBe( + nestedRealPath, + ); + expect(await resolveClientConfigTransactionPath(nestedRealPath)).toBe( + nestedRealPath, + ); + + await writeFile(realPath, "{}\n"); + expect(await resolveClientConfigTransactionPath(aliasPath)).toBe( + realPath, + ); + expect(await resolveClientConfigTransactionPath(realPath)).toBe(realPath); + }, + ); + it("preserves existing keys, creates a backup, and reads back the MCP entry", async () => { const directory = await createTestTempDirectory("rea-setup-"); const configPath = join(directory, "mcp.json"); diff --git a/tests/process-global/setupConfigurationConflicts.test.ts b/tests/process-global/setupConfigurationConflicts.test.ts new file mode 100644 index 0000000000..96c4ac2990 --- /dev/null +++ b/tests/process-global/setupConfigurationConflicts.test.ts @@ -0,0 +1,454 @@ +import { + chmod, + lstat, + mkdir, + readFile, + readdir, + rename, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { join, relative } from "node:path"; +import { afterEach, expect, it, vi } from "vitest"; + +const writers = vi.hoisted(() => ({ + before: undefined as undefined | ((path: string) => Promise), + after: undefined as undefined | ((path: string) => Promise), + duringRead: undefined as undefined | (() => Promise), +})); + +vi.mock( + "../../src/application/ClientConfigurationPublication.js", + async (importOriginal) => { + const actual = + await importOriginal< + typeof import("../../src/application/ClientConfigurationPublication.js") + >(); + return { + ...actual, + ClientConfigurationPublication: { + write: async ( + ...args: Parameters< + typeof actual.ClientConfigurationPublication.write + > + ) => { + await writers.before?.(args[0]); + const publication = await actual.ClientConfigurationPublication.write( + ...args, + ); + await writers.after?.(args[0]); + return publication; + }, + }, + }; + }, +); + +vi.mock("../../src/process/BoundedFileBytes.js", async (importOriginal) => { + const actual = + await importOriginal< + typeof import("../../src/process/BoundedFileBytes.js") + >(); + return { + ...actual, + readBoundedFileBytes: async ( + ...args: Parameters + ) => { + const bytes = await actual.readBoundedFileBytes(...args); + const mutation = writers.duringRead; + writers.duringRead = undefined; + await mutation?.(); + return bytes; + }, + }; +}); + +import { configureClientConfiguration } from "../../src/application/SetupClientConfiguration.js"; +import { createTestTempDirectory } from "../fixtures/temporaryDirectory.js"; + +afterEach(() => { + writers.before = undefined; + writers.after = undefined; + writers.duringRead = undefined; +}); + +it("reports a conflict when a file changes while the freshness snapshot is being read", async () => { + const root = await createTestTempDirectory("rea-config-read-conflict-"); + const path = join(root, "config.json"); + const changed = '{"external":"changed during read"}\n'; + await writeFile(path, "{}\n"); + writers.before = async (target) => { + writers.duringRead = () => writeFile(target, changed); + }; + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(path, "utf8")).toBe(changed); +}); + +it.each(["before", "after"] as const)( + "preserves an invalid UTF-8 replacement written %s installation and reports a conflict", + async (phase) => { + const root = await createTestTempDirectory( + "rea-config-invalid-replacement-", + ); + const path = join(root, "config.json"); + const original = "{}\n"; + const changed = Buffer.from([0xff, 0xfe]); + await writeFile(path, original); + writers[phase] = (target) => writeFile(target, changed); + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(path)).toEqual(changed); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(original); + expect(await readdir(root)).toEqual([ + "config.json", + "config.json.rea.backup", + ]); + }, +); + +it("preserves an unrelated concurrent edit when a matching recovery backup already exists", async () => { + const root = await createTestTempDirectory("rea-config-conflict-"); + const path = join(root, "config.json"); + const original = '{"theme":"dark"}\n'; + const changed = + '{"theme":"light","mcpServers":{"other":{"command":"other"}}}\n'; + await writeFile(path, original); + await writeFile(`${path}.rea.backup`, original); + writers.before = (target) => writeFile(target, changed); + + const result = await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }); + + expect(result).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(path, "utf8")).toBe(changed); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(original); + expect(await readdir(root)).toEqual([ + "config.json", + "config.json.rea.backup", + ]); + writers.before = undefined; + const retry = await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }); + expect(retry.status).toBe("configured"); + if (retry.status !== "configured" || retry.backupPath === undefined) + throw new Error("Retry did not preserve its current recovery snapshot"); + expect(await readFile(retry.backupPath, "utf8")).toBe(changed); + expect(JSON.parse(await readFile(path, "utf8"))).toMatchObject({ + theme: "light", + mcpServers: { other: { command: "other" } }, + }); + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toEqual({ status: "unchanged" }); +}); + +it("does not overwrite a file created by another writer during first-run setup", async () => { + const root = await createTestTempDirectory("rea-config-first-run-conflict-"); + const path = join(root, "config.json"); + const changed = '{"theme":"light"}\n'; + writers.before = (target) => writeFile(target, changed); + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(path, "utf8")).toBe(changed); + expect(await readdir(root)).toEqual(["config.json"]); +}); + +it("does not recreate a configuration deleted during setup", async () => { + const root = await createTestTempDirectory("rea-config-deleted-"); + const path = join(root, "config.json"); + await writeFile(path, "{}\n"); + writers.before = (target) => rm(target); + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + await expect(lstat(path)).rejects.toMatchObject({ code: "ENOENT" }); +}); + +it("preserves an external edit made after the atomic write without rolling it back", async () => { + const root = await createTestTempDirectory("rea-config-after-write-"); + const path = join(root, "config.json"); + const changed = '{"theme":"external"}\n'; + await writeFile(path, "{}\n"); + writers.after = (target) => writeFile(target, changed); + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(path, "utf8")).toBe(changed); +}); + +it("does not adopt another writer's replacement merely because its bytes match", async () => { + const root = await createTestTempDirectory("rea-config-replaced-"); + const path = join(root, "config.json"); + const replacement = join(root, "replacement.json"); + let replacementInode: number | undefined; + writers.after = async (target) => { + await writeFile(replacement, await readFile(target)); + replacementInode = (await lstat(replacement)).ino; + await rename(replacement, target); + }; + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect((await lstat(path)).ino).toBe(replacementInode); +}); + +it("detects a replacement of the original file even when its contents are identical", async () => { + const root = await createTestTempDirectory("rea-config-original-replaced-"); + const path = join(root, "config.json"); + const replacement = join(root, "replacement.json"); + await writeFile(path, "{}\n"); + let replacementInode: number | undefined; + writers.before = async (target) => { + await writeFile(replacement, await readFile(target)); + replacementInode = (await lstat(replacement)).ino; + await rename(replacement, target); + }; + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect((await lstat(path)).ino).toBe(replacementInode); + expect(await readFile(path, "utf8")).toBe("{}\n"); +}); + +it.skipIf(process.platform === "win32")( + "does not follow a retargeted configuration symlink", + async () => { + const root = await createTestTempDirectory("rea-config-retargeted-"); + const path = join(root, "config.json"); + const first = join(root, "first.json"); + const second = join(root, "second.json"); + await writeFile(first, "{}\n"); + await writeFile(second, '{"external":true}\n'); + await symlink(first, path); + writers.before = async () => { + await rm(path); + await symlink(second, path); + }; + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(first, "utf8")).toBe("{}\n"); + expect(await readFile(second, "utf8")).toBe('{"external":true}\n'); + }, +); + +it.each(["absolute", "relative"])( + "merges queued setup requests using a %s path to the same configuration", + async (pathKind) => { + const root = await createTestTempDirectory("rea-config-queued-"); + const path = join(root, "config.json"); + const client = { + name: "cursor", + configPath: path, + format: "json", + } as const; + const results = await Promise.all([ + configureClientConfiguration(client, { FIRST: "1" }), + configureClientConfiguration( + { + ...client, + configPath: + pathKind === "relative" ? relative(process.cwd(), path) : path, + }, + { SECOND: "2" }, + ), + ]); + + expect(results.every((result) => result.status === "configured")).toBe( + true, + ); + expect(JSON.parse(await readFile(path, "utf8"))).toMatchObject({ + mcpServers: { rea: { env: { FIRST: "1", SECOND: "2" } } }, + }); + }, +); + +it.skipIf(process.platform === "win32")( + "serializes setup through a directory symlink and its real directory", + async () => { + for (const presence of ["missing", "existing"] as const) { + const root = await createTestTempDirectory( + `rea-config-dir-alias-${presence}-`, + ); + const realDirectory = join(root, "real"); + const alias = join(root, "alias"); + await mkdir(realDirectory); + await symlink(realDirectory, alias, "dir"); + const realPath = join(realDirectory, "config.json"); + const aliasPath = join(alias, "config.json"); + if (presence === "existing") await writeFile(realPath, "{}\n"); + + const entered: string[] = []; + let releaseFirst!: () => void; + const release = new Promise((resolve) => { + releaseFirst = resolve; + }); + let markStarted!: () => void; + const started = new Promise((resolve) => { + markStarted = resolve; + }); + writers.before = async (target) => { + entered.push(target); + if (entered.length === 1) { + markStarted(); + await release; + } + }; + + const first = configureClientConfiguration( + { name: "cursor", configPath: realPath, format: "json" }, + { FIRST: "1" }, + ); + try { + await started; + const second = configureClientConfiguration( + { name: "cursor", configPath: aliasPath, format: "json" }, + { SECOND: "2" }, + ); + const secondReachedWriter = await new Promise((resolve) => { + let giveUp: ReturnType | undefined; + const timer = setInterval(() => { + if (entered.length > 1) { + clearInterval(timer); + clearTimeout(giveUp); + resolve(true); + } + }, 15); + giveUp = setTimeout(() => { + clearInterval(timer); + resolve(false); + }, 1_000); + }); + expect(secondReachedWriter).toBe(false); + expect(entered).toEqual([realPath]); + releaseFirst(); + const [firstResult, secondResult] = await Promise.all([first, second]); + expect([firstResult.status, secondResult.status]).toEqual([ + "configured", + "configured", + ]); + expect(JSON.parse(await readFile(realPath, "utf8"))).toMatchObject({ + mcpServers: { rea: { env: { FIRST: "1", SECOND: "2" } } }, + }); + expect(await readFile(aliasPath, "utf8")).toBe( + await readFile(realPath, "utf8"), + ); + } finally { + releaseFirst(); + writers.before = undefined; + } + } + }, +); + +it.skipIf(process.platform === "win32")( + "preserves a concurrent permission change even when the configuration bytes match", + async () => { + const root = await createTestTempDirectory("rea-config-mode-conflict-"); + const path = join(root, "config.json"); + const original = "{}\n"; + await writeFile(path, original, { mode: 0o600 }); + writers.before = (target) => chmod(target, 0o640); + + expect( + await configureClientConfiguration({ + name: "cursor", + configPath: path, + format: "json", + }), + ).toMatchObject({ status: "failed", reason: "conflict" }); + expect(await readFile(path, "utf8")).toBe(original); + expect((await lstat(path)).mode & 0o777).toBe(0o640); + expect(await readFile(`${path}.rea.backup`, "utf8")).toBe(original); + }, +); + +it("releases a failed setup transaction so a queued request can configure the file", async () => { + const root = await createTestTempDirectory("rea-config-queued-failure-"); + const path = join(root, "config.json"); + const client = { name: "cursor", configPath: path, format: "json" } as const; + let markStarted!: () => void; + const started = new Promise((resolve) => { + markStarted = resolve; + }); + let releaseWriter!: () => void; + const release = new Promise((resolve) => { + releaseWriter = resolve; + }); + writers.before = async () => { + writers.before = undefined; + markStarted(); + await release; + throw new Error("The first writer could not stage its configuration"); + }; + + const first = configureClientConfiguration(client, { FIRST: "1" }); + await started; + const second = configureClientConfiguration(client, { SECOND: "2" }); + releaseWriter(); + const [failed, queued] = await Promise.all([first, second]); + + expect(failed).toEqual({ status: "failed", reason: "write" }); + expect(queued.status).toBe("configured"); + expect(JSON.parse(await readFile(path, "utf8")).mcpServers.rea.env).toEqual({ + SECOND: "2", + }); + expect(await readdir(root)).toEqual(["config.json"]); + expect(await configureClientConfiguration(client)).toEqual({ + status: "unchanged", + }); +}); diff --git a/tests/process-global/setupConfigurationRollback.test.ts b/tests/process-global/setupConfigurationRollback.test.ts index 1c5a8020e7..07ef98f844 100644 --- a/tests/process-global/setupConfigurationRollback.test.ts +++ b/tests/process-global/setupConfigurationRollback.test.ts @@ -395,7 +395,7 @@ it.each(["existing", "first-run"] as const)( expect(await fs.readFile(replacement, "utf8")).toBe(foreign); expect((await fs.lstat(path)).isSymbolicLink()).toBe(true); - expect(result).toMatchObject({ status: "failed", reason: "rollback" }); + expect(result).toMatchObject({ status: "failed", reason: "conflict" }); if (kind === "existing") expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); }, @@ -423,7 +423,7 @@ it("retains a replacement regular file when readback fails", async () => { ); expect(await fs.readFile(path, "utf8")).toBe(foreign); - expect(result).toMatchObject({ status: "failed", reason: "rollback" }); + expect(result).toMatchObject({ status: "failed", reason: "conflict" }); expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); }); @@ -502,7 +502,7 @@ it("rejects a replaced publication even when the replacement passes registration {}, ["rea", "mcp"], ), - ).toMatchObject({ status: "failed", reason: "rollback" }); + ).toMatchObject({ status: "failed", reason: "conflict" }); expect(await fs.readFile(path, "utf8")).toBe(foreign); expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); }); @@ -525,7 +525,7 @@ it("retains concurrent edits to the published inode rather than overwriting them {}, ["rea", "mcp"], ), - ).toMatchObject({ status: "failed", reason: "rollback" }); + ).toMatchObject({ status: "failed", reason: "conflict" }); expect(await fs.readFile(path, "utf8")).toBe(edited); expect(await fs.readFile(`${path}.rea.backup`, "utf8")).toBe(original); });