From d43027e660d07bde82e5839bcbd5ce3858a19edb Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Sat, 24 Oct 2026 02:09:30 -0400 Subject: [PATCH] fix(cli): close HACK-1426 follow-ups (up E_PROJECT_NOT_FOUND, git-accurate gitignore entry, no-TTY Tailscale test) - `hack up` without a project now prints the closed `E_PROJECT_NOT_FOUND: ...` line like down, restart, ps, logs and open. - ensureGitignoreEntry reads each line as git does (trailing CR and spaces dropped, leading whitespace kept), so an indented `.hack.secret.key` no longer counts as the entry and is no longer refused as "re-included"; the exact entry is appended instead. - tests: no-TTY guard test for "Turn on Tailscale?", a named warm-up for the cold transpiler cache in that file, the ACL test's setup moved inside its try, and the paired mode-000 `.hack` fixture without `.hack/.internal` restored. Refs HACK-1426 Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-target-result-contract.md | 2 +- src/commands/project.ts | 22 +++-- src/lib/fs.ts | 24 ++++- tests/git-info-exclude.test.ts | 18 ++-- tests/missing-project-error-code.test.ts | 37 +++++++- tests/no-tty-prompt-sites-cli.test.ts | 99 +++++++++++++++++++-- tests/project-env-key-ignore.test.ts | 43 +++++++++ tests/project-target-inspect.test.ts | 30 +++++-- 8 files changed, 236 insertions(+), 39 deletions(-) diff --git a/docs/reference/ax-target-result-contract.md b/docs/reference/ax-target-result-contract.md index bce008873..c4b2c5aeb 100644 --- a/docs/reference/ax-target-result-contract.md +++ b/docs/reference/ax-target-result-contract.md @@ -228,7 +228,7 @@ WU21). The inspector itself only produces snapshots. | Malformed Hack config or Compose file, oversized input | `config_parse_failed`, `compose_parse_failed`, `generation_input_too_large` | same | paired fixture | | Unreadable input (directory in place of a file, no read permission) | `input_unreadable`, `input_permission_denied` | same | paired fixture (permission case skipped as root) | | Input guard refusals (native/legacy conflict, unsafe adoption state, non-directory project dir) | `project_inputs_unsupported`, `adoption_state_unsafe`, `input_unreadable` | same | paired fixture | -| Unsearchable `.hack` or `.hack/.internal` on a never-adopted project | `input_permission_denied` (permission fix named first) | same | paired fixture and domain test, each at mode `000` and `0644` on both directories (skipped as root; the message is checked unconditionally) | +| Unsearchable `.hack` or `.hack/.internal` on a never-adopted project | `input_permission_denied` (permission fix named first) | same | paired fixture and domain test, each at mode `000` and `0644` on both directories, plus a paired `.hack` at `000` with no `.hack/.internal` (skipped as root; the message is checked unconditionally) | | Uncheckable input presence (`.dev` file or unsearchable `.dev` beside a valid `.hack`) | `input_unreadable`, `input_permission_denied` | same | paired fixture and domain test (unsearchable case skipped as root; `EPERM` through the classifier seam and an injected presence-check fault) | | Detached linked worktree, no project, missing path | closed refusals | same | paired fixture | | Unknown service | `service_not_found` | same | paired fixture | diff --git a/src/commands/project.ts b/src/commands/project.ts index b801775b6..8c37f4a73 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -5335,18 +5335,16 @@ async function runUpCommand({ projectOpt: args.options.project, }); } catch (error: unknown) { - if (error instanceof MissingProjectContextError) { - if (json) { - return emitLifecycleResult({ - result: errorResult({ - code: "E_PROJECT_NOT_FOUND", - message: error.message, - }), - exitCode: 1, - }); - } - logger.error({ message: error.message }); - return 1; + // Without --json the error reaches the CLI's closed-error renderer, like + // `down` and `restart`: one `E_PROJECT_NOT_FOUND: ...` line, no stack. + if (json && error instanceof MissingProjectContextError) { + return emitLifecycleResult({ + result: errorResult({ + code: "E_PROJECT_NOT_FOUND", + message: error.message, + }), + exitCode: 1, + }); } throw error; } diff --git a/src/lib/fs.ts b/src/lib/fs.ts index fd17e01ca..96bf4114b 100644 --- a/src/lib/fs.ts +++ b/src/lib/fs.ts @@ -701,8 +701,28 @@ export function mergeManagedGitignoreBlock(opts: { return `${base}${separator}${block}\n`; } +/** A line ending's carriage return, which git drops from an ignore line. */ +const GITIGNORE_LINE_CR = /\r$/; +/** Trailing spaces, which git drops from an ignore line (only spaces). */ +const GITIGNORE_TRAILING_SPACES = / +$/; + +/** + * The pattern git reads from one `.gitignore` line: a trailing carriage return + * and trailing spaces are dropped, while leading whitespace is part of the + * pattern (` .hack.secret.key` names a file that starts with two spaces). + * Escaped trailing spaces (`\ `) are not unescaped: no Hack entry ends in one. + */ +function gitignoreLinePattern(line: string): string { + return line + .replace(GITIGNORE_LINE_CR, "") + .replace(GITIGNORE_TRAILING_SPACES, ""); +} + /** * Ensures an entry exists in .gitignore. Creates the file if it doesn't exist. + * A line counts as the entry only when git reads it as the same pattern (see + * {@link gitignoreLinePattern}), so an indented copy, which git does not apply + * to the entry's path, does not stop the entry from being added. * @returns Whether the file was modified */ export async function ensureGitignoreEntry(opts: { @@ -715,7 +735,9 @@ export async function ensureGitignoreEntry(opts: { if (existing !== null) { const lines = existing.split("\n"); - const hasEntry = lines.some((line) => line.trim() === entryLine); + const hasEntry = lines.some( + (line) => gitignoreLinePattern(line) === entryLine + ); if (hasEntry) { return { changed: false }; } diff --git a/tests/git-info-exclude.test.ts b/tests/git-info-exclude.test.ts index 67688c785..1de846017 100644 --- a/tests/git-info-exclude.test.ts +++ b/tests/git-info-exclude.test.ts @@ -161,15 +161,17 @@ test.skipIf(process.platform !== "darwin")( async () => { const exclude = join(repo, ".git", "info", "exclude"); await writeFile(exclude, "# user rule\n"); - await chmod(exclude, 0o444); - const acl = Bun.spawnSync( - ["/bin/chmod", "+a", `${userInfo().username} allow write`, exclude], - { stdin: "ignore", stdout: "ignore", stderr: "ignore" } - ); - expect(acl.exitCode).toBe(0); - // The ACL makes access(W_OK) succeed; only the mode bits refuse it. - await access(exclude, constants.W_OK); + // Every step after the mode change runs inside the try, so a failed ACL + // grant or probe still restores mode 0644 for the cleanup. try { + await chmod(exclude, 0o444); + const acl = Bun.spawnSync( + ["/bin/chmod", "+a", `${userInfo().username} allow write`, exclude], + { stdin: "ignore", stdout: "ignore", stderr: "ignore" } + ); + expect(acl.exitCode).toBe(0); + // The ACL makes access(W_OK) succeed; only the mode bits refuse it. + await access(exclude, constants.W_OK); await ensureHackGitInfoExclude({ projectDir: join(repo, ".hack") }); expect(await readFile(exclude, "utf8")).toBe("# user rule\n"); expect((await lstat(exclude)).mode & 0o777).toBe(0o444); diff --git a/tests/missing-project-error-code.test.ts b/tests/missing-project-error-code.test.ts index 604c171ff..b5284c8f5 100644 --- a/tests/missing-project-error-code.test.ts +++ b/tests/missing-project-error-code.test.ts @@ -245,11 +245,13 @@ async function emptyProject() { async function cli(opts: { readonly root: string; readonly args: readonly string[]; + /** Working directory; defaults to the fixture root. */ + readonly cwd?: string; }) { const child = Bun.spawn( [process.execPath, resolve("index.ts"), ...opts.args], { - cwd: opts.root, + cwd: opts.cwd ?? opts.root, env: privateCliEnv({ root: opts.root, env: { HACK_NO_INTERACTIVE: "1", HACK_LOGGER: "console" }, @@ -324,11 +326,40 @@ for (const [name, expectation] of Object.entries(COMMANDS)) { ); } +/** + * The commands pinned above in their `--json` form. Without `--json` each + * prints the same closed line (HACK-1426: `up` used to drop the code). + */ +const ENVELOPE_COMMANDS_WITHOUT_JSON = [ + "up", + "down", + "restart", + "ps", + "logs", + "open", +] as const; + +for (const command of ENVELOPE_COMMANDS_WITHOUT_JSON) { + test( + `${command} -p without --json prints one closed line and no stack`, + async () => { + const { root, dir } = await emptyProject(); + const result = await cli({ root, args: [command, "-p", dir] }); + expect(result.code).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe( + `ERROR: E_PROJECT_NOT_FOUND: ${MISSING_PROJECT_MESSAGE}\n` + ); + }, + SPAWN_TIMEOUT_MS + ); +} + test( - "ps -p without --json prints one closed line and no stack", + "up from a working directory without a project prints the closed E_PROJECT_NOT_FOUND line", async () => { const { root, dir } = await emptyProject(); - const result = await cli({ root, args: ["ps", "-p", dir] }); + const result = await cli({ root, cwd: dir, args: ["up"] }); expect(result.code).toBe(1); expect(result.stdout).toBe(""); expect(result.stderr).toBe( diff --git a/tests/no-tty-prompt-sites-cli.test.ts b/tests/no-tty-prompt-sites-cli.test.ts index 8e3df4261..48742691b 100644 --- a/tests/no-tty-prompt-sites-cli.test.ts +++ b/tests/no-tty-prompt-sites-cli.test.ts @@ -33,7 +33,9 @@ import { privateCliEnv } from "./helpers/private-cli-env.ts"; * and `HACK_SESSIONS_MUX=none` (no tmux or zellij is run). `secrets` children * preload a `Bun.secrets` stub, so no case reaches the OS keychain even when a * guard is mutated away. `update` children preload a fetch stub, so nothing - * reaches the network. No Docker socket, launchd or keychain is touched. + * reaches the network. The `ssh --tailscale` child finds a logging + * `tailscale` stand-in first on PATH, never the real CLI. No Docker socket, + * launchd or keychain is touched. */ const REPO_ROOT = resolve(import.meta.dir, ".."); @@ -51,8 +53,17 @@ const ATTACHED_PROMPT_WORKER = join( * shared Bun cache is warm; the regressed prompt loop never exits. */ const KILL_AFTER_MS = 10_000; -/** Overall bound for each test, covering a cold first start. */ +/** Overall bound for each test. */ const TEST_TIMEOUT_MS = 30_000; +/** + * Bound for the one warm-up child in `beforeAll`. Each file run starts with a + * fresh private HOME, so Bun's runtime transpiler cache (kept under HOME) is + * cold, and a cold start of the CLI took 6.3 s on a loaded host (HACK-1426). + * The warm-up pays that cost once, outside every case's {@link KILL_AFTER_MS}. + */ +const WARM_UP_KILL_AFTER_MS = 90_000; +/** `beforeAll` bound: the warm-up child plus margin. */ +const WARM_UP_HOOK_TIMEOUT_MS = WARM_UP_KILL_AFTER_MS + 10_000; /** Clack's active-prompt marker: no case may render a live prompt. */ const ACTIVE_PROMPT_MARKER = "◆"; @@ -67,10 +78,47 @@ type RunResult = { let fileRoot: string | null = null; let caseRoot: string | null = null; -beforeAll(async () => { - // One HOME per file keeps Bun's transpiler cache warm across cases. - fileRoot = await mkdtemp(join(tmpdir(), "hack-no-tty-prompt-sites-")); -}); +beforeAll( + async () => { + // One HOME per file keeps Bun's transpiler cache warm across cases. + fileRoot = await mkdtemp(join(tmpdir(), "hack-no-tty-prompt-sites-")); + await warmTranspilerCache(fileRoot); + }, + { timeout: WARM_UP_HOOK_TIMEOUT_MS } +); + +/** + * Loads the CLI's module graph once under the file's private HOME, so no case + * pays the cold transpile inside its {@link KILL_AFTER_MS} bound. `--help` + * needs no project, prompt, network or keychain. Only a kill fails the file: + * the cases themselves pin every exit code and message. + */ +async function warmTranspilerCache(root: string): Promise { + const proc = Bun.spawn([process.execPath, ENTRYPOINT, "--help"], { + cwd: root, + env: privateCliEnv({ + root, + env: { + HACK_HOME: join(root, "warm-up-hack-home"), + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + HACK_SESSIONS_MUX: "none", + HACK_NO_INTERACTIVE: "1", + NO_COLOR: "1", + }, + }), + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + timeout: WARM_UP_KILL_AFTER_MS, + killSignal: "SIGKILL", + }); + await proc.exited; + if (proc.signalCode !== null) { + throw new Error( + `CLI warm-up was killed (${proc.signalCode}) after ${WARM_UP_KILL_AFTER_MS} ms` + ); + } +} afterAll(async () => { if (fileRoot) { @@ -369,6 +417,45 @@ test( { timeout: TEST_TIMEOUT_MS } ); +test( + "ssh --tailscale does not offer to turn on a stopped Tailscale without a TTY", + async () => { + // A `tailscale` stand-in on PATH reports a stopped backend and logs each + // call, so the test sees whether `tailscale up` would have run. + const bin = join(requireCaseRoot(), "bin"); + await mkdir(bin, { recursive: true }); + const callLog = join(requireCaseRoot(), "tailscale.log"); + const stub = join(bin, "tailscale"); + await writeFile( + stub, + [ + "#!/bin/sh", + `printf '%s\\n' "$*" >> '${callLog}'`, + 'if [ "$1" = status ]; then', + ` printf '%s\\n' '{"BackendState":"Stopped"}'`, + "fi", + "exit 0", + "", + ].join("\n") + ); + await chmod(stub, 0o755); + const result = await runPiped({ + args: ["ssh", "--tailscale"], + env: { PATH: `${bin}:/usr/bin:/bin` }, + }); + expectSettled(result); + expect(result.exitCode).toBe(1); + const output = `${result.stdout}${result.stderr}`; + expect(output).toContain("Tailscale is stopped"); + expect(output).toContain("Run `tailscale up`, then re-run `hack ssh`."); + expect(output).not.toContain("Turn on Tailscale?"); + const calls = (await readFile(callLog, "utf8")).split("\n"); + expect(calls).toContain("status --json"); + expect(calls).not.toContain("up"); + }, + { timeout: TEST_TIMEOUT_MS } +); + test( "ssh --direct host prompt refuses without a TTY and names --host", async () => { diff --git a/tests/project-env-key-ignore.test.ts b/tests/project-env-key-ignore.test.ts index 252807bfe..3c2845a5f 100644 --- a/tests/project-env-key-ignore.test.ts +++ b/tests/project-env-key-ignore.test.ts @@ -17,6 +17,7 @@ import { PROJECT_CONFIG_FILENAME, PROJECT_ENV_KEY_FILENAME, } from "../src/constants.ts"; +import { ensureGitignoreEntry } from "../src/lib/fs.ts"; import { resolveProjectEnvConfigPath } from "../src/lib/project-env-config.ts"; import { restoreEnv } from "./helpers/env.ts"; import { privateCliEnv } from "./helpers/private-cli-env.ts"; @@ -376,6 +377,48 @@ test("a rule that re-includes the key is refused both before creating a key and expect(await readFile(repo.layer, "utf8")).toBe(layerBefore); }, 60_000); +test("an indented entry, which git does not apply, is completed by an exact entry instead of being refused as re-included", async () => { + // HACK-1426: git keeps leading whitespace in a pattern, so this line does + // not ignore the key; Hack must not count it as the entry. + const repo = await createRepo({ git: true }); + const indented = `${BASE_GITIGNORE} ${PROJECT_ENV_KEY_FILENAME}\n`; + await writeFile(repo.gitignore, indented); + + const run = await setSecret(repo, "NEW_SECRET", NEXT_VALUE); + + expect(run.exitCode).toBe(0); + expect(`${run.stdout}${run.stderr}`).not.toContain("re-includes the key"); + expect(await keyExists(repo)).toBe(true); + expectNoKeyMaterial(run, (await readFile(repo.keyPath, "utf8")).trim()); + expect(await readFile(repo.gitignore, "utf8")).toBe( + `${indented}\n# project env key\n${PROJECT_ENV_KEY_FILENAME}\n` + ); + expect(await keyIsCommittable(repo)).toBe(false); +}, 60_000); + +test("ensureGitignoreEntry reads each line as git does: trailing spaces and CR dropped, leading whitespace kept", async () => { + const root = await mkdtemp(join(tmpdir(), "hack-gitignore-entry-")); + tempDirs.add(root); + const path = join(root, ".gitignore"); + const entry = PROJECT_ENV_KEY_FILENAME; + + for (const present of [`${entry} \n`, `${entry}\r\n`, `a\n${entry} \r\n`]) { + await writeFile(path, present); + expect(await ensureGitignoreEntry({ gitignorePath: path, entry })).toEqual({ + changed: false, + }); + expect(await readFile(path, "utf8")).toBe(present); + } + + for (const absent of [` ${entry}\n`, `\t${entry}\n`, `${entry}\t\n`]) { + await writeFile(path, absent); + expect(await ensureGitignoreEntry({ gitignorePath: path, entry })).toEqual({ + changed: true, + }); + expect(await readFile(path, "utf8")).toBe(`${absent}\n${entry}\n`); + } +}); + 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); diff --git a/tests/project-target-inspect.test.ts b/tests/project-target-inspect.test.ts index 953bde0bb..7de241f73 100644 --- a/tests/project-target-inspect.test.ts +++ b/tests/project-target-inspect.test.ts @@ -258,6 +258,12 @@ beforeAll(async () => { }); // Never adopted: .hack/.internal exists only so its mode can be dropped. await mkdir(join(root, "unsearchable", ".hack", ".internal")); + // Never adopted and without .hack/.internal: the most common real layout. + await writeProject({ + repo: join(root, "unsearchable-no-internal"), + name: "unsearchable-no-internal", + services: ["api"], + }); // Native and legacy inputs side by side (a conflicting project family). const nativeConflict = join(root, "native-conflict"); await writeProject({ @@ -709,21 +715,29 @@ describe("paired CLI/MCP target fixtures", () => { test.skipIf(process.getuid?.() === 0)( "an unsearchable .hack or .hack/.internal is a closed refusal without the path", async () => { - const path = join(fixture.root, "unsearchable"); // HACK-1415: every case the proof matrix names runs on both surfaces, - // on a project that never used adoption. - for (const [dir, mode] of [ - [".hack", 0o000], - [".hack", 0o644], - [".hack/.internal", 0o000], - [".hack/.internal", 0o644], + // on a project that never used adoption. HACK-1426: the first case is + // a .hack at mode 000 with no .hack/.internal at all. + for (const [project, dir, mode] of [ + ["unsearchable-no-internal", ".hack", 0o000], + ["unsearchable", ".hack", 0o000], + ["unsearchable", ".hack", 0o644], + ["unsearchable", ".hack/.internal", 0o000], + ["unsearchable", ".hack/.internal", 0o644], ] as const) { + const path = join(fixture.root, project); const locked = join(path, dir); await chmod(locked, mode); try { // An unsearchable project directory is a permission fault (#402's // `permission` selection kind), not unsafe adoption state. - expect({ dir, mode, result: await inspectBoth({ path }) }).toEqual({ + expect({ + project, + dir, + mode, + result: await inspectBoth({ path }), + }).toEqual({ + project, dir, mode, result: targetRefusalEnvelope({