diff --git a/docs/env.md b/docs/env.md index b3e376ea..32dc18f3 100644 --- a/docs/env.md +++ b/docs/env.md @@ -98,7 +98,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 @@ -649,6 +654,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 @@ -716,15 +749,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`: @@ -885,6 +920,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/reference/application-benchmark-protocol.md b/docs/reference/application-benchmark-protocol.md index 98bafa25..ef7184d0 100644 --- a/docs/reference/application-benchmark-protocol.md +++ b/docs/reference/application-benchmark-protocol.md @@ -287,8 +287,13 @@ 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 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. @@ -315,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/docs/reference/ax-target-result-contract.md b/docs/reference/ax-target-result-contract.md index 4a6fe221..178aa312 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,16 @@ 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`. 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. @@ -208,6 +219,8 @@ 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) | +| 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/docs/security/env-secrets-threat-model.md b/docs/security/env-secrets-threat-model.md index 5ebacde6..7cff5ae7 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/scripts/check-application-benchmark-protocol.py b/scripts/check-application-benchmark-protocol.py index bc20e433..43fc956f 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") @@ -679,12 +704,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/src/backends/native-project-process.ts b/src/backends/native-project-process.ts index b898d8f5..ed3c467e 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/src/commands/doctor.ts b/src/commands/doctor.ts index 03574297..52ea034d 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -91,6 +91,7 @@ import { import { describeEnvKeyInspection, type EnvKeyInspection, + gitIsLocalKeyIgnored, gitListTrackedKeyPaths, inspectProjectEnvKey, resolveEnvKeyCandidates, @@ -2990,6 +2991,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 5ee08e9d..88a1c80f 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/env-layer-lock.ts b/src/lib/env-layer-lock.ts new file mode 100644 index 00000000..0daa2239 --- /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 8b4e1195..ad920c22 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/native-compose-adoption-marker.ts b/src/lib/native-compose-adoption-marker.ts index 1231ca3e..b10faddc 100644 --- a/src/lib/native-compose-adoption-marker.ts +++ b/src/lib/native-compose-adoption-marker.ts @@ -34,6 +34,9 @@ export const LEGACY_ADOPTION_SELECTION_MESSAGES = { } as const; /** 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: keyof typeof LEGACY_ADOPTION_SELECTION_MESSAGES; + constructor( kind: keyof typeof LEGACY_ADOPTION_SELECTION_MESSAGES = "adoption" ) { @@ -42,6 +45,7 @@ export class LegacyComposeAdoptionSelectionError extends HackCliError { message: LEGACY_ADOPTION_SELECTION_MESSAGES[kind], }); this.name = "LegacyComposeAdoptionSelectionError"; + this.kind = kind; } } function refuse(): never { diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 373b9470..fa7684f5 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, @@ -720,6 +721,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 @@ -2605,18 +2647,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: { @@ -2628,6 +2675,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 }); @@ -2649,7 +2698,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", }); @@ -2693,9 +2764,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, @@ -2713,6 +2787,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 }); @@ -2727,7 +2803,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", }); @@ -2766,6 +2860,7 @@ export async function unsetProjectEnvValue(opts: { secretsprovider: PROJECT_ENV_SECRETS_PROVIDER, values: nextValues, }, + expectedExisting: text, }); return { changed, filePath }; } @@ -2873,10 +2968,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 @@ -2936,11 +3033,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(); @@ -3853,6 +3957,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 @@ -3929,6 +4038,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, @@ -4047,12 +4163,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({ @@ -4061,14 +4222,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/src/lib/project-input-selection.ts b/src/lib/project-input-selection.ts index 10a44b8e..1ddea5f7 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/projects-registry-lock.ts b/src/lib/projects-registry-lock.ts index 4448879f..e7e70fe8 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/src/lib/target-inspect.ts b/src/lib/target-inspect.ts index c4bdbf22..2dd3f547 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, @@ -30,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, @@ -170,6 +174,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 +368,11 @@ 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, 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 * before the generation is re-read; production callers never pass it. @@ -382,6 +396,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 +413,32 @@ 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 ProjectInputAccessError) { + return error.access === "permission" + ? "input_permission_denied" + : "input_unreadable"; + } + if (error instanceof LegacyComposeAdoptionSelectionError) { + if (error.kind === "permission") { + return "input_permission_denied"; + } + 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 +696,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 +713,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/doctor-env-key.test.ts b/tests/doctor-env-key.test.ts index f3b9283c..05a9c8c7 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/native-project-process.test.ts b/tests/native-project-process.test.ts index 49e17032..cc4e9195 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); diff --git a/tests/project-env-hydrate.test.ts b/tests/project-env-hydrate.test.ts index 39d1b361..869eefc2 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-key-ignore.test.ts b/tests/project-env-key-ignore.test.ts new file mode 100644 index 00000000..d8f2de92 --- /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-layer-lock.test.ts b/tests/project-env-layer-lock.test.ts new file mode 100644 index 00000000..c98af390 --- /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); diff --git a/tests/project-env-write-refusals.test.ts b/tests/project-env-write-refusals.test.ts index 2c1f2dc7..18beaa8e 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); diff --git a/tests/project-input-selection.test.ts b/tests/project-input-selection.test.ts index 3883edd3..b9544e5b 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 16f41355..b9442a41 100644 --- a/tests/project-target-inspect.test.ts +++ b/tests/project-target-inspect.test.ts @@ -232,6 +232,51 @@ 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"); + // 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 }); @@ -635,6 +680,83 @@ 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 { + // An unsearchable .hack is a permission fault (#402's `permission` + // selection kind), not unsafe adoption state. + expectRefusal(await inspectBoth({ path }), "input_permission_denied"); + const cli = await runCli({ path }); + expect(JSON.stringify(cli.envelope)).not.toContain(fixture.root); + expect(JSON.stringify(cli.envelope)).not.toContain("EACCES"); + } finally { + await chmod(projectDir, 0o755); + } + }, + 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 () => { @@ -887,6 +1009,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 [ diff --git a/tests/python/test_application_benchmark_protocol.py b/tests/python/test_application_benchmark_protocol.py index 4ac0b0af..aa72ac2a 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() @@ -603,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])), @@ -618,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.""" @@ -632,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)