fix(migrate): report a failing migration once, naming the migration

.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: <message>` 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`.

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<ReturnType<typeof runNonInteractive>>;
+    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({

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 }
+      );
     }
   }
 

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<void> {
+    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<void> {
+    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/);
+  });
+});