diff --git a/docs/cli.md b/docs/cli.md index 64dbd24b4..13bdee6f5 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -129,6 +129,9 @@ Historical remote guides are excluded from the supported documentation. - `--json` on `hack up`/`down`/`restart`/`doctor` emits a `{ok, data | error: {code, message}}` envelope with stable `E_*` error codes. `--json` on `hack up` implies `--detach`. +- A `--path` (or working directory) without a `.hack/` project reports `E_PROJECT_NOT_FOUND` + from every project command, for example `hack ps -p --json` and `hack up -p --json`. + Without `--json` it prints one `E_PROJECT_NOT_FOUND` line and no stack trace. - Known Hack errors and closed native runtime refusals print their message only, never a stack trace; set `HACK_DEBUG=1` to add the stack. Unexpected exceptions always print their stack. - `--no-interactive` (or `HACK_NO_INTERACTIVE=1`) is a global flag: commands never prompt — they @@ -199,6 +202,26 @@ rather than printing Hack's version, and a child `--json` does not switch Hack's to JSON. A `--` before the child command is optional and is not passed on; a later `--` (as in `hack run web npm test -- --watch`) reaches the child unchanged. +### Option parsing + +Hack parses options with the resolved command's own options plus the global ones, so each command +keeps its own short flags and option types. `-p` is `--path` for project commands (`init`, `up`, +`down`, `ps`, `logs`, `env ...`, `config ...`, `setup ...`) and `--port` for `hack ssh`. An option +that only another command declares is refused as `Option(s) not valid for ""`. + +An option placed before the command name, as in `hack -p up`, is read as that command reads +it. There is one remaining edge: when a spelling placed before the command name is a flag in one +command and takes a value in another, Hack reads it as a flag, so it never consumes the command +name. No spelling in the current CLI has this conflict. Put options after the command name to +avoid it. + +Per-command parsing replaced one parse over every command's options. Comparing the old and new +parsers on edge cases found one difference, the intended one: `-p ` on a project command (for +example `up -p /x` or `--no-interactive up -p /x`) now sets `--path`. The old parser set `ssh`'s +`--port`, which the command then refused. These cases parse the same as before: `--help --all`, +`help --all`, `-- up`, `--json run web tool --json`, `--path /x up`, `logs --tail`, `up --tail`, +`ssh --path /x` and `x ext cmd -p 1` (where `-p 1` belongs to the extension command). + `session exec` takes the workspace command as one argument, so quote a command that has its own arguments or flags: `hack session exec ws "npm --version"`. Hack options go before it, and an unquoted `hack session exec ws npm --version` refuses with a hint to quote the command instead diff --git a/docs/guides/native-candidate.md b/docs/guides/native-candidate.md index 17cc8a96d..87d2e2f43 100644 --- a/docs/guides/native-candidate.md +++ b/docs/guides/native-candidate.md @@ -868,8 +868,10 @@ confirmed. There is no automatic command replay or fallback to Compose. `run` uses the running project's environment and profiles. `--env` and `--profile` are optional; when given they must equal the selection recorded at startup (profiles compare as a set). A different selection, or a legacy mapping without -recorded selectors, refuses with `E_USAGE` before any runtime request. `run` never -restarts the project to change its selection. +recorded selectors, refuses with `E_USAGE` before any runtime request. An empty +value (`--env ""`, `--profile ""`, or one that is only spaces or commas) also +refuses with `E_USAGE`: it does not mean "inherit" or an empty selection. Omit +the option to inherit. `run` never restarts the project to change its selection. Services with no managed environment values also support `run`; they keep the admitted image's environment without creating a private environment mount or launcher. The one-off request still carries an explicit empty, identity-bound diff --git a/src/backends/native-project-one-off.ts b/src/backends/native-project-one-off.ts index 7116b50ed..cd0bdcc9a 100644 --- a/src/backends/native-project-one-off.ts +++ b/src/backends/native-project-one-off.ts @@ -35,6 +35,32 @@ function decode(value: unknown): Buffer { export const NATIVE_RUN_SELECTION_REFUSAL = "Native `hack run` uses the running project's environment and profiles, and the requested --env or --profile does not match them. Omit --env and --profile, or pass exactly the selection `hack up` started with. No command ran."; +/** The closed `E_USAGE` refusal of an empty native `run --env` or `--profile`. */ +export const NATIVE_RUN_EMPTY_SELECTION_REFUSAL = + "Native `hack run` does not accept an empty --env or --profile value. Omit the option to use the running selection, or pass the env or profiles `hack up` started with. No command ran."; + +/** + * Refuse an empty native `run --env` or `--profile` value before it is + * resolved. Elsewhere an empty `--env` means base layers and an empty + * `--profile` means no profiles, but a native run never changes the running + * selection, so an empty value could only silently mean "inherit" or an + * explicit empty selection. Whitespace and comma-only values count as empty. + */ +export function requireNonEmptyNativeRunOptions(opts: { + readonly env: string | undefined; + readonly profile: string | undefined; +}): void { + const emptyEnv = opts.env?.trim() === ""; + const emptyProfile = + opts.profile?.split(",").every((part) => part.trim() === "") === true; + if (emptyEnv || emptyProfile) { + throw new HackCliError({ + code: "E_USAGE", + message: NATIVE_RUN_EMPTY_SELECTION_REFUSAL, + }); + } +} + /** * The selection a native one-off asks for. An omitted field inherits the * running selection; a given field must equal it exactly. Profiles compare as @@ -56,10 +82,11 @@ export function requireNativeRunSelection(opts: { readonly selection?: NativeRunSelection; }): void { const { run, selection } = opts; + // A legacy mapping without a recorded env has `effectiveEnvName` undefined, + // which never equals a given selection (a string or the base `null`). const envDiffers = selection?.envName !== undefined && - (run.effectiveEnvName === undefined || - selection.envName !== run.effectiveEnvName); + selection.envName !== run.effectiveEnvName; const profilesDiffer = selection?.profiles !== undefined && (run.profiles === undefined || diff --git a/src/commands/project.ts b/src/commands/project.ts index cf1aaf27d..4e28732fd 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -54,6 +54,7 @@ import { import { type NativeRunSelection, nativeProjectOneOff, + requireNonEmptyNativeRunOptions, } from "../backends/native-project-one-off.ts"; import { retireNativeRecoveredPublisher, @@ -4817,11 +4818,18 @@ async function requireProjectContext(startDir: string) { return ctx; } -class MissingProjectContextError extends Error { +/** + * No project at the selected path. It carries `E_PROJECT_NOT_FOUND`, the code + * `up`, `down` and `restart` already reported, so every project command that + * resolves its project here (`ps`, `logs`, `run`, ...) reports the same closed + * code under `--json` and one closed line without it. + */ +class MissingProjectContextError extends HackCliError { constructor() { - super( - `No ${HACK_PROJECT_DIR_PRIMARY}/ (or legacy .dev/) found. Run: hack init` - ); + super({ + code: "E_PROJECT_NOT_FOUND", + message: `No ${HACK_PROJECT_DIR_PRIMARY}/ (or legacy .dev/) found. Run: hack init`, + }); this.name = "MissingProjectContextError"; } } @@ -7472,8 +7480,8 @@ async function handlePs({ /** * Compose runs need the Compose operation; a native one-off instead carries its * `--env`/`--profile` request. Omitted options inherit the running selection; - * given options must match it exactly, which the native backend checks before - * any runtime request (no implicit restart). + * an empty value is refused; given options must match it exactly, which the + * native backend checks before any runtime request (no implicit restart). */ function runBackendSelection( native: boolean, @@ -7483,6 +7491,10 @@ function runBackendSelection( requireComposeOperationAvailable("run"); return; } + requireNonEmptyNativeRunOptions({ + env: args.options.env, + profile: args.options.profile, + }); return { envName: resolveRequestedEnvName({ envOption: args.options.env }), profiles: diff --git a/tests/cli-closed-error-output.test.ts b/tests/cli-closed-error-output.test.ts index 34a7fcce3..9850d98d0 100644 --- a/tests/cli-closed-error-output.test.ts +++ b/tests/cli-closed-error-output.test.ts @@ -4,6 +4,10 @@ import { checkNativeHttpsPort } from "../src/backends/native-https-port.ts"; import { NativeHttpsStartupError } from "../src/backends/native-https-startup-failure.ts"; import { nativeHttpsVerificationError } from "../src/backends/native-project-https.ts"; import { NativePartialShutdownError } from "../src/backends/native-project-interrupted-cleanup.ts"; +import { + NATIVE_RUN_SELECTION_REFUSAL, + requireNativeRunSelection, +} from "../src/backends/native-project-one-off.ts"; import { NativeRuntimeRequestError } from "../src/backends/native-runtime-client.ts"; import { CliUsageError } from "../src/cli/command.ts"; import { handleRunCliError } from "../src/cli/run.ts"; @@ -167,9 +171,8 @@ test("--json keeps one envelope for a closed refusal", async () => { }); test("a handler's usage refusal prints its message and one hint line, not the help", async () => { - const error = new CliUsageError( - "Native run uses the started project's environment and profiles; restart with the requested selections first." - ); + // The `run` handler's own refusal of a missing service positional. + const error = new CliUsageError("Missing required argument: service"); expect( await handleRunCliError({ error, @@ -182,6 +185,34 @@ test("a handler's usage refusal prints its message and one hint line, not the he ); }); +test("the native run selection refusal prints one coded closed line and no hint", async () => { + // The current refusal (#368) replaced the removed "restart with the + // requested selections first" usage error; it is a coded E_USAGE error. + const error = thrownBy(() => + requireNativeRunSelection({ + run: { + run: "a".repeat(32), + owner: "b".repeat(32), + namespace: "c".repeat(64), + planId: "d".repeat(64), + profiles: ["toolchain"], + effectiveEnvName: null, + aws: null, + }, + selection: { profiles: ["web"] }, + }) + ); + expect(error).toBeInstanceOf(HackCliError); + expect( + await handleRunCliError({ + error, + jsonRequested: false, + handlerPath: ["run"], + }) + ).toBe(1); + expect(output).toBe(`ERROR: E_USAGE: ${NATIVE_RUN_SELECTION_REFUSAL}\n`); +}); + test("a usage error found while parsing still prints the help", async () => { const error = new CliUsageError("Unknown option(s): --bogus"); expect(await handleRunCliError({ error, jsonRequested: false })).toBe(1); @@ -190,7 +221,9 @@ test("a usage error found while parsing still prints the help", async () => { }); test("a handler's usage refusal under --json stays one E_USAGE envelope", async () => { - const error = new CliUsageError("Native exec does not support --env."); + const error = new CliUsageError( + "Native exec does not support --env or --profile changes; it uses the running service environment." + ); expect( await handleRunCliError({ error, diff --git a/tests/cli-per-command-options.test.ts b/tests/cli-per-command-options.test.ts index 008944a79..a005dbc8b 100644 --- a/tests/cli-per-command-options.test.ts +++ b/tests/cli-per-command-options.test.ts @@ -215,6 +215,60 @@ test("the child-command boundary uses the resolved command's option types", () = expect(findPassthroughStart(COLLIDING_CLI, ["gamma", "-r", "ls"])).toBe(2); }); +test("a lone - is a positional, so it can start the child command", () => { + // `-` is never an option: here it is gamma's child command, and the `-r` + // after it belongs to the child. + expect(findPassthroughStart(COLLIDING_CLI, ["gamma", "-", "-r"])).toBe(1); + expect(parseCliArgv(COLLIDING_CLI, ["gamma", "-", "-r"]).positionals).toEqual( + ["gamma", "-", "-r"] + ); +}); + +/** + * A passthrough command that also has a subcommand, so a subcommand name can + * appear after the command's own positional. + */ +const SUBCOMMAND_CLI = defineCli({ + name: "demo", + version: "0.0.0", + summary: "A passthrough command with a subcommand", + globalOptions: [], + commands: [ + defineCommand({ + name: "delta", + summary: "service then child command", + group: "Project", + options: [optFlag], + positionals: [ + { name: "service" }, + { name: "command", multiple: true, passthrough: true }, + ], + subcommands: [ + defineCommand({ + name: "ls", + summary: "a subcommand of delta", + group: "Project", + options: [], + positionals: [], + subcommands: [], + } as const), + ], + } as const), + ], +} as const); + +test("after a command's own positional, a subcommand name is the child command", () => { + // `ls` names a subcommand of delta only before delta's first positional; + // after `web` it is the child command, and its `-r` belongs to the child. + expect( + findPassthroughStart(SUBCOMMAND_CLI, ["delta", "web", "ls", "-r"]) + ).toBe(2); + // Before any positional, `ls` still resolves as the subcommand. + expect( + findPassthroughStart(SUBCOMMAND_CLI, ["delta", "ls", "-r"]) + ).toBeNull(); +}); + const roots: string[] = []; afterEach(async () => { for (const root of roots.splice(0)) { diff --git a/tests/missing-project-error-code.test.ts b/tests/missing-project-error-code.test.ts new file mode 100644 index 000000000..cc03f8684 --- /dev/null +++ b/tests/missing-project-error-code.test.ts @@ -0,0 +1,90 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdir, mkdtemp, realpath, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +/** + * HACK-1339: every project command reports a path without a project with the + * same closed code. `up`, `down` and `restart` already reported + * E_PROJECT_NOT_FOUND; `ps` and `logs` reported E_UNEXPECTED (and, without + * `--json`, a stack trace). + */ + +const MESSAGE = "No .hack/ (or legacy .dev/) found. Run: hack init"; +/** A JavaScript stack frame as Bun prints it. */ +const STACK_FRAME = /^\s+at /m; +/** A source CLI spawn can take several seconds on a loaded runner. */ +const SPAWN_TIMEOUT_MS = 30_000; + +const roots: string[] = []; +afterEach(async () => { + for (const root of roots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } +}); + +async function emptyProject() { + const root = await realpath( + await mkdtemp(join(tmpdir(), "hack-missing-project-")) + ); + roots.push(root); + const dir = join(root, "empty"); + await mkdir(dir); + return { root, dir }; +} + +async function cli(opts: { + readonly root: string; + readonly args: readonly string[]; +}) { + const child = Bun.spawn( + [process.execPath, resolve("index.ts"), ...opts.args], + { + cwd: opts.root, + env: privateCliEnv({ + root: opts.root, + env: { HACK_NO_INTERACTIVE: "1", HACK_LOGGER: "console" }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + timeout: 20_000, + } + ); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { stdout, stderr, code }; +} + +for (const command of ["ps", "logs", "up", "down", "restart"]) { + test( + `${command} -p --json reports E_PROJECT_NOT_FOUND`, + async () => { + const { root, dir } = await emptyProject(); + const result = await cli({ root, args: [command, "-p", dir, "--json"] }); + expect(result.code).toBe(1); + expect(JSON.parse(result.stdout)).toEqual({ + ok: false, + error: { code: "E_PROJECT_NOT_FOUND", message: MESSAGE }, + }); + }, + SPAWN_TIMEOUT_MS + ); +} + +test( + "ps -p without --json prints one closed line and no stack", + async () => { + const { root, dir } = await emptyProject(); + const result = await cli({ root, args: ["ps", "-p", dir] }); + expect(result.code).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe(`ERROR: E_PROJECT_NOT_FOUND: ${MESSAGE}\n`); + expect(result.stderr).not.toMatch(STACK_FRAME); + }, + SPAWN_TIMEOUT_MS +); diff --git a/tests/native-run-selection-cli.test.ts b/tests/native-run-selection-cli.test.ts index 7c7716692..362ce4fa3 100644 --- a/tests/native-run-selection-cli.test.ts +++ b/tests/native-run-selection-cli.test.ts @@ -9,7 +9,10 @@ import { } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; -import { NATIVE_RUN_SELECTION_REFUSAL } from "../src/backends/native-project-one-off.ts"; +import { + NATIVE_RUN_EMPTY_SELECTION_REFUSAL, + NATIVE_RUN_SELECTION_REFUSAL, +} from "../src/backends/native-project-one-off.ts"; import { saveNativeProjectRun } from "../src/backends/native-project-run.ts"; import { privateCliEnv, privateCliHome } from "./helpers/private-cli-env.ts"; @@ -259,3 +262,43 @@ for (const request of [ expect(await nativeCalls(f.root)).toEqual([]); }, 30_000); } + +/** + * HACK-1338: an empty `--env` or `--profile` is a closed usage error, never a + * silent "inherit" or an explicit empty selection. The fixture runs with no + * profiles and the base env, so without this rule `--env ""` (base) and + * `--profile ""` (no profiles) would match the running selection and run. + */ +for (const request of [ + { name: "an empty --env", args: ["--env", ""] }, + { name: "an empty --env=", args: ["--env="] }, + { name: "a whitespace --env", args: ["--env", " "] }, + { name: "an empty --profile", args: ["--profile", ""] }, + { name: "a comma-only --profile", args: ["--profile", " , "] }, +]) { + test(`run with ${request.name} refuses with one closed line and no runtime request`, async () => { + const f = await fixture({ profiles: [], effectiveEnvName: null }); + const result = await cli({ + fixture: f, + args: ["run", ...request.args, "toolchain", "bun", "--version"], + }); + expect(result.code).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr.trim()).toBe( + `ERROR: E_USAGE: ${NATIVE_RUN_EMPTY_SELECTION_REFUSAL}` + ); + expect(result.stderr).not.toMatch(STACK_FRAME); + expect(await nativeCalls(f.root)).toEqual([]); + }, 30_000); +} + +test("run without --env or --profile still inherits the running selection", async () => { + const f = await fixture({ profiles: [], effectiveEnvName: null }); + const result = await cli({ + fixture: f, + args: ["run", "toolchain", "bun", "--version"], + }); + expect(result.code).toBe(0); + expect(result.stdout).toBe(CHILD_STDOUT); + expect(operations(await nativeCalls(f.root))).toContain("run-service"); +}, 30_000);