fix: suppress UI spinners and telemetry notices when --json is used#742
fix: suppress UI spinners and telemetry notices when --json is used#742CosticaPuntaru wants to merge 1 commit intoFission-AI:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request implements clean JSON output for the OpenSpec CLI. When Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Modified status, instructions, templates, and new-change commands to conditionally start the ora spinner only when not in JSON mode. - Updated the global preAction hook to detect the --json flag and pass a silent flag to telemetry notices. - Added --json support to 'new change' command for machine-readable result reporting. - Added a new spec for machine-readable-output capability. This ensures stdout/stderr pollution does not break JSON parsing for automated tools and AI agents.
CosticaPuntaru
force-pushed
the
fix/clean-json-output
branch
from
315b2d0 to
8c8ab0a
Compare
Greptile SummaryThis PR successfully addresses the issue of UI pollution when
One issue found: The error handler in Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 8c8ab0a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/telemetry/index.ts (1)
122-145:⚠️ Potential issue | 🟠 Major
noticeSeenis persisted even when the notice was suppressed, so the user may never see it.When
silentistrue(JSON mode), the notice is correctly suppressed, but line 141 still marksnoticeSeen: true. If the user's first interaction with the CLI happens to use--json, they will never be shown the telemetry notice on subsequent non-JSON runs. This is a privacy/compliance concern — users should be informed about data collection at least once.Move the
noticeSeenpersistence inside the block that actually displays the notice.Proposed fix
if (!options.silent) { // Display notice console.log( 'Note: OpenSpec collects anonymous usage stats. Opt out: OPENSPEC_TELEMETRY=0' ); - } - // Mark as seen - await updateTelemetryConfig({ noticeSeen: true }); + // Mark as seen only when actually displayed + await updateTelemetryConfig({ noticeSeen: true }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/telemetry/index.ts` around lines 122 - 145, The function maybeShowTelemetryNotice currently marks noticeSeen via updateTelemetryConfig({ noticeSeen: true }) regardless of options.silent; move the persistence so it only runs when the notice is actually displayed. Inside maybeShowTelemetryNotice, after you check if (!options.silent) and after the console.log that shows the notice, call updateTelemetryConfig({ noticeSeen: true }); remove or skip the existing unconditional updateTelemetryConfig call so that config.noticeSeen is only set when the notice was shown to the user. Ensure you still await getTelemetryConfig() and preserve the early returns for !isTelemetryEnabled() and config.noticeSeen.src/cli/index.ts (1)
504-513:⚠️ Potential issue | 🟠 Major
console.log()in the catch block emits an empty line to stdout in JSON mode, corrupting output.
ora's default stream isprocess.stderr, soora().fail()is safe. However, the precedingconsole.log()(line 509) writes a bare newline to stdout. After adding--json, whennewChangeCommandthrows and rethrows, this catch block runs and stdout receives an empty line — any consumer doingJSON.parse(stdout)will throw.Fix the catch block to branch on
options.json:🐛 Proposed fix
.action(async (name: string, options: NewChangeOptions) => { try { await newChangeCommand(name, options); } catch (error) { - console.log(); - ora().fail(`Error: ${(error as Error).message}`); + if (options.json) { + console.log(JSON.stringify({ success: false, error: (error as Error).message }, null, 2)); + } else { + console.log(); + ora().fail(`Error: ${(error as Error).message}`); + } process.exit(1); } });This also removes the need for
new-change.tsto special-case the error path, so thethrow errorin its catch can remain. Note the sameconsole.log()pattern exists in other commands' catch blocks (status,instructions,templates) — those may warrant the same treatment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 504 - 513, The catch block in the action handler for the CLI command prints a bare newline to stdout before calling ora().fail, which corrupts JSON output; modify the action's catch to avoid writing that newline when options.json is true (i.e., only call console.log() when !options.json) and keep ora().fail((error as Error).message) for stderr handling; apply the same pattern to the other command handlers that use the same catch pattern (status, instructions, templates) so JSON output isn't polluted.
🧹 Nitpick comments (2)
openspec/changes/archive/2026-02-22-clean-json-output/design.md (1)
31-41: Design pattern differs slightly from the actual implementation.The documented pattern guards
spinner.stop()withif (!options.json)inside afinallyblock, but the actual commands callspinner.stop()unconditionally in both the success and catch paths (nofinally). The implementation is fine sinceora'sstop()is safe on an unstarted spinner, but you may want to reconcile the doc to match what was actually shipped—or vice versa—so future contributors follow a single pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-02-22-clean-json-output/design.md` around lines 31 - 41, Documentation and implementation diverge: the doc shows guarding spinner.stop() with if (!options.json) inside a finally, but the shipped code calls spinner.stop() unconditionally in success and catch paths; reconcile them by updating the design.md snippet (or the implementation) so both match. Either change the design.md example to show the actual pattern using ora spinner with unconditional spinner.stop() (noting ora.stop() is safe when not started), or change the implementation to use a single try/finally block that only stops the spinner when !options.json; update references to spinner, options.json, ora, try/finally, and spinner.stop() accordingly so future readers and code follow the same pattern.openspec/specs/machine-readable-output/spec.md (1)
27-34: Error scenario leaves stdout contract undefined — consider specifying it.The scenario declares that on failure: exit code is non-zero and the error goes to stderr. It doesn't state what stdout contains (empty, or a
{ success: false, error: "..." }JSON object). Machine-readable consumers parsing stdout will get an empty line (seesrc/cli/index.tscatch block below), which failsJSON.parse. Either document that stdout is empty on error, or prescribe a structured JSON error body on stdout for consistency with the success path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/machine-readable-output/spec.md` around lines 27 - 34, The spec's error scenario is underspecified about stdout on failure; update the spec.md "3. Error Handling" scenario to explicitly state that when a command fails with --json the stdout MUST be empty (no JSON) and the error text MUST be written to stderr with a non-zero exit code; then update the implementation referenced (the catch block in src/cli/index.ts) to stop emitting any stdout content on error and instead write the error message only to stderr and exit non-zero so machine-readable consumers won't receive broken/empty JSON from stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/archive/2026-02-22-clean-json-output/specs/machine-readable-output/spec.md`:
- Line 1: The header "Capability: Machine Readable Output" should be
hyphenated—replace the title line so it reads "Machine-Readable Output" (i.e.,
change the header text containing "Capability: Machine Readable Output" to use
"Machine-Readable Output") and apply the identical correction to the canonical
spec copy as well as this archive copy so both titles match exactly.
In `@openspec/specs/machine-readable-output/spec.md`:
- Line 1: The heading "Machine Readable Output" should use a hyphen as a
compound modifier; update the heading text (the top-level title string "#
Capability: Machine Readable Output") to "# Capability: Machine-Readable Output"
so the compound adjective is hyphenated consistently throughout the spec.
In `@src/cli/index.ts`:
- Around line 77-82: The telemetry notice is being marked seen even when
silenced by --json; modify maybeShowTelemetryNotice so that
updateTelemetryConfig({ noticeSeen: true }) is only called inside the branch
that displays the notice (i.e., only when options.silent is false).
Specifically, in the maybeShowTelemetryNotice function move the
updateTelemetryConfig call into the if (!options.silent) block (or add the
conditional around it) and remove any unconditional updateTelemetryConfig
invocation so noticeSeen is not set when the notice is suppressed.
In `@src/commands/workflow/new-change.ts`:
- Around line 70-75: In the catch block where you currently call spinner.fail
and then rethrow (checking options.json and using spinner.fail(`Failed to create
change '${name}'`)), ensure that when options.json is true you emit a structured
JSON error object to stdout (e.g., { error: true, message: error.message,
details: ... }) and do not rethrow; specifically, replace the throw error path
for options.json === true with a console.log of the JSON error payload and a
graceful return (or process.exit with non-zero status) so the upstream handler
doesn't print an empty line and JSON consumers receive a valid error body, while
keeping spinner.fail + throw for non-JSON mode.
---
Outside diff comments:
In `@src/cli/index.ts`:
- Around line 504-513: The catch block in the action handler for the CLI command
prints a bare newline to stdout before calling ora().fail, which corrupts JSON
output; modify the action's catch to avoid writing that newline when
options.json is true (i.e., only call console.log() when !options.json) and keep
ora().fail((error as Error).message) for stderr handling; apply the same pattern
to the other command handlers that use the same catch pattern (status,
instructions, templates) so JSON output isn't polluted.
In `@src/telemetry/index.ts`:
- Around line 122-145: The function maybeShowTelemetryNotice currently marks
noticeSeen via updateTelemetryConfig({ noticeSeen: true }) regardless of
options.silent; move the persistence so it only runs when the notice is actually
displayed. Inside maybeShowTelemetryNotice, after you check if (!options.silent)
and after the console.log that shows the notice, call updateTelemetryConfig({
noticeSeen: true }); remove or skip the existing unconditional
updateTelemetryConfig call so that config.noticeSeen is only set when the notice
was shown to the user. Ensure you still await getTelemetryConfig() and preserve
the early returns for !isTelemetryEnabled() and config.noticeSeen.
---
Nitpick comments:
In `@openspec/changes/archive/2026-02-22-clean-json-output/design.md`:
- Around line 31-41: Documentation and implementation diverge: the doc shows
guarding spinner.stop() with if (!options.json) inside a finally, but the
shipped code calls spinner.stop() unconditionally in success and catch paths;
reconcile them by updating the design.md snippet (or the implementation) so both
match. Either change the design.md example to show the actual pattern using ora
spinner with unconditional spinner.stop() (noting ora.stop() is safe when not
started), or change the implementation to use a single try/finally block that
only stops the spinner when !options.json; update references to spinner,
options.json, ora, try/finally, and spinner.stop() accordingly so future readers
and code follow the same pattern.
In `@openspec/specs/machine-readable-output/spec.md`:
- Around line 27-34: The spec's error scenario is underspecified about stdout on
failure; update the spec.md "3. Error Handling" scenario to explicitly state
that when a command fails with --json the stdout MUST be empty (no JSON) and the
error text MUST be written to stderr with a non-zero exit code; then update the
implementation referenced (the catch block in src/cli/index.ts) to stop emitting
any stdout content on error and instead write the error message only to stderr
and exit non-zero so machine-readable consumers won't receive broken/empty JSON
from stdout.
| @@ -0,0 +1,34 @@ | |||
| # Capability: Machine Readable Output | |||
There was a problem hiding this comment.
Same hyphenation nit as the canonical spec.
Title should be Machine-Readable Output. Based on learnings, this delta creates the spec from scratch, so the correction should be applied identically in both this archive copy and openspec/specs/machine-readable-output/spec.md.
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Use a hyphen to join words.
Context: # Capability: Machine Readable Output ## Purpose Allow autom...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/archive/2026-02-22-clean-json-output/specs/machine-readable-output/spec.md`
at line 1, The header "Capability: Machine Readable Output" should be
hyphenated—replace the title line so it reads "Machine-Readable Output" (i.e.,
change the header text containing "Capability: Machine Readable Output" to use
"Machine-Readable Output") and apply the identical correction to the canonical
spec copy as well as this archive copy so both titles match exactly.
| @@ -0,0 +1,34 @@ | |||
| # Capability: Machine Readable Output | |||
There was a problem hiding this comment.
Machine-Readable Output — add hyphen to compound modifier.
"Machine Readable Output" should be "Machine-Readable Output" when used as a compound modifier (title/heading).
📝 Proposed fix
-# Capability: Machine Readable Output
+# Capability: Machine-Readable Output📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Capability: Machine Readable Output | |
| # Capability: Machine-Readable Output |
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Use a hyphen to join words.
Context: # Capability: Machine Readable Output ## Purpose Allow autom...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/specs/machine-readable-output/spec.md` at line 1, The heading
"Machine Readable Output" should use a hyphen as a compound modifier; update the
heading text (the top-level title string "# Capability: Machine Readable
Output") to "# Capability: Machine-Readable Output" so the compound adjective is
hyphenated consistently throughout the spec.
| // Check if action command is requesting JSON output | ||
| const actionOpts = actionCommand.opts(); | ||
| const isJson = actionOpts.json === true; | ||
|
|
||
| // Show first-run telemetry notice (if not seen) | ||
| await maybeShowTelemetryNotice(); | ||
| await maybeShowTelemetryNotice({ silent: isJson }); |
There was a problem hiding this comment.
Telemetry notice is permanently suppressed for users whose first run uses --json.
maybeShowTelemetryNotice({ silent: true }) does not display the notice, but looking at src/telemetry/index.ts (lines 136–143), updateTelemetryConfig({ noticeSeen: true }) is called unconditionally — it runs outside the if (!options.silent) guard. On a first run with --json, the notice is silenced but noticeSeen is set to true, so the notice is never shown on any subsequent non-JSON invocation either. Users who start with --json are never informed about telemetry collection.
Fix in src/telemetry/index.ts:
// In maybeShowTelemetryNotice:
if (!options.silent) {
console.log(
'Note: OpenSpec collects anonymous usage stats. Opt out: OPENSPEC_TELEMETRY=0'
);
await updateTelemetryConfig({ noticeSeen: true }); // ← move inside this block
}
// Remove the unconditional call below🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/index.ts` around lines 77 - 82, The telemetry notice is being marked
seen even when silenced by --json; modify maybeShowTelemetryNotice so that
updateTelemetryConfig({ noticeSeen: true }) is only called inside the branch
that displays the notice (i.e., only when options.silent is false).
Specifically, in the maybeShowTelemetryNotice function move the
updateTelemetryConfig call into the if (!options.silent) block (or add the
conditional around it) and remove any unconditional updateTelemetryConfig
invocation so noticeSeen is not set when the notice is suppressed.
| } catch (error) { | ||
| spinner.fail(`Failed to create change '${name}'`); | ||
| if (!options.json) { | ||
| spinner.fail(`Failed to create change '${name}'`); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Error path in JSON mode doesn't emit a structured error body before rethrowing.
When options.json is true, the function skips spinner.fail but still rethrows. The upstream catch block in src/cli/index.ts then executes console.log(), writing an empty line to stdout before calling ora().fail() on stderr. Any tool parsing stdout as JSON will fail — the spec's Requirement 3 is violated in the error case.
The cleanest fix is in the new change action handler in src/cli/index.ts (see the comment there), but an alternative is to fully resolve the error here without rethrowing in JSON mode:
🐛 Alternative fix (handle JSON error locally)
} catch (error) {
- if (!options.json) {
- spinner.fail(`Failed to create change '${name}'`);
- }
- throw error;
+ if (options.json) {
+ console.log(JSON.stringify({ success: false, error: (error as Error).message }, null, 2));
+ process.exitCode = 1;
+ return;
+ }
+ spinner.fail(`Failed to create change '${name}'`);
+ throw error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| spinner.fail(`Failed to create change '${name}'`); | |
| if (!options.json) { | |
| spinner.fail(`Failed to create change '${name}'`); | |
| } | |
| throw error; | |
| } | |
| } catch (error) { | |
| if (options.json) { | |
| console.log(JSON.stringify({ success: false, error: (error as Error).message }, null, 2)); | |
| process.exitCode = 1; | |
| return; | |
| } | |
| spinner.fail(`Failed to create change '${name}'`); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/workflow/new-change.ts` around lines 70 - 75, In the catch block
where you currently call spinner.fail and then rethrow (checking options.json
and using spinner.fail(`Failed to create change '${name}'`)), ensure that when
options.json is true you emit a structured JSON error object to stdout (e.g., {
error: true, message: error.message, details: ... }) and do not rethrow;
specifically, replace the throw error path for options.json === true with a
console.log of the JSON error payload and a graceful return (or process.exit
with non-zero status) so the upstream handler doesn't print an empty line and
JSON consumers receive a valid error body, while keeping spinner.fail + throw
for non-JSON mode.
|
|
||
| ## Requirements | ||
|
|
||
| ### 1. Spinner Suppression |
There was a problem hiding this comment.
Can we switch these numbered sections to the standard requirement format? Using headings like ### Requirement: ... makes this consistent with OpenSpec conventions and easier for tooling to parse.
|
|
||
| #### Scenario: First run with JSON | ||
| - **Given** a new environment where the telemetry notice hasn't been shown | ||
| - **When** running `openspec list --json` |
There was a problem hiding this comment.
I agree with suppressing the notice in JSON mode. Can we make deferred notice behavior explicit here too: suppress on JSON runs, do not mark noticeSeen yet, and show it on the first later non-JSON run. That keeps JSON clean and still guarantees user disclosure.
|
|
||
| ### New Capabilities | ||
|
|
||
| - `machine-readable-output`: Ensures all CLI commands providing a `--json` flag produce clean, parseable stdout without terminal visual effects or side-channel messages. |
There was a problem hiding this comment.
Nice fix direction. One scope detail: this looks like a modification of existing capabilities, not a brand new one. We are changing telemetry notice behavior and workflow command output behavior. Could we move this to Modified Capabilities and list telemetry plus cli-artifact-workflow?
| - **Then** the telemetry notice is NOT printed to stdout | ||
| - **And** only the JSON change list is printed | ||
|
|
||
| ### 3. Error Handling |
There was a problem hiding this comment.
Could we define stdout behavior on failure more explicitly for JSON mode? Suggest: stdout stays empty, error text goes to stderr, and exit code is non-zero. That gives consumers a clear contract.
| } | ||
|
|
||
| // Mark as seen | ||
| await updateTelemetryConfig({ noticeSeen: true }); |
There was a problem hiding this comment.
Small logic issue here: noticeSeen is saved even when silent mode suppresses the notice. If a user starts with --json, they may never see the disclosure. Can we move this update inside the if (!options.silent) block?
| @@ -0,0 +1,34 @@ | |||
| # Capability: Machine Readable Output | |||
There was a problem hiding this comment.
This archived change spec looks like a full future-state document. Current conventions for change specs use delta sections like ## ADDED Requirements and ## MODIFIED Requirements. Could we convert this to delta format so it stays consistent with tooling and other archived changes?
| .description('Create a new change directory') | ||
| .option('--description <text>', 'Description to add to README.md') | ||
| .option('--schema <name>', `Workflow schema to use (default: ${DEFAULT_SCHEMA})`) | ||
| .option('--json', 'Output as JSON (success/failure details)') |
There was a problem hiding this comment.
Nice addition of --json here. One related fix is still needed in the catch block just below: it currently does console.log() before ora().fail(...). In JSON mode that writes to stdout and can break parsers. Can we guard the spacer with if (!options.json) and keep error output on stderr only?
2 participants
Description
This PR addresses the issue where OpenSpec CLI output is polluted with spinner text and telemetry notices even when the
--jsonflag is active. This pollution causes parsing errors for automation tools and AI agents.Changes
status,instructions,templates, andnew-changecommands to skip starting theoraspinner if thejsonoption is present.preActionhook insrc/cli/index.tsto detect the--jsonflag and silence the telemetry notice.--jsonsupport to thenew changecommand to provide machine-readable success/failure details.openspec/specs/machine-readable-output/spec.mdto document this behavior.Testing
openspec status --jsonproduces clean JSON.openspec instructions apply --jsonproduces clean JSON.--json.openspec new change test --jsonproduces clean JSON.Summary by CodeRabbit
Release Notes
--jsonflag to CLI commands for clean, machine-readable JSON output--jsonis used