diff --git a/docs/design/limits-and-recovery.md b/docs/design/limits-and-recovery.md index 4ed7a49ace..94c507e6b2 100644 --- a/docs/design/limits-and-recovery.md +++ b/docs/design/limits-and-recovery.md @@ -18,8 +18,10 @@ default, any floor, the maximum, the schema, the parser, and the recovery text. Register that schema on `environmentSchema` in [`environment.ts`](https://github.com/morluto/rea/blob/main/src/config/environment.ts). `parseConfig` calls `parseEnvironment`. Doctor uses that same parser through -`nativeAnalysisConfigurationError`, so startup and `rea doctor` accept the -same values. Add the name to `SELECTED_REGISTRATION_SETTINGS` so `rea setup` +`inspectNativeAnalysisConfiguration`, which also preserves a supplied Hopper +timeout's parsed diagnostic when another setting is invalid. Startup and +`rea doctor` accept the same values. Add the name to +`SELECTED_REGISTRATION_SETTINGS` so `rea setup` copies it into the client registration. Unset keeps today's behavior. An invalid value is a `ConfigurationError` and projects as `configuration_invalid`. Do not fall back to the default. diff --git a/src/application/Doctor.test.ts b/src/application/Doctor.test.ts index 683634ed08..5a1b36cb71 100644 --- a/src/application/Doctor.test.ts +++ b/src/application/Doctor.test.ts @@ -12,6 +12,16 @@ import { HopperReleaseError } from "./HopperReleaseFeed.js"; import { MAC_HOPPER_SYSTEM_LAUNCHER } from "./MacHopperPaths.js"; import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; +const configurationHost = ( + environment: Readonly, +): DoctorHost => ({ + ...systemDoctorHost({ environment: { ...environment } }), + ...host({ + installationPaths: () => Promise.resolve([]), + clientRegistrations: () => Promise.resolve([]), + }), +}); + describe("doctor Hopper startup timeout", () => { const timeoutHost = ( value: string | undefined, @@ -157,16 +167,6 @@ describe("doctor Hopper startup timeout", () => { }); describe("doctor native analysis configuration", () => { - const configurationHost = ( - environment: Readonly, - ): DoctorHost => ({ - ...systemDoctorHost({ environment: { ...environment } }), - ...host({ - installationPaths: () => Promise.resolve([]), - clientRegistrations: () => Promise.resolve([]), - }), - }); - it.each([ ["REA_ANALYSIS_PROVIDER", "Hopper"], ["REA_REGEX_MATCHING_DEADLINE_MS", "0"], @@ -202,9 +202,78 @@ describe("doctor native analysis configuration", () => { details: { setting }, }), ); + expect( + result.checks.some(({ name }) => name === "hopper-startup-timeout"), + ).toBe(false); }, ); + it.each([ + ["REA_GHIDRA_STARTUP_TIMEOUT_MS", "abc"], + ["HOPPER_LOADER_ARGS_JSON", "{"], + ["REA_GHIDRA_COMPILER_SPEC_ID", "gcc"], + ["CALLER_SETTING", "private\0value"], + ] as const)( + "keeps a valid supplied Hopper timeout when %s rejects native admission", + async (setting, value) => { + const result = await runDoctor( + undefined, + configurationHost({ + [setting]: value, + REA_HOPPER_STARTUP_TIMEOUT_MS: "120001", + }), + { providers: ["hopper"] }, + ); + expect(result.healthy).toBe(false); + expect(result.scope_checks).toContainEqual( + expect.objectContaining({ + name: "hopper-startup-timeout", + ok: true, + detail: "REA_HOPPER_STARTUP_TIMEOUT_MS=120001", + }), + ); + expect(result.scope_checks).toContainEqual( + expect.objectContaining({ + name: `runtime-configuration:${setting}`, + ok: false, + }), + ); + }, + ); + + it("reports an invalid Hopper timeout alongside an unrelated NUL admission refusal", async () => { + const result = await runDoctor( + undefined, + configurationHost({ + CALLER_SETTING: "private\0value", + REA_HOPPER_STARTUP_TIMEOUT_MS: "0", + }), + { providers: ["hopper"] }, + ); + expect(result.healthy).toBe(false); + expect(result.scope_checks).toContainEqual( + expect.objectContaining({ + name: "hopper-startup-timeout", + ok: false, + detail: + "REA_HOPPER_STARTUP_TIMEOUT_MS: must be a positive decimal integer", + }), + ); + expect(result.scope_checks).toContainEqual( + expect.objectContaining({ + name: "runtime-configuration:CALLER_SETTING", + ok: false, + detail: expect.stringContaining("NUL"), + }), + ); + expect( + result.scope_checks.filter( + ({ name }) => name === "hopper-startup-timeout", + ), + ).toHaveLength(1); + expect(JSON.stringify(result.checks)).not.toContain("private"); + }); + it("keeps a lowercase provider preference ready for the selected engine", async () => { const result = await runDoctor( undefined, @@ -257,15 +326,6 @@ describe("doctor native analysis configuration", () => { }); describe("doctor MCP startup readiness", () => { - const configurationHost = ( - environment: Readonly, - ): DoctorHost => ({ - ...systemDoctorHost({ environment: { ...environment } }), - ...host({ - installationPaths: () => Promise.resolve([]), - clientRegistrations: () => Promise.resolve([]), - }), - }); const alignedCodex: ClientRegistrationStatus = { client: "codex", config_path: "/home/user/.codex/config.toml", diff --git a/src/application/Doctor.ts b/src/application/Doctor.ts index 8249899f41..2f55203220 100644 --- a/src/application/Doctor.ts +++ b/src/application/Doctor.ts @@ -11,9 +11,11 @@ import { constants } from "node:fs"; import { access, stat } from "node:fs/promises"; import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import type { RetainedResourceScope } from "./AnalysisProvider.js"; -import { nativeAnalysisConfigurationError } from "../config/parseConfig.js"; +import { + inspectNativeAnalysisConfiguration, + type NativeAnalysisConfigurationInspection, +} from "../config/parseConfig.js"; import { homeDirectoryFromEnvironment } from "../config/homeDirectory.js"; -import type { ConfigurationSettingIssue } from "../domain/configurationErrors.js"; import { snapshotEnvironment } from "../process/snapshotEnvironment.js"; import { join } from "node:path"; @@ -108,12 +110,11 @@ export interface DoctorHost { readonly runtime: JavaScriptRuntime; readonly homeDirectory: string; readonly configuredHopperPath?: string; - readonly configuredHopperStartupTimeout?: string; /** * Settings `parseConfig` rejects before native analysis starts. * Undefined means this host did not run that admission. */ - readonly configurationIssues?: readonly ConfigurationSettingIssue[]; + readonly nativeConfiguration?: NativeAnalysisConfigurationInspection; readonly configuredIlspyCmdPath?: string; macosVersion(): Promise; linuxDistribution(): Promise; @@ -539,7 +540,7 @@ export const systemDoctorHost = ( const platform = options.platform ?? process.platform; const architecture = options.architecture ?? process.arch; const environment = options.environment ?? process.env; - const configurationError = nativeAnalysisConfigurationError( + const nativeConfiguration = inspectNativeAnalysisConfiguration( snapshotEnvironment(environment, platform), ); const hostExecFileOutput = options.execFileOutput ?? execFileOutput; @@ -576,13 +577,7 @@ export const systemDoctorHost = ( ...(environment.HOPPER_LAUNCHER_PATH === undefined ? {} : { configuredHopperPath: environment.HOPPER_LAUNCHER_PATH }), - ...(environment.REA_HOPPER_STARTUP_TIMEOUT_MS === undefined - ? {} - : { - configuredHopperStartupTimeout: - environment.REA_HOPPER_STARTUP_TIMEOUT_MS, - }), - configurationIssues: configurationError?.settings ?? [], + nativeConfiguration, ...(environment.REA_ILSPY_CMD_PATH === undefined ? {} : { configuredIlspyCmdPath: environment.REA_ILSPY_CMD_PATH }), diff --git a/src/application/DoctorDiagnostics.ts b/src/application/DoctorDiagnostics.ts index 3cbfc7e784..0713c6b602 100644 --- a/src/application/DoctorDiagnostics.ts +++ b/src/application/DoctorDiagnostics.ts @@ -2,7 +2,6 @@ import { SUPPORTED_NODE_VERSION_PROSE, supportsJavaScriptRuntime, } from "../domain/runtimeVersion.js"; -import { providerStartupTimeoutSchema } from "../config/providerStartupTimeout.js"; import { hopperLauncherDiscovery } from "./HopperLauncherDiscovery.js"; import { LINUX_HOPPER_SETUP_HOSTS, supportsReaHost } from "./LinuxHopper.js"; import type { @@ -193,16 +192,14 @@ const hopperCheck = ( const optionalHopperStartupTimeoutCheck = ( host: DoctorHost, ): DoctorCheck | undefined => { - if (host.configuredHopperStartupTimeout === undefined) return undefined; - const parsed = providerStartupTimeoutSchema.safeParse( - host.configuredHopperStartupTimeout, - ); + const parsed = host.nativeConfiguration?.configuredHopperStartupTimeout; + if (parsed === undefined) return undefined; return check( "hopper-startup-timeout", - parsed.success, - parsed.success - ? `REA_HOPPER_STARTUP_TIMEOUT_MS=${String(parsed.data)}` - : `REA_HOPPER_STARTUP_TIMEOUT_MS: ${parsed.error.issues.map(({ message }) => message).join("; ")}`, + parsed.ok, + parsed.ok + ? `REA_HOPPER_STARTUP_TIMEOUT_MS=${String(parsed.value)}` + : `REA_HOPPER_STARTUP_TIMEOUT_MS: ${parsed.error.settings.map(({ constraint }) => constraint).join("; ")}`, { classification: "config_drift", remediation: @@ -219,7 +216,7 @@ const optionalHopperStartupTimeoutCheck = ( const runtimeConfigurationChecks = ( host: DoctorHost, ): readonly DoctorCheck[] => { - const issues = host.configurationIssues; + const issues = host.nativeConfiguration?.issues; if (issues === undefined || issues.length === 0) return []; const grouped: { setting: string; constraints: string[] }[] = []; for (const issue of issues) { diff --git a/src/config.test.ts b/src/config.test.ts index a4261a5faf..939fcc4e88 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from "vitest"; import { defaultHopperLauncherPath, - nativeAnalysisConfigurationError, + inspectNativeAnalysisConfiguration, parseConfig, } from "./config/parseConfig.js"; import { @@ -100,16 +100,16 @@ describe("runtime configuration", () => { }); it("shares native admission with doctor and ignores the JEB-only timeout", () => { - expect(nativeAnalysisConfigurationError({})).toBeUndefined(); + expect(inspectNativeAnalysisConfiguration({})).toEqual({ issues: [] }); expect( - nativeAnalysisConfigurationError({ REA_JEB_MCP_TIMEOUT_MS: "abc" }), - ).toBeUndefined(); + inspectNativeAnalysisConfiguration({ REA_JEB_MCP_TIMEOUT_MS: "abc" }), + ).toEqual({ issues: [] }); expect(parseConfig({ REA_JEB_MCP_TIMEOUT_MS: "abc" }).ok).toBe(true); - const rejected = nativeAnalysisConfigurationError({ + const rejected = inspectNativeAnalysisConfiguration({ REA_ANALYSIS_PROVIDER: "Hopper", REA_GHIDRA_STARTUP_TIMEOUT_MS: "abc", }); - expect(rejected?.settings.map(({ setting }) => setting)).toEqual([ + expect(rejected.issues.map(({ setting }) => setting)).toEqual([ "REA_ANALYSIS_PROVIDER", "REA_GHIDRA_STARTUP_TIMEOUT_MS", ]); @@ -121,6 +121,33 @@ describe("runtime configuration", () => { ).toBe(false); }); + it("preserves the timeout's schema cause when aggregate admission stops at NUL", () => { + const inspected = inspectNativeAnalysisConfiguration({ + CALLER_SETTING: "private\0value", + REA_HOPPER_STARTUP_TIMEOUT_MS: "0", + }); + expect(inspected.issues).toEqual([ + { + setting: "CALLER_SETTING", + constraint: + "Environment variable names and values must not contain NUL bytes", + }, + ]); + const timeout = inspected.configuredHopperStartupTimeout; + if (timeout === undefined || timeout.ok) + throw new Error("Expected the configured timeout's independent refusal"); + expect(timeout.error.cause).toMatchObject({ + name: "ZodError", + issues: [{ message: "must be a positive decimal integer" }], + }); + expect(timeout.error.settings).toEqual([ + { + setting: "REA_HOPPER_STARTUP_TIMEOUT_MS", + constraint: "must be a positive decimal integer", + }, + ]); + }); + it("parses absolute BYO Ghidra and optional Java paths", () => { expect( parseConfig({ diff --git a/src/config/parseConfig.ts b/src/config/parseConfig.ts index 60235532b7..cdc35f9589 100644 --- a/src/config/parseConfig.ts +++ b/src/config/parseConfig.ts @@ -1,8 +1,12 @@ import { accessSync, constants, statSync } from "node:fs"; -import type { ConfigurationError } from "../domain/configurationErrors.js"; -import { ok, type Result } from "../domain/result.js"; +import { + ConfigurationError, + type ConfigurationSettingIssue, +} from "../domain/configurationErrors.js"; +import { err, ok, type Result } from "../domain/result.js"; import { parseEnvironment } from "./environment.js"; +import { providerStartupTimeoutSchema } from "./providerStartupTimeout.js"; import { homeDirectoryFromEnvironment } from "./homeDirectory.js"; import { DEFAULT_BRIDGE_MAX_REQUEST } from "./bridgeMaxRequestBytes.js"; import { DEFAULT_REGEX_MATCHING_DEADLINE } from "./regexMatchingDeadline.js"; @@ -48,15 +52,44 @@ const nativeAnalysisEnvironment = ( return appEnvironment; }; -/** - * The configuration error native commands return, without probing launchers. - * Doctor uses this so readiness and analysis admit the same settings. - */ -export const nativeAnalysisConfigurationError = ( +/** Native admission issues and an explicitly configured timeout's independent facts. */ +export interface NativeAnalysisConfigurationInspection { + readonly issues: readonly ConfigurationSettingIssue[]; + readonly configuredHopperStartupTimeout?: Result; +} + +/** Inspect native admission without probing launchers or losing mixed-invalid diagnostics. */ +export const inspectNativeAnalysisConfiguration = ( environment: Readonly>, -): ConfigurationError | undefined => { +): NativeAnalysisConfigurationInspection => { const parsed = parseEnvironment(nativeAnalysisEnvironment(environment)); - return parsed.ok ? undefined : parsed.error; + const issues = parsed.ok ? [] : parsed.error.settings; + const configuredTimeout = environment.REA_HOPPER_STARTUP_TIMEOUT_MS; + if (configuredTimeout === undefined) return { issues }; + if (parsed.ok) + return { + issues, + configuredHopperStartupTimeout: ok( + parsed.value.REA_HOPPER_STARTUP_TIMEOUT_MS, + ), + }; + // Another setting may reject the aggregate before this supplied timeout's + // facts are available. Its dedicated diagnostic still uses the same schema. + const timeout = providerStartupTimeoutSchema.safeParse(configuredTimeout); + return { + issues, + configuredHopperStartupTimeout: timeout.success + ? ok(timeout.data) + : err( + new ConfigurationError("Invalid REA environment configuration", { + cause: timeout.error, + settings: timeout.error.issues.map(({ message }) => ({ + setting: "REA_HOPPER_STARTUP_TIMEOUT_MS", + constraint: message, + })), + }), + ), + }; }; /** Parse provider configuration and runtime prerequisites at the composition root. */