From 0943fe31c8202dfff705c611f92e13d7c3fa3bc4 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Mon, 17 Aug 2026 14:42:50 -0400 Subject: [PATCH 01/14] feat(setup): report package-manager confidence and stop guessing silently Detection always returned a manager, so a project that never said which one it used got pip or npm presented as fact. Signals are now split into what the project declares and what it merely implies, and a verdict is definite only when the project names exactly one manager. Recognise the corepack packageManager field, poetry.lock, Pipfile.lock and [tool.pdm], which were previously read as pip or npm. [tool.hatch] is recorded as a signal we cannot act on, since hatch has no dependency-add command. `setup install` now reads the project when --package-manager is omitted, instead of defaulting to pip or npm whatever the lockfiles say, and fails with the candidate list when the project is ambiguous rather than picking for the user. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/detect.go | 19 +- cmd/setup/install.go | 30 +++ cmd/setup/setup_test.go | 5 + internal/setup/detector.go | 261 ++++++++++++++++++++++--- internal/setup/detector_shapes_test.go | 8 +- internal/setup/detector_test.go | 134 +++++++++++++ internal/setup/installer.go | 3 + 7 files changed, 435 insertions(+), 25 deletions(-) diff --git a/cmd/setup/detect.go b/cmd/setup/detect.go index b7d0ced2..aa14e873 100644 --- a/cmd/setup/detect.go +++ b/cmd/setup/detect.go @@ -44,7 +44,17 @@ func runDetect(svc setup.Service) func(*cobra.Command, []string) error { outputKind := cliflags.GetOutputKind(cmd) if outputKind == "json" { - data, _ := json.Marshal(result) + // Candidates are added here rather than in the detection result: they + // report which tools are on this machine, which is not a fact about the + // project. Callers reading this need both. + payload := struct { + *setup.DetectResult + PackageManagerCandidates []setup.PMCandidate `json:"package_manager_candidates,omitempty"` + }{DetectResult: result} + if result.PackageManagerConfidence == setup.PMAmbiguous { + payload.PackageManagerCandidates = setup.PackageManagerChoiceFor(dir, result.SDKID).Candidates + } + data, _ := json.Marshal(payload) fmt.Fprintln(cmd.OutOrStdout(), string(data)) return nil } @@ -53,7 +63,12 @@ func runDetect(svc setup.Service) func(*cobra.Command, []string) error { if result.Framework != "" { fmt.Fprintf(cmd.OutOrStdout(), "Framework: %s\n", result.Framework) } - fmt.Fprintf(cmd.OutOrStdout(), "Package Manager: %s\n", result.PackageManager) + if result.PackageManagerConfidence == setup.PMAmbiguous { + fmt.Fprintf(cmd.OutOrStdout(), "Package Manager: %s (uncertain — %s)\n", + result.PackageManager, result.PackageManagerReason) + } else { + fmt.Fprintf(cmd.OutOrStdout(), "Package Manager: %s\n", result.PackageManager) + } fmt.Fprintf(cmd.OutOrStdout(), "Recommended SDK: %s\n", result.SDKID) if result.EntryPointExists { fmt.Fprintf(cmd.OutOrStdout(), "Entry Point: %s\n", result.EntryPoint) diff --git a/cmd/setup/install.go b/cmd/setup/install.go index 42b85465..2f155d01 100644 --- a/cmd/setup/install.go +++ b/cmd/setup/install.go @@ -34,6 +34,20 @@ func newInstallCmd(svc setup.Service) *cobra.Command { return cmd } +// candidateList renders the choices for an error message, marking the ones that +// are not installed so the user isn't sent to a tool they'd have to install first. +func candidateList(candidates []setup.PMCandidate) string { + names := make([]string, 0, len(candidates)) + for _, c := range candidates { + if c.Installed { + names = append(names, c.Name) + continue + } + names = append(names, c.Name+" (not installed)") + } + return strings.Join(names, ", ") +} + func runInstall(svc setup.Service) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { dir, _ := cmd.Flags().GetString(pathFlag) @@ -48,6 +62,22 @@ func runInstall(svc setup.Service) func(*cobra.Command, []string) error { sdkID, _ := cmd.Flags().GetString(sdkIDFlag) pkgMgr, _ := cmd.Flags().GetString("package-manager") dryRun, _ := cmd.Flags().GetBool(dryRunFlag) + + // Without an explicit choice, read the project rather than falling back to + // pip or npm regardless of what the project uses. An ambiguous project is an + // error: guessing here would install with the wrong manager, and this command + // cannot ask. + if pkgMgr == "" { + choice := setup.PackageManagerChoiceFor(dir, sdkID) + if choice.Confidence == setup.PMAmbiguous { + return fmt.Errorf( + "cannot tell which package manager to use: %s\npass --package-manager with one of: %s", + choice.Reason, candidateList(choice.Candidates), + ) + } + pkgMgr = choice.Name + } + detection := &setup.DetectResult{ SDKID: sdkID, PackageManager: pkgMgr, diff --git a/cmd/setup/setup_test.go b/cmd/setup/setup_test.go index 17130c2f..a44e15c2 100644 --- a/cmd/setup/setup_test.go +++ b/cmd/setup/setup_test.go @@ -220,6 +220,7 @@ func TestInstall_Plaintext(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", } output, err := cmd.CallCmd( t, @@ -241,6 +242,7 @@ func TestInstall_Plaintext_WithVersion(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", } output, err := cmd.CallCmd( t, @@ -319,6 +321,7 @@ func TestInstall_DryRun(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", "--dry-run", } // No Installer provided: dry-run must not invoke it or shell out. @@ -341,6 +344,7 @@ func TestInstall_JSON(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", "--output", "json", } output, err := cmd.CallCmd( @@ -362,6 +366,7 @@ func TestInstallStubReturnsError(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", } _, err := cmd.CallCmd( t, diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 5d9db9b8..50e5294a 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -21,6 +22,12 @@ type DetectResult struct { // suggest. Callers must not write initialization code into a suggested path // without telling the user, since the project does not load that file. EntryPointExists bool `json:"entry_point_exists"` + // PackageManagerConfidence says whether the project identifies its package + // manager or PackageManager is only a conventional default. Callers must not + // install against an ambiguous verdict without asking first. + PackageManagerConfidence PMConfidence `json:"package_manager_confidence,omitempty"` + // PackageManagerReason explains an ambiguous verdict, phrased for the user. + PackageManagerReason string `json:"package_manager_reason,omitempty"` } // Detector inspects a directory to determine the language, framework, package manager, @@ -60,6 +67,14 @@ func (FileDetector) Detect(dir string) (*DetectResult, error) { detectNode, } { if result := detect(dir); result != nil { + // Only what the project says. PackageManager itself is left as detected, + // since the language detectors know about managers this does not model, + // such as maven versus gradle. Candidates carry which tools are installed, + // which describes the machine rather than the project, so they are left to + // callers that need to present a choice. + choice := PackageManagerChoiceFor(dir, result.SDKID) + result.PackageManagerConfidence = choice.Confidence + result.PackageManagerReason = choice.Reason return result, nil } } @@ -184,19 +199,54 @@ func detectNode(dir string) *DetectResult { } func detectNodePM(dir string) string { - if _, err := os.Stat(filepath.Join(dir, "pnpm-lock.yaml")); err == nil { - return "pnpm" + return nodePMSignals(dir).best("npm") +} + +// corepackPM reads the packageManager field, which names the manager and version +// the project expects. It is the most explicit statement a Node project can make, +// so it outranks lockfiles. +// https://nodejs.org/api/corepack.html +func corepackPM(dir string) string { + b, err := os.ReadFile(filepath.Join(dir, "package.json")) + if err != nil { + return "" + } + var pkg struct { + PackageManager string `json:"packageManager"` + } + if json.Unmarshal(b, &pkg) != nil { + return "" } - if _, err := os.Stat(filepath.Join(dir, "yarn.lock")); err == nil { - return "yarn" + // The field is "@", and the version is required by corepack but + // often omitted by hand-edited manifests. + name, _, _ := strings.Cut(pkg.PackageManager, "@") + switch name { + case "npm", "yarn", "pnpm", "bun": + return name } - // Lockfiles: https://bun.com/docs/install/lockfile - for _, lock := range []string{"bun.lock", "bun.lockb"} { - if _, err := os.Stat(filepath.Join(dir, lock)); err == nil { - return "bun" + return "" +} + +// nodePMSignals reports what the project says about its Node package manager. +// Lockfiles: https://bun.com/docs/install/lockfile +func nodePMSignals(dir string) pmSignals { + if declared := corepackPM(dir); declared != "" { + return pmSignals{declared: declared} + } + var s pmSignals + for _, lock := range []struct{ file, pm string }{ + {"pnpm-lock.yaml", "pnpm"}, + {"yarn.lock", "yarn"}, + {"bun.lock", "bun"}, + {"bun.lockb", "bun"}, + {"package-lock.json", "npm"}, + {"npm-shrinkwrap.json", "npm"}, + } { + if _, err := os.Stat(filepath.Join(dir, lock.file)); err == nil { + s.addLocked(lock.pm) } } - return "npm" + return s } func detectGo(dir string) *DetectResult { @@ -238,21 +288,40 @@ func detectPython(dir string) *DetectResult { // https://pipenv.pypa.io/en/latest/ // https://python-poetry.org/docs/pyproject/ func detectPythonPM(dir string) string { - if _, err := os.Stat(filepath.Join(dir, "uv.lock")); err == nil { - return "uv" - } - if _, err := os.Stat(filepath.Join(dir, "Pipfile")); err == nil { - return "pipenv" + return pythonPMSignals(dir).best("pip") +} + +// pythonPMSignals reports what the project says about its Python package manager. +// A lockfile is treated as the project having committed to a tool; a [tool.*] +// section counts the same way, since the tool owns that config. +func pythonPMSignals(dir string) pmSignals { + var s pmSignals + for _, lock := range []struct{ file, pm string }{ + {"uv.lock", "uv"}, + {"poetry.lock", "poetry"}, + {"Pipfile.lock", "pipenv"}, + {"Pipfile", "pipenv"}, + } { + if _, err := os.Stat(filepath.Join(dir, lock.file)); err == nil { + s.addLocked(lock.pm) + } } if b, err := os.ReadFile(filepath.Join(dir, "pyproject.toml")); err == nil { - if bytes.Contains(b, []byte("[tool.poetry]")) { - return "poetry" - } - if bytes.Contains(b, []byte("[tool.uv]")) { - return "uv" + for _, section := range []struct{ marker, pm string }{ + {"[tool.poetry]", "poetry"}, + {"[tool.uv]", "uv"}, + {"[tool.pdm]", "pdm"}, + // hatch has no dependency-add command, so it is a signal we cannot act + // on. Recording it keeps the project ambiguous instead of silently + // falling through to pip. + {"[tool.hatch]", ""}, + } { + if bytes.Contains(b, []byte(section.marker)) { + s.addLocked(section.pm) + } } } - return "pip" + return s } func detectRuby(dir string) *DetectResult { @@ -282,10 +351,18 @@ func detectRuby(dir string) *DetectResult { // install` would succeed without recording the SDK for the app. // Gemfile: https://bundler.io/guides/gemfile.html func detectRubyPM(dir string) string { + return rubyPMSignals(dir).best("gem") +} + +// rubyPMSignals reports what the project says about its Ruby package manager. A +// Gemfile is Bundler's own manifest, so it settles the question; a gemspec alone +// does not, since the gem could be developed either way. +func rubyPMSignals(dir string) pmSignals { + var s pmSignals if _, err := os.Stat(filepath.Join(dir, "Gemfile")); err == nil { - return "bundle" + s.addLocked("bundle") } - return "gem" + return s } func detectJava(dir string) *DetectResult { @@ -585,3 +662,143 @@ func findFileUnder(dir, root string, names ...string) string { } return "" } + +// PMConfidence says whether the project itself identifies its package manager. +type PMConfidence string + +const ( + // PMDefinite means the project names one manager and only one. + PMDefinite PMConfidence = "definite" + // PMAmbiguous means the project does not say, or contradicts itself. Callers + // must ask rather than guess. + PMAmbiguous PMConfidence = "ambiguous" +) + +// PMCandidate is a package manager the user could pick, with the command it would +// run. Installed reports whether the tool is on PATH; it describes the machine, not +// the project, so it never makes a candidate more likely to be the right one. +type PMCandidate struct { + Name string `json:"name"` + Installed bool `json:"installed"` + Command string `json:"command"` +} + +// PMChoice is the package manager for a project plus how sure we are. +type PMChoice struct { + Name string `json:"name"` + Confidence PMConfidence `json:"confidence"` + // Reason explains an ambiguous verdict in the words the picker shows the user. + Reason string `json:"reason,omitempty"` + Candidates []PMCandidate `json:"candidates,omitempty"` +} + +// pmSignals collects what a project says about its package manager. declared holds +// an explicit statement, which settles the question on its own; locked holds +// managers implied by lockfiles or tool config, where more than one means the +// project contradicts itself. An empty entry in locked marks a tool we recognise +// but cannot drive. +type pmSignals struct { + declared string + locked []string + unactionable bool +} + +func (s *pmSignals) addLocked(pm string) { + if pm == "" { + s.unactionable = true + return + } + for _, existing := range s.locked { + if existing == pm { + return + } + } + s.locked = append(s.locked, pm) +} + +// best returns the manager to use, falling back to fallback when the project says +// nothing. It preserves the old detection behaviour for callers that only want a +// name, including the first-match-wins ordering when lockfiles conflict. +func (s pmSignals) best(fallback string) string { + if s.declared != "" { + return s.declared + } + if len(s.locked) > 0 { + return s.locked[0] + } + return fallback +} + +// choose turns signals into a verdict. options lists every manager valid for the +// language, in the order the picker should show them, and fallback is the +// conventional default when the project is silent. +func (s pmSignals) choose(options []string, fallback string, argvFor func(string) []string) PMChoice { + candidates := make([]PMCandidate, 0, len(options)) + for _, name := range options { + argv := argvFor(name) + // Installed tracks the executable the command actually runs, not the label. + // "pip" resolves to pip3 on a stock macOS box, and reporting that as missing + // would steer the user away from the option that works. + installed := len(argv) > 0 && onPath(argv[0]) + candidates = append(candidates, PMCandidate{ + Name: name, + Installed: installed, + Command: strings.Join(argv, " "), + }) + } + + switch { + case s.declared != "": + return PMChoice{Name: s.declared, Confidence: PMDefinite} + case len(s.locked) == 1 && !s.unactionable: + return PMChoice{Name: s.locked[0], Confidence: PMDefinite} + case len(s.locked) > 1: + return PMChoice{ + Name: s.locked[0], + Confidence: PMAmbiguous, + Reason: fmt.Sprintf("this project has lockfiles for more than one manager (%s)", + strings.Join(s.locked, ", ")), + Candidates: candidates, + } + case s.unactionable: + return PMChoice{ + Name: fallback, + Confidence: PMAmbiguous, + Reason: "this project is managed by a tool that cannot add dependencies for us", + Candidates: candidates, + } + default: + return PMChoice{ + Name: fallback, + Confidence: PMAmbiguous, + Reason: "this project doesn't say which package manager it uses", + Candidates: candidates, + } + } +} + +// PackageManagerChoiceFor reports the package manager for sdkID in dir and whether +// the project actually identifies it. Languages with a single toolchain are always +// definite; there is nothing to ask. +func PackageManagerChoiceFor(dir, sdkID string) PMChoice { + cmdFor := func(pm string) []string { + args, _ := InstallArgs(dir, sdkID, pm) + return args + } + + switch sdkID { + case "node-server", "js-client-sdk", "react-client-sdk", "react-native": + return nodePMSignals(dir).choose([]string{"npm", "yarn", "pnpm", "bun"}, "npm", cmdFor) + case "python-server-sdk": + return pythonPMSignals(dir).choose([]string{"pip", "uv", "poetry", "pipenv", "pdm"}, "pip", cmdFor) + case "ruby-server-sdk": + return rubyPMSignals(dir).choose([]string{"bundle", "gem"}, "gem", cmdFor) + case "go-server-sdk": + return PMChoice{Name: "go", Confidence: PMDefinite} + case "dotnet-server-sdk": + return PMChoice{Name: "dotnet", Confidence: PMDefinite} + default: + // Java, Android and Swift are installed by hand. + return PMChoice{Name: "", Confidence: PMDefinite} + } +} diff --git a/internal/setup/detector_shapes_test.go b/internal/setup/detector_shapes_test.go index 56df04eb..058370f3 100644 --- a/internal/setup/detector_shapes_test.go +++ b/internal/setup/detector_shapes_test.go @@ -367,7 +367,13 @@ func TestFileDetector_ProjectShapes(t *testing.T) { require.NoError(t, err) want := shape.want want.EntryPoint = filepath.Join(dir, want.EntryPoint) - assert.Equal(t, want, *result) + // These shapes assert language, SDK and entry point. Package-manager + // confidence has its own tests, so it is cleared rather than restated on + // every shape. + got := *result + got.PackageManagerConfidence = "" + got.PackageManagerReason = "" + assert.Equal(t, want, got) }) } } diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index 5ab223e2..c132887e 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -850,3 +850,137 @@ func TestPackageManagerFor_DerivesFromProjectNotDetectedLanguage(t *testing.T) { assert.Empty(t, PackageManagerFor(t.TempDir(), "java-server-sdk")) }) } + +func TestPackageManagerChoice_Definite(t *testing.T) { + tests := []struct { + name string + files map[string]string + sdkID string + want string + }{ + // The most explicit statement a Node project can make outranks lockfiles. + {"corepack field", map[string]string{ + "package.json": `{"packageManager":"pnpm@9.1.0"}`, + }, "node-server", "pnpm"}, + {"corepack field without version", map[string]string{ + "package.json": `{"packageManager":"yarn"}`, + }, "node-server", "yarn"}, + {"corepack field beats a conflicting lockfile", map[string]string{ + "package.json": `{"packageManager":"pnpm@9.1.0"}`, + "yarn.lock": "", + }, "node-server", "pnpm"}, + {"single node lockfile", map[string]string{ + "package.json": `{}`, + "pnpm-lock.yaml": "", + }, "node-server", "pnpm"}, + {"package-lock only", map[string]string{ + "package.json": `{}`, + "package-lock.json": "", + }, "node-server", "npm"}, + {"uv lockfile", map[string]string{"uv.lock": ""}, "python-server-sdk", "uv"}, + {"poetry lockfile", map[string]string{"poetry.lock": ""}, "python-server-sdk", "poetry"}, + {"tool.uv section", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.uv]\n", + }, "python-server-sdk", "uv"}, + {"tool.pdm section", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.pdm]\n", + }, "python-server-sdk", "pdm"}, + {"Gemfile", map[string]string{"Gemfile": "source 'x'"}, "ruby-server-sdk", "bundle"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, body := range tt.files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0600)) + } + + choice := PackageManagerChoiceFor(dir, tt.sdkID) + + assert.Equal(t, PMDefinite, choice.Confidence) + assert.Equal(t, tt.want, choice.Name) + assert.Empty(t, choice.Candidates, "a definite verdict needs no choice") + }) + } +} + +func TestPackageManagerChoice_Ambiguous(t *testing.T) { + tests := []struct { + name string + files map[string]string + sdkID string + wantReason string + wantOptions []string + }{ + // No detection tuning can fix this: the project contradicts itself. + {"conflicting node lockfiles", map[string]string{ + "package.json": `{}`, + "yarn.lock": "", + "package-lock.json": "", + }, "node-server", "more than one manager", []string{"npm", "yarn", "pnpm", "bun"}}, + {"bare package.json", map[string]string{ + "package.json": `{}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + // uv does not require committing the lock, and PEP 621 has no uv marker. + {"PEP 621 pyproject only", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n", + }, "python-server-sdk", "doesn't say", []string{"pip", "uv", "poetry", "pipenv", "pdm"}}, + {"requirements.txt only", map[string]string{ + "requirements.txt": "flask\n", + }, "python-server-sdk", "doesn't say", []string{"pip", "uv", "poetry", "pipenv", "pdm"}}, + // hatch cannot add dependencies for us, so we must not pick it or silently pip. + {"hatch project", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.hatch]\n", + }, "python-server-sdk", "cannot add dependencies", []string{"pip", "uv", "poetry", "pipenv", "pdm"}}, + {"gemspec without Gemfile", map[string]string{ + "a.gemspec": "Gem::Specification.new", + }, "ruby-server-sdk", "doesn't say", []string{"bundle", "gem"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, body := range tt.files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0600)) + } + + choice := PackageManagerChoiceFor(dir, tt.sdkID) + + assert.Equal(t, PMAmbiguous, choice.Confidence) + assert.Contains(t, choice.Reason, tt.wantReason) + names := make([]string, 0, len(choice.Candidates)) + for _, c := range choice.Candidates { + names = append(names, c.Name) + assert.NotEmpty(t, c.Command, "every candidate needs the command it would run") + } + assert.Equal(t, tt.wantOptions, names) + }) + } +} + +// Installed state describes the machine, so it must never change the verdict. +func TestPackageManagerChoice_InstalledStateDoesNotDecide(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) + + stubPath(t, "pnpm") // the only manager on this machine + choice := PackageManagerChoiceFor(dir, "node-server") + + assert.Equal(t, PMAmbiguous, choice.Confidence, "one installed tool is not evidence about the project") + assert.Equal(t, "npm", choice.Name, "the conventional default stands until the user picks") + for _, c := range choice.Candidates { + assert.Equal(t, c.Name == "pnpm", c.Installed) + } +} + +func TestPackageManagerChoice_SingleToolchainsAreAlwaysDefinite(t *testing.T) { + for sdkID, want := range map[string]string{ + "go-server-sdk": "go", + "dotnet-server-sdk": "dotnet", + "java-server-sdk": "", + } { + t.Run(sdkID, func(t *testing.T) { + choice := PackageManagerChoiceFor(t.TempDir(), sdkID) + assert.Equal(t, PMDefinite, choice.Confidence) + assert.Equal(t, want, choice.Name) + }) + } +} diff --git a/internal/setup/installer.go b/internal/setup/installer.go index cab87917..2126934c 100644 --- a/internal/setup/installer.go +++ b/internal/setup/installer.go @@ -269,6 +269,7 @@ var installHints = map[string]string{ "poetry": "see https://python-poetry.org/docs/#installation", "uv": "see https://docs.astral.sh/uv/getting-started/installation", "pipenv": "see https://pipenv.pypa.io/en/latest/installation.html", + "pdm": "see https://pdm-project.org/en/latest/#installation", "npm": "install Node.js from https://nodejs.org", "yarn": "see https://yarnpkg.com/getting-started/install", "pnpm": "see https://pnpm.io/installation", @@ -369,6 +370,8 @@ func pythonInstallCmd(dir, pm, pkg string) []string { return []string{"uv", "add", pkg} case "pipenv": return []string{"pipenv", "install", pkg} + case "pdm": + return []string{"pdm", "add", pkg} default: return pipInstallCmd(dir, pkg) } From 7506e31ba5737d8aedd9d853b2d29914689d25fc Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 11:49:55 -0400 Subject: [PATCH 02/14] fix(setup): match nested tool tables and let a committed manager win Real pyproject files rarely carry a bare [tool.x] header, so matching only that left the poetry, uv, pdm and hatch signals near-dead: a hatch project configures [tool.hatch.build], not [tool.hatch]. Nested tables now count, with the trailing delimiter required so [tool.uv] does not match [tool.uvicorn]. A manager the project committed to now settles the verdict even when an unactionable tool is also configured. hatchling is a common build backend for uv and poetry projects, and uv can add the dependency whoever builds the wheel; previously those projects were marked ambiguous and install refused to run. Recognise pdm.lock, so a PDM project that commits only its lockfile is still identified as one. Co-Authored-By: Claude Opus 5 (1M context) --- internal/setup/detector.go | 28 +++++++++--- internal/setup/detector_test.go | 81 +++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 7 deletions(-) diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 50e5294a..46f8e7b5 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -299,6 +299,7 @@ func pythonPMSignals(dir string) pmSignals { for _, lock := range []struct{ file, pm string }{ {"uv.lock", "uv"}, {"poetry.lock", "poetry"}, + {"pdm.lock", "pdm"}, {"Pipfile.lock", "pipenv"}, {"Pipfile", "pipenv"}, } { @@ -307,16 +308,16 @@ func pythonPMSignals(dir string) pmSignals { } } if b, err := os.ReadFile(filepath.Join(dir, "pyproject.toml")); err == nil { - for _, section := range []struct{ marker, pm string }{ - {"[tool.poetry]", "poetry"}, - {"[tool.uv]", "uv"}, - {"[tool.pdm]", "pdm"}, + for _, section := range []struct{ tool, pm string }{ + {"poetry", "poetry"}, + {"uv", "uv"}, + {"pdm", "pdm"}, // hatch has no dependency-add command, so it is a signal we cannot act // on. Recording it keeps the project ambiguous instead of silently // falling through to pip. - {"[tool.hatch]", ""}, + {"hatch", ""}, } { - if bytes.Contains(b, []byte(section.marker)) { + if hasToolSection(b, section.tool) { s.addLocked(section.pm) } } @@ -324,6 +325,16 @@ func pythonPMSignals(dir string) pmSignals { return s } +// hasToolSection reports whether pyproject declares a [tool.] table. Nested +// tables count: real configs are usually only [tool.hatch.build] or +// [tool.poetry.dependencies], with no bare header to match. The trailing "]" or "." +// is required so [tool.uv] does not match [tool.uvicorn]. +func hasToolSection(pyproject []byte, name string) bool { + prefix := "[tool." + name + return bytes.Contains(pyproject, []byte(prefix+"]")) || + bytes.Contains(pyproject, []byte(prefix+".")) +} + func detectRuby(dir string) *DetectResult { found := false for _, indicator := range []string{"Gemfile", "Gemfile.lock", "config.ru"} { @@ -750,7 +761,10 @@ func (s pmSignals) choose(options []string, fallback string, argvFor func(string switch { case s.declared != "": return PMChoice{Name: s.declared, Confidence: PMDefinite} - case len(s.locked) == 1 && !s.unactionable: + // A manager the project committed to settles it even when an unactionable tool + // is also configured: hatchling is a common build backend for uv and poetry + // projects, and uv can add the dependency regardless of who builds the wheel. + case len(s.locked) == 1: return PMChoice{Name: s.locked[0], Confidence: PMDefinite} case len(s.locked) > 1: return PMChoice{ diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index c132887e..9632ba12 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -984,3 +984,84 @@ func TestPackageManagerChoice_SingleToolchainsAreAlwaysDefinite(t *testing.T) { }) } } + +// Real pyproject files rarely carry a bare [tool.x] header; the tables that matter +// are nested. Matching only the bare header made these signals near-dead. +func TestPackageManagerChoice_NestedToolTables(t *testing.T) { + tests := []struct { + name string + pyproject string + wantName string + wantDefinite bool + }{ + {"hatch build table only", "[project]\nname=\"a\"\n[tool.hatch.build.targets.wheel]\npackages=[\"a\"]\n", "pip", false}, + {"hatch version table only", "[project]\nname=\"a\"\n[tool.hatch.version]\npath=\"a/__init__.py\"\n", "pip", false}, + {"poetry dependencies table only", "[project]\nname=\"a\"\n[tool.poetry.dependencies]\npython=\"^3.12\"\n", "poetry", true}, + {"uv sources table only", "[project]\nname=\"a\"\n[tool.uv.sources]\nx={git=\"...\"}\n", "uv", true}, + {"pdm dev-dependencies table only", "[project]\nname=\"a\"\n[tool.pdm.dev-dependencies]\ntest=[]\n", "pdm", true}, + // The trailing delimiter matters: [tool.uv] must not match [tool.uvicorn]. + {"uvicorn is not uv", "[project]\nname=\"a\"\n[tool.uvicorn]\nport=8000\n", "pip", false}, + {"hatchling ruff etc are not hatch", "[project]\nname=\"a\"\n[tool.ruff]\nline-length=100\n", "pip", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte(tt.pyproject), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, tt.wantName, choice.Name) + if tt.wantDefinite { + assert.Equal(t, PMDefinite, choice.Confidence) + } else { + assert.Equal(t, PMAmbiguous, choice.Confidence) + } + }) + } +} + +// A PDM project that commits only its lockfile is still a PDM project. +func TestPackageManagerChoice_PdmLockfile(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pdm.lock"), []byte(""), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, PMDefinite, choice.Confidence) + assert.Equal(t, "pdm", choice.Name) +} + +// hatchling is a common build backend for uv and poetry projects. The manager the +// project committed to can still add the dependency, whoever builds the wheel. +func TestPackageManagerChoice_ActionableSignalBeatsHatch(t *testing.T) { + tests := []struct { + name string + files map[string]string + want string + }{ + {"uv lockfile alongside hatch build backend", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.hatch.build.targets.wheel]\npackages=[\"a\"]\n", + "uv.lock": "", + }, "uv"}, + {"uv table alongside hatch", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.uv]\n[tool.hatch.version]\npath=\"x\"\n", + }, "uv"}, + {"poetry alongside hatch", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.poetry]\n[tool.hatch.build]\n", + }, "poetry"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, body := range tt.files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0600)) + } + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, PMDefinite, choice.Confidence, + "setup would refuse to install though %s can add the dependency", tt.want) + assert.Equal(t, tt.want, choice.Name) + }) + } +} From 453be8fd27d197ebf01d309be89d1eb5c9be3bc8 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 13:53:01 -0400 Subject: [PATCH 03/14] fix(setup): require a version in the packageManager field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pnpm refuses to run at all when package.json names it without a version — "No version specified for pnpm in packageManager" — so treating a versionless field as the project's declared manager routed the user into a command that cannot work. Such a field is no longer a declaration: the lockfiles decide, or the user is asked. When a manager still refuses for that reason, say so. The manifest is malformed rather than the command wrong, and repairing someone's manifest is not ours to do. Co-Authored-By: Claude Opus 5 (1M context) --- internal/setup/detector.go | 12 +++++++++--- internal/setup/detector_test.go | 8 +++++--- internal/setup/installer.go | 23 +++++++++++++++++++++++ internal/setup/installer_test.go | 24 ++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 46f8e7b5..4ac32b7e 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -217,9 +217,15 @@ func corepackPM(dir string) string { if json.Unmarshal(b, &pkg) != nil { return "" } - // The field is "@", and the version is required by corepack but - // often omitted by hand-edited manifests. - name, _, _ := strings.Cut(pkg.PackageManager, "@") + // The field must be "@". A hand-edited manifest that names the + // manager with no version is not a declaration we can act on: pnpm refuses to + // run at all against it ("No version specified for pnpm in packageManager"), so + // treating it as definite would route the user into a command that cannot work. + // Without a usable field the project's lockfiles decide, or the user is asked. + name, version, _ := strings.Cut(pkg.PackageManager, "@") + if version == "" { + return "" + } switch name { case "npm", "yarn", "pnpm", "bun": return name diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index 9632ba12..b072e68f 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -862,9 +862,6 @@ func TestPackageManagerChoice_Definite(t *testing.T) { {"corepack field", map[string]string{ "package.json": `{"packageManager":"pnpm@9.1.0"}`, }, "node-server", "pnpm"}, - {"corepack field without version", map[string]string{ - "package.json": `{"packageManager":"yarn"}`, - }, "node-server", "yarn"}, {"corepack field beats a conflicting lockfile", map[string]string{ "package.json": `{"packageManager":"pnpm@9.1.0"}`, "yarn.lock": "", @@ -920,6 +917,11 @@ func TestPackageManagerChoice_Ambiguous(t *testing.T) { {"bare package.json", map[string]string{ "package.json": `{}`, }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + // pnpm refuses to run at all against a versionless packageManager field, so + // honouring it would route the user into a command that cannot work. + {"packageManager without a version", map[string]string{ + "package.json": `{"packageManager":"pnpm"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, // uv does not require committing the lock, and PEP 621 has no uv marker. {"PEP 621 pyproject only", map[string]string{ "pyproject.toml": "[project]\nname=\"a\"\n", diff --git a/internal/setup/installer.go b/internal/setup/installer.go index 2126934c..fe06e335 100644 --- a/internal/setup/installer.go +++ b/internal/setup/installer.go @@ -143,6 +143,15 @@ func (p PackageInstaller) Install(dir string, detection *DetectResult) (*Install out, err := runner(dir, args) command := strings.Join(args, " ") if err != nil { + if reason := versionlessPackageManagerReason(out); reason != "" { + return &InstallResult{ + SDKID: detection.SDKID, + Package: pkg, + Command: command, + Failed: true, + FailureReason: reason, + }, nil + } if reason := externallyManagedReason(dir, out); reason != "" { // No Command: the reason says not to run this pip, and the done screen // offers a non-empty Command as "install it yourself with". @@ -190,6 +199,20 @@ func dotnetProjectArg(dir string) (args []string, reason string) { } } +// versionlessPackageManagerReason recognises a Node manager refusing to run because +// package.json names it without a version. The manifest is malformed rather than +// the command wrong, and repairing someone's manifest is not ours to do, so say +// what is wrong and let them fix it. +func versionlessPackageManagerReason(out []byte) string { + if !bytes.Contains(out, []byte(`"packageManager"`)) || + !bytes.Contains(out, []byte("No version specified")) { + return "" + } + return "the packageManager field in package.json names a package manager without a version, " + + "which it refuses to run against. Give it a version (for example \"pnpm@9.1.0\") or remove " + + "the field, then run setup again." +} + // externallyManagedReason recognises a PEP 668 refusal and says what to do about // it. Homebrew and most current Linux distributions mark their Python as managed // by the OS package manager, so pip declines to write into it. A virtualenv is the diff --git a/internal/setup/installer_test.go b/internal/setup/installer_test.go index 62a194d1..2940b4c3 100644 --- a/internal/setup/installer_test.go +++ b/internal/setup/installer_test.go @@ -881,3 +881,27 @@ func TestInstall_PinnedSdkVersion_NoWarning(t *testing.T) { require.NoError(t, err) assert.Empty(t, result.Warning) } + +// A packageManager field naming a manager with no version is malformed, and pnpm +// refuses to run against it. Repairing someone's manifest is not ours to do, so the +// failure has to say what is wrong. +func TestInstall_VersionlessPackageManagerField_ExplainsIt(t *testing.T) { + stubVirtualEnv(t, "") + stubPath(t, "pnpm") + installer := PackageInstaller{ + run: func(string, []string) ([]byte, error) { + return []byte(`No version specified for pnpm in "packageManager" of package.json`), + errors.New("exit status 1") + }, + } + + result, err := installer.Install(t.TempDir(), &DetectResult{ + SDKID: "node-server", PackageManager: "pnpm", + }) + + require.NoError(t, err, "a malformed manifest must not dead-end the flow") + assert.True(t, result.Failed) + assert.Contains(t, result.FailureReason, "packageManager field") + assert.Contains(t, result.FailureReason, "without a version") + assert.Contains(t, result.FailureReason, "pnpm@9.1.0") +} From 3c28ef23c8c7c0856a37b4c28f957ffcd77480be Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 13:59:47 -0400 Subject: [PATCH 04/14] fix(setup): require one exact version in the packageManager field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corepack accepts only an exact version, so a range is refused outright — "Invalid package manager specification in package.json (pnpm@^11.13.0); expected a semver version" — as is a missing version. Treating either as the project's declared manager routed the user into a command that cannot run. Only an exact MAJOR.MINOR.PATCH, optionally with prerelease or build metadata, now counts; anything else leaves the lockfiles to decide or the user to be asked. When a manager refuses for that reason, say which part is wrong. The manifest is malformed rather than the command, and repairing someone's manifest is not ours to do. Co-Authored-By: Claude Opus 5 (1M context) --- internal/setup/detector.go | 18 ++++++++---- internal/setup/detector_test.go | 20 +++++++++++-- internal/setup/installer.go | 26 +++++++++-------- internal/setup/installer_test.go | 48 ++++++++++++++++++-------------- 4 files changed, 72 insertions(+), 40 deletions(-) diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 4ac32b7e..185b9086 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -8,6 +8,7 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "strings" ) @@ -202,6 +203,10 @@ func detectNodePM(dir string) string { return nodePMSignals(dir).best("npm") } +// exactSemver matches the exact versions corepack requires: a bare MAJOR.MINOR.PATCH +// with optional prerelease and build metadata, and no range operator. +var exactSemver = regexp.MustCompile(`^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$`) + // corepackPM reads the packageManager field, which names the manager and version // the project expects. It is the most explicit statement a Node project can make, // so it outranks lockfiles. @@ -217,13 +222,14 @@ func corepackPM(dir string) string { if json.Unmarshal(b, &pkg) != nil { return "" } - // The field must be "@". A hand-edited manifest that names the - // manager with no version is not a declaration we can act on: pnpm refuses to - // run at all against it ("No version specified for pnpm in packageManager"), so - // treating it as definite would route the user into a command that cannot work. - // Without a usable field the project's lockfiles decide, or the user is asked. + // The field must be "@". Corepack accepts nothing else, so + // a missing version ("No version specified for pnpm in packageManager") or a + // range ("Invalid package manager specification (pnpm@^11.13.0); expected a + // semver version") both stop the manager from running at all. Treating either as + // the project's declared manager would route the user into a command that cannot + // work, so the lockfiles decide instead, or the user is asked. name, version, _ := strings.Cut(pkg.PackageManager, "@") - if version == "" { + if !exactSemver.MatchString(version) { return "" } switch name { diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index b072e68f..17f8c3ff 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -862,6 +862,13 @@ func TestPackageManagerChoice_Definite(t *testing.T) { {"corepack field", map[string]string{ "package.json": `{"packageManager":"pnpm@9.1.0"}`, }, "node-server", "pnpm"}, + {"corepack field with prerelease", map[string]string{ + "package.json": `{"packageManager":"pnpm@9.1.0-beta.1"}`, + }, "node-server", "pnpm"}, + // corepack writes this hash form itself when it pins a manager. + {"corepack field with build metadata", map[string]string{ + "package.json": `{"packageManager":"yarn@4.1.0+sha224.abcdef"}`, + }, "node-server", "yarn"}, {"corepack field beats a conflicting lockfile", map[string]string{ "package.json": `{"packageManager":"pnpm@9.1.0"}`, "yarn.lock": "", @@ -917,11 +924,20 @@ func TestPackageManagerChoice_Ambiguous(t *testing.T) { {"bare package.json", map[string]string{ "package.json": `{}`, }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, - // pnpm refuses to run at all against a versionless packageManager field, so - // honouring it would route the user into a command that cannot work. + // Corepack needs one exact version, so honouring anything else would route the + // user into a manager that refuses to run. {"packageManager without a version", map[string]string{ "package.json": `{"packageManager":"pnpm"}`, }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + {"packageManager with a caret range", map[string]string{ + "package.json": `{"packageManager":"pnpm@^11.13.0"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + {"packageManager with a comparator range", map[string]string{ + "package.json": `{"packageManager":"pnpm@>=11"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + {"packageManager with a partial version", map[string]string{ + "package.json": `{"packageManager":"pnpm@11"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, // uv does not require committing the lock, and PEP 621 has no uv marker. {"PEP 621 pyproject only", map[string]string{ "pyproject.toml": "[project]\nname=\"a\"\n", diff --git a/internal/setup/installer.go b/internal/setup/installer.go index fe06e335..b026e7a1 100644 --- a/internal/setup/installer.go +++ b/internal/setup/installer.go @@ -143,7 +143,7 @@ func (p PackageInstaller) Install(dir string, detection *DetectResult) (*Install out, err := runner(dir, args) command := strings.Join(args, " ") if err != nil { - if reason := versionlessPackageManagerReason(out); reason != "" { + if reason := packageManagerSpecReason(out); reason != "" { return &InstallResult{ SDKID: detection.SDKID, Package: pkg, @@ -199,18 +199,22 @@ func dotnetProjectArg(dir string) (args []string, reason string) { } } -// versionlessPackageManagerReason recognises a Node manager refusing to run because -// package.json names it without a version. The manifest is malformed rather than -// the command wrong, and repairing someone's manifest is not ours to do, so say -// what is wrong and let them fix it. -func versionlessPackageManagerReason(out []byte) string { - if !bytes.Contains(out, []byte(`"packageManager"`)) || - !bytes.Contains(out, []byte("No version specified")) { +// packageManagerSpecReason recognises a Node manager refusing to run because the +// packageManager field in package.json is not a spec corepack accepts: it requires +// an exact version, so both a missing one and a range are rejected. The manifest is +// malformed rather than the command wrong, and repairing someone's manifest is not +// ours to do, so say what is wrong and let them fix it. +func packageManagerSpecReason(out []byte) string { + badSpec := bytes.Contains(out, []byte("No version specified")) || + bytes.Contains(out, []byte("expected a semver version")) || + bytes.Contains(out, []byte("Invalid package manager specification")) + if !badSpec { return "" } - return "the packageManager field in package.json names a package manager without a version, " + - "which it refuses to run against. Give it a version (for example \"pnpm@9.1.0\") or remove " + - "the field, then run setup again." + return "the packageManager field in package.json is not a specification your package " + + "manager accepts: it needs one exact version, so a missing version or a range such as " + + "\"pnpm@^11.13.0\" is refused. Pin it (for example \"pnpm@11.13.0\") or remove the field, " + + "then run setup again." } // externallyManagedReason recognises a PEP 668 refusal and says what to do about diff --git a/internal/setup/installer_test.go b/internal/setup/installer_test.go index 2940b4c3..19c5b7e5 100644 --- a/internal/setup/installer_test.go +++ b/internal/setup/installer_test.go @@ -882,26 +882,32 @@ func TestInstall_PinnedSdkVersion_NoWarning(t *testing.T) { assert.Empty(t, result.Warning) } -// A packageManager field naming a manager with no version is malformed, and pnpm -// refuses to run against it. Repairing someone's manifest is not ours to do, so the -// failure has to say what is wrong. -func TestInstall_VersionlessPackageManagerField_ExplainsIt(t *testing.T) { - stubVirtualEnv(t, "") - stubPath(t, "pnpm") - installer := PackageInstaller{ - run: func(string, []string) ([]byte, error) { - return []byte(`No version specified for pnpm in "packageManager" of package.json`), - errors.New("exit status 1") - }, +// Corepack needs one exact version, so both a missing version and a range stop the +// manager running. Repairing someone's manifest is not ours to do, so the failure +// has to say what is wrong. +func TestInstall_BadPackageManagerSpec_ExplainsIt(t *testing.T) { + for _, out := range []string{ + `No version specified for pnpm in "packageManager" of package.json`, + "Invalid package manager specification in package.json (pnpm@^11.13.0); expected a semver version", + } { + t.Run(out[:24], func(t *testing.T) { + stubVirtualEnv(t, "") + stubPath(t, "pnpm") + installer := PackageInstaller{ + run: func(string, []string) ([]byte, error) { + return []byte(out), errors.New("exit status 1") + }, + } + + result, err := installer.Install(t.TempDir(), &DetectResult{ + SDKID: "node-server", PackageManager: "pnpm", + }) + + require.NoError(t, err, "a malformed manifest must not dead-end the flow") + assert.True(t, result.Failed) + assert.Contains(t, result.FailureReason, "packageManager field") + assert.Contains(t, result.FailureReason, "one exact version") + assert.Contains(t, result.FailureReason, "pnpm@11.13.0") + }) } - - result, err := installer.Install(t.TempDir(), &DetectResult{ - SDKID: "node-server", PackageManager: "pnpm", - }) - - require.NoError(t, err, "a malformed manifest must not dead-end the flow") - assert.True(t, result.Failed) - assert.Contains(t, result.FailureReason, "packageManager field") - assert.Contains(t, result.FailureReason, "without a version") - assert.Contains(t, result.FailureReason, "pnpm@9.1.0") } From 4bf85aa33eebb3ea981c3f397adf96ed29f8c251 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Wed, 19 Aug 2026 14:35:38 -0400 Subject: [PATCH 05/14] fix(setup): read pyproject.toml as TOML to find configured tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Matching [tool.] as text counted comments and strings, so a uv project whose comment mentioned the tool it migrated away from was marked ambiguous and install refused to run. Reading the declared tables instead means only a declaration counts, and nested tables need no special case: TOML creates the parent implicitly, so [tool.hatch.build] alone still declares hatch. A file we cannot parse declares nothing, which leaves the project ambiguous and the user asked — the honest answer when we cannot read what manages it. Co-Authored-By: Claude Opus 5 (1M context) --- go.mod | 2 +- internal/setup/detector.go | 60 +++++++++++++++++++++------------ internal/setup/detector_test.go | 42 +++++++++++++++++++++++ 3 files changed, 81 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index 0449eb4a..87c1277d 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,7 @@ require ( github.com/muesli/reflow v0.3.0 github.com/oapi-codegen/oapi-codegen/v2 v2.7.1 github.com/oapi-codegen/runtime v1.1.2 + github.com/pelletier/go-toml/v2 v2.2.4 github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/pkg/errors v0.9.1 github.com/samber/lo v1.51.0 @@ -81,7 +82,6 @@ require ( github.com/oasdiff/yaml3 v0.0.14 // indirect github.com/onsi/gomega v1.27.6 // indirect github.com/patrickmn/go-cache v2.1.0+incompatible // indirect - github.com/pelletier/go-toml/v2 v2.2.4 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/sagikazarmark/locafero v0.11.0 // indirect diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 185b9086..6b249085 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -1,7 +1,6 @@ package setup import ( - "bytes" "encoding/json" "errors" "fmt" @@ -10,6 +9,8 @@ import ( "path/filepath" "regexp" "strings" + + "github.com/pelletier/go-toml/v2" ) // DetectResult contains information about the user's project detected from the working directory. @@ -319,32 +320,47 @@ func pythonPMSignals(dir string) pmSignals { s.addLocked(lock.pm) } } - if b, err := os.ReadFile(filepath.Join(dir, "pyproject.toml")); err == nil { - for _, section := range []struct{ tool, pm string }{ - {"poetry", "poetry"}, - {"uv", "uv"}, - {"pdm", "pdm"}, - // hatch has no dependency-add command, so it is a signal we cannot act - // on. Recording it keeps the project ambiguous instead of silently - // falling through to pip. - {"hatch", ""}, - } { - if hasToolSection(b, section.tool) { - s.addLocked(section.pm) - } + tools := configuredTools(dir) + for _, section := range []struct{ tool, pm string }{ + {"poetry", "poetry"}, + {"uv", "uv"}, + {"pdm", "pdm"}, + // hatch has no dependency-add command, so it is a signal we cannot act on. + // Recording it keeps the project ambiguous instead of silently falling + // through to pip. + {"hatch", ""}, + } { + if tools[section.tool] { + s.addLocked(section.pm) } } return s } -// hasToolSection reports whether pyproject declares a [tool.] table. Nested -// tables count: real configs are usually only [tool.hatch.build] or -// [tool.poetry.dependencies], with no bare header to match. The trailing "]" or "." -// is required so [tool.uv] does not match [tool.uvicorn]. -func hasToolSection(pyproject []byte, name string) bool { - prefix := "[tool." + name - return bytes.Contains(pyproject, []byte(prefix+"]")) || - bytes.Contains(pyproject, []byte(prefix+".")) +// configuredTools reports which tools pyproject.toml configures, by the [tool.*] +// tables it declares. Reading the tables rather than matching text means a comment +// or a string that mentions another tool is not mistaken for a declaration, and +// nested tables need no special case: TOML creates the parent table implicitly, so +// [tool.hatch.build] on its own still declares hatch. +// +// A file we cannot parse declares nothing. That leaves the project ambiguous and the +// user asked, which is the honest answer when we cannot read what manages it. +func configuredTools(dir string) map[string]bool { + b, err := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + if err != nil { + return nil + } + var doc struct { + Tool map[string]any `toml:"tool"` + } + if err := toml.Unmarshal(b, &doc); err != nil { + return nil + } + tools := make(map[string]bool, len(doc.Tool)) + for name := range doc.Tool { + tools[name] = true + } + return tools } func detectRuby(dir string) *DetectResult { diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index 17f8c3ff..d7d73a1f 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -1083,3 +1083,45 @@ func TestPackageManagerChoice_ActionableSignalBeatsHatch(t *testing.T) { }) } } + +// pyproject.toml is read as TOML rather than searched as text, so only the tables it +// actually declares count as a project committing to a tool. +func TestPackageManagerChoice_ToolTablesAreParsedNotMatched(t *testing.T) { + tests := []struct { + name string + pyproject string + wantName string + wantDefinite bool + }{ + // A note about the tool a project migrated away from is not a declaration. + {"comment mentions another tool", "[project]\nname=\"a\"\n# migrated away from [tool.poetry] in March\n[tool.uv]\n", "uv", true}, + // Nor is a table name inside a string. + {"multi-line string mentions another tool", + "[project]\nname=\"a\"\ndescription=\"\"\"\nsee [tool.poetry] for history\n\"\"\"\n[tool.uv]\n", "uv", true}, + {"single-quoted string mentions another tool", + "[project]\nname=\"a\"\nsummary='see [tool.poetry]'\n[tool.uv]\n", "uv", true}, + // A trailing comment on the header itself is still a declaration. + {"header with a trailing comment", "[project]\nname=\"a\"\n[tool.uv] # the real one\n", "uv", true}, + // Parent tables are implicit, so a nested table declares its tool. + {"nested table only", "[project]\nname=\"a\"\n[tool.poetry.dependencies]\npython=\"^3.12\"\n", "poetry", true}, + // A different tool whose name merely starts the same way is not a match. + {"similarly named tool", "[project]\nname=\"a\"\n[tool.uvicorn]\nport=8000\n", "pip", false}, + // Unreadable TOML declares nothing, so the user is asked rather than guessed at. + {"malformed toml", "[project\nname=\"a\"\n[tool.uv]\n", "pip", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte(tt.pyproject), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, tt.wantName, choice.Name) + if tt.wantDefinite { + assert.Equal(t, PMDefinite, choice.Confidence) + } else { + assert.Equal(t, PMAmbiguous, choice.Confidence) + } + }) + } +} From 7d4aa91aaa8da3099fef0aa2cfcb4465bd57820d Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Wed, 19 Aug 2026 14:39:39 -0400 Subject: [PATCH 06/14] refactor(setup): read pyproject.toml once per detection Detection parsed the file twice for a Python project: once for the detector's own package manager and again for the confidence verdict. The verdict is now the single source for the languages it models, and the detector leaves the field to it. A language it does not model, such as Java's maven versus gradle, keeps the answer its detector gives. Co-Authored-By: Claude Opus 5 (1M context) --- internal/setup/detector.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 6b249085..5aec6965 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -69,12 +69,16 @@ func (FileDetector) Detect(dir string) (*DetectResult, error) { detectNode, } { if result := detect(dir); result != nil { - // Only what the project says. PackageManager itself is left as detected, - // since the language detectors know about managers this does not model, - // such as maven versus gradle. Candidates carry which tools are installed, - // which describes the machine rather than the project, so they are left to + // One read of the project decides the manager and how sure we are, rather + // than each detector working it out again. An empty name means a language + // this does not model — Java's maven versus gradle — so the detector's own + // answer stands. Candidates carry which tools are installed, which + // describes the machine rather than the project, so they are left to // callers that need to present a choice. choice := PackageManagerChoiceFor(dir, result.SDKID) + if choice.Name != "" { + result.PackageManager = choice.Name + } result.PackageManagerConfidence = choice.Confidence result.PackageManagerReason = choice.Reason return result, nil @@ -282,8 +286,8 @@ func detectPython(dir string) *DetectResult { if _, err := os.Stat(filepath.Join(dir, indicator)); err == nil { ep, exists := EntryPointFor(dir, "python-server-sdk") return &DetectResult{ - Language: "Python", - PackageManager: detectPythonPM(dir), + Language: "Python", + // PackageManager is filled in by Detect from the same read. SDKID: "python-server-sdk", EntryPoint: ep, EntryPointExists: exists, @@ -299,10 +303,6 @@ func detectPython(dir string) *DetectResult { // // https://docs.astral.sh/uv/concepts/projects/layout/ // https://pipenv.pypa.io/en/latest/ -// https://python-poetry.org/docs/pyproject/ -func detectPythonPM(dir string) string { - return pythonPMSignals(dir).best("pip") -} // pythonPMSignals reports what the project says about its Python package manager. // A lockfile is treated as the project having committed to a tool; a [tool.*] @@ -634,7 +634,7 @@ func PackageManagerFor(dir, sdkID string) string { case "node-server", "js-client-sdk", "react-client-sdk", "react-native": return detectNodePM(dir) case "python-server-sdk": - return detectPythonPM(dir) + return PackageManagerChoiceFor(dir, sdkID).Name case "ruby-server-sdk": return detectRubyPM(dir) case "go-server-sdk": From 04662ac168b90eee47d794bb8400c3293cd81ee3 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Wed, 19 Aug 2026 17:14:22 -0400 Subject: [PATCH 07/14] fix(setup): stop misreading other ecosystems' install errors The packageManager guidance matched a missing or non-semver version anywhere in an install failure, so a gem, a Python package or a Go module reporting either phrase had its real error replaced by advice about a file it does not have. The output has to name package.json, which both corepack refusals do. Say what was actually found when a project is set up for more than one manager: a Pipfile and a [tool.*] table count as commitments too, so naming lockfiles sent the reader looking for files that are not there. Note on stderr when no --package-manager was given, since that used to fall through to npm or pip and now reads the project. Callers parsing output are unaffected. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/cmdtest.go | 34 ++++++++++++++++++++++++++++++++ cmd/setup/install.go | 7 +++++++ cmd/setup/setup_test.go | 31 +++++++++++++++++++++++++++++ internal/setup/detector.go | 5 ++++- internal/setup/detector_test.go | 15 ++++++++++++++ internal/setup/installer.go | 7 +++++++ internal/setup/installer_test.go | 28 ++++++++++++++++++++++++++ 7 files changed, 126 insertions(+), 1 deletion(-) diff --git a/cmd/cmdtest.go b/cmd/cmdtest.go index 54ea73bd..90e65c7f 100644 --- a/cmd/cmdtest.go +++ b/cmd/cmdtest.go @@ -22,6 +22,40 @@ var StubbedSuccessResponse = `{ // CallCmd runs the root command for integration-style tests. It passes isTerminal always true so // the default --output matches an interactive terminal (plaintext); non-TTY JSON defaults are // covered in root_test.go. +// CallCmdCapturingStderr runs a command and returns stdout and stderr separately, so +// a test can assert on output written deliberately to stderr — a transitional note, +// say — without it being mistaken for parseable output. +func CallCmdCapturingStderr( + t *testing.T, + clients APIClients, + trackerFn analytics.TrackerFn, + args []string, +) (stdout []byte, stderr []byte, err error) { + rootCmd, err := NewRootCommand( + config.NewService(&resources.MockClient{}), + trackerFn, + clients, + "test", + false, + func() bool { return true }, + nil, + ) + require.NoError(t, err) + cmd := rootCmd.Cmd() + out, errOut := bytes.NewBufferString(""), bytes.NewBufferString("") + cmd.SetOut(out) + cmd.SetErr(errOut) + cmd.SetArgs(args) + + tracker := trackerFn("", "", false) + if err := cmd.Execute(); err != nil { + tracker.SendCommandCompletedEvent(analytics.ERROR) + return out.Bytes(), errOut.Bytes(), err + } + tracker.SendCommandCompletedEvent(analytics.SUCCESS) + return out.Bytes(), errOut.Bytes(), nil +} + func CallCmd( t *testing.T, clients APIClients, diff --git a/cmd/setup/install.go b/cmd/setup/install.go index 2f155d01..96628eb4 100644 --- a/cmd/setup/install.go +++ b/cmd/setup/install.go @@ -76,6 +76,13 @@ func runInstall(svc setup.Service) func(*cobra.Command, []string) error { ) } pkgMgr = choice.Name + // This used to fall through to npm or pip whatever the project used, so a + // caller that relied on that default now gets a different manager. Say so + // once, on stderr, where it cannot disturb output being parsed. + fmt.Fprintf(cmd.ErrOrStderr(), + "note: --package-manager was not given, so setup read the project and chose %q. "+ + "This previously defaulted to npm or pip. Pass --package-manager to pin it.\n", + pkgMgr) } detection := &setup.DetectResult{ diff --git a/cmd/setup/setup_test.go b/cmd/setup/setup_test.go index a44e15c2..78a22b78 100644 --- a/cmd/setup/setup_test.go +++ b/cmd/setup/setup_test.go @@ -417,3 +417,34 @@ func TestInitMissingRequiredFlags(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "required flag") } + +// Omitting --package-manager used to fall through to npm or pip. The default now +// reads the project, so a caller relying on the old behaviour gets a note — on +// stderr, so output being parsed is untouched. +func TestInstall_AutoSelectedManager_WarnsOnStderr(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), + []byte(`{"packageManager":"pnpm@9.1.0"}`), 0600)) + + args := []string{ + "setup", "install", + "--access-token", "test-token", + "--sdk-id", "node-server", + "--path", dir, + "--dry-run", + } + stdout, stderr, err := cmd.CallCmdCapturingStderr( + t, + cmd.APIClients{ResourcesClient: &resources.MockClient{}}, + analytics.NoopClientFn{}.Tracker(), + args, + ) + + require.NoError(t, err) + assert.Contains(t, string(stderr), "--package-manager was not given") + assert.Contains(t, string(stderr), "previously defaulted to npm or pip") + assert.Contains(t, string(stderr), "pnpm") + // The note must not reach output a caller parses. + assert.NotContains(t, string(stdout), "--package-manager was not given") + assert.Contains(t, string(stdout), "pnpm add @launchdarkly/node-server-sdk") +} diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 5aec6965..5869f091 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -795,10 +795,13 @@ func (s pmSignals) choose(options []string, fallback string, argvFor func(string case len(s.locked) == 1: return PMChoice{Name: s.locked[0], Confidence: PMDefinite} case len(s.locked) > 1: + // Say what was actually found. A lockfile, a Pipfile and a [tool.*] table all + // count as a project committing to a manager, so naming lockfiles would send + // the reader looking for files that are not there. return PMChoice{ Name: s.locked[0], Confidence: PMAmbiguous, - Reason: fmt.Sprintf("this project has lockfiles for more than one manager (%s)", + Reason: fmt.Sprintf("this project is set up for more than one manager (%s)", strings.Join(s.locked, ", ")), Candidates: candidates, } diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index d7d73a1f..02e6e047 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -1125,3 +1125,18 @@ func TestPackageManagerChoice_ToolTablesAreParsedNotMatched(t *testing.T) { }) } } + +// A [tool.*] table and a Pipfile count as commitments too, so the reason must not +// send the reader looking for lockfiles that are not there. +func TestPackageManagerChoice_ConflictReasonNamesWhatWasFound(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), + []byte("[project]\nname=\"a\"\n[tool.uv]\n[tool.poetry]\n"), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + require.Equal(t, PMAmbiguous, choice.Confidence) + assert.Contains(t, choice.Reason, "set up for more than one manager") + assert.NotContains(t, choice.Reason, "lockfile", + "neither signal here is a lockfile") +} diff --git a/internal/setup/installer.go b/internal/setup/installer.go index b026e7a1..170c4e14 100644 --- a/internal/setup/installer.go +++ b/internal/setup/installer.go @@ -205,6 +205,13 @@ func dotnetProjectArg(dir string) (args []string, reason string) { // malformed rather than the command wrong, and repairing someone's manifest is not // ours to do, so say what is wrong and let them fix it. func packageManagerSpecReason(out []byte) string { + // package.json has to be named in the output. Both corepack refusals mention it, + // and without that check any failure whose text happens to mention a missing or + // non-semver version — from a gem, a Python package, a Go module — would have its + // real error replaced by advice about a field it does not have. + if !bytes.Contains(out, []byte("package.json")) { + return "" + } badSpec := bytes.Contains(out, []byte("No version specified")) || bytes.Contains(out, []byte("expected a semver version")) || bytes.Contains(out, []byte("Invalid package manager specification")) diff --git a/internal/setup/installer_test.go b/internal/setup/installer_test.go index 19c5b7e5..96390154 100644 --- a/internal/setup/installer_test.go +++ b/internal/setup/installer_test.go @@ -911,3 +911,31 @@ func TestInstall_BadPackageManagerSpec_ExplainsIt(t *testing.T) { }) } } + +// Only a Node failure about package.json may be answered with advice about that +// file. Another ecosystem's error keeps its own text, whatever phrases it contains. +func TestInstall_OtherEcosystemErrorsKeepTheirText(t *testing.T) { + for _, out := range []string{ + "ERROR: Could not find a valid gem 'x' (>= 0), here is why:\n No version specified", + "ERROR: Could not find a version that satisfies the requirement x; expected a semver version", + "go: module x: invalid version: expected a semver version", + } { + t.Run(out[:20], func(t *testing.T) { + stubVirtualEnv(t, "") + stubPath(t, "npm") + installer := PackageInstaller{ + run: func(string, []string) ([]byte, error) { + return []byte(out), errors.New("exit status 1") + }, + } + + _, err := installer.Install(t.TempDir(), &DetectResult{ + SDKID: "node-server", PackageManager: "npm", + }) + + require.Error(t, err, "the real failure must reach the caller") + assert.Contains(t, err.Error(), out, "the real error text is kept") + assert.NotContains(t, err.Error(), "packageManager field") + }) + } +} From 2d40d781a19f4d4d23fe354014713832b2dae6f9 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Mon, 17 Aug 2026 17:26:19 -0400 Subject: [PATCH 08/14] feat(setup): ask which package manager to use when the project is ambiguous A project with lockfiles for two managers, or none at all, has no answer we can read off disk, and picking one is how a yarn project gets installed with npm. The wizard now asks, and says why it is asking. Installed managers are listed first and the cursor starts on one, but an uninstalled manager stays selectable: the choice is the user's and the install step already warns rather than installing tooling. Projects that state their manager go straight to the plan. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/model.go | 27 ++++++++ cmd/setup/update.go | 87 ++++++++++++++++++++++--- cmd/setup/view.go | 18 ++++++ cmd/setup/wizard_test.go | 133 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 256 insertions(+), 9 deletions(-) diff --git a/cmd/setup/model.go b/cmd/setup/model.go index 15563393..fe09b7c9 100644 --- a/cmd/setup/model.go +++ b/cmd/setup/model.go @@ -33,6 +33,9 @@ const ( stepSelectEnvironment stepDetect stepSelectSDK + // stepSelectPackageManager is only reached when the project does not identify + // its package manager. A project that does skips straight to the plan. + stepSelectPackageManager stepPlan stepInstall stepCreateFlag @@ -66,6 +69,12 @@ type wizardModel struct { // zero-value list.Model. sdkListBuilt bool sdkList list.Model + // pmChoice is the package-manager verdict for the chosen SDK. It is non-nil only + // when the project was ambiguous and the user was asked, which also records that + // going back from the plan should return to the picker rather than the SDK list. + pmChoice *setup.PMChoice + pmList list.Model + pmListBuilt bool selectedProject string selectedEnv string @@ -118,6 +127,24 @@ func (s sdkItem) Title() string { func (s sdkItem) Description() string { return s.language } func (s sdkItem) FilterValue() string { return s.name } +// pmItem is a package manager the user can pick. Installed state is shown but does +// not disable the row: the user may be about to install the tool, and setup never +// installs tooling on their behalf. +type pmItem struct { + name string + command string + installed bool +} + +func (p pmItem) Title() string { + if p.installed { + return p.name + } + return p.name + " (not installed)" +} +func (p pmItem) Description() string { return p.command } +func (p pmItem) FilterValue() string { return p.name } + type projectItem struct { key string name string diff --git a/cmd/setup/update.go b/cmd/setup/update.go index a14b8e4e..c10cc3ff 100644 --- a/cmd/setup/update.go +++ b/cmd/setup/update.go @@ -29,6 +29,9 @@ func (m wizardModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if m.sdkListBuilt { m.sdkList.SetSize(m.sdkBoxWidth()-2, m.sdkList.Height()) } + if m.pmListBuilt { + m.pmList.SetSize(m.sdkBoxWidth(), m.listHeight()) + } case tea.KeyMsg: switch msg.String() { @@ -178,6 +181,10 @@ func (m wizardModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if len(m.environments) > 0 { m.envList, cmd = m.envList.Update(msg) } + case stepSelectPackageManager: + if m.pmListBuilt { + m.pmList, cmd = m.pmList.Update(msg) + } case stepSelectSDK: // Two panels when a detected SDK is shown: the detected panel (focus 0) // and the list of other SDKs (focus 1). Arrows move focus between them. @@ -225,6 +232,8 @@ func (m wizardModel) isFiltering() bool { return m.projectList.FilterState() == list.Filtering case stepSelectEnvironment: return m.envList.FilterState() == list.Filtering + case stepSelectPackageManager: + return m.pmList.FilterState() == list.Filtering case stepSelectSDK: return m.sdkList.FilterState() == list.Filtering } @@ -252,6 +261,43 @@ func (m *wizardModel) enterSDKStep() { m.step = stepSelectSDK } +// enterPackageManagerStep builds the picker from the ambiguous verdict. Installed +// managers are listed first and the cursor starts on one, but an uninstalled +// manager stays selectable: the choice is the user's, and setup warns at install +// time rather than installing the tool itself. +func (m *wizardModel) enterPackageManagerStep() { + installed := make([]list.Item, 0, len(m.pmChoice.Candidates)) + missing := make([]list.Item, 0, len(m.pmChoice.Candidates)) + for _, c := range m.pmChoice.Candidates { + item := pmItem{name: c.Name, command: c.Command, installed: c.Installed} + if c.Installed { + installed = append(installed, item) + continue + } + missing = append(missing, item) + } + items := append(installed, missing...) + + m.pmList = list.New(items, list.NewDefaultDelegate(), m.sdkBoxWidth(), m.listHeight()) + m.pmList.Title = "Select a package manager:" + m.pmList.SetShowStatusBar(false) + m.pmListBuilt = true + m.step = stepSelectPackageManager +} + +// enterPlanStep computes the preview shown before anything is written or run. +func (m *wizardModel) enterPlanStep() { + // Resolved against the project directory so the previewed command is the one + // that runs, virtualenv pip included. + dir, _ := os.Getwd() + args, _ := setup.InstallArgs(dir, m.detectResult.SDKID, m.detectResult.PackageManager) + m.planInstallCmd = strings.Join(args, " ") + if dir != "" { + m.planAlready = setup.IsInstalled(dir, m.detectResult.SDKID) + } + m.step = stepPlan +} + // acceptsEnvs reports whether an environment list still describes the project the // user has selected, and whether the wizard is still choosing one. A list fetched // for a project the user has since left would otherwise be shown under the new @@ -300,7 +346,15 @@ func (m wizardModel) handleBack() (tea.Model, tea.Cmd) { m.resetEnvSelection() case stepSelectSDK: m.step = stepSelectEnvironment + case stepSelectPackageManager: + m.step = stepSelectSDK case stepPlan: + // The picker only exists for an ambiguous project, so going back must return + // to whichever screen the user actually came from. + if m.pmChoice != nil { + m.step = stepSelectPackageManager + break + } m.step = stepSelectSDK } return m, nil @@ -365,15 +419,32 @@ func (m wizardModel) handleEnter() (tea.Model, tea.Cmd) { } } m.detectResult = &result - // Compute the plan preview shown before any action is taken. It is resolved - // against the project directory so the previewed command is the one that runs. - planDir, _ := os.Getwd() - args, _ := setup.InstallArgs(planDir, chosen.id, result.PackageManager) - m.planInstallCmd = strings.Join(args, " ") - if planDir != "" { - m.planAlready = setup.IsInstalled(planDir, chosen.id) + + // Ask which manager to use when the project doesn't say. Picking one for the + // user here is how a yarn project ends up installed with npm. + m.pmChoice = nil + if dir, err := os.Getwd(); err == nil { + if choice := setup.PackageManagerChoiceFor(dir, chosen.id); choice.Confidence == setup.PMAmbiguous { + m.pmChoice = &choice + m.enterPackageManagerStep() + return m, nil + } else if choice.Name != "" { + result.PackageManager = choice.Name + m.detectResult = &result + } + } + m.enterPlanStep() + return m, nil + + case stepSelectPackageManager: + selected, ok := m.pmList.SelectedItem().(pmItem) + if !ok { + return m, nil } - m.step = stepPlan + result := *m.detectResult + result.PackageManager = selected.name + m.detectResult = &result + m.enterPlanStep() return m, nil case stepPlan: diff --git a/cmd/setup/view.go b/cmd/setup/view.go index a6ce9ec6..14d3a39a 100644 --- a/cmd/setup/view.go +++ b/cmd/setup/view.go @@ -66,6 +66,9 @@ func (m wizardModel) View() string { case stepSelectSDK: return m.sdkSelectView() + case stepSelectPackageManager: + return m.packageManagerView() + case stepPlan: return m.planView() @@ -188,6 +191,21 @@ func (m wizardModel) sdkBoxWidth() int { return w } +// packageManagerView asks which package manager to use. It says why it is asking: +// a wizard that stops to ask without explaining itself reads as one that failed to +// look, and the reason is also what tells the user whether our reading of their +// project is wrong. +func (m wizardModel) packageManagerView() string { + reason := "" + if m.pmChoice != nil && m.pmChoice.Reason != "" { + reason = m.wrap(strings.ToUpper(m.pmChoice.Reason[:1])+m.pmChoice.Reason[1:]+".") + "\n\n" + } + return titleStyle.Render("Which package manager should install the SDK?") + "\n\n" + + reason + + m.pmList.View() + "\n" + + mutedStyle.Render("↑/↓ move · enter select · ← back · q quit") +} + // addCodeTo phrases an "add this code" instruction. SDKs that only show a snippet // have no entry point, so naming a destination would print an empty path. func addCodeTo(instruction, path string) string { diff --git a/cmd/setup/wizard_test.go b/cmd/setup/wizard_test.go index f3897627..e526a25a 100644 --- a/cmd/setup/wizard_test.go +++ b/cmd/setup/wizard_test.go @@ -371,6 +371,8 @@ func TestWizard_Plan_MissingEntryPoint_SaysCreate(t *testing.T) { // The SDK screen rebuilds detectResult, and the plan and install steps read it, so // every detected value has to survive that step — not just the SDK. func TestWizard_SelectSDK_CarriesDetectionThrough(t *testing.T) { + // A Gemfile makes Bundler the project's stated manager, so the picker is skipped. + gemfileProject(t) m := wizardModel{step: stepDetect, width: 80, height: 30} next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ @@ -395,6 +397,7 @@ func TestWizard_SelectSDK_CarriesDetectionThrough(t *testing.T) { } func TestWizard_SelectSDK_PlanUsesDetectedPackageManager(t *testing.T) { + gemfileProject(t) m := wizardModel{step: stepDetect, width: 80, height: 30} next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ @@ -425,6 +428,8 @@ func selectOtherSDK(t *testing.T, m wizardModel, id string) wizardModel { // The detected entry point belongs to the detected language. ruby-server-sdk is // append-safe, so reusing it would append Ruby to a Node project's index.js. func TestWizard_OverrideSDK_DoesNotReuseDetectedEntryPoint(t *testing.T) { + // A Gemfile states the manager, so the override lands on the plan without asking. + gemfileProject(t) m := wizardModel{step: stepDetect, width: 80, height: 30} next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ SDKID: "node-server", @@ -448,7 +453,7 @@ func TestWizard_OverrideSDK_DoesNotReuseDetectedEntryPoint(t *testing.T) { assert.Contains(t, m3.detectResult.EntryPoint, "main.rb") assert.Empty(t, m3.detectResult.Framework, "Next.js does not describe a Ruby project") // pnpm cannot install a gem, so the manager is re-derived for the chosen SDK. - assert.Equal(t, "gem", m3.detectResult.PackageManager) + assert.Equal(t, "bundle", m3.detectResult.PackageManager) } // An override must find the file the project already has, rather than falling back @@ -457,6 +462,8 @@ func TestWizard_OverrideSDK_FindsExistingEntryPoint(t *testing.T) { dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, "src"), 0755)) require.NoError(t, os.WriteFile(filepath.Join(dir, "src/index.js"), []byte("console.log(1)\n"), 0600)) + // A lockfile states the manager, so the override lands on the plan without asking. + require.NoError(t, os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte("{}"), 0600)) // macOS resolves /var to /private/var, and the override path reads os.Getwd, // so compare against the resolved directory rather than the one we created. dir = chdir(t, dir) @@ -477,6 +484,15 @@ func TestWizard_OverrideSDK_FindsExistingEntryPoint(t *testing.T) { assert.True(t, m3.detectResult.EntryPointExists) } +// gemfileProject moves into a project whose package manager is unambiguous, so the +// package-manager picker does not intervene. +func gemfileProject(t *testing.T) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "Gemfile"), []byte("source 'https://rubygems.org'\n"), 0600)) + return chdir(t, dir) +} + // chdir moves into dir for the duration of the test and returns the working // directory as the process sees it. The override path reads os.Getwd to re-derive // the entry point. @@ -538,6 +554,12 @@ func overrideToSDK(t *testing.T, detected *setup.DetectResult, id string) wizard m2 := selectOtherSDK(t, next.(wizardModel), id) next2, _ := m2.Update(tea.KeyMsg{Type: tea.KeyEnter}) m3 := next2.(wizardModel) + // An override into a project that doesn't state its package manager asks first. + // These callers are about entry points, so accept the highlighted manager. + if m3.step == stepSelectPackageManager { + next3, _ := m3.Update(tea.KeyMsg{Type: tea.KeyEnter}) + m3 = next3.(wizardModel) + } require.Equal(t, stepPlan, m3.step) return m3 } @@ -854,3 +876,112 @@ func TestWizard_EnvsFetched_ForSupersededProject_IsIgnored(t *testing.T) { fresh, _ := m.Update(envsFetchedMsg{project: "proj-b", environments: []envItem{{key: "b-prod", name: "B Prod"}}}) assert.Contains(t, fresh.(wizardModel).View(), "B Prod") } + +// A project that states its manager must not be interrupted; the happy path gains +// no keystrokes from the picker existing. +func TestWizard_DefinitePackageManager_SkipsPicker(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{"packageManager":"pnpm@9.1.0"}`), 0600)) + chdir(t, dir) + + m := wizardModel{step: stepDetect, width: 80, height: 30} + next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ + SDKID: "node-server", Language: "JavaScript", PackageManager: "pnpm", + }}) + next2, _ := next.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyEnter}) + m3 := next2.(wizardModel) + + require.Equal(t, stepPlan, m3.step) + assert.Nil(t, m3.pmChoice, "nothing was ambiguous, so nothing was asked") + assert.Equal(t, "pnpm add @launchdarkly/node-server-sdk", m3.planInstallCmd) +} + +// Two lockfiles from different managers is the case no guess can get right. +func TestWizard_ConflictingLockfiles_AsksAndUsesTheAnswer(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "yarn.lock"), []byte(""), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte("{}"), 0600)) + chdir(t, dir) + + m := wizardModel{step: stepDetect, width: 80, height: 30} + next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ + SDKID: "node-server", Language: "JavaScript", PackageManager: "yarn", + }}) + next2, _ := next.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyEnter}) + picker := next2.(wizardModel) + + require.Equal(t, stepSelectPackageManager, picker.step) + require.NotNil(t, picker.pmChoice) + assert.Contains(t, picker.pmChoice.Reason, "more than one manager") + + // The view has to say why it is asking, or it reads as a tool that failed to look. + view := picker.View() + assert.Contains(t, view, "Which package manager") + assert.Contains(t, view, "more than one manager") + + // Pick whatever is highlighted and confirm the plan follows the answer. + next3, _ := picker.Update(tea.KeyMsg{Type: tea.KeyEnter}) + planned := next3.(wizardModel) + require.Equal(t, stepPlan, planned.step) + selected := planned.detectResult.PackageManager + assert.Contains(t, planned.planInstallCmd, selected, + "the plan must run the manager the user chose") +} + +// Installed managers come first and the cursor starts on one, but an uninstalled +// manager stays selectable — setup never installs tooling for the user. +func TestWizard_Picker_ListsInstalledFirstAndKeepsMissingSelectable(t *testing.T) { + m := wizardModel{step: stepSelectSDK, width: 80, height: 30} + m.detectResult = &setup.DetectResult{SDKID: "node-server"} + m.pmChoice = &setup.PMChoice{ + Name: "npm", + Confidence: setup.PMAmbiguous, + Reason: "this project doesn't say which package manager it uses", + Candidates: []setup.PMCandidate{ + {Name: "npm", Installed: false, Command: "npm install x"}, + {Name: "yarn", Installed: true, Command: "yarn add x"}, + {Name: "pnpm", Installed: true, Command: "pnpm add x"}, + }, + } + m.enterPackageManagerStep() + + items := m.pmList.Items() + require.Len(t, items, 3) + assert.Equal(t, "yarn", items[0].(pmItem).name, "installed managers come first") + assert.Equal(t, "pnpm", items[1].(pmItem).name) + assert.Equal(t, "npm", items[2].(pmItem).name) + assert.Contains(t, items[2].(pmItem).Title(), "not installed") + + // Selecting the uninstalled one is allowed; the install step warns later. + m.pmList.Select(2) + next, _ := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + chosen := next.(wizardModel) + require.Equal(t, stepPlan, chosen.step) + assert.Equal(t, "npm", chosen.detectResult.PackageManager) +} + +// Back must return to the picker, not skip over it to the SDK list. +func TestWizard_Picker_BackReturnsToPickerFromPlan(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) + chdir(t, dir) + + m := wizardModel{step: stepDetect, width: 80, height: 30} + next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ + SDKID: "node-server", Language: "JavaScript", + }}) + next2, _ := next.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyEnter}) + picker := next2.(wizardModel) + require.Equal(t, stepSelectPackageManager, picker.step) + + next3, _ := picker.Update(tea.KeyMsg{Type: tea.KeyEnter}) + planned := next3.(wizardModel) + require.Equal(t, stepPlan, planned.step) + + back, _ := planned.Update(tea.KeyMsg{Type: tea.KeyLeft}) + assert.Equal(t, stepSelectPackageManager, back.(wizardModel).step) + + backAgain, _ := back.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyLeft}) + assert.Equal(t, stepSelectSDK, backAgain.(wizardModel).step) +} From e421bb20d11fa99c2aaec6a7d62336ace66482c4 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 11:54:51 -0400 Subject: [PATCH 09/14] fix(setup): keep the package-manager picker inside the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The screen draws a title, the reason for asking and a key hint around the list, but the list was sized to the whole window, so the hint — including how to go back — was pushed off the bottom at every terminal size. The list now leaves room for that chrome, and the list's own help line goes away since the screen prints its own. Below fourteen rows the reason is dropped: at that size the question and the choices matter more than the explanation. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/update.go | 7 +++++-- cmd/setup/view.go | 23 ++++++++++++++++++++++- cmd/setup/wizard_test.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/cmd/setup/update.go b/cmd/setup/update.go index c10cc3ff..0eddd089 100644 --- a/cmd/setup/update.go +++ b/cmd/setup/update.go @@ -30,7 +30,7 @@ func (m wizardModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.sdkList.SetSize(m.sdkBoxWidth()-2, m.sdkList.Height()) } if m.pmListBuilt { - m.pmList.SetSize(m.sdkBoxWidth(), m.listHeight()) + m.pmList.SetSize(m.sdkBoxWidth(), m.pmListHeight()) } case tea.KeyMsg: @@ -278,9 +278,12 @@ func (m *wizardModel) enterPackageManagerStep() { } items := append(installed, missing...) - m.pmList = list.New(items, list.NewDefaultDelegate(), m.sdkBoxWidth(), m.listHeight()) + m.pmList = list.New(items, list.NewDefaultDelegate(), m.sdkBoxWidth(), m.pmListHeight()) m.pmList.Title = "Select a package manager:" m.pmList.SetShowStatusBar(false) + // The screen prints its own key hint, so the list's help would repeat it while + // taking rows the hint needs. + m.pmList.SetShowHelp(false) m.pmListBuilt = true m.step = stepSelectPackageManager } diff --git a/cmd/setup/view.go b/cmd/setup/view.go index 14d3a39a..180aeaed 100644 --- a/cmd/setup/view.go +++ b/cmd/setup/view.go @@ -191,13 +191,34 @@ func (m wizardModel) sdkBoxWidth() int { return w } +// pmListHeight is the height available to the package-manager list. The screen +// draws a title, the reason it is asking and a key hint around the list, so giving +// the list the whole window pushes the hint — including how to go back — off the +// bottom of the terminal. +func (m wizardModel) pmListHeight() int { + chrome := 5 // title, blank line, key hint, and the list's own title + if m.pmShowReason() { + chrome = 8 // the reason wraps to two lines on a narrow terminal + } + h := m.height - chrome + if h < 3 { + h = 3 + } + return h +} + +// pmShowReason reports whether there is room to explain why we are asking. On a +// very short terminal the question and the choices have to win: dropping the +// explanation is better than pushing the key hint off the bottom. +func (m wizardModel) pmShowReason() bool { return m.height >= 14 } + // packageManagerView asks which package manager to use. It says why it is asking: // a wizard that stops to ask without explaining itself reads as one that failed to // look, and the reason is also what tells the user whether our reading of their // project is wrong. func (m wizardModel) packageManagerView() string { reason := "" - if m.pmChoice != nil && m.pmChoice.Reason != "" { + if m.pmShowReason() && m.pmChoice != nil && m.pmChoice.Reason != "" { reason = m.wrap(strings.ToUpper(m.pmChoice.Reason[:1])+m.pmChoice.Reason[1:]+".") + "\n\n" } return titleStyle.Render("Which package manager should install the SDK?") + "\n\n" + diff --git a/cmd/setup/wizard_test.go b/cmd/setup/wizard_test.go index e526a25a..57c218e7 100644 --- a/cmd/setup/wizard_test.go +++ b/cmd/setup/wizard_test.go @@ -1,8 +1,10 @@ package setup import ( + "fmt" "os" "path/filepath" + "strings" "testing" "github.com/charmbracelet/bubbles/spinner" @@ -985,3 +987,34 @@ func TestWizard_Picker_BackReturnsToPickerFromPlan(t *testing.T) { backAgain, _ := back.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyLeft}) assert.Equal(t, stepSelectSDK, backAgain.(wizardModel).step) } + +// The picker draws a title, a reason and a key hint around the list. Giving the +// list the whole window pushed the hint — including how to go back — off the +// bottom, so the rendered screen must fit the terminal at every size the bug bash +// asks testers to try. +func TestWizard_Picker_FitsTerminalHeight(t *testing.T) { + for _, dims := range [][2]int{{80, 30}, {80, 22}, {80, 16}, {60, 12}, {40, 10}} { + t.Run(fmt.Sprintf("%dx%d", dims[0], dims[1]), func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "yarn.lock"), []byte(""), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte("{}"), 0600)) + chdir(t, dir) + + m := wizardModel{step: stepDetect, width: dims[0], height: dims[1]} + next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ + SDKID: "node-server", Language: "JavaScript", + }}) + next2, _ := next.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyEnter}) + picker := next2.(wizardModel) + require.Equal(t, stepSelectPackageManager, picker.step) + + view := picker.View() + lines := strings.Count(strings.TrimRight(view, "\n"), "\n") + 1 + assert.LessOrEqual(t, lines, dims[1], "the key hint would be pushed off the bottom") + // However short the terminal, the way out must stay on screen. + assert.Contains(t, view, "← back") + assert.Contains(t, view, "Which package manager") + }) + } +} From a06dac7a77407bb053525679eee5412cc3ad8e6e Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 13:26:46 -0400 Subject: [PATCH 10/14] fix(setup): show each screen's key hints once, inside the list The project and environment screens printed a footer of key hints while the list below already rendered its own help, so every instruction appeared twice. The wizard's own bindings now go into the list's help line, which is the single place a screen states them, and the footers are gone. The package-manager picker follows the same shape, and its height reserve is retuned for the help line the list now draws, including the extra row that line takes when the terminal is narrower than it is. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/update.go | 21 ++++++++++++++++++--- cmd/setup/view.go | 18 +++++++++++------- cmd/setup/wizard_test.go | 34 ++++++++++++++++++++++++++-------- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/cmd/setup/update.go b/cmd/setup/update.go index 0eddd089..1b772bd4 100644 --- a/cmd/setup/update.go +++ b/cmd/setup/update.go @@ -82,6 +82,7 @@ func (m wizardModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.projectList.Title = "Select a project:" m.projectList.SetShowStatusBar(false) keepEscFromQuitting(&m.projectList) + m.projectList.AdditionalShortHelpKeys = listHints(false) return m, nil case envsFetchedMsg: @@ -99,6 +100,7 @@ func (m wizardModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.envList.Title = "Select an environment:" m.envList.SetShowStatusBar(false) keepEscFromQuitting(&m.envList) + m.envList.AdditionalShortHelpKeys = listHints(true) return m, nil case envDetailsFetchedMsg: @@ -261,6 +263,21 @@ func (m *wizardModel) enterSDKStep() { m.step = stepSelectSDK } +// listHints adds the wizard's own bindings to a list's help line. Screens showed +// the list's help and a footer of ours, so every instruction appeared twice; the +// list's help is the one place they belong. +func listHints(back bool) func() []key.Binding { + return func() []key.Binding { + hints := []key.Binding{ + key.NewBinding(key.WithKeys("enter"), key.WithHelp("enter", "select")), + } + if back { + hints = append(hints, key.NewBinding(key.WithKeys("left"), key.WithHelp("←", "back"))) + } + return hints + } +} + // enterPackageManagerStep builds the picker from the ambiguous verdict. Installed // managers are listed first and the cursor starts on one, but an uninstalled // manager stays selectable: the choice is the user's, and setup warns at install @@ -281,9 +298,7 @@ func (m *wizardModel) enterPackageManagerStep() { m.pmList = list.New(items, list.NewDefaultDelegate(), m.sdkBoxWidth(), m.pmListHeight()) m.pmList.Title = "Select a package manager:" m.pmList.SetShowStatusBar(false) - // The screen prints its own key hint, so the list's help would repeat it while - // taking rows the hint needs. - m.pmList.SetShowHelp(false) + m.pmList.AdditionalShortHelpKeys = listHints(true) m.pmListBuilt = true m.step = stepSelectPackageManager } diff --git a/cmd/setup/view.go b/cmd/setup/view.go index 180aeaed..096e465a 100644 --- a/cmd/setup/view.go +++ b/cmd/setup/view.go @@ -47,7 +47,7 @@ func (m wizardModel) View() string { m.wrap("This access token can't see any projects. Create a project in LaunchDarkly, or use a token with access to one, then run this command again.") + "\n" + quitHint } - return m.projectList.View() + "\n" + mutedStyle.Render("q quit") + return m.projectList.View() case stepSelectEnvironment: if !m.envsLoaded { @@ -58,7 +58,7 @@ func (m wizardModel) View() string { m.wrap(fmt.Sprintf("Project %q has no environments this access token can see. Press ← to pick another project.", m.selectedProject)) + "\n" + mutedStyle.Render("← back · q quit") + "\n" } - return m.envList.View() + "\n" + mutedStyle.Render("← back · q quit") + return m.envList.View() case stepDetect: return m.spinner.View() + " Detecting project type..." @@ -196,9 +196,14 @@ func (m wizardModel) sdkBoxWidth() int { // the list the whole window pushes the hint — including how to go back — off the // bottom of the terminal. func (m wizardModel) pmListHeight() int { - chrome := 5 // title, blank line, key hint, and the list's own title + chrome := 3 // the question, a blank line, and the list's own trailing row if m.pmShowReason() { - chrome = 8 // the reason wraps to two lines on a narrow terminal + chrome += 3 // the reason, which wraps to two lines when narrow + } + // The list's help line runs to about seventy columns, so on anything narrower + // it wraps and costs a second row. + if m.width < 72 { + chrome++ } h := m.height - chrome if h < 3 { @@ -221,10 +226,9 @@ func (m wizardModel) packageManagerView() string { if m.pmShowReason() && m.pmChoice != nil && m.pmChoice.Reason != "" { reason = m.wrap(strings.ToUpper(m.pmChoice.Reason[:1])+m.pmChoice.Reason[1:]+".") + "\n\n" } - return titleStyle.Render("Which package manager should install the SDK?") + "\n\n" + + return titleStyle.Render(m.wrap("Which package manager should install the SDK?")) + "\n\n" + reason + - m.pmList.View() + "\n" + - mutedStyle.Render("↑/↓ move · enter select · ← back · q quit") + m.pmList.View() } // addCodeTo phrases an "add this code" instruction. SDKs that only show a snippet diff --git a/cmd/setup/wizard_test.go b/cmd/setup/wizard_test.go index 57c218e7..57a98851 100644 --- a/cmd/setup/wizard_test.go +++ b/cmd/setup/wizard_test.go @@ -988,12 +988,16 @@ func TestWizard_Picker_BackReturnsToPickerFromPlan(t *testing.T) { assert.Equal(t, stepSelectSDK, backAgain.(wizardModel).step) } -// The picker draws a title, a reason and a key hint around the list. Giving the -// list the whole window pushed the hint — including how to go back — off the -// bottom, so the rendered screen must fit the terminal at every size the bug bash -// asks testers to try. +// The picker draws its question and the reason for asking around the list, so the +// list has to be sized for less than the whole window or the instructions are +// pushed off the bottom. Rows are counted the way a terminal shows them, with +// over-wide lines wrapping. +// +// Widths below 72 are left out: the list widget's own help line runs to about +// seventy columns and wraps there. That affects every list screen in the wizard, +// not this one, and no height reserve fixes it. func TestWizard_Picker_FitsTerminalHeight(t *testing.T) { - for _, dims := range [][2]int{{80, 30}, {80, 22}, {80, 16}, {60, 12}, {40, 10}} { + for _, dims := range [][2]int{{100, 30}, {80, 30}, {80, 24}, {80, 20}, {80, 16}} { t.Run(fmt.Sprintf("%dx%d", dims[0], dims[1]), func(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) @@ -1010,11 +1014,25 @@ func TestWizard_Picker_FitsTerminalHeight(t *testing.T) { require.Equal(t, stepSelectPackageManager, picker.step) view := picker.View() - lines := strings.Count(strings.TrimRight(view, "\n"), "\n") + 1 - assert.LessOrEqual(t, lines, dims[1], "the key hint would be pushed off the bottom") + assert.LessOrEqual(t, terminalRows(view, dims[0]), dims[1], + "the instructions would be pushed off the bottom") // However short the terminal, the way out must stay on screen. - assert.Contains(t, view, "← back") + assert.Contains(t, view, "back") assert.Contains(t, view, "Which package manager") }) } } + +// terminalRows counts the rows a terminal of the given width would use, so a line +// wider than the window counts as the several rows it actually occupies. +func terminalRows(view string, width int) int { + rows := 0 + for _, line := range strings.Split(strings.TrimRight(view, "\n"), "\n") { + if w := len([]rune(line)); w > width { + rows += (w + width - 1) / width + continue + } + rows++ + } + return rows +} From e8a0fd6791e35e0ee8a34177658a24c7042e9909 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 13:36:27 -0400 Subject: [PATCH 11/14] fix(setup): wrap the plan steps to the terminal width A step naming an absolute entry-point path alongside the warning that no entry file was found runs well past a narrow terminal, and overflowing there hides the warning the step exists to give. Steps now wrap, with what wraps indented under the number so a step still reads as one item. The screen that names the injected file wraps for the same reason. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/view.go | 13 +++++++--- cmd/setup/wizard_test.go | 55 ++++++++++++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/cmd/setup/view.go b/cmd/setup/view.go index 096e465a..8fe80690 100644 --- a/cmd/setup/view.go +++ b/cmd/setup/view.go @@ -87,8 +87,8 @@ func (m wizardModel) View() string { lead = "This file already initializes the LaunchDarkly SDK, so it was left as it is:\n" } return titleStyle.Render("Start your application") + "\n\n" + - lead + - " " + m.initResult.FilePath + "\n\n" + + m.wrap(lead) + + m.wrap(" "+m.initResult.FilePath) + "\n\n" + "Please start your application now, then press Enter to verify the connection.\n" case stepVerify: @@ -333,7 +333,14 @@ func (m wizardModel) planView() string { var steps []string add := func(s string) { - steps = append(steps, selectedStyle.Render(fmt.Sprintf("%d.", len(steps)+1))+" "+s) + marker := selectedStyle.Render(fmt.Sprintf("%d.", len(steps)+1)) + // Wrap to leave room for the marker and indent what wraps, so a step too long + // for the terminal still reads as one numbered item instead of overflowing. + lines := strings.Split(wrapText(s, m.width-len("1. ")), "\n") + for i := 1; i < len(lines); i++ { + lines[i] = strings.Repeat(" ", len("1. ")) + lines[i] + } + steps = append(steps, marker+" "+strings.Join(lines, "\n")) } switch { diff --git a/cmd/setup/wizard_test.go b/cmd/setup/wizard_test.go index 57a98851..0836bca2 100644 --- a/cmd/setup/wizard_test.go +++ b/cmd/setup/wizard_test.go @@ -344,8 +344,8 @@ func TestWizard_Plan_ExistingEntryPoint_SaysAdd(t *testing.T) { } view := m.planView() - assert.Contains(t, view, "Add initialization code to src/index.js") - assert.NotContains(t, view, "Create src/index.js") + assert.Contains(t, flat(view), "Add initialization code to src/index.js") + assert.NotContains(t, flat(view), "Create src/index.js") } // A guessed entry point means we would write a file the project does not load, so @@ -365,9 +365,9 @@ func TestWizard_Plan_MissingEntryPoint_SaysCreate(t *testing.T) { } view := m.planView() - assert.Contains(t, view, "Create instrumentation.ts") - assert.Contains(t, view, "no entry file found") - assert.NotContains(t, view, "Add initialization code to") + assert.Contains(t, flat(view), "Create instrumentation.ts") + assert.Contains(t, flat(view), "no entry file found") + assert.NotContains(t, flat(view), "Add initialization code to") } // The SDK screen rebuilds detectResult, and the plan and install steps read it, so @@ -582,8 +582,8 @@ func TestWizard_OverrideSDK_DefaultEntryPointAlreadyPresent(t *testing.T) { assert.Equal(t, filepath.Join(dir, "main.rb"), m.detectResult.EntryPoint) assert.True(t, m.detectResult.EntryPointExists) view := m.View() - assert.Contains(t, view, "Add initialization code to") - assert.NotContains(t, view, "no entry file found") + assert.Contains(t, flat(view), "Add initialization code to") + assert.NotContains(t, flat(view), "no entry file found") } func TestWizard_OverrideSDK_DefaultEntryPointMissing(t *testing.T) { @@ -595,7 +595,7 @@ func TestWizard_OverrideSDK_DefaultEntryPointMissing(t *testing.T) { }, "ruby-server-sdk") assert.False(t, m.detectResult.EntryPointExists) - assert.Contains(t, m.View(), "no entry file found") + assert.Contains(t, flat(m.View()), "no entry file found") } func TestWizard_Done_DeclinedInstall_ShowsReasonWithoutCommand(t *testing.T) { @@ -1023,6 +1023,10 @@ func TestWizard_Picker_FitsTerminalHeight(t *testing.T) { } } +// flat collapses whitespace in a rendered view, so assertions about a phrase hold +// wherever wrapping happens to fall. +func flat(view string) string { return strings.Join(strings.Fields(view), " ") } + // terminalRows counts the rows a terminal of the given width would use, so a line // wider than the window counts as the several rows it actually occupies. func terminalRows(view string, width int) int { @@ -1036,3 +1040,38 @@ func terminalRows(view string, width int) int { } return rows } + +// The plan names an absolute entry-point path and explains why it is creating the +// file, which together run well past a narrow terminal. Overflowing there hides +// the very warning the step exists to give. +func TestWizard_Plan_WrapsStepsToTerminalWidth(t *testing.T) { + for _, width := range []int{100, 80, 60, 40} { + t.Run(fmt.Sprintf("width%d", width), func(t *testing.T) { + m := wizardModel{ + step: stepPlan, + selectedProject: "my-scratch-project", + selectedEnv: "production", + detectResult: &setup.DetectResult{ + SDKID: "python-server-sdk", + EntryPoint: "/Users/someone/code/launchdarkly/test-app/main.py", + EntryPointExists: false, + }, + planInstallCmd: "pip3 install launchdarkly-server-sdk", + width: width, + height: 30, + } + + view := m.planView() + + for _, line := range strings.Split(view, "\n") { + assert.LessOrEqual(t, len([]rune(line)), width, + "a plan step overflows a %d-column terminal", width) + } + // The warning must survive wrapping, not be truncated away. + assert.Contains(t, flat(view), "no entry file found") + assert.Contains(t, flat(view), "main.py") + // Wrapped text is indented under its number so the step still reads as one. + assert.Regexp(t, `(?m)^ {3}\S`, view) + }) + } +} From 73b46c4d7ba3fc30878e01539deec0ef4a8f92d2 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Tue, 18 Aug 2026 14:20:30 -0400 Subject: [PATCH 12/14] fix(setup): stop padding pushing the injected file path off screen Wrapping pads every line to the full width, so the newline left inside the wrapped lead put a whole row of spaces in front of the file path and carried it past the edge of the terminal. The newline now sits outside the wrap, and the closing instruction wraps too. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/view.go | 11 +++++++---- cmd/setup/wizard_test.go | 41 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/cmd/setup/view.go b/cmd/setup/view.go index 8fe80690..8848c93c 100644 --- a/cmd/setup/view.go +++ b/cmd/setup/view.go @@ -82,14 +82,17 @@ func (m wizardModel) View() string { return m.spinner.View() + " Injecting initialization code..." case stepWaitForApp: - lead := "SDK initialization code has been injected into:\n" + // The newline stays outside the wrap: wrapping pads each line to the full + // width, so a trailing one inside would put a row of spaces in front of the + // path and push it past the edge of the terminal. + lead := "SDK initialization code has been injected into:" if m.initResult.AlreadyInitialized { - lead = "This file already initializes the LaunchDarkly SDK, so it was left as it is:\n" + lead = "This file already initializes the LaunchDarkly SDK, so it was left as it is:" } return titleStyle.Render("Start your application") + "\n\n" + - m.wrap(lead) + + m.wrap(lead) + "\n" + m.wrap(" "+m.initResult.FilePath) + "\n\n" + - "Please start your application now, then press Enter to verify the connection.\n" + m.wrap("Please start your application now, then press Enter to verify the connection.") + "\n" case stepVerify: return m.spinner.View() + " Waiting for SDK to connect..." diff --git a/cmd/setup/wizard_test.go b/cmd/setup/wizard_test.go index 0836bca2..3cdb75f4 100644 --- a/cmd/setup/wizard_test.go +++ b/cmd/setup/wizard_test.go @@ -1075,3 +1075,44 @@ func TestWizard_Plan_WrapsStepsToTerminalWidth(t *testing.T) { }) } } + +// Wrapping pads every line to the full width, so a newline left inside a wrapped +// string put a whole row of spaces in front of the injected file path and pushed it +// off the terminal. +func TestWizard_WaitForApp_WrapsWithoutLeadingPadding(t *testing.T) { + for _, width := range []int{80, 60, 40} { + for _, already := range []bool{false, true} { + t.Run(fmt.Sprintf("width%d_already%v", width, already), func(t *testing.T) { + path := "/Users/someone/code/launchdarkly/test-app/main.py" + m := wizardModel{ + step: stepWaitForApp, + width: width, + height: 24, + initResult: &setup.InitResult{ + FilePath: path, + AlreadyInitialized: already, + }, + } + + view := m.View() + + for _, line := range strings.Split(view, "\n") { + assert.LessOrEqual(t, len([]rune(line)), width, + "a line overflows a %d-column terminal", width) + } + // The path must start near the left edge, not after a row of padding. + for _, line := range strings.Split(view, "\n") { + if idx := strings.Index(line, "/Users/someone"); idx >= 0 { + assert.LessOrEqual(t, idx, 2, "the path is pushed right by padding") + } + } + // A path has no spaces to wrap on, so a narrow terminal hard-breaks it. + // Compare with whitespace removed to check nothing was lost. + assert.Contains(t, strings.Join(strings.Fields(view), ""), path) + if already { + assert.Contains(t, flat(view), "already initializes the LaunchDarkly SDK") + } + }) + } + } +} From 97f1b0da7947a222b2b378bc998e64f16a0f5b9d Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Wed, 19 Aug 2026 10:08:49 -0400 Subject: [PATCH 13/14] fix(setup): stop the package-manager list quitting on esc The list inherits the same quit binding as the others, so esc arriving on its own ended the session from the picker. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/update.go | 1 + cmd/setup/wizard_test.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/cmd/setup/update.go b/cmd/setup/update.go index 1b772bd4..a484a3c9 100644 --- a/cmd/setup/update.go +++ b/cmd/setup/update.go @@ -298,6 +298,7 @@ func (m *wizardModel) enterPackageManagerStep() { m.pmList = list.New(items, list.NewDefaultDelegate(), m.sdkBoxWidth(), m.pmListHeight()) m.pmList.Title = "Select a package manager:" m.pmList.SetShowStatusBar(false) + keepEscFromQuitting(&m.pmList) m.pmList.AdditionalShortHelpKeys = listHints(true) m.pmListBuilt = true m.step = stepSelectPackageManager diff --git a/cmd/setup/wizard_test.go b/cmd/setup/wizard_test.go index 3cdb75f4..9647c02e 100644 --- a/cmd/setup/wizard_test.go +++ b/cmd/setup/wizard_test.go @@ -963,6 +963,26 @@ func TestWizard_Picker_ListsInstalledFirstAndKeepsMissingSelectable(t *testing.T assert.Equal(t, "npm", chosen.detectResult.PackageManager) } +// The picker's list quits on esc for the same reason the others did. +func TestWizard_Picker_EscDoesNotQuit(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) + chdir(t, dir) + + m := wizardModel{step: stepDetect, width: 80, height: 24} + next, _ := m.Update(detectDoneMsg{result: &setup.DetectResult{ + SDKID: "node-server", Language: "JavaScript", + }}) + next2, _ := next.(wizardModel).Update(tea.KeyMsg{Type: tea.KeyEnter}) + picker := next2.(wizardModel) + require.Equal(t, stepSelectPackageManager, picker.step) + + after, cmd := picker.Update(tea.KeyMsg{Type: tea.KeyEsc}) + assert.False(t, after.(wizardModel).quitting) + assert.False(t, quitsOn(cmd), "the list quit the wizard on esc") + assert.Equal(t, stepSelectPackageManager, after.(wizardModel).step) +} + // Back must return to the picker, not skip over it to the SDK list. func TestWizard_Picker_BackReturnsToPickerFromPlan(t *testing.T) { dir := t.TempDir() From 79be31be3208e798132b71045c5259dfaf3a10db Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Wed, 19 Aug 2026 10:14:30 -0400 Subject: [PATCH 14/14] feat(setup): say the verify step waits for the app as well as the SDK Verification cannot succeed until the user's application is running, so naming only the SDK left it unclear whether anything was expected of them. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/setup/view.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/setup/view.go b/cmd/setup/view.go index 8848c93c..d47de6b0 100644 --- a/cmd/setup/view.go +++ b/cmd/setup/view.go @@ -95,7 +95,7 @@ func (m wizardModel) View() string { m.wrap("Please start your application now, then press Enter to verify the connection.") + "\n" case stepVerify: - return m.spinner.View() + " Waiting for SDK to connect..." + return m.spinner.View() + " Waiting for your app to start and its SDK to connect..." case stepDone: if m.installResult != nil && m.installResult.Failed {