Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions cmd/plugin/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name>`.
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
Expand Down Expand Up @@ -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 <name>`, 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
Expand Down
90 changes: 90 additions & 0 deletions cmd/plugin/discovery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 <name>`, 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")
})
}
12 changes: 11 additions & 1 deletion docs/development/plugins.md
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
```

Expand All @@ -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 <name>`, 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

Expand Down
3 changes: 3 additions & 0 deletions docs/development/remote-plugins.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,16 @@ 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"
}
}
```

`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
Expand Down
55 changes: 49 additions & 6 deletions internal/plugin/discover.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
// <name>`) don't surface warnings about unrelated plugins.
// <name>`) 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

Expand All @@ -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).
Expand Down Expand Up @@ -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
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version skip misclassified as conflict

Medium Severity

cliVersionSkip runs only after the seen name check, so when a higher-priority compatible plugin already claimed the name, an incompatible peer is recorded as SkipReasonNameConflict instead of SkipReasonVersionIncompatible. That turns a routine version skip into a WARN on every dr invocation (via reportDiscoveryConflicts), violating the silence rule for version-incompatible plugins. A common case is a managed compatible install plus an older same-named PATH binary.

Suggested change
}
if conflict := cliVersionSkip(&manifest, pluginDir); conflict != nil {
return nil, conflict, nil
}
if seen[manifest.Name] {
return nil, &PluginConflict{Name: manifest.Name, Path: pluginDir}, nil
}
Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bbf0ab. Configure here.


executable, err := resolvePlatformExecutable(pluginDir, &manifest)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -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{
Expand Down
Loading
Loading