Add first-run detection and install-time setup wizard#49
Conversation
- install.sh: run `supermodel setup` automatically after install when attached to a TTY (skips piped/CI installs) - cmd/root.go: PersistentPreRunE nudges users without a config to run `supermodel setup`; no-op for setup/login/logout/version/help/completion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdded a root-level Cobra PersistentPreRunE that skips config loading for an explicit exemption list; for other commands it loads config and enforces presence of an API key. Installer now invokes Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
- Authentication is now inline: if no API key, opens browser OAuth flow immediately instead of redirecting user to run `supermodel login` first - Removed the file mode toggle step — file mode is the default, no question needed; the product is explained in the header instead - 3 clean steps: Authenticate → Repository → Claude Code hook → Analyze - Punchier copy, fewer words per step - Removed unused boolPtr and selectMenu helpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add back the file mode Y/N step with a concise explanation of what .graph files are and how agents use them - Replace the "run analyze now?" prompt with an unconditional supermodel watch launch — explains what watch does and how to stop/restart before handing over the terminal Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/root.go (1)
37-42: Optional: avoid the secondconfig.Load()on every gated command.This pre-run now reads config once, and commands like
cmd/analyze.goread it again inRunE. Passing the loadedcfgthroughcmd.Context()would keep the gate and drop the duplicate file/env parse on every call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 37 - 42, Pre-load the config once in the root command pre-run and attach it to the cobra command context so subcommands stop calling config.Load() again: after cfg, err := config.Load() in the root pre-run, create a new context via context.WithValue(ctx, configKey, cfg) and call cmd.SetContext(newCtx); then update gated commands (e.g., Analyze RunE) to read cfg from cmd.Context().Value(configKey) instead of calling config.Load(), and remove the duplicated file/env parse.
🤖 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/root.go`:
- Around line 37-40: The current check conflates config load errors with missing
API key; change the logic around config.Load() in root.go so you first handle a
non-nil err from config.Load() by printing the actual error (include err) to
stderr and exiting, and only if err is nil then inspect cfg.APIKey and print the
"Run 'supermodel setup'..." nudge when cfg.APIKey == ""; reference the cfg
variable and config.Load() call and ensure the err branch reports the real error
instead of the setup message.
- Around line 13-20: The noConfigCommands allowlist misses Cobra's internal
completion commands so PersistentPreRunE tries to load config for "__complete"
and crashes; update the map noConfigCommands to include the keys "__complete"
and "__completeNoDesc", or modify the root PersistentPreRunE check to walk the
command's parent chain (using cmd.Name() on each ancestor) and skip config
loading if any ancestor name is in noConfigCommands; reference the
noConfigCommands variable and the root PersistentPreRunE handler when making the
change.
In `@install.sh`:
- Around line 76-79: The setup wizard is using stdout terminal check ([ -t 1 ])
but promptui reads stdin, which is the piped shell; modify the logic around the
installation invocation (the block that calls "$INSTALL_DIR/$BINARY" setup) to
instead verify /dev/tty exists and is readable, and when launching the setup
command redirect /dev/tty into its stdin (e.g., open /dev/tty as stdin for the
"$INSTALL_DIR/$BINARY" setup process) so interactive prompts (called via setup)
work even in piped installs; keep the existing variables INSTALL_DIR and BINARY
and only change the condition and invocation to use /dev/tty as stdin.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 37-42: Pre-load the config once in the root command pre-run and
attach it to the cobra command context so subcommands stop calling config.Load()
again: after cfg, err := config.Load() in the root pre-run, create a new context
via context.WithValue(ctx, configKey, cfg) and call cmd.SetContext(newCtx); then
update gated commands (e.g., Analyze RunE) to read cfg from
cmd.Context().Value(configKey) instead of calling config.Load(), and remove the
duplicated file/env parse.
🪄 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: b3d6e0d8-8bfc-4689-8967-d12754086374
📒 Files selected for processing (2)
cmd/root.goinstall.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/setup/wizard.go (1)
137-177:⚠️ Potential issue | 🟠 MajorMake config-save failure fatal before printing success.
With the new first-run gate, setup is only really done once the config is persisted. Right now a save error is downgraded to a warning, then the wizard still prints
Setup completeand starts watch mode. A user who came in with only an env-var API key can walk away thinking setup worked, then hit the “Run 'supermodel setup' to get started.” nudge again on the next command.💡 Suggested fix
cfg.Files = boolPtr(filesEnabled) if err := cfg.Save(); err != nil { - fmt.Fprintf(os.Stderr, " %sWarning: could not save config: %v%s\n", yellow, err, reset) + return fmt.Errorf("save config: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/setup/wizard.go` around lines 137 - 177, The config save failure should be fatal: after calling cfg.Save() (the existing if err := cfg.Save(); err != nil { ... } block), do not just warn and continue; instead print the error and terminate so the function never prints "Setup complete" or starts watch mode. Replace the current warning branch with a fatal exit (e.g., print the error via fmt.Fprintf and call os.Exit(1) or return a non-nil error from this function) so that on cfg.Save() failure the code never reaches the success messages or the return files.Watch(...) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/setup/wizard.go`:
- Around line 50-58: After auth.Login(ctx) succeeds, the code currently ignores
a failure from config.Load() and continues using the stale cfg which can
overwrite the new API key later; update the post-login reload in wizard.go so
that if config.Load() returns an error you return that error (or wrap it with
context) instead of keeping the old cfg, i.e. in the block after auth.Login(ctx)
check loadErr and fail fast (return fmt.Errorf(...)) when config.Load() fails so
the new key in the persisted config isn't lost.
---
Outside diff comments:
In `@internal/setup/wizard.go`:
- Around line 137-177: The config save failure should be fatal: after calling
cfg.Save() (the existing if err := cfg.Save(); err != nil { ... } block), do not
just warn and continue; instead print the error and terminate so the function
never prints "Setup complete" or starts watch mode. Replace the current warning
branch with a fatal exit (e.g., print the error via fmt.Fprintf and call
os.Exit(1) or return a non-nil error from this function) so that on cfg.Save()
failure the code never reaches the success messages or the return
files.Watch(...) call.
🪄 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: cb41b5b5-19c8-465b-b5d9-5b595f7e43dc
📒 Files selected for processing (1)
internal/setup/wizard.go
| if cfg.APIKey == "" { | ||
| fmt.Printf(" %sRun 'supermodel login' first, then re-run 'supermodel setup'.%s\n\n", yellow, reset) | ||
| return nil | ||
| fmt.Printf(" %sOpening your browser to sign in and generate an API key…%s\n\n", dWhite, reset) | ||
| if err := auth.Login(ctx); err != nil { | ||
| return fmt.Errorf("authentication failed — run 'supermodel login' to try again") | ||
| } | ||
| // Reload config to pick up the saved key. | ||
| if reloaded, loadErr := config.Load(); loadErr == nil { | ||
| cfg = reloaded | ||
| } |
There was a problem hiding this comment.
Return on a post-login reload failure.
Small but real edge case: auth.Login(ctx) has already saved the new key, so if config.Load() fails here and we keep the old cfg, the later save on Line 138 can write that stale struct back out and clear the key we just created. I’d fail fast instead of silently continuing.
💡 Suggested fix
// Reload config to pick up the saved key.
- if reloaded, loadErr := config.Load(); loadErr == nil {
- cfg = reloaded
- }
+ reloaded, loadErr := config.Load()
+ if loadErr != nil {
+ return fmt.Errorf("reload config after login: %w", loadErr)
+ }
+ cfg = reloaded
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/setup/wizard.go` around lines 50 - 58, After auth.Login(ctx)
succeeds, the code currently ignores a failure from config.Load() and continues
using the stale cfg which can overwrite the new API key later; update the
post-login reload in wizard.go so that if config.Load() returns an error you
return that error (or wrap it with context) instead of keeping the old cfg, i.e.
in the block after auth.Login(ctx) check loadErr and fail fast (return
fmt.Errorf(...)) when config.Load() fails so the new key in the persisted config
isn't lost.
- cmd/root.go: split config load error from missing-key nudge so real errors (e.g. corrupt YAML) show the actual message rather than the setup prompt - cmd/root.go: add __complete and __completeNoDesc to noConfigCommands so Cobra's shell completion internals don't crash without a config - install.sh: replace [ -t 1 ] with [ -r /dev/tty ] and redirect /dev/tty as stdin for setup invocation, so interactive prompts work correctly in piped installs (curl … | sh) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
supermodel setupwhen the terminal is interactive (TTY-gated — skips silent/piped/CI installs)PersistentPreRunEchecks for a config file on every command; if none exists, printsRun 'supermodel setup' to get started.and exits — bypass list coverssetup,login,logout,version,help,completionTest plan
curl … | shin a terminal → setup wizard launches automaticallycurl … | shpiped to a file / in CI → setup wizard does NOT launchsupermodel analyzewith no config → prints nudge and exits cleanlysupermodel setupwith no config → wizard runs without nudge loopsupermodel versionwith no config → prints version, no nudge🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes