From c4d3d100c316e5968ce2f976b6854ad8790c7d06 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Fri, 23 Oct 2026 23:07:14 -0400 Subject: [PATCH] fix(agents): keep a value-free diagnostic on adoption marker access refusals Follow-ups to #452 (HACK-1390) on the target inspector. - LegacyComposeAdoptionSelectionError's `permission` and `input-access` kinds now keep the same non-enumerable class/code diagnostic as ProjectInputAccessError (read with legacyComposeAdoptionSelectionDiagnostic). The helpers move to src/lib/value-free-diagnostic.ts so both errors share them. - Pin that inputPresent() classifies through projectInputAccessFault by faulting the real presence check with EPERM, EACCES and EIO. - Paired CLI/MCP fixtures now cover .hack and .hack/.internal at modes 000 and 0644, matching the proof-matrix row. - The input_permission_denied message assertions run unconditionally, also as root. Refs HACK-1415 Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-target-result-contract.md | 7 +- src/lib/native-compose-adoption-marker.ts | 50 +++++- src/lib/project-input-selection.ts | 40 ++--- src/lib/value-free-diagnostic.ts | 51 ++++++ tests/project-input-selection.test.ts | 171 +++++++++++++++++++- tests/project-target-inspect.test.ts | 63 +++++--- 6 files changed, 325 insertions(+), 57 deletions(-) create mode 100644 src/lib/value-free-diagnostic.ts diff --git a/docs/reference/ax-target-result-contract.md b/docs/reference/ax-target-result-contract.md index 81a049992..bce008873 100644 --- a/docs/reference/ax-target-result-contract.md +++ b/docs/reference/ax-target-result-contract.md @@ -178,6 +178,9 @@ When the guard cannot check whether a project input exists (for example a `.dev` file, or a `.dev` directory the caller cannot search, beside a valid `.hack`), the refusal is `input_permission_denied` (`EACCES`, `EPERM`) or `input_unreadable` (any other fault), like a read fault. +The filesystem guard refusals keep the fault's class and errno code (for example +`Error:EACCES`) only as an internal, value-free diagnostic for tests and +debugging; it never appears in the envelope or the printed error. `retry` is advice for the caller. Like suggested actions and application logs, it carries no execution authority. @@ -225,8 +228,8 @@ WU21). The inspector itself only produces snapshots. | Malformed Hack config or Compose file, oversized input | `config_parse_failed`, `compose_parse_failed`, `generation_input_too_large` | same | paired fixture | | Unreadable input (directory in place of a file, no read permission) | `input_unreadable`, `input_permission_denied` | same | paired fixture (permission case skipped as root) | | Input guard refusals (native/legacy conflict, unsafe adoption state, non-directory project dir) | `project_inputs_unsupported`, `adoption_state_unsafe`, `input_unreadable` | same | paired fixture | -| Unsearchable `.hack` or `.hack/.internal` on a never-adopted project | `input_permission_denied` (permission fix named first) | same | paired fixture and domain test (skipped as root) | -| Uncheckable input presence (`.dev` file or unsearchable `.dev` beside a valid `.hack`) | `input_unreadable`, `input_permission_denied` | same | paired fixture and domain test (unsearchable case skipped as root; `EPERM` through the classifier seam) | +| Unsearchable `.hack` or `.hack/.internal` on a never-adopted project | `input_permission_denied` (permission fix named first) | same | paired fixture and domain test, each at mode `000` and `0644` on both directories (skipped as root; the message is checked unconditionally) | +| Uncheckable input presence (`.dev` file or unsearchable `.dev` beside a valid `.hack`) | `input_unreadable`, `input_permission_denied` | same | paired fixture and domain test (unsearchable case skipped as root; `EPERM` through the classifier seam and an injected presence-check fault) | | Detached linked worktree, no project, missing path | closed refusals | same | paired fixture | | Unknown service | `service_not_found` | same | paired fixture | | Config changed during read | `generation_changed_during_read` | same code path | domain test | diff --git a/src/lib/native-compose-adoption-marker.ts b/src/lib/native-compose-adoption-marker.ts index b10faddc9..731a4fa5f 100644 --- a/src/lib/native-compose-adoption-marker.ts +++ b/src/lib/native-compose-adoption-marker.ts @@ -13,6 +13,11 @@ import { sameFile, } from "./native-compose-private-state.ts"; import { parseImportDocument } from "./native-config-import-parser.ts"; +import { + attachValueFreeDiagnostic, + readValueFreeDiagnostic, + valueFreeErrorDiagnostic, +} from "./value-free-diagnostic.ts"; export type LegacyComposeAdoptionSelection = | "prepared" @@ -32,13 +37,26 @@ export const LEGACY_ADOPTION_SELECTION_MESSAGES = { permission: "Cannot inspect Hack project inputs: permission denied on .hack or a directory inside it. Make .hack readable and searchable by your user (for example chmod u+rwx .hack), then retry. Values omitted.", } as const; -/** Private owner markers fence upgraded discovery without parsing authored inputs or contacting an engine. */ +const DEFAULT_DIAGNOSTIC = "LegacyComposeAdoptionSelectionError"; + +/** + * Private owner markers fence upgraded discovery without parsing authored + * inputs or contacting an engine. + * + * The `permission` and `input-access` kinds are filesystem faults, so, like + * `ProjectInputAccessError`, they keep the fault only as a value-free + * diagnostic: its class name and errno code (for example `Error:EACCES`), + * never its message or path, as a non-enumerable own property. Read it with + * {@link legacyComposeAdoptionSelectionDiagnostic}. The `adoption` kind keeps + * none: its causes may come from parsed private state. + */ export class LegacyComposeAdoptionSelectionError extends HackCliError { /** Which refusal this is, so readers can classify it without its message. */ readonly kind: keyof typeof LEGACY_ADOPTION_SELECTION_MESSAGES; constructor( - kind: keyof typeof LEGACY_ADOPTION_SELECTION_MESSAGES = "adoption" + kind: keyof typeof LEGACY_ADOPTION_SELECTION_MESSAGES = "adoption", + diagnostic: string = DEFAULT_DIAGNOSTIC ) { super({ code: kind === "permission" ? "E_PERMISSION" : "E_CONFIG_INVALID", @@ -46,8 +64,29 @@ export class LegacyComposeAdoptionSelectionError extends HackCliError { }); this.name = "LegacyComposeAdoptionSelectionError"; this.kind = kind; + if (kind !== "adoption") { + attachValueFreeDiagnostic({ + target: this, + diagnostic, + fallback: DEFAULT_DIAGNOSTIC, + }); + } } } + +/** + * The value-free diagnostic of a `permission` or `input-access` + * {@link LegacyComposeAdoptionSelectionError} (for example `Error:ENOTDIR`), + * for tests and debugging. Undefined for any other value. + */ +export function legacyComposeAdoptionSelectionDiagnostic( + error: unknown +): string | undefined { + return error instanceof LegacyComposeAdoptionSelectionError + ? readValueFreeDiagnostic(error) + : undefined; +} + function refuse(): never { throw new LegacyComposeAdoptionSelectionError(); } @@ -101,6 +140,7 @@ export async function inspectLegacyComposeAdoptionSelection(opts: { }): Promise { const directories: HeldDirectory[] = []; let stateRootFailure: StateRootFailure | null = null; + let stateRootDiagnostic = DEFAULT_DIAGNOSTIC; try { if ( !isRecord(opts) || @@ -120,6 +160,7 @@ export async function inspectLegacyComposeAdoptionSelection(opts: { return null; } stateRootFailure = classifyStateRootFailure(error); + stateRootDiagnostic = valueFreeErrorDiagnostic(error); throw error; } for (const path of [ @@ -157,7 +198,10 @@ export async function inspectLegacyComposeAdoptionSelection(opts: { return status; } catch { if (stateRootFailure !== null) { - throw new LegacyComposeAdoptionSelectionError(stateRootFailure); + throw new LegacyComposeAdoptionSelectionError( + stateRootFailure, + stateRootDiagnostic + ); } refuse(); } finally { diff --git a/src/lib/project-input-selection.ts b/src/lib/project-input-selection.ts index 9f99cb312..0d38126a4 100644 --- a/src/lib/project-input-selection.ts +++ b/src/lib/project-input-selection.ts @@ -11,6 +11,11 @@ import { HackCliError } from "./cli-result.ts"; import { pathExists } from "./fs.ts"; import { isRecord } from "./guards.ts"; import { assertLegacyComposeAdoptionSelectionStable } from "./native-compose-adoption-marker.ts"; +import { + attachValueFreeDiagnostic, + readValueFreeDiagnostic, + valueFreeErrorDiagnostic, +} from "./value-free-diagnostic.ts"; export const NATIVE_PROJECT_FILENAME = "hack.project.json"; type ProjectDirName = @@ -57,11 +62,6 @@ export class ProjectInputSelectionError extends HackCliError { } } -/** Non-enumerable property holding a {@link ProjectInputAccessError}'s diagnostic. */ -const PROJECT_INPUT_ACCESS_DIAGNOSTIC_PROPERTY = "diagnosticCause"; -const DIAGNOSTIC_NAME = /^[A-Za-z][A-Za-z0-9]{0,63}$/; -const DIAGNOSTIC_CODE = /^[A-Z][A-Z0-9_]{0,63}$/; -const DIAGNOSTIC = /^[A-Za-z][A-Za-z0-9]{0,63}(?::[A-Z][A-Z0-9_]{0,63})?$/; const DEFAULT_DIAGNOSTIC = "ProjectInputAccessError"; /** @@ -91,9 +91,10 @@ export class ProjectInputAccessError extends HackCliError { }); this.name = "ProjectInputAccessError"; this.access = access; - Object.defineProperty(this, PROJECT_INPUT_ACCESS_DIAGNOSTIC_PROPERTY, { - value: DIAGNOSTIC.test(diagnostic) ? diagnostic : DEFAULT_DIAGNOSTIC, - enumerable: false, + attachValueFreeDiagnostic({ + target: this, + diagnostic, + fallback: DEFAULT_DIAGNOSTIC, }); } } @@ -105,15 +106,8 @@ export class ProjectInputAccessError extends HackCliError { export function projectInputAccessDiagnostic( error: unknown ): string | undefined { - if (!(error instanceof ProjectInputAccessError)) { - return undefined; - } - const diagnostic: unknown = Reflect.get( - error, - PROJECT_INPUT_ACCESS_DIAGNOSTIC_PROPERTY - ); - return typeof diagnostic === "string" && DIAGNOSTIC.test(diagnostic) - ? diagnostic + return error instanceof ProjectInputAccessError + ? readValueFreeDiagnostic(error) : undefined; } @@ -133,18 +127,6 @@ export function projectInputAccessFault( ); } -/** Class name and identifier-shaped code of `error`; never its message. */ -function valueFreeErrorDiagnostic(error: unknown): string { - if (!(error instanceof Error)) { - return error === null ? "null" : typeof error; - } - const name = DIAGNOSTIC_NAME.test(error.name) ? error.name : "Error"; - const code: unknown = Reflect.get(error, "code"); - return typeof code === "string" && DIAGNOSTIC_CODE.test(code) - ? `${name}:${code}` - : name; -} - /** * Inspect only the named root, without parsing, following authored-file links, * searching ancestors, touching registration, or normalizing configuration. diff --git a/src/lib/value-free-diagnostic.ts b/src/lib/value-free-diagnostic.ts new file mode 100644 index 000000000..b85d9268b --- /dev/null +++ b/src/lib/value-free-diagnostic.ts @@ -0,0 +1,51 @@ +/** + * Value-free diagnostics for closed refusals. + * + * A closed refusal keeps a fixed, value-free message, so the fault behind it + * would otherwise be lost to tests and debugging. These helpers keep only the + * fault's class name and identifier-shaped errno code (for example + * `Error:EACCES`), never its message or path, as a non-enumerable own property + * rather than `cause`. Envelopes, printed errors and serializers therefore stay + * unchanged. + */ + +/** Non-enumerable own property holding a refusal's diagnostic. */ +const DIAGNOSTIC_PROPERTY = "diagnosticCause"; +const DIAGNOSTIC_NAME = /^[A-Za-z][A-Za-z0-9]{0,63}$/; +const DIAGNOSTIC_CODE = /^[A-Z][A-Z0-9_]{0,63}$/; +const DIAGNOSTIC = /^[A-Za-z][A-Za-z0-9]{0,63}(?::[A-Z][A-Z0-9_]{0,63})?$/; + +/** Class name and identifier-shaped code of `error`; never its message. */ +export function valueFreeErrorDiagnostic(error: unknown): string { + if (!(error instanceof Error)) { + return error === null ? "null" : typeof error; + } + const name = DIAGNOSTIC_NAME.test(error.name) ? error.name : "Error"; + const code: unknown = Reflect.get(error, "code"); + return typeof code === "string" && DIAGNOSTIC_CODE.test(code) + ? `${name}:${code}` + : name; +} + +/** + * Attach `diagnostic` to `target` as a non-enumerable own property, or + * `fallback` when `diagnostic` is not identifier-shaped. + */ +export function attachValueFreeDiagnostic(opts: { + readonly target: object; + readonly diagnostic: string; + readonly fallback: string; +}): void { + Object.defineProperty(opts.target, DIAGNOSTIC_PROPERTY, { + value: DIAGNOSTIC.test(opts.diagnostic) ? opts.diagnostic : opts.fallback, + enumerable: false, + }); +} + +/** The attached diagnostic of `target`, or undefined when none is valid. */ +export function readValueFreeDiagnostic(target: object): string | undefined { + const diagnostic: unknown = Reflect.get(target, DIAGNOSTIC_PROPERTY); + return typeof diagnostic === "string" && DIAGNOSTIC.test(diagnostic) + ? diagnostic + : undefined; +} diff --git a/tests/project-input-selection.test.ts b/tests/project-input-selection.test.ts index 32244ef66..a215536b3 100644 --- a/tests/project-input-selection.test.ts +++ b/tests/project-input-selection.test.ts @@ -1,4 +1,5 @@ -import { afterEach, expect, test } from "bun:test"; +import { afterEach, expect, mock, spyOn, test } from "bun:test"; +import * as fs from "node:fs/promises"; import { chmod, mkdir, @@ -17,6 +18,7 @@ import { import { inspectLegacyComposeAdoptionSelection, LegacyComposeAdoptionSelectionError, + legacyComposeAdoptionSelectionDiagnostic, } from "../src/lib/native-compose-adoption-marker.ts"; import { findProjectContext, @@ -35,6 +37,7 @@ import { const roots: string[] = []; afterEach(async () => { + mock.restore(); for (const root of roots.splice(0)) { await rm(root, { recursive: true, force: true }); } @@ -217,9 +220,10 @@ async function inputAccessFault(root: string): Promise { function expectValueFreeDiagnostic( error: unknown, root: string, - diagnostic: string + diagnostic: string, + read: (error: unknown) => string | undefined = projectInputAccessDiagnostic ): void { - expect(projectInputAccessDiagnostic(error)).toBe(diagnostic); + expect(read(error)).toBe(diagnostic); expect((error as Error).cause).toBeUndefined(); expect(Object.keys(error as object)).not.toContain("diagnosticCause"); const code = diagnostic.split(":")[1] ?? diagnostic; @@ -280,6 +284,167 @@ test("EPERM and EACCES classify as permission; other faults as unreadable", () = expect(projectInputAccessDiagnostic(new Error("other"))).toBeUndefined(); }); +/** A synthetic filesystem fault shaped like Bun's, with a path in its message. */ +function fsFault(code: string, path: string): Error { + return Object.assign( + new Error(`${code}: operation failed, lstat '${path}'`), + { + code, + syscall: "lstat", + path, + } + ); +} + +/** Make `lstat` of exactly `path` fail with `code`; other paths are real. */ +function failLstat(path: string, code: string): void { + const realLstat = fs.lstat; + spyOn(fs, "lstat").mockImplementation((( + ...args: Parameters + ) => + args[0] === path + ? Promise.reject(fsFault(code, path)) + : realLstat(...args)) as typeof fs.lstat); +} + +test("input presence checks classify through the projectInputAccessFault seam", async () => { + // HACK-1415: the seam test above proves the classifier, not that + // inputPresent() uses it. Fault the real presence check of one input so a + // re-inlined classifier that drops EPERM (or the diagnostic) fails here. + for (const [code, access] of [ + ["EPERM", "permission"], + ["EACCES", "permission"], + ["EIO", "unreadable"], + ] as const) { + const root = await fixture(); + await file(root, ".hack/docker-compose.yml"); + failLstat(join(root, ".dev", "docker-compose.yml"), code); + const error = await inputAccessFault(root); + mock.restore(); + expect(error).toBeInstanceOf(ProjectInputAccessError); + expect({ + code, + access: (error as ProjectInputAccessError).access, + }).toEqual({ code, access }); + expectValueFreeDiagnostic(error, root, `Error:${code}`); + } +}); + +async function adoptionSelectionFault(root: string): Promise { + try { + await inspectLegacyComposeAdoptionSelection({ projectRoot: root }); + } catch (error: unknown) { + return error; + } + throw new Error("unexpected adoption selection success"); +} + +test("a file where .hack belongs keeps a value-free adoption marker diagnostic", async () => { + // HACK-1415: the adoption marker's filesystem refusals carry the same + // class/code diagnostic as ProjectInputAccessError. + const root = await fixture(); + await file(root, ".hack", "not a directory"); + for (const error of [ + await adoptionSelectionFault(root), + await inputAccessFault(root), + ]) { + expect(error).toBeInstanceOf(LegacyComposeAdoptionSelectionError); + expect((error as LegacyComposeAdoptionSelectionError).kind).toBe( + "input-access" + ); + expectValueFreeDiagnostic( + error, + root, + "Error:ENOTDIR", + legacyComposeAdoptionSelectionDiagnostic + ); + } +}); + +test("EPERM on the adoption state root is a permission refusal with its diagnostic", async () => { + // A fixture cannot produce EPERM, so fault the marker's state-root lstat. + const root = await fixture(); + await file(root, ".hack/docker-compose.yml"); + failLstat(join(root, ".hack/.internal/legacy-compose-adoption-v1"), "EPERM"); + const error = await adoptionSelectionFault(root); + expect(error).toBeInstanceOf(LegacyComposeAdoptionSelectionError); + expect((error as LegacyComposeAdoptionSelectionError).kind).toBe( + "permission" + ); + expect((error as LegacyComposeAdoptionSelectionError).code).toBe( + "E_PERMISSION" + ); + expectValueFreeDiagnostic( + error, + root, + "Error:EPERM", + legacyComposeAdoptionSelectionDiagnostic + ); +}); + +test("only filesystem adoption marker refusals carry a diagnostic", () => { + expect( + legacyComposeAdoptionSelectionDiagnostic( + new LegacyComposeAdoptionSelectionError() + ) + ).toBeUndefined(); + expect( + legacyComposeAdoptionSelectionDiagnostic( + new LegacyComposeAdoptionSelectionError( + "permission", + "/private/fixture/.hack" + ) + ) + ).toBe("LegacyComposeAdoptionSelectionError"); + expect( + legacyComposeAdoptionSelectionDiagnostic( + new LegacyComposeAdoptionSelectionError("input-access") + ) + ).toBe("LegacyComposeAdoptionSelectionError"); + expect( + legacyComposeAdoptionSelectionDiagnostic( + new ProjectInputAccessError("permission", "Error:EACCES") + ) + ).toBeUndefined(); +}); + +// Root can search a mode-000 or mode-0644 directory, so this fault cannot be +// produced. +test.skipIf(process.getuid?.() === 0)( + "an unsearchable .hack or .hack/.internal keeps a value-free adoption marker diagnostic", + async () => { + for (const [dir, mode] of [ + [".hack", 0o000], + [".hack", 0o644], + [".hack/.internal", 0o000], + [".hack/.internal", 0o644], + ] as const) { + const root = await fixture(); + await file(root, ".hack/docker-compose.yml"); + await mkdir(join(root, ".hack/.internal")); + const locked = join(root, dir); + await chmod(locked, mode); + try { + const error = await inputAccessFault(root); + expect(error).toBeInstanceOf(LegacyComposeAdoptionSelectionError); + expect({ + dir, + mode, + kind: (error as LegacyComposeAdoptionSelectionError).kind, + }).toEqual({ dir, mode, kind: "permission" }); + expectValueFreeDiagnostic( + error, + root, + "Error:EACCES", + legacyComposeAdoptionSelectionDiagnostic + ); + } finally { + await chmod(locked, 0o755); + } + } + } +); + // Root can search a mode-000 directory, so this fault cannot be produced. test.skipIf(process.getuid?.() === 0)( "an unsearchable .dev beside valid .hack inputs is a typed permission refusal", diff --git a/tests/project-target-inspect.test.ts b/tests/project-target-inspect.test.ts index 9f90e5fb6..953bde0bb 100644 --- a/tests/project-target-inspect.test.ts +++ b/tests/project-target-inspect.test.ts @@ -256,6 +256,8 @@ beforeAll(async () => { name: "unsearchable", services: ["api"], }); + // Never adopted: .hack/.internal exists only so its mode can be dropped. + await mkdir(join(root, "unsearchable", ".hack", ".internal")); // Native and legacy inputs side by side (a conflicting project family). const nativeConflict = join(root, "native-conflict"); await writeProject({ @@ -702,22 +704,38 @@ describe("paired CLI/MCP target fixtures", () => { CASE_TIMEOUT_MS ); - // Root can search a mode-000 directory, so this fault cannot be produced. + // Root can search a mode-000 or mode-0644 directory, so this fault cannot be + // produced. test.skipIf(process.getuid?.() === 0)( - "an unsearchable project directory is a closed refusal without the path", + "an unsearchable .hack or .hack/.internal is a closed refusal without the path", async () => { const path = join(fixture.root, "unsearchable"); - const projectDir = join(path, ".hack"); - await chmod(projectDir, 0o000); - try { - // An unsearchable .hack is a permission fault (#402's `permission` - // selection kind), not unsafe adoption state. - expectRefusal(await inspectBoth({ path }), "input_permission_denied"); - const cli = await runCli({ path }); - expect(JSON.stringify(cli.envelope)).not.toContain(fixture.root); - expect(JSON.stringify(cli.envelope)).not.toContain("EACCES"); - } finally { - await chmod(projectDir, 0o755); + // HACK-1415: every case the proof matrix names runs on both surfaces, + // on a project that never used adoption. + for (const [dir, mode] of [ + [".hack", 0o000], + [".hack", 0o644], + [".hack/.internal", 0o000], + [".hack/.internal", 0o644], + ] as const) { + const locked = join(path, dir); + await chmod(locked, mode); + try { + // An unsearchable project directory is a permission fault (#402's + // `permission` selection kind), not unsafe adoption state. + expect({ dir, mode, result: await inspectBoth({ path }) }).toEqual({ + dir, + mode, + result: targetRefusalEnvelope({ + reason: "input_permission_denied", + }), + }); + const cli = await runCli({ path }); + expect(JSON.stringify(cli.envelope)).not.toContain(fixture.root); + expect(JSON.stringify(cli.envelope)).not.toContain("EACCES"); + } finally { + await chmod(locked, 0o755); + } } }, CASE_TIMEOUT_MS @@ -1009,17 +1027,22 @@ describe("target inspector domain", () => { ); }); + test("the permission-denied refusal names the permission fix, not adoption recovery", () => { + // HACK-1390: a project that never used adoption must not be told to run + // adoption recovery when its project directories cannot be searched. The + // message does not depend on the user, so this also runs as root + // (HACK-1415). + const message = TARGET_REFUSALS.input_permission_denied.message; + expect(message).toContain("permission denied. Fix the permissions first"); + expect(message).toContain("chmod u+rwx .hack"); + expect(message.toLowerCase()).not.toContain("adoption"); + }); + // Root can search a mode-000 or mode-0644 directory, so this fault cannot be // produced. test.skipIf(process.getuid?.() === 0)( - "an unsearchable .hack or .hack/.internal names the permission fix, not adoption recovery", + "an unsearchable .hack or .hack/.internal is a permission refusal, not unsafe adoption state", async () => { - // HACK-1390: a project that never used adoption must not be told to run - // adoption recovery when its project directories cannot be searched. - const message = TARGET_REFUSALS.input_permission_denied.message; - expect(message).toContain("permission denied. Fix the permissions first"); - expect(message).toContain("chmod u+rwx .hack"); - expect(message.toLowerCase()).not.toContain("adoption"); for (const [dir, mode] of [ [".hack", 0o000], [".hack", 0o644],