Skip to content

Managed plugin install dir + ENTIRE_PLUGIN_DATA_DIR#1121

Draft
ashtom wants to merge 22 commits intosoph/external-command-supportfrom
ashtom/plugin-exploration
Draft

Managed plugin install dir + ENTIRE_PLUGIN_DATA_DIR#1121
ashtom wants to merge 22 commits intosoph/external-command-supportfrom
ashtom/plugin-exploration

Conversation

@ashtom
Copy link
Copy Markdown
Member

@ashtom ashtom commented May 5, 2026

https://entire.io/gh/entireio/cli/trails/298

Stacked on #1104. Targets `soph/external-command-support` as the base so the diff shows only the additive layer; rebase onto `main` once #1104 lands.

Summary

Two additions on top of the kubectl-style dispatcher in #1104. Both are purely additive — the dispatcher in `plugin.go` keeps its raw `$PATH` model.

  • `ENTIRE_PLUGIN_DATA_DIR` — per-plugin durable storage dir set in `runPlugin`'s env regardless of where the binary lives. Plugins installed via raw `$PATH` and via `entire plugin install` get the same contract.
  • Managed bin dir at `$XDG_DATA_HOME/entire/plugins/bin` (override: `$ENTIRE_PLUGIN_DIR/bin`). `main.go` prepends it to `$PATH` before `MaybeRunPlugin` runs, so the existing `exec.LookPath` discovers managed installs with no special-casing.
  • `entire plugin install/list/remove` Cobra commands manage the dir. Local-symlink installs only; release-asset and git-clone installs remain deferred.

Docs at `docs/architecture/external-commands.md` updated with a "Managed install directory" subsection and the new env var row.

Why this shape

This is the smaller follow-up I owed after closing #1116 (gh-style managed store). The kubectl dispatcher in #1104 is the right primitive; this just gives users `entire plugin install` for the local-dev workflow without forking the resolution path.

Two things from #1116 we deliberately did not carry over:

  • No `entire plugin exec` escape hatch. No real demand yet; the first time a built-in shadows a useful plugin we can revisit.
  • No stricter name validation. Soph's path-traversal + `agent-` prefix checks in `isPluginCandidate` are sufficient.

Test plan

  • `mise run lint` (0 issues)
  • `mise run test` (cli unit tests including new `plugin_store_test.go`)
  • `mise run test:integration` (`TestExternalCommand_EnvVarsForwarded` extended to assert `ENTIRE_PLUGIN_DATA_DIR`)
  • `mise run test:e2e:canary`
  • Manual: `entire plugin install ./entire-hello` → `entire plugin list` → `entire hello world` resolves via the auto-prepended PATH and `ENTIRE_PLUGIN_DATA_DIR` is set to the per-plugin path

🤖 Generated with Claude Code


Note

Medium Risk
Medium risk because it introduces new filesystem-managed plugin install/remove behavior and mutates PATH/plugin environments at startup, which could affect command resolution and subprocess behavior across platforms.

Overview
Adds a built-in entire plugin command group to install/list/remove external plugins into a per-user managed directory, using symlink→hardlink→copy installs with atomic replace and Windows extension/bare-name conflict handling.

Makes managed installs automatically discoverable by prepending the managed bin dir to PATH at startup (with a restore for built-in command runs), while keeping kubectl-style exec.LookPath dispatch.

Extends plugin execution to set ENTIRE_PLUGIN_DATA_DIR (per-plugin durable storage) and to upsert ENTIRE_* vars to avoid inherited shadowing; adds validation to reject ./.. plugin names and tests/docs covering the new behavior.

Reviewed by Cursor Bugbot for commit 2afff28. Configure here.

Soph and others added 13 commits May 4, 2026 13:53
When the user invokes `entire <name>` and `<name>` isn't a built-in
subcommand, look for an `entire-<name>` binary on PATH and exec it with
the remaining args. Stdio passes through, exit code propagates, and
context cancellation sends SIGINT (with a 5s grace before SIGKILL) so
plugins get a chance to clean up.

Built-ins always win on name conflict, and binaries matching the
`entire-agent-*` prefix are explicitly skipped — they belong to the
external agent protocol, not the passthrough plugin path.

Plugin invocations get `ENTIRE_REPO_ROOT` and `ENTIRE_CLI_VERSION` in
their environment, mirroring what external agents already see.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 9965c83a5806
Mirror gh's extension policy: third-party plugin names can carry
sensitive identifiers, so we never record them. Plugins listed in the
hardcoded officialPlugins allowlist (currently empty — populate as
Entire ships its own plugins) emit a `cli_plugin_executed` event with
the plugin name; everything else runs silently.

Telemetry follows the existing opt-in semantics — settings.Telemetry
must be explicitly true and ENTIRE_TELEMETRY_OPTOUT must be unset. The
event payload deliberately omits flags and args; only command path,
plugin name, cli_version, os, arch, and isEntireEnabled are recorded.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 34fb779fd053
The unit tests in cmd/entire/cli/plugin_test.go exercise resolvePlugin
and runPlugin in isolation but never invoke the real `entire` binary,
so the wiring in cmd/entire/main.go (pre-Cobra dispatch, exit-code
propagation, stdio passthrough, signal handling) was only covered by
manual smoke checks.

These integration tests build the actual binary and shell out:
- happy-path argv/stdout/stderr/exit-code
- exit code propagation (non-zero)
- built-in commands win over plugins of the same name
- unknown command falls through to Cobra's normal error path
- flag-shaped args after the plugin name aren't eaten by Cobra
- stdin passthrough
- ENTIRE_CLI_VERSION reaches the plugin's environment
- entire-agent-* binaries are skipped (reserved for the agent protocol)

A separate Unix-only file covers the signal edge: SIGINT to the parent
must reach the plugin via the dispatcher's context-cancel handler so
the plugin can clean up rather than being SIGKILL'd. The plugin writes
a readiness marker before signalling to avoid racing the trap install
against the kill.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: abcdeb6500a8
Code reuse:
- Export agent/external.StripExeExt and reuse it from plugin.go instead of
  the duplicated stripPluginExeExt helper.
- Replace the manual loop in IsOfficialPlugin with slices.Contains.

Quality:
- Collapse the redundant double-prefix check in isAgentProtocolBinary —
  StripExeExt only removes suffixes, so the un-stripped check is
  subsumed by the stripped check.
- Drop the stdin/stdout/stderr params from runPlugin; production always
  routes to os.Std*, and the test only asserts argv + exit code.
- Align runPlugin's error message with the codebase convention
  ("Failed to run plugin X" — no "entire:" prefix).
- Strip narrative/restating doc comments across plugin.go, plugin_official.go,
  detached.go, main.go, and the tests; keep only non-obvious WHY.
- Replace the always-passing TestOfficialPlugins_DefaultAllowlist with
  the misleading "//immutable" comment removed from the global.

Test improvements:
- TestPluginDispatch_BuiltinWins now uses writePluginScript like its
  siblings instead of inlining its own shell.
- TestPluginDispatch_AgentProtocolBinarySkipped now also asserts the
  invocation falls through to Cobra's unknown-command path, not just
  that the binary wasn't run.
- The signal test's plugin-loop bound is now expressed via a named
  constant tied to the parent's WaitDelay, documenting why it must
  outlast the SIGKILL fallback window.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 65bf76b862d4
…rough

exec.LookPath conflates "not on PATH" with "found but not executable",
so an entire-<name> file with mode 0o644 was silently falling through
to Cobra's generic unknown-command path — confusing for users who
installed a plugin but forgot to chmod +x.

resolvePlugin now scans PATH on LookPath miss; if it finds a
non-directory file with the right name, returns it so runPlugin reports
"Failed to run plugin <name>" with a non-zero exit. This matches what
the MaybeDispatchPlugin doc comment already claimed.

Coverage:
- TestResolvePlugin_NonExecutableSurfacesAsLaunchError (unit)
- TestPluginDispatch_NonExecutableReportsLaunchError (integration)

Also: the new test added a third "windows" string literal in the cli
package, tripping goconst — replaced with the existing windowsGOOS
constant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: b17ee321e0b8
Plugin commands bypass rootCmd.ExecuteContext, so PersistentPostRun in
root.go never runs for them. That dropped versioncheck.CheckAndNotify
entirely for plugins — a user-visible regression compared with every
built-in top-level command, which surface "a new release of Entire is
available" notices through that hook.

MaybeDispatchPlugin now invokes CheckAndNotify directly after the
plugin returns. Future shared post-run behavior added at the root will
need similar wiring here unless we factor it out.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: a0e53aaf1ef1
Cobra's PersistentPostRun runs only after a successful RunE, so built-in
command telemetry and the version-check notice both skip failed/crashing
invocations. Plugin telemetry was firing unconditionally after runPlugin,
which would inflate usage events with launch failures and make the plugin
event stream incomparable with the existing command stream.

Wrap both maybeTrackPluginInvocation and versioncheck.CheckAndNotify in
an exit-0 guard so plugin post-hooks track Cobra's semantics.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 710ea750b55b
Adds docs/architecture/plugin-dispatch.md covering the kubectl-style
passthrough model: discovery rules, plugin author contract (env vars,
exit codes, signal handling, Windows specifics), telemetry posture
(allowlist + reasoning), and a side-by-side comparison with the
agent protocol so the two PATH-naming conventions
(entire-<name> vs entire-agent-<name>) are unambiguous.

Also points the project CLAUDE.md at the new doc so it's reachable
when navigating the codebase.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 4efc7b48f24c
The previous wording ("Eager scan during init (or lazy for hooks)")
overstates how external agent discovery actually works. The
implementation is conditional and command-specific:

- Normal discovery is gated by the external_agents setting
  (agent/external/discovery.go).
- Several commands trigger it lazily at runtime: attach, resume,
  rewind, hooks_cmd.
- Setup flows bypass the gate entirely via DiscoverAndRegisterAlways.

Reword to "Lazy at command entry, gated by external_agents setting
(setup flows bypass the gate via DiscoverAndRegisterAlways)" so plugin
authors comparing the two systems get an accurate mental model.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 7e4787f2290b
The plugin-dispatch architecture doc promotes ENTIRE_REPO_ROOT to part
of the plugin contract alongside ENTIRE_CLI_VERSION, but coverage only
asserted the latter. A regression in repo-root propagation would
contradict the documented behavior without failing tests.

Spawn the parent CLI from inside a freshly-init'd git repo (via
testutil.InitRepo), have the plugin echo both env vars to a file,
and assert ENTIRE_REPO_ROOT matches the resolved repo path exactly.

Symlinks are evaluated up front to match what paths.WorktreeRoot
returns on macOS (/var → /private/var), avoiding flakes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: a031a2305924
The user-facing "dispatch" terminology collides with the existing
top-level `entire dispatch` command, which causes confusion when
discussing the kubectl-style PATH-resolution feature.

Sweep:

- Doc: docs/architecture/plugin-dispatch.md → external-commands.md,
  title "External Commands", prose updated throughout. Comparison
  table header now reads "External Commands" vs "Agent Protocol".
- API: MaybeDispatchPlugin → MaybeRunPlugin (in plugin.go and main.go).
- Tests: TestPluginDispatch_* → TestExternalCommand_* across both
  integration test files; files renamed to external_command_test.go
  and external_command_signal_unix_test.go.
- CLAUDE.md cross-link updated.

Internal nouns (officialPlugins, runPlugin, resolvePlugin,
IsOfficialPlugin, plugin.go) are kept — kubectl uses "plugin"
internally too, and the conflict was only with the verb.

No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 63a22badde7c
The integration tests that spawn the real entire binary were using raw
exec.Command, but the established pattern in this package
(testenv.go's RunCLIWithError) is execx.NonInteractive, which puts the
child in a new session with no controlling terminal. That ensures
interactive.CanPromptInteractively() returns false in the child without
relying on env-var plumbing — important if a future change makes any
plugin-dispatch path attempt interactive prompts.

Switches all 10 invocations across external_command_test.go and
external_command_signal_unix_test.go. The signal test continues to
pass: Setsid puts the child in a new session/process group, but
direct cmd.Process.Signal(os.Interrupt) to the parent's PID is
unaffected, and the parent's context-cancel handler still forwards
SIGINT to the plugin via exec.Cmd.Cancel.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 0171e6b97736
Layered on top of the kubectl-style dispatcher in #1104 — purely
additive, no parallel mechanism.

- ENTIRE_PLUGIN_DATA_DIR: per-plugin durable storage path. Set in
  runPlugin's env regardless of where the binary lives, so plugins
  installed via raw $PATH and via 'entire plugin install' get the same
  contract.

- Managed bin dir at $XDG_DATA_HOME/entire/plugins/bin (override:
  $ENTIRE_PLUGIN_DIR/bin). main.go prepends it to $PATH at startup so
  the existing exec.LookPath resolution in resolvePlugin discovers
  managed installs without any special-casing.

- 'entire plugin install/list/remove' for managing the dir.
  Local-symlink installs only; binary-release and git-clone installs
  remain deferred until there's demand.

Docs in docs/architecture/external-commands.md updated to describe
the managed dir and the ENTIRE_PLUGIN_DATA_DIR env var.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: c41bcd1aec30
Copilot AI review requested due to automatic review settings May 5, 2026 16:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a lightweight “managed plugin install” layer on top of the existing kubectl-style external command dispatcher by (1) prepending a per-user managed bin directory to PATH at startup and (2) injecting a per-plugin durable storage directory via ENTIRE_PLUGIN_DATA_DIR for every plugin invocation.

Changes:

  • Prepend a managed plugin bin dir to PATH before MaybeRunPlugin so exec.LookPath can discover managed installs without a separate resolution mechanism.
  • Introduce ENTIRE_PLUGIN_DATA_DIR (computed from a per-user plugin root + plugin name) and forward it to external commands.
  • Add entire plugin install/list/remove commands plus unit/integration coverage for the managed store and the new env var.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
docs/architecture/external-commands.md Documents managed plugin dir discovery and ENTIRE_PLUGIN_DATA_DIR.
cmd/entire/main.go Prepends managed plugin bin dir to PATH before plugin dispatch.
cmd/entire/cli/root.go Registers the new entire plugin command group.
cmd/entire/cli/plugin.go Injects ENTIRE_PLUGIN_DATA_DIR when running a resolved plugin.
cmd/entire/cli/plugin_test.go Updates unit test call site for runPlugin signature change.
cmd/entire/cli/plugin_store.go Implements managed plugin bin/data directories + install/list/remove helpers.
cmd/entire/cli/plugin_store_test.go Adds unit tests for managed store behaviors and PATH prepending.
cmd/entire/cli/plugin_group.go Implements entire plugin {install,list,remove} Cobra commands.
cmd/entire/cli/integration_test/external_command_test.go Extends integration test to assert ENTIRE_PLUGIN_DATA_DIR.

Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go Outdated
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_group.go Outdated
Comment thread cmd/entire/cli/root.go
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go Outdated
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go Outdated
- validatePluginName: shared rules, used by PluginDataDir and
  InstallPluginFromPath. Rejects "."/".." (which would collapse out of
  the joined path), agent-* (dispatcher reserves it), flag-shaped
  names, and slashes. isPluginCandidate gets the same "."/".."
  tightening for defense in depth.

- bareNameFromBinaryName: strip .exe/.bat/.cmd only on Windows. On
  Unix the dispatcher uses exact-match exec.LookPath, so accepting
  entire-foo.exe would yield a managed entry that could never resolve.

- InstallPluginFromPath: refuse self-install when the source path
  equals the managed destination (path-clean equality only — using
  os.SameFile would false-fire on the legitimate "previous install is
  a symlink to src" case). Replace step is now atomic via symlink-to-
  tmp + rename, so a failed --force never leaves the previous install
  missing.

- plugin_group.go Long help: describe the actual XDG / Windows /
  ENTIRE_PLUGIN_DIR precedence instead of hard-coding the Linux/macOS
  default.

- external-commands.md: note that the new built-in `entire plugin`
  command group shadows any pre-existing `entire-plugin` external
  command (intentional, but worth flagging).

Tests:
- TestValidatePluginName + TestPluginDataDir_RejectsPathTraversal
- TestInstallPluginFromPath_RejectsAgentReservedName
- TestInstallPluginFromPath_RejectsSelfInstall (verifies source
  survives the rejection)
- TestInstallPluginFromPath_AtomicForceReplace
- TestBareNameFromBinaryName: platform-conditional cases

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 93848fc24cf0
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 5, 2026

Thanks Copilot and Cursor — addressed in 7e1451d. Mapping each comment to the fix:

Copilot — PluginDataDir lets ./.. escape the data subtree
Added validatePluginName rejecting "", ".", "..", agent-*, leading -, and path separators. PluginDataDir calls it first; same rules also tightened in isPluginCandidate so a literal entire . can't even reach runPlugin.

Copilot + Cursor — InstallPluginFromPath accepts unrunnable names (e.g. entire-agent-foo)
The derived bare name is now run through validatePluginName. entire plugin install /path/to/entire-agent-foo errors out with plugin name "agent-foo" is reserved for the external agent protocol.

Copilot + Cursor — bareNameFromBinaryName strips .exe/.bat/.cmd on every platform, breaking Unix discovery
Stripping is now gated on runtime.GOOS == windowsGOOS. On Unix, entire-foo.exe no longer collapses to foo (which would have been an entry that exact-match exec.LookPath could never find). Test exercises both branches.

Cursor — --force can delete the source if src == dest
Added a self-install guard that compares filepath.Clean(src) with filepath.Clean(dest) and refuses early. Used path equality rather than os.SameFile because SameFile would false-fire on the normal repeat-install case (where the existing managed entry is a symlink we created pointing back at src). Test verifies the source survives the rejection.

Cursor — failed --force replace leaves the plugin uninstalled
Replaced os.Remove(dest) + os.Symlink(src, dest) with os.Symlink(src, dest.tmp) + os.Rename(dest.tmp, dest). Atomic on POSIX; Go's os.Rename on Windows uses MoveFileEx with MOVEFILE_REPLACE_EXISTING. If symlink or rename fails, the previous install is intact.

Copilot — help text hard-codes ~/.local/share/...
plugin_group.go Long description now lists the precedence ($ENTIRE_PLUGIN_DIR/bin$XDG_DATA_HOME/... → Linux/macOS default → %LOCALAPPDATA%\...).

Copilot — registering entire plugin shadows any existing entire-plugin external command
The collision is intentional (managing plugins is a built-in concern), but added a "Compatibility note" callout to docs/architecture/external-commands.md flagging it.

mise run check is clean; new tests cover validator rules, self-install rejection, atomic replace, and platform-conditional extension stripping.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go Outdated
- pluginParentDir: gate XDG_DATA_HOME to non-Windows. The Windows
  branch (LOCALAPPDATA) was previously unreachable when XDG_DATA_HOME
  was set in MSYS/Cygwin environments, producing a surprising
  location. Tests for both branches.

- materializeManagedEntry: new helper that tries symlink → hardlink →
  copy in that order. Symlinks on Windows require Developer Mode or
  admin, which would have made `entire plugin install` unusable for
  typical users. Mirrors the pattern in setup_test.go's
  copyExecutable. Symlink stays the preferred path so the dev-loop
  property of "rebuild source, managed entry follows" is preserved
  wherever it works.

- bareNameFromBinaryName comment: clarify that on Unix we don't strip
  extensions because doing so would create a list/invocation-name
  mismatch (entry listed as "pgr" but only invocable as "pgr.exe"),
  not because the entry would be uninvocable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 5671463236d4
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 5, 2026

Second round addressed in 6aab5ce.

pluginParentDir honors XDG_DATA_HOME on Windows
Gated the XDG branch to non-Windows. On Windows, resolution is now ENTIRE_PLUGIN_DIRLOCALAPPDATA%USERPROFILE%\AppData\Local\..., with XDG_DATA_HOME deliberately ignored even when set (e.g. under MSYS/Cygwin). Two new tests pin both branches.

os.Symlink fails on Windows for non-admin users
Introduced materializeManagedEntry(src, dest, srcInfo) that tries symlink → hardlink → copy in that order. Mirrors the pattern in setup_test.go's copyExecutable. Symlink remains the preferred path so the dev-loop property (rebuild source, managed entry follows) is preserved on Unix and on Windows-with-Developer-Mode. The atomic-rename via dest.tmp is unchanged. Added a Unix smoke test that the helper terminates correctly on the happy path; a contrived "symlink and hardlink both fail" scenario is hard to set up portably and the implementation just composes three well-tested stdlib calls.

bareNameFromBinaryName comment is misleading
Right — on Unix entire-pgr.exe is invocable, just only as entire pgr.exe (since isPluginCandidate allows dots). The reason we don't strip there is the list/invocation-name mismatch ("listed as pgr but only pgr.exe resolves"), not literal non-invocability. Comment rewritten to explain that.

mise run check clean; canary green.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comment thread cmd/entire/cli/plugin_store.go Outdated
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go Outdated
Comment thread cmd/entire/cli/plugin_store_test.go Outdated
Comment thread cmd/entire/cli/plugin.go
- pluginParentDir: defer os.UserHomeDir to the fallback branches so
  LOCALAPPDATA / XDG_DATA_HOME / ENTIRE_PLUGIN_DIR resolution doesn't
  fail when home lookup is broken.

- Split the managed-dir constant into two segments
  (pluginManagedTopDir + pluginManagedSubDir). filepath.Join now
  produces native separators throughout, including on Windows where
  "entire/plugins" would otherwise leak forward slashes into
  user-facing paths.

- PrependPluginBinDirToPATH: use strings.EqualFold on Windows when
  checking whether the first PATH entry is already the managed dir.
  Windows PATH lookups are case-insensitive, so a case-different first
  entry refers to the same dir and we shouldn't double-prepend.

- TestMaterializeManagedEntry: rename _FallsThroughToCopy to
  _HappyPath. The previous name implied the test exercised the copy
  fallback, but on Unix the symlink branch always wins. Forcing the
  earlier branches to fail without injection hooks is non-portable;
  honest naming is the cheaper fix.

- runPlugin: when PluginDataDir errors (only realistic case: a
  degenerate environment with no resolvable home + no LOCALAPPDATA /
  XDG_DATA_HOME / ENTIRE_PLUGIN_DIR), warn to stderr and proceed
  without ENTIRE_PLUGIN_DATA_DIR. Plugins that don't read the var
  shouldn't be punished for the user's environment, but the failure
  is now visible. Doc updated to reflect the contract caveat.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 6e60baaf0d7b
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

Third round addressed in 8ac8bc5.

os.UserHomeDir blocks env-var branches
Reordered pluginParentDir. Each branch now calls UserHomeDir only when its env var (LOCALAPPDATA / XDG_DATA_HOME) is absent. ENTIRE_PLUGIN_DIR still wins outright. A degenerate environment with LOCALAPPDATA set but home lookup broken now resolves correctly.

Forward slash in path constant
Split "entire/plugins" into pluginManagedTopDir = "entire" + pluginManagedSubDir = "plugins". filepath.Join now produces native separators throughout, so Windows paths in entire plugin list won't show \…\entire/plugins\bin.

Case-insensitive PATH idempotency on Windows
Introduced pathEntriesEqual(a, b) which uses strings.EqualFold on Windows and exact equality elsewhere. PrependPluginBinDirToPATH now uses it, so a case-different first entry doesn't trigger a double-prepend.

Misleading test name
Renamed TestMaterializeManagedEntry_FallsThroughToCopyTestMaterializeManagedEntry_HappyPath and updated the doc comment to be honest: on Unix the symlink branch wins, the hardlink/copy fallbacks exist for Windows-without-Developer-Mode, and forcing them to fire without injection hooks isn't portable.

ENTIRE_PLUGIN_DATA_DIR contract violation on error
Went with option (a): in runPlugin, when PluginDataDir fails (only realistic case: os.UserHomeDir failure with no env-var override), print warning: ENTIRE_PLUGIN_DATA_DIR not set for plugin %q: %v to stderr and proceed. The validator branch can't fire here because isPluginCandidate already accepted the name in resolvePlugin. Doc in external-commands.md updated to reflect the caveat.

mise run check clean; canary green.

@ashtom ashtom requested a review from Copilot May 6, 2026 06:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comment thread cmd/entire/cli/plugin.go Outdated
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_group.go
Comment thread cmd/entire/cli/plugin_store_test.go Outdated
- runPlugin: use upsertEnv (already in package, see explain.go) for
  ENTIRE_CLI_VERSION, ENTIRE_REPO_ROOT, ENTIRE_PLUGIN_DATA_DIR. Plain
  append left any pre-existing ENTIRE_* values from the user's shell
  in place; getenv() typically returns the first match, so the
  CLI-computed value would be silently shadowed.

- pathEntriesEqual: filepath.Clean both sides before comparing.
  Trailing-separator differences (`.../bin/` vs `.../bin`) and slash
  orientation on Windows would otherwise miss the idempotency check
  and double-prepend on every invocation.

- materializeManagedEntry: split the copy fallback into a streaming
  copyFileStreaming helper using os.Open + io.Copy + chmod-on-create
  via os.OpenFile mode. os.ReadFile loaded the whole binary into
  memory, which is a real spike for plugins in the tens of MB. On
  copy failure, dest is removed so a partial file isn't left behind.

- plugin install help: replace "symlink" wording with "link or copy",
  and explain the symlink-preferred-then-fallback order so the help
  matches actual behavior on Windows-without-Developer-Mode.

- TestPluginParentDir_WindowsIgnoresXDG: replace the loose
  strings.Contains(got, "fake") assertion with a structural check
  (filepath.HasPrefix(clean(got), clean(xdgRoot))). The substring
  could false-fail on Windows paths that legitimately contain the
  literal "fake".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 0b689149c0a3
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

Fourth round addressed in 48391f9.

Env vars appended after os.Environ() could be shadowed
runPlugin now uses upsertEnv (already in the package, in explain.go) for ENTIRE_CLI_VERSION, ENTIRE_REPO_ROOT, and ENTIRE_PLUGIN_DATA_DIR. Pre-existing values from the user's shell are now overwritten rather than left as the first match.

pathEntriesEqual doesn't normalize trailing separators
Both sides now run through filepath.Clean before the comparison. That handles trailing slashes (.../bin/ vs .../bin) and Windows slash/backslash mixing.

Copy fallback reads whole binary into memory
Split out copyFileStreaming using os.Open + io.Copy + os.OpenFile with mode-on-create. Cleans up the partial dest on copy failure. Plugin binaries in the tens of MB no longer cause heap spikes.

entire plugin install help says "symlinking"
Updated to "Link or copy" with a brief note about the symlink → hardlink → copy order. Group help got the same treatment.

TestPluginParentDir_WindowsIgnoresXDG substring assertion
Replaced strings.Contains(got, "fake") with a structural check: cleaned got must not have the cleaned XDG-rooted path as a prefix. No more false-fails on legitimate Windows paths that happen to contain the literal "fake".

mise run check clean, integration + canary green.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread cmd/entire/cli/plugin_store.go Outdated
bareNameFromBinaryName now calls into agent/external rather than
maintaining a parallel .exe/.bat/.cmd switch. plugin.go already
imports the same package for isAgentProtocolBinary, so there's no new
layering cost — and a single source of truth means agent discovery
and managed-plugin listing will stay consistent if the canonical
Windows-executable extension list ever needs to grow.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 4b119065e61e
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

Fifth round addressed in 108065b.

stripPluginExeExt duplicates agent/external.StripExeExt
Dropped the local copy and call external.StripExeExt directly. plugin.go already imported the package for isAgentProtocolBinary, so there's no new layering cost — single source of truth for the Windows-executable extension list across agent discovery and managed-plugin listing.

mise run check clean.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread cmd/entire/cli/plugin_group.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

@cursor review

Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin_store.go
Comment thread cmd/entire/cli/plugin.go
- Bare-name conflict check (Medium). InstallPluginFromPath now lists
  every managed entry whose bare name matches and refuses to install
  without --force when any exist. With --force, all variants other
  than the destination are removed before the atomic rename. On
  Windows this prevents entire-foo.exe and entire-foo.bat coexisting,
  with PATHEXT silently choosing which one runs.

- Random tmp path (Medium). Replace dest+".tmp" with a 16-hex-char
  ".install-<random>" path generated via crypto/rand. The previous
  scheme could clobber a legitimate plugin named "foo.tmp" (file
  "entire-foo.tmp"), since the validator allows dots. The
  ".install-" prefix doesn't match the "entire-" listing filter, so
  in-progress installs don't surface in `entire plugin list`.

- Strip inherited ENTIRE_PLUGIN_DATA_DIR on error (Low). Add
  removeEnvKey alongside upsertEnv in explain.go. runPlugin now
  drops the inherited value when PluginDataDir fails so the warning's
  "not set" claim matches what the plugin actually sees, blocking a
  shell-set ENTIRE_PLUGIN_DATA_DIR from leaking through.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 28dbaf5cf678
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

Cursor Bugbot round addressed in b8ef07e.

Extension variants bypass replacement (Medium)
InstallPluginFromPath now lists every managed entry whose bare name matches and refuses without --force when any exist. With --force, all variants other than the destination are removed before the atomic rename. New installedVariantsByBareName helper centralizes the lookup. Closes the Windows hole where entire-foo.exe and entire-foo.bat could coexist with PATHEXT silently picking the winner.

Temporary path can clobber a legitimate plugin (Medium)
Replaced dest + ".tmp" with makeInstallTmpPath — 16 hex chars from crypto/rand joined to a .install- prefix. The dot-prefix is deliberately distinct from entire-, so ListInstalledPlugins filters it out and an in-progress install never surfaces in entire plugin list. The previous scheme would have clobbered a valid plugin literally named foo.tmp (the validator allows dots).

Stale ENTIRE_PLUGIN_DATA_DIR leak (Low)
Added removeEnvKey next to upsertEnv in explain.go. runPlugin's error path now strips any inherited value before warning, so the "not set" message matches what the plugin actually sees.

Tests added for tmp-path uniqueness, dotted-plugin survival, bare-name conflict gating, and removeEnvKey. mise run check clean, integration + canary green.

@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

@cursor review

Comment thread cmd/entire/cli/plugin_store.go Outdated
Comment thread cmd/entire/main.go Outdated
- RemoveInstalledPlugin now iterates installedVariantsByBareName and
  removes every match, not just the first. Symmetric with the
  install-side bare-name conflict fix: on Windows, entire-foo.exe
  and entire-foo.bat could share the bare name "foo", and the
  previous implementation would have left a runnable variant behind
  after `entire plugin remove foo` reported success.

- PrependPluginBinDirToPATH now returns a restore closure. main.go
  invokes it after MaybeRunPlugin returns false, so built-in commands
  and the subprocesses they spawn (git, hooks, less, ...) see the
  user's original PATH rather than one with the managed plugin dir
  prepended. When a plugin is dispatched, the restore is intentionally
  skipped: the os.Exit ends the process, and the plugin child
  inherits the prepended PATH so it can spawn sibling managed plugins.

  No-op cases (already-prepended, lookup failure, Setenv error) return
  no-op restore funcs so callers always have a safe func to call.

Tests:
- TestPrependPluginBinDirToPATH now asserts PATH is restored to its
  exact prior value after the closure runs, and that an idempotent
  second call returns a no-op restore.
- TestRemoveInstalledPlugin_RemovesAllVariants exercises the loop
  body with a single Unix entry; the multi-variant case it guards
  against is Windows-specific and covered by the implementation
  reading installedVariantsByBareName.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 5d2e94150bf2
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

Addressed in 2afff28.

Variant removal is incomplete (Medium)
RemoveInstalledPlugin now iterates installedVariantsByBareName and removes every match, symmetric with the install-side fix. On Unix the loop typically runs once; on Windows it nukes all of .exe/.bat/.cmd for the same bare name.

Managed PATH leaks into built-ins (Medium)
PrependPluginBinDirToPATH now returns a restore closure. main.go invokes it after MaybeRunPlugin returns false, so built-ins and their subprocesses see the user's original PATH. When a plugin is dispatched, the restore is intentionally skipped — the os.Exit ends the process and the plugin child inherits the prepended PATH so it can spawn sibling managed plugins.

Test now asserts PATH is restored to its exact prior value and that a no-op call (already-prepended) returns a safe no-op closure.

mise run check clean, integration + canary green.

@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2afff28. Configure here.

Comment thread cmd/entire/cli/plugin_store.go
Windows PATHEXT defaults to ".COM;.EXE;.BAT;.CMD;…", so exec.LookPath
resolves a `.com` next to a `.exe`. Without stripping it in the
canonical helper, foo.exe and foo.com end up with distinct bare names
in the managed plugin store while PATHEXT silently picks the .com
first when a user types `entire foo` — giving them a different binary
than the one their bare-name conflict check would have flagged.

The fix is in StripExeExt itself rather than a parallel local list,
so plugin install/list/remove and external-agent discovery share one
source of truth for "Windows executable extension".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: d2f8ff912a93
@ashtom
Copy link
Copy Markdown
Member Author

ashtom commented May 6, 2026

Addressed in 6118494.

Windows plugin variants can coexist (Medium)
Added .com to the canonical StripExeExt list in agent/external/discovery.go (which the plugin store now reuses). Concrete fix:

  • Before: installing entire-foo.com after entire-foo.exe produced bare names "foo.com" and "foo" — the bare-name conflict check missed them.
  • After: both strip to "foo", so the install command refuses without --force and removal nukes both variants.

The fix lives in the canonical helper rather than a parallel local list so plugin install/list/remove and external-agent discovery stay in sync. Test added for .com in lower and upper case.

mise run check clean.

@Soph Soph force-pushed the soph/external-command-support branch from c6b5e5c to ec0a9cb Compare May 6, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants