Skip to content

fix: show errors with --quiet and --verbose/--debug, and stub the macOS browser in tests - #202

Open
pjcdawkins wants to merge 3 commits into
mainfrom
cli-197-quiet-errors-macos-browser-stub
Open

pjcdawkins wants to merge 3 commits into
mainfrom
cli-197-quiet-errors-macos-browser-stub

Conversation

@pjcdawkins

Copy link
Copy Markdown
Contributor

Two fixes found while working on the Go auth migration (#195), which also apply to the current auth:

  • exitWithError hid errors when --quiet was combined with --verbose or --debug, although the root pre-run ignores --quiet then. For example, an error starting the legacy CLI was not shown with -qv. Both now use isQuiet().
  • The integration tests' fake browser only stubbed xdg-open, but the CLI uses open on macOS, so login tests opened a real browser or waited for the login timeout there.

🤖 Generated with Claude Code

pjcdawkins and others added 2 commits October 8, 2026 14:13
The root pre-run ignores --quiet when --verbose or --debug is also set,
but exitWithError still hid errors, e.g. when the legacy CLI could not
start. Both now use isQuiet().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On macOS the CLI opens URLs with `open`, which the fake browser did not
stub, so login tests opened a real browser or waited for the timeout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔍 Full review · 3 files reviewed

🔵 Minor point

  • integration-tests/quiet_errors_test.go:33 — The wantError cases only check assert.NotEmpty(t, stderr). They never check that the startup error itself was printed. In the -q --debug case, any DEBUG line from debugLogf makes stderr non-empty, even without the exitWithError fix. For example, the legacy-index debug message in Execute or a debug line from a future component would do it. So the test would pass with the old viper.GetBool("quiet") check. It should assert that stderr contains the error text (e.g. "failed to initialize PHP CLI").
Verification
  • isQuiet() uses the same condition (quiet && !debug && !verbose) as the inline expression it replaces in PersistentPreRun, so the pre-run behaves as before.
  • exitWithError is only called from Run and the help func, after flag parsing and viper.BindPFlags, so isQuiet() sees the -v/--debug flags.
  • Setting TEST_CLI_TMP to a regular file makes Config.TempDir's MkdirAll fail before any c.debug call in CLIWrapper.init. Every case therefore exits 1 through the non-ExitError branch of exitWithError.
  • The legacy CLI's Url::getDefaultBrowser returns open on macOS without checking PATH and probes xdg-open on Linux, so stubbing both names covers both platforms.

The new TestQuietErrors integration test covers the quiet/verbose/debug cases. It runs in the integration-test CI job (make integration-test). Nothing tests the open stub on macOS, and the make test unit tests leave out integration-tests.

Review details
  • Commit: 63be299
  • Model: claude-opus-5-5

Review 1 of 10 for this pull request · View the full run

The test only checked that stderr was not empty, which a debug line
would also satisfy with --debug.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant