From bc5437063d5e9ccb3ca26b1d15cf1d4c40686043 Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Tue, 4 Aug 2026 21:51:38 +0530 Subject: [PATCH] fix(cli): declare @wrnexus/authz dependency, exit cleanly on bad authz input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review fixes for Task 13: - packages/cli/package.json was missing @wrnexus/authz, and packages/authz/package.json was missing @wrnexus/core despite importing its types in index.ts/middleware.ts/advanced.ts. Both only worked in-repo because bare "@wrnexus/*" specifiers resolve through the root tsconfig.json paths map; a standalone install of @wrnexus/cli or @wrnexus/authz would fail at runtime. - authz.ts's unknown/missing-subcommand and bad --dialect paths now console.error + process.exit(1), matching db.ts's convention, instead of throwing — index.ts's top-level catch previously printed those as a raw stack trace. Added a subprocess-level test that spawns the real CLI and asserts stderr has the usage line with no stack frame. - nextMigrationNumber now extracts the leading-digit run the same way db/migrate.ts's nextNumber does, instead of a fixed slice(0, 4) that would have undercounted once a migration number passed 9999. --- packages/authz/package.json | 1 + packages/cli/package.json | 1 + packages/cli/src/authz.ts | 30 ++++++---- packages/cli/test/authz-command.test.ts | 79 +++++++++++++++++++++++-- 4 files changed, 95 insertions(+), 16 deletions(-) diff --git a/packages/authz/package.json b/packages/authz/package.json index 94afc646..014807f8 100644 --- a/packages/authz/package.json +++ b/packages/authz/package.json @@ -9,6 +9,7 @@ "./db": "./src/db.ts" }, "dependencies": { + "@wrnexus/core": "workspace:*", "@wrnexus/db": "workspace:*" } } diff --git a/packages/cli/package.json b/packages/cli/package.json index 2ef75a6d..17c6d364 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -23,6 +23,7 @@ "@wrnexus/mcp": "workspace:*", "@wrnexus/playground": "workspace:*", "@wrnexus/db": "workspace:*", + "@wrnexus/authz": "workspace:*", "@wrnexus/plugin": "workspace:*", "@wrnexus/syntax": "workspace:*", "@wrnexus/typecheck": "workspace:*", diff --git a/packages/cli/src/authz.ts b/packages/cli/src/authz.ts index 8af48500..b83de5cb 100644 --- a/packages/cli/src/authz.ts +++ b/packages/cli/src/authz.ts @@ -41,23 +41,33 @@ export async function loadAuthzCatalog(appDir: string): Promise { return mergeCatalogs(sources); } +/** Print a message and exit non-zero, matching db.ts's convention for user-facing + * CLI errors: never throw, so index.ts's generic `main().catch` handler (which + * prints the raw error, stack and all) is never reached for an expected failure. */ +function fail(message: string): never { + console.error(message); + process.exit(1); +} + +// Same leading-digit extraction as db/migrate.ts's `nextNumber`: a fixed +// `slice(0, 4)` would undercount once a migration number grows past 9999. function nextMigrationNumber(dir: string): string { if (!existsSync(dir)) return "0001"; - const numbers = readdirSync(dir) - .map((name) => Number.parseInt(name.slice(0, 4), 10)) - .filter((value) => Number.isInteger(value)); - return String((numbers.length ? Math.max(...numbers) : 0) + 1).padStart(4, "0"); + let max = 0; + for (const name of readdirSync(dir)) { + const match = /^(\d+)/.exec(name); + if (match) max = Math.max(max, Number(match[1])); + } + return String(max + 1).padStart(4, "0"); } /** Parse `--dialect=` from CLI args. Defaults to sqlite; rejects unknown values. */ function resolveDialect(args: string[]): Dialect { const flag = args.find((arg) => arg.startsWith("--dialect=")); if (!flag) return "sqlite"; - const value = flag.split("=")[1]; - if ((DIALECTS as readonly string[]).includes(value ?? "")) return value as Dialect; - throw new Error( - `WRN-AUTHZ-INIT: unrecognised --dialect='${value}'. Use one of: ${DIALECTS.join(", ")}.`, - ); + const value = flag.split("=")[1] ?? ""; + if ((DIALECTS as readonly string[]).includes(value)) return value as Dialect; + return fail(`Unrecognised --dialect='${value}'. Use one of: ${DIALECTS.join(", ")}.`); } export async function runAuthzCommand( @@ -118,6 +128,6 @@ export async function runAuthzCommand( } default: - throw new Error(USAGE); + fail(USAGE); } } diff --git a/packages/cli/test/authz-command.test.ts b/packages/cli/test/authz-command.test.ts index 1e6eb958..fefa1fd2 100644 --- a/packages/cli/test/authz-command.test.ts +++ b/packages/cli/test/authz-command.test.ts @@ -1,4 +1,4 @@ -import { afterAll, describe, expect, test } from "bun:test"; +import { afterAll, describe, expect, spyOn, test } from "bun:test"; import { mkdirSync, mkdtempSync, @@ -45,6 +45,31 @@ afterAll(() => { rmSync(scratchRoot, { recursive: true, force: true }); }); +/** + * authz.ts's `fail()` helper (usage errors, bad --dialect) mirrors db.ts's + * convention: console.error + process.exit(1), never throw — so index.ts's + * generic `main().catch(err) { console.error(err); process.exit(1); }` (which + * prints the raw Error, stack and all) never sees an expected validation + * failure. That means a *direct* call to runAuthzCommand() would normally kill + * the whole test worker via a real process.exit(); intercept both console.error + * and process.exit so the failure path stays testable in-process. + */ +async function expectCleanFailure(run: () => Promise): Promise { + const errors: string[] = []; + const originalError = console.error; + console.error = (...args: unknown[]) => void errors.push(args.join(" ")); + const exitSpy = spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`__process_exit_${code}__`); + }) as never); + try { + await expect(run()).rejects.toThrow(/^__process_exit_1__$/); + } finally { + console.error = originalError; + exitSpy.mockRestore(); + } + return errors.join("\n"); +} + describe("wrnexus authz", () => { test("loadAuthzCatalog merges every declaration", async () => { const catalog = await loadAuthzCatalog(join(scaffold(), "app")); @@ -87,14 +112,38 @@ describe("wrnexus authz", () => { expect(output).toContain("editor"); }); - test("an unknown subcommand throws with usage", async () => { - await expect(runAuthzCommand(scaffold(), "bogus", [])).rejects.toThrow(/usage/i); + test("an unknown subcommand prints usage and exits 1, not a thrown error", async () => { + const errorOutput = await expectCleanFailure(() => runAuthzCommand(scaffold(), "bogus", [])); + expect(errorOutput).toMatch(/usage/i); }); - test("a missing subcommand throws with usage", async () => { - await expect(runAuthzCommand(scaffold(), undefined, [])).rejects.toThrow(/usage/i); + test("a missing subcommand prints usage and exits 1", async () => { + const errorOutput = await expectCleanFailure(() => runAuthzCommand(scaffold(), undefined, [])); + expect(errorOutput).toMatch(/usage/i); }); + test("CLI subprocess: unknown subcommand prints usage without a stack trace", async () => { + const cliEntry = join(import.meta.dir, "..", "src", "index.ts"); + const root = scaffold(); + const proc = Bun.spawn({ + cmd: ["bun", cliEntry, "authz", "bogus"], + cwd: root, + env: { ...process.env, WRNEXUS_NO_UPDATE_CHECK: "1" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stderr] = await Promise.all([ + new Response(proc.stderr).text(), + new Response(proc.stdout).text(), + ]); + const exitCode = await proc.exited; + expect(exitCode).not.toBe(0); + expect(stderr).toMatch(/usage/i); + // A raw Error/stack trace looks like "at (file.ts:12:34)"; the clean + // console.error(usage) + process.exit(1) path never produces that shape. + expect(stderr).not.toMatch(/at .*\.ts:\d+/); + }, 15000); + test("list does not crash on an app with no app/authz directory", async () => { mkdirSync(scratchRoot, { recursive: true }); const root = mkdtempSync(join(scratchRoot, "empty-")); @@ -160,7 +209,10 @@ describe("wrnexus authz", () => { test("init with an unrecognised --dialect= does not silently fall back to sqlite", async () => { const root = scaffold(); mkdirSync(join(root, "app", "db", "migrations"), { recursive: true }); - await expect(runAuthzCommand(root, "init", ["--dialect=oracle"])).rejects.toThrow(/dialect/i); + const errorOutput = await expectCleanFailure(() => + runAuthzCommand(root, "init", ["--dialect=oracle"]), + ); + expect(errorOutput).toMatch(/dialect/i); }); test("init writes a migration that the migration runner can parse", async () => { @@ -178,6 +230,21 @@ describe("wrnexus authz", () => { expect(migration.down).toContain("DROP TABLE IF EXISTS _wrn_authz_assignment"); }); + test("init numbers the next migration correctly past a 5-digit prefix", async () => { + // nextMigrationNumber originally sliced the first 4 characters of the + // filename, which would have parsed "10000_big.sql" as "1000" and reused + // that number instead of advancing past it. It must match db/migrate.ts's + // leading-digit regex instead. + const root = scaffold(); + const dir = join(root, "app", "db", "migrations"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "0001_users.sql"), "-- +up\n\n-- +down\n", "utf8"); + writeFileSync(join(dir, "10000_big.sql"), "-- +up\n\n-- +down\n", "utf8"); + await runAuthzCommand(root, "init", []); + const file = readdirSync(dir).find((name) => name.includes("authz")); + expect(file).toBe("10001_authz_tables.sql"); + }); + test("generate does not clobber a real declaration file", async () => { const root = scaffold(); await runAuthzCommand(root, "generate", []);