Skip to content

fix: keep CLI JSON output parseable#3257

Closed
financialvice wants to merge 2 commits into
superdoc-dev:mainfrom
financialvice:cg/cli-json-stdout
Closed

fix: keep CLI JSON output parseable#3257
financialvice wants to merge 2 commits into
superdoc-dev:mainfrom
financialvice:cg/cli-json-stdout

Conversation

@financialvice
Copy link
Copy Markdown
Contributor

@financialvice financialvice commented May 12, 2026

Summary

  • add a CLI-level console diagnostic policy for machine/quiet modes
  • suppress global console.debug / console.log / console.info / console.warn / console.error while JSON or quiet commands execute, so lower-level library diagnostics cannot bypass the CLI io abstraction and pollute stdout
  • add a subprocess regression test that runs the CLI with NODE_ENV unset and parses superdoc info --json stdout directly

Why

superdoc <command> --json should produce machine-parseable JSON on stdout. In normal non-test CLI execution, SuperEditor telemetry logs this line with console.debug before the JSON envelope:

[super-editor] Telemetry: enabled

Because Node writes console.debug to stdout, automation that expected JSON.parse(stdout) failed even though the command itself succeeded. --quiet also could not suppress this because the log bypassed the CLI io abstraction.

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 30000
  • NODE_ENV= bun run apps/cli/src/index.ts info demos/nodejs/sample-document.docx --json > stdout 2> stderr followed by JSON.parse(stdout)
  • pre-commit hook: format, lint, and CLI build passed

Note: I also tried the full CLI typecheck, but current main has unrelated typecheck failures in CLI conformance/type-surface files, so I did not treat that as validation for this small CLI-output fix.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@caio-pizzol caio-pizzol self-assigned this May 13, 2026
Copy link
Copy Markdown
Contributor

@caiopizzol caiopizzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread apps/cli/src/index.ts
Comment on lines +180 to +211
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;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Thanks again for finding this and adding the subprocess regression. I opened #3678 as a maintainer follow-up based on this branch.

It keeps the same core fix intent and the subprocess test, but replaces the global console.* suppression with a narrower boundary: remove the telemetry debug log, keep invokeCommand() free of process-level I/O side effects, and only protect stdout from the executable CLI entrypoint.

I’m going to use that PR to land the fix.

@caio-pizzol caio-pizzol closed this Jun 8, 2026
@caio-pizzol
Copy link
Copy Markdown
Contributor

Thanks for digging into this. The underlying issue is real: CLI JSON output should stay parseable even when library diagnostics are emitted.

We are going to close this PR rather than merge it as-is because we want to implement the fix at the CLI executable boundary, not by applying broad console suppression inside the importable CLI APIs. That keeps run() / invokeCommand() side-effect free and lets us protect both JSON channels correctly: stdout for success responses and stderr for failure responses.

We will follow up here with a link once our PR is ready. Appreciate you surfacing the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants