fix: keep CLI JSON output parseable#3257
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
financialvice
force-pushed
the
cg/cli-json-stdout
branch
from
ad0256f to
367ccc0
Compare
caiopizzol
left a comment
caiopizzol
left a comment
There was a problem hiding this comment.
@financialvice thanks for the PR - real issue, and the subprocess test is the right shape.
the noisy line is console.debug('[super-editor] Telemetry: enabled') at Editor.ts:627; apps/mcp/src/index.ts:5-11 already worked around the same line once. hiding it behind a debug flag closes the leak with no other behavior change (telemetry stays on, just no dev log on stdout). if you want a backstop for other stray console writes, the right spot is the CLI startup at apps/cli/src/index.ts:467, not inside invokeCommand.
i have a local patch that drops the per-call wrapper, gates the telemetry log, and keeps your subprocess test - cli.test.ts 107/107 green. happy to push it as a maintainer fixup if you'd prefer, or take it yourself. one thing inline either way.
| async function withConsoleDiagnosticPolicy<T>(globals: GlobalOptions, operation: () => Promise<T>): Promise<T> { | ||
| if (globals.output === 'pretty' && !globals.quiet) { | ||
| return operation(); | ||
| } | ||
|
|
||
| const original: ConsoleSnapshot = { | ||
| debug: console.debug, | ||
| info: console.info, | ||
| log: console.log, | ||
| warn: console.warn, | ||
| error: console.error, | ||
| }; | ||
| const suppress = (..._values: unknown[]) => { | ||
| return; | ||
| }; | ||
|
|
||
| console.debug = suppress; | ||
| console.info = suppress; | ||
| console.log = suppress; | ||
| console.warn = suppress; | ||
| console.error = suppress; | ||
|
|
||
| try { | ||
| return await operation(); | ||
| } finally { | ||
| console.debug = original.debug; | ||
| console.info = original.info; | ||
| console.log = original.log; | ||
| console.warn = original.warn; | ||
| console.error = original.error; | ||
| } | ||
| } |
There was a problem hiding this comment.
this swaps out the global console from inside invokeCommand, but the doc comment (line 339) says invokeCommand runs "without process-level I/O side effects" - any code that imports it gets the side effect anyway. calling it twice at the same time (e.g. Promise.all([invokeCommand(...), invokeCommand(...)])) also leaves the process console silenced afterward, because the two save/restore cycles overlap.
4 participants
Summary
console.debug/console.log/console.info/console.warn/console.errorwhile JSON or quiet commands execute, so lower-level library diagnostics cannot bypass the CLIioabstraction and pollute stdoutNODE_ENVunset and parsessuperdoc info --jsonstdout directlyWhy
superdoc <command> --jsonshould produce machine-parseable JSON on stdout. In normal non-test CLI execution, SuperEditor telemetry logs this line withconsole.debugbefore the JSON envelope:Because Node writes
console.debugto stdout, automation that expectedJSON.parse(stdout)failed even though the command itself succeeded.--quietalso could not suppress this because the log bypassed the CLIioabstraction.The root issue is not telemetry specifically. The root issue is that JSON/quiet CLI execution did not own process-level
console.*output, so any dependency could write directly to stdout outside the CLI response contract. This PR fixes that at the top-level CLI boundary instead of special-casing one telemetry log.Validation
pnpm --filter @superdoc-dev/cli exec bun test src/__tests__/cli.test.ts -t "json output is parseable" --timeout 30000NODE_ENV= bun run apps/cli/src/index.ts info demos/nodejs/sample-document.docx --json > stdout 2> stderrfollowed byJSON.parse(stdout)Note: I also tried the full CLI typecheck, but current
mainhas unrelated typecheck failures in CLI conformance/type-surface files, so I did not treat that as validation for this small CLI-output fix.