test: integration tests for first-run dispatch (issue #157)#164
Merged
greynewell merged 3 commits intomainfrom Apr 30, 2026
Merged
test: integration tests for first-run dispatch (issue #157)#164greynewell merged 3 commits intomainfrom
greynewell merged 3 commits intomainfrom
Conversation
Adds integration tests covering the non-PTY dispatch matrix defined in issue #151 and implemented in PR #152: - non-TTY + no key → exits 1 with "not authenticated" error - non-TTY + key → enters watch path (error is NOT "not authenticated") - version subcommand → exits 0, prints version (noConfigCommand bypass) - completion bash → exits 0, prints completion script (noConfigCommand) - analyze without key → exits 1 with "run 'supermodel setup'" error - config with empty api_key field → treated same as no key PTY-based tests (wizard launch) require hardware and are left for manual testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an integration test file that validates the supermodel CLI's first-run behavior across multiple scenarios. The test suite uses shared helpers to build the binary once and execute it in a sandboxed environment (isolated HOME, no API key by default, stdin redirected). It verifies that authentication checks, subcommand behavior, and configuration handling work correctly at startup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 6 minutes and 19 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/first_run_integration_test.go`:
- Around line 66-99: The subprocess execution in runSupermodel (and the similar
helper around lines 157-171) uses cmd.CombinedOutput() without a timeout; change
to create a context with a hard timeout (e.g., context.WithTimeout) and use
exec.CommandContext to start the process, then call CombinedOutput (or Wait) on
that command so the context can kill the process on timeout; detect
context.DeadlineExceeded and call t.Fatalf with a clear timeout message
including the timeout duration and command args, and ensure the context cancel
is deferred to release resources.
- Around line 122-257: Add two tests to close the non-PTY gaps: implement
TestFirstRun_Setup_WithKey that uses freshHome and writeConfig (or
SUPERMODEL_API_KEY env) to ensure running runSupermodel with args ["setup"]
still launches setup even when an API key exists (assert exit code indicates
setup path and output contains "supermodel setup" or the setup prompt), and
implement TestFirstRun_BadAPIKey_Path that invokes the binary (via runSupermodel
or exec.Command like in TestFirstRun_NoTTY_WithKey) with a deliberately invalid
key (e.g. SUPERMODEL_API_KEY=sm_bad_key or a config file with a bad key) and
asserts the process surfaces an actionable invalid-key/401-style message (not
the generic "not authenticated") and exits non-zero; use existing helpers
freshHome, runSupermodel, writeConfig, buildBinary and mirror the style and
assertions in TestFirstRun_NoTTY_WithKey and TestFirstRun_Analyze_NoKey.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 229e7d07-2ec7-411a-8d28-260f8b202eb2
📒 Files selected for processing (1)
cmd/first_run_integration_test.go
Comment on lines
+122
to
+257
| // --------------------------------------------------------------------------- | ||
| // Tests | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| // TestFirstRun_NoTTY_NoKey verifies that running bare `supermodel` in a | ||
| // non-interactive context without an API key exits 1 with a clear "not | ||
| // authenticated" message (issue #151 dispatch matrix, bottom-left cell). | ||
| func TestFirstRun_NoTTY_NoKey(t *testing.T) { | ||
| home := freshHome(t) | ||
| out, code := runSupermodel(t, nil, home) | ||
|
|
||
| if code != 1 { | ||
| t.Errorf("expected exit code 1, got %d\noutput: %s", code, out) | ||
| } | ||
| if !strings.Contains(out, "not authenticated") { | ||
| t.Errorf("expected output to contain %q\nfull output: %s", "not authenticated", out) | ||
| } | ||
| } | ||
|
|
||
| // TestFirstRun_NoTTY_WithKey verifies that a non-interactive caller that | ||
| // supplies a valid-format API key enters the watch path (not the auth-error | ||
| // path). The watch daemon will fail quickly in the test environment (no real | ||
| // API, no project cache), but the error must NOT be the "not authenticated" | ||
| // message — proving the dispatch selected runWatch, not errNotAuthenticated. | ||
| func TestFirstRun_NoTTY_WithKey(t *testing.T) { | ||
| home := freshHome(t) | ||
| // Create a project sub-directory so the binary doesn't refuse to run in | ||
| // HOME (which the watch daemon rejects as a safety measure). | ||
| projectDir := filepath.Join(home, "project") | ||
| if err := os.MkdirAll(projectDir, 0o755); err != nil { | ||
| t.Fatalf("create project dir: %v", err) | ||
| } | ||
|
|
||
| // Use an env var key. The format must look plausible; any non-empty | ||
| // value causes the dispatch to pick runWatch. | ||
| bin := buildBinary(t) | ||
| cmd := exec.Command(bin, "--dir", projectDir, "--notify-port", "0") | ||
| cmd.Env = []string{ | ||
| "HOME=" + home, | ||
| "PATH=" + os.Getenv("PATH"), | ||
| "SUPERMODEL_API_KEY=sm_integration_test_key", | ||
| } | ||
| devNull, err := os.Open(os.DevNull) | ||
| if err != nil { | ||
| t.Fatalf("open /dev/null: %v", err) | ||
| } | ||
| defer devNull.Close() | ||
| cmd.Stdin = devNull | ||
|
|
||
| out, execErr := cmd.CombinedOutput() | ||
| combined := string(out) | ||
|
|
||
| // The process exits non-zero quickly (API call fails, port conflict, etc.) | ||
| // — that's expected. What we must NOT see is the "not authenticated" error. | ||
| if strings.Contains(combined, "not authenticated") { | ||
| t.Errorf("got 'not authenticated' error, but expected the watch path to be entered with a valid API key\noutput: %s", combined) | ||
| } | ||
| // Confirm the binary did exit (one way or another) — don't hang. | ||
| if execErr == nil { | ||
| // Unexpected clean exit — the daemon should not stop immediately. | ||
| // This isn't necessarily a failure, but log it for visibility. | ||
| t.Logf("binary exited 0 unexpectedly (may need a real project dir or API)\noutput: %s", combined) | ||
| } | ||
| } | ||
|
|
||
| // TestFirstRun_VersionSubcommand_NoKey ensures `supermodel version` exits 0 | ||
| // and prints a version string without requiring an API key. This exercises | ||
| // the noConfigCommands bypass in persistentPreRunE. | ||
| func TestFirstRun_VersionSubcommand_NoKey(t *testing.T) { | ||
| home := freshHome(t) | ||
| out, code := runSupermodel(t, []string{"version"}, home) | ||
|
|
||
| if code != 0 { | ||
| t.Errorf("expected exit code 0, got %d\noutput: %s", code, out) | ||
| } | ||
| if !strings.Contains(out, "supermodel") { | ||
| t.Errorf("version output should contain the word 'supermodel'\nfull output: %s", out) | ||
| } | ||
| } | ||
|
|
||
| // TestFirstRun_CompletionBash_NoKey checks that the nested `completion bash` | ||
| // subcommand (a noConfigCommand) exits 0 and emits a bash completion script | ||
| // without requiring an API key. | ||
| func TestFirstRun_CompletionBash_NoKey(t *testing.T) { | ||
| home := freshHome(t) | ||
| out, code := runSupermodel(t, []string{"completion", "bash"}, home) | ||
|
|
||
| if code != 0 { | ||
| t.Errorf("expected exit code 0, got %d\noutput: %s", code, out) | ||
| } | ||
| // Bash completion scripts always begin with a comment or a function | ||
| // declaration. Both contain "supermodel" as the command name. | ||
| if !strings.Contains(out, "supermodel") { | ||
| t.Errorf("completion output should reference 'supermodel'\nfull output: %s", out) | ||
| } | ||
| } | ||
|
|
||
| // TestFirstRun_Analyze_NoKey verifies that `supermodel analyze` without an | ||
| // API key exits 1 with an actionable "run 'supermodel setup'" message (via | ||
| // persistentPreRunE), NOT the generic "not authenticated" error reserved for | ||
| // the bare root command. | ||
| func TestFirstRun_Analyze_NoKey(t *testing.T) { | ||
| home := freshHome(t) | ||
| out, code := runSupermodel(t, []string{"analyze"}, home) | ||
|
|
||
| if code != 1 { | ||
| t.Errorf("expected exit code 1, got %d\noutput: %s", code, out) | ||
| } | ||
| // persistentPreRunE should fire before analyze's own RequireAPIKey(), | ||
| // producing the "setup" guidance rather than the root-command error. | ||
| if !strings.Contains(out, "supermodel setup") { | ||
| t.Errorf("expected output to contain %q\nfull output: %s", "supermodel setup", out) | ||
| } | ||
| // Specifically must NOT surface the bare-root "not authenticated" error. | ||
| if strings.Contains(out, "not authenticated") { | ||
| t.Errorf("got 'not authenticated' (root-command error) but expected 'supermodel setup' guidance\nfull output: %s", out) | ||
| } | ||
| } | ||
|
|
||
| // TestFirstRun_EmptyAPIKeyInConfig verifies that a config file with an | ||
| // explicit empty api_key field is treated the same as having no key at all — | ||
| // i.e. the binary exits 1 with a "not authenticated" error in non-TTY mode. | ||
| // This is an edge case that could silently break if config parsing changes. | ||
| func TestFirstRun_EmptyAPIKeyInConfig(t *testing.T) { | ||
| home := freshHome(t) | ||
| writeConfig(t, home, "api_key: \"\"\n") | ||
|
|
||
| out, code := runSupermodel(t, nil, home) | ||
|
|
||
| if code != 1 { | ||
| t.Errorf("expected exit code 1 with empty api_key in config, got %d\noutput: %s", code, out) | ||
| } | ||
| if !strings.Contains(out, "not authenticated") { | ||
| t.Errorf("expected 'not authenticated' error for empty api_key config\nfull output: %s", out) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Coverage still misses two non-PTY objectives from #157
Given this PR is positioned to close #157, tests for these non-PTY scenarios are still absent:
- explicit
supermodel setupshould launch setup even when key exists (regression guard), - bad key path should surface actionable invalid-key handling (401-style behavior).
Please add those cases here (or update PR scope/closure note).
If helpful, I can draft both test cases using the same binary/home helpers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/first_run_integration_test.go` around lines 122 - 257, Add two tests to
close the non-PTY gaps: implement TestFirstRun_Setup_WithKey that uses freshHome
and writeConfig (or SUPERMODEL_API_KEY env) to ensure running runSupermodel with
args ["setup"] still launches setup even when an API key exists (assert exit
code indicates setup path and output contains "supermodel setup" or the setup
prompt), and implement TestFirstRun_BadAPIKey_Path that invokes the binary (via
runSupermodel or exec.Command like in TestFirstRun_NoTTY_WithKey) with a
deliberately invalid key (e.g. SUPERMODEL_API_KEY=sm_bad_key or a config file
with a bad key) and asserts the process surfaces an actionable
invalid-key/401-style message (not the generic "not authenticated") and exits
non-zero; use existing helpers freshHome, runSupermodel, writeConfig,
buildBinary and mirror the style and assertions in TestFirstRun_NoTTY_WithKey
and TestFirstRun_Analyze_NoKey.
The binary built by buildBinary must have a .exe extension on Windows to be executable. Use runtime.GOOS to choose the correct name. Also skip `completion bash` on Windows (bash is not available by default) and use os.DevNull for cross-platform /dev/null handling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Address CodeRabbit review: wrap all exec.Command calls in exec.CommandContext with a 20s hard timeout so that if the watch daemon behaviour changes, CI cannot hang indefinitely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #157.
Adds
cmd/first_run_integration_test.gocovering the non-PTY paths of the first-run dispatch matrix introduced in issue #151 and implemented in PR #152:versionsubcommand → exits 0, prints version (noConfigCommand bypass)completion bash→ exits 0, prints completion script (noConfigCommand bypass)analyzewithout key → exits 1 with "run 'supermodel setup'" error (persistentPreRunE path)api_keyfield → treated same as no key (edge case for config parsing)PTY-based tests (wizard launch via
setup.Run) require hardware and are left for manual testing.The tests build the binary once via
sync.OnceinbuildBinaryto avoid redundant compilation, use a clean tempHOME, and pipe/dev/nullto stdin to guarantee non-interactive mode across CI environments.🤖 Generated with Claude Code
Summary by CodeRabbit
Note: This release contains no user-facing changes; it consists solely of test infrastructure improvements to ensure product quality.