fix(cli): keep init --json stdout to a parseable envelope - #313
fix(cli): keep init --json stdout to a parseable envelope#313thecodedrift wants to merge 2 commits into
Conversation
runNonInteractive (the shared body of `init` and `init --no-interactive`) unconditionally console.log'd the "no tools detected" fallback notice and the per-target skill/command summary, so `init --json` wrote prose to stdout ahead of the JSON envelope and `taskless init --json | jq .` failed to parse. That prose now goes to stderr under --json (stdout otherwise, matching the existing human output), following the same convention already used by ensureTasklessDirectory's migration notices and by verify/test. Also fixes test/migrated-envelope.test.ts's parseEnvelope helper, which only ever read stdout's LAST line via .at(-1) and so could never detect anything printed before it -- exactly how this bug went unnoticed. It now JSON.parses the whole trimmed stdout.
…nd too
`no-implicit-migration.test.ts` carried the same `stdout.trim().split("\n")
.at(-1)` idiom in four places. A helper that reads only the last line cannot
fail on anything printed before it, which is exactly how the bug this PR fixes
stayed invisible, so leaving four more of them in place leaves four more places
for it to hide.
Measured rather than assumed, twice. Widening them to parse the whole of stdout
leaves all 25 tests passing, so no command in that file prints prose ahead of
its envelope today. Reintroducing the `--json` bug then fails four of them,
where the old helpers passed it silently.
The remaining `.at(-1)` in that file reads the last line of STDERR to check a
message, which is a legitimate use and is left alone.
Refs #284
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 28s —— View job Code ReviewReview mode: incremental — read 0 prior review thread(s) before reviewing.
SummaryThe fix does what it says: Three [New] findings from a closer read:
Nothing else stood out as a correctness, security, or performance issue. The remaining candidate observations from my review passes (e.g., no shared |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async (command) => { | ||
| const { stdout } = await runCli([command, "--json", "-d", directory]); | ||
| const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { | ||
| const envelope = JSON.parse(stdout.trim()) as { |
There was a problem hiding this comment.
[New] The JSON.parse(stdout.trim()) fix is duplicated at 4 call sites here (117, 234, 281, 334), each with its own inline result type, instead of extracting a shared helper the way migrated-envelope.test.ts (touched in this same diff) factors it into a documented parseEnvelope. That helper's comment explains why whole-string parsing matters — it's the regression guard for the exact bug this PR fixes — but that explanation isn't attached to any of the 4 sites here. A future edit could reintroduce .split("\n").at(-1) at one of them without anyone noticing the parallel in the other file.
Related: narrowing to JSON.parse(stdout.trim()) also dropped the ?? "{}" fallback that used to guard empty stdout, uniformly across all 4 sites. Minor — the test still fails either way — but on genuinely empty stdout it now throws SyntaxError: Unexpected end of JSON input instead of a legible expect(...) failure.
| // machine consumer parsing stdout. Matches `ensureTasklessDirectory`'s own | ||
| // default (`runMigrations` falls back to `console.error`) and the | ||
| // `verify`/`test` convention of routing prose off stdout under `--json`. | ||
| const log = options.json ? console.error : console.log; |
There was a problem hiding this comment.
[New] The comment above (256-262) claims this diff "matches … the verify/test convention of routing prose off stdout under --json." But further down, ensureTasklessDirectory(cwd) (line 277) is called with no onNotice option, so its migration prose ("Migrating .taskless/…" / "Migrated .taskless/…") always falls back to console.error unconditionally — regardless of --json. That's not what verify.ts does: it passes an onNotice that suppresses the message entirely under --json (verify.ts:75-79):
onNotice: (message: string) => {
if (!json) console.error(message);
},and EnsureOptions.onNotice's own doc comment (filesystem/directory.ts:14-16) says explicitly: "Callers that emit --json should pass a callback that suppresses output under that flag: the same information is on the envelope's migrated field…"
init --json's envelope does carry a migrated field (line 106-108), so this call site is exactly the case that doc comment describes, and it isn't wired that way. This doesn't corrupt stdout (the default already targets stderr), but it's a real gap between the stated convention and the code, and worth either wiring an onNotice here for consistency or correcting the comment.
1 participant
Summary
init --json(includinginit --no-interactive --json) wrote human prose to stdout ahead of the JSON envelope, sotaskless init --json | jq .failed to parse.runNonInteractive, the shared body behindinitandinit --no-interactive, unconditionallyconsole.log'd:Both now route through a
loghelper that isconsole.errorunder--jsonandconsole.logotherwise — matching the conventionensureTasklessDirectory's migration notices andverify/testalready use, and matching the reload notice's existing (already correct) handling in this same file.Where the prose goes under
--jsonstderr. It stays visible to a person watching the terminal (stderr renders in a normal terminal same as stdout) without corrupting a machine consumer's view of stdout. This information is not on the JSON envelope (finer-grained than
migrated/commandsInstalled), so dropping it silently was not an option — moving it off stdout was.Test fix (the other half, from #284)
test/migrated-envelope.test.ts'sparseEnvelopehelper only ever read stdout's last line via.trim().split("\n").at(-1). A helper shaped that way cannot fail on anything printed before the envelope — which is exactly how theinit --jsonprose-on-stdout bug went undetected by this file's own tests. It now doesJSON.parse(stdout.trim())on the whole thing, so any extra output anywhere in stdout fails the parse.Mutation check
const log = console.log;, ignoring thejsonoption) and rebuilt.parseEnvelope(whole-stdout parse): failed,SyntaxError: Unexpected token 'N', "No tools d"... is not valid JSON, on bothinit --jsontests in the file..at(-1)helper, run by hand against the same corrupted stdout: parsed successfully — it read only the JSON line and never saw the prose above it. This is the concrete demonstration that the test-helper fix is the load-bearing half of this PR, not just the production fix.Other tests with the same blind spot
packages/cli/test/no-implicit-migration.test.tsuses the identicalstdout.trim().split("\n").at(-1)idiom in four places (lines 117, 234, 281, 334) plus once on stderr (303). Same shape of bug, same fix would apply, but that file is outside this PR's assigned scope — noting it here rather than editing it.Verification
pnpm typecheck— passpnpm test— 83 files / 1345 tests passpnpm lint(builds first, runspnpm cli check) — pass, "No issues found."init --no-interactive --json -d .in a scratch dir, piped stdout throughJSON.parse— parses cleanly:Fixes #279
Refs #284