feat(nubi): add query timeout flag, resilient polling, and recovery hints - #115
feat(nubi): add query timeout flag, resilient polling, and recovery hints#115blue4209211 wants to merge 2 commits into
Conversation
…ints - Add --timeout / -t flag to nbctl nubi query (default: 0 / unlimited) - Add resilient polling in nubi query to retry transient errors and update spinner - Display Session ID, Conversation ID, nbctl nubi get command, and browser URL when query times out or is canceled - Emit structured JSON error payload when -o json / --format json is requested on trigger failure or timeout - Provide contextual diagnostic hints when account-level access is denied - Fix RunWithMockServer test isolation by ensuring NBCTL_TESTING=true is set - Add comprehensive test coverage for query timeouts, retries, and access errors
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --timeout flag to the nubi query command, improves error handling for timeouts and cancellations (with detailed user guidance for both text and JSON outputs), and refactors the polling mechanism to tolerate transient errors and use a ticker. It also adds comprehensive tests for these scenarios. The review feedback suggests improving sub-second duration rounding to avoid displaying 0s and enhancing context error checks in the polling loop to ensure robust handling of cancellations and timeouts.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a '--timeout' flag to the 'nubi query' command, enhances error handling and recovery messaging for timeouts and cancellations (supporting both text and JSON outputs), and refactors the polling mechanism to use a ticker with transient error retries. The review feedback highlights several improvement opportunities, including addressing a potential data race with the spinner suffix, avoiding thread-unsafe dynamic environment variable modifications in tests, robustly distinguishing trigger errors from polling errors instead of relying on string matching, and ensuring non-zero exit codes are returned on failure in JSON mode.
| } | ||
|
|
||
| // If polling failed after triggering investigation, provide recovery information | ||
| if nubiClient.SessionID != "" && !strings.Contains(err.Error(), "triggering investigation") { |
There was a problem hiding this comment.
Relying on strings.Contains(err.Error(), "triggering investigation") to distinguish between trigger failures and polling failures is fragile. If the error wrapping message in triggerAndPoll is ever modified or translated, this check will silently break, potentially leading to misleading recovery hints being displayed. Consider defining a custom error type (e.g., TriggerError) or using a structured error check to make this distinction robust.
| if s.spinner != nil && statusText != "" { | ||
| s.spinner.Suffix = " " + statusText | ||
| } |
There was a problem hiding this comment.
Updating s.spinner.Suffix directly while the spinner is active (running in its own goroutine) introduces a data race, as briandowns/spinner does not synchronize access to its public fields. This can trigger the Go race detector (go test -race). Consider checking if the spinner is active before updating, or using a thread-safe wrapper/alternative if race-free execution is required.
| _ = os.Setenv("NBCTL_TESTING", "true") | ||
| defer func() { _ = os.Unsetenv("NBCTL_TESTING") }() |
There was a problem hiding this comment.
Calling os.Setenv and os.Unsetenv dynamically inside test helper functions is not thread-safe and can cause concurrent map write panics if tests are run in parallel or with the -race flag.
Since pkg/testutil is a test-only package, you can safely set NBCTL_TESTING once in an init() function at the package level and remove these dynamic set/unset calls entirely.
For example, add this to pkg/testutil/helpers.go:
func init() {
os.Setenv("NBCTL_TESTING", "true")
}And then remove the os.Setenv and os.Unsetenv calls from RunWithMockServer and RunWithSimpleGraphQL.
| jsonResp["hint"] = hint | ||
| } | ||
| format.GetFormat().Print(jsonResp) | ||
| return nil |
There was a problem hiding this comment.
Returning nil (which exits with status 0) when a query execution or trigger fails in JSON mode makes it difficult for scripts, CI/CD pipelines, or automation tools to detect failures using standard exit codes. Consider returning a non-zero exit code on failure even when outputting JSON, perhaps by returning a custom error that is silenced by Cobra (using SilenceErrors: true) so it doesn't print raw error text to stderr.
1 participant
Summary of Changes
1. Add
--timeout/-tflag tonbctl nubi query--timeout 5m,-t 2m).0(unlimited / wait until complete or canceled, preserving existing behavior).2. Resilient Polling & Live Progress Updates
statusTextfromGetConversation.3. Clear Post-Timeout & Post-Cancellation Guidance
When a query times out or is canceled:
Query timed out after <duration>.orRequest canceled.--format json/-o json):4. Machine-Readable Trigger Errors & Access Hints
--async -o jsonor synchronous-o jsonencounters a trigger error (such asapi: user does not have access), it now outputs structured JSON onstdoutinstead of dumping raw Go error text tostderr.nbctl auth assign-role.5. Test Isolation Fix in
pkg/testutil/helpers.goRunWithMockServersetsNBCTL_TESTING=trueduring mock execution to prevent tests from reading~/.nudgebee/configand connecting to production.Verification
cmd/nubi_test.gocovering timeout in text/JSON, transient error retry, and access denied errors.go test ./...).make lint/golangci-lint runwith 0 issues).