diff --git a/docs/plans/2026-02-06-external-plugin-discovery-design.md b/docs/plans/2026-02-06-external-plugin-discovery-design.md new file mode 100644 index 00000000..52bebbb9 --- /dev/null +++ b/docs/plans/2026-02-06-external-plugin-discovery-design.md @@ -0,0 +1,131 @@ +# External Plugin Discovery - Phase 1 MVP Design + +Issue: https://github.com/steipete/gogcli/issues/188 + +## Overview + +Implement cargo/git-style external command discovery allowing `gog foo bar` to execute `gog-foo-bar` binary from PATH. + +## Design Decisions + +### Decision 1: Post-parse fallback (Option B) vs Pre-parse interception (Option A) + +**Chosen: Post-parse fallback (Option B)** + +| Aspect | Option A (Pre-parse) | Option B (Post-parse) | +|--------|---------------------|----------------------| +| Performance | Slower on normal commands (PATH check first) | Faster for built-in commands | +| Simplicity | Cleaner - no error parsing needed | Need to detect Kong error types | +| Plugin priority | Plugins can shadow built-ins | Built-ins always take precedence | +| Safety | Less safe - accidental shadowing | Safer | + +**Why Option B:** + +1. **Safety**: Built-in commands always take precedence - an external binary cannot accidentally or maliciously shadow core functionality +2. **Convention**: Follows git/cargo pattern where built-ins win +3. **Performance**: No PATH scanning for normal command usage (majority of invocations) + +### Decision 2: Longest-first (greedy) matching + +**Chosen: Longest-first** + +When user types `gog docs headings list`, search order: +1. `gog-docs-headings-list` (most specific) +2. `gog-docs-headings` +3. `gog-docs` (least specific) + +**Why longest-first:** + +1. **Specificity wins**: More specific plugin takes precedence over generic one +2. **Convention**: Matches cargo/git behavior +3. **Composability**: `gog-docs-headings` handles headings, while `gog-docs` could handle generic docs operations - no conflict + +### Decision 3: Binary prefix `gog-*` + +**Chosen: `gog-` prefix** + +Example: `gog docs headings` → `gog-docs-headings` + +**Why `gog-` not `gogcli-`:** + +1. **Consistency**: Matches the CLI binary name users invoke +2. **Brevity**: Shorter for plugin developers +3. **Convention**: git uses `git-*`, cargo uses `cargo-*` (matches binary name) + +## Phase 1 MVP Scope + +**Included:** + +* PATH discovery + exec +* Longest-first matching +* Pass remaining args to plugin +* Unit tests documenting behavior + +**Excluded (Phase 2+):** + +* `--help-oneliner` protocol +* Help integration (plugins in `gog --help`) +* Environment variable passing (GOG_AUTH_TOKEN_PATH, etc.) +* Discovery caching + +## Implementation + +### Files + +| File | Purpose | +|------|---------| +| `internal/cmd/external.go` | Discovery + exec logic | +| `internal/cmd/external_test.go` | Unit tests | +| `internal/cmd/root.go` | Integrate into Execute() | + +### Core Algorithm + +```go +func tryExternalCommand(args []string) error { + // Longest-first: try most specific binary first + // Why: More specific plugins should take precedence (cargo/git pattern) + for i := len(args); i > 0; i-- { + binaryName := "gog-" + strings.Join(args[:i], "-") + if path, err := exec.LookPath(binaryName); err == nil { + return execExternal(path, args[i:]) + } + } + return nil // not found +} +``` + +### Integration Point + +In `root.go` `Execute()`, after `parser.Parse(args)` fails: + +```go +kctx, err := parser.Parse(args) +if err != nil { + // Try external command before returning parse error + // Why post-parse: Built-in commands always take precedence (safer) + if extErr := tryExternalCommand(args); extErr != nil { + return extErr + } + // Fall through to original error if no external command found + ... +} +``` + +## Commits + +1. `feat(plugin): add external command discovery and execution` +2. `test(plugin): add unit tests for external command discovery` +3. `feat(plugin): integrate external commands into Execute()` + +## Future Phases + +**Phase 2 (separate PRs):** + +* `--help-oneliner` protocol for help integration +* Environment variable passing to plugins +* Plugin listing in `gog --help` + +**Phase 3:** + +* Discovery caching for performance +* Version compatibility checks diff --git a/internal/cmd/external.go b/internal/cmd/external.go new file mode 100644 index 00000000..70d1b19c --- /dev/null +++ b/internal/cmd/external.go @@ -0,0 +1,93 @@ +package cmd + +import ( + "errors" + "os" + "os/exec" + "strings" + "syscall" +) + +// externalCommandPrefix is the prefix for external plugin binaries. +// Uses "gog-" to match the CLI binary name (following git/cargo convention). +const externalCommandPrefix = "gog-" + +// ErrExternalNotFound indicates no external command was found for the given args. +var ErrExternalNotFound = errors.New("external command not found") + +// tryExternalCommand attempts to find and execute an external plugin binary. +// +// Design: Post-parse fallback (Option B) +// This function is called AFTER Kong parsing fails with "unknown command". +// Why: Built-in commands always take precedence over external plugins. +// This prevents accidental or malicious shadowing of core functionality +// and matches the git/cargo convention. +// +// Algorithm: Longest-first (greedy) matching +// For args ["docs", "headings", "list"], tries in order: +// 1. gog-docs-headings-list (most specific) +// 2. gog-docs-headings +// 3. gog-docs (least specific) +// +// Why longest-first: More specific plugins should win over generic ones. +// Example: gog-docs-headings handles "headings" specifically, while gog-docs +// might handle generic docs operations. The specific plugin takes precedence. +// +// Returns: +// - nil if no external command found (caller should return original error) +// - error from exec if external command found but execution failed +// - does not return if exec succeeds (replaces current process) +func tryExternalCommand(args []string) error { + if len(args) == 0 { + return ErrExternalNotFound + } + + path, remainingArgs := findExternalCommand(args) + if path == "" { + return ErrExternalNotFound + } + + return execExternal(path, remainingArgs) +} + +// findExternalCommand searches PATH for a matching external plugin binary. +// Uses longest-first matching: tries most specific binary name first. +// Returns the path to the binary and remaining arguments to pass to it. +func findExternalCommand(args []string) (binaryPath string, remainingArgs []string) { + // Longest-first: start with all args, progressively try fewer + // Why: More specific plugins take precedence (e.g., gog-docs-headings over gog-docs) + for i := len(args); i > 0; i-- { + binaryName := externalCommandPrefix + strings.Join(args[:i], "-") + if path, err := exec.LookPath(binaryName); err == nil { + return path, args[i:] + } + } + return "", nil +} + +// execExternal replaces the current process with the external command. +// Uses syscall.Exec for true process replacement (no child process). +func execExternal(binaryPath string, args []string) error { + // Build argv: binary name followed by remaining arguments + argv := append([]string{binaryPath}, args...) + + // Use syscall.Exec to replace current process + // This is the standard pattern for CLI plugin dispatch (git, cargo) + return syscall.Exec(binaryPath, argv, os.Environ()) +} + +// LookPath is a variable to allow mocking in tests. +// In production, this is exec.LookPath. +var lookPath = exec.LookPath + +// findExternalCommandWithLookPath is like findExternalCommand but uses +// the package-level lookPath variable for testability. +func findExternalCommandWithLookPath(args []string) (binaryPath string, remainingArgs []string) { + for i := len(args); i > 0; i-- { + binaryName := externalCommandPrefix + strings.Join(args[:i], "-") + if path, err := lookPath(binaryName); err == nil { + return path, args[i:] + } + } + return "", nil +} diff --git a/internal/cmd/external_test.go b/internal/cmd/external_test.go new file mode 100644 index 00000000..c38e6ec3 --- /dev/null +++ b/internal/cmd/external_test.go @@ -0,0 +1,161 @@ +package cmd + +import ( + "errors" + "os/exec" + "testing" +) + +// mockLookPath creates a mock LookPath function that returns success +// for binaries in the "exists" set. +func mockLookPath(exists map[string]string) func(string) (string, error) { + return func(name string) (string, error) { + if path, ok := exists[name]; ok { + return path, nil + } + return "", exec.ErrNotFound + } +} + +func TestFindExternalCommand_LongestFirstMatching(t *testing.T) { + // This test documents the longest-first (greedy) matching behavior. + // Why longest-first: More specific plugins should take precedence. + // Example: gog-docs-headings should handle "docs headings" even if + // gog-docs also exists. + + tests := []struct { + name string + args []string + existingBinaries map[string]string + wantPath string + wantArgs []string + }{ + { + name: "exact match single arg", + args: []string{"docs"}, + existingBinaries: map[string]string{ + "gog-docs": "/usr/bin/gog-docs", + }, + wantPath: "/usr/bin/gog-docs", + wantArgs: []string{}, + }, + { + name: "exact match two args", + args: []string{"docs", "headings"}, + existingBinaries: map[string]string{ + "gog-docs-headings": "/usr/bin/gog-docs-headings", + }, + wantPath: "/usr/bin/gog-docs-headings", + wantArgs: []string{}, + }, + { + name: "longest match wins over shorter", + // When both gog-docs and gog-docs-headings exist, + // gog-docs-headings should win for "docs headings" args + args: []string{"docs", "headings"}, + existingBinaries: map[string]string{ + "gog-docs": "/usr/bin/gog-docs", + "gog-docs-headings": "/usr/bin/gog-docs-headings", + }, + wantPath: "/usr/bin/gog-docs-headings", + wantArgs: []string{}, + }, + { + name: "falls back to shorter when longer not found", + // gog-docs-headings-list doesn't exist, so fall back to gog-docs-headings + args: []string{"docs", "headings", "list"}, + existingBinaries: map[string]string{ + "gog-docs-headings": "/usr/bin/gog-docs-headings", + }, + wantPath: "/usr/bin/gog-docs-headings", + wantArgs: []string{"list"}, + }, + { + name: "passes remaining args to plugin", + args: []string{"docs", "headings", "--docid", "ABC123"}, + existingBinaries: map[string]string{ + "gog-docs-headings": "/usr/bin/gog-docs-headings", + }, + wantPath: "/usr/bin/gog-docs-headings", + wantArgs: []string{"--docid", "ABC123"}, + }, + { + name: "three-level nesting", + args: []string{"docs", "headings", "list", "--limit", "10"}, + existingBinaries: map[string]string{ + "gog-docs": "/usr/bin/gog-docs", + "gog-docs-headings": "/usr/bin/gog-docs-headings", + "gog-docs-headings-list": "/usr/bin/gog-docs-headings-list", + }, + wantPath: "/usr/bin/gog-docs-headings-list", + wantArgs: []string{"--limit", "10"}, + }, + { + name: "no match returns empty", + args: []string{"unknown", "command"}, + existingBinaries: map[string]string{}, + wantPath: "", + wantArgs: nil, + }, + { + name: "empty args returns empty", + args: []string{}, + existingBinaries: map[string]string{}, + wantPath: "", + wantArgs: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Save and restore original lookPath + origLookPath := lookPath + defer func() { lookPath = origLookPath }() + + lookPath = mockLookPath(tt.existingBinaries) + + gotPath, gotArgs := findExternalCommandWithLookPath(tt.args) + + if gotPath != tt.wantPath { + t.Errorf("path = %q, want %q", gotPath, tt.wantPath) + } + + if !slicesEqual(gotArgs, tt.wantArgs) { + t.Errorf("args = %v, want %v", gotArgs, tt.wantArgs) + } + }) + } +} + +func TestTryExternalCommand_NotFound(t *testing.T) { + // Save and restore original lookPath + origLookPath := lookPath + defer func() { lookPath = origLookPath }() + + lookPath = mockLookPath(map[string]string{}) + + err := tryExternalCommand([]string{"nonexistent"}) + if !errors.Is(err, ErrExternalNotFound) { + t.Errorf("err = %v, want ErrExternalNotFound", err) + } +} + +func TestTryExternalCommand_EmptyArgs(t *testing.T) { + err := tryExternalCommand([]string{}) + if !errors.Is(err, ErrExternalNotFound) { + t.Errorf("err = %v, want ErrExternalNotFound", err) + } +} + +// slicesEqual compares two string slices for equality. +func slicesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/cmd/root.go b/internal/cmd/root.go index d301db26..55853694 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -112,6 +112,17 @@ func Execute(args []string) (err error) { kctx, err := parser.Parse(args) if err != nil { + // External command discovery (cargo/git-style plugins) + // Design: Post-parse fallback (Option B) + // Why: Built-in commands always take precedence over external plugins. + // This prevents accidental or malicious shadowing of core functionality + // and follows the git/cargo convention where built-ins win. + // See: https://github.com/steipete/gogcli/issues/188 + if extErr := tryExternalCommand(args); extErr == nil || !errors.Is(extErr, ErrExternalNotFound) { + // Either external command executed (replaced process) or exec failed + return extErr + } + // No external command found; return original Kong parse error parsedErr := wrapParseError(err) _, _ = fmt.Fprintln(os.Stderr, errfmt.Format(parsedErr)) return parsedErr