From 0943fe31c8202dfff705c611f92e13d7c3fa3bc4 Mon Sep 17 00:00:00 2001 From: Francisco Fantl Date: Mon, 17 Aug 2026 14:42:50 -0400 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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") + }) + } +}