fix: route ask/docs/diff options past root-command flag collisions (#52)

The root command defines -b/--branch, --model, --format, --full-clone, --keep-temp, etc. Because the root has options + an action alongside subcommands, Commander captures those flags at the root before the subcommand parses them, so several subcommand options were silently ignored:

- ask: --branch, --model
- docs: --branch
- diff: --format, --full-clone, --keep-temp

Extract pure, tested getFlagValue/hasFlag helpers in src/utils.ts and read the affected options from raw argv (the approach diff already used for --output). Removed the masking default on diff --format so the fallback can take effect (runPullRequestDiff already defaults to markdown).

Tests: getFlagValue/hasFlag unit tests; an in-process cli routing test that mocks the command runners and asserts values reach them; a real-CLI diff e2e asserting --format html and --keep-temp take effect.

Co-authored-by: Arthur742Ramos <223556219+Copilot@users.noreply.github.com>

Author: Arthur742Ramos

CHANGELOG.md

@@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `bootcamp ask`, `docs`, and `diff` now honor options whose flag names collide with the root command. `ask --branch/--model`, `docs --branch`, and `diff --format/--full-clone/--keep-temp` were captured by the root command and silently ignored; they are now read from raw argv (via shared, tested `getFlagValue`/`hasFlag` helpers in `src/utils.ts`).
 - `bootcamp health` now honors `--branch` and `--max-files`. These short flags (`-b`/`-m`) collide with the root command's options, which captured them before the subcommand could; the command now falls back to reading raw argv (matching the `diff --output` approach). The `--json` output also gained a `filesScanned` field.
 - Reused shared prompt helper builders in `src/agent.ts` for standard/fast prompt construction.
 - Preserved caught error causes in `src/watch.ts` non-fast-forward rethrows.

src/cli.ts

@@ -35,6 +35,7 @@ import { resolveOutputFormat } from "./services/config-resolution.js";
 import type { OutputFormat } from "./formatter.js";
 import type { BootcampOptions, StylePack } from "./types.js";
 import { startServer } from "./web/server.js";
+import { getFlagValue, hasFlag } from "./utils.js";
 import pkg from "../package.json" with { type: "json" };
 
 const VERSION = pkg.version;
@@ -149,22 +150,11 @@ function getActionOptions<T extends Record<string, unknown>>(opts: Command | T):
 }
 
 function getCliFlagValue(flags: string[]): string | undefined {
-  const args = process.argv.slice(2);
-
-  for (let index = 0; index < args.length; index++) {
-    const arg = args[index];
-    if (flags.includes(arg)) {
-      return args[index + 1];
-    }
-
-    for (const flag of flags) {
-      if (arg.startsWith(`${flag}=`)) {
-        return arg.slice(flag.length + 1);
-      }
-    }
-  }
+  return getFlagValue(process.argv.slice(2), flags);
+}
 
-  return undefined;
+function hasCliFlag(flags: string[]): boolean {
+  return hasFlag(process.argv.slice(2), flags);
 }
 
 function isNegativeOptionEnabled(
@@ -290,8 +280,8 @@ program
   .action(async (repoUrl: string, rawOpts) => {
     const opts = getActionOptions<AskActionOptions>(rawOpts as Command | AskActionOptions);
     await runAskCommand(repoUrl, {
-      branch: opts.branch,
-      model: opts.model,
+      branch: opts.branch || getCliFlagValue(["--branch", "-b"]),
+      model: opts.model || getCliFlagValue(["--model"]),
       noClone: isNegativeOptionEnabled(opts, "noClone", "clone"),
       verbose: opts.verbose,
     });
@@ -301,18 +291,18 @@ program
   .command("diff <repo-pr>")
   .description("Generate onboarding diff for a GitHub PR")
   .option("-o, --output <dir>", "Output directory")
-  .option("--format <format>", "Output format: markdown, html, pdf", "markdown")
+  .option("--format <format>", "Output format: markdown, html, pdf")
   .option("--full-clone", "Perform a full clone instead of shallow clone (slower but includes full history)")
   .option("--keep-temp", "Keep temporary clone directory")
   .option("-v, --verbose", "Show detailed output")
   .action(async (repoPr: string, rawOpts) => {
     const opts = getActionOptions<DiffActionOptions>(rawOpts as Command | DiffActionOptions);
     await runPullRequestDiff(repoPr, {
       output: opts.output || getCliFlagValue(["--output", "-o"]),
-      format: opts.format,
-      fullClone: opts.fullClone || false,
-      keepTemp: opts.keepTemp || false,
-      verbose: opts.verbose || false,
+      format: opts.format || getCliFlagValue(["--format"]),
+      fullClone: opts.fullClone || hasCliFlag(["--full-clone"]),
+      keepTemp: opts.keepTemp || hasCliFlag(["--keep-temp"]),
+      verbose: opts.verbose || hasCliFlag(["--verbose", "-v"]),
     });
   });
 
@@ -335,7 +325,12 @@ program
   .option("-v, --verbose", "Show detailed output")
   .action(async (repoUrl: string, rawOpts) => {
     const opts = getActionOptions<DocsActionOptions>(rawOpts as Command | DocsActionOptions);
-    await runDocsCommand(repoUrl, opts);
+    await runDocsCommand(repoUrl, {
+      check: opts.check,
+      fix: opts.fix,
+      branch: opts.branch || getCliFlagValue(["--branch", "-b"]),
+      verbose: opts.verbose || hasCliFlag(["--verbose", "-v"]),
+    });
   });
 
 program

src/utils.ts

@@ -49,6 +49,39 @@ export function escapeRegex(str: string): string {
   return str.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
 }
 
+/**
+ * Read the value of a CLI flag directly from a raw argument list.
+ *
+ * Supports both `--flag value` and `--flag=value` forms. This exists to work
+ * around Commander routing options whose flag names collide with the root
+ * command (e.g. `--branch`, `--max-files`, `--style`) to the root rather than
+ * the subcommand. Pure (takes `args`) so it is easy to test.
+ */
+export function getFlagValue(args: string[], flags: string[]): string | undefined {
+  for (let index = 0; index < args.length; index++) {
+    const arg = args[index];
+    if (flags.includes(arg)) {
+      return args[index + 1];
+    }
+    for (const flag of flags) {
+      if (arg.startsWith(`${flag}=`)) {
+        return arg.slice(flag.length + 1);
+      }
+    }
+  }
+  return undefined;
+}
+
+/**
+ * Whether any of the given boolean flags is present in a raw argument list.
+ *
+ * Companion to {@link getFlagValue} for boolean options (e.g. `--full-clone`)
+ * that collide with root-command flags. Pure (takes `args`) for testability.
+ */
+export function hasFlag(args: string[], flags: string[]): boolean {
+  return args.some((arg) => flags.includes(arg) || flags.some((flag) => arg.startsWith(`${flag}=`)));
+}
+
 /**
  * Convert a filesystem path to POSIX form (forward slashes).
  *

test/cli-option-routing.test.ts

@@ -0,0 +1,69 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+
+const runAskCommand = vi.fn().mockResolvedValue(undefined);
+const runDocsCommand = vi.fn().mockResolvedValue(undefined);
+const runPullRequestDiff = vi.fn().mockResolvedValue(undefined);
+
+vi.mock("../src/commands/ask-command.js", () => ({
+  runAskCommand: (...args: any[]) => runAskCommand(...args),
+}));
+vi.mock("../src/commands/docs-command.js", () => ({
+  runDocsCommand: (...args: any[]) => runDocsCommand(...args),
+}));
+vi.mock("../src/commands/diff-command.js", () => ({
+  runPullRequestDiff: (...args: any[]) => runPullRequestDiff(...args),
+}));
+
+import { program } from "../src/cli.js";
+
+async function runArgv(argv: string[]): Promise<void> {
+  const saved = process.argv;
+  process.argv = ["node", "cli", ...argv];
+  try {
+    await program.parseAsync(process.argv);
+  } finally {
+    process.argv = saved;
+  }
+}
+
+describe("CLI option routing past root-command flag collisions", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it("routes ask --branch and --model to the ask command", async () => {
+    await runArgv(["ask", "./repo", "--branch", "dev", "--model", "m1"]);
+    expect(runAskCommand).toHaveBeenCalledTimes(1);
+    const [repo, opts] = runAskCommand.mock.calls[0];
+    expect(repo).toBe("./repo");
+    expect(opts.branch).toBe("dev");
+    expect(opts.model).toBe("m1");
+  });
+
+  it("routes docs --branch to the docs command", async () => {
+    await runArgv(["docs", "./repo", "--branch", "release", "--check"]);
+    expect(runDocsCommand).toHaveBeenCalledTimes(1);
+    const [repo, opts] = runDocsCommand.mock.calls[0];
+    expect(repo).toBe("./repo");
+    expect(opts.branch).toBe("release");
+    expect(opts.check).toBe(true);
+  });
+
+  it("routes diff --format, --full-clone, and --keep-temp to the diff command", async () => {
+    await runArgv(["diff", "owner/repo#1", "--format", "html", "--full-clone", "--keep-temp"]);
+    expect(runPullRequestDiff).toHaveBeenCalledTimes(1);
+    const [target, opts] = runPullRequestDiff.mock.calls[0];
+    expect(target).toBe("owner/repo#1");
+    expect(opts.format).toBe("html");
+    expect(opts.fullClone).toBe(true);
+    expect(opts.keepTemp).toBe(true);
+  });
+
+  it("leaves options unset when not provided", async () => {
+    await runArgv(["diff", "owner/repo#2"]);
+    const [, opts] = runPullRequestDiff.mock.calls[0];
+    expect(opts.format).toBeUndefined();
+    expect(opts.fullClone).toBe(false);
+    expect(opts.keepTemp).toBe(false);
+  });
+});

test/e2e/diff-command.e2e.test.ts

@@ -214,4 +214,46 @@ describe("diff command", () => {
     expect(diffDoc).toContain("Major version bump: 1.0.0 → 2.0.0");
     expect(diffDoc).toContain("Removed export: greet in src/index.ts");
   }, 90_000);
+
+  it("honors --format and --keep-temp routed past root-option collisions", async () => {
+    const tempDir = await mkdtemp(join(tmpdir(), "bootcamp-diff-e2e-"));
+    tempDirs.push(tempDir);
+
+    const { bareRepoPath, baseSha, headSha } = await createDiffFixture(tempDir);
+    const { server, apiBaseUrl } = await startPullRequestApiServer(baseSha, headSha);
+    servers.push(server);
+
+    const outputDir = join(tempDir, "diff-output-html");
+    const bareRepoUrl = pathToFileURL(bareRepoPath).href;
+    const result = await runCli(
+      [
+        "diff",
+        `${FIXTURE_OWNER}/${FIXTURE_REPO}#${PR_NUMBER}`,
+        "--output",
+        outputDir,
+        "--format",
+        "html",
+        "--keep-temp",
+      ],
+      {
+        GIT_CONFIG_COUNT: "1",
+        GIT_CONFIG_KEY_0: `url.${bareRepoUrl}.insteadOf`,
+        GIT_CONFIG_VALUE_0: `https://github.com/${FIXTURE_OWNER}/${FIXTURE_REPO}.git`,
+        REPO_BOOTCAMP_GITHUB_API_BASE_URL: apiBaseUrl,
+      },
+      60_000,
+      tempDir
+    );
+
+    expect(result.exitCode).toBe(0);
+    const plainStdout = stripAnsi(result.stdout);
+    // --format routed correctly -> HTML output file.
+    expect(plainStdout).toMatch(/File:\s+\S+\.html/);
+    // --keep-temp routed correctly -> clone retained.
+    expect(plainStdout).toContain("Temporary clone kept at");
+
+    const htmlDoc = await readFile(join(outputDir, "DIFF.html"), "utf-8");
+    expect(htmlDoc).toContain("<");
+    expect(htmlDoc).toContain("Fixture PR");
+  }, 90_000);
 });

test/utils.test.ts

@@ -6,7 +6,7 @@ import { describe, it, expect } from "vitest";
 import { mkdtemp, rm, writeFile } from "fs/promises";
 import { join } from "path";
 import { tmpdir } from "os";
-import { SKIP_DIRS, README_NAMES, readFileSafe, escapeRegex } from "../src/utils.js";
+import { SKIP_DIRS, README_NAMES, readFileSafe, escapeRegex, getFlagValue, hasFlag } from "../src/utils.js";
 
 describe("SKIP_DIRS", () => {
   it("is a Set containing common directories to skip", () => {
@@ -103,3 +103,43 @@ describe("escapeRegex", () => {
     expect(escapeRegex(input)).toBe(expected);
   });
 });
+
+describe("getFlagValue", () => {
+  it("reads a value from the `--flag value` form", () => {
+    expect(getFlagValue(["--branch", "dev", "--json"], ["--branch", "-b"])).toBe("dev");
+  });
+
+  it("reads a value from the `--flag=value` form", () => {
+    expect(getFlagValue(["--branch=dev"], ["--branch", "-b"])).toBe("dev");
+  });
+
+  it("matches any of the provided aliases", () => {
+    expect(getFlagValue(["-b", "main"], ["--branch", "-b"])).toBe("main");
+  });
+
+  it("returns undefined when the flag is absent", () => {
+    expect(getFlagValue(["--json"], ["--branch", "-b"])).toBeUndefined();
+  });
+
+  it("returns the first occurrence when repeated", () => {
+    expect(getFlagValue(["--branch", "a", "--branch", "b"], ["--branch"])).toBe("a");
+  });
+});
+
+describe("hasFlag", () => {
+  it("detects a present boolean flag", () => {
+    expect(hasFlag(["--full-clone", "--json"], ["--full-clone"])).toBe(true);
+  });
+
+  it("detects an alias", () => {
+    expect(hasFlag(["-v"], ["--verbose", "-v"])).toBe(true);
+  });
+
+  it("detects the `--flag=value` form", () => {
+    expect(hasFlag(["--keep-temp=true"], ["--keep-temp"])).toBe(true);
+  });
+
+  it("returns false when absent", () => {
+    expect(hasFlag(["--json"], ["--full-clone"])).toBe(false);
+  });
+});