refactor: decouple --output json from non-interactive mode#41
Conversation
Remove the PersistentPreRunE override that forced IsTerminalFunc to return false whenever --output json was set. Interactive prompts now run normally in a TTY regardless of output format; JSON serialisation and TTY detection are fully orthogonal. Also collapse the JSON-specific error branch in resolveRequestIDInteractive to the standard ErrNotInteractive path, and delete the now-obsolete TestResolveRequestIDInteractive_JSONMode test.
There was a problem hiding this comment.
Pull request overview
Decouples --output json from interactivity so JSON is treated as a serialization format rather than forcing non-interactive behavior across all commands.
Changes:
- Removed the root
PersistentPreRunEhook that forcedui.IsTerminalFuncto report non-interactive mode when--output jsonis set. - Simplified
resolveRequestIDInteractiveto return the standardui.ErrNotInteractivepath (no JSON-specific error branch). - Updated docs/changelog and removed the now-obsolete JSON-mode request picker test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/root.go | Stops globally overriding terminal detection when --output json is used. |
| cmd/request_picker.go | Removes JSON-specific non-interactive error handling in request ID picker resolution. |
| cmd/request_picker_test.go | Deletes the JSON-mode-specific test case for request ID picker behavior. |
| CLAUDE.md | Updates internal documentation to reflect JSON output no longer affecting interactivity. |
| CHANGELOG.md | Notes the behavior change for --output json regarding interactive prompts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if outputFormat != "text" && outputFormat != "json" { | ||
| return fmt.Errorf("invalid output format %q: must be one of: text, json", outputFormat) | ||
| } | ||
| if isJSONOutput() { | ||
| ui.IsTerminalFunc = func(fd uintptr) bool { return false } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Now that --output json no longer forces non-interactive mode, some commands can enter interactive flows while still expecting machine-readable output. At least cmd/revoke.go prints human text to stdout on the “no active sessions” and “revocation canceled” paths, which will break consumers expecting JSON when -o json is set. Consider auditing interactive commands so that in JSON mode either (a) non-JSON messages go to stderr and stdout remains valid JSON, or (b) cancellation/empty states return a JSON payload (or a non-zero error) instead of plain text.
| } | ||
| return nil | ||
| }, | ||
| RunE: runFn, |
There was a problem hiding this comment.
Behavior around --output json affecting interactivity is a cross-cutting contract change, but there isn’t a test guarding it anymore (the JSON-mode request picker test was removed). Add a regression test that executes a trivial command with --output json and asserts ui.IsTerminalFunc is not mutated / interactive detection remains unchanged, so future refactors don’t reintroduce global side effects.
| RunE: runFn, | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| originalIsTerminalFunc := ui.IsTerminalFunc | |
| defer func() { | |
| ui.IsTerminalFunc = originalIsTerminalFunc | |
| }() | |
| return runFn(cmd, args) | |
| }, |
Closes #40
Summary
PersistentPreRunEoverride that forcedui.IsTerminalFuncto returnfalsewhenever--output jsonwas set — this affected all commands globallyresolveRequestIDInteractiveto the standardErrNotInteractivepathTestResolveRequestIDInteractive_JSONModetestBehaviour change
--output jsonis now a pure serialisation flag. Interactive pickers and prompts (e.g.grant request get -o jsonwith no ID,grant request submit -o jsonwithout--target/--role) work in a TTY, writing prompts to stderr and JSON results to stdout. When stdin is genuinely not a TTY, all prompts still returnErrNotInteractivewith the appropriate bypass-flag hint — unchanged.Test plan
make testpassesmake lintcleangrant request get -o jsonin a TTY → interactive picker opens; selected request prints as JSONgrant request get -o json < /dev/null→ returnsErrNotInteractivewith hintgrant request get <id> -o json→ prints JSON (no regression)