From 5bbf0abfe00c07c6e17e216621ac11430998de93 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Thu, 20 Aug 2026 23:37:52 -0700 Subject: [PATCH] feat(plugin): silently skip CLI-version-incompatible plugins during discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire the inert compatibleCLIVersion/cliVersionSkip predicate (added in a prior commit) into actual plugin discovery behavior: - internal/plugin/discover.go: call cliVersionSkip in loadManagedPlugin and in getManifestsParallel's merge/dedup loop, both strictly before the seen[...] name reservation, so an incompatible plugin never blocks a compatible, identically-named plugin and is reported as an incompatibility rather than a name conflict. - internal/plugin/discover.go: add ConflictsForReason and switch LogConflicts on PluginConflict.Reason — WARN for name conflicts (unchanged wording), INFO for version incompatibility. - cmd/plugin/discovery.go: extract reportDiscoveryConflicts so routine command registration only warns about name conflicts and stays silent about version-incompatibility skips; those surface only via `dr plugin list` / `dr plugin version `, which already pass the full conflict set through LogConflicts unfiltered. - docs: document maxCLIVersion/minCLIVersion semantics in docs/development/plugins.md and docs/development/remote-plugins.md. Verified via a real built binary that `dr plugin list`/`dr plugin version` surface the skip with upgrade guidance while ordinary command discovery (`dr --help`) stays silent. Refs: CFX-4730. Stacks on #814. --- cmd/plugin/discovery.go | 23 ++- cmd/plugin/discovery_test.go | 90 +++++++++ docs/development/plugins.md | 12 +- docs/development/remote-plugins.md | 3 + internal/plugin/discover.go | 55 +++++- internal/plugin/discover_test.go | 179 +++++++++++++++++- .../cli-plugin-version-constraints/tasks.md | 34 ++-- 7 files changed, 365 insertions(+), 31 deletions(-) diff --git a/cmd/plugin/discovery.go b/cmd/plugin/discovery.go index 157553ffd..b0a99bc14 100644 --- a/cmd/plugin/discovery.go +++ b/cmd/plugin/discovery.go @@ -51,9 +51,11 @@ func RegisterPluginCommands(rootCmd *cobra.Command) { plugins, conflicts := internalPlugin.DiscoverPluginsWithContext(ctx) - // Registering commands considers every discovered plugin, so every - // conflict is relevant here (it affects which binary wins a command name). - internalPlugin.LogConflicts(conflicts) + // Registering commands warns about name conflicts (they affect which + // binary wins a command name on every invocation) but stays silent about + // CLI version incompatibility skips here; those surface only through + // `dr plugin list` / `dr plugin version `. + reportDiscoveryConflicts(conflicts) // Seed the shared discovery cache so a later plugin.GetPlugins() call // (e.g. from `dr plugin list` or `dr plugin version`) reuses this result @@ -86,6 +88,21 @@ func RegisterPluginCommands(rootCmd *cobra.Command) { } } +// reportDiscoveryConflicts logs name conflicts at Warn (unchanged, pre-existing +// behavior) and stays silent about CLI version-incompatibility skips on this +// routine, per-invocation discovery path — Info-level output there would run +// on every `dr` command, which the spec forbids ("Silent on routine command +// discovery"). Version-incompatibility skips are only surfaced by `dr plugin +// list` and `dr plugin version `, which call internalPlugin.LogConflicts +// directly on the full (unfiltered) conflict set. +func reportDiscoveryConflicts(conflicts []internalPlugin.PluginConflict) { + internalPlugin.LogConflicts(internalPlugin.ConflictsForReason(conflicts, internalPlugin.SkipReasonNameConflict)) + + if versionSkips := internalPlugin.ConflictsForReason(conflicts, internalPlugin.SkipReasonVersionIncompatible); len(versionSkips) > 0 { + log.Debug("Plugin(s) skipped: CLI version incompatible", "count", len(versionSkips)) + } +} + func createPluginCommand(p internalPlugin.DiscoveredPlugin) *cobra.Command { executable := p.Executable // Capture for closure manifest := p.Manifest // Capture for closure diff --git a/cmd/plugin/discovery_test.go b/cmd/plugin/discovery_test.go index 1847b326d..e9d80aceb 100644 --- a/cmd/plugin/discovery_test.go +++ b/cmd/plugin/discovery_test.go @@ -15,13 +15,50 @@ package plugin import ( + "bytes" + "os" "path/filepath" "testing" + "github.com/datarobot/cli/internal/log" + internalPlugin "github.com/datarobot/cli/internal/plugin" "github.com/datarobot/cli/internal/testutil" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// captureLogOutput redirects os.Stderr to a pipe, reinitializes the stderr +// logger, runs fn, then returns everything written during fn's execution. +// Mirrors the equivalent helper in internal/plugin/discover_test.go. +func captureLogOutput(t *testing.T, fn func()) string { + t.Helper() + + r, w, err := os.Pipe() + require.NoError(t, err) + + origStderr := os.Stderr + os.Stderr = w + + log.StartStderr() + + fn() + + w.Close() + + os.Stderr = origStderr + + t.Cleanup(log.StopStderr) + + var buf bytes.Buffer + + _, err = buf.ReadFrom(r) + require.NoError(t, err) + + r.Close() + + return buf.String() +} + func TestIsManagedPlugin(t *testing.T) { t.Run("returns true for plugin in primary XDG dir", func(t *testing.T) { tmpXDG := t.TempDir() @@ -59,3 +96,56 @@ func TestIsManagedPlugin(t *testing.T) { assert.False(t, isManagedPlugin(pathPlugin)) }) } + +// TestReportDiscoveryConflicts verifies routine discovery reporting stays +// silent (Info-level) about CLI version incompatibility skips while still +// warning about name conflicts, per the spec's "Silent on routine command +// discovery" requirement. Version-incompatibility skips surface only through +// `dr plugin list` / `dr plugin version `, which call +// internalPlugin.LogConflicts directly (unfiltered) instead of this helper. +func TestReportDiscoveryConflicts(t *testing.T) { + t.Run("name conflicts are logged at Warn", func(t *testing.T) { + output := captureLogOutput(t, func() { + reportDiscoveryConflicts([]internalPlugin.PluginConflict{ + {Name: "widget", Path: "/usr/local/bin/dr-widget"}, + }) + }) + + assert.Contains(t, output, "widget") + assert.Contains(t, output, "WARN") + }) + + t.Run("version-incompatibility skips produce no Info-level output", func(t *testing.T) { + output := captureLogOutput(t, func() { + reportDiscoveryConflicts([]internalPlugin.PluginConflict{ + { + Name: "gadget", + Path: "/usr/local/bin/dr-gadget", + Reason: internalPlugin.SkipReasonVersionIncompatible, + Detail: "requires dr >= 2.0.0 (running 1.9.0); run 'dr self update'", + }, + }) + }) + + assert.NotContains(t, output, "gadget", + "a version-incompatibility skip must stay silent on routine command discovery") + assert.Empty(t, output) + }) + + t.Run("mixed conflicts only surface the name conflict", func(t *testing.T) { + output := captureLogOutput(t, func() { + reportDiscoveryConflicts([]internalPlugin.PluginConflict{ + {Name: "widget", Path: "/usr/local/bin/dr-widget"}, + { + Name: "gadget", + Path: "/usr/local/bin/dr-gadget", + Reason: internalPlugin.SkipReasonVersionIncompatible, + Detail: "requires dr >= 2.0.0 (running 1.9.0); run 'dr self update'", + }, + }) + }) + + assert.Contains(t, output, "widget") + assert.NotContains(t, output, "gadget") + }) +} diff --git a/docs/development/plugins.md b/docs/development/plugins.md index 2018fbdb7..b43eea28e 100644 --- a/docs/development/plugins.md +++ b/docs/development/plugins.md @@ -69,7 +69,9 @@ The CLI currently understands the following fields: "name": "my-plugin", "version": "1.2.3", "description": "Adds extra commands to dr", - "authentication": true + "authentication": true, + "minCLIVersion": "1.0.0", + "maxCLIVersion": "2.0.0" } ``` @@ -87,6 +89,14 @@ The CLI currently understands the following fields: - If no valid credentials exist, the user will be prompted to log in. - Respects the global `--skip-auth` flag. - Defaults to `false` if omitted. +- `minCLIVersion` / `maxCLIVersion` (string): Plain semver strings (no range syntax) declaring the inclusive `[minCLIVersion, maxCLIVersion]` window of `dr` versions the plugin supports. Either, both, or neither may be set. + - Checked at discovery time, before the plugin is loaded — an incompatible plugin never runs. + - Both bounds are inclusive: a running CLI version exactly equal to `minCLIVersion` or `maxCLIVersion` still loads the plugin. + - Comparison uses only the CLI's core version (`major.minor.patch`); any CLI prerelease/build metadata is ignored. + - A malformed `minCLIVersion`/`maxCLIVersion` value always skips the plugin, even on a `dev` CLI build. + - When the running CLI version itself is unparseable (including the `dev` build used by local/source builds), the check is bypassed and the plugin loads unconditionally — there is no reliable CLI version to compare against. + - A plugin skipped for version incompatibility is never loaded and never counted as a name conflict; it is reported at Info level and surfaced in `dr plugin list` / `dr plugin version `, but stays silent on ordinary command discovery. + - If the CLI reports the plugin requires a newer version, run `dr self update`; if it requires an older version, update the plugin instead. ### Notes / recommendations diff --git a/docs/development/remote-plugins.md b/docs/development/remote-plugins.md index 310bd7597..cebd8577b 100644 --- a/docs/development/remote-plugins.md +++ b/docs/development/remote-plugins.md @@ -59,6 +59,7 @@ The `manifest.json` inside each package defines platform-specific executables: "version": "0.1.6", "description": "AI agent design, coding, and deployment assistant", "minCLIVersion": "0.2.0", + "maxCLIVersion": "1.0.0", "scripts": { "posix": "scripts/dr-assist.sh", "windows": "scripts/dr-assist.ps1" @@ -66,6 +67,8 @@ The `manifest.json` inside each package defines platform-specific executables: } ``` +`minCLIVersion`/`maxCLIVersion` are optional, inclusive semver bounds on the running `dr` version — see [Manifest JSON schema](./plugins.md#manifest-json-schema) for the full compatibility semantics (comparison basis, malformed-bound handling, and the `dev`-build bypass). + ## Implementation steps ### 1. Create plugin registry schema diff --git a/internal/plugin/discover.go b/internal/plugin/discover.go index 937aee8c8..bf7245d12 100644 --- a/internal/plugin/discover.go +++ b/internal/plugin/discover.go @@ -67,19 +67,28 @@ func PrimeCache(plugins []DiscoveredPlugin, conflicts []PluginConflict) { }) } -// LogConflicts logs a WARN for each conflict, in the same format previously -// emitted directly by discovery internals. Callers choose which conflicts to -// pass in — e.g. all of them for a full listing, or only those returned by -// ConflictsForName when only one specific plugin was requested. +// LogConflicts reports each conflict at a level chosen by its Reason: WARN +// for a name conflict (the pre-existing, unchanged behavior), or INFO for a +// CLI version incompatibility, which is a routine/expected skip rather than +// an operational warning. Callers choose which conflicts to pass in — e.g. +// all of them for a full listing, or only those returned by ConflictsForName +// when only one specific plugin was requested. func LogConflicts(conflicts []PluginConflict) { for _, c := range conflicts { - log.Warn("Plugin name already registered, skipping", "name", c.Name, "path", c.Path) + switch c.Reason { + case SkipReasonVersionIncompatible: + log.Info("Plugin skipped: CLI version incompatible", "name", c.Name, "path", c.Path, "detail", c.Detail) + case SkipReasonNameConflict: + log.Warn("Plugin name already registered, skipping", "name", c.Name, "path", c.Path) + } } } // ConflictsForName filters conflicts down to those matching a single plugin // name, so callers that only care about one plugin (e.g. `dr plugin version -// `) don't surface warnings about unrelated plugins. +// `) don't surface warnings about unrelated plugins. The filter is +// reason-agnostic: it returns matches regardless of whether they are name +// conflicts or version-incompatibility skips. func ConflictsForName(conflicts []PluginConflict, name string) []PluginConflict { var matched []PluginConflict @@ -92,6 +101,22 @@ func ConflictsForName(conflicts []PluginConflict, name string) []PluginConflict return matched } +// ConflictsForReason filters conflicts down to those matching a single skip +// reason, so callers can separate name conflicts from version-incompatibility +// skips (e.g. routine command registration warns only about name conflicts +// and stays silent about version skips). +func ConflictsForReason(conflicts []PluginConflict, reason PluginSkipReason) []PluginConflict { + var matched []PluginConflict + + for _, c := range conflicts { + if c.Reason == reason { + matched = append(matched, c) + } + } + + return matched +} + // DiscoverPluginsWithContext discovers all plugins under the given context deadline, // along with any name conflicts encountered (a plugin skipped because another // plugin already claimed its manifest name from a higher-priority location). @@ -305,6 +330,15 @@ func loadManagedPlugin(dir, name string, seen map[string]bool) (*DiscoveredPlugi return nil, &PluginConflict{Name: manifest.Name, Path: pluginDir}, nil } + // Evaluate the CLI version compatibility check before doing any more work + // for this manifest, and — critically — before the seen[...] reservation + // below: an incompatible plugin must never claim the name slot a + // compatible, identically-named plugin from a lower-priority location + // would otherwise take. + if conflict := cliVersionSkip(&manifest, pluginDir); conflict != nil { + return nil, conflict, nil + } + executable, err := resolvePlatformExecutable(pluginDir, &manifest) if err != nil { return nil, nil, err @@ -444,6 +478,15 @@ func getManifestsParallel(ctx context.Context, executables []string, seen map[st continue } + // As in loadManagedPlugin, this check must run before the seen[...] + // reservation below, so an incompatible plugin never blocks a + // compatible, identically-named plugin from claiming the name. + if conflict := cliVersionSkip(r.manifest, r.path); conflict != nil { + conflicts = append(conflicts, *conflict) + + continue + } + seen[r.manifest.Name] = true plugins = append(plugins, DiscoveredPlugin{ diff --git a/internal/plugin/discover_test.go b/internal/plugin/discover_test.go index ca8ca7a01..da771b21a 100644 --- a/internal/plugin/discover_test.go +++ b/internal/plugin/discover_test.go @@ -17,6 +17,7 @@ package plugin import ( "bytes" "context" + "encoding/json" "fmt" "os" "path/filepath" @@ -594,6 +595,12 @@ func (s *DiscoverWithContextSuite) TestCancelledContextSkipsPATHPlugins() { } func (s *DiscoverWithContextSuite) TestDuplicatePATHEntryDoesNotWarn() { + // No manifest here declares a CLI version bound, so this override has no + // effect on the outcome — it just proves the assertions below are not + // silently passing only because of the "dev" bypass in `go test`. + restore := setCurrentCLIVersionForTest(s.T(), "1.0.0") + defer restore() + createMockPlugin(s.T(), s.pluginDir, "dr-ctx-dup", `{"name":"ctx-dup","version":"1.0.0","description":"Dup"}`) // The same directory listed twice in PATH used to make discovery scan it @@ -615,29 +622,128 @@ func (s *DiscoverWithContextSuite) TestDuplicatePATHEntryDoesNotWarn() { s.Equal(1, matches, "plugin must only appear once even though its dir is listed twice in PATH") } +// TestVersionIncompatiblePluginDoesNotBlockCompatibleSameNamePlugin covers the +// spec's "Skip Ordering Relative to Name Deduplication" requirement end-to-end: +// two PATH directories export the same manifest name, the lexicographically-first +// violates maxCLIVersion. The version-incompatible manifest must not reserve the +// name, so the second, compatible manifest still registers under it — and the +// skip must be reported as a version incompatibility, never as a name conflict. +func (s *DiscoverWithContextSuite) TestVersionIncompatiblePluginDoesNotBlockCompatibleSameNamePlugin() { + restore := setCurrentCLIVersionForTest(s.T(), "2.0.0") + defer restore() + + dir2, err := os.MkdirTemp("", "plugin-discoverctx-dir2") + s.Require().NoError(err) + + defer os.RemoveAll(dir2) + + // s.pluginDir sorts before dir2's basename in setDiscoveryPath's PATH + // ordering below, so it is scanned first; its manifest violates + // maxCLIVersion against the running (overridden) CLI version. + incompatibleManifest := `{"name":"shared-ctx-plugin","version":"1.0.0","description":"Old","maxCLIVersion":"1.5.0"}` + compatibleManifest := `{"name":"shared-ctx-plugin","version":"2.0.0","description":"New"}` + + exe1 := createMockPlugin(s.T(), s.pluginDir, "dr-shared-ctx", incompatibleManifest) + exe2 := createMockPlugin(s.T(), dir2, "dr-shared-ctx", compatibleManifest) + + setDiscoveryPath(s.T(), s.pluginDir+string(os.PathListSeparator)+dir2) + + plugins, conflicts := DiscoverPluginsWithContext(context.Background()) + + registered := pluginByName(plugins, "shared-ctx-plugin") + s.Require().NotNil(registered, "the compatible plugin must still register under the shared name") + s.Equal(exe2, registered.Executable) + + versionConflicts := ConflictsForReason(conflicts, SkipReasonVersionIncompatible) + s.Require().Len(versionConflicts, 1) + s.Equal(exe1, versionConflicts[0].Path) + + s.Empty(ConflictsForReason(conflicts, SkipReasonNameConflict), + "the incompatible plugin must be reported as a version incompatibility, not a name conflict") +} + // createManagedTestPlugin creates a minimal managed plugin directory structure under pluginsDir. func createManagedTestPlugin(t *testing.T, pluginsDir, dirName, pluginName string) { t.Helper() + createManagedTestPluginWithBounds(t, pluginsDir, dirName, pluginName, "", "") +} + +// createManagedTestPluginWithBounds is createManagedTestPlugin extended with +// optional minCLIVersion/maxCLIVersion manifest fields, for exercising the CLI +// version compatibility check against managed plugin discovery. Empty bounds +// behave identically to createManagedTestPlugin (no constraint declared). +func createManagedTestPluginWithBounds(t *testing.T, pluginsDir, dirName, pluginName, minCLIVersion, maxCLIVersion string) { + t.Helper() + pluginDir := filepath.Join(pluginsDir, dirName) require.NoError(t, os.MkdirAll(filepath.Join(pluginDir, "scripts"), 0o755)) - manifestJSON := fmt.Sprintf( - `{"name":%q,"version":"1.0.0","scripts":{"posix":"scripts/run.sh","windows":"scripts/run.ps1"}}`, - pluginName, - ) + manifest := PluginManifest{ + BasicPluginManifest: BasicPluginManifest{Name: pluginName, Version: "1.0.0"}, + Scripts: &PluginScripts{Posix: "scripts/run.sh", Windows: "scripts/run.ps1"}, + MinCLIVersion: minCLIVersion, + MaxCLIVersion: maxCLIVersion, + } + + manifestJSON, err := json.Marshal(manifest) + require.NoError(t, err) - require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "manifest.json"), []byte(manifestJSON), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "manifest.json"), manifestJSON, 0o644)) createScript(t, filepath.Join(pluginDir, "scripts", "run.sh"), "#!/bin/sh\nexit 0\n") require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "scripts", "run.ps1"), []byte("exit 0"), 0o644)) } +// TestDiscoverPlugins_ManagedPluginBelowMinimumIsSkipped verifies loadManagedPlugin +// evaluates cliVersionSkip before reserving the manifest name: a managed plugin +// declaring a minCLIVersion above the running CLI version must not load and must +// be reported as a version incompatibility, not loaded and not a name conflict. +func TestDiscoverPlugins_ManagedPluginBelowMinimumIsSkipped(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("HOME env override is Unix-specific") + } + + restore := setCurrentCLIVersionForTest(t, "1.0.0") + defer restore() + + tmpHome := t.TempDir() + tmpXDG := t.TempDir() + + t.Setenv("HOME", tmpHome) + testutil.SetXDGEnv(t, "XDG_CONFIG_HOME", tmpXDG) + + viperx.Reset() + viperx.Set("plugin.manifest_timeout_ms", 5000) + + pluginsDir := filepath.Join(tmpXDG, "datarobot", "plugins") + createManagedTestPluginWithBounds(t, pluginsDir, "future-plugin", "future-plugin", "2.0.0", "") + + plugins, conflicts := DiscoverPluginsWithContext(context.Background()) + + assert.Nil(t, pluginByName(plugins, "future-plugin"), + "a managed plugin declaring a minCLIVersion above the running CLI version must not load") + + versionConflicts := ConflictsForReason(conflicts, SkipReasonVersionIncompatible) + require.Len(t, versionConflicts, 1) + assert.Equal(t, "future-plugin", versionConflicts[0].Name) + assert.Contains(t, versionConflicts[0].Detail, "2.0.0") + + assert.Empty(t, ConflictsForReason(conflicts, SkipReasonNameConflict), + "a version-incompatibility skip must never be reported as a name conflict") +} + func TestDiscoverPlugins_FindsPluginsInXDGConfigDirs(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("HOME env override is Unix-specific") } + // Neither managed plugin below declares a CLI version bound, so this + // override has no effect on the outcome — it just proves the assertions + // below are not silently passing only because of the "dev" bypass. + restore := setCurrentCLIVersionForTest(t, "1.0.0") + defer restore() + tmpHome := t.TempDir() tmpXDG := t.TempDir() tmpConfigDir := t.TempDir() @@ -673,6 +779,9 @@ func TestUniqueDirs(t *testing.T) { } func TestLogConflicts(t *testing.T) { + // Byte-identical to the pre-existing wording: these are all name + // conflicts (the zero-value Reason), so the level/text split introduced + // for version incompatibility must not change this output at all. output := captureLogOutput(t, func() { LogConflicts([]PluginConflict{ {Name: "potato", Path: "/usr/local/bin/dr-potato"}, @@ -684,6 +793,27 @@ func TestLogConflicts(t *testing.T) { assert.Contains(t, output, "/usr/local/bin/dr-potato") assert.Contains(t, output, "carrot") assert.Contains(t, output, "/opt/bin/dr-carrot") + assert.Contains(t, output, "WARN") + assert.NotContains(t, output, "INFO") +} + +func TestLogConflictsVersionIncompatible(t *testing.T) { + output := captureLogOutput(t, func() { + LogConflicts([]PluginConflict{ + { + Name: "widget", + Path: "/usr/local/bin/dr-widget", + Reason: SkipReasonVersionIncompatible, + Detail: "requires dr >= 2.0.0 (running 1.9.0); run 'dr self update'", + }, + }) + }) + + assert.Contains(t, output, "widget") + assert.Contains(t, output, "/usr/local/bin/dr-widget") + assert.Contains(t, output, "requires dr >= 2.0.0") + assert.Contains(t, output, "INFO", "version-incompatibility skips must log at Info level, not Warn") + assert.NotContains(t, output, "WARN") } func TestLogConflictsEmpty(t *testing.T) { @@ -698,14 +828,49 @@ func TestConflictsForName(t *testing.T) { conflicts := []PluginConflict{ {Name: "potato", Path: "/usr/local/bin/dr-potato"}, {Name: "carrot", Path: "/opt/bin/dr-carrot"}, - {Name: "potato", Path: "/opt/bin/dr-potato"}, + { + Name: "potato", + Path: "/opt/bin/dr-potato", + Reason: SkipReasonVersionIncompatible, + Detail: "requires dr >= 2.0.0 (running 1.9.0); run 'dr self update'", + }, } + // The filter must stay reason-agnostic: a name match returns regardless + // of whether the conflict is a name collision or a version incompatibility. assert.Equal(t, []PluginConflict{ {Name: "potato", Path: "/usr/local/bin/dr-potato"}, - {Name: "potato", Path: "/opt/bin/dr-potato"}, + { + Name: "potato", + Path: "/opt/bin/dr-potato", + Reason: SkipReasonVersionIncompatible, + Detail: "requires dr >= 2.0.0 (running 1.9.0); run 'dr self update'", + }, }, ConflictsForName(conflicts, "potato")) assert.Empty(t, ConflictsForName(conflicts, "turnip")) assert.Empty(t, ConflictsForName(nil, "potato")) } + +func TestConflictsForReason(t *testing.T) { + versionConflict := PluginConflict{ + Name: "widget", + Path: "/usr/local/bin/dr-widget", + Reason: SkipReasonVersionIncompatible, + Detail: "requires dr >= 2.0.0 (running 1.9.0); run 'dr self update'", + } + conflicts := []PluginConflict{ + {Name: "potato", Path: "/usr/local/bin/dr-potato"}, + versionConflict, + {Name: "carrot", Path: "/opt/bin/dr-carrot"}, + } + + assert.Equal(t, []PluginConflict{ + {Name: "potato", Path: "/usr/local/bin/dr-potato"}, + {Name: "carrot", Path: "/opt/bin/dr-carrot"}, + }, ConflictsForReason(conflicts, SkipReasonNameConflict)) + + assert.Equal(t, []PluginConflict{versionConflict}, ConflictsForReason(conflicts, SkipReasonVersionIncompatible)) + + assert.Empty(t, ConflictsForReason(nil, SkipReasonNameConflict)) +} diff --git a/openspec/changes/cli-plugin-version-constraints/tasks.md b/openspec/changes/cli-plugin-version-constraints/tasks.md index 084adc660..15d70078d 100644 --- a/openspec/changes/cli-plugin-version-constraints/tasks.md +++ b/openspec/changes/cli-plugin-version-constraints/tasks.md @@ -41,26 +41,32 @@ Chain strategy: stacked-to-main ## Phase 4: Discovery Wiring (PR 2, TDD) -- [ ] 4.1 RED: in `internal/plugin/discover_test.go`, extend `createManagedTestPlugin` to accept min/max bounds; add case asserting an out-of-range managed plugin is absent from results and present in conflicts with `Reason: SkipReasonVersionIncompatible`. Spec: Managed plugin below minimum. -- [ ] 4.2 RED: add `DiscoverWithContextSuite` case — two PATH dirs export the same manifest name, lexicographically-first violates `maxCLIVersion`; assert second registers under that name, conflicts hold exactly one `SkipReasonVersionIncompatible` record and zero `SkipReasonNameConflict` records. Spec: Skip Ordering Relative to Name Deduplication. -- [ ] 4.3 RED: override `currentCLIVersion` (with `t.Cleanup` restore) in `TestDuplicatePATHEntryDoesNotWarn` and the XDG-config-dirs discovery test to a real semver so the `dev` bypass does not mask assertions. -- [ ] 4.4 Confirm 4.1-4.3 fail against current code (`go test ./internal/plugin/... -race`). -- [ ] 4.5 GREEN: call `cliVersionSkip` in `loadManagedPlugin` (~L304) and `getManifestsParallel` (~L441 loop), both immediately before the `seen[...]` reservation; append its non-nil result to conflicts and `continue` without setting `seen[name]`. -- [ ] 4.6 Run `go test ./internal/plugin/... -race -v` until 4.1-4.3 are GREEN. +- [x] 4.1 RED: in `internal/plugin/discover_test.go`, extend `createManagedTestPlugin` to accept min/max bounds; add case asserting an out-of-range managed plugin is absent from results and present in conflicts with `Reason: SkipReasonVersionIncompatible`. Spec: Managed plugin below minimum. +- [x] 4.2 RED: add `DiscoverWithContextSuite` case — two PATH dirs export the same manifest name, lexicographically-first violates `maxCLIVersion`; assert second registers under that name, conflicts hold exactly one `SkipReasonVersionIncompatible` record and zero `SkipReasonNameConflict` records. Spec: Skip Ordering Relative to Name Deduplication. +- [x] 4.3 RED: override `currentCLIVersion` (with `t.Cleanup`/`defer` restore) in `TestDuplicatePATHEntryDoesNotWarn` and the XDG-config-dirs discovery test to a real semver so the `dev` bypass does not mask assertions. +- [x] 4.4 Confirmed 4.1-4.3 fail against current code (`go vet ./internal/plugin/...` → `undefined: ConflictsForReason` compile failure). +- [x] 4.5 GREEN: call `cliVersionSkip` in `loadManagedPlugin` (before the executable resolution and the `seen[...]` reservation) and `getManifestsParallel`'s merge loop (before its `seen[...]` reservation); append its non-nil result to conflicts and `continue` without setting `seen[name]`. +- [x] 4.6 `go test ./internal/plugin/... -race -v` — all new and existing cases GREEN. ## Phase 5: Reporting & CLI Wiring (PR 2, TDD) -- [ ] 5.1 RED: extend `TestLogConflicts` asserting name-conflict wording is byte-identical to today, and a `SkipReasonVersionIncompatible` record emits Info-level output including `Detail`. Add `TestConflictsForReason`; extend `TestConflictsForName` fixtures with mixed reasons proving the filter stays reason-agnostic. Spec: Structured Single-Channel Skip Reporting. -- [ ] 5.2 GREEN: add `ConflictsForReason(conflicts []PluginConflict, reason PluginSkipReason) []PluginConflict` in `internal/plugin/discover.go`; change `LogConflicts` to switch on `Reason` — Warn for name conflicts (unchanged wording), Info for version incompatibility. -- [ ] 5.3 GREEN: in `cmd/plugin/discovery.go` (L56), change `RegisterPluginCommands` to call `LogConflicts(ConflictsForReason(conflicts, SkipReasonNameConflict))` and add a Debug line for the remainder, so routine discovery stays silent for version skips. Spec: Silent on routine command discovery. -- [ ] 5.4 Verify `dr plugin list` / `dr plugin version ` surface skipped/incompatible plugins with required-version detail (manual harness run, or existing list/version command tests if present). +- [x] 5.1 RED: extended `TestLogConflicts` asserting name-conflict wording/level is byte-identical to today (WARN, no INFO), added `TestLogConflictsVersionIncompatible` (INFO, includes `Detail`, no WARN), added `TestConflictsForReason`, and extended `TestConflictsForName` fixtures with a mixed-reason record proving the filter stays reason-agnostic. Spec: Structured Single-Channel Skip Reporting. +- [x] 5.2 GREEN: added `ConflictsForReason(conflicts []PluginConflict, reason PluginSkipReason) []PluginConflict` in `internal/plugin/discover.go`; `LogConflicts` now switches on `Reason` — Warn for `SkipReasonNameConflict`, Info for `SkipReasonVersionIncompatible` (exhaustive switch, no `default`, to satisfy the `exhaustive` linter). +- [x] 5.3 GREEN: extracted `reportDiscoveryConflicts` in `cmd/plugin/discovery.go`, called from `RegisterPluginCommands` in place of the direct `LogConflicts(conflicts)` call; it calls `LogConflicts(ConflictsForReason(conflicts, SkipReasonNameConflict))` and logs a Debug line for the remainder, so routine discovery stays silent for version skips. Added `TestReportDiscoveryConflicts` (RED confirmed via `undefined: reportDiscoveryConflicts` before implementation) covering name-conflict-only, version-only, and mixed cases. Spec: Silent on routine command discovery. +- [x] 5.4 Verified via a real built binary (`go build -ldflags "-X .../version.Version=1.0.0"`) with a fake `dr-oldwidget` PATH plugin declaring `maxCLIVersion: 0.9.0`: `dr plugin list` excludes it from the table and prints `INFO Plugin skipped: CLI version incompatible ... detail="supports dr <= 0.9.0 (running 1.0.0); update the plugin"`; `dr plugin version oldwidget` prints the same Info line then `Error: plugin "oldwidget" not found`; plain `dr --help` prints neither the plugin name nor "version incompatible" (confirms silence on ordinary discovery). No code changes were needed in `cmd/plugin/list/cmd.go` or `cmd/plugin/version/cmd.go` — confirmed empirically, not just assumed from design. ## Phase 6: Documentation (PR 2) -- [ ] 6.1 `docs/development/plugins.md` (~L82-89): add `maxCLIVersion` and `minCLIVersion` to the Optional fields list with inclusive-bound and dev-bypass semantics. -- [ ] 6.2 `docs/development/remote-plugins.md` (~L61): add `maxCLIVersion` next to the existing `minCLIVersion` example. +- [x] 6.1 `docs/development/plugins.md` (~L82-89): added `minCLIVersion`/`maxCLIVersion` to the manifest JSON example and a new Optional-fields bullet documenting inclusive bounds, core-version-only comparison, malformed-bound-always-skips, the `dev`/unparseable-CLI bypass, and the reporting channel. +- [x] 6.2 `docs/development/remote-plugins.md` (~L61): added `maxCLIVersion` next to the existing `minCLIVersion` example, with a cross-reference to the full semantics in `plugins.md`. ## Phase 7: Final Gates (PR 1 and PR 2, each before opening) -- [x] 7.1 `task lint` clean on each slice. (PR 1 done; PR 2 pending) -- [x] 7.2 `task test` (race + coverage) green on each slice. (PR 1 done; PR 2 pending) +- [x] 7.1 `task lint` clean on each slice. (PR 1 done; PR 2 done — 0 issues on all 3 GOOS targets) +- [x] 7.2 `task test` (race + coverage) green on each slice. (PR 1 done; PR 2 done — full repo suite green, `internal/plugin` 80.9% coverage) + +## Delivery Status + +PR 1: https://github.com/datarobot-oss/cli/pull/814 (draft, branch `aj/cli-version-constraints` off `main`). All PR-1-scoped tasks (Phase 1-3, and the PR-1 portion of Phase 7) complete. + +PR 2: branch `aj/cli-version-constraints-discovery` off `aj/cli-version-constraints` (stacked). All PR-2-scoped tasks (Phase 4-6, and the PR-2 portion of Phase 7) complete. See apply-progress for the PR URL once opened.