diff --git a/.changeset/init-json-envelope.md b/.changeset/init-json-envelope.md new file mode 100644 index 00000000..5194f842 --- /dev/null +++ b/.changeset/init-json-envelope.md @@ -0,0 +1,13 @@ +--- +"@taskless/cli": patch +--- + +`init --json` now writes only the parseable envelope to stdout. Previously, +the non-interactive install path (also reached from `init --no-interactive +--json`) unconditionally logged human-readable prose — the "no tools +detected" fallback notice and the per-target skill/command summary — to +stdout ahead of the JSON envelope, so `taskless init --json | jq .` failed +with a JSON parse error. That prose now goes to stderr, where it stays +visible to a person watching the terminal without corrupting a machine +consumer's view of stdout, matching how `verify`/`test` and the migration +notice already behave under `--json`. diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 58cd4c86..eb7f520c 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -94,7 +94,7 @@ export const initCommand = defineCommand({ ); } - const result = await runNonInteractive(cwd); + const result = await runNonInteractive(cwd, { json: args.json }); if (args.json) { console.log( JSON.stringify({ @@ -245,11 +245,20 @@ export const updateCommand = defineCommand({ }, }); -async function runNonInteractive(cwd: string): Promise<{ +async function runNonInteractive( + cwd: string, + options: { json?: boolean } = {} +): Promise<{ commandsInstalled: boolean; reloadNotice: string | undefined; migrated: MigrationReport | undefined; }> { + // Under `--json`, stdout carries only the envelope printed by the caller. + // This per-target summary is not on that envelope (it is finer-grained than + // `migrated`/`commandsInstalled`), so rather than drop it, it goes to + // stderr — visible to a person watching the terminal, invisible to a + // machine consumer parsing stdout. + const log = options.json ? console.error : console.log; // Sampled BEFORE the directory is created, and that order is the whole // point. `ensureTasklessDirectory` mkdir -p's, so afterwards a pre-existing // project is indistinguishable from a fresh one. @@ -263,7 +272,24 @@ async function runNonInteractive(cwd: string): Promise<{ // can report what a migration moved. `check`, `verify` and `test` used to // carry this on their own envelopes and refuse rather than migrate now, so // the field followed the behaviour rather than being dropped. - const migrated = await ensureTasklessDirectory(cwd); + // + // The migration notice is suppressed entirely under `--json`, rather than + // moved to stderr like the per-target summary above: unlike that summary, + // this information IS already on the envelope, as `migrated`, so printing + // it a second time would just be noise. This is the actual `verify`/`test` + // convention (`verify.ts`'s `onNotice: (message) => { if (!json) + // console.error(message); }`), and the case `EnsureOptions.onNotice`'s own + // doc comment describes: "callers that emit `--json` should pass a + // callback that suppresses output under that flag: the same information is + // on the envelope's `migrated` field". Omitting `onNotice` here, as before, + // left it on the default fallback (unconditional `console.error`), which + // never corrupts stdout but doesn't suppress the duplicate under `--json` + // either — the gap a reviewer of this PR caught. + const migrated = await ensureTasklessDirectory(cwd, { + onNotice: (message: string) => { + if (!options.json) console.error(message); + }, + }); if (wasNewProject) { // A project this CLI just created has no entries to walk: everything the // ledger describes is already true of the scaffold it wrote. @@ -291,7 +317,7 @@ async function runNonInteractive(cwd: string): Promise<{ const reloadNotice = getReloadNotice({ previousCliVersion, cliVersion }); if (detected.length === 0) { - console.log(`No tools detected. Using fallback: ${DEFAULT_SHIM_DIR}/`); + log(`No tools detected. Using fallback: ${DEFAULT_SHIM_DIR}/`); } const skillsByTarget = groupValuesByTarget( @@ -332,33 +358,29 @@ async function runNonInteractive(cwd: string): Promise<{ removedSkills.length === 0 && removedCommands.length === 0 ) { - console.log(`${target.label} (${target.dir}/): up to date`); + log(`${target.label} (${target.dir}/): up to date`); continue; } - console.log( + log( `${target.label} (${target.dir}/): wrote ${String(writtenSkills.length)} skill ${noun}(s)` ); for (const name of writtenSkills) { - console.log(` - ${name}`); + log(` - ${name}`); } if (writtenCommands.length > 0) { - console.log(` + ${String(writtenCommands.length)} command ${noun}(s)`); + log(` + ${String(writtenCommands.length)} command ${noun}(s)`); } if (removedSkills.length > 0) { - console.log( - ` removed ${String(removedSkills.length)} obsolete skill(s):` - ); + log(` removed ${String(removedSkills.length)} obsolete skill(s):`); for (const name of removedSkills) { - console.log(` - ${name}`); + log(` - ${name}`); } } if (removedCommands.length > 0) { - console.log( - ` removed ${String(removedCommands.length)} obsolete command(s):` - ); + log(` removed ${String(removedCommands.length)} obsolete command(s):`); for (const name of removedCommands) { - console.log(` - ${name}`); + log(` - ${name}`); } } } diff --git a/packages/cli/test/error-envelope.test.ts b/packages/cli/test/error-envelope.test.ts index 55d56494..2c738b80 100644 --- a/packages/cli/test/error-envelope.test.ts +++ b/packages/cli/test/error-envelope.test.ts @@ -44,10 +44,7 @@ async function runCli( function parseEnvelope(stdout: string): ErrorEnvelope { // The envelope is the last JSON line in stdout. (Some commands also // print progress to stderr, so we ignore that.) - const lines = stdout.split("\n").filter((l) => l.trim().startsWith("{")); - expect(lines.length).toBeGreaterThan(0); - const last = lines.at(-1)!; - return JSON.parse(last) as ErrorEnvelope; + return JSON.parse(stdout.trim()) as ErrorEnvelope; } describe("standardized error envelope (--json)", () => { diff --git a/packages/cli/test/migrated-envelope.test.ts b/packages/cli/test/migrated-envelope.test.ts index e0e36a48..ae026e5d 100644 --- a/packages/cli/test/migrated-envelope.test.ts +++ b/packages/cli/test/migrated-envelope.test.ts @@ -36,9 +36,18 @@ async function runCli( } } +/** + * Parse `--json` stdout as the WHOLE envelope, not just its last line. + * + * `stdout.trim().split("\n").at(-1)` was the earlier shape of this helper, + * and it is exactly why #279 (`init --json` printing prose ahead of the + * envelope) stayed invisible: a helper that only ever reads the last line + * cannot fail on anything printed before it. `JSON.parse` on the trimmed + * whole string fails loudly the moment stdout carries a second thing, + * whichever end it lands on. + */ function parseEnvelope(stdout: string): Record { - const line = stdout.trim().split("\n").at(-1) ?? ""; - return JSON.parse(line) as Record; + return JSON.parse(stdout.trim()) as Record; } /** The versions a project seeded at 3 must be carried through. */ @@ -120,6 +129,26 @@ describe("who migrates, and who refuses", () => { expectSeededMigration(envelope.migrated); }); + it("init --json reports the migration on stdout and stays silent about it on stderr", async () => { + // The migration notice duplicates the envelope's `migrated` field, so + // under `--json` it is suppressed rather than moved to stderr - unlike + // the per-target install summary, which stderr DOES carry under `--json` + // because that detail has no field of its own. Catches a regression that + // routes this notice back through the unconditional `console.error` + // fallback `ensureTasklessDirectory` uses when no `onNotice` is passed. + await seedVersion3(); + + const { stderr } = await runCli([ + "init", + "--no-interactive", + "--json", + "-d", + temporaryDirectory, + ]); + + expect(stderr).not.toContain("Migrat"); + }); + it("init --json omits the field when nothing migrated", async () => { // Absence is the signal, so a consumer never reads empty arrays to decide. await seedVersion3(); diff --git a/packages/cli/test/no-implicit-migration.test.ts b/packages/cli/test/no-implicit-migration.test.ts index d93d5097..0cf04bb2 100644 --- a/packages/cli/test/no-implicit-migration.test.ts +++ b/packages/cli/test/no-implicit-migration.test.ts @@ -57,6 +57,21 @@ async function runCli( } } +/** + * Parse `--json` stdout as the WHOLE envelope, not just its last line. + * + * The earlier shape of every call site here was + * `JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}")`, and that is exactly + * why `init --json` printing prose ahead of its envelope (#279) went + * undetected: a helper that only ever reads the last line cannot fail on + * anything printed before it. `JSON.parse` on the trimmed whole string fails + * loudly the moment stdout carries a second thing, whichever end it lands on + * — do not narrow this back to a last-line read. + */ +function parseEnvelope(stdout: string): T { + return JSON.parse(stdout.trim()) as T; +} + const FLAT_RULE = "id: no-eval\nlanguage: TypeScript\nseverity: error\nmessage: no eval\nrule:\n pattern: eval($A)\n"; @@ -114,10 +129,10 @@ describe("a reporting command never migrates", () => { "%s --json carries the code an agent branches on", async (command) => { const { stdout } = await runCli([command, "--json", "-d", directory]); - const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { + const envelope = parseEnvelope<{ ok?: boolean; code?: string; - }; + }>(stdout); expect(envelope.ok).toBe(false); // Distinct from SCAFFOLD_VERSION_MISMATCH, which is the opposite // direction and asks the caller to upgrade the CLI instead. @@ -231,9 +246,9 @@ describe("a reporting command never migrates", () => { directory, ]); - const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { + const envelope = parseEnvelope<{ migrated?: { from: number; to: number }; - }; + }>(stdout); expect(envelope.migrated?.from).toBe(3); expect(envelope.migrated?.to).toBe(LATEST_SCHEMA_VERSION); await expect( @@ -278,11 +293,11 @@ describe("a manifest that cannot be parsed", () => { "%s --json reports the file, not a version it guessed", async (command) => { const { stdout } = await runCli([command, "--json", "-d", directory]); - const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { + const envelope = parseEnvelope<{ ok?: boolean; code?: string; message?: string; - }; + }>(stdout); expect(envelope.ok).toBe(false); expect(envelope.code).toBe("SCAFFOLD_MANIFEST_UNREADABLE"); expect(envelope.message).toContain("taskless.json"); @@ -331,9 +346,9 @@ describe("a manifest that cannot be parsed", () => { "-d", bare, ]); - const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { + const envelope = parseEnvelope<{ migrated?: { from: number; to: number }; - }; + }>(stdout); expect(envelope.migrated?.from).toBe(0); expect(envelope.migrated?.to).toBe(LATEST_SCHEMA_VERSION); } finally {