From 3efc27564fd2e2250e6138d5d3459e07f9b3e000 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:37:48 -0700 Subject: [PATCH] fix(migrate): report a failing migration once, naming the migration runMigrations printed `Migration N failed: ` and then rethrew the original, so the caller printed the same text again without the migration number. Fold the prefix into the rethrown error instead: one string, one printer. A CLIError keeps its code and reported flag; anything else stays a plain Error (so it still classifies as INTERNAL_ERROR) with the original as `cause`. Also give `init --json` the error envelope its siblings have. A throwing migration left stdout empty, so a machine consumer had only the exit code. The manifest stamp in the same catch is untouched. --- .changeset/migration-failure-reporting.md | 7 + packages/cli/src/commands/init.ts | 30 +++- packages/cli/src/filesystem/migrate.ts | 38 +++++- packages/cli/test/migration-failure.test.ts | 144 ++++++++++++++++++++ 4 files changed, 214 insertions(+), 5 deletions(-) create mode 100644 .changeset/migration-failure-reporting.md create mode 100644 packages/cli/test/migration-failure.test.ts diff --git a/.changeset/migration-failure-reporting.md b/.changeset/migration-failure-reporting.md new file mode 100644 index 00000000..8aa0ed44 --- /dev/null +++ b/.changeset/migration-failure-reporting.md @@ -0,0 +1,7 @@ +--- +"@taskless/cli": patch +--- + +A failing schema migration is now reported once instead of twice, and the message names the migration that refused. `runMigrations` used to print `Migration N failed: ` itself and then rethrow the original, so the caller printed the same text again without the migration number — the longer and more useful the refusal, the worse it read. The prefix now rides on the rethrown error, so there is one string and one printer, and the original `CLIError` code (for example `SCAFFOLD_CONFLICT`) is preserved. + +`taskless init --json` also emits an error envelope when a migration fails. It previously wrote nothing at all to stdout and put prose on stderr, leaving a machine consumer with only the exit code; it now writes the standard `{ ok: false, code, message }` envelope, with the migration number in `message`. diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 26943266..74bfaccc 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -73,7 +73,35 @@ export const initCommand = defineCommand({ const cwd = resolve(args.dir ?? process.cwd()); const telemetry = await getTelemetry(cwd); - const result = await runNonInteractive(cwd, { json: args.json }); + // Under `--json`, a failure has to reach stdout as an envelope like every + // other machine-readable command's does. It did not: a throwing migration + // left stdout completely empty and put prose on stderr, so a consumer that + // parses stdout saw nothing at all and had only the exit code to go on. + // `update` has carried the envelope for its own failures all along + // (`makeErrorEnvelope` below); this is the same shape, not a second one. + // + // The rethrow is what keeps this from becoming a swallow: `reported` + // suppresses the top-level handler's stderr print (the envelope is the + // report) while the throw still sets the exit code and gives telemetry the + // failure to classify. Carrying the original `code` through means + // `SCAFFOLD_CONFLICT` is still what `cli_error` records. + let result: Awaited>; + try { + result = await runNonInteractive(cwd, { json: args.json }); + } catch (error) { + if (!args.json) throw error; + const message = error instanceof Error ? error.message : String(error); + const code = + error instanceof CLIError && error.code ? error.code : "INTERNAL_ERROR"; + console.log(JSON.stringify(makeErrorEnvelope(code, message))); + throw new CLIError( + message, + error instanceof CLIError ? error.code : undefined, + { + reported: true, + } + ); + } if (args.json) { console.log( JSON.stringify({ diff --git a/packages/cli/src/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index 908b56a5..d1b4bf9c 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -297,9 +297,6 @@ export async function runMigrations( try { await migrate(tasklessDirectory); } catch (error) { - console.error( - `Migration ${String(v)} failed: ${error instanceof Error ? error.message : String(error)}` - ); // Write manifest at last successful version so we don't re-run // completed migrations. Re-read from disk so we preserve whatever // earlier successful migrations wrote (instead of writing back `raw`, @@ -332,7 +329,40 @@ export async function runMigrations( }); } } - throw error; + // One string, one printer. This used to `console.error` the prefixed + // message here and then rethrow the original, so whoever owns the + // surface printed the same text a second time: once naming the + // migration, once not (taskless/cli#389). Folding the prefix into the + // rethrown message keeps the migration number — the only thing that + // says WHICH migration refused, since a migration's own message names + // paths and never itself — and leaves the printing to the one layer + // that owns it: the top-level handler, the wizard, or a `--json` + // envelope. + // + // Both branches end in a throw, unconditionally. The failure mode + // opposite to a doubled line is swallowing, which would exit 0 and let + // `init` report success over a half-migrated tree. + if (error instanceof CLIError) { + // `code` is carried through rather than re-coded: telemetry attributes + // on it, and collapsing every migration failure to one code would + // flatten `SCAFFOLD_CONFLICT` (a deliberate, actionable refusal) into + // the same bucket as an unexpected fault. `reported` rides along so a + // throw site that already showed the user something is still not + // printed again. + throw new CLIError( + `Migration ${String(v)} failed: ${error.message}`, + error.code, + { reported: error.reported } + ); + } + // Deliberately a plain `Error`, not a `CLIError`: an unrecognized fault + // should keep classifying as INTERNAL_ERROR, exactly as it did when the + // original propagated. Only the message gains the prefix, and `cause` + // keeps the original reachable for anything inspecting it. + throw new Error( + `Migration ${String(v)} failed: ${error instanceof Error ? error.message : String(error)}`, + { cause: error } + ); } } diff --git a/packages/cli/test/migration-failure.test.ts b/packages/cli/test/migration-failure.test.ts new file mode 100644 index 00000000..38fe1bc9 --- /dev/null +++ b/packages/cli/test/migration-failure.test.ts @@ -0,0 +1,144 @@ +import { execFile } from "node:child_process"; +import { mkdir, mkdtemp, rm, writeFile } 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 } from "vitest"; + +import { cliRejectionToResult } from "./support/spawn-cli"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +/** + * These spawn the BUILT CLI rather than calling `runMigrations` directly, and + * that is the point of the file. + * + * The bug (taskless/cli#389) was a `console.error` inside `runMigrations` on + * top of the caller's own print, so it exists only in the seam between the two + * layers; a unit test of `runMigrations` cannot see it. More importantly, the + * failure mode opposite to a doubled line is SWALLOWING — a catch that stops + * rethrowing would exit 0 and let `init` report success over a half-migrated + * tree. Only a real process has an exit code, so the `exitCode` assertion in + * each test below is the guard that matters, and the occurrence count is the + * one that describes the bug. + */ +async function runCli( + args: string[], + cwd: string +): Promise<{ stdout: string; stderr: string; exitCode: number }> { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args], { + cwd, + }); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + return cliRejectionToResult(error, [binPath, ...args]); + } +} + +describe("a failing migration is reported exactly once", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "taskless-migration-failure-")); + await mkdir(join(cwd, ".taskless"), { recursive: true }); + // Version 3 is below migration 4, so `init` runs the engine partition. + await writeFile( + join(cwd, ".taskless", "taskless.json"), + JSON.stringify({ version: 3 }), + "utf8" + ); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + /** + * A *file* where migration 4 needs the `sg/` engine directory. Its + * `assertNoDirectoryConflicts` refuses with a coded `CLIError` + * (SCAFFOLD_CONFLICT) before moving anything. + */ + async function seedConflictingEngineDirectory(): Promise { + await writeFile(join(cwd, ".taskless", "sg"), "not a directory\n", "utf8"); + } + + /** + * A file at `.taskless/rules` instead. Nothing checks that path, so the + * migration reaches a bare `mkdir` and throws an EEXIST `Error` — an + * unrecognized fault rather than a deliberate refusal. Both shapes doubled, + * so both are pinned. + */ + async function seedUnexpectedFault(): Promise { + await writeFile( + join(cwd, ".taskless", "rules"), + "not a directory\n", + "utf8" + ); + } + + it("prints a coded refusal once, naming the migration", async () => { + await seedConflictingEngineDirectory(); + + const result = await runCli(["init"], cwd); + + // The guard against swallowing: a catch that stopped rethrowing would + // exit 0 here with `init` claiming success over a half-migrated tree. + expect(result.exitCode).toBe(1); + expect(result.stderr.match(/Cannot partition/g)).toHaveLength(1); + // The migration number is the only thing naming WHICH migration refused — + // the refusal's own text names `.taskless/sg` and never itself. + expect(result.stderr).toMatch( + /Migration 4 failed: Cannot partition \.taskless\// + ); + }); + + it("prints an unexpected fault once, naming the migration", async () => { + await seedUnexpectedFault(); + + const result = await runCli(["init"], cwd); + + expect(result.exitCode).toBe(1); + expect(result.stderr.match(/EEXIST/g)).toHaveLength(1); + expect(result.stderr).toMatch(/Migration 4 failed: EEXIST/); + }); + + it("emits a stdout envelope under --json, naming the migration", async () => { + await seedConflictingEngineDirectory(); + + const result = await runCli(["init", "--json"], cwd); + + expect(result.exitCode).toBe(1); + // It used to be empty: a machine consumer parsing stdout got nothing at + // all and had only the exit code to read. + expect(result.stdout.trim()).not.toBe(""); + const envelope = JSON.parse(result.stdout.trim()) as { + ok: boolean; + code: string; + message: string; + }; + expect(envelope.ok).toBe(false); + // The original code survives the wrap; re-coding to INTERNAL_ERROR would + // cost an agent the one thing it branches on. + expect(envelope.code).toBe("SCAFFOLD_CONFLICT"); + expect(envelope.message).toMatch(/^Migration 4 failed: /); + expect(envelope.message).toContain("Cannot partition"); + // The envelope is the report, so nothing repeats it on stderr. + expect(result.stderr).not.toMatch(/Cannot partition/); + }); + + it("emits an INTERNAL_ERROR envelope for an unexpected fault", async () => { + await seedUnexpectedFault(); + + const result = await runCli(["init", "--json"], cwd); + + expect(result.exitCode).toBe(1); + const envelope = JSON.parse(result.stdout.trim()) as { + code: string; + message: string; + }; + expect(envelope.code).toBe("INTERNAL_ERROR"); + expect(envelope.message).toMatch(/^Migration 4 failed: EEXIST/); + }); +});