From 5ea2c280b8f21d8ca11c07bf7c987d1b2d8ba8aa Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 25 Oct 2026 04:36:04 +0800 Subject: [PATCH] refactor(doctor): retain parsed configuration diagnostics Return native admission issues and an explicit Hopper timeout result from the configuration owner. Doctor consumes those parsed facts instead of raw timeout text plus a separate issue list. Reuse a successful aggregate parse; validate the individual timeout independently when other settings reject it. Preserve aggregate error precedence, JEB isolation, omission, constraints, causes and provider scope. Keep partial diagnostics when unrelated NUL, loader arguments, language selection or timeout settings are invalid. Validation: configuration and Doctor workflow regressions, mixed-invalid settings, full static/docs checks. No installed provider was exercised. --- docs/design/limits-and-recovery.md | 6 +- src/application/Doctor.test.ts | 98 ++++++++++++++++++++++------ src/application/Doctor.ts | 19 ++---- src/application/DoctorDiagnostics.ts | 17 ++--- src/config.test.ts | 39 +++++++++-- src/config/parseConfig.ts | 51 ++++++++++++--- 6 files changed, 172 insertions(+), 58 deletions(-) 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. */