diff --git a/.changeset/help-flag-on-subcommands.md b/.changeset/help-flag-on-subcommands.md new file mode 100644 index 00000000..afaa949a --- /dev/null +++ b/.changeset/help-flag-on-subcommands.md @@ -0,0 +1,18 @@ +--- +"@taskless/cli": patch +--- + +Make `--help` work on every command, instead of running the command. + +`taskless check --help` printed no usage — it ran `check`. So did every other +subcommand: `--help` was parsed as an unrecognized flag and the command body +executed anyway, which meant asking `init` how it works installed skills, and +asking `check` how it works migrated the `.taskless/` scaffold. The only place +help worked was the bare `taskless --help`, whose own output tells you to run +`taskless --help`. + +`--help` and `-h` are now recognized at every depth, including nested commands +(`taskless auth login --help` describes `login`, not `auth`), and a working +directory passed before the command (`taskless -d ./repo check --help`) no +longer confuses which command you asked about. The usage text itself is +unchanged, and nothing else about how commands run has changed. diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index f048e1f1..d6c5b87c 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -4,6 +4,7 @@ import { defineCommand } from "citty"; import { hasValeRules, runEngines } from "../rules/dispatch"; import { assembleEngineConfigs } from "../rules/assemble"; +import { splitRawArguments } from "../util/argv"; import { formatText } from "../util/format"; import { ensureTasklessDirectory } from "../filesystem/directory"; import { listRuleIds, planEngineDispatch } from "../rules/engines"; @@ -66,40 +67,13 @@ async function filterExistingPaths( } /** - * Extract positional arguments from rawArgs. citty's rawArgs contains the - * original argv for this subcommand, so we drop anything starting with `-` - * and drop known flag values (e.g. `-d `). Once `--` is seen, all - * remaining arguments are treated as positional paths (conventional POSIX - * end-of-options marker), which lets users pass paths that begin with `-`. + * Extract positional path arguments from rawArgs. The shared scanner knows the + * global value-taking flags and the POSIX `--` end-of-options marker (which is + * what lets a path beginning with `-` be scanned); `--timeout` is check's own + * value-taking flag, so it is named here rather than in the shared set. */ function extractPositionalPaths(rawArguments: string[]): string[] { - const paths: string[] = []; - let afterDoubleDash = false; - for (let index = 0; index < rawArguments.length; index++) { - const argument = rawArguments[index]!; - if (afterDoubleDash) { - paths.push(argument); - continue; - } - if (argument === "--") { - afterDoubleDash = true; - continue; - } - if (argument.startsWith("-")) { - // Skip value for short/long flags that take a value - if ( - (argument === "-d" || - argument === "--dir" || - argument === "--timeout") && - index + 1 < rawArguments.length - ) { - index += 1; - } - continue; - } - paths.push(argument); - } - return paths; + return splitRawArguments(rawArguments, ["--timeout"]).positionals; } /** A runtime rule that will not run, with why (advisory). */ diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index d780cb68..1f833e62 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -15,6 +15,8 @@ import { shutdownTelemetry, } from "./telemetry"; import { emitRunEvents, resolveCommandName, resolveCwd } from "./telemetry-run"; +import { DIR_FLAGS, hasHelpFlag, splitRawArguments } from "./util/argv"; +import { showResolvedUsage } from "./util/help"; import { CLIError } from "./util/cli-error"; const subCommands = { @@ -64,29 +66,18 @@ const main = defineCommand({ // citty always calls the parent's run handler, even after a subcommand. // Only take action when no positional args (i.e. no subcommand) were // provided. A value following `-d`/`--dir` is a flag value, not a - // positional, so skip it when scanning. - const hasPositional = rawArgs.some((argument, index) => { - if (argument.startsWith("-")) return false; - const previous = rawArgs[index - 1]; - if (previous === "-d" || previous === "--dir") return false; - return true; - }); - if (hasPositional) { + // positional, so splitRawArguments skips it. + const { positionals, flags } = splitRawArguments(rawArgs); + if (positionals.length > 0) { return; } // Only delegate to `init` when the only flags present are ones init - // also understands (`-d` / `--dir`). Help/version/json flags and - // any unknown flags should fall through to citty's default help instead - // of silently launching the wizard. - const onlyInitFlags = rawArgs.every((argument, index) => { - if (!argument.startsWith("-")) { - const previous = rawArgs[index - 1]; - return previous === "-d" || previous === "--dir"; - } - if (argument === "-d" || argument === "--dir") return true; - return false; - }); + // also understands (`-d` / `--dir`). Version/json flags, a bare `--`, and + // any unknown flags should fall through to citty's default help instead of + // silently launching the wizard. (`--help`/`-h` never reach here — they + // are intercepted before dispatch below.) + const onlyInitFlags = flags.every((flag) => DIR_FLAGS.has(flag)); if (!onlyInitFlags) { await showUsage(cmd); return; @@ -122,7 +113,13 @@ const startedAt = Date.now(); const startIdentity = await resolveRunIdentity(runCwd); let thrown: unknown; try { - await runCommand(main, { rawArgs: rawArguments }); + // Help is intercepted here, before dispatch, because citty implements + // `--help` only in runMain — and runMain exits the process on both its help + // and error paths, which would skip the `finally` below and drop the cli_run + // denominator. Rendering here returns normally instead. + await (hasHelpFlag(rawArguments) + ? showResolvedUsage(main, rawArguments) + : runCommand(main, { rawArgs: rawArguments })); } catch (error) { // CLIError = expected failure. Most throw sites (the `fail()` helpers) print // and set exitCode first and mark themselves `reported`; one that does not diff --git a/packages/cli/src/telemetry-run.ts b/packages/cli/src/telemetry-run.ts index 3933522b..58ebf11a 100644 --- a/packages/cli/src/telemetry-run.ts +++ b/packages/cli/src/telemetry-run.ts @@ -1,6 +1,7 @@ import { resolve } from "node:path"; import type { TelemetryClient } from "./telemetry"; +import { splitRawArguments } from "./util/argv"; import { CLIError } from "./util/cli-error"; /** @@ -11,16 +12,7 @@ import { CLIError } from "./util/cli-error"; * command for an agent invocation is just `agent`. */ export function resolveCommandName(rawArguments: string[]): string { - const valueFlags = new Set(["-d", "--dir"]); - const positionals: string[] = []; - for (let index = 0; index < rawArguments.length; index++) { - const argument = rawArguments[index]!; - if (argument.startsWith("-")) { - if (!argument.includes("=") && valueFlags.has(argument)) index++; - continue; - } - positionals.push(argument); - } + const { positionals } = splitRawArguments(rawArguments); if (positionals.length === 0) return "(default)"; const top = positionals[0]!; diff --git a/packages/cli/src/util/argv.ts b/packages/cli/src/util/argv.ts new file mode 100644 index 00000000..879933a3 --- /dev/null +++ b/packages/cli/src/util/argv.ts @@ -0,0 +1,79 @@ +/** + * Raw-argv scanning shared by everything that has to answer "which tokens are + * positionals?" — subcommand resolution for `--help`, the root command's + * no-subcommand check, `check`'s path arguments, and telemetry's command name. + * One copy, because the rules are subtle: + * + * - `-d`/`--dir` take a value, so the token after one of them is a flag value + * and NOT a positional (`taskless -d /tmp check` runs `check`, not `/tmp`). + * - `--` is the POSIX end-of-options marker: every token after it is a + * positional even if it starts with `-`, which is what lets `taskless check + * -- -h` scan a path literally named `-h` instead of asking for help. + */ + +/** Flags every command takes a value for. */ +export const DIR_FLAGS = new Set(["-d", "--dir"]); + +/** The POSIX end-of-options marker. */ +const END_OF_OPTIONS = "--"; + +export interface SplitArguments { + /** + * Tokens that are not flags and not the value of a value-taking flag, plus + * everything after `--`. + */ + positionals: string[]; + /** + * Option tokens, in order, including the `--` marker itself. Values consumed + * by a value-taking flag are not included. + */ + flags: string[]; +} + +/** + * Split raw argv into positionals and flags, skipping flag values and honoring + * `--`. `valueFlags` names flags beyond `-d`/`--dir` that consume the next + * token (e.g. `check`'s `--timeout`). + */ +export function splitRawArguments( + rawArguments: string[], + valueFlags: readonly string[] = [] +): SplitArguments { + const consumesValue = + valueFlags.length > 0 ? new Set([...DIR_FLAGS, ...valueFlags]) : DIR_FLAGS; + const positionals: string[] = []; + const flags: string[] = []; + for (let index = 0; index < rawArguments.length; index++) { + const argument = rawArguments[index]!; + if (argument === END_OF_OPTIONS) { + flags.push(argument); + positionals.push(...rawArguments.slice(index + 1)); + break; + } + if (argument.startsWith("-")) { + flags.push(argument); + // `--dir=` carries its own value; `-d ` eats the next token — + // unless that token is `--`, which ends the options rather than being one. + if ( + consumesValue.has(argument) && + rawArguments[index + 1] !== undefined && + rawArguments[index + 1] !== END_OF_OPTIONS + ) { + index++; + } + continue; + } + positionals.push(argument); + } + return { positionals, flags }; +} + +/** + * True when argv asks for help. Scanned through `splitRawArguments` so neither + * a flag value nor a path after `--` that happens to read like `-h` is mistaken + * for a help request. + */ +export function hasHelpFlag(rawArguments: string[]): boolean { + const { flags } = splitRawArguments(rawArguments); + return flags.includes("--help") || flags.includes("-h"); +} diff --git a/packages/cli/src/util/help.ts b/packages/cli/src/util/help.ts new file mode 100644 index 00000000..4e19cbdc --- /dev/null +++ b/packages/cli/src/util/help.ts @@ -0,0 +1,56 @@ +import { showUsage } from "citty"; +import type { ArgsDef, CommandDef, Resolvable } from "citty"; + +import { splitRawArguments } from "./argv"; + +/** + * citty only implements `--help` inside `runMain`, and `runMain`'s help path + * calls `process.exit(0)` — which skips `finally` blocks, including the one in + * `src/index.ts` that emits the `cli_run` telemetry denominator. So the CLI + * keeps `runCommand` and resolves help itself, here, using citty's own + * exported `showUsage` so no usage text is hand-written. + * + * citty's `resolveSubCommand` is not exported, and its own walk would resolve + * `taskless -d /tmp check --help` to `/tmp` (it takes the first non-`-` token), + * so the walk below uses the shared argv scanner instead. + */ + +/** Resolve a citty `Resolvable` (value, promise, or factory) to its value. */ +async function resolveValue( + value: Resolvable | undefined +): Promise { + return typeof value === "function" + ? await (value as () => T | Promise)() + : await value; +} + +/** + * Render usage for the deepest command the positionals resolve to, with its + * parent (so nested commands like `auth login` print their own usage under + * their own name). Returns normally — never exits — so the caller's telemetry + * `finally` still runs. + */ +export async function showResolvedUsage( + root: CommandDef, + rawArguments: string[] +): Promise { + // Widen once: the walk descends into subcommands, each with its own args + // shape, and only the rendering (which reads meta and args generically) is + // done with the result. + let command = root as unknown as CommandDef; + let parent: CommandDef | undefined; + + for (const name of splitRawArguments(rawArguments).positionals) { + const subCommands = await resolveValue(command.subCommands); + const child = subCommands + ? await resolveValue(subCommands[name]) + : undefined; + // The first token that is not a child of the current command ends the + // walk (it is a positional argument, or a typo) — render what resolved. + if (!child) break; + parent = command; + command = child; + } + + await showUsage(command, parent); +} diff --git a/packages/cli/test/help-flag.test.ts b/packages/cli/test/help-flag.test.ts new file mode 100644 index 00000000..53bb64ec --- /dev/null +++ b/packages/cli/test/help-flag.test.ts @@ -0,0 +1,194 @@ +import { execFile } from "node:child_process"; +import { mkdtemp, readdir, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { hasHelpFlag, splitRawArguments } from "../src/util/argv"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +describe("--help below the root", () => { + let temporaryDirectory: string; + + beforeEach(async () => { + temporaryDirectory = await mkdtemp(join(tmpdir(), "taskless-help-")); + }); + + afterEach(async () => { + await rm(temporaryDirectory, { recursive: true, force: true }); + }); + + // The defect was that `--help` fell through as an unknown flag and the + // command body ran. Asserting only on output would pass against the broken + // CLI, so this asserts `init` did not install anything. + it.each(["--help", "-h"])( + "%s on a leaf subcommand prints usage and runs nothing", + async (flag) => { + const { stdout } = await execFileAsync("node", [ + binPath, + "init", + flag, + "-d", + temporaryDirectory, + ]); + + expect(stdout).toContain("USAGE"); + expect(stdout).toContain("taskless init"); + // A real `init` writes a skills directory here (see cli.test.ts). + expect(await readdir(temporaryDirectory)).toEqual([]); + } + ); + + it("renders the nested subcommand's usage, not its parent's", async () => { + const { stdout } = await execFileAsync("node", [ + binPath, + "auth", + "login", + "--help", + ]); + + expect(stdout).toContain("auth login"); + expect(stdout).toContain("Authenticate with taskless.io"); + // `auth`'s own usage lists its subcommands; login's does not. + expect(stdout).not.toContain("Remove saved authentication"); + }); + + it("treats the token after -d as a flag value, not a subcommand", async () => { + const { stdout } = await execFileAsync("node", [ + binPath, + "-d", + temporaryDirectory, + "check", + "--help", + ]); + + expect(stdout).toContain("taskless check"); + expect(stdout).toContain("Run Taskless rules against your codebase"); + // Falling back to the root would print the full command list. + expect(stdout).not.toContain("Manage authentication with taskless.io"); + expect(await readdir(temporaryDirectory)).toEqual([]); + }); + + // `--` is the escape hatch `check` supports so a path beginning with `-` can + // still be scanned. Intercepting help before dispatch must not eat it. + it("does not read a path after -- as a help request", async () => { + const { stdout } = await execFileAsync("node", [ + binPath, + "check", + "-d", + temporaryDirectory, + "--json", + "--", + "-h", + ]); + + expect(stdout).not.toContain("USAGE"); + const parsed = JSON.parse(stdout.trim()) as { success: boolean }; + expect(parsed.success).toBe(true); + }); + + it("leaves the root's --help unchanged", async () => { + const { stdout } = await execFileAsync("node", [binPath, "--help"]); + + expect(stdout).toContain("Taskless CLI"); + expect(stdout).toContain("USAGE"); + expect(stdout).toContain("COMMANDS"); + expect(stdout).toContain("Manage authentication with taskless.io"); + }); +}); + +describe("splitRawArguments", () => { + it("treats the token after -d/--dir as a value, not a positional", () => { + expect(splitRawArguments(["-d", "/tmp", "check"])).toEqual({ + positionals: ["check"], + flags: ["-d"], + }); + }); + + it("treats everything after -- as a positional", () => { + expect(splitRawArguments(["check", "--", "-h", "--json"])).toEqual({ + positionals: ["check", "-h", "--json"], + flags: ["--"], + }); + }); + + it("does not swallow -- as the value of -d", () => { + expect(splitRawArguments(["-d", "--", "src"])).toEqual({ + positionals: ["src"], + flags: ["-d", "--"], + }); + }); + + it("accepts command-specific value flags", () => { + expect( + splitRawArguments(["check", "--timeout", "5", "src"], ["--timeout"]) + .positionals + ).toEqual(["check", "src"]); + }); +}); + +describe("hasHelpFlag", () => { + it.each([ + [["check", "--help"], true], + [["auth", "login", "-h"], true], + [["-d", "/tmp", "check", "--help"], true], + [["check"], false], + // A path after `--`, and a directory that reads like a flag, are values. + [["check", "--", "-h"], false], + [["-d", "-h", "check"], false], + ])("resolves %j to %s", (argv, expected) => { + expect(hasHelpFlag(argv)).toBe(expected); + }); +}); + +// REGRESSION GUARD — DO NOT "SIMPLIFY" THE HELP PATH TO citty's runMain. +// +// runMain's help branch calls process.exit(0), which does not run `finally` +// blocks. The entry's finally block is what emits cli_run, the per-invocation +// telemetry denominator. Swapping in runMain would silently stop reporting +// every help (and every failed) invocation. This test fails in that world: +// either cli_run is never captured, or process.exit tears down the worker. +describe("help invocations still emit cli_run", () => { + const capture = vi.fn(); + + beforeEach(() => { + capture.mockClear(); + vi.resetModules(); + vi.doMock("../src/telemetry", () => ({ + getTelemetry: () => + Promise.resolve({ capture, shutdown: () => Promise.resolve() }), + resolveRunIdentity: () => + Promise.resolve({ anonymous: true, loggedIn: false }), + shutdownTelemetry: () => Promise.resolve(), + })); + }); + + afterEach(() => { + vi.doUnmock("../src/telemetry"); + vi.resetModules(); + }); + + it("captures cli_run for `info --help`", async () => { + const argv = process.argv; + const write = vi + .spyOn(process.stdout, "write") + .mockImplementation(() => true); + process.argv = ["node", "taskless", "info", "--help"]; + + try { + // Importing the entry runs the CLI: it is a top-level script. + await import("../src/index"); + } finally { + process.argv = argv; + write.mockRestore(); + } + + expect(capture).toHaveBeenCalledWith( + "cli_run", + expect.objectContaining({ command: "info", success: true }) + ); + }); +});