From 88302a57e00a2494499068a9a397dd991dbd89ed Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Fri, 23 Oct 2026 23:30:01 -0400 Subject: [PATCH] fix(env): close the remaining raw env write refusals and publish without hard links On a filesystem without hard links (ExFAT, FAT) every env write failed with a raw ENOTSUP from the lock receipt's and the first project key's link(2) publish, so the key could never be created there. The exclusive publish now falls back to an O_CREAT|O_EXCL|O_NOFOLLOW empty reservation that the complete, fsynced sibling is renamed over, keeping the no-overwrite and no-follow guarantees. The other refusals that were still raw on next are now one value-free line with an E_* code and no stack: - hydrate with a symlinked (dangling or live) or non-directory .hack/.internal is E_PATH_CONFLICT; nothing is created through it - an --env overlay name over 128 characters is refused up front as E_USAGE, and ENAMETOOLONG is classified as E_PATH_CONFLICT - a project env key lock that stays held is E_LOCKED, naming the lock (and its recovery guard when that is held) and the remedy - a .hack/.gitignore directory is E_PATH_CONFLICT, as docs/env.md said Adds mutation-proven tests for these and for the previously untested lock-unavailable codes, atomic-write refusal codes and the staging EISDIR mapping. Refs HACK-1377 Co-Authored-By: Claude Opus 5.5 --- docs/env.md | 34 +- .../hack-diagnose/references/error-codes.md | 3 +- src/agents/error-code-reference.ts | 5 + src/cli/env-selection.ts | 11 +- src/lib/cli-result.ts | 1 + src/lib/exclusive-publish.ts | 149 +++ src/lib/fs.ts | 89 +- src/lib/private-file.ts | 23 +- src/lib/project-env-config.ts | 54 +- src/lib/project.ts | 16 +- src/lib/projects-registry-lock.ts | 32 +- tests/fixtures/staging-errno-preload.ts | 15 +- tests/project-env-write-refusal-gaps.test.ts | 921 ++++++++++++++++++ tests/project-env-write-refusals.test.ts | 7 +- 14 files changed, 1306 insertions(+), 54 deletions(-) create mode 100644 src/lib/exclusive-publish.ts create mode 100644 tests/project-env-write-refusal-gaps.test.ts diff --git a/docs/env.md b/docs/env.md index 32dc18f3c..096c40d2e 100644 --- a/docs/env.md +++ b/docs/env.md @@ -184,9 +184,12 @@ Use `--env=base` to bypass that default and read only `.hack/hack.env.default.ya command that accepts `--env`; an overlay file named after one of those spellings is never selected by name. Any other value must already be a canonical overlay name: lowercase letters and digits joined by single hyphens (`^[a-z0-9]+(-[a-z0-9]+)*$`, for example -`qa` or `qa-2`); surrounding whitespace is ignored. Hack never rewrites a requested name, -so `QA`, `q_a` or `X=1` is refused with a fixed `Invalid --env value.` message that does -not repeat the argument, before any project file is read or any child process starts. +`qa` or `qa-2`), at most 128 characters long; surrounding whitespace is ignored. Hack +never rewrites a requested name, so `QA`, `q_a`, `X=1` or a name longer than 128 +characters is refused with a fixed `Invalid --env value.` message (`E_USAGE` under +`--json`) that does not repeat the argument, before any project file is read or any +child process starts. The limit keeps every file name Hack derives from the overlay +name, such as the layer's lock, within the usual 255-byte file name limit. See the [command and delivery matrix](#command-and-delivery-matrix) for what each command does with the selection. @@ -637,7 +640,28 @@ message in the envelope: you cannot write (for example mode `0555`) both give this code. - `E_NO_SPACE`: the disk or the disk quota is full. - `E_PATH_CONFLICT`: a path is the wrong kind of entry, such as a file where Hack needs - a directory, or a directory where Hack needs a file. + a directory, a directory where Hack needs a file (including a `.hack/.gitignore` that + is a directory or a symlink), or a `.hack/.internal` that `hack env hydrate` finds to + be a symlink (dangling or not) or a file. A file name or path that is too long for + the filesystem is reported with this code too; use a shorter `--env` overlay name or + move the project to a shorter path. +- `E_LOCKED`: the project env key lock (`/.hack.secret.key.lock`, taken + by the first `--secret` value) stayed held for the whole 10-second wait. Nothing was + written. Another hack command may be creating the key; retry when it finishes. If no + other hack command is running, the lock is stale (for example its process ID now + belongs to an unrelated process, which Hack never treats as abandoned): remove the + lock file the message names, then retry. + +The first `--secret` value also works on a filesystem without hard links, such as ExFAT +or FAT on a USB drive. Hack normally publishes the key file and its locks with a hard +link, so they appear complete or not at all. Where that is not possible, it creates an +empty placeholder exclusively and renames the complete file over it, which keeps the +same rules: an existing file or symlink is never replaced or followed, and a reader +never sees a partial file. If Hack is killed between those two steps, an empty +`.hack.secret.key` or lock file is left behind. Hack refuses it rather than use or +replace it; remove it and run the command again. ExFAT and FAT do not store Unix +permissions, so on such a drive the key file is not limited to your user the way mode +`0600` limits it elsewhere. If you omit `key`, `value`, or `--service`, `hack env add` prompts for them interactively. Pass `--no-interactive` (or set `HACK_NO_INTERACTIVE=1`) for scripted/agent usage: the @@ -892,7 +916,7 @@ Write-path guarantees in a linked worktree (first secret added from any checkout - otherwise an existing shared key (git common dir) is reused - otherwise the primary checkout's key is adopted by copying it to the shared location, so sibling checkouts converge on one key - only then is a new key generated, and it is written to the shared location — never silently to the checkout -- concurrent first use from several checkouts converges on one key: when no key exists, creation runs under a cooperative lock at `/.hack.secret.key.lock`, and a key file is published only when complete, never empty or partial (Hack v4 writers do not take the lock) +- concurrent first use from several checkouts converges on one key: when no key exists, creation runs under a cooperative lock at `/.hack.secret.key.lock`, and a key file is published only when complete, never empty or partial (Hack v4 writers do not take the lock; on a filesystem without hard links, such as ExFAT, an empty placeholder is visible for a moment before the complete key replaces it) - if the shared location cannot be resolved (git unavailable) or written, Hack falls back to a checkout-local key and prints a loud divergence warning - `hack doctor` flags divergent `.hack.secret.key` contents across checkouts with the exact paths diff --git a/plugins/hack/skills/hack-diagnose/references/error-codes.md b/plugins/hack/skills/hack-diagnose/references/error-codes.md index 309ed4746..77c2fccec 100644 --- a/plugins/hack/skills/hack-diagnose/references/error-codes.md +++ b/plugins/hack/skills/hack-diagnose/references/error-codes.md @@ -26,10 +26,11 @@ stay within the request. | `E_PERMISSION` | A filesystem or runtime permission was denied, or a file Hack must write is on a read-only filesystem. | `hack doctor --json` | | `E_NO_SPACE` | A file Hack must write could not be written because the disk is full or the disk quota is exceeded. | `df -h ` | | `E_PATH_CONFLICT` | A path Hack must read or write is the wrong kind of entry: a file where a directory is needed, or a directory or special file where a regular file is needed. | `ls -ld ` | +| `E_LOCKED` | A lock Hack needs stayed held for the whole bounded wait, by another Hack command or by a stale lock Hack cannot prove abandoned; nothing guarded by it was written. | `ls -l ` | | `E_INTERACTIVE_REQUIRED` | The command needs an answer at a prompt but is running without one. | `hack --help` | | `E_USAGE` | Arguments or flags were invalid for the command. | `hack --help` | | `E_UNEXPECTED` | An unclassified failure; the message is the only detail. | `hack doctor --json` | --- -Content revision: `4c1343788d6a`. Generated by Hack; edit the canonical source and regenerate the plugin. +Content revision: `dc08a206e03d`. Generated by Hack; edit the canonical source and regenerate the plugin. diff --git a/src/agents/error-code-reference.ts b/src/agents/error-code-reference.ts index f7b6c1c6e..09550a489 100644 --- a/src/agents/error-code-reference.ts +++ b/src/agents/error-code-reference.ts @@ -102,6 +102,11 @@ const ERROR_CODE_GUIDES: Readonly> = { "A path Hack must read or write is the wrong kind of entry: a file where a directory is needed, or a directory or special file where a regular file is needed.", readFirst: "`ls -ld `", }, + E_LOCKED: { + meaning: + "A lock Hack needs stayed held for the whole bounded wait, by another Hack command or by a stale lock Hack cannot prove abandoned; nothing guarded by it was written.", + readFirst: "`ls -l `", + }, E_INTERACTIVE_REQUIRED: { meaning: "The command needs an answer at a prompt but is running without one.", diff --git a/src/cli/env-selection.ts b/src/cli/env-selection.ts index c88abf402..88baea5d8 100644 --- a/src/cli/env-selection.ts +++ b/src/cli/env-selection.ts @@ -1,4 +1,7 @@ -import { parseEnvConfigSelection } from "../lib/project.ts"; +import { + ENV_CONFIG_NAME_MAX_LENGTH, + parseEnvConfigSelection, +} from "../lib/project.ts"; import { CliUsageError } from "./command.ts"; /** @@ -6,8 +9,7 @@ import { CliUsageError } from "./command.ts"; * and never the argument, which may be a mistyped `KEY=value` or a pasted * value. */ -export const INVALID_ENV_SELECTION_MESSAGE = - "Invalid --env value. Overlay names must match ^[a-z0-9]+(-[a-z0-9]+)*$; use base, default or none for base layers only."; +export const INVALID_ENV_SELECTION_MESSAGE = `Invalid --env value. Overlay names must match ^[a-z0-9]+(-[a-z0-9]+)*$ and be at most ${ENV_CONFIG_NAME_MAX_LENGTH} characters; use base, default or none for base layers only.`; /** * Resolve a CLI `--env` value through the one shared selection grammar. @@ -16,7 +18,8 @@ export const INVALID_ENV_SELECTION_MESSAGE = * - `null`: base layers only. `base`, `default` and `none` (any case) and an * empty value bypass the configured default overlay. * - `string`: the overlay name, which must already be canonical - * (`ENV_CONFIG_NAME_PATTERN`); surrounding whitespace is ignored. + * (`ENV_CONFIG_NAME_PATTERN`) and at most `ENV_CONFIG_NAME_MAX_LENGTH` + * characters; surrounding whitespace is ignored. * * Anything else, including any value containing `=`, is refused with a fixed, * value-free usage error. A requested name is never slugged: slugging diff --git a/src/lib/cli-result.ts b/src/lib/cli-result.ts index 7b4df9a0c..c4a1076e4 100644 --- a/src/lib/cli-result.ts +++ b/src/lib/cli-result.ts @@ -35,6 +35,7 @@ export const HACK_ERROR_CODES = [ "E_PERMISSION", "E_NO_SPACE", "E_PATH_CONFLICT", + "E_LOCKED", "E_INTERACTIVE_REQUIRED", "E_USAGE", "E_UNEXPECTED", diff --git a/src/lib/exclusive-publish.ts b/src/lib/exclusive-publish.ts new file mode 100644 index 000000000..c06042e0a --- /dev/null +++ b/src/lib/exclusive-publish.ts @@ -0,0 +1,149 @@ +import { constants, type Stats } from "node:fs"; +import { link, lstat, open, rename, unlink } from "node:fs/promises"; +import { isRecord } from "./guards.ts"; + +/** + * Exclusive publication of a complete file at a path that must not already + * exist, for filesystems with and without hard links. + * + * Ownership: the caller owns `source` (a fully written, fsynced sibling it + * created exclusively) and decides what an occupied `target` means. These + * helpers never follow a symlink at `target` and never replace an entry they + * did not create. + */ + +/** + * `link(2)` failures that mean the filesystem cannot make hard links: ExFAT + * and FAT report ENOTSUP on macOS and EPERM on Linux. A real permission + * problem also reports EPERM; the fallback then fails with its own + * permission error, so nothing is hidden. + */ +const HARD_LINK_UNSUPPORTED_CODES: ReadonlySet = new Set([ + "ENOTSUP", + "EOPNOTSUPP", + "EPERM", +]); + +/** Whether a `link(2)` error means the filesystem has no hard links. */ +export function isHardLinkUnsupportedError(error: unknown): boolean { + return ( + isRecord(error) && + typeof error.code === "string" && + HARD_LINK_UNSUPPORTED_CODES.has(error.code) + ); +} + +/** + * Publishes `source` at `target` only when no entry exists at `target`, and + * reports whether this call published it. `source` must be on the same + * filesystem (a sibling). + * + * Primary path: `link(source, target)`, which fails with EEXIST instead of + * replacing anything; `source` keeps its name. When the filesystem has no + * hard links ({@link isHardLinkUnsupportedError}), it falls back to + * {@link publishByExclusivePlaceholder}, after which `source` no longer + * exists. Callers that remove `source` afterwards must tolerate ENOENT. + * + * EEXIST resolves to false; every other error propagates unchanged. + */ +export async function publishFileExclusive(opts: { + readonly source: string; + readonly target: string; +}): Promise { + try { + await link(opts.source, opts.target); + return true; + } catch (error: unknown) { + if (hasCode(error, "EEXIST")) { + return false; + } + if (!isHardLinkUnsupportedError(error)) { + throw error; + } + } + return await publishByExclusivePlaceholder(opts); +} + +/** + * Publication without hard links: reserve `target` with an empty + * O_CREAT|O_EXCL|O_NOFOLLOW file (mode 0600), then rename the complete + * `source` over that reservation. + * + * Guarantees kept from the hard-link path: + * - no overwrite: the exclusive create fails with EEXIST on any existing + * entry, including a dangling symlink, and the rename replaces only the + * empty regular file this call created. It is checked again (still an + * empty regular file with the same identity) immediately before the + * rename; if anything else is there the call reports the path as occupied + * and leaves it alone. + * - no follow: neither O_EXCL|O_NOFOLLOW nor rename(2) follows a symlink at + * the final component. + * - readers see either nothing, the empty reservation, or the complete file, + * never a partial one. + * + * Difference: a process killed between the reservation and the rename + * leaves an empty file at `target`. Callers already treat an empty file as + * invalid, never as theirs, so it is refused rather than adopted or + * reclaimed; it must be removed by hand. A failed rename removes the + * reservation while it is still the empty file this call created. + */ +export async function publishByExclusivePlaceholder(opts: { + readonly source: string; + readonly target: string; +}): Promise { + let reserved: Stats; + try { + const placeholder = await open( + opts.target, + constants.O_CREAT | + constants.O_EXCL | + constants.O_NOFOLLOW | + constants.O_WRONLY, + 0o600 + ); + try { + reserved = await placeholder.stat(); + } finally { + await placeholder.close(); + } + } catch (error: unknown) { + if (hasCode(error, "EEXIST")) { + return false; + } + throw error; + } + if (!(await isOwnReservation({ path: opts.target, reserved }))) { + return false; + } + try { + await rename(opts.source, opts.target); + } catch (error: unknown) { + if (await isOwnReservation({ path: opts.target, reserved })) { + await unlink(opts.target).catch(() => undefined); + } + throw error; + } + return true; +} + +/** + * Whether `path` still names the empty regular file this call reserved. + * Size is checked as well as identity: some filesystems without hard links + * (ExFAT) give every empty file the same inode number. + */ +async function isOwnReservation(opts: { + readonly path: string; + readonly reserved: Stats; +}): Promise { + const current = await lstat(opts.path).catch(() => null); + return ( + current?.isFile() === true && + current.size === 0 && + current.dev === opts.reserved.dev && + current.ino === opts.reserved.ino + ); +} + +function hasCode(error: unknown, code: string): boolean { + return isRecord(error) && error.code === code; +} diff --git a/src/lib/fs.ts b/src/lib/fs.ts index fd17e01ca..638c15886 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -40,12 +40,22 @@ export async function ensureDir(absoluteDir: string): Promise { * {@link ensureDir} for a write path whose failure the CLI reports: a * directory that cannot be created for one of the {@link FileWriteFailure} * reasons is refused with a closed {@link FileWriteRefusedError} that names - * the directory, never the raw errno text. Other failures propagate as is. + * the directory, never the raw errno text. An entry already at the path that + * is not a directory (a file, or a dangling symlink, which a recursive mkdir + * reports as EEXIST) is refused as `not_a_directory_entry`. Other failures + * propagate as is. */ export async function ensureDirForWrite(absoluteDir: string): Promise { try { await mkdir(absoluteDir, { recursive: true }); } catch (error: unknown) { + if (hasErrorCode(error, "EEXIST")) { + throw new FileWriteRefusedError({ + path: absoluteDir, + failure: "not_a_directory_entry", + operation: "create_directory", + }); + } throw closedFileWriteError({ error, path: absoluteDir, @@ -54,6 +64,56 @@ export async function ensureDirForWrite(absoluteDir: string): Promise { } } +/** + * {@link ensureDirForWrite} for a directory Hack creates files in and that + * must not redirect them: the final component must be a real directory. A + * symlink (dangling or not) or any other entry there is refused with a closed + * `not_a_directory_entry` {@link FileWriteRefusedError} (`E_PATH_CONFLICT`) + * before anything is created through it. Ancestors are not inspected. The + * check is repeated after a create, so an entry that races the mkdir is + * refused too; it is not a fence against a later swap. + */ +export async function ensureRealDirectoryForWrite( + absoluteDir: string +): Promise { + if (await isRealDirectoryOrAbsent(absoluteDir)) { + await ensureDirForWrite(absoluteDir); + if (await isRealDirectoryOrAbsent(absoluteDir)) { + return; + } + } + throw new FileWriteRefusedError({ + path: absoluteDir, + failure: "not_a_directory_entry", + operation: "create_directory", + }); +} + +/** False for a symlink or non-directory entry; lstat failures are closed. */ +async function isRealDirectoryOrAbsent(absoluteDir: string): Promise { + try { + return (await lstat(absoluteDir)).isDirectory(); + } catch (error: unknown) { + if (hasErrorCode(error, "ENOENT")) { + return true; + } + throw closedFileWriteError({ + error, + path: absoluteDir, + operation: "create_directory", + }); + } +} + +function hasErrorCode(error: unknown, code: string): boolean { + return ( + typeof error === "object" && + error !== null && + "code" in error && + (error as { readonly code: unknown }).code === code + ); +} + /** * Filesystem failures that the user, not Hack, must fix before a write can * succeed. Each maps from a fixed set of errno codes; any other error is @@ -64,7 +124,10 @@ export type FileWriteFailure = | "read_only_filesystem" | "no_space" | "not_a_directory" - | "is_a_directory"; + | "is_a_directory" + | "name_too_long" + /** A symlink or non-directory where a directory is needed; never an errno. */ + | "not_a_directory_entry"; const FILE_WRITE_FAILURE_CODES: Readonly> = { EACCES: "permission_denied", @@ -74,12 +137,14 @@ const FILE_WRITE_FAILURE_CODES: Readonly> = { EDQUOT: "no_space", ENOTDIR: "not_a_directory", EISDIR: "is_a_directory", + ENAMETOOLONG: "name_too_long", }; /** * The stable `--json` code of each {@link FileWriteFailure}. A read-only * filesystem is reported as a permission problem; a path of the wrong kind - * (a file where a directory is needed, or the reverse) as a path conflict. + * (a file where a directory is needed, or the reverse) or one too long for + * the filesystem as a path conflict. */ const FILE_WRITE_FAILURE_ERROR_CODES: Readonly< Record @@ -89,6 +154,8 @@ const FILE_WRITE_FAILURE_ERROR_CODES: Readonly< no_space: "E_NO_SPACE", not_a_directory: "E_PATH_CONFLICT", is_a_directory: "E_PATH_CONFLICT", + name_too_long: "E_PATH_CONFLICT", + not_a_directory_entry: "E_PATH_CONFLICT", }; /** The {@link FileWriteFailure} of a filesystem error, or null for any other error. */ @@ -114,6 +181,8 @@ const FILE_WRITE_FAILURE_REASONS: Readonly> = { no_space: "no space is left on the device or the disk quota is exceeded", not_a_directory: "a parent of that path is not a directory", is_a_directory: "that path is a directory", + name_too_long: "a file name or the path is too long for the filesystem", + not_a_directory_entry: "that path is a symlink or not a directory", }; const FILE_WRITE_PERMISSION_REMEDIES: Readonly< @@ -133,6 +202,9 @@ const FILE_WRITE_FAILURE_REMEDIES: Readonly< no_space: "Free some disk space or quota, then retry", not_a_directory: "Replace that path with a directory, then retry", is_a_directory: "Move that directory out of the way, then retry", + name_too_long: + "Use a shorter name or move the project to a shorter path, then retry", + not_a_directory_entry: "Replace it with a regular directory, then retry", }; /** The fixed remedy sentence for a failure; never names a path or an errno. */ @@ -436,6 +508,8 @@ const ATOMIC_WRITE_STAGING_REFUSALS: Readonly< no_space: "no_space", not_a_directory: "not_a_directory", is_a_directory: "not_regular_file", + name_too_long: "name_too_long", + not_a_directory_entry: "not_a_directory", }; /** The closed refusal for a classified staging failure, else `error` as is. */ @@ -462,7 +536,8 @@ export type AtomicWriteRefusal = | "directory_not_writable" | "read_only_filesystem" | "no_space" - | "not_a_directory"; + | "not_a_directory" + | "name_too_long"; const ATOMIC_WRITE_REFUSAL_MESSAGES: Readonly< Record @@ -478,13 +553,14 @@ const ATOMIC_WRITE_REFUSAL_MESSAGES: Readonly< read_only_filesystem: `${FILE_WRITE_FAILURE_REASONS.read_only_filesystem}. ${FILE_WRITE_FAILURE_REMEDIES.read_only_filesystem}`, no_space: `${FILE_WRITE_FAILURE_REASONS.no_space}. ${FILE_WRITE_FAILURE_REMEDIES.no_space}`, not_a_directory: `${FILE_WRITE_FAILURE_REASONS.not_a_directory}. ${FILE_WRITE_FAILURE_REMEDIES.not_a_directory}`, + name_too_long: `${FILE_WRITE_FAILURE_REASONS.name_too_long}. ${FILE_WRITE_FAILURE_REMEDIES.name_too_long}`, }; /** * The stable `--json` code of each {@link AtomicWriteRefusal}, consistent with * {@link FileWriteRefusedError}: access, ownership and read-only refusals are - * `E_PERMISSION`, a full disk is `E_NO_SPACE`, and a path of the wrong kind is - * `E_PATH_CONFLICT`. + * `E_PERMISSION`, a full disk is `E_NO_SPACE`, and a path of the wrong kind + * or one too long for the filesystem is `E_PATH_CONFLICT`. */ const ATOMIC_WRITE_REFUSAL_ERROR_CODES: Readonly< Record @@ -498,6 +574,7 @@ const ATOMIC_WRITE_REFUSAL_ERROR_CODES: Readonly< read_only_filesystem: "E_PERMISSION", no_space: "E_NO_SPACE", not_a_directory: "E_PATH_CONFLICT", + name_too_long: "E_PATH_CONFLICT", }; /** diff --git a/src/lib/private-file.ts b/src/lib/private-file.ts index 48e7dac08..2f1ff8f35 100644 --- a/src/lib/private-file.ts +++ b/src/lib/private-file.ts @@ -2,7 +2,6 @@ import { randomUUID } from "node:crypto"; import { constants, type Stats } from "node:fs"; import { type FileHandle, - link, lstat, open, realpath, @@ -10,6 +9,7 @@ import { unlink, } from "node:fs/promises"; import { basename, dirname, join } from "node:path"; +import { publishFileExclusive } from "./exclusive-publish.ts"; import { closedFileWriteError } from "./fs.ts"; import { isRecord } from "./guards.ts"; @@ -385,10 +385,17 @@ export async function replacePrivateFileAtomic( * The complete contents are staged in a private (0600, fsynced) sibling and * published with link(2), which fails with EEXIST instead of replacing an * existing entry. Unlike {@link createPrivateFileExclusive}, readers never see - * an empty or partial file, so a concurrent loser can adopt the winner's file + * a partial file, so a concurrent loser can adopt the winner's file * immediately. An existing regular file returns false untouched; a symlink * (dangling or not), a non-regular entry, or a symlinked or replaced directory * from `directories` throws `refusal()`. The staging file is always removed. + * + * On a filesystem without hard links (ExFAT, FAT) the sibling is renamed over + * an exclusive, no-follow empty reservation instead (see + * `publishByExclusivePlaceholder`): an existing entry is still never replaced + * or followed, and readers see nothing, an empty file or the complete file. + * A process killed between the two steps leaves an empty file, which is + * refused, never adopted, until it is removed. */ export async function publishPrivateFileExclusive( opts: ReplaceablePrivateFileOptions & { readonly text: string } @@ -409,13 +416,11 @@ export async function publishPrivateFileExclusive( await file.writeFile(opts.text, "utf8"); await file.sync(); await recheckRealDirectories({ directories, refusal: opts.refusal }); - try { - await link(staging, opts.path); - published = true; - } catch (error: unknown) { - if (!(isRecord(error) && error.code === "EEXIST")) { - throw error; - } + published = await publishFileExclusive({ + source: staging, + target: opts.path, + }); + if (!published) { // Lost the race: classify what is there now (regular file or refusal). await observeReplaceableTarget(opts); } diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index fa7684f57..173a6592a 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -26,7 +26,7 @@ import { HACK_DIR_GITIGNORE_END_MARKER, HACK_DIR_GITIGNORE_ENTRIES, } from "../templates.ts"; -import { ClosedCliError } from "./cli-result.ts"; +import { ClosedCliError, HackCliError } from "./cli-result.ts"; import { parseDotEnv, serializeDotEnv } from "./env.ts"; import { withEnvLayerLock } from "./env-layer-lock.ts"; import { @@ -35,6 +35,7 @@ import { ensureDir, ensureDirForWrite, ensureGitignoreEntry, + ensureRealDirectoryForWrite, mergeManagedGitignoreBlock, pathExists, readAtomicWriteTargetText, @@ -79,7 +80,10 @@ import { assertLegacyProjectInputFamily, ProjectInputSelectionError, } from "./project-input-selection.ts"; -import { withProjectsRegistryLock } from "./projects-registry-lock.ts"; +import { + ProjectsRegistryLockTimeoutError, + withProjectsRegistryLock, +} from "./projects-registry-lock.ts"; import { isNativeManagedLocalTracked, resolvePrimaryLocalProjectDir, @@ -592,12 +596,13 @@ export const HACK_DIR_GITIGNORE_REFUSAL = /** * Refusal to read or write `.hack/.gitignore` through a symlink or a - * non-regular entry. Names no path or contents. Closed: the CLI prints its - * message as one line, never with a stack. + * non-regular entry (for example a directory). Names no path or contents. + * Closed with `E_PATH_CONFLICT`: the CLI prints it as one `CODE: message` + * line, never with a stack, and `--json` reports the same code. */ -export class HackDirGitignoreRefusalError extends ClosedCliError { +export class HackDirGitignoreRefusalError extends HackCliError { constructor() { - super(HACK_DIR_GITIGNORE_REFUSAL); + super({ code: "E_PATH_CONFLICT", message: HACK_DIR_GITIGNORE_REFUSAL }); this.name = "HackDirGitignoreRefusalError"; } } @@ -3337,6 +3342,7 @@ async function readExistingProjectEnvKeyFileText(opts: { /** * Takes the project's hydration lock: `.internal/env-hydrate.lock`, created * exclusively (no symlink follow, mode 0600) and holding the owner's PID. + * `.internal` itself must be a real directory (or absent, then created). * * An existing lock is never removed or taken over, because a second process * cannot prove the first one is gone without racing a third. It refuses as @@ -3350,7 +3356,9 @@ async function acquireProjectEnvHydrationLock(opts: { readonly lockPath: string; readonly refuse: ProjectEnvHydrationRefuse; }): Promise<() => Promise> { - await ensureDirForWrite(dirname(opts.lockPath)); + // A symlinked or non-directory `.internal` (including a dangling link) is + // refused as E_PATH_CONFLICT; the lock is never created through it. + await ensureRealDirectoryForWrite(dirname(opts.lockPath)); await assertLegacyProjectDirectory({ projectDir: opts.projectDir }); try { await createPrivateFileExclusive({ @@ -3987,9 +3995,10 @@ export async function ensureProjectEnvSecretKey(opts: { // git common dir serializes creation; under it the full lookup runs again // and adopts a key another checkout created meanwhile. let started = false; + const lockPath = `${sharedLocation.path}.lock`; try { return await withProjectsRegistryLock({ - lockPath: `${sharedLocation.path}.lock`, + lockPath, label: "project env key", run: async () => { started = true; @@ -4000,6 +4009,12 @@ export async function ensureProjectEnvSecretKey(opts: { if (started) { throw error; } + if (error instanceof ProjectsRegistryLockTimeoutError) { + throw new ProjectEnvKeyLockTimeoutError({ + lockPath, + recoveryBusy: error.recoveryBusy, + }); + } if (!isProjectEnvKeyLockUnavailable(error)) { // Any other filesystem failure the user must fix (a full disk or quota, // a non-directory on the way) is refused as the key it blocked, never @@ -4018,6 +4033,29 @@ export async function ensureProjectEnvSecretKey(opts: { } } +/** + * The project env key lock stayed held for the whole bounded wait: another + * hack command is creating the key, or a lock was left behind by a process + * whose PID is now reused (which is never reclaimed automatically), or an + * interrupted recovery left its guard. Closed (`E_LOCKED`) and value-free: it + * names the lock path and the remedy only. Nothing was written. + */ +export class ProjectEnvKeyLockTimeoutError extends HackCliError { + constructor(opts: { + readonly lockPath: string; + readonly recoveryBusy: boolean; + }) { + const stale = opts.recoveryBusy + ? `${opts.lockPath} and the directory ${opts.lockPath}.recovery` + : opts.lockPath; + super({ + code: "E_LOCKED", + message: `Timed out waiting for the project env key lock ${opts.lockPath}; nothing was written. Another hack command may be creating the key: retry when it finishes. If no other hack command is running, the lock is stale: remove ${stale}, then retry.`, + }); + this.name = "ProjectEnvKeyLockTimeoutError"; + } +} + function isProjectEnvKeyLockUnavailable(error: unknown): boolean { return ( isRecord(error) && diff --git a/src/lib/project.ts b/src/lib/project.ts index 29305f16b..a6b223413 100644 --- a/src/lib/project.ts +++ b/src/lib/project.ts @@ -135,6 +135,14 @@ export const ENV_BASE_SELECTION_ALIASES: ReadonlySet = new Set([ */ export const ENV_CONFIG_NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; +/** + * Longest accepted overlay name. An overlay name becomes part of several file + * names in one directory; the longest is the env layer lock's staging name, + * `hack.env..local.yaml.lock..owner` (the name plus 68 bytes), + * which must fit the common 255-byte file name limit with room to spare. + */ +export const ENV_CONFIG_NAME_MAX_LENGTH = 128; + /** * Parse a requested overlay selection (CLI `--env`, daemon `env`). * @@ -142,7 +150,8 @@ export const ENV_CONFIG_NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; * - `null`: base layers only (empty, or a base alias in any case). * - `string`: the overlay name, exactly as given apart from surrounding * whitespace. - * - `undefined` for a non-empty input: refused. A requested name is never + * - `undefined` for a non-empty input that is not a canonical overlay name + * or is longer than {@link ENV_CONFIG_NAME_MAX_LENGTH}: refused. A requested name is never * slugged, so `X=secret` or `QA` cannot silently become `xsecret` or `qa` * and then be printed or written into an overlay file name. Callers must * refuse with fixed text that does not include the input. @@ -163,7 +172,10 @@ export function parseEnvConfigSelection( return null; } - return ENV_CONFIG_NAME_PATTERN.test(trimmed) ? trimmed : undefined; + return trimmed.length <= ENV_CONFIG_NAME_MAX_LENGTH && + ENV_CONFIG_NAME_PATTERN.test(trimmed) + ? trimmed + : undefined; } export function sanitizeBranchSlug(input: string): string { diff --git a/src/lib/projects-registry-lock.ts b/src/lib/projects-registry-lock.ts index e7e70fe8a..97ad17112 100644 --- a/src/lib/projects-registry-lock.ts +++ b/src/lib/projects-registry-lock.ts @@ -2,7 +2,6 @@ import { randomUUID } from "node:crypto"; import { constants, type Stats } from "node:fs"; import { type FileHandle, - link, lstat, mkdir, open, @@ -11,6 +10,7 @@ import { unlink, } from "node:fs/promises"; import { setTimeout as delay } from "node:timers/promises"; +import { publishFileExclusive } from "./exclusive-publish.ts"; import { isRecord } from "./guards.ts"; const WAIT_MS = 10_000; @@ -20,11 +20,16 @@ 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. + * `recoveryBusy` is true when the recovery guard (`.recovery`) was held + * at the end of the wait, which only offline inspection can clear. */ export class ProjectsRegistryLockTimeoutError extends Error { - constructor(message: string) { + readonly recoveryBusy: boolean; + + constructor(message: string, opts: { readonly recoveryBusy?: boolean } = {}) { super(message); this.name = "ProjectsRegistryLockTimeoutError"; + this.recoveryBusy = opts.recoveryBusy ?? false; } } @@ -37,6 +42,11 @@ interface Owner { /** * Serialize cooperative registry writers. A fully written owner is published by * an exclusive hard link, so a killed publisher never leaves an empty lock. + * On a filesystem without hard links (ExFAT, FAT) the owner is published by an + * exclusive empty reservation that the complete owner is renamed over; see + * `publishByExclusivePlaceholder`. There, a publisher killed between the two + * steps leaves an empty lock, which is never reclaimed (it names no owner) and + * times out until it is removed by hand. * Age is not authority: only ESRCH permits reclamation, under a separate guard. * Live/reused/inaccessible PIDs and uncertain receipts are never reclaimed. * @@ -66,8 +76,9 @@ export async function withProjectsRegistryLock(opts: { await acquire({ ...opts, candidate }); try { // The descriptor stays open until release, preventing inode reuse while a - // caller still holds its receipt. Remove the staging name before running. - await unlink(candidate); + // caller still holds its receipt. Remove the staging name before running; + // the no-hard-link fallback has already renamed it into place. + await removeAbsentOkay(candidate); opts.signal?.throwIfAborted(); return await opts.run(); } finally { @@ -135,7 +146,8 @@ async function acquire(opts: { ? "; recovery guard is busy (interrupted recovery requires offline inspection)" : ""; throw new ProjectsRegistryLockTimeoutError( - `Timed out waiting for ${opts.label ?? "projects registry"} lock${recovery}` + `Timed out waiting for ${opts.label ?? "projects registry"} lock${recovery}`, + { recoveryBusy } ); } // Jitter prevents a burst of CLI clients from retrying in lockstep. @@ -146,15 +158,7 @@ async function acquire(opts: { } async function publish(candidate: string, path: string): Promise { - try { - await link(candidate, path); - return true; - } catch (error) { - if (hasCode(error, "EEXIST")) { - return false; - } - throw error; - } + return await publishFileExclusive({ source: candidate, target: path }); } function hasCode(error: unknown, code: string): boolean { diff --git a/tests/fixtures/staging-errno-preload.ts b/tests/fixtures/staging-errno-preload.ts index f3fec9555..902a56885 100644 --- a/tests/fixtures/staging-errno-preload.ts +++ b/tests/fixtures/staging-errno-preload.ts @@ -9,9 +9,10 @@ import * as fs from "node:fs/promises"; * would. No real disk is filled. * * - `HACK_TEST_ERRNO_STEP` picks the step (default `open`): `open`, `access`, - * `lstat` or `rename` of a matching path (the rename's source), `write` or - * `sync` on a handle opened at a matching path, or `bun-write` (`Bun.write`) - * to a matching path. + * `lstat` or `rename` of a matching path (the rename's source), `link` from + * a matching source (for example ENOTSUP, as on a filesystem without hard + * links such as ExFAT), `write` or `sync` on a handle opened at a matching + * path, or `bun-write` (`Bun.write`) to a matching path. * - `HACK_TEST_ERRNO_PATH` is a regular expression a path must match (default * `\.tmp$`, an atomic-write staging sibling). * @@ -28,6 +29,7 @@ const STEPS = [ "access", "lstat", "rename", + "link", "write", "sync", "bun-write", @@ -53,6 +55,7 @@ const realOpen = fs.open; const realAccess = fs.access; const realLstat = fs.lstat; const realRename = fs.rename; +const realLink = fs.link; type Handle = Awaited>; @@ -98,6 +101,12 @@ mock.module("node:fs/promises", () => ({ } return await realRename(...args); }, + link: async (...args: Parameters) => { + if (step === "link" && matches(args[0])) { + throw injected("link", String(args[0])); + } + return await realLink(...args); + }, })); if (step === "bun-write") { diff --git a/tests/project-env-write-refusal-gaps.test.ts b/tests/project-env-write-refusal-gaps.test.ts new file mode 100644 index 000000000..79e3e8a57 --- /dev/null +++ b/tests/project-env-write-refusal-gaps.test.ts @@ -0,0 +1,921 @@ +import { afterEach, expect, test } from "bun:test"; +import { + chmod, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + readlink, + realpath, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { basename, join, resolve } from "node:path"; + +import { INVALID_ENV_SELECTION_MESSAGE } from "../src/cli/env-selection.ts"; +import { + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, +} from "../src/constants.ts"; +import { HackCliError, type HackErrorCode } from "../src/lib/cli-result.ts"; +import { + isHardLinkUnsupportedError, + publishByExclusivePlaceholder, +} from "../src/lib/exclusive-publish.ts"; +import { + type AtomicWriteRefusal, + AtomicWriteRefusedError, + ensureDirForWrite, + ensureRealDirectoryForWrite, + FileWriteRefusedError, +} from "../src/lib/fs.ts"; +import { + ENV_CONFIG_NAME_MAX_LENGTH, + parseEnvConfigSelection, +} from "../src/lib/project.ts"; +import { + HACK_DIR_GITIGNORE_REFUSAL, + HackDirGitignoreRefusalError, + ProjectEnvKeyLockTimeoutError, + resolveProjectEnvConfigPath, +} from "../src/lib/project-env-config.ts"; +import { + ProjectsRegistryLockTimeoutError, + withProjectsRegistryLock, +} from "../src/lib/projects-registry-lock.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1377: env write refusals still raw after HACK-1330. + * + * 1. A filesystem without hard links (ExFAT, FAT) refused every env write + * with a raw ENOTSUP from the lock's and the key's `link(2)` publish; the + * publish now falls back to an exclusive reservation plus rename. + * 2. A dangling `.hack/.internal` symlink made hydrate print a raw EEXIST. + * 3. A very long `--env` name printed a raw ENAMETOOLONG with a staging path. + * 4. A stale project env key lock timed out with a stack. + * 5. A `.hack/.gitignore` directory printed a line with no code. + * 6. Mappings whose revert failed no test. + * + * Every value is a synthetic test string; every child gets a private HOME, + * HACK_HOME and Docker client config inside a temp directory, never the + * keychain; an env layer exists before any `--secret` set. + */ + +const REPO_ROOT = resolve(import.meta.dir, ".."); +const ENTRYPOINT = resolve(REPO_ROOT, "index.ts"); +const ERRNO_PRELOAD = resolve( + import.meta.dir, + "fixtures/staging-errno-preload.ts" +); +const KEPT_VALUE = "synthetic-kept-41c7"; +const NEW_VALUE = "synthetic-new-90ad"; +const LAYER = `version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global:\n KEPT_SETTING: ${KEPT_VALUE}\n`; +const HYDRATE_INPUT = `NEW_SETTING=${NEW_VALUE}\n`; + +/** Permission bits do not bind root; permission-based cases skip under it. */ +const RUNS_AS_ROOT = (process.geteuid?.() ?? process.getuid?.() ?? -1) === 0; + +const tempDirs = new Set(); + +afterEach(async () => { + for (const tempDir of tempDirs) { + await rm(tempDir, { recursive: true, force: true }); + } + tempDirs.clear(); +}, 30_000); + +type Repo = { + readonly root: string; + readonly projectRoot: string; + readonly projectDir: string; + readonly tracked: string; +}; + +async function createRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), "hack-env-gaps-")); + 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-gaps", dev_host: "env-gaps.hack" }, null, 2)}\n` + ); + const tracked = resolveProjectEnvConfigPath({ projectDir, envName: null }); + await writeFile(tracked, LAYER); + return { root, projectRoot, projectDir, tracked }; +} + +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; + readonly logger?: string; + /** Injected errno: the preload fails one filesystem step of one path. */ + readonly inject?: { + readonly code: string; + readonly step: string; + readonly path: string; + }; +}): Promise { + const preload = opts.inject ? ["--preload", ERRNO_PRELOAD] : []; + const injected = opts.inject + ? { + HACK_TEST_STAGING_ERRNO: opts.inject.code, + HACK_TEST_ERRNO_STEP: opts.inject.step, + HACK_TEST_ERRNO_PATH: opts.inject.path, + } + : {}; + const proc = Bun.spawn( + [process.execPath, ...preload, ENTRYPOINT, ...opts.args], + { + 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_LOGGER: opts.logger ?? "console", + HACK_NO_INTERACTIVE: "1", + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + ...injected, + }, + }), + 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 runGit(repo: Repo, args: readonly string[]): Promise { + const proc = Bun.spawn(["git", ...args], { + cwd: repo.projectRoot, + env: privateCliEnv({ + root: repo.root, + env: { GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null" }, + }), + stdout: "ignore", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([ + new Response(proc.stderr).text(), + proc.exited, + ]); + expect({ args, exitCode, stderr }).toEqual({ args, exitCode: 0, stderr: "" }); +} + +/** Output lines that carry text, ignoring the clack frame (`│`, `■`, ...). */ +function textLines(run: CliRun): string[] { + return `${run.stdout}\n${run.stderr}` + .split("\n") + .filter((line) => /[A-Za-z]/.test(line)); +} + +const RAW_ERRNO_CODES = [ + "ENOTSUP", + "EOPNOTSUPP", + "EPERM", + "EEXIST", + "ENAMETOOLONG", + "EISDIR", + "ENOENT", + "EROFS", +] as const; + +/** One closed line: no stack, no source path, no value, no errno, no staging name. */ +function expectOneLineRefusal(run: CliRun, expected: string): void { + expect(run.exitCode).toBe(1); + const lines = textLines(run); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(expected); + expectNoRawDetail(run); +} + +function expectNoRawDetail(run: CliRun): void { + const output = `${run.stdout}${run.stderr}`; + expect(output).not.toMatch(/\bat\s.*\.ts:\d+/); + expect(output).not.toContain(REPO_ROOT); + expect(output).not.toContain(KEPT_VALUE); + expect(output).not.toContain(NEW_VALUE); + expect(output).not.toContain(".tmp"); + expect(output).not.toContain(".owner"); + for (const code of RAW_ERRNO_CODES) { + expect(output).not.toContain(code); + } + expect(output).not.toMatch(/\b(open|mkdir|rename|link) '/); +} + +/** Exit 0 with only the success line: no warning, error or raw detail. */ +function expectSuccess(run: CliRun): void { + expect({ exitCode: run.exitCode, stdout: run.stdout }).toEqual({ + exitCode: 0, + stdout: "", + }); + expect(run.stderr).toStartWith("SUCCESS: Updated "); + expect(textLines(run)).toHaveLength(1); + expectNoRawDetail(run); +} + +type JsonFailure = { readonly code: string; readonly message: string }; + +function jsonFailure(run: CliRun): JsonFailure { + expect(run.exitCode).toBe(1); + const envelope: unknown = JSON.parse(run.stdout); + expect(envelope).toMatchObject({ ok: false }); + return (envelope as { readonly error: JsonFailure }).error; +} + +function setArgs(repo: Repo, key: string, ...extra: string[]): string[] { + return [ + "env", + "set", + key, + "--value-stdin", + ...extra, + "--path", + repo.projectRoot, + ]; +} + +/** Names in `directory` that a failed or finished write must not leave. */ +async function leftovers(directory: string): Promise { + const names = await readdir(directory).catch(() => [] as string[]); + return names.filter( + (name) => + name.endsWith(".tmp") || name.endsWith(".owner") || name.endsWith(".lock") + ); +} + +// --------------------------------------------------------------------------- +// 1. No hard links: the exclusive publish falls back to reservation + rename. +// --------------------------------------------------------------------------- + +/** Every `link(2)` fails as it does on ExFAT (macOS) or FAT (Linux). */ +function noHardLinks(code: "ENOTSUP" | "EPERM") { + return { code, step: "link", path: "." } as const; +} + +for (const code of ["ENOTSUP", "EPERM"] as const) { + for (const git of [false, true]) { + test(`with link(2) failing ${code}, the first --secret ${git ? "in a git checkout " : ""}creates the key and later writes still lock`, async () => { + const repo = await createRepo(); + if (git) { + await runGit(repo, ["init", "-q"]); + } + const keyPath = resolve(repo.projectRoot, ".hack.secret.key"); + + const first = await runCli({ + repo, + args: setArgs(repo, "FIRST_SECRET", "--secret"), + stdin: `${NEW_VALUE}\n`, + inject: noHardLinks(code), + }); + expectSuccess(first); + expectNoRawDetail(first); + const keyText = (await readFile(keyPath, "utf8")).trim(); + expect(keyText.length).toBeGreaterThan(0); + expect((await lstat(keyPath)).mode & 0o777).toBe(0o600); + + // The lock was released (a second write would otherwise time out), the + // key was adopted, and nothing was left behind. + const second = await runCli({ + repo, + args: setArgs(repo, "SECOND_SECRET", "--secret"), + stdin: `${NEW_VALUE}\n`, + inject: noHardLinks(code), + }); + expect(second.exitCode).toBe(0); + expect((await readFile(keyPath, "utf8")).trim()).toBe(keyText); + const layer = await readFile(repo.tracked, "utf8"); + expect(layer).toContain("FIRST_SECRET"); + expect(layer).toContain("SECOND_SECRET"); + expect(layer).toContain(KEPT_VALUE); + expect(layer).not.toContain(NEW_VALUE); + expect(await leftovers(repo.projectRoot)).toEqual([]); + expect(await leftovers(repo.projectDir)).toEqual([]); + expect(await leftovers(resolve(repo.projectDir, ".internal"))).toEqual( + [] + ); + if (git) { + expect(await leftovers(resolve(repo.projectRoot, ".git"))).toEqual([]); + } + }, 60_000); + } +} + +test("with link(2) failing ENOTSUP, concurrent sets of different keys are all kept", async () => { + const repo = await createRepo(); + const keys = ["ALPHA_SETTING", "BRAVO_SETTING", "CHARLIE_SETTING"]; + const runs = await Promise.all( + keys.map( + async (key) => + await runCli({ + repo, + args: setArgs(repo, key), + stdin: `${NEW_VALUE}\n`, + inject: noHardLinks("ENOTSUP"), + }) + ) + ); + expect(runs.map((run) => run.exitCode)).toEqual([0, 0, 0]); + const layer = await readFile(repo.tracked, "utf8"); + for (const key of keys) { + expect(layer).toContain(key); + } + expect(layer).toContain(KEPT_VALUE); + expect(await leftovers(resolve(repo.projectDir, ".internal"))).toEqual([]); +}, 60_000); + +test("only link(2) errors that mean no hard links select the fallback", () => { + for (const code of ["ENOTSUP", "EOPNOTSUPP", "EPERM"]) { + expect(isHardLinkUnsupportedError({ code })).toBe(true); + } + for (const code of ["EEXIST", "EACCES", "EXDEV", "ENOENT", "EMLINK"]) { + expect(isHardLinkUnsupportedError({ code })).toBe(false); + } + expect(isHardLinkUnsupportedError("ENOTSUP")).toBe(false); + expect(isHardLinkUnsupportedError(null)).toBe(false); +}); + +async function scratch(): Promise { + const directory = await mkdtemp(join(tmpdir(), "hack-publish-")); + tempDirs.add(directory); + return directory; +} + +test("the reservation fallback publishes the complete file and consumes the source", async () => { + const directory = await scratch(); + const source = join(directory, ".target.staged.tmp"); + const target = join(directory, "target"); + await writeFile(source, "complete\n", { mode: 0o600 }); + + expect(await publishByExclusivePlaceholder({ source, target })).toBe(true); + expect(await readFile(target, "utf8")).toBe("complete\n"); + expect(await readdir(directory)).toEqual(["target"]); +}); + +test("the reservation fallback never replaces an existing file", async () => { + const directory = await scratch(); + const source = join(directory, ".target.staged.tmp"); + const target = join(directory, "target"); + await writeFile(source, "mine\n"); + await writeFile(target, "theirs\n"); + + expect(await publishByExclusivePlaceholder({ source, target })).toBe(false); + expect(await readFile(target, "utf8")).toBe("theirs\n"); + expect(await readFile(source, "utf8")).toBe("mine\n"); +}); + +test("the reservation fallback never follows or replaces a symlink", async () => { + const directory = await scratch(); + const source = join(directory, ".target.staged.tmp"); + const target = join(directory, "target"); + const elsewhere = join(directory, "elsewhere"); + await writeFile(source, "mine\n"); + await symlink(elsewhere, target); + + expect(await publishByExclusivePlaceholder({ source, target })).toBe(false); + expect(await readlink(target)).toBe(elsewhere); + expect(await readdir(directory)).not.toContain("elsewhere"); +}); + +test("a failed rename removes the reservation it created", async () => { + const directory = await scratch(); + const target = join(directory, "target"); + let caught: unknown; + try { + await publishByExclusivePlaceholder({ + source: join(directory, "missing-source"), + target, + }); + } catch (error: unknown) { + caught = error; + } + expect(caught).toMatchObject({ code: "ENOENT" }); + expect(await readdir(directory)).toEqual([]); +}); + +// Real ExFAT: opt-in on macOS, because it attaches and mounts a disk image. +// `HACK_TEST_EXFAT=1 bun test tests/project-env-write-refusal-gaps.test.ts` +const EXFAT_ENABLED = + process.platform === "darwin" && process.env.HACK_TEST_EXFAT === "1"; + +async function sh( + command: readonly string[] +): Promise<{ readonly exitCode: number; readonly stdout: string }> { + const proc = Bun.spawn([...command], { stdout: "pipe", stderr: "pipe" }); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + return { exitCode, stdout }; +} + +test.skipIf(!EXFAT_ENABLED)( + "on a real ExFAT volume the first --secret creates the key, in and out of git", + async () => { + const base = await mkdtemp(join(tmpdir(), "hack-exfat-")); + tempDirs.add(base); + const image = join(base, "volume.img"); + const mountPoint = join(base, "mnt"); + await mkdir(mountPoint); + expect((await sh(["mkfile", "-n", "64m", image])).exitCode).toBe(0); + const attached = await sh([ + "hdiutil", + "attach", + "-nomount", + "-imagekey", + "diskimage-class=CRawDiskImage", + image, + ]); + const device = attached.stdout.trim().split(/\s+/)[0] ?? ""; + expect(device).toMatch(/^\/dev\/disk\d+$/); + try { + expect( + (await sh(["newfs_exfat", "-v", "hackexfat", device])).exitCode + ).toBe(0); + expect( + (await sh(["diskutil", "mount", "-mountPoint", mountPoint, device])) + .exitCode + ).toBe(0); + const mounts = await sh(["mount"]); + expect(mounts.stdout).toMatch(new RegExp(`${device} on .*\\(exfat`)); + for (const git of [false, true]) { + const root = await mkdtemp(join(mountPoint, "case-")); + const projectRoot = join(root, "repo"); + 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: "env-exfat", dev_host: "env-exfat.hack" })}\n` + ); + const tracked = resolveProjectEnvConfigPath({ + projectDir, + envName: null, + }); + await writeFile(tracked, LAYER); + const repo: Repo = { root, projectRoot, projectDir, tracked }; + if (git) { + await runGit(repo, ["init", "-q"]); + } + for (const key of ["FIRST_SECRET", "SECOND_SECRET"]) { + const run = await runCli({ + repo, + args: setArgs(repo, key, "--secret"), + stdin: `${NEW_VALUE}\n`, + }); + expectSuccess(run); + } + const keyText = ( + await readFile(join(projectRoot, ".hack.secret.key"), "utf8") + ).trim(); + expect(keyText.length).toBeGreaterThan(0); + expect(await readFile(tracked, "utf8")).toContain("SECOND_SECRET"); + expect(await leftovers(join(projectDir, ".internal"))).toEqual([]); + if (git) { + expect(await leftovers(join(projectRoot, ".git"))).toEqual([]); + } + } + } finally { + await sh(["diskutil", "unmount", "force", mountPoint]); + await sh(["hdiutil", "detach", "-force", device]); + } + }, + 120_000 +); + +// --------------------------------------------------------------------------- +// 2. A symlinked or non-directory `.hack/.internal` refuses hydrate closed. +// --------------------------------------------------------------------------- + +const INTERNAL_REFUSAL = (internal: string) => + `E_PATH_CONFLICT: Could not create directory ${internal}: that path is a symlink or not a directory. Replace it with a regular directory, then retry.`; + +for (const kind of ["dangling", "live"] as const) { + test(`hydrate with a ${kind} .hack/.internal symlink is a closed E_PATH_CONFLICT`, async () => { + const repo = await createRepo(); + const internal = resolve(repo.projectDir, ".internal"); + const elsewhere = resolve(repo.root, "elsewhere"); + if (kind === "live") { + await mkdir(elsewhere); + } + await symlink(elsewhere, internal); + + for (const logger of ["console", "clack", "gum"]) { + const run = await runCli({ + repo, + args: ["env", "hydrate", "--path", repo.projectRoot], + stdin: HYDRATE_INPUT, + logger, + }); + expectOneLineRefusal(run, INTERNAL_REFUSAL(internal)); + } + const json = await runCli({ + repo, + args: ["env", "hydrate", "--json", "--path", repo.projectRoot], + stdin: HYDRATE_INPUT, + }); + expect(jsonFailure(json)).toEqual({ + code: "E_PATH_CONFLICT", + message: INTERNAL_REFUSAL(internal).replace("E_PATH_CONFLICT: ", ""), + }); + // Nothing was created through the link, and the link is untouched. + expect(await readlink(internal)).toBe(elsewhere); + expect(await readdir(elsewhere).catch(() => null)).toEqual( + kind === "live" ? [] : null + ); + expect(await readFile(repo.tracked, "utf8")).toBe(LAYER); + }, 60_000); +} + +test("ensureDirForWrite refuses a dangling symlink or a file closed, not as EEXIST", async () => { + const directory = await scratch(); + const dangling = join(directory, "dangling"); + const file = join(directory, "file"); + await symlink(join(directory, "nowhere"), dangling); + await writeFile(file, "x"); + for (const path of [dangling, file]) { + let caught: unknown; + try { + await ensureDirForWrite(path); + } catch (error: unknown) { + caught = error; + } + expect(caught).toBeInstanceOf(FileWriteRefusedError); + expect(caught).toMatchObject({ + code: "E_PATH_CONFLICT", + failure: "not_a_directory_entry", + message: INTERNAL_REFUSAL(path).replace("E_PATH_CONFLICT: ", ""), + }); + } + // A real directory (new or existing) is fine. + await ensureRealDirectoryForWrite(join(directory, "fresh")); + await ensureRealDirectoryForWrite(join(directory, "fresh")); + expect((await lstat(join(directory, "fresh"))).isDirectory()).toBe(true); +}); + +// --------------------------------------------------------------------------- +// 3. A too-long `--env` name is a closed E_USAGE; ENAMETOOLONG is closed too. +// --------------------------------------------------------------------------- + +const LONGEST_NAME = "e".repeat(ENV_CONFIG_NAME_MAX_LENGTH); +const TOO_LONG_NAME = "e".repeat(215); + +test("overlay names are accepted up to the limit and refused beyond it", () => { + expect(ENV_CONFIG_NAME_MAX_LENGTH).toBe(128); + expect(parseEnvConfigSelection(LONGEST_NAME)).toBe(LONGEST_NAME); + expect(parseEnvConfigSelection(`${LONGEST_NAME}e`)).toBeUndefined(); + expect(parseEnvConfigSelection(TOO_LONG_NAME)).toBeUndefined(); + expect(INVALID_ENV_SELECTION_MESSAGE).toContain("at most 128 characters"); +}); + +test("set, unset and hydrate refuse a too-long --env before touching the project", async () => { + const repo = await createRepo(); + for (const args of [ + setArgs(repo, "NEW_SETTING", "--env", TOO_LONG_NAME), + ["env", "unset", "KEPT_SETTING", "--env", TOO_LONG_NAME], + ]) { + const run = await runCli({ repo, args, stdin: `${NEW_VALUE}\n` }); + expect(run.exitCode).toBe(1); + // A usage error found while parsing arguments also prints the help. + expect(run.stderr).toContain(`ERROR: ${INVALID_ENV_SELECTION_MESSAGE}`); + const output = `${run.stdout}${run.stderr}`; + for (const raw of ["ENAMETOOLONG", ".owner", "hack.env.e", NEW_VALUE]) { + expect(output).not.toContain(raw); + } + expect(output).not.toMatch(/\bat\s.*\.ts:\d+/); + expect(output).not.toContain(TOO_LONG_NAME); + } + const json = await runCli({ + repo, + args: [ + "env", + "hydrate", + "--json", + "--env", + TOO_LONG_NAME, + "--path", + repo.projectRoot, + ], + stdin: HYDRATE_INPUT, + }); + expect(jsonFailure(json)).toEqual({ + code: "E_USAGE", + message: INVALID_ENV_SELECTION_MESSAGE, + }); + expect((await readdir(repo.projectDir)).sort()).toEqual( + [ + PROJECT_COMPOSE_FILENAME, + PROJECT_CONFIG_FILENAME, + basename(repo.tracked), + ].sort() + ); +}, 60_000); + +test("an overlay name at the limit still writes its layer", async () => { + const repo = await createRepo(); + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SETTING", "--env", LONGEST_NAME), + stdin: `${NEW_VALUE}\n`, + }); + expectSuccess(run); + const overlay = resolveProjectEnvConfigPath({ + projectDir: repo.projectDir, + envName: LONGEST_NAME, + }); + expect(await readFile(overlay, "utf8")).toContain("NEW_SETTING"); +}, 60_000); + +const NAME_TOO_LONG_REASON = + "a file name or the path is too long for the filesystem. Use a shorter name or move the project to a shorter path, then retry."; + +test("an injected ENAMETOOLONG at the layer staging and at its lock is a closed E_PATH_CONFLICT", async () => { + const repo = await createRepo(); + const stagingPattern = "/\\.hack\\.env\\.default\\.yaml\\.[^/]+\\.tmp$"; + const lockOwnerPattern = + "/hack\\.env\\.default\\.yaml\\.lock\\.[^/]+\\.owner$"; + for (const [path, expected] of [ + [ + stagingPattern, + `E_PATH_CONFLICT: Refusing to replace ${repo.tracked}: ${NAME_TOO_LONG_REASON}`, + ], + [ + lockOwnerPattern, + `E_PATH_CONFLICT: Could not write ${repo.tracked}: ${NAME_TOO_LONG_REASON}`, + ], + ] as const) { + for (const args of [ + setArgs(repo, "NEW_SETTING"), + ["env", "unset", "KEPT_SETTING", "--path", repo.projectRoot], + ]) { + const run = await runCli({ + repo, + args, + stdin: `${NEW_VALUE}\n`, + inject: { code: "ENAMETOOLONG", step: "open", path }, + }); + expectOneLineRefusal(run, expected); + } + } + expect(await readFile(repo.tracked, "utf8")).toBe(LAYER); + expect(await leftovers(repo.projectDir)).toEqual([]); +}, 60_000); + +// --------------------------------------------------------------------------- +// 4. A stale project env key lock is a closed E_LOCKED naming the remedy. +// --------------------------------------------------------------------------- + +test("a key lock held by a live unrelated PID times out as a closed E_LOCKED", async () => { + const repo = await createRepo(); + await runGit(repo, ["init", "-q"]); + const lockPath = resolve( + await realpath(repo.projectRoot), + ".git", + ".hack.secret.key.lock" + ); + // This test runner is alive and is not the CLI child: a reused PID. + await writeFile(lockPath, `${process.pid}\n`, { mode: 0o600 }); + const expected = `E_LOCKED: Timed out waiting for the project env key lock ${lockPath}; nothing was written. Another hack command may be creating the key: retry when it finishes. If no other hack command is running, the lock is stale: remove ${lockPath}, then retry.`; + + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SECRET", "--secret"), + stdin: `${NEW_VALUE}\n`, + }); + expectOneLineRefusal(run, expected); + expect(await readFile(lockPath, "utf8")).toBe(`${process.pid}\n`); + expect((await readdir(repo.projectRoot)).sort()).toEqual([".git", ".hack"]); + expect(await readFile(repo.tracked, "utf8")).toBe(LAYER); +}, 60_000); + +test("a lock timeout reports a busy recovery guard, and the key refusal names it", async () => { + const directory = await scratch(); + const lockPath = join(directory, "resource.lock"); + // A dead owner (a PID far above any pid_max) with its recovery guard held. + await writeFile(lockPath, "2147483646\n", { mode: 0o600 }); + await mkdir(`${lockPath}.recovery`); + let caught: unknown; + try { + await withProjectsRegistryLock({ + lockPath, + timeoutMs: 50, + run: async () => "ran", + }); + } catch (error: unknown) { + caught = error; + } + expect(caught).toBeInstanceOf(ProjectsRegistryLockTimeoutError); + expect((caught as ProjectsRegistryLockTimeoutError).recoveryBusy).toBe(true); + + const plain = new ProjectEnvKeyLockTimeoutError({ + lockPath, + recoveryBusy: false, + }); + const busy = new ProjectEnvKeyLockTimeoutError({ + lockPath, + recoveryBusy: true, + }); + expect(plain).toBeInstanceOf(HackCliError); + expect(plain.code).toBe("E_LOCKED"); + expect(plain.message).toEndWith( + `the lock is stale: remove ${lockPath}, then retry.` + ); + expect(busy.message).toEndWith( + `the lock is stale: remove ${lockPath} and the directory ${lockPath}.recovery, then retry.` + ); +}); + +// --------------------------------------------------------------------------- +// 5. A `.hack/.gitignore` directory is E_PATH_CONFLICT, as docs/env.md says. +// --------------------------------------------------------------------------- + +test("a .hack/.gitignore directory is E_PATH_CONFLICT for set --local and hydrate --json", async () => { + const repo = await createRepo(); + const gitignore = resolve(repo.projectDir, ".gitignore"); + await mkdir(gitignore); + + for (const logger of ["console", "clack", "gum"]) { + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SETTING", "--local"), + stdin: `${NEW_VALUE}\n`, + logger, + }); + expectOneLineRefusal(run, `E_PATH_CONFLICT: ${HACK_DIR_GITIGNORE_REFUSAL}`); + } + const json = await runCli({ + repo, + args: ["env", "hydrate", "--json", "--path", repo.projectRoot], + stdin: HYDRATE_INPUT, + }); + expect(jsonFailure(json)).toEqual({ + code: "E_PATH_CONFLICT", + message: HACK_DIR_GITIGNORE_REFUSAL, + }); + expect((await lstat(gitignore)).isDirectory()).toBe(true); + expect(new HackDirGitignoreRefusalError().code).toBe("E_PATH_CONFLICT"); +}, 60_000); + +// --------------------------------------------------------------------------- +// 6. Mappings whose revert used to fail no test. +// --------------------------------------------------------------------------- + +const KEY_LOCK_OWNER_PATTERN = + "/\\.hack\\.secret\\.key\\.lock\\.[^/]+\\.owner$"; + +// The git dir cannot hold the key lock: the key is created unserialized, as +// before, instead of the lock failure being reported. +for (const code of ["ENOENT", "EPERM", "EROFS", "EACCES"] as const) { + test(`an injected ${code} at the key lock creates the key without the lock`, async () => { + const repo = await createRepo(); + await runGit(repo, ["init", "-q"]); + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SECRET", "--secret"), + stdin: `${NEW_VALUE}\n`, + inject: { code, step: "open", path: KEY_LOCK_OWNER_PATTERN }, + }); + expectSuccess(run); + const key = await readFile( + resolve(repo.projectRoot, ".hack.secret.key"), + "utf8" + ); + expect(key.trim().length).toBeGreaterThan(0); + expect(await readFile(repo.tracked, "utf8")).toContain("NEW_SECRET"); + expect(await leftovers(resolve(repo.projectRoot, ".git"))).toEqual([]); + }, 60_000); +} + +const ATOMIC_WRITE_REFUSALS: Readonly< + Record +> = { + unreadable: [ + "E_PERMISSION", + "it exists but cannot be read, so its contents cannot be kept", + ], + not_regular_file: ["E_PATH_CONFLICT", "it is not a regular file"], + not_writable: ["E_PERMISSION", "it is not writable"], + owner_not_preserved: [ + "E_PERMISSION", + "its owner and group cannot be preserved", + ], + group_not_preserved: [ + "E_PERMISSION", + "its group cannot be preserved because you are not a member of that group", + ], + directory_not_writable: [ + "E_PERMISSION", + "its directory is not writable. Make that directory writable by your user, then retry", + ], + read_only_filesystem: [ + "E_PERMISSION", + "the filesystem is read-only. Use a writable checkout or remount the filesystem read-write, then retry", + ], + no_space: [ + "E_NO_SPACE", + "no space is left on the device or the disk quota is exceeded. Free some disk space or quota, then retry", + ], + not_a_directory: [ + "E_PATH_CONFLICT", + "a parent of that path is not a directory. Replace that path with a directory, then retry", + ], + name_too_long: ["E_PATH_CONFLICT", NAME_TOO_LONG_REASON.replace(/\.$/, "")], +}; + +test("every atomic-write refusal has its fixed code and message", () => { + for (const [reason, [code, text]] of Object.entries(ATOMIC_WRITE_REFUSALS)) { + const refusal = new AtomicWriteRefusedError({ + path: "/synthetic/layer.yaml", + reason: reason as AtomicWriteRefusal, + }); + expect({ reason, code: refusal.code, message: refusal.message }).toEqual({ + reason, + code, + message: `Refusing to replace /synthetic/layer.yaml: ${text}.`, + }); + } +}); + +test("a directory at the layer path is a closed E_PATH_CONFLICT", async () => { + const repo = await createRepo(); + await rm(repo.tracked); + await mkdir(repo.tracked); + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SETTING"), + stdin: `${NEW_VALUE}\n`, + }); + expectOneLineRefusal( + run, + `E_PATH_CONFLICT: Refusing to replace ${repo.tracked}: it is not a regular file.` + ); +}, 60_000); + +test.skipIf(RUNS_AS_ROOT)( + "a read-only layer is a closed E_PERMISSION not_writable", + async () => { + const repo = await createRepo(); + await chmod(repo.tracked, 0o444); + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SETTING"), + stdin: `${NEW_VALUE}\n`, + }); + expectOneLineRefusal( + run, + `E_PERMISSION: Refusing to replace ${repo.tracked}: it is not writable.` + ); + expect(await readFile(repo.tracked, "utf8")).toBe(LAYER); + }, + 60_000 +); + +test("an injected EISDIR while staging the layer is a closed not_regular_file refusal", async () => { + const repo = await createRepo(); + const run = await runCli({ + repo, + args: setArgs(repo, "NEW_SETTING"), + stdin: `${NEW_VALUE}\n`, + inject: { + code: "EISDIR", + step: "open", + path: "/\\.hack\\.env\\.default\\.yaml\\.[^/]+\\.tmp$", + }, + }); + expectOneLineRefusal( + run, + `E_PATH_CONFLICT: Refusing to replace ${repo.tracked}: it is not a regular file.` + ); + expect(await readFile(repo.tracked, "utf8")).toBe(LAYER); + expect(await leftovers(repo.projectDir)).toEqual([]); +}, 60_000); diff --git a/tests/project-env-write-refusals.test.ts b/tests/project-env-write-refusals.test.ts index 18beaa8e0..013612b25 100644 --- a/tests/project-env-write-refusals.test.ts +++ b/tests/project-env-write-refusals.test.ts @@ -838,10 +838,12 @@ test.skipIf(RUNS_AS_ROOT)( ); // The CLI prints a closed error as one line; other code still narrows on -// these classes with instanceof, so they keep their names and messages. +// these classes with instanceof, so they keep their names and messages. The +// .hack/.gitignore refusal is a path of the wrong kind (HACK-1377). test("project key and .hack/.gitignore refusals are closed errors", () => { const gitignore = new HackDirGitignoreRefusalError(); - expect(gitignore).toBeInstanceOf(ClosedCliError); + expect(gitignore).toBeInstanceOf(HackCliError); + expect(gitignore.code).toBe("E_PATH_CONFLICT"); expect(gitignore).toBeInstanceOf(HackDirGitignoreRefusalError); expect(gitignore.name).toBe("HackDirGitignoreRefusalError"); expect(gitignore.message).toBe(HACK_DIR_GITIGNORE_REFUSAL); @@ -1221,6 +1223,7 @@ test("the write-failure classifier maps only its fixed errno set", () => { EDQUOT: ["no_space", "E_NO_SPACE"], ENOTDIR: ["not_a_directory", "E_PATH_CONFLICT"], EISDIR: ["is_a_directory", "E_PATH_CONFLICT"], + ENAMETOOLONG: ["name_too_long", "E_PATH_CONFLICT"], } as const; for (const [code, [failure, errorCode]] of Object.entries(cases)) { const raw = Object.assign(new Error(`${code}: raw, open '/x/.y.tmp'`), {