From ded3b85b3ab839a91c4763eac4257f9131f5d72f Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 13:14:56 -0400 Subject: [PATCH 1/7] fix(agents): close project input guard refusals in the target inspector `hack project target` let the shared project input guards escape as detail-less errors. A project directory the caller cannot search, or an unparseable legacy adoption receipt, printed a detail-less E_CONFIG_INVALID envelope, which the MCP adapter turned into data: null. The inspector now maps those guard refusals wherever they are raised, including inside config reads: native or conflicting inputs become project_inputs_unsupported, an unsafe or uncheckable adoption state becomes the new provisional adoption_state_unsafe refusal, and an input access failure becomes input_unreadable. The adoption guard error now carries its kind so callers classify it without parsing its message. Paired CLI/MCP fixtures cover each case, and the MCP argv builder is pinned literally so a dropped selector is caught independently of the paired runs. Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-target-result-contract.md | 9 ++ src/lib/native-compose-adoption-marker.ts | 4 + src/lib/target-inspect.ts | 59 +++++++---- tests/project-target-inspect.test.ts | 103 ++++++++++++++++++++ 4 files changed, 155 insertions(+), 20 deletions(-) diff --git a/docs/reference/ax-target-result-contract.md b/docs/reference/ax-target-result-contract.md index 4a6fe2213..f2a852729 100644 --- a/docs/reference/ax-target-result-contract.md +++ b/docs/reference/ax-target-result-contract.md @@ -147,6 +147,7 @@ fixed text that never repeats caller input, paths or registry values. | `path_not_found` | `E_PROJECT_NOT_FOUND` | `project` | `fix_selector` | | `no_project_context` | `E_PROJECT_NOT_FOUND` | `project` | `fix_selector` | | `project_inputs_unsupported` | `E_CONFIG_INVALID` | `project` | `fix_project` | +| `adoption_state_unsafe` | `E_CONFIG_INVALID` | `project` | `fix_project` | | `config_parse_failed` | `E_CONFIG_PARSE` | `project` | `fix_project` | | `compose_parse_failed` | `E_CONFIG_PARSE` | `project` | `fix_project` | | `detached_checkout` | `E_USAGE` | `instance` | `fix_selector` | @@ -163,6 +164,13 @@ read permission, or a directory where a config file belongs) is reported as filesystem error). The OS message is never surfaced, because it contains the absolute path. +The shared project input guards that every project command runs are mapped +too. Native or conflicting project files are `project_inputs_unsupported`. A +legacy adoption state that is unparseable, interrupted, or cannot be checked +(for example a project directory the caller cannot search) is +`adoption_state_unsafe`. A non-directory where the project directory belongs is +`input_unreadable`. + `retry` is advice for the caller. Like suggested actions and application logs, it carries no execution authority. @@ -208,6 +216,7 @@ WU21). The inspector itself only produces snapshots. | Two registrations owning one checkout | `ambiguous_project` | same | paired fixture | | 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, unsearchable or non-directory project dir) | `project_inputs_unsupported`, `adoption_state_unsafe`, `input_unreadable` | same | paired fixture (unsearchable case skipped as root) | | 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 038402f97..498fb124f 100644 --- a/src/lib/native-compose-adoption-marker.ts +++ b/src/lib/native-compose-adoption-marker.ts @@ -21,6 +21,9 @@ export type LegacyComposeAdoptionSelection = | "rolled-back"; /** Private owner markers fence upgraded discovery without parsing authored inputs or contacting an engine. */ export class LegacyComposeAdoptionSelectionError extends HackCliError { + /** Which refusal this is, so readers can classify it without its message. */ + readonly kind: "adoption" | "input-access"; + constructor(kind: "adoption" | "input-access" = "adoption") { super({ code: "E_CONFIG_INVALID", @@ -30,6 +33,7 @@ export class LegacyComposeAdoptionSelectionError extends HackCliError { : "Legacy adoption selection is unsafe or interrupted. Use explicit adoption recovery; values omitted.", }); this.name = "LegacyComposeAdoptionSelectionError"; + this.kind = kind; } } function refuse(): never { diff --git a/src/lib/target-inspect.ts b/src/lib/target-inspect.ts index c4bdbf22d..b41227d0a 100644 --- a/src/lib/target-inspect.ts +++ b/src/lib/target-inspect.ts @@ -18,6 +18,7 @@ import { resolveGitCurrentBranch, } from "./git-worktree.ts"; import { isRecord } from "./guards.ts"; +import { LegacyComposeAdoptionSelectionError } from "./native-compose-adoption-marker.ts"; import { defaultProjectSlugFromPath, findProjectContext, @@ -170,6 +171,13 @@ export const TARGET_REFUSALS = Object.freeze({ message: "The selected project's inputs are not supported by this inspector (native or conflicting project files).", }, + adoption_state_unsafe: { + code: "E_CONFIG_INVALID", + stage: "project", + retry: "fix_project", + message: + "The project's legacy adoption state is unreadable, interrupted or unsafe. Use explicit adoption recovery.", + }, config_parse_failed: { code: "E_CONFIG_PARSE", stage: "project", @@ -357,8 +365,10 @@ export function targetRefusalEnvelope(opts: { * A filesystem fault while reading inputs (for example a permission-denied * Compose file, or a directory where a config file belongs) is mapped to the * closed `input_permission_denied` / `input_unreadable` refusals, so the OS - * message and its absolute path never reach the caller. Only non-filesystem - * faults (programming errors) still throw. + * message and its absolute path never reach the caller. The shared project + * input guards' own refusals (native or conflicting inputs, an unsafe or + * unreadable legacy adoption state) are mapped too, wherever they are raised. + * Only other faults (programming errors) still throw. * * `afterResolve` is a test seam that runs after the target is resolved and * before the generation is re-read; production callers never pass it. @@ -382,6 +392,10 @@ export async function inspectTarget(opts: { : { observedGeneration: error.observedGeneration }), }); } + const guarded = projectInputGuardReason(error); + if (guarded !== null) { + return targetRefusalEnvelope({ reason: guarded }); + } const fault = filesystemFaultCode(error); if (fault !== null) { return targetRefusalEnvelope({ @@ -395,6 +409,24 @@ export async function inspectTarget(opts: { } } +/** + * Closed reason for a refusal raised by the shared project input guards. Those + * guards run during discovery and again inside every config read, so they are + * classified here rather than at one call site. Their messages are value-free + * but are not catalogue text, so they are never passed through. + */ +function projectInputGuardReason(error: unknown): TargetRefusalReason | null { + if (error instanceof ProjectInputSelectionError) { + return "project_inputs_unsupported"; + } + if (error instanceof LegacyComposeAdoptionSelectionError) { + return error.kind === "input-access" + ? "input_unreadable" + : "adoption_state_unsafe"; + } + return null; +} + /** Shape of an errno code such as `EACCES` or `EISDIR`. */ const ERRNO_CODE_PATTERN = /^E[A-Z0-9]+$/; @@ -652,12 +684,10 @@ async function selectProject(opts: { if (!(await isDirectory(match.repoRoot))) { throw new TargetRefusal("registered_root_missing"); } - const context = await guardInputs(() => - findProjectContextAtRoot({ - projectRoot: match.repoRoot, - projectDirName: match.projectDirName, - }) - ); + const context = await findProjectContextAtRoot({ + projectRoot: match.repoRoot, + projectDirName: match.projectDirName, + }); if (!context) { throw new TargetRefusal("no_project_context"); } @@ -671,24 +701,13 @@ async function selectProject(opts: { if (opts.selector.path !== null && !(await isDirectory(startDir))) { throw new TargetRefusal("path_not_found"); } - const context = await guardInputs(() => findProjectContext(startDir)); + const context = await findProjectContext(startDir); if (!context) { throw new TargetRefusal("no_project_context"); } return { context, kind: opts.selector.path === null ? "cwd" : "path" }; } -async function guardInputs(read: () => Promise): Promise { - try { - return await read(); - } catch (error: unknown) { - if (error instanceof ProjectInputSelectionError) { - throw new TargetRefusal("project_inputs_unsupported"); - } - throw error; - } -} - /** * Registration that owns this exact checkout root: its primary root, or one of * its recorded linked worktrees. Name equality never matches, so independent diff --git a/tests/project-target-inspect.test.ts b/tests/project-target-inspect.test.ts index 16f41355c..c5fc1c97f 100644 --- a/tests/project-target-inspect.test.ts +++ b/tests/project-target-inspect.test.ts @@ -232,6 +232,42 @@ beforeAll(async () => { services: ["api"], }); await chmod(join(lockedCompose, ".hack", "docker-compose.yml"), 0o000); + // Faults the shared project input guards raise before any input is read: + // an unparseable legacy adoption receipt, and a project directory the + // inspector cannot search (its mode is dropped inside the test). + const adoptionUnsafe = join(root, "adoption-unsafe"); + await writeProject({ + repo: adoptionUnsafe, + name: "adopt", + services: ["api"], + }); + const adoptionState = join( + adoptionUnsafe, + ".hack", + ".internal", + "legacy-compose-adoption-v1" + ); + await mkdir(adoptionState, { recursive: true, mode: 0o700 }); + await writeFile(join(adoptionState, "receipt.json"), "{not json", { + mode: 0o600, + }); + await writeProject({ + repo: join(root, "unsearchable"), + name: "unsearchable", + services: ["api"], + }); + // Native and legacy inputs side by side (a conflicting project family). + const nativeConflict = join(root, "native-conflict"); + await writeProject({ + repo: nativeConflict, + name: "native", + services: ["api"], + }); + await writeFile(join(nativeConflict, ".hack", "hack.project.json"), "{}\n"); + // A regular file where the project directory belongs. + const fileProjectDir = join(root, "file-project-dir"); + await mkdir(fileProjectDir, { recursive: true }); + await writeFile(join(fileProjectDir, ".hack"), "not a directory\n"); await mkdir(join(root, "outside"), { recursive: true }); @@ -635,6 +671,47 @@ describe("paired CLI/MCP target fixtures", () => { CASE_TIMEOUT_MS ); + test( + "project input guard refusals are closed refusals, not detail-less errors", + async () => { + // An unsafe legacy adoption state used to escape as a detail-less + // E_CONFIG_INVALID, which the adapter turned into data: null. + expectRefusal( + await inspectBoth({ path: join(fixture.root, "adoption-unsafe") }), + "adoption_state_unsafe" + ); + expectRefusal( + await inspectBoth({ path: join(fixture.root, "native-conflict") }), + "project_inputs_unsupported" + ); + // A file where `.hack` belongs is an input access failure. + expectRefusal( + await inspectBoth({ path: join(fixture.root, "file-project-dir") }), + "input_unreadable" + ); + }, + CASE_TIMEOUT_MS + ); + + // Root can search a mode-000 directory, so this fault cannot be produced. + test.skipIf(process.getuid?.() === 0)( + "an unsearchable project directory is a closed refusal without the path", + async () => { + const path = join(fixture.root, "unsearchable"); + const projectDir = join(path, ".hack"); + await chmod(projectDir, 0o000); + try { + expectRefusal(await inspectBoth({ path }), "adoption_state_unsafe"); + 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); + } + }, + CASE_TIMEOUT_MS + ); + test( "two registrations that own one checkout are refused as ambiguous", async () => { @@ -887,6 +964,32 @@ describe("target inspector domain", () => { ); }); + test("the MCP argv carries every selector as one --name=value token", () => { + // The paired cases build the direct CLI argv with this same helper, so a + // dropped selector would change both sides alike. Pin the argv literally. + expect( + buildTargetInspectArgs({ + projectName: "alpha", + path: "/x", + branch: "feature/login", + service: "api", + env: "staging", + expectGeneration: "cfg-v0:0123456789abcdef0123456789abcdef", + }) + ).toEqual([ + "project", + "target", + "--json", + "--project=alpha", + "--path=/x", + "--branch=feature/login", + "--service=api", + "--env=staging", + "--expect-generation=cfg-v0:0123456789abcdef0123456789abcdef", + ]); + expect(buildTargetInspectArgs({})).toEqual(["project", "target", "--json"]); + }); + test("selectors are validated before any project lookup", async () => { const registryPath = join(fixture.state, "projects.json"); for (const selector of [ From 20032d4d7cb1067af17105dcff7f999c1c2785c3 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 11:41:43 -0400 Subject: [PATCH 2/7] fix(perf): judge window means from a correctly rounded sum The evaluator computed a window mean with built-in sum(), which rounds after every addition and changed its algorithm in Python 3.12. Ten idle samples of 0.01 averaged to 0.009999999999999998 on the macOS system Python 3.9, so a mean exactly at the exclusive 1% idle ceiling passed there and failed on 3.12+. Use math.fsum so every interpreter judges the same correctly rounded mean, and add a regression test that fails with plain sum() on both 3.9 and 3.12+. Co-Authored-By: Claude Opus 5.5 --- .../application-benchmark-protocol.md | 6 ++++-- .../check-application-benchmark-protocol.py | 12 +++++++++++- .../test_application_benchmark_protocol.py | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/docs/reference/application-benchmark-protocol.md b/docs/reference/application-benchmark-protocol.md index 98bafa25e..b7ee8290a 100644 --- a/docs/reference/application-benchmark-protocol.md +++ b/docs/reference/application-benchmark-protocol.md @@ -287,8 +287,10 @@ A different or missing value is refused (`artifact_identity_mismatch`, frozen correctness check (`workload_correctness_weakened`). A revision may add correctness checks. The sample counts are floors at their frozen values: a protocol that plans fewer than 10 lifecycle runs or 60 tail samples per lane is refused, even -where the program floor or the p95 bound would allow fewer. Every number in the protocol or a summary must be -within ±(2⁵³ − 1), the interoperable JSON range; a larger value is refused as malformed. +where the program floor or the p95 bound would allow fewer. Every number in the +protocol or a summary must be within ±(2⁵³ − 1), the interoperable JSON range; a larger +value is refused as malformed. A mean is the correctly rounded sum divided by the +count, so a mean at an exclusive ceiling gets the same verdict on every Python version. These are acceptance thresholds, not measured achievements. diff --git a/scripts/check-application-benchmark-protocol.py b/scripts/check-application-benchmark-protocol.py index bc20e4336..15dd7c53f 100644 --- a/scripts/check-application-benchmark-protocol.py +++ b/scripts/check-application-benchmark-protocol.py @@ -679,12 +679,22 @@ def validate_summary(protocol, summary): return sorted(found), workload +def window_mean(samples): + """The correctly rounded sum divided by the count. + + Built-in sum() rounds after every addition, and Python 3.12 changed how, so a mean + at an exclusive ceiling (ten samples of 0.01) could land just below it on one + interpreter and on it on another. math.fsum gives the same verdict everywhere. + """ + return math.fsum(samples) / len(samples) + + def _statistic(samples, statistic): if statistic == "median": return median(samples) if statistic == "p95": return nearest_rank(samples, P95_QUANTILE) - return sum(samples) / len(samples) + return window_mean(samples) def _sample_floor(protocol, statistic): diff --git a/tests/python/test_application_benchmark_protocol.py b/tests/python/test_application_benchmark_protocol.py index 4ac0b0af9..5be3436f2 100644 --- a/tests/python/test_application_benchmark_protocol.py +++ b/tests/python/test_application_benchmark_protocol.py @@ -4,6 +4,7 @@ import importlib.util import io import json +import math from pathlib import Path import re import tempfile @@ -515,6 +516,23 @@ def test_absolute_ceilings_respect_inclusivity(self): summary["lanes"]["candidate"]["info_latency_ms"] = entry([50] * 56 + [101] * 4) self.assertEqual(verdicts(self.judge(summary), "info_latency_ms"), ["fail"]) + def test_window_mean_is_correctly_rounded_on_every_interpreter(self): + # Built-in sum() gives 0.009999999999999998 here before Python 3.12, which would + # pass the exclusive 1% idle ceiling; on 3.12+ it differs from the exact mean on + # the second list. Both must judge from the correctly rounded mean. + self.assertEqual(protocol_check.window_mean([0.01] * 10), 0.01) + large = [1000000000000000.2, 0.001, 0.007, 0.1, 0.01, 3.0, 1.0, 0.0, 0.007, 1000000000000000.2] + exact = math.fsum(large) / len(large) + self.assertEqual(protocol_check.window_mean(large), exact) + # A stricter extra budget whose exclusive ceiling is that exact mean must fail. + strict = mutated(lambda d: d["budgets"]["absolute"].append( + {"metric": "probe_window_mean", "statistic": "window_mean", "max": exact, "inclusive": False})) + self.assertEqual(protocol_check.validate_protocol(strict), []) + summary = small_summary() + summary["lanes"]["candidate"]["probe_window_mean"] = entry(large) + _, records = protocol_check.evaluate(strict, summary) + self.assertEqual(verdicts(records, "probe_window_mean"), ["fail"]) + def test_a_not_applicable_baseline_with_a_reason_is_skipped_and_missing_lanes_are_inconclusive(self): summary = small_summary() summary["lanes"]["hack-4.1.1"]["warm_startup_seconds"] = not_applicable() From b0c3195b5c6bd2fc30936606f75ed8f3ff4e4d65 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 18:09:15 -0400 Subject: [PATCH 3/7] fix(perf): refuse any out-of-range number in a protocol or summary The protocol page says every number in a protocol or summary must be finite and within +/-(2^53 - 1), but the checker only bounded the fields it judged with _number. A count read with _count (p95_min_samples, expected) and any field the checker never reads accepted 2^53, 10^400, NaN or Infinity: a protocol with p95_min_samples = 2^53 validated, and a summary with expected = 10^400 was judged inconclusive instead of being refused. Scan every number in the whole document iteratively and add the closed codes protocol_number_out_of_range and summary_number_out_of_range, reported alongside any field-specific code. The page now names both codes and says the bound covers unread fields. Refs: HACK-1333, HACK-1169 Co-Authored-By: Claude Opus 5.5 --- .../application-benchmark-protocol.md | 11 ++-- .../check-application-benchmark-protocol.py | 27 ++++++++- .../test_application_benchmark_protocol.py | 55 +++++++++++++++++-- 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/docs/reference/application-benchmark-protocol.md b/docs/reference/application-benchmark-protocol.md index b7ee8290a..ef7184d0d 100644 --- a/docs/reference/application-benchmark-protocol.md +++ b/docs/reference/application-benchmark-protocol.md @@ -287,9 +287,12 @@ A different or missing value is refused (`artifact_identity_mismatch`, frozen correctness check (`workload_correctness_weakened`). A revision may add correctness checks. The sample counts are floors at their frozen values: a protocol that plans fewer than 10 lifecycle runs or 60 tail samples per lane is refused, even -where the program floor or the p95 bound would allow fewer. Every number in the -protocol or a summary must be within ±(2⁵³ − 1), the interoperable JSON range; a larger -value is refused as malformed. A mean is the correctly rounded sum divided by the +where the program floor or the p95 bound would allow fewer. Every number anywhere in +the protocol or a summary, including a field the checker does not otherwise read, must +be finite and within ±(2⁵³ − 1), the interoperable JSON range. Any other number, +including the `NaN` and `Infinity` that Python's JSON parser accepts, is refused as +`protocol_number_out_of_range` or `summary_number_out_of_range`, alongside any code +for the field that holds it. A mean is the correctly rounded sum divided by the count, so a mean at an exclusive ceiling gets the same verdict on every Python version. These are acceptance thresholds, not measured achievements. @@ -317,7 +320,7 @@ record. It exits 1 when any verdict fails or is inconclusive. It exits 2, withou judging, when the protocol or summary is invalid: for example a summary for another cache class or scale point, a candidate marked `not_applicable`, a `not_applicable` record that carries samples, a negative sample, a planned count that is not a positive -integer, or a value of the wrong type. Diagnostics are closed codes and never repeat +integer, a number outside ±(2⁵³ − 1), or a value of the wrong type. Diagnostics are closed codes and never repeat input values. A summary has this shape. `samples` lists only admitted samples; `flagged` and `failed` diff --git a/scripts/check-application-benchmark-protocol.py b/scripts/check-application-benchmark-protocol.py index 15dd7c53f..43fc956fb 100644 --- a/scripts/check-application-benchmark-protocol.py +++ b/scripts/check-application-benchmark-protocol.py @@ -11,7 +11,8 @@ Revision-1 identities (artifacts, host, lane kinds and backends, candidate capacity, workload pins, rotated lane order) are exact; budgets and sample counts are floors at their frozen values, which a revision may tighten. -Every number must lie within MAX_JSON_NUMBER. +Every number anywhere in the protocol or a summary must be finite and lie within +MAX_JSON_NUMBER, including fields the checker does not otherwise read. Diagnostics are closed codes from DIAGNOSTICS; they never echo input values or paths. A budget verdict is one of VERDICTS. A statistic is judged only from complete, sufficient, admitted samples: missing, short, flagged or unaccounted samples are @@ -197,8 +198,10 @@ "statistic_unknown", "observer_policy_missing", "known_limit_malformed", + "protocol_number_out_of_range", "summary_unreadable", "summary_not_object", + "summary_number_out_of_range", "summary_revision_mismatch", "summary_workload_unknown", "summary_cache_class_mismatch", @@ -313,6 +316,24 @@ def _number(value): and -MAX_JSON_NUMBER <= value <= MAX_JSON_NUMBER +def _numbers_in_range(value): + """True when every number anywhere in a JSON value satisfies _number. + + Booleans are not numbers and object keys are strings, so neither is checked. The + walk is iterative, so a deeply nested value cannot raise RecursionError. + """ + pending = [value] + while pending: + node = pending.pop() + if isinstance(node, dict): + pending.extend(node.values()) + elif isinstance(node, list): + pending.extend(node) + elif isinstance(node, (int, float)) and not isinstance(node, bool) and not _number(node): + return False + return True + + def _same(value, frozen): """Exact JSON equality: unlike ==, 4.0 is not 4 and true is not 1.""" return json.dumps(value, sort_keys=True) == json.dumps(frozen, sort_keys=True) @@ -554,6 +575,8 @@ def validate_protocol(doc): if not isinstance(doc, dict): return ["protocol_not_object"] found = set() + if not _numbers_in_range(doc): + found.add("protocol_number_out_of_range") if doc.get("protocol") != PROTOCOL_ID: found.add("protocol_id_mismatch") if not _count(doc.get("revision")) or doc["revision"] != PROTOCOL_REVISION: @@ -647,6 +670,8 @@ def validate_summary(protocol, summary): if workload is None: return ["summary_workload_unknown"], None found = set() + if not _numbers_in_range(summary): + found.add("summary_number_out_of_range") # Budgets are frozen for the warm class only; other classes are reported separately. if summary.get("cache_class") != protocol["cache_classes"]["budget_class"]: found.add("summary_cache_class_mismatch") diff --git a/tests/python/test_application_benchmark_protocol.py b/tests/python/test_application_benchmark_protocol.py index 5be3436f2..aa72ac2a4 100644 --- a/tests/python/test_application_benchmark_protocol.py +++ b/tests/python/test_application_benchmark_protocol.py @@ -621,7 +621,8 @@ def change(summary): ("summary_entry_malformed", baseline("cpu_seconds_per_task", "flagged", -1)), ("summary_entry_malformed", baseline("cpu_seconds_per_task", "failed", "0")), ("summary_entry_malformed", baseline("cpu_seconds_per_task", "samples", [1, "2"])), - ("summary_entry_malformed", baseline("cpu_seconds_per_task", "samples", [float("nan")] * 10)), + (("summary_entry_malformed", "summary_number_out_of_range"), + baseline("cpu_seconds_per_task", "samples", [float("nan")] * 10)), ("summary_entry_malformed", baseline("cpu_seconds_per_task", "median", 1.0)), ("summary_entry_malformed", candidate("watcher_ack_ms", [100] * 60)), ("summary_lane_malformed", lambda s: s["lanes"].update(compose=[MARKER])), @@ -636,7 +637,7 @@ def change(summary): summary = small_summary() change(summary) with self.subTest(code=code, summary=summary.get("workload")): - self.assertEqual(self.refused(summary), [code]) + self.assertEqual(self.refused(summary), [code] if isinstance(code, str) else list(code)) def test_oversized_integers_are_refused_not_a_traceback(self): """Python parses 10**400 exactly; converting it to a float would raise OverflowError.""" @@ -650,15 +651,57 @@ def test_oversized_integers_are_refused_not_a_traceback(self): with self.subTest(case=name): summary = small_summary() change(summary) - self.assertEqual(evaluate_file(summary), - (2, {"status": "invalid", "diagnostics": ["summary_entry_malformed"]})) + self.assertEqual(evaluate_file(summary), (2, {"status": "invalid", "diagnostics": [ + "summary_entry_malformed", "summary_number_out_of_range"]})) summary = small_summary() summary["observer_cost"]["cpu_seconds_per_task"] = HUGE - self.assertEqual(evaluate_file(summary), - (2, {"status": "invalid", "diagnostics": ["summary_observer_malformed"]})) + self.assertEqual(evaluate_file(summary), (2, {"status": "invalid", "diagnostics": [ + "summary_number_out_of_range", "summary_observer_malformed"]})) protocol = mutated(lambda d: budget(d, "comparative", "cpu_seconds_per_task").update(max_ratio=HUGE)) self.assertIn("comparative_budget_weakened", protocol_check.validate_protocol(protocol)) + def test_every_number_anywhere_must_be_finite_and_interoperable(self): + # The page promises every number in a protocol or summary is within +/-(2**53 - 1). + # These sit where no other check would refuse them: an unread field, or a count + # that is only compared as a floor or against the sample list. + limit = 2 ** 53 - 1 + for value in (limit + 1, -limit - 1, HUGE, float("nan"), float("inf"), -float("inf"), 1e300): + with self.subTest(value=repr(value)): + unread = mutated(lambda d: d.update(notes=[{"bytes": value}])) + self.assertEqual(protocol_check.validate_protocol(unread), ["protocol_number_out_of_range"]) + floor = mutated(lambda d: d["samples"].update(p95_min_samples=value)) + self.assertIn("protocol_number_out_of_range", protocol_check.validate_protocol(floor)) + summary = small_summary() + summary["lanes"]["candidate"]["unbudgeted_metric"] = {"samples": [value]} + summary["notes"] = value + self.assertEqual(self.refused(summary), ["summary_number_out_of_range"]) + summary = small_summary() + summary["lanes"]["candidate"]["info_latency_ms"]["expected"] = value + self.assertIn("summary_number_out_of_range", self.refused(summary)) + self.assertEqual(protocol_check.validate_protocol(mutated(lambda d: d["samples"].update( + p95_min_samples=limit))), []) + self.assertEqual(protocol_check.validate_protocol(mutated(lambda d: d.update( + notes=[limit, -limit, 1e-300, True]))), []) + summary = small_summary() + summary["lanes"]["candidate"]["unbudgeted_metric"] = {"samples": [limit, -limit]} + self.judge(summary) + summary["lanes"]["candidate"]["unbudgeted_metric"]["samples"].append(limit + 1) + self.assertEqual(evaluate_file(summary), + (2, {"status": "invalid", "diagnostics": ["summary_number_out_of_range"]})) + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "protocol.json" + path.write_text(json.dumps(mutated(lambda d: d.update(notes=float("nan"))))) + self.assertEqual(run_cli(["--protocol", str(path)]), + (2, {"status": "invalid", "diagnostics": ["protocol_number_out_of_range"]})) + + def test_a_deeply_nested_unread_field_is_scanned_without_raising(self): + # Far deeper than the recursion limit: a recursive walk would raise RecursionError. + deep = [2 ** 53] + for _ in range(100000): + deep = [deep] + self.assertEqual(protocol_check.validate_protocol(mutated(lambda d: d.update(notes=deep))), + ["protocol_number_out_of_range"]) + def test_an_overflowing_ratio_fails_and_the_report_stays_strict_json(self): summary = small_summary() summary["lanes"]["compose"]["warm_startup_seconds"] = entry([5e-324] * 10) From 85c8ec2322edac41480c26f1000a0b708eaa145b Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 21 Oct 2026 18:19:02 -0400 Subject: [PATCH 4/7] fix(agents): close uncheckable project input presence in the target inspector The shared project input guard checks each candidate input with lstat and treats only ENOENT as absence. Any other fault escaped as a plain Error, so a `.dev` file, or a `.dev` directory the caller cannot search, beside a valid `.hack` made `hack project target` print a detail-less E_UNEXPECTED envelope (and the MCP adapter returned data: null). The guard now throws a typed, value-free ProjectInputAccessError that classifies the fault as permission (EACCES, EPERM) or unreadable without keeping the OS message or its path. The target inspector maps it to the closed input_permission_denied / input_unreadable refusals. Other commands now see E_CONFIG_INVALID with the same value-free message as the adoption guard's input-access refusal, instead of E_UNEXPECTED. Paired CLI/MCP fixtures and domain tests cover both cases; each was proven by mutation. Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-target-result-contract.md | 6 ++- src/lib/project-input-selection.ts | 29 ++++++++++++-- src/lib/target-inspect.ts | 13 +++++- tests/project-input-selection.test.ts | 44 +++++++++++++++++++++ tests/project-target-inspect.test.ts | 43 ++++++++++++++++++++ 5 files changed, 128 insertions(+), 7 deletions(-) diff --git a/docs/reference/ax-target-result-contract.md b/docs/reference/ax-target-result-contract.md index f2a852729..178aa312f 100644 --- a/docs/reference/ax-target-result-contract.md +++ b/docs/reference/ax-target-result-contract.md @@ -169,7 +169,10 @@ too. Native or conflicting project files are `project_inputs_unsupported`. A legacy adoption state that is unparseable, interrupted, or cannot be checked (for example a project directory the caller cannot search) is `adoption_state_unsafe`. A non-directory where the project directory belongs is -`input_unreadable`. +`input_unreadable`. 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. `retry` is advice for the caller. Like suggested actions and application logs, it carries no execution authority. @@ -217,6 +220,7 @@ 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, unsearchable or non-directory project dir) | `project_inputs_unsupported`, `adoption_state_unsafe`, `input_unreadable` | same | paired fixture (unsearchable case 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) | | 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/project-input-selection.ts b/src/lib/project-input-selection.ts index 10a44b8eb..1ddea5f7e 100644 --- a/src/lib/project-input-selection.ts +++ b/src/lib/project-input-selection.ts @@ -57,6 +57,27 @@ export class ProjectInputSelectionError extends HackCliError { } } +/** + * Stable, value-free refusal when a project input's presence cannot be + * checked (for example a file where `.dev` belongs, or a project directory the + * caller cannot search). Only ENOENT means absence, so the guard refuses + * rather than guessing. `access` classifies the fault without its OS message, + * which carries the absolute path and is therefore never kept. + */ +export class ProjectInputAccessError extends HackCliError { + readonly access: "permission" | "unreadable"; + + constructor(access: "permission" | "unreadable") { + super({ + code: "E_CONFIG_INVALID", + message: + "Cannot inspect Hack project inputs. Check filesystem permissions and paths before retrying. Values omitted.", + }); + this.name = "ProjectInputAccessError"; + this.access = access; + } +} + /** * Inspect only the named root, without parsing, following authored-file links, * searching ancestors, touching registration, or normalizing configuration. @@ -188,12 +209,12 @@ async function inputPresent(path: string): Promise { await lstat(path); return true; } catch (error: unknown) { - if (isRecord(error) && error.code === "ENOENT") { + const code = isRecord(error) ? error.code : undefined; + if (code === "ENOENT") { return false; } - throw new Error( - "Cannot inspect Hack project inputs. Check filesystem permissions and paths before retrying.", - { cause: error } + throw new ProjectInputAccessError( + code === "EACCES" || code === "EPERM" ? "permission" : "unreadable" ); } } diff --git a/src/lib/target-inspect.ts b/src/lib/target-inspect.ts index b41227d0a..d2ce63fba 100644 --- a/src/lib/target-inspect.ts +++ b/src/lib/target-inspect.ts @@ -31,7 +31,10 @@ import { sanitizeBranchSlug, sanitizeProjectSlug, } from "./project.ts"; -import { ProjectInputSelectionError } from "./project-input-selection.ts"; +import { + ProjectInputAccessError, + ProjectInputSelectionError, +} from "./project-input-selection.ts"; import { AmbiguousProjectNameError, normalizeProjectName, @@ -367,7 +370,8 @@ export function targetRefusalEnvelope(opts: { * closed `input_permission_denied` / `input_unreadable` refusals, so the OS * message and its absolute path never reach the caller. The shared project * input guards' own refusals (native or conflicting inputs, an unsafe or - * unreadable legacy adoption state) are mapped too, wherever they are raised. + * unreadable legacy adoption state, an input whose presence cannot be checked) + * are mapped too, wherever they are raised. * Only other faults (programming errors) still throw. * * `afterResolve` is a test seam that runs after the target is resolved and @@ -419,6 +423,11 @@ function projectInputGuardReason(error: unknown): TargetRefusalReason | null { if (error instanceof ProjectInputSelectionError) { return "project_inputs_unsupported"; } + if (error instanceof ProjectInputAccessError) { + return error.access === "permission" + ? "input_permission_denied" + : "input_unreadable"; + } if (error instanceof LegacyComposeAdoptionSelectionError) { return error.kind === "input-access" ? "input_unreadable" diff --git a/tests/project-input-selection.test.ts b/tests/project-input-selection.test.ts index 3883edd3e..b9544e5b3 100644 --- a/tests/project-input-selection.test.ts +++ b/tests/project-input-selection.test.ts @@ -28,6 +28,7 @@ import { discoverProjectInputs, inspectProjectInputsAtRoot, NATIVE_PROJECT_FILENAME, + ProjectInputAccessError, } from "../src/lib/project-input-selection.ts"; const roots: string[] = []; @@ -197,6 +198,49 @@ test.each([ ); }); +async function inputAccessFault(root: string): Promise { + try { + await inspectProjectInputsAtRoot({ projectRoot: root }); + } catch (error: unknown) { + return error; + } + throw new Error("unexpected input selection success"); +} + +test("a .dev file beside valid .hack inputs is a typed, value-free access refusal", async () => { + const root = await fixture(); + await file(root, ".hack/docker-compose.yml"); + await file(root, ".dev", "not a directory"); + const error = await inputAccessFault(root); + expect(error).toBeInstanceOf(ProjectInputAccessError); + expect((error as ProjectInputAccessError).access).toBe("unreadable"); + expect((error as ProjectInputAccessError).code).toBe("E_CONFIG_INVALID"); + expect((error as Error).cause).toBeUndefined(); + expect(String(error)).not.toContain(root); + expect(String(error)).not.toContain("ENOTDIR"); +}); + +// 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", + async () => { + const root = await fixture(); + await file(root, ".hack/docker-compose.yml"); + const dev = join(root, ".dev"); + await mkdir(dev); + await chmod(dev, 0); + try { + const error = await inputAccessFault(root); + expect(error).toBeInstanceOf(ProjectInputAccessError); + expect((error as ProjectInputAccessError).access).toBe("permission"); + expect(String(error)).not.toContain(root); + expect(String(error)).not.toContain("EACCES"); + } finally { + await chmod(dev, 0o700); + } + } +); + test("supported legacy discovery keeps primary-before-legacy precedence", async () => { const outer = await fixture(); await file(outer, ".hack/docker-compose.yml"); diff --git a/tests/project-target-inspect.test.ts b/tests/project-target-inspect.test.ts index c5fc1c97f..7e32967ef 100644 --- a/tests/project-target-inspect.test.ts +++ b/tests/project-target-inspect.test.ts @@ -268,6 +268,15 @@ beforeAll(async () => { const fileProjectDir = join(root, "file-project-dir"); await mkdir(fileProjectDir, { recursive: true }); await writeFile(join(fileProjectDir, ".hack"), "not a directory\n"); + // A valid .hack project whose legacy .dev input directory cannot be checked: + // a regular file where .dev belongs, and a .dev directory whose mode is + // dropped inside the test. + const devFile = join(root, "dev-file"); + await writeProject({ repo: devFile, name: "devfile", services: ["api"] }); + await writeFile(join(devFile, ".dev"), "not a directory\n"); + const devLocked = join(root, "dev-locked"); + await writeProject({ repo: devLocked, name: "devlocked", services: ["api"] }); + await mkdir(join(devLocked, ".dev"), { recursive: true }); await mkdir(join(root, "outside"), { recursive: true }); @@ -712,6 +721,40 @@ describe("paired CLI/MCP target fixtures", () => { CASE_TIMEOUT_MS ); + test( + "a .dev file beside a valid .hack is a closed refusal without the path", + async () => { + // The input presence check used to escape as a detail-less E_UNEXPECTED. + expectRefusal( + await inspectBoth({ path: join(fixture.root, "dev-file") }), + "input_unreadable" + ); + const cli = await runCli({ path: join(fixture.root, "dev-file") }); + expect(JSON.stringify(cli.envelope)).not.toContain(fixture.root); + expect(JSON.stringify(cli.envelope)).not.toContain("ENOTDIR"); + }, + CASE_TIMEOUT_MS + ); + + // Root can search a mode-000 directory, so this fault cannot be produced. + test.skipIf(process.getuid?.() === 0)( + "an unsearchable .dev beside a valid .hack is a closed refusal without the path", + async () => { + const path = join(fixture.root, "dev-locked"); + const devDir = join(path, ".dev"); + await chmod(devDir, 0o000); + try { + 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(devDir, 0o755); + } + }, + CASE_TIMEOUT_MS + ); + test( "two registrations that own one checkout are refused as ambiguous", async () => { From 4dfbb6a035c37713d4f3f269a6a4fabbd7c7a870 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Thu, 22 Oct 2026 00:52:05 -0400 Subject: [PATCH 5/7] fix(native): keep the closed exit code when the graph owner's pipes drain late The foreground graph owner drained its child's stdout and stderr for only 250 ms after exit. Under heavy oversubscription the pipe readers could be scheduled after that window, so the closed native error code (or readiness line) was dropped and a generic startup failure was reported. Replace the window with the named, documented NATIVE_EXIT_DRAIN_BOUND_MS (5 s), still measured from the observed exit. EOF still ends a normal drain at once; the bound only ends a drain that a descendant holding the pipes keeps open. Tests: a writer that emits the code only after the stand-in is reaped is kept (fails with the 250 ms window), and a held descendant still settles within the bound while it is alive (fails with a widened drain). Refs HACK-1350 Co-Authored-By: Claude Opus 5.5 --- src/backends/native-project-process.ts | 21 +- tests/native-project-process.test.ts | 265 ++++++++++++++++++++----- 2 files changed, 236 insertions(+), 50 deletions(-) diff --git a/src/backends/native-project-process.ts b/src/backends/native-project-process.ts index b898d8f54..ed3c467ef 100644 --- a/src/backends/native-project-process.ts +++ b/src/backends/native-project-process.ts @@ -22,6 +22,20 @@ const RUN = /^[a-f0-9]{32}$/; const MAX_OUTPUT = 16 * 1024 * 1024; const MAX_INPUT = 256 * 1024; +/** + * Exit-relative bound on draining the owned child's stdout and stderr. + * + * The native runtime writes its closed error code (and any readiness line) + * before it exits, so those bytes are already in the pipes when the exit is + * observed; with no other writer, EOF follows immediately and the drain ends + * there. This bound only ends a drain that EOF cannot end: a descendant that + * inherited a pipe and keeps it open. It is measured from the observed exit, + * not from spawn, and is deliberately generous so that a heavily + * oversubscribed host still schedules the pipe readers before it expires. + * A 250 ms bound lost the closed code under load (HACK-1350). + */ +export const NATIVE_EXIT_DRAIN_BOUND_MS = 5000; + export type NativeExitDiagnostic = { readonly exitCode: number; readonly nativeCode?: string; @@ -317,14 +331,15 @@ async function serveGraphProcess( } ); // Descendants may inherit a pipe after the owned receiver exits. Its exit, - // not unrelated writers' EOF, bounds draining; final graph inspection remains - // the caller's authority for cleanup, regardless of a successful exit code. + // not unrelated writers' EOF, bounds draining (see NATIVE_EXIT_DRAIN_BOUND_MS); + // final graph inspection remains the caller's authority for cleanup, + // regardless of a successful exit code. const drain = new AbortController(); const owner = new AbortController(); let drainTimer: ReturnType | undefined; void child.exited.then(() => { owner.abort(); - drainTimer = setTimeout(() => drain.abort(), 250); + drainTimer = setTimeout(() => drain.abort(), NATIVE_EXIT_DRAIN_BOUND_MS); }); const failureCodes = readNativeFailureCodes(child.stderr, drain.signal); const failureCode = failureCodes.then((failure) => failure?.code); diff --git a/tests/native-project-process.test.ts b/tests/native-project-process.test.ts index 49e170325..cc4e91956 100644 --- a/tests/native-project-process.test.ts +++ b/tests/native-project-process.test.ts @@ -2,7 +2,10 @@ import { afterEach, expect, test } from "bun:test"; import { chmod, mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { serveNativeProjectGraph } from "../src/backends/native-project-process.ts"; +import { + NATIVE_EXIT_DRAIN_BOUND_MS, + serveNativeProjectGraph, +} from "../src/backends/native-project-process.ts"; import { expectClosedRefusal } from "./helpers/closed-refusal.ts"; const roots: string[] = []; @@ -227,53 +230,221 @@ test("readiness rejection retains identity while observing safe native cleanup c expect(Bun.inspect(diagnostic)).not.toContain("synthetic-private-cleanup"); }); -test("owned exit completes without waiting for descendant-held stdout and stderr", async () => { - const opts = await fixture(` - const keeper = Bun.spawn(["/bin/sh", "-c", "sleep 3"], { - stdin: "ignore", stdout: "inherit", stderr: "inherit", detached: true, - }); - keeper.unref(); - console.log(JSON.stringify({kind:"graph_foreground_ready",run:"${run}"})); - console.error(JSON.stringify({code:"graph_owner_recovery",message:"synthetic-private-detail"})); - process.exit(0); +/** Slack for host load between the stand-in's recorded exit and settlement. */ +const DRAIN_SETTLE_MARGIN_MS = 5000; +/** Fixed, well above the former 250 ms window and well below the named bound. */ +const LATE_CODE_DELAY_MS = 1000; +/** A held descendant always retires by itself, even if this test process dies. */ +const DESCENDANT_CAP_MS = 20_000; +const SETTLEMENT_WAIT_MS = DESCENDANT_CAP_MS + 5000; + +async function waitForFile(path: string, budgetMs: number): Promise { + const deadline = performance.now() + budgetMs; + while (!(await Bun.file(path).exists())) { + if (performance.now() >= deadline) { + return false; + } + await Bun.sleep(10); + } + return true; +} + +function processPresent(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + if ( + error !== null && + typeof error === "object" && + "code" in error && + error.code === "ESRCH" + ) { + return false; + } + throw error; + } +} + +/** + * Run `body`, then release the stand-in's descendant through `release` and + * confirm it finished. An unconfirmed descendant retains the fixture root. + */ +async function settleDescendant(opts: { + readonly root: string; + readonly release: string; + readonly finished: string; + readonly body: () => Promise; +}): Promise { + let failed: unknown; + try { + await opts.body(); + } catch (error: unknown) { + failed = error; + } finally { + await Bun.write(join(opts.root, opts.release), "release"); + } + const completed = await waitForFile( + join(opts.root, opts.finished), + SETTLEMENT_WAIT_MS + ); + if (!completed) { + roots.splice(roots.indexOf(opts.root), 1); + throw new AggregateError( + failed === undefined ? [] : [failed], + `Owned descendant completion is unconfirmed; fixture retained at ${opts.root}.` + ); + } + if (failed !== undefined) { + throw failed; + } +} + +// Keeps the stand-in's stdout and stderr open until the test releases it. +const HOLDING_KEEPER = `import {existsSync, writeFileSync} from "node:fs"; +writeFileSync("keeper-pid", String(process.pid)); +const end = Date.now() + ${DESCENDANT_CAP_MS}; +while (!existsSync("release") && Date.now() < end) await Bun.sleep(20); +writeFileSync("keeper-finished", "finished");`; + +test.each([ + ["ready", 0], + ["early failure", 2], +] as const)( + "%s exit settles within the named drain bound while a descendant holds stdout and stderr", + async (mode, exitCode) => { + const ready = mode === "ready"; + const nativeCode = ready ? "graph_owner_recovery" : "graph_budget"; + const opts = await fixture(` + import { spawn } from "node:child_process"; + import { existsSync, writeFileSync } from "node:fs"; + spawn(process.execPath, ["--eval", ${JSON.stringify(HOLDING_KEEPER)}], { + detached: true, stdio: ["ignore", 1, 2], + }).unref(); + const deadline = Date.now() + 10000; + while (!existsSync("keeper-pid") && Date.now() < deadline) await Bun.sleep(10); + if (!existsSync("keeper-pid")) process.exit(3); + ${ready ? `console.log(JSON.stringify({kind:"graph_foreground_ready",run:"${run}"}));` : ""} + console.error(JSON.stringify({code:"${nativeCode}",message:"synthetic-private-detail"})); + writeFileSync("exited-at", String(Date.now())); + process.exit(${exitCode}); `); - let diagnostic: unknown; - const started = performance.now(); - const result = await serveNativeProjectGraph({ - ...opts, - onReady: async () => {}, - onExitDiagnostic: (value) => { - diagnostic = value; - }, - }); - expect(result).toBe(0); - expect(performance.now() - started).toBeLessThan(1500); - expect(diagnostic).toEqual({ - exitCode: 0, - nativeCode: "graph_owner_recovery", - }); - expect(Bun.inspect(diagnostic)).not.toContain("synthetic-private-detail"); -}, 6000); + const root = opts.projectRoot; + await settleDescendant({ + root, + release: "release", + finished: "keeper-finished", + body: async () => { + let diagnostic: unknown; + let readyCount = 0; + let result: number | undefined; + let caught: unknown; + try { + result = await serveNativeProjectGraph({ + ...opts, + startupTimeoutMs: 30_000, + onReady: async () => { + readyCount++; + }, + onExitDiagnostic: (value) => { + diagnostic = value; + }, + }); + } catch (error: unknown) { + caught = error; + } + const settledAt = Date.now(); + const exitedAt = Number(await Bun.file(join(root, "exited-at")).text()); + const keeper = Number(await Bun.file(join(root, "keeper-pid")).text()); + // The bound, not the descendant's EOF, ended the drain: it is still holding. + expect(processPresent(keeper)).toBe(true); + expect(await Bun.file(join(root, "keeper-finished")).exists()).toBe( + false + ); + expect(settledAt - exitedAt).toBeLessThan( + NATIVE_EXIT_DRAIN_BOUND_MS + DRAIN_SETTLE_MARGIN_MS + ); + if (ready) { + expect(caught).toBeUndefined(); + expect(result).toBe(0); + expect(readyCount).toBe(1); + } else { + expect(readyCount).toBe(0); + const refusal = await expectClosedRefusal( + Promise.reject(caught), + nativeCode + ); + expect(refusal.message).not.toContain("synthetic-private-detail"); + } + expect(diagnostic).toEqual({ exitCode, nativeCode }); + expect(Bun.inspect(diagnostic)).not.toContain( + "synthetic-private-detail" + ); + }, + }); + }, + 60_000 +); -test("early native failure retains its safe code when descendant pipes stay open", async () => { +// Writes the closed code on the stand-in's inherited stderr only after the +// test has observed the stand-in reaped, i.e. after its exit was reported. +const LATE_CODE_WRITER = `import {existsSync, writeFileSync} from "node:fs"; +writeFileSync("writer-pid", String(process.pid)); +const end = Date.now() + ${DESCENDANT_CAP_MS}; +while (!existsSync("exit-observed") && Date.now() < end) await Bun.sleep(10); +if (existsSync("exit-observed")) { + await Bun.sleep(${LATE_CODE_DELAY_MS}); + process.stderr.write(JSON.stringify({code:"graph_budget",message:"synthetic-private-detail"})); +} +writeFileSync("writer-finished", "finished");`; + +test("closed code written after the reported exit is kept regardless of a late reader", async () => { + expect(LATE_CODE_DELAY_MS * 2).toBeLessThanOrEqual( + NATIVE_EXIT_DRAIN_BOUND_MS + ); const opts = await fixture(` - const keeper = Bun.spawn(["/bin/sh", "-c", "sleep 3"], { - stdin: "ignore", stdout: "inherit", stderr: "inherit", detached: true, - }); - keeper.unref(); - console.error(JSON.stringify({code:"graph_budget",message:"synthetic-private-detail"})); - process.exit(2); + import { spawn } from "node:child_process"; + import { existsSync, writeFileSync } from "node:fs"; + spawn(process.execPath, ["--eval", ${JSON.stringify(LATE_CODE_WRITER)}], { + detached: true, stdio: ["ignore", "ignore", 2], + }).unref(); + const deadline = Date.now() + 10000; + while (!existsSync("writer-pid") && Date.now() < deadline) await Bun.sleep(10); + writeFileSync("child-pid", String(process.pid)); + process.exit(existsSync("writer-pid") ? 2 : 3); `); - let ready = false; - const started = performance.now(); - await expect( - serveNativeProjectGraph({ - ...opts, - onReady: async () => { - ready = true; - }, - }) - ).rejects.toThrow("graph_budget"); - expect(ready).toBe(false); - expect(performance.now() - started).toBeLessThan(1500); -}, 6000); + const root = opts.projectRoot; + await settleDescendant({ + root, + release: "exit-observed", + finished: "writer-finished", + body: async () => { + let diagnostic: unknown; + let readyCount = 0; + const outcome = serveNativeProjectGraph({ + ...opts, + startupTimeoutMs: 30_000, + onReady: async () => { + readyCount++; + }, + onExitDiagnostic: (value) => { + diagnostic = value; + }, + }); + void outcome.catch(() => undefined); + expect(await waitForFile(join(root, "child-pid"), 20_000)).toBe(true); + const child = Number(await Bun.file(join(root, "child-pid")).text()); + // Absence means the CLI reaped the stand-in: its exit has been reported. + const deadline = performance.now() + 20_000; + while (processPresent(child) && performance.now() < deadline) { + await Bun.sleep(10); + } + expect(processPresent(child)).toBe(false); + await Bun.write(join(root, "exit-observed"), "observed"); + const refusal = await expectClosedRefusal(outcome, "graph_budget"); + expect(refusal.message).not.toContain("synthetic-private-detail"); + expect(readyCount).toBe(0); + expect(diagnostic).toEqual({ exitCode: 2, nativeCode: "graph_budget" }); + }, + }); +}, 60_000); From d14b133bab6639ed3c2140b6764ae912bf2c7c91 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Thu, 22 Oct 2026 20:37:13 -0400 Subject: [PATCH 6/7] fix(env): create the project key only after its root .gitignore entry HACK-1375. In a git checkout without a key, a failed root .gitignore write (read-only file, read-only mount, full disk) refused `hack env set --secret` but left `.hack.secret.key` behind. The retry the refusal suggested then found the key, skipped the ignore step and succeeded, so `git status` offered the key for commit. - Write the root .gitignore entry before a new checkout-local key; a failed entry write refuses with no key on disk. - Before `--secret` set or hydrate uses an existing checkout-local key that git does not ignore, add the entry; if that fails, or a later rule re-includes the key, refuse closed and encrypt nothing. Keys git already ignores, and checkouts outside git, cause no write. - `hack doctor` reports a present, un-ignored checkout-local key as `env-key-unignored` (warn) with a value-free remedy. Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 8 +- docs/security/env-secrets-threat-model.md | 2 +- src/commands/doctor.ts | 2 + src/lib/doctor-env-key.ts | 66 ++++ src/lib/project-env-config.ts | 113 ++++++- tests/doctor-env-key.test.ts | 117 +++++++ tests/project-env-key-ignore.test.ts | 384 ++++++++++++++++++++++ tests/project-env-write-refusals.test.ts | 22 +- 8 files changed, 697 insertions(+), 17 deletions(-) create mode 100644 tests/project-env-key-ignore.test.ts diff --git a/docs/env.md b/docs/env.md index 45f1c4d07..f201a83cc 100644 --- a/docs/env.md +++ b/docs/env.md @@ -89,7 +89,12 @@ How it is maintained: - the entries live inside a marked, hack-managed block; anything you add outside the markers is preserved on every rewrite - the root `.gitignore` still gets a `.hack.secret.key` entry when the secret - key is first generated (the key lives at the repo root, not under `.hack/`) + key is first generated (the key lives at the repo root, not under `.hack/`). + Hack writes that entry before the key, so if the entry cannot be written, no + key is created. Before a `--secret` set or hydrate uses an existing + checkout-local key that git does not ignore, Hack adds the entry; if it + cannot, or a later rule re-includes the key, the command is refused and + nothing is encrypted with that key - older repos that added `.hack/.internal/` to the root `.gitignore` keep working; the entries are equivalent and nothing is removed @@ -870,6 +875,7 @@ it cannot spell portably as `/.hack.secret.key`. | `env-key-wrong-owner` | warn | the file is not owned by the current user | | `env-key-wrong-length` | warn | shorter than the 43-character base64url form of a 256-bit key, or empty | | `env-key-low-entropy` | warn | long enough but repetitive (estimated under 128 bits) | +| `env-key-unignored` | warn | the checkout-local key is not ignored by git, so `git status` offers it for commit; suggests adding it to the root `.gitignore` | | `env-key-symlink` | error | the key path is a symlink; Hack refuses symlinked keys | | `env-key-not-regular` | error | the key path is a directory or another non-file entry | | `env-key-unreadable` | error | unreadable, oversized or not UTF-8 | diff --git a/docs/security/env-secrets-threat-model.md b/docs/security/env-secrets-threat-model.md index 5ebacde6d..7cff5ae79 100644 --- a/docs/security/env-secrets-threat-model.md +++ b/docs/security/env-secrets-threat-model.md @@ -92,7 +92,7 @@ refer to `hack-dance/hack`. | Threat | Mitigation on `next` | | --- | --- | -| The key is committed. | The root `.gitignore` gets a `.hack.secret.key` entry when the key is created. Encryption refuses a git-tracked key, including an inherited primary key, before it is copied (#288). `hack doctor` reports `env-key-tracked` at any depth (#309) and the "generated files" check can untrack a root key with `--fix`. | +| The key is committed. | The root `.gitignore` gets a `.hack.secret.key` entry before the key is created, so a failed entry write leaves no key; an existing checkout-local key git does not ignore is repaired or refused before a `--secret` write uses it, and `hack doctor` reports it as `env-key-unignored` (HACK-1375). Encryption refuses a git-tracked key, including an inherited primary key, before it is copied (#288). `hack doctor` reports `env-key-tracked` at any depth (#309) and the "generated files" check can untrack a root key with `--fix`. | | A key copy ships in build output or a static site. | `hack doctor` reports `env-key-public-path` for copies under `public/`, `static/`, `dist/`, `build/`, `out/`, `www/`, `_site/`, `.next/` and `.output/` (#309). | | A weak, short or repetitive key. | `env-key-wrong-length` and `env-key-low-entropy` (#309). Generated keys are 32 random bytes in base64url. | | Materialized `.hack/.env` or local overrides are committed. | Hack owns a committed `.hack/.gitignore` block, and `hack doctor` flags tracked generated files (see [ignore rules](../env.md#ignore-rules-committed-hackgitignore)). | diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index d6770d32b..88d8907e0 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -87,6 +87,7 @@ import { import { describeEnvKeyInspection, type EnvKeyInspection, + gitIsLocalKeyIgnored, gitListTrackedKeyPaths, inspectProjectEnvKey, resolveEnvKeyCandidates, @@ -3003,6 +3004,7 @@ async function checkEnvKeyAtRoot(opts: { env: process.env, uid: typeof process.getuid === "function" ? process.getuid() : null, listTrackedKeyPaths: gitListTrackedKeyPaths, + isLocalKeyIgnored: gitIsLocalKeyIgnored, }); return envKeyCheckResult(inspection); } diff --git a/src/lib/doctor-env-key.ts b/src/lib/doctor-env-key.ts index 5ee08e9df..88a1c80f6 100644 --- a/src/lib/doctor-env-key.ts +++ b/src/lib/doctor-env-key.ts @@ -6,6 +6,7 @@ import { resolveGitPrimaryWorktreeRoot } from "./git-worktree.ts"; import { isRecord } from "./guards.ts"; import { readBoundedPrivateFile } from "./private-file.ts"; import { + isGitIgnoredProjectEnvPath, resolveProjectEnvKeyPath, resolveProjectEnvSharedKeyLocation, } from "./project-env-config.ts"; @@ -34,6 +35,7 @@ export type EnvKeyDiagnosticCode = | "env-key-weak-permissions" | "env-key-wrong-owner" | "env-key-tracked" + | "env-key-unignored" | "env-key-public-path" | "env-key-wrong-length" | "env-key-low-entropy"; @@ -69,6 +71,14 @@ export type ListTrackedKeyPaths = (opts: { readonly projectRoot: string; }) => Promise; +/** + * Whether git's ignore rules match the checkout-local key file; null when + * unknown (git unavailable, or the project is not in a work tree). + */ +export type IsLocalKeyIgnored = (opts: { + readonly projectRoot: string; +}) => Promise; + export type InspectEnvKeyOptions = { readonly projectRoot: string; /** Candidate key files in resolution order (local, shared, primary). */ @@ -78,6 +88,7 @@ export type InspectEnvKeyOptions = { /** Current user id; null skips the ownership and permission checks (non-POSIX). */ readonly uid: number | null; readonly listTrackedKeyPaths: ListTrackedKeyPaths; + readonly isLocalKeyIgnored: IsLocalKeyIgnored; }; /** Generated keys are 32 random bytes rendered as unpadded base64url. */ @@ -462,6 +473,46 @@ async function inspectPublicArtifactCopies(opts: { return findings; } +type CandidatePresence = { + readonly role: EnvKeyCandidateRole; + readonly present: boolean; +}; + +/** + * `env-key-unignored` when git positively does not ignore the present + * checkout-local key. Only that key can be committed from this checkout: the + * shared key lives in the git dir, and the primary's belongs to that checkout. + * An absent local key or an unknown answer (no git, probe failure) reports + * nothing. + */ +async function inspectLocalKeyIgnored(args: { + readonly opts: InspectEnvKeyOptions; + readonly presence: readonly CandidatePresence[]; +}): Promise { + const { opts } = args; + if ( + !args.presence.some( + (entry) => entry.role === "local" && entry.present === true + ) + ) { + return []; + } + const ignored = await opts + .isLocalKeyIgnored({ projectRoot: opts.projectRoot }) + .catch(() => null); + if (ignored !== false) { + return []; + } + return [ + { + code: "env-key-unignored", + status: "warn", + subject: PROJECT_ENV_KEY_FILENAME, + remedy: `printf '\\n%s\\n' ${PROJECT_ENV_KEY_FILENAME} >> .gitignore, then confirm 'git status' no longer lists ${PROJECT_ENV_KEY_FILENAME}.`, + }, + ]; +} + function sortFindings(findings: readonly EnvKeyFinding[]): EnvKeyFinding[] { return findings .map((finding, index) => ({ finding, index })) @@ -482,6 +533,7 @@ export async function inspectProjectEnvKey( ): Promise { const findings: EnvKeyFinding[] = []; let anyPresent = false; + const presence: CandidatePresence[] = []; const seen = new Set(); for (const candidate of opts.candidates) { if (seen.has(candidate.path)) { @@ -490,9 +542,12 @@ export async function inspectProjectEnvKey( seen.add(candidate.path); const inspected = await inspectCandidate({ candidate, uid: opts.uid }); anyPresent ||= inspected.present; + presence.push({ role: candidate.role, present: inspected.present }); findings.push(...inspected.findings); } + findings.push(...(await inspectLocalKeyIgnored({ opts, presence }))); + const tracked = await opts .listTrackedKeyPaths({ projectRoot: opts.projectRoot }) .catch(() => null); @@ -620,6 +675,16 @@ export const gitListTrackedKeyPaths: ListTrackedKeyPaths = async (opts) => { ); }; +/** + * Default ignore probe: `git check-ignore --no-index` for the checkout-local + * key. Null when git is unavailable or the root is not a work tree. + */ +export const gitIsLocalKeyIgnored: IsLocalKeyIgnored = async (opts) => + await isGitIgnoredProjectEnvPath({ + projectRoot: opts.projectRoot, + path: resolveProjectEnvKeyPath(opts), + }); + const SUMMARY: Record = { "env-key-ok": "key material is private and in the 256-bit class", "env-key-absent": "no project env key provisioned", @@ -629,6 +694,7 @@ const SUMMARY: Record = { "env-key-weak-permissions": "is accessible by group or others (want 0600)", "env-key-wrong-owner": "is not owned by the current user", "env-key-tracked": "is tracked by git", + "env-key-unignored": "is not ignored by git, so it could be committed", "env-key-public-path": "is present in a public artifact directory", "env-key-wrong-length": "is shorter than a 256-bit key", "env-key-low-entropy": "has too little entropy for a 256-bit key", diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 63a29c6fb..9bfca5e87 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -672,6 +672,47 @@ async function isGitTrackedProjectEnvPath(opts: { return null; } +/** + * Whether git's ignore rules (`.gitignore` files, `.git/info/exclude`, + * `core.excludesFile`) match `path`, whether or not it is tracked + * (`check-ignore --no-index`). Null when git is unavailable or `projectRoot` + * is not inside a work tree, where nothing can be committed. + */ +export async function isGitIgnoredProjectEnvPath(opts: { + readonly projectRoot: string; + readonly path: string; +}): Promise { + let proc: Bun.Subprocess<"ignore", "ignore", "ignore">; + try { + proc = Bun.spawn({ + cmd: [ + "git", + "-C", + opts.projectRoot, + "check-ignore", + "-q", + "--no-index", + "--", + toGitRelativePath(opts), + ], + stderr: "ignore", + stdin: "ignore", + stdout: "ignore", + }); + } catch { + return null; + } + + const exitCode = await proc.exited; + if (exitCode === 0) { + return true; + } + if (exitCode === 1) { + return false; + } + return null; +} + /** * Whether `startDir` or an ancestor holds a `.git` entry (directory, linked * worktree file or symlink), or `GIT_DIR` points git elsewhere. It does not ask @@ -3805,6 +3846,11 @@ export type EnsureProjectEnvSecretKeyResult = { * and new key files are created exclusively with mode 0600 (an existing entry, * including a dangling symlink, is never replaced or followed). * + * Ignore invariant: a checkout-local key is returned only once git ignores it. + * The root `.gitignore` entry is written before a new local key, so a failed + * write leaves no key behind, and an existing local key that git does not + * ignore is repaired or refused (see `ensureProjectEnvKeyIgnored`). + * * Concurrency: when no key exists, creation in a git checkout runs under a * cooperative lock at `/.hack.secret.key.lock`, so concurrent * first users across primary and linked checkouts converge on one key. Legacy @@ -3881,6 +3927,13 @@ async function ensureProjectEnvSecretKeyUnlocked(opts: { const existing = await readLegacyProjectEnvKeyFile({ path: keyPath }); if (existing !== null) { await assertProjectEnvKeyUntracked({ keyPath }); + // A key left un-ignored (an older Hack, a failed ignore write, a hand- + // copied key) is repaired before it is used, or refused. + await ensureProjectEnvKeyIgnored({ + projectRoot: opts.projectRoot, + keyPath, + creating: false, + }); return { keyPath, keyText: existing, @@ -3999,12 +4052,57 @@ async function writeProjectEnvKeyWithFallback(opts: { ); } + // The ignore entry comes first: if it cannot be written, no key file exists + // for a retry to pick up unignored. + await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); + await ensureProjectEnvKeyIgnored({ + projectRoot: opts.projectRoot, + keyPath: opts.localKeyPath, + creating: true, + }); const keyText = await writeProjectEnvKeyFile({ projectRoot: opts.projectRoot, path: opts.localKeyPath, keyText: opts.keyText, }); - await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); + emitProjectEnvKeyWarnings({ warnings }); + return { keyPath: opts.localKeyPath, keyText, warnings }; +} + +/** Fixed, value-free refusal for a checkout-local key git would commit. */ +function projectEnvKeyUnignoredError(opts: { + readonly rootGitignore: string; +}): ProjectEnvKeyRefusalError { + return new ProjectEnvKeyRefusalError( + `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${opts.rootGitignore}. Add a '${PROJECT_ENV_KEY_FILENAME}' line to that file (making it writable if needed) and remove any rule that re-includes the key, then retry; key contents are never included in this message.` + ); +} + +/** + * Keeps the checkout-local key file out of commits: makes sure the root + * `.gitignore` lists it and that git then ignores it. + * + * - `creating`: runs before a new key file is written. The entry is always + * ensured (also outside git, so a later `git init` ignores the key); a + * failed write is refused as that `.gitignore` write, and the caller writes + * no key. + * - otherwise (an existing key): only a key git positively does not ignore is + * repaired, so an ignored key or a checkout outside git causes no write. A + * failed repair is refused with {@link projectEnvKeyUnignoredError}, and the + * key is not used. + * + * Either way, a key git still does not ignore after the entry (for example a + * later `!.hack.secret.key` rule) is refused. + */ +async function ensureProjectEnvKeyIgnored(opts: { + readonly projectRoot: string; + readonly keyPath: string; + readonly creating: boolean; +}): Promise { + const probe = { projectRoot: opts.projectRoot, path: opts.keyPath }; + if (!opts.creating && (await isGitIgnoredProjectEnvPath(probe)) !== false) { + return; + } const rootGitignore = resolve(opts.projectRoot, ".gitignore"); try { await ensureGitignoreEntry({ @@ -4013,14 +4111,13 @@ async function writeProjectEnvKeyWithFallback(opts: { comment: "# project env key", }); } catch (error: unknown) { - throw closedFileWriteError({ - error, - path: rootGitignore, - operation: "write", - }); + throw opts.creating + ? closedFileWriteError({ error, path: rootGitignore, operation: "write" }) + : projectEnvKeyUnignoredError({ rootGitignore }); + } + if ((await isGitIgnoredProjectEnvPath(probe)) === false) { + throw projectEnvKeyUnignoredError({ rootGitignore }); } - emitProjectEnvKeyWarnings({ warnings }); - return { keyPath: opts.localKeyPath, keyText, warnings }; } /** diff --git a/tests/doctor-env-key.test.ts b/tests/doctor-env-key.test.ts index f3b9283cf..05a9c8c7b 100644 --- a/tests/doctor-env-key.test.ts +++ b/tests/doctor-env-key.test.ts @@ -20,6 +20,7 @@ import { type EnvKeyCandidate, type EnvKeyInspection, findNestedPublicArtifactDirs, + gitIsLocalKeyIgnored, gitListTrackedKeyPaths, type InspectEnvKeyOptions, inspectProjectEnvKey, @@ -68,6 +69,7 @@ async function inspect( env: {}, uid, listTrackedKeyPaths: async () => [], + isLocalKeyIgnored: async () => true, ...overrides, }); } @@ -258,6 +260,67 @@ describe("env key doctor diagnostic", () => { expect(serialized(inspection)).not.toContain("\u001b"); }); + test("a checkout-local key git does not ignore is a warning with a value-free remedy", async () => { + const root = await tempProject(); + const key = generatedKey(); + await writeKey({ path: join(root, KEY_FILE), text: key, mode: 0o600 }); + + const inspection = await inspect(root, { + isLocalKeyIgnored: async () => false, + }); + + expect(inspection.status).toBe("warn"); + expect(inspection.findings).toEqual([ + { + code: "env-key-unignored", + status: "warn", + subject: KEY_FILE, + remedy: `printf '\\n%s\\n' ${KEY_FILE} >> .gitignore, then confirm 'git status' no longer lists ${KEY_FILE}.`, + }, + ]); + expect(serialized(inspection)).toContain( + "env-key-unignored: .hack.secret.key is not ignored by git" + ); + expectValueFree({ output: serialized(inspection), root, secrets: [key] }); + }); + + test("negative control: an ignored, unknown or absent local key is not reported as unignored", async () => { + const root = await tempProject(); + // Absent local key: the probe is not consulted at all. + expect( + codes(await inspect(root, { isLocalKeyIgnored: async () => false })) + ).toEqual(["env-key-absent"]); + + await writeKey({ + path: join(root, KEY_FILE), + text: generatedKey(), + mode: 0o600, + }); + for (const probe of [ + async () => true, + async () => null, + () => Promise.reject(new Error("git exploded")), + ]) { + expect(codes(await inspect(root, { isLocalKeyIgnored: probe }))).toEqual([ + "env-key-ok", + ]); + } + }); + + test("only the checkout-local key is checked for ignore rules", async () => { + const root = await tempProject(); + const sharedPath = join(root, "git-common", KEY_FILE); + await mkdir(join(root, "git-common")); + await writeKey({ path: sharedPath, text: generatedKey(), mode: 0o600 }); + + const inspection = await inspect(root, { + candidates: [localCandidate(root), { role: "shared", path: sharedPath }], + isLocalKeyIgnored: async () => false, + }); + + expect(codes(inspection)).toEqual(["env-key-ok"]); + }); + test("an unavailable git listing is not reported as tracked", async () => { const root = await tempProject(); await writeKey({ @@ -675,3 +738,57 @@ describe("gitListTrackedKeyPaths", () => { expect(await gitListTrackedKeyPaths({ projectRoot: root })).toBeNull(); }); }); + +/** + * Runs `run` with git's global and system configuration (and so any personal + * excludes file) out of reach, restoring the previous environment. + */ +async function withPrivateGitEnvironment( + home: string, + run: () => Promise +): Promise { + const overrides: Record = { + HOME: home, + XDG_CONFIG_HOME: join(home, ".config"), + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }; + const previous = Object.fromEntries( + Object.keys(overrides).map((key) => [key, process.env[key]]) + ); + Object.assign(process.env, overrides); + try { + return await run(); + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) { + Reflect.deleteProperty(process.env, key); + } else { + process.env[key] = value; + } + } + } +} + +describe("gitIsLocalKeyIgnored", () => { + test("follows the work tree's ignore rules and is null outside git", async () => { + const root = await tempProject(); + const repo = join(root, "repo"); + const outside = join(root, "outside"); + await mkdir(repo); + await mkdir(outside); + await withPrivateGitEnvironment(join(root, "home"), async () => { + expect(await gitIsLocalKeyIgnored({ projectRoot: outside })).toBeNull(); + const init = Bun.spawn(["git", "-C", repo, "init", "-q"], { + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + }); + expect(await init.exited).toBe(0); + await writeFile(join(repo, KEY_FILE), "x"); + expect(await gitIsLocalKeyIgnored({ projectRoot: repo })).toBe(false); + await writeFile(join(repo, ".gitignore"), `${KEY_FILE}\n`); + expect(await gitIsLocalKeyIgnored({ projectRoot: repo })).toBe(true); + }); + }); +}); diff --git a/tests/project-env-key-ignore.test.ts b/tests/project-env-key-ignore.test.ts new file mode 100644 index 000000000..d8f2de92c --- /dev/null +++ b/tests/project-env-key-ignore.test.ts @@ -0,0 +1,384 @@ +import { afterEach, expect, test } from "bun:test"; +import { + chmod, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, + PROJECT_ENV_KEY_FILENAME, +} from "../src/constants.ts"; +import { resolveProjectEnvConfigPath } from "../src/lib/project-env-config.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1375: the checkout-local `.hack.secret.key` and its root `.gitignore` + * entry are atomic in effect. When the entry cannot be written, no key is + * created, so the retry the refusal suggests cannot succeed with a key git + * would commit; and an existing key git does not ignore is repaired before a + * `--secret` write uses it, or refused. + * + * Every child runs the source CLI with a private HOME, HACK_HOME and Docker + * client config, keychain fallback disabled, and git's global and system + * configuration out of reach. Each fixture writes the env layer first so + * `--secret` uses the project key, never the keychain. Values are synthetic. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const FIRST_VALUE = ["synthetic", "first", "c41e"].join("-"); +const NEXT_VALUE = ["synthetic", "next", "9b07"].join("-"); +const LAYER = + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global:\n PLAIN_SETTING: kept\n"; +const BASE_GITIGNORE = "node_modules/\n"; + +/** Permission bits do not bind an effective uid of 0. */ +const RUNS_AS_ROOT = (process.geteuid?.() ?? process.getuid?.() ?? -1) === 0; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const tempDir of tempDirs) { + const entries = await readdir(tempDir, { + recursive: true, + withFileTypes: true, + }).catch(() => []); + for (const entry of entries) { + if (entry.isFile()) { + await chmod(join(entry.parentPath, entry.name), 0o600).catch( + () => undefined + ); + } + } + await rm(tempDir, { recursive: true, force: true }); + } + tempDirs.clear(); +}); + +type Repo = { + readonly root: string; + readonly projectRoot: string; + readonly projectDir: string; + readonly layer: string; + readonly keyPath: string; + readonly gitignore: string; +}; + +function gitEnv(root: string): Record { + return privateCliEnv({ + root, + env: { + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }, + }); +} + +async function git( + repo: Repo, + args: readonly string[] +): Promise<{ readonly exitCode: number; readonly stdout: string }> { + const proc = Bun.spawn(["git", "-C", repo.projectRoot, ...args], { + env: gitEnv(repo.root), + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + }); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + return { exitCode, stdout }; +} + +/** A project with its env layer and root `.gitignore`, in git unless `git: false`. */ +async function createRepo(opts: { readonly git: boolean }): Promise { + const root = await mkdtemp(join(tmpdir(), "hack-env-key-ignore-")); + tempDirs.add(root); + const projectRoot = resolve(root, "repo"); + const projectDir = resolve(projectRoot, ".hack"); + await mkdir(projectDir, { recursive: true }); + await writeFile( + resolve(projectDir, PROJECT_COMPOSE_FILENAME), + "services:\n api:\n image: alpine:3.20\n" + ); + await writeFile( + resolve(projectDir, PROJECT_CONFIG_FILENAME), + `${JSON.stringify({ name: "env-key-ignore", dev_host: "env-key-ignore.hack" }, null, 2)}\n` + ); + const layer = resolveProjectEnvConfigPath({ projectDir, envName: null }); + await writeFile(layer, LAYER); + const gitignore = resolve(projectRoot, ".gitignore"); + await writeFile(gitignore, BASE_GITIGNORE); + const repo: Repo = { + root, + projectRoot, + projectDir, + layer, + keyPath: resolve(projectRoot, PROJECT_ENV_KEY_FILENAME), + gitignore, + }; + if (opts.git) { + expect((await git(repo, ["init", "-q"])).exitCode).toBe(0); + } + return repo; +} + +type CliRun = { + readonly exitCode: number; + readonly stdout: string; + readonly stderr: string; +}; + +async function runCli(opts: { + readonly repo: Repo; + readonly args: readonly string[]; + readonly stdin: string; +}): Promise { + const proc = Bun.spawn( + [ + process.execPath, + ENTRYPOINT, + ...opts.args, + "--path", + opts.repo.projectRoot, + ], + { + cwd: opts.repo.projectRoot, + env: privateCliEnv({ + root: opts.repo.root, + env: { + HACK_HOME: join(opts.repo.root, "hack-home"), + HACK_GLOBAL_CONFIG_PATH: join(opts.repo.root, "hack-config.json"), + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + HACK_LAUNCHD_DISABLE: "1", + HACK_LOGGER: "console", + HACK_NO_INTERACTIVE: "1", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }, + }), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + } + ); + proc.stdin.write(opts.stdin); + await proc.stdin.end(); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { exitCode, stdout, stderr }; +} + +async function setSecret( + repo: Repo, + key: string, + value: string +): Promise { + return await runCli({ + repo, + args: ["env", "set", key, "--secret", "--value-stdin"], + stdin: `${value}\n`, + }); +} + +async function keyExists(repo: Repo): Promise { + return (await lstat(repo.keyPath).catch(() => null)) !== null; +} + +/** Whether `git status` offers the key for commit. */ +async function keyIsCommittable(repo: Repo): Promise { + const status = await git(repo, [ + "status", + "--porcelain", + "--untracked-files=all", + ]); + expect(status.exitCode).toBe(0); + return status.stdout + .split("\n") + .some((line) => line.endsWith(` ${PROJECT_ENV_KEY_FILENAME}`)); +} + +function expectOneLineRefusal(run: CliRun, expected: string): void { + expect(run.exitCode).toBe(1); + const lines = `${run.stdout}\n${run.stderr}` + .split("\n") + .filter((line) => /[A-Za-z]/.test(line)); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(expected); + const output = `${run.stdout}${run.stderr}`; + expect(output).not.toMatch(/\bat\s.*\.ts:\d+/); + expect(output).not.toContain(FIRST_VALUE); + expect(output).not.toContain(NEXT_VALUE); +} + +function expectNoKeyMaterial(run: CliRun, keyText: string): void { + expect(keyText.length).toBeGreaterThan(0); + expect(`${run.stdout}${run.stderr}`).not.toContain(keyText); +} + +function unignoredRefusal(repo: Repo): string { + return `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${repo.gitignore}.`; +} + +/** Creates the key with a first secret, then drops its ignore entry (the pre-fix state). */ +async function createUnignoredKey(repo: Repo): Promise { + const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE); + expect(first.exitCode).toBe(0); + const keyText = (await readFile(repo.keyPath, "utf8")).trim(); + await writeFile(repo.gitignore, BASE_GITIGNORE); + expect(await keyIsCommittable(repo)).toBe(true); + return keyText; +} + +test.skipIf(RUNS_AS_ROOT)( + "a read-only root .gitignore refuses the first secret without creating the key, and the retry after fixing it ignores the key", + async () => { + const repo = await createRepo({ git: true }); + await chmod(repo.gitignore, 0o444); + + // The reported sequence: refuse, then retry as the refusal suggests. + for (let attempt = 0; attempt < 2; attempt += 1) { + const run = await setSecret(repo, "NEW_SECRET", NEXT_VALUE); + expectOneLineRefusal( + run, + `E_PERMISSION: Could not write ${repo.gitignore}: permission denied. Make the file and its directory writable by your user, then retry.` + ); + expect(await keyExists(repo)).toBe(false); + expect(await readFile(repo.layer, "utf8")).toBe(LAYER); + } + expect(await keyIsCommittable(repo)).toBe(false); + + await chmod(repo.gitignore, 0o644); + const retry = await setSecret(repo, "NEW_SECRET", NEXT_VALUE); + expect(retry.exitCode).toBe(0); + expect(await keyExists(repo)).toBe(true); + expectNoKeyMaterial(retry, (await readFile(repo.keyPath, "utf8")).trim()); + expect(await readFile(repo.gitignore, "utf8")).toBe( + `${BASE_GITIGNORE}\n# project env key\n${PROJECT_ENV_KEY_FILENAME}\n` + ); + expect(await keyIsCommittable(repo)).toBe(false); + expect(await readFile(repo.layer, "utf8")).toContain("NEW_SECRET"); + }, + 60_000 +); + +test("an existing key git does not ignore is repaired by the next secret set", async () => { + const repo = await createRepo({ git: true }); + const keyText = await createUnignoredKey(repo); + + const run = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); + + expect(run.exitCode).toBe(0); + expectNoKeyMaterial(run, keyText); + expect((await readFile(repo.keyPath, "utf8")).trim()).toBe(keyText); + expect(await readFile(repo.gitignore, "utf8")).toContain( + `\n${PROJECT_ENV_KEY_FILENAME}\n` + ); + expect(await keyIsCommittable(repo)).toBe(false); + expect(await readFile(repo.layer, "utf8")).toContain("NEXT_SECRET"); +}, 60_000); + +test("secret hydration repairs an existing key git does not ignore", async () => { + const repo = await createRepo({ git: true }); + const keyText = await createUnignoredKey(repo); + + const run = await runCli({ + repo, + args: ["env", "hydrate", "--secret"], + stdin: `HYDRATED_SECRET=${NEXT_VALUE}\n`, + }); + + expect(run.exitCode).toBe(0); + expectNoKeyMaterial(run, keyText); + expect(await keyIsCommittable(repo)).toBe(false); +}, 60_000); + +test.skipIf(RUNS_AS_ROOT)( + "an existing unignored key with a read-only root .gitignore is refused before use", + async () => { + const repo = await createRepo({ git: true }); + const keyText = await createUnignoredKey(repo); + const layerBefore = await readFile(repo.layer, "utf8"); + await chmod(repo.gitignore, 0o444); + + const run = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); + + expectOneLineRefusal(run, unignoredRefusal(repo)); + expectNoKeyMaterial(run, keyText); + expect(await readFile(repo.layer, "utf8")).toBe(layerBefore); + expect(await readFile(repo.gitignore, "utf8")).toBe(BASE_GITIGNORE); + expect((await readFile(repo.keyPath, "utf8")).trim()).toBe(keyText); + }, + 60_000 +); + +test.skipIf(RUNS_AS_ROOT)( + "negative control: a key ignored by another rule needs no root .gitignore write", + async () => { + const repo = await createRepo({ git: true }); + const keyText = await createUnignoredKey(repo); + await writeFile( + resolve(repo.projectRoot, ".git", "info", "exclude"), + `${PROJECT_ENV_KEY_FILENAME}\n` + ); + expect(await keyIsCommittable(repo)).toBe(false); + await chmod(repo.gitignore, 0o444); + + const run = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); + + expect(run.exitCode).toBe(0); + expectNoKeyMaterial(run, keyText); + expect(await readFile(repo.gitignore, "utf8")).toBe(BASE_GITIGNORE); + }, + 60_000 +); + +test("a rule that re-includes the key is refused both before creating a key and before using one", async () => { + const repo = await createRepo({ git: true }); + const reincluding = `${PROJECT_ENV_KEY_FILENAME}\n!${PROJECT_ENV_KEY_FILENAME}\n`; + await writeFile(repo.gitignore, reincluding); + + const first = await setSecret(repo, "NEW_SECRET", NEXT_VALUE); + expectOneLineRefusal(first, unignoredRefusal(repo)); + expect(await keyExists(repo)).toBe(false); + expect(await readFile(repo.layer, "utf8")).toBe(LAYER); + + // An existing key under the same rule is refused before it encrypts. + await writeFile(repo.gitignore, BASE_GITIGNORE); + const keyText = await createUnignoredKey(repo); + await writeFile(repo.gitignore, reincluding); + const layerBefore = await readFile(repo.layer, "utf8"); + const next = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); + expectOneLineRefusal(next, unignoredRefusal(repo)); + expectNoKeyMaterial(next, keyText); + expect(await readFile(repo.layer, "utf8")).toBe(layerBefore); +}, 60_000); + +test("outside git an existing key is used without writing a root .gitignore", async () => { + const repo = await createRepo({ git: false }); + const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE); + expect(first.exitCode).toBe(0); + // A new key still gets its entry, so a later `git init` ignores it. + expect(await readFile(repo.gitignore, "utf8")).toContain( + `\n${PROJECT_ENV_KEY_FILENAME}\n` + ); + await rm(repo.gitignore); + + const run = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE); + + expect(run.exitCode).toBe(0); + expect(await lstat(repo.gitignore).catch(() => null)).toBeNull(); +}, 60_000); diff --git a/tests/project-env-write-refusals.test.ts b/tests/project-env-write-refusals.test.ts index 2c1f2dc7e..18beaa8e0 100644 --- a/tests/project-env-write-refusals.test.ts +++ b/tests/project-env-write-refusals.test.ts @@ -963,14 +963,15 @@ for (const logger of ["console", "clack", "gum"] as const) { ); // D2: the first secret must create .hack.secret.key in a repo root that - // refuses new files (publishPrivateFileExclusive's staging open). + // refuses new files. Its root .gitignore entry is written first + // (HACK-1375), so that write is the one refused. test.skipIf(RUNS_AS_ROOT)( `set --secret creating the key in a non-writable repo root prints one line with HACK_LOGGER=${logger}`, async () => { const repo = await createRepo(); const tracked = layerPath(repo, "tracked"); await writeFile(tracked, LAYER); - const keyPath = resolve(repo.projectRoot, ".hack.secret.key"); + const rootGitignore = resolve(repo.projectRoot, ".gitignore"); await chmod(repo.projectRoot, 0o555); let run: CliRun; try { @@ -983,7 +984,7 @@ for (const logger of ["console", "clack", "gum"] as const) { } finally { await chmod(repo.projectRoot, 0o755); } - expectOneLineRefusal(run, permissionWriteRefusal(keyPath)); + expectOneLineRefusal(run, permissionWriteRefusal(rootGitignore)); expectNoErrnoOrStaging(run); expect(await readdir(repo.projectRoot)).toEqual([".hack"]); expect(await readFile(tracked, "utf8")).toBe(LAYER); @@ -1508,7 +1509,14 @@ for (const [step, code] of [ `E_NO_SPACE: Could not write ${keyPath}: ${NO_SPACE_REASON}` ); expectNoErrnoOrStaging(run); - expect(await readdir(repo.projectRoot)).toEqual([".hack"]); + // The ignore entry is written before the key (HACK-1375) and stays. + expect((await readdir(repo.projectRoot)).sort()).toEqual([ + ".gitignore", + ".hack", + ]); + expect( + await readFile(resolve(repo.projectRoot, ".gitignore"), "utf8") + ).toBe("# project env key\n.hack.secret.key\n"); expect(await readFile(tracked, "utf8")).toBe(LAYER); }, 60_000); } @@ -1544,8 +1552,8 @@ test("an injected EACCES at the .hack/.gitignore lstat is a closed read refusal" expect(await readFile(local, "utf8")).toBe(LAYER); }, 60_000); -// The root `.gitignore` entry written after a new key. The key itself is -// left behind unignored when this write fails; that is tracked separately. +// The root `.gitignore` entry is written before a new key (HACK-1375), so a +// full disk refuses before any key file exists for a retry to pick up. test("an injected ENOSPC writing the root .gitignore is a closed refusal", async () => { const repo = await createRepo(); const tracked = layerPath(repo, "tracked"); @@ -1564,7 +1572,7 @@ test("an injected ENOSPC writing the root .gitignore is a closed refusal", async `E_NO_SPACE: Could not write ${rootGitignore}: ${NO_SPACE_REASON}` ); expectNoErrnoOrStaging(run); - expect(await readdir(repo.projectRoot)).not.toContain(".gitignore"); + expect(await readdir(repo.projectRoot)).toEqual([".hack"]); expect(await readFile(tracked, "utf8")).toBe(LAYER); }, 60_000); From 4c6caa496dfc328d3b3bc4a75b6a9f66dcca8417 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Thu, 22 Oct 2026 20:41:48 -0400 Subject: [PATCH 7/7] fix(env): serialize env layer read-modify-write so concurrent sets keep every key Two concurrent `hack env set` commands with different keys on the same layer both exited 0 and one key was lost: each read the layer, changed it in memory and renamed a new file over it with no lock, so the last rename won (HACK-1376). Every env layer writer now holds a per-layer cooperative lock at `/.internal/.lock` for its whole read, change and write: `hack env set`/`add`/`unset`, `hack env hydrate`, and the legacy `.hack/.env` set and remove. The lock reuses the projects-registry owner protocol (exclusive hard-link receipt, dead-owner recovery only on ESRCH), waits at most 10 seconds and then refuses closed, naming only the layer, with nothing written. set and unset also compare-and-swap against the bytes they read, so a writer that takes no lock (an editor, Hack v4) turns into a retryable AtomicWriteConflictError instead of a silent overwrite. A symlinked `.internal` is refused; a layer whose directory is not writable runs unlocked, since no writer can replace it. Refs HACK-1376 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 38 +- src/lib/env-layer-lock.ts | 177 +++++++ src/lib/hack-env.ts | 83 ++- src/lib/project-env-config.ts | 91 +++- src/lib/projects-registry-lock.ts | 13 +- tests/project-env-hydrate.test.ts | 63 ++- tests/project-env-layer-lock.test.ts | 722 +++++++++++++++++++++++++++ 7 files changed, 1135 insertions(+), 52 deletions(-) create mode 100644 src/lib/env-layer-lock.ts create mode 100644 tests/project-env-layer-lock.test.ts diff --git a/docs/env.md b/docs/env.md index 45f1c4d07..1692cf3d6 100644 --- a/docs/env.md +++ b/docs/env.md @@ -634,6 +634,34 @@ hack env unset SERVICE_TOKEN --service api --env qa hack env unset API_BASE_URL --local ``` +### Concurrent writes to one env file + +Hack env commands that change an env file never silently lose another command's change. +`hack env set`, `hack env add`, `hack env unset` and `hack env hydrate` each read the +file, change it and replace it while holding that file's write lock, +`.hack/.internal/.lock` (for example `.hack/.internal/hack.env.yaml.lock`). +The same applies to the legacy `.hack/.env` files. So when several commands change +different keys of the same file at once, every change is kept. A command +that finds the lock held waits up to 10 seconds, then stops with a one-line message that +names the file and says nothing was written; run it again. The lock is released when the +command ends. If a command is killed while it holds the lock, the next command takes the +lock over once it confirms that process has exited. + +`.hack/.internal/` is ignored by the managed `.hack/.gitignore`, so lock files never show +in `git status`. If `.hack/.internal` is a symlink or not a directory, these commands +refuse instead of creating the lock through it. If you cannot write the directory that +holds the env file (for example a read-only checkout), no lock is taken: no command can +replace the file there, so a command that changes nothing still succeeds and one that +would change the file is refused as before. + +Programs that do not take this lock, such as an editor or Hack v4, are not kept out. For +`hack.env*.yaml` files, `hack env set`, `add`, `unset` and `hydrate` also check that the +file still has the contents they read just before replacing it. If it changed, the +command writes nothing and stops with a retryable conflict ("The file changed while it +was being replaced; nothing was written."), or, for `hydrate`, a `concurrent_write` +refusal. That check narrows the window in which such a program's write can be +overwritten, but cannot close it. + ### Hydrate machine-local values in one step `hack env hydrate` imports many values into the machine-local layer at once, for example @@ -701,15 +729,17 @@ Hydration refuses, without changing the layer, when: re-run with `--secret`, or remove the key with `hack env unset --local` first - another `hack env hydrate` for the same project is running, or an interrupted one left a stale lock (see below) -- the local layer changed between hydration's read and its rename, for example from a - concurrent `hack env set --local` +- the local layer changed between hydration's read and its rename, by a program that + does not take the env file's write lock (for example an editor or Hack v4) +- hydration waited 10 seconds for the env file's write lock without getting it Concurrent hydrations are serialized by `.hack/.internal/env-hydrate.lock`, which holds the owner's process ID. A hydration that is killed (for example with SIGKILL) cannot remove it; the next hydration then reports the stale lock and its path instead of taking it over. Remove the file once no hydration is running. `hack env set --local` does not -take this lock. Hydration checks the layer again just before its rename, which narrows -the window in which such a write could be overwritten but cannot close it. +take this lock, but hydration also holds the local file's write lock (see +[Concurrent writes to one env file](#concurrent-writes-to-one-env-file)), so a +concurrent `hack env set --local` waits for the hydration and both changes are kept. Materialize a compatibility `.hack/.env`: diff --git a/src/lib/env-layer-lock.ts b/src/lib/env-layer-lock.ts new file mode 100644 index 000000000..0daa22397 --- /dev/null +++ b/src/lib/env-layer-lock.ts @@ -0,0 +1,177 @@ +import { constants } from "node:fs"; +import { access, lstat } from "node:fs/promises"; +import { basename, dirname, join } from "node:path"; +import { ClosedCliError } from "./cli-result.ts"; +import { closedFileWriteError, ensureDirForWrite } from "./fs.ts"; +import { isRecord } from "./guards.ts"; +import { + ProjectsRegistryLockTimeoutError, + withProjectsRegistryLock, +} from "./projects-registry-lock.ts"; + +/** Default bounded wait for another writer of the same env layer. */ +export const ENV_LAYER_LOCK_WAIT_MS = 10_000; + +/** + * Another `hack env` writer held the layer's lock for the whole bounded wait + * (or an interrupted lock recovery needs offline inspection). Closed and + * retryable: nothing was read or written, and the message names only the + * layer path, never a key or value. + */ +export class EnvLayerLockTimeoutError extends ClosedCliError { + constructor(opts: { readonly path: string; readonly waitMs: number }) { + super( + `Another hack env command is writing ${opts.path}; gave up after waiting ${formatWait(opts.waitMs)} for it. Nothing was written. Retry when it finishes.` + ); + this.name = "EnvLayerLockTimeoutError"; + } +} + +/** + * A `.internal` entry next to the layer that is a symlink or not a directory. + * The lock is never created through it. + */ +export class EnvLayerLockRefusedError extends ClosedCliError { + constructor(opts: { readonly path: string }) { + super( + `Refused to update ${opts.path}: the .internal entry beside it is a symlink or not a directory, so its write lock cannot be taken safely. Replace it with a regular directory and retry. Nothing was written.` + ); + this.name = "EnvLayerLockRefusedError"; + } +} + +/** + * Lock path for one env layer file: `/.internal/.lock`. + * `.internal/` is in Hack's managed `.hack/.gitignore`, so the lock and its + * short-lived `.owner` staging names never show up in `git status`. + */ +export function resolveEnvLayerLockPath(opts: { + readonly layerPath: string; +}): string { + return join( + dirname(opts.layerPath), + ".internal", + `${basename(opts.layerPath)}.lock` + ); +} + +/** + * Serializes one read-modify-write of an env layer file (`hack.env*.yaml`, + * `.hack/.env*`) across processes. + * + * Contract: + * - `run` must read the layer, compute the change and write it; it runs while + * this process exclusively holds the layer's cooperative lock, so two + * writers that both use this helper never lose each other's update (the + * second reads what the first wrote). + * - The lock is the projects-registry owner protocol: an exclusive hard-link + * publish of a complete `pid + nonce` receipt, released only by its owner, + * reclaimed only after `kill(pid, 0)` reports the owner gone (ESRCH). + * - The wait is bounded (`timeoutMs`, default {@link ENV_LAYER_LOCK_WAIT_MS}) + * with jittered polling. When it runs out, {@link EnvLayerLockTimeoutError} + * is thrown and `run` never starts, so nothing is written. + * - A symlinked or non-directory `.internal` beside the layer is refused with + * {@link EnvLayerLockRefusedError}; a filesystem failure while taking the + * lock (permission, full disk) is refused as the layer it blocked, never as + * the lock's random staging name. Errors from `run` propagate unchanged. + * - Exception: when the lock cannot be taken and the layer's own directory is + * not writable (a read-only checkout or mount), `run` runs without the lock. + * No Hack writer can replace that layer, so nothing can be lost, a no-op + * set or unset still succeeds and a real change is refused by the write. + * - Writers that do not take this lock (Hack v4, editors) are not excluded; + * callers additionally compare-and-swap against the bytes they read. + */ +export async function withEnvLayerLock(opts: { + readonly layerPath: string; + readonly run: () => Promise; + /** Bounded wait override; tests only. */ + readonly timeoutMs?: number; +}): Promise { + const lockPath = resolveEnvLayerLockPath(opts); + const waitMs = opts.timeoutMs ?? ENV_LAYER_LOCK_WAIT_MS; + let started = false; + try { + await ensureEnvLayerLockDirectory({ + layerPath: opts.layerPath, + lockDir: dirname(lockPath), + }); + return await withProjectsRegistryLock({ + lockPath, + label: "env layer", + timeoutMs: waitMs, + run: async () => { + started = true; + return await opts.run(); + }, + }); + } catch (error: unknown) { + if (started || error instanceof EnvLayerLockRefusedError) { + throw error; + } + if (error instanceof ProjectsRegistryLockTimeoutError) { + throw new EnvLayerLockTimeoutError({ path: opts.layerPath, waitMs }); + } + if (!(await directoryWritable(dirname(opts.layerPath)))) { + // No Hack writer can replace a layer whose directory it cannot write + // (every replace stages a sibling and renames it), so there is nothing + // to serialize: run unlocked, so a no-op set or unset still succeeds + // and a real change is refused by the writer itself, as before. + return await opts.run(); + } + throw closedFileWriteError({ + error, + path: opts.layerPath, + operation: "write", + }); + } +} + +async function directoryWritable(path: string): Promise { + try { + await access(path, constants.W_OK); + return true; + } catch { + return false; + } +} + +async function ensureEnvLayerLockDirectory(opts: { + readonly layerPath: string; + readonly lockDir: string; +}): Promise { + if (!(await lockDirectoryState(opts)).absent) { + return; + } + await ensureDirForWrite(opts.lockDir); + // Re-check: something other than a directory may have raced the mkdir. + await lockDirectoryState(opts); +} + +async function lockDirectoryState(opts: { + readonly layerPath: string; + readonly lockDir: string; +}): Promise<{ readonly absent: boolean }> { + let isDirectory: boolean; + try { + isDirectory = (await lstat(opts.lockDir)).isDirectory(); + } catch (error: unknown) { + if (isRecord(error) && error.code === "ENOENT") { + return { absent: true }; + } + throw closedFileWriteError({ + error, + path: opts.layerPath, + operation: "write", + }); + } + if (!isDirectory) { + throw new EnvLayerLockRefusedError({ path: opts.layerPath }); + } + return { absent: false }; +} + +function formatWait(waitMs: number): string { + return waitMs >= 1000 && waitMs % 1000 === 0 + ? `${waitMs / 1000}s` + : `${waitMs}ms`; +} diff --git a/src/lib/hack-env.ts b/src/lib/hack-env.ts index 8b4e11957..ad920c22f 100644 --- a/src/lib/hack-env.ts +++ b/src/lib/hack-env.ts @@ -6,6 +6,7 @@ import { } from "../constants.ts"; import { readControlPlaneConfig } from "../control-plane/sdk/config.ts"; import { parseDotEnv, serializeDotEnv } from "./env.ts"; +import { withEnvLayerLock } from "./env-layer-lock.ts"; import { readTextFile } from "./fs.ts"; import { ensureHackGitInfoExclude } from "./git-info-exclude.ts"; import { getString, isRecord } from "./guards.ts"; @@ -508,46 +509,78 @@ export function resolveEnvSecretKey(opts: { * machine-local files out of `git status` through the repository's * `info/exclude` (see `ensureHackGitInfoExclude`); it never writes the * working tree for that. + * + * Concurrency: the read, merge and write run under the file's env layer lock + * (`withEnvLayerLock`), so concurrent sets and removals of different keys all + * survive; a writer that waits longer than the bounded wait refuses closed + * without writing. */ export async function upsertDotEnvValue(opts: { readonly envFile: string; readonly key: string; readonly value: string; + /** Bounded wait for the layer lock; tests only. */ + readonly layerLockTimeoutMs?: number; }): Promise<{ readonly changed: boolean }> { - const existingText = - (await readPrivateEnvFile({ envFile: opts.envFile })) ?? ""; - const env = parseDotEnv(existingText); - const nextEnv: Record = { ...env, [opts.key]: opts.value }; - const nextText = serializeDotEnvStable(nextEnv); - // After the read, so a refused (for example symlinked) file never spawns git. - await ensureHackGitInfoExclude({ projectDir: dirname(opts.envFile) }); - return await writePrivateEnvFileIfChanged({ - envFile: opts.envFile, - text: nextText, + // Refuse a symlinked file or `.hack` before the lock creates anything. + await readPrivateEnvFile({ envFile: opts.envFile }); + return await withEnvLayerLock({ + layerPath: opts.envFile, + timeoutMs: opts.layerLockTimeoutMs, + run: async () => { + const existingText = + (await readPrivateEnvFile({ envFile: opts.envFile })) ?? ""; + const env = parseDotEnv(existingText); + const nextEnv: Record = { + ...env, + [opts.key]: opts.value, + }; + const nextText = serializeDotEnvStable(nextEnv); + // After the read, so a refused (for example symlinked) file never spawns git. + await ensureHackGitInfoExclude({ projectDir: dirname(opts.envFile) }); + return await writePrivateEnvFileIfChanged({ + envFile: opts.envFile, + text: nextText, + }); + }, }); } +/** Removes one key from a project dotenv file; locked like {@link upsertDotEnvValue}. */ export async function removeDotEnvKey(opts: { readonly envFile: string; readonly key: string; + /** Bounded wait for the layer lock; tests only. */ + readonly layerLockTimeoutMs?: number; }): Promise<{ readonly changed: boolean }> { - const existingText = - (await readPrivateEnvFile({ envFile: opts.envFile })) ?? ""; - const env = parseDotEnv(existingText); - if (!(opts.key in env)) { + const preflight = await readPrivateEnvFile({ envFile: opts.envFile }); + if (preflight === null || !(opts.key in parseDotEnv(preflight))) { + // Nothing to remove: take no lock and create nothing. return { changed: false }; } - const nextEnv: Record = {}; - for (const [key, value] of Object.entries(env)) { - if (key === opts.key) { - continue; - } - nextEnv[key] = value; - } - const nextText = serializeDotEnvStable(nextEnv); - return await writePrivateEnvFileIfChanged({ - envFile: opts.envFile, - text: nextText, + return await withEnvLayerLock({ + layerPath: opts.envFile, + timeoutMs: opts.layerLockTimeoutMs, + run: async () => { + const existingText = + (await readPrivateEnvFile({ envFile: opts.envFile })) ?? ""; + const env = parseDotEnv(existingText); + if (!(opts.key in env)) { + return { changed: false }; + } + const nextEnv: Record = {}; + for (const [key, value] of Object.entries(env)) { + if (key === opts.key) { + continue; + } + nextEnv[key] = value; + } + const nextText = serializeDotEnvStable(nextEnv); + return await writePrivateEnvFileIfChanged({ + envFile: opts.envFile, + text: nextText, + }); + }, }); } diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 63a29c6fb..bbf875dee 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -28,6 +28,7 @@ import { } from "../templates.ts"; import { ClosedCliError } from "./cli-result.ts"; import { parseDotEnv, serializeDotEnv } from "./env.ts"; +import { withEnvLayerLock } from "./env-layer-lock.ts"; import { AtomicWriteConflictError, closedFileWriteError, @@ -2557,18 +2558,23 @@ function opensToPlaintext(opts: { async function readProjectEnvLayerForMutation(opts: { readonly path: string; readonly environment: string; -}): Promise { +}): Promise<{ + readonly read: ProjectEnvConfigReadResult; + /** The exact bytes read (null when absent), for the compare-and-swap. */ + readonly text: string | null; +}> { + const text = await readAtomicWriteTargetText(opts.path); const read = parseProjectEnvConfigText({ path: opts.path, environment: opts.environment, - text: await readAtomicWriteTargetText(opts.path), + text, }); if (read.parseError) { throw new ClosedCliError( `Failed to parse ${opts.path}: ${read.parseError}` ); } - return read; + return { read, text }; } export async function setProjectEnvValue(opts: { @@ -2580,6 +2586,8 @@ export async function setProjectEnvValue(opts: { readonly value: string; readonly secret: boolean; readonly local?: boolean; + /** Bounded wait for the layer lock; tests only. */ + readonly layerLockTimeoutMs?: number; }): Promise { await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); @@ -2601,7 +2609,29 @@ export async function setProjectEnvValue(opts: { projectDir: opts.projectDir, envName: opts.envName, }); - const read = await readProjectEnvLayerForMutation({ + // The read, the change and the write form one critical section per layer, + // so a concurrent set or unset of another key is never lost (HACK-1376). + return await withEnvLayerLock({ + layerPath: filePath, + timeoutMs: opts.layerLockTimeoutMs, + run: async () => + await setLockedProjectEnvValue({ ...opts, scope, filePath }), + }); +} + +async function setLockedProjectEnvValue(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly envName: string | null; + readonly scope: string; + readonly key: string; + readonly value: string; + readonly secret: boolean; + readonly local?: boolean; + readonly filePath: string; +}): Promise { + const { filePath, scope } = opts; + const { read, text } = await readProjectEnvLayerForMutation({ path: filePath, environment: opts.envName ?? "default", }); @@ -2645,9 +2675,12 @@ export async function setProjectEnvValue(opts: { secretsprovider: PROJECT_ENV_SECRETS_PROVIDER, values: nextValues, }; + // The lock excludes cooperating writers; the compare-and-swap turns a write + // by anything else since the read into a closed, retryable conflict. const changed = await writeProjectEnvConfigFile({ path: filePath, config: nextConfig, + expectedExisting: text, }); return { filePath, @@ -2665,6 +2698,8 @@ export async function unsetProjectEnvValue(opts: { readonly scope: string; readonly key: string; readonly local?: boolean; + /** Bounded wait for the layer lock; tests only. */ + readonly layerLockTimeoutMs?: number; }): Promise<{ readonly changed: boolean; readonly filePath: string }> { await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); await assertLegacyProjectInputFamily({ projectRoot: opts.projectRoot }); @@ -2679,7 +2714,25 @@ export async function unsetProjectEnvValue(opts: { projectDir: opts.projectDir, envName: opts.envName, }); - const read = await readProjectEnvLayerForMutation({ + // Same per-layer critical section as `setProjectEnvValue` (HACK-1376). + return await withEnvLayerLock({ + layerPath: filePath, + timeoutMs: opts.layerLockTimeoutMs, + run: async () => await unsetLockedProjectEnvValue({ ...opts, filePath }), + }); +} + +async function unsetLockedProjectEnvValue(opts: { + readonly projectRoot: string; + readonly projectDir: string; + readonly envName: string | null; + readonly scope: string; + readonly key: string; + readonly local?: boolean; + readonly filePath: string; +}): Promise<{ readonly changed: boolean; readonly filePath: string }> { + const { filePath } = opts; + const { read, text } = await readProjectEnvLayerForMutation({ path: filePath, environment: opts.envName ?? "default", }); @@ -2718,6 +2771,7 @@ export async function unsetProjectEnvValue(opts: { secretsprovider: PROJECT_ENV_SECRETS_PROVIDER, values: nextValues, }, + expectedExisting: text, }); return { changed, filePath }; } @@ -2825,10 +2879,12 @@ export type ProjectEnvHydrationResult = { * rename, so the previous layer stays in place. * - Concurrency: hydrations of one project are serialized by an exclusive * lock file under `.internal/`; a second hydration refuses instead of - * waiting. The write is also a compare-and-swap against the bytes read, so a - * different writer that changed the layer meanwhile makes hydration refuse - * rather than overwrite it. A lock left by a killed process is reported, not - * removed. + * waiting. A lock left by a killed process is reported, not removed. Inside + * it, hydration takes the layer's write lock (`withEnvLayerLock`), the same + * one `hack env set`/`unset` hold, waiting a bounded time for them. The + * write is also a compare-and-swap against the bytes read, so a writer that + * takes no lock and changed the layer meanwhile makes hydration refuse + * rather than overwrite it. * - Wrong key: every secret already in the layer must decrypt under the key * readers would use, or hydration refuses. Secret mode encrypts with the key * file `hack env add --secret` uses and refuses to create one while only @@ -2888,11 +2944,18 @@ export async function hydrateProjectEnvLocalLayer(opts: { refuse, }); try { - return await hydrateLockedProjectEnvLayer({ - ...opts, - scope, - filePath, - refuse, + // Also take the per-layer lock that `hack env set`/`unset` hold, so a + // concurrent set of another key waits instead of making hydration refuse + // as `concurrent_write` (or being overwritten). + return await withEnvLayerLock({ + layerPath: filePath, + run: async () => + await hydrateLockedProjectEnvLayer({ + ...opts, + scope, + filePath, + refuse, + }), }); } finally { await release(); diff --git a/src/lib/projects-registry-lock.ts b/src/lib/projects-registry-lock.ts index 4448879fe..e7e70fe8a 100644 --- a/src/lib/projects-registry-lock.ts +++ b/src/lib/projects-registry-lock.ts @@ -17,6 +17,17 @@ const WAIT_MS = 10_000; const OWNER_LIMIT = 128; const OWNER_PATTERN = /^([1-9]\d*)\n(?:[a-f0-9-]{36}\n)?$/; +/** + * The bounded wait for a cooperative lock ran out while another live writer + * (or an interrupted recovery) held it. Nothing guarded by the lock ran. + */ +export class ProjectsRegistryLockTimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = "ProjectsRegistryLockTimeoutError"; + } +} + interface Owner { readonly info: Stats; readonly bytes: string; @@ -123,7 +134,7 @@ async function acquire(opts: { const recovery = recoveryBusy ? "; recovery guard is busy (interrupted recovery requires offline inspection)" : ""; - throw new Error( + throw new ProjectsRegistryLockTimeoutError( `Timed out waiting for ${opts.label ?? "projects registry"} lock${recovery}` ); } diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts index 39d1b3619..869eefc29 100644 --- a/tests/project-env-hydrate.test.ts +++ b/tests/project-env-hydrate.test.ts @@ -704,8 +704,52 @@ test("a layer changed by another writer during an import is not overwritten", as const worker = spawnWorker(repo, "staging"); await readUntil(worker.stdout, "prepared\n"); - // A concurrent `hack env set --local` commits its own change meanwhile. - await setProjectEnvValue({ + // A writer that takes no lock (an editor, Hack v4) changes the layer + // meanwhile; only the compare-and-swap can notice it. + const edited = layerValues(await readFile(repo.localFile, "utf8")); + edited.global = { ...edited.global, OPERATOR_OVERRIDE: "operator-value" }; + await writeFile( + repo.localFile, + YAML.stringify( + { + version: 1, + environment: "default", + secretsprovider: "project_key", + values: edited, + }, + null, + 2 + ) + ); + const concurrent = await readFile(repo.localFile); + await writeFile(resolve(repo.projectDir, "go"), ""); + + const [stdout, exitCode] = await Promise.all([ + readUntil(worker.stdout, "\n"), + worker.exited, + ]); + expect(exitCode).toBe(0); + expect(stdout).toBe("refused:concurrent_write\n"); + // The other writer's layer stands; no imported key was applied. + expect((await readFile(repo.localFile)).equals(concurrent)).toBe(true); + expect(await resolvedGlobal(repo)).toMatchObject({ + API_TOKEN: TOKEN, + OPERATOR_OVERRIDE: "operator-value", + }); + expect((await resolvedGlobal(repo)).DB_PASS).toBeUndefined(); + expect(await stagingFiles(repo)).toEqual([]); + expect(await lockExists(repo)).toBe(false); +}, 30_000); + +test("a `hack env set --local` during an import waits for it, and both changes survive", async () => { + const repo = await createRepo(); + await hydrate(repo, { API_TOKEN: TOKEN }); + + const worker = spawnWorker(repo, "staging"); + await readUntil(worker.stdout, "prepared\n"); + // The paused import holds the layer lock, so this set must wait for it. + let setSettled = false; + const pendingSet = setProjectEnvValue({ projectRoot: repo.projectRoot, projectDir: repo.projectDir, envName: null, @@ -714,8 +758,11 @@ test("a layer changed by another writer during an import is not overwritten", as value: "operator-value", secret: false, local: true, + }).finally(() => { + setSettled = true; }); - const concurrent = await readFile(repo.localFile); + await Bun.sleep(300); + expect(setSettled).toBe(false); await writeFile(resolve(repo.projectDir, "go"), ""); const [stdout, exitCode] = await Promise.all([ @@ -723,14 +770,14 @@ test("a layer changed by another writer during an import is not overwritten", as worker.exited, ]); expect(exitCode).toBe(0); - expect(stdout).toBe("refused:concurrent_write\n"); - // The other writer's layer stands; no imported key was applied. - expect((await readFile(repo.localFile)).equals(concurrent)).toBe(true); + expect(stdout).toBe("committed\n"); + expect((await pendingSet).changed).toBe(true); + // Neither writer lost the other's update. expect(await resolvedGlobal(repo)).toMatchObject({ - API_TOKEN: TOKEN, + API_TOKEN: CHANGED, + DB_PASS: CHANGED, OPERATOR_OVERRIDE: "operator-value", }); - expect((await resolvedGlobal(repo)).DB_PASS).toBeUndefined(); expect(await stagingFiles(repo)).toEqual([]); expect(await lockExists(repo)).toBe(false); }, 30_000); diff --git a/tests/project-env-layer-lock.test.ts b/tests/project-env-layer-lock.test.ts new file mode 100644 index 000000000..c98af390c --- /dev/null +++ b/tests/project-env-layer-lock.test.ts @@ -0,0 +1,722 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { + chmod, + mkdir, + mkdtemp, + readdir, + readFile, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { basename, dirname, join, resolve } from "node:path"; +import { YAML } from "bun"; + +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, +} from "../src/constants.ts"; +import { ClosedCliError } from "../src/lib/cli-result.ts"; +import { parseDotEnv } from "../src/lib/env.ts"; +import { + EnvLayerLockRefusedError, + EnvLayerLockTimeoutError, + resolveEnvLayerLockPath, +} from "../src/lib/env-layer-lock.ts"; +import { + AtomicWriteConflictError, + AtomicWriteRefusedError, +} from "../src/lib/fs.ts"; +import { removeDotEnvKey, upsertDotEnvValue } from "../src/lib/hack-env.ts"; +import { + resolveProjectEnvConfig, + resolveProjectEnvConfigPath, + resolveProjectEnvLocalConfigPath, + resolveProjectEnvSharedKeyLocation, + setProjectEnvValue, + unsetProjectEnvValue, +} from "../src/lib/project-env-config.ts"; +import { restoreEnv } from "./helpers/env.ts"; +import { privateCliEnv, privateCliHome } from "./helpers/private-cli-env.ts"; +import { createSecretWorktreeFixture } from "./helpers/secret-fixture.ts"; + +/** + * HACK-1376: concurrent writers of one env layer must not lose each other's + * update. Before the fix, `hack env set` read the layer, changed it in memory + * and renamed a new file over it with no lock, so the last rename won and the + * other key silently disappeared while both commands exited 0. + * + * Stress cases start WORKERS fresh processes (no shared module state), hold + * them at a stdin barrier and release them together, then require that every + * process succeeded and every key is present. Removing the layer lock from + * `setProjectEnvValue`/`unsetProjectEnvValue`/`upsertDotEnvValue` makes them + * fail (lost keys, or compare-and-swap conflicts). + * + * Isolation: every case runs in a temp root with a private HOME, HACK_HOME, + * global config and Docker client config; the keychain fallback is disabled + * and git reads no global or system config. Every value is a synthetic test + * string; no output is checked for values beyond proving their absence. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const PROJECT_ENV_MODULE = resolve( + REPO_ROOT, + "src", + "lib", + "project-env-config.ts" +); +const HACK_ENV_MODULE = resolve(REPO_ROOT, "src", "lib", "hack-env.ts"); +const WORKERS = 8; +const PARENT_ENV_KEYS = [ + "HOME", + "HACK_HOME", + "HACK_GLOBAL_CONFIG_PATH", + "HACK_SECRETS_FILE_KEY", + "HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK", + "HACK_ENV_SECRET_KEY", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_GLOBAL", +] as const; + +let root: string | null = null; +const previousEnv = new Map(); + +beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), "hack-env-layer-lock-")); + for (const key of PARENT_ENV_KEYS) { + previousEnv.set(key, process.env[key]); + } + process.env.HOME = privateCliHome(root); + process.env.HACK_HOME = join(root, "hack-home"); + process.env.HACK_GLOBAL_CONFIG_PATH = join(root, "hack.config.json"); + process.env.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK = "1"; + process.env.GIT_CONFIG_NOSYSTEM = "1"; + process.env.GIT_CONFIG_GLOBAL = "/dev/null"; + Reflect.deleteProperty(process.env, "HACK_SECRETS_FILE_KEY"); + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); +}); + +afterEach(async () => { + if (root) { + await rm(root, { recursive: true, force: true }); + root = null; + } + for (const key of PARENT_ENV_KEYS) { + restoreEnv(key, previousEnv.get(key)); + } + previousEnv.clear(); +}); + +function requireRoot(): string { + if (!root) { + throw new Error("Missing temp root"); + } + return root; +} + +type Project = { + readonly projectRoot: string; + readonly projectDir: string; +}; + +/** A non-git Hack project; the env layer file is created by each case. */ +async function createProject(): Promise { + const projectRoot = join(requireRoot(), "project"); + const projectDir = join(projectRoot, ".hack"); + await mkdir(projectDir, { recursive: true }); + await writeFile( + join(projectDir, PROJECT_COMPOSE_FILENAME), + "services:\n api:\n image: alpine:3.20\n" + ); + await writeFile( + join(projectDir, PROJECT_CONFIG_FILENAME), + `${JSON.stringify({ name: "layer-lock", dev_host: "layer-lock.hack" }, null, 2)}\n` + ); + return { projectRoot, projectDir }; +} + +/** The bytes Hack itself writes for this layer, so a no-op leaves them unchanged. */ +function layerText(global: Readonly>): string { + const yaml = YAML.stringify( + { + version: 1, + environment: "default", + secretsprovider: "project_key", + values: { global }, + }, + null, + 2 + ); + return yaml.endsWith("\n") ? yaml : `${yaml}\n`; +} + +function synthetic(key: string): string { + return `synthetic-${key.toLowerCase()}-value`; +} + +function childEnv(): Record { + return privateCliEnv({ + root: requireRoot(), + env: { + HACK_HOME: process.env.HACK_HOME, + HACK_GLOBAL_CONFIG_PATH: process.env.HACK_GLOBAL_CONFIG_PATH, + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + HACK_LOGGER: "console", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + }, + }); +} + +type WorkerExit = { readonly code: number; readonly stderr: string }; + +/** + * Starts one process per command, waits until every one printed `loaded`, + * then releases them together by writing `go` to each stdin. + */ +async function runReleasedTogether( + commands: readonly (readonly string[])[] +): Promise { + const workers = commands.map((cmd) => + Bun.spawn({ + cmd: [process.execPath, ...cmd], + cwd: requireRoot(), + env: childEnv(), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }) + ); + try { + await Promise.all( + workers.map(async (worker) => { + const reader = worker.stdout.getReader(); + let seen = ""; + while (!seen.includes("loaded\n")) { + const chunk = await reader.read(); + if (chunk.done) { + throw new Error("worker exited before loading"); + } + seen += new TextDecoder().decode(chunk.value); + } + reader.releaseLock(); + }) + ); + for (const worker of workers) { + worker.stdin.write("go\n"); + await worker.stdin.flush(); + } + } catch (error: unknown) { + for (const worker of workers) { + worker.kill(); + } + throw error; + } finally { + for (const worker of workers) { + await worker.stdin.end(); + } + } + return await Promise.all( + workers.map(async (worker) => ({ + code: await worker.exited, + stderr: await new Response(worker.stderr).text(), + })) + ); +} + +/** + * A worker script: imports `module`, prints `loaded`, waits for `go` on + * stdin, then runs `body` (which may use `args`, the worker's argv). + */ +async function writeWorker(opts: { + readonly name: string; + readonly imports: string; + readonly module: string; + readonly body: string; +}): Promise { + const path = join(requireRoot(), opts.name); + await writeFile( + path, + [ + `import { ${opts.imports} } from ${JSON.stringify(opts.module)};`, + "const args = process.argv.slice(2);", + "process.stdout.write('loaded\\n');", + "const reader = Bun.stdin.stream().getReader();", + "let seen = '';", + "while (!seen.includes('go\\n')) {", + " const chunk = await reader.read();", + " if (chunk.done) { process.exit(3); }", + " seen += new TextDecoder().decode(chunk.value);", + "}", + opts.body, + "", + ].join("\n") + ); + return path; +} + +function expectAllSucceeded(exits: readonly WorkerExit[]): void { + for (const exit of exits) { + expect({ code: exit.code, stderr: exit.stderr }).toEqual({ + code: 0, + stderr: "", + }); + } +} + +async function resolvedGlobal( + project: Project +): Promise> { + const resolved = await resolveProjectEnvConfig({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + serviceNames: ["api"], + }); + return { ...(resolved?.globalEnv ?? {}) }; +} + +function keys(prefix: string): string[] { + return Array.from({ length: WORKERS }, (_, index) => `${prefix}_${index}`); +} + +const PROJECT_WORKER_BODY = [ + "const [projectRoot, projectDir, op, layer, key] = args;", + "const base = { projectRoot, projectDir, envName: null, scope: 'global', key, local: layer === 'local' };", + "if (op === 'set') {", + " await setProjectEnvValue({ ...base, value: `synthetic-${key.toLowerCase()}-value`, secret: layer === 'local' });", + "} else {", + " await unsetProjectEnvValue(base);", + "}", +].join("\n"); + +for (const layer of ["tracked", "local"] as const) { + test(`${WORKERS} concurrent sets of distinct keys on the ${layer} layer all survive`, async () => { + const project = await createProject(); + const file = + layer === "local" + ? resolveProjectEnvLocalConfigPath({ + projectDir: project.projectDir, + envName: null, + }) + : resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + await writeFile(file, layerText({ PRESET: synthetic("PRESET") })); + const script = await writeWorker({ + name: "project-env-worker.ts", + imports: "setProjectEnvValue, unsetProjectEnvValue", + module: PROJECT_ENV_MODULE, + body: PROJECT_WORKER_BODY, + }); + + const added = keys("ADDED"); + expectAllSucceeded( + await runReleasedTogether( + added.map((key) => [ + script, + project.projectRoot, + project.projectDir, + "set", + layer, + key, + ]) + ) + ); + + const global = await resolvedGlobal(project); + for (const key of ["PRESET", ...added]) { + expect(global[key]).toBe(synthetic(key)); + } + // The lock leaves nothing behind: no lock, owner or staging file. + expect(await readdir(join(project.projectDir, ".internal"))).toEqual([]); + expect( + (await readdir(project.projectDir)).filter((name) => + name.endsWith(".tmp") + ) + ).toEqual([]); + }, 60_000); +} + +test("concurrent sets and unsets of different keys on one layer all apply", async () => { + const project = await createProject(); + const file = resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + const kept = keys("KEPT"); + const dropped = keys("DROPPED"); + await writeFile( + file, + layerText( + Object.fromEntries( + [...kept, ...dropped].map((key) => [key, synthetic(key)]) + ) + ) + ); + const script = await writeWorker({ + name: "project-env-mixed-worker.ts", + imports: "setProjectEnvValue, unsetProjectEnvValue", + module: PROJECT_ENV_MODULE, + body: PROJECT_WORKER_BODY, + }); + + const added = keys("ADDED"); + const base = [script, project.projectRoot, project.projectDir]; + expectAllSucceeded( + await runReleasedTogether([ + ...added.map((key) => [...base, "set", "tracked", key]), + ...dropped.map((key) => [...base, "unset", "tracked", key]), + ]) + ); + + const global = await resolvedGlobal(project); + expect(Object.keys(global).sort()).toEqual([...kept, ...added].sort()); + for (const key of [...kept, ...added]) { + expect(global[key]).toBe(synthetic(key)); + } +}, 60_000); + +test(`${WORKERS} concurrent \`hack env set\` commands all keep their key`, async () => { + const project = await createProject(); + const file = resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + await writeFile(file, layerText({ PRESET: synthetic("PRESET") })); + // A preload holds each CLI process at the barrier before the CLI loads. + const preload = join(requireRoot(), "cli-barrier.ts"); + await writeFile( + preload, + [ + "process.stdout.write('loaded\\n');", + "const reader = Bun.stdin.stream().getReader();", + "let seen = '';", + "while (!seen.includes('go\\n')) {", + " const chunk = await reader.read();", + " if (chunk.done) { process.exit(3); }", + " seen += new TextDecoder().decode(chunk.value);", + "}", + "reader.releaseLock();", + "", + ].join("\n") + ); + const added = keys("CLI_ADDED"); + const exits = await runReleasedTogether( + added.map((key) => [ + "--preload", + preload, + ENTRYPOINT, + "env", + "set", + key, + synthetic(key), + "--path", + project.projectRoot, + ]) + ); + for (const exit of exits) { + expect(exit.code).toBe(0); + for (const key of added) { + expect(exit.stderr).not.toContain(synthetic(key)); + } + } + const global = await resolvedGlobal(project); + for (const key of ["PRESET", ...added]) { + expect(global[key]).toBe(synthetic(key)); + } +}, 120_000); + +test("concurrent legacy dotenv sets and removals of different keys all apply", async () => { + const project = await createProject(); + const envFile = join(project.projectDir, ".env"); + const kept = keys("KEPT"); + const dropped = keys("DROPPED"); + await writeFile( + envFile, + [...kept, ...dropped].map((key) => `${key}=${synthetic(key)}\n`).join(""), + { mode: 0o600 } + ); + const script = await writeWorker({ + name: "dotenv-worker.ts", + imports: "removeDotEnvKey, upsertDotEnvValue", + module: HACK_ENV_MODULE, + body: [ + "const [envFile, op, key] = args;", + "if (op === 'set') {", + " await upsertDotEnvValue({ envFile, key, value: `synthetic-${key.toLowerCase()}-value` });", + "} else {", + " await removeDotEnvKey({ envFile, key });", + "}", + ].join("\n"), + }); + + const added = keys("ADDED"); + expectAllSucceeded( + await runReleasedTogether([ + ...added.map((key) => [script, envFile, "set", key]), + ...dropped.map((key) => [script, envFile, "remove", key]), + ]) + ); + + const env = parseDotEnv(await readFile(envFile, "utf8")); + expect(Object.keys(env).sort()).toEqual([...kept, ...added].sort()); + for (const key of [...kept, ...added]) { + expect(env[key]).toBe(synthetic(key)); + } +}, 60_000); + +/** Publishes a live owner receipt (this test process) at the layer's lock. */ +async function holdLayerLock(layerPath: string): Promise { + const lockPath = resolveEnvLayerLockPath({ layerPath }); + await mkdir(dirname(lockPath), { recursive: true }); + await writeFile(lockPath, `${process.pid}\n${randomUUID()}\n`, { + mode: 0o600, + }); + return lockPath; +} + +async function closedRefusalOf(pending: Promise): Promise { + try { + await pending; + } catch (error: unknown) { + if (error instanceof ClosedCliError) { + return error; + } + throw error; + } + throw new Error("Expected a closed refusal"); +} + +test("a writer that cannot get the layer lock in time refuses closed and writes nothing", async () => { + const project = await createProject(); + const file = resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + const original = layerText({ PRESET: synthetic("PRESET") }); + await writeFile(file, original); + const envFile = join(project.projectDir, ".env"); + const originalDotEnv = `PRESET=${synthetic("PRESET")}\n`; + await writeFile(envFile, originalDotEnv, { mode: 0o600 }); + const lockPath = await holdLayerLock(file); + const dotEnvLockPath = await holdLayerLock(envFile); + const base = { + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + layerLockTimeoutMs: 300, + }; + + const refusals = [ + await closedRefusalOf( + setProjectEnvValue({ + ...base, + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + }) + ), + await closedRefusalOf(unsetProjectEnvValue({ ...base, key: "PRESET" })), + await closedRefusalOf( + upsertDotEnvValue({ + envFile, + key: "BLOCKED", + value: synthetic("BLOCKED"), + layerLockTimeoutMs: 300, + }) + ), + await closedRefusalOf( + removeDotEnvKey({ envFile, key: "PRESET", layerLockTimeoutMs: 300 }) + ), + ]; + for (const [index, refusal] of refusals.entries()) { + const layer = index < 2 ? file : envFile; + expect(refusal).toBeInstanceOf(EnvLayerLockTimeoutError); + expect(refusal.message).toBe( + `Another hack env command is writing ${layer}; gave up after waiting 300ms for it. Nothing was written. Retry when it finishes.` + ); + expect(refusal.message).not.toContain(synthetic("BLOCKED")); + expect(refusal.message).not.toContain(synthetic("PRESET")); + } + expect(await readFile(file, "utf8")).toBe(original); + expect(await readFile(envFile, "utf8")).toBe(originalDotEnv); + + // Once the holder releases, the same write goes through. + await rm(lockPath); + await rm(dotEnvLockPath); + const done = await setProjectEnvValue({ + ...base, + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + }); + expect(done.changed).toBe(true); + expect((await resolvedGlobal(project)).BLOCKED).toBe(synthetic("BLOCKED")); + expect(await readdir(dirname(lockPath))).toEqual([]); +}); + +// Permission bits do not bind root, so a read-only directory only exists for others. +const RUNS_AS_ROOT = (process.geteuid?.() ?? process.getuid?.() ?? -1) === 0; + +test.skipIf(RUNS_AS_ROOT)( + "in a read-only .hack directory a no-op set or unset still succeeds and a change is refused", + async () => { + const project = await createProject(); + const file = resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + const original = layerText({ PRESET: synthetic("PRESET") }); + await writeFile(file, original); + const base = { + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + }; + await chmod(project.projectDir, 0o555); + try { + // No lock can be created here, and none is needed: nothing can be + // replaced, so a no-op keeps working as it did before the lock. + expect( + (await unsetProjectEnvValue({ ...base, key: "ABSENT" })).changed + ).toBe(false); + expect( + ( + await setProjectEnvValue({ + ...base, + key: "PRESET", + value: synthetic("PRESET"), + secret: false, + }) + ).changed + ).toBe(false); + const refusal = await closedRefusalOrCliError( + setProjectEnvValue({ + ...base, + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + }) + ); + expect(refusal).toBeInstanceOf(AtomicWriteRefusedError); + expect(refusal.message).not.toContain(synthetic("BLOCKED")); + } finally { + await chmod(project.projectDir, 0o755); + } + expect(await readFile(file, "utf8")).toBe(original); + expect(await readdir(project.projectDir)).not.toContain(".internal"); + } +); + +async function closedRefusalOrCliError( + pending: Promise +): Promise { + try { + await pending; + } catch (error: unknown) { + if (error instanceof Error) { + return error; + } + throw error; + } + throw new Error("Expected the write to be refused"); +} + +test("a symlinked .internal beside the layer is refused before any lock or write", async () => { + const project = await createProject(); + const file = resolveProjectEnvConfigPath({ + projectDir: project.projectDir, + envName: null, + }); + const original = layerText({ PRESET: synthetic("PRESET") }); + await writeFile(file, original); + const elsewhere = join(requireRoot(), "elsewhere"); + await mkdir(elsewhere); + await symlink(elsewhere, join(project.projectDir, ".internal")); + + const refusal = await closedRefusalOf( + setProjectEnvValue({ + projectRoot: project.projectRoot, + projectDir: project.projectDir, + envName: null, + scope: "global", + key: "BLOCKED", + value: synthetic("BLOCKED"), + secret: false, + }) + ); + expect(refusal).toBeInstanceOf(EnvLayerLockRefusedError); + expect(refusal.message).toBe( + `Refused to update ${file}: the .internal entry beside it is a symlink or not a directory, so its write lock cannot be taken safely. Replace it with a regular directory and retry. Nothing was written.` + ); + expect(await readFile(file, "utf8")).toBe(original); + expect(await readdir(elsewhere)).toEqual([]); +}); + +test("a set refuses with a conflict when a writer that takes no lock changed the layer after its read", async () => { + const base = requireRoot(); + const fixture = await createSecretWorktreeFixture({ + parent: base, + home: privateCliHome(base), + name: "layer-cas", + }); + const projectDir = join(fixture.primaryRoot, ".hack"); + const file = resolveProjectEnvConfigPath({ projectDir, envName: null }); + await writeFile(file, layerText({ PRESET: synthetic("PRESET") })); + // No project key exists yet, so a secret set creates one under the key + // lock in the git common dir, after it has read the layer. Holding that + // lock pauses the set between its read and its write. + const shared = await resolveProjectEnvSharedKeyLocation({ + projectRoot: fixture.primaryRoot, + }); + if (!shared) { + throw new Error("Expected a shared key location in a git checkout"); + } + const keyLockPath = `${shared.path}.lock`; + await writeFile(keyLockPath, `${process.pid}\n${randomUUID()}\n`, { + mode: 0o600, + }); + + const pending = setProjectEnvValue({ + projectRoot: fixture.primaryRoot, + projectDir, + envName: null, + scope: "global", + key: "SECRET_ADDED", + value: synthetic("SECRET_ADDED"), + secret: true, + }).then( + () => null, + (error: unknown) => error + ); + // The set is waiting once its owner receipt is staged beside the key lock. + const lockDir = dirname(keyLockPath); + const staged = `${basename(keyLockPath)}.`; + for (let attempt = 0; attempt < 500; attempt += 1) { + const names = await readdir(lockDir); + if ( + names.some((name) => name.startsWith(staged) && name.endsWith(".owner")) + ) { + break; + } + await Bun.sleep(10); + } + const edited = layerText({ + PRESET: synthetic("PRESET"), + HAND_EDITED: synthetic("HAND_EDITED"), + }); + await writeFile(file, edited); + await rm(keyLockPath); + + const error = await pending; + expect(error).toBeInstanceOf(AtomicWriteConflictError); + // The other writer's layer stands; the set wrote nothing. + expect(await readFile(file, "utf8")).toBe(edited); +}, 30_000);