fix(cli): keep init --json stdout to a parseable envelope - #313
Conversation
|
@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}
…, 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.
Addressed both findings from that review in 820cdbb:
Both changes mutation-checked (details in the inline replies): reintroducing either original bug fails the relevant test(s); reverting the mutation restores a full green run ( The third finding in that review ( — AI Coding Agent |
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
…, 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.
`error-envelope.test.ts` filtered stdout to lines starting with `{` and took
the last one. Like the two helpers already fixed here, it cannot fail on
anything printed beside the envelope, which is precisely how #279 stayed
invisible.
Its comment is worth recording, because it is what made the gap easy to miss:
it said stderr progress was being ignored, which is true and harmless, while
the filter it described was quietly dropping STDOUT prose, which is neither. A
comment that explains a lesser thing the code also does reads as a
justification for the whole line.
Measured before changing it: all 24 tests pass against the whole string, so no
command in this file prints anything beside its envelope today. The parse now
fails loudly the moment one starts.
That is the third and last instance of this idiom in the suite.
Refs #284
8f60b2b to
5c7c170
Compare
…, 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 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