Skip to content

Commit 820cdbb

Browse files
committed
fix(cli): wire onNotice so migration prose is suppressed under --json, and share the whole-stdout envelope parser
Review feedback on #313: 1. init.ts's comment claimed this diff matches the verify/test convention of routing --json prose off stdout, but ensureTasklessDirectory(cwd) was called with no onNotice, so its migration notice always fell back to an unconditional console.error regardless of --json. verify.ts actually SUPPRESSES that notice entirely under --json (its info already lives on the envelope), matching EnsureOptions.onNotice's own doc comment. Wired the same onNotice here so the code matches what the comment says, added a regression test asserting stderr carries nothing about the migration under --json, and mutation-checked it (dropping the !json guard fails the new test; restoring it passes). 2. no-implicit-migration.test.ts's four JSON.parse(stdout.trim()) call sites (from a prior commit widening them off the old blind .at(-1) read) are now a single parseEnvelope<T> helper, documented the same way migrated-envelope.test.ts's version is, so the reasoning for whole-string parsing isn't duplicated four times without its rationale attached. Mutation-checked: reintroducing the original init --json stdout-prose bug fails both init --json tests in this file through the shared helper.
1 parent e130b67 commit 820cdbb

3 files changed

Lines changed: 62 additions & 12 deletions

File tree

‎packages/cli/src/commands/init.ts‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -257,9 +257,7 @@ async function runNonInteractive(
257257
// This per-target summary is not on that envelope (it is finer-grained than
258258
// `migrated`/`commandsInstalled`), so rather than drop it, it goes to
259259
// stderr — visible to a person watching the terminal, invisible to a
260-
// machine consumer parsing stdout. Matches `ensureTasklessDirectory`'s own
261-
// default (`runMigrations` falls back to `console.error`) and the
262-
// `verify`/`test` convention of routing prose off stdout under `--json`.
260+
// machine consumer parsing stdout.
263261
const log = options.json ? console.error : console.log;
264262
// Sampled BEFORE the directory is created, and that order is the whole
265263
// point. `ensureTasklessDirectory` mkdir -p's, so afterwards a pre-existing
@@ -274,7 +272,24 @@ async function runNonInteractive(
274272
// can report what a migration moved. `check`, `verify` and `test` used to
275273
// carry this on their own envelopes and refuse rather than migrate now, so
276274
// the field followed the behaviour rather than being dropped.
277-
const migrated = await ensureTasklessDirectory(cwd);
275+
//
276+
// The migration notice is suppressed entirely under `--json`, rather than
277+
// moved to stderr like the per-target summary above: unlike that summary,
278+
// this information IS already on the envelope, as `migrated`, so printing
279+
// it a second time would just be noise. This is the actual `verify`/`test`
280+
// convention (`verify.ts`'s `onNotice: (message) => { if (!json)
281+
// console.error(message); }`), and the case `EnsureOptions.onNotice`'s own
282+
// doc comment describes: "callers that emit `--json` should pass a
283+
// callback that suppresses output under that flag: the same information is
284+
// on the envelope's `migrated` field". Omitting `onNotice` here, as before,
285+
// left it on the default fallback (unconditional `console.error`), which
286+
// never corrupts stdout but doesn't suppress the duplicate under `--json`
287+
// either — the gap a reviewer of this PR caught.
288+
const migrated = await ensureTasklessDirectory(cwd, {
289+
onNotice: (message: string) => {
290+
if (!options.json) console.error(message);
291+
},
292+
});
278293
if (wasNewProject) {
279294
// A project this CLI just created has no entries to walk: everything the
280295
// ledger describes is already true of the scaffold it wrote.

‎packages/cli/test/migrated-envelope.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,26 @@ describe("who migrates, and who refuses", () => {
129129
expectSeededMigration(envelope.migrated);
130130
});
131131

132+
it("init --json reports the migration on stdout and stays silent about it on stderr", async () => {
133+
// The migration notice duplicates the envelope's `migrated` field, so
134+
// under `--json` it is suppressed rather than moved to stderr - unlike
135+
// the per-target install summary, which stderr DOES carry under `--json`
136+
// because that detail has no field of its own. Catches a regression that
137+
// routes this notice back through the unconditional `console.error`
138+
// fallback `ensureTasklessDirectory` uses when no `onNotice` is passed.
139+
await seedVersion3();
140+
141+
const { stderr } = await runCli([
142+
"init",
143+
"--no-interactive",
144+
"--json",
145+
"-d",
146+
temporaryDirectory,
147+
]);
148+
149+
expect(stderr).not.toContain("Migrat");
150+
});
151+
132152
it("init --json omits the field when nothing migrated", async () => {
133153
// Absence is the signal, so a consumer never reads empty arrays to decide.
134154
await seedVersion3();

‎packages/cli/test/no-implicit-migration.test.ts‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,21 @@ async function runCli(
5757
}
5858
}
5959

60+
/**
61+
* Parse `--json` stdout as the WHOLE envelope, not just its last line.
62+
*
63+
* The earlier shape of every call site here was
64+
* `JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}")`, and that is exactly
65+
* why `init --json` printing prose ahead of its envelope (#279) went
66+
* undetected: a helper that only ever reads the last line cannot fail on
67+
* anything printed before it. `JSON.parse` on the trimmed whole string fails
68+
* loudly the moment stdout carries a second thing, whichever end it lands on
69+
* — do not narrow this back to a last-line read.
70+
*/
71+
function parseEnvelope<T>(stdout: string): T {
72+
return JSON.parse(stdout.trim()) as T;
73+
}
74+
6075
const FLAT_RULE =
6176
"id: no-eval\nlanguage: TypeScript\nseverity: error\nmessage: no eval\nrule:\n pattern: eval($A)\n";
6277

@@ -114,10 +129,10 @@ describe("a reporting command never migrates", () => {
114129
"%s --json carries the code an agent branches on",
115130
async (command) => {
116131
const { stdout } = await runCli([command, "--json", "-d", directory]);
117-
const envelope = JSON.parse(stdout.trim()) as {
132+
const envelope = parseEnvelope<{
118133
ok?: boolean;
119134
code?: string;
120-
};
135+
}>(stdout);
121136
expect(envelope.ok).toBe(false);
122137
// Distinct from SCAFFOLD_VERSION_MISMATCH, which is the opposite
123138
// direction and asks the caller to upgrade the CLI instead.
@@ -231,9 +246,9 @@ describe("a reporting command never migrates", () => {
231246
directory,
232247
]);
233248

234-
const envelope = JSON.parse(stdout.trim()) as {
249+
const envelope = parseEnvelope<{
235250
migrated?: { from: number; to: number };
236-
};
251+
}>(stdout);
237252
expect(envelope.migrated?.from).toBe(3);
238253
expect(envelope.migrated?.to).toBe(LATEST_SCHEMA_VERSION);
239254
await expect(
@@ -278,11 +293,11 @@ describe("a manifest that cannot be parsed", () => {
278293
"%s --json reports the file, not a version it guessed",
279294
async (command) => {
280295
const { stdout } = await runCli([command, "--json", "-d", directory]);
281-
const envelope = JSON.parse(stdout.trim()) as {
296+
const envelope = parseEnvelope<{
282297
ok?: boolean;
283298
code?: string;
284299
message?: string;
285-
};
300+
}>(stdout);
286301
expect(envelope.ok).toBe(false);
287302
expect(envelope.code).toBe("SCAFFOLD_MANIFEST_UNREADABLE");
288303
expect(envelope.message).toContain("taskless.json");
@@ -331,9 +346,9 @@ describe("a manifest that cannot be parsed", () => {
331346
"-d",
332347
bare,
333348
]);
334-
const envelope = JSON.parse(stdout.trim()) as {
349+
const envelope = parseEnvelope<{
335350
migrated?: { from: number; to: number };
336-
};
351+
}>(stdout);
337352
expect(envelope.migrated?.from).toBe(0);
338353
expect(envelope.migrated?.to).toBe(LATEST_SCHEMA_VERSION);
339354
} finally {

0 commit comments

Comments
 (0)