fix: print getting-started message after npm global install#161
fix: print getting-started message after npm global install#161greynewell merged 3 commits intomainfrom
Conversation
|
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 (2)
WalkthroughTests are added to validate that both shell and npm installation scripts provide post-install guidance without triggering interactive wizards, and a terminal detection function is refactored for test mockability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 8 minutes and 26 seconds. Comment |
greynewell
force-pushed
the
fix/npm-postinstall-message
branch
from
f5c159e to
0acc295
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/install_script_test.go`:
- Around line 9-31: The new TestInstallScript adds assertions that will fail
against the current install.sh because it still invokes the setup subcommand and
lacks the getting-started message; either update install.sh to remove the setup
invocation (so it no longer contains the literal `" setup` or "supermodel setup"
matches checked by TestInstallScript) and add a getting-started prompt that
mentions running 'supermodel' (so the lowercased content satisfies the
hasRunSupermodel OR hasGetStarted checks), or remove/move this test out of
cmd/install_script_test.go and into the PR that actually changes install.sh so
tests and script changes remain paired.
In `@cmd/npm_package_test.go`:
- Around line 27-33: The test currently passes if either "get started" or a "run
'supermodel'" variant exists; update the assertion in the anonymous t.Run (uses
variables lower, hasRunSupermodel, hasGetStarted) to require the explicit
guidance by checking both conditions and that the phrase "project directory"
appears (e.g., require hasRunSupermodel && hasGetStarted &&
strings.Contains(lower, "project directory")), and update the t.Error message
accordingly so the test fails unless the full getting-started instruction
including "run 'supermodel' ... in project directory" is present.
In `@cmd/root.go`:
- Around line 17-23: The openDevTty stub currently always returns an error, so
the stdin fallback never opens /dev/tty; replace the stub implementation of
openDevTty with a real attempt to open "/dev/tty" (using os.Open) and return the
resulting *os.File and error (keeping it replaceable for tests), and ensure
callers like stdinIsTerminal can call openDevTty and handle the returned
file/error; on Windows you may still return an appropriate error if os.Open
fails.
In `@internal/auth/handler_test.go`:
- Around line 300-316: The test fails because it references non-existent
injection hooks openBrowserFunc, stdinReader, and loginOut; add package-level
injectable variables in internal/auth/handler.go (e.g., var openBrowserFunc
func(string) error, var stdinReader io.Reader, var loginOut io.Writer) and
update the Login/loginManual/readSecret code paths to use these variables
instead of direct calls to the browser, os.Stdin or stdout; ensure their default
initializations point to the real implementations (openBrowserFunc =
openBrowser, stdinReader = os.Stdin, loginOut = os.Stdout) so production
behavior is unchanged, allowing the test to override them and restore via
t.Cleanup.
🪄 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: 517fc3cc-0006-412f-b247-f87dd1790733
📒 Files selected for processing (5)
cmd/install_script_test.gocmd/npm_package_test.gocmd/root.gocmd/root_test.gointernal/auth/handler_test.go
| func TestInstallScript(t *testing.T) { | ||
| data, err := os.ReadFile("../install.sh") | ||
| if err != nil { | ||
| t.Fatalf("could not read install.sh: %v", err) | ||
| } | ||
| content := string(data) | ||
|
|
||
| t.Run("does not run setup wizard at install time", func(t *testing.T) { | ||
| // The script uses $BINARY variable, so match the literal subcommand argument | ||
| if strings.Contains(content, `" setup`) || strings.Contains(content, "supermodel setup") { | ||
| t.Error("install.sh must not run the setup subcommand at install time — the wizard now auto-launches from bare 'supermodel' in a project directory (PR #152)") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("includes getting-started message", func(t *testing.T) { | ||
| lower := strings.ToLower(content) | ||
| hasRunSupermodel := strings.Contains(lower, "run 'supermodel'") || strings.Contains(lower, "run \"supermodel\"") | ||
| hasGetStarted := strings.Contains(lower, "get started") | ||
| if !hasRunSupermodel && !hasGetStarted { | ||
| t.Error("install.sh must include a getting-started message directing users to run 'supermodel' in their project directory") | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
This adds a guaranteed-red test without the paired install.sh change.
Right now this test fails against the current script (install.sh still calls setup at install.sh Line 80 and lacks the required getting-started prompt). If this PR is scoped to npm postinstall, this blocks merge unnecessarily—either include the install.sh behavior change here or move this test to the PR that changes install.sh.
🧰 Tools
🪛 GitHub Actions: Build
[error] 19-19: TestInstallScript failed: "install.sh must not run the setup subcommand at install time — the wizard now auto-launches from bare 'supermodel' in a project directory (PR #152)"
[error] 28-28: TestInstallScript failed: "install.sh must include a getting-started message directing users to run 'supermodel' in their project directory"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/install_script_test.go` around lines 9 - 31, The new TestInstallScript
adds assertions that will fail against the current install.sh because it still
invokes the setup subcommand and lacks the getting-started message; either
update install.sh to remove the setup invocation (so it no longer contains the
literal `" setup` or "supermodel setup" matches checked by TestInstallScript)
and add a getting-started prompt that mentions running 'supermodel' (so the
lowercased content satisfies the hasRunSupermodel OR hasGetStarted checks), or
remove/move this test out of cmd/install_script_test.go and into the PR that
actually changes install.sh so tests and script changes remain paired.
| // openDevTty opens /dev/tty (the process's controlling terminal). It is a | ||
| // package-level var so tests can replace it with a mock. On Windows, | ||
| // os.Open("/dev/tty") will fail, which is expected — Windows Console API | ||
| // (used by term.IsTerminal above) handles Windows Terminal/PowerShell. | ||
| var openDevTty = func() (*os.File, error) { | ||
| return nil, fmt.Errorf("not implemented") | ||
| } |
There was a problem hiding this comment.
openDevTty is a stub and the fallback path is still non-functional.
At Line 21-23 this always errors, and stdinIsTerminal never consumes this hook, so the /dev/tty fallback behavior is still missing (and the new test fails).
Suggested fix
// openDevTty opens /dev/tty (the process's controlling terminal). It is a
// package-level var so tests can replace it with a mock. On Windows,
// os.Open("/dev/tty") will fail, which is expected — Windows Console API
// (used by term.IsTerminal above) handles Windows Terminal/PowerShell.
var openDevTty = func() (*os.File, error) {
- return nil, fmt.Errorf("not implemented")
+ return os.Open("/dev/tty")
}
// stdinIsTerminal reports whether stdin is connected to an interactive
// terminal. Pulled into a var so tests can stub it.
var stdinIsTerminal = func() bool {
- return term.IsTerminal(int(syscall.Stdin)) //nolint:unconvert // syscall.Stdin is uintptr on Windows
+ if term.IsTerminal(int(syscall.Stdin)) { //nolint:unconvert // syscall.Stdin is uintptr on Windows
+ return true
+ }
+ f, err := openDevTty()
+ if err != nil {
+ return false
+ }
+ _ = f.Close()
+ return true
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/root.go` around lines 17 - 23, The openDevTty stub currently always
returns an error, so the stdin fallback never opens /dev/tty; replace the stub
implementation of openDevTty with a real attempt to open "/dev/tty" (using
os.Open) and return the resulting *os.File and error (keeping it replaceable for
tests), and ensure callers like stdinIsTerminal can call openDevTty and handle
the returned file/error; on Windows you may still return an appropriate error if
os.Open fails.
| orig := openBrowserFunc | ||
| openBrowserFunc = func(url string) error { | ||
| return fmt.Errorf("no display available") | ||
| } | ||
| t.Cleanup(func() { openBrowserFunc = orig }) | ||
|
|
||
| // Provide stdin replacement so loginManual can read the pasted key. | ||
| stdinInput := "smsk_live_headless_test\n" | ||
| origStdinReader := stdinReader | ||
| stdinReader = strings.NewReader(stdinInput) | ||
| t.Cleanup(func() { stdinReader = origStdinReader }) | ||
|
|
||
| // Capture output to verify the auth URL was printed. | ||
| var outBuf bytes.Buffer | ||
| origOut := loginOut | ||
| loginOut = &outBuf | ||
| t.Cleanup(func() { loginOut = origOut }) |
There was a problem hiding this comment.
This test uses undefined injection hooks and currently breaks compilation.
At Line 300-316, openBrowserFunc, stdinReader, and loginOut don’t exist in the package, so all platforms fail to build.
Please either:
- add these injectable vars in
internal/auth/handler.goand routeLogin/loginManual/readSecretthrough them, or - rewrite this test to use existing public seams only.
🧰 Tools
🪛 GitHub Actions: Build
[error] 300-300: Go test compile error: undefined: openBrowserFunc
🪛 GitHub Actions: Lint
[error] 300-300: golangci-lint: build/test error: undefined: openBrowserFunc
🪛 GitHub Check: golangci-lint
[failure] 316-316:
undefined: loginOut (typecheck)
[failure] 315-315:
undefined: loginOut
[failure] 314-314:
undefined: loginOut
[failure] 310-310:
undefined: stdinReader
[failure] 309-309:
undefined: stdinReader
[failure] 308-308:
undefined: stdinReader
[failure] 304-304:
undefined: openBrowserFunc
[failure] 301-301:
undefined: openBrowserFunc
[failure] 300-300:
undefined: openBrowserFunc
🪛 GitHub Check: test (macos-latest)
[failure] 316-316:
undefined: loginOut
[failure] 315-315:
undefined: loginOut
[failure] 314-314:
undefined: loginOut
[failure] 310-310:
undefined: stdinReader
[failure] 309-309:
undefined: stdinReader
[failure] 308-308:
undefined: stdinReader
[failure] 304-304:
undefined: openBrowserFunc
[failure] 301-301:
undefined: openBrowserFunc
[failure] 300-300:
undefined: openBrowserFunc
🪛 GitHub Check: test (ubuntu-latest)
[failure] 316-316:
undefined: loginOut
[failure] 315-315:
undefined: loginOut
[failure] 314-314:
undefined: loginOut
[failure] 310-310:
undefined: stdinReader
[failure] 309-309:
undefined: stdinReader
[failure] 308-308:
undefined: stdinReader
[failure] 304-304:
undefined: openBrowserFunc
[failure] 301-301:
undefined: openBrowserFunc
[failure] 300-300:
undefined: openBrowserFunc
🪛 GitHub Check: test (windows-latest)
[failure] 316-316:
undefined: loginOut
[failure] 315-315:
undefined: loginOut
[failure] 314-314:
undefined: loginOut
[failure] 310-310:
undefined: stdinReader
[failure] 309-309:
undefined: stdinReader
[failure] 308-308:
undefined: stdinReader
[failure] 304-304:
undefined: openBrowserFunc
[failure] 301-301:
undefined: openBrowserFunc
[failure] 300-300:
undefined: openBrowserFunc
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/handler_test.go` around lines 300 - 316, The test fails because
it references non-existent injection hooks openBrowserFunc, stdinReader, and
loginOut; add package-level injectable variables in internal/auth/handler.go
(e.g., var openBrowserFunc func(string) error, var stdinReader io.Reader, var
loginOut io.Writer) and update the Login/loginManual/readSecret code paths to
use these variables instead of direct calls to the browser, os.Stdin or stdout;
ensure their default initializations point to the real implementations
(openBrowserFunc = openBrowser, stdinReader = os.Stdin, loginOut = os.Stdout) so
production behavior is unchanged, allowing the test to override them and restore
via t.Cleanup.
0acc295 to
037157d
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes #159. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rted CodeRabbit feedback: use && instead of || so the test requires both the 'run supermodel' instruction and the 'get started' phrase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Closes #159.
npm install -g @supermodeltools/cliinstalls the binary but gives no guidance to the user. This PR adds a postinstall message tonpm/install.jsthat directs users to runsupermodelinside their project directory.Changes
npm/install.jspostinstall scriptcmd/npm_package_test.go) that verifies the message existsTDD
Failing test committed first — the
includes getting-started messagesubtest fails before the fix, and passes after.🤖 Generated with Claude Code
Summary by CodeRabbit