Skip to content

External command support#1104

Open
Soph wants to merge 14 commits intomainfrom
soph/external-command-support
Open

External command support#1104
Soph wants to merge 14 commits intomainfrom
soph/external-command-support

Conversation

@Soph
Copy link
Copy Markdown
Collaborator

@Soph Soph commented May 4, 2026

Add kubectl-style external commands (entire-<name>entire <name>)

When a user types entire command, look for an entire-command binary on $PATH and exec it with the remaining args. Stdio, signals, and exit codes pass through. External commands have no protocol, no JSON contract — just argv in, exit code out. Built-ins always win on name conflicts; entire-agent-* is reserved for the external agent protocol and skipped here.

Why kubectl-style and not gh-style

gh manages extensions through gh extension install and a dedicated install dir. That's heavier — version pinning, update notifications, an install registry. Kubectl-style PATH resolution is the lower-friction starting point and migrates cleanly to gh-style later if we ever want managed installs.

Design decisions worth reviewer attention

  • Telemetry posture — names on a hardcoded officialPlugins allowlist (currently empty) emit cli_plugin_executed events; everything else runs silently. Mirrors gh's policy because third-party command names can carry sensitive identifiers (project, vendor). Gated on exit-0 to match Cobra's PersistentPostRun semantics.
  • Post-hook parity — external commands bypass rootCmd.ExecuteContext, so MaybeRunPlugin runs versioncheck.CheckAndNotify itself. Without this, every external-command invocation would silently drop the version-update notice that built-ins receive.
  • Non-executable detectionexec.LookPath conflates "not on PATH" with "found but not executable". The resolver distinguishes them: a 0644 entire-pgr surfaces as a launch error, not a fall-through to "unknown command".
  • Signal handling — context cancellation sends SIGINT (with a 5s grace before SIGKILL via cmd.WaitDelay) so external commands can clean up rather than being force-killed.
  • Cobra lazy-init for help / completionMaybeRunPlugin calls Find before Execute, but Cobra adds the help and completion commands to the tree lazily inside Execute. Without priming them, Find(["help"]) returned an "unknown command" error and an entire-help binary on PATH would shadow the built-in. The resolver now calls InitDefaultHelpCmd and InitDefaultCompletionCmd before Find; both are idempotent and Execute calls them again later. Regression tests in plugin_test.go.
  • Environment filtering — Plugins do not inherit the parent's full environment. Defense in depth: a plugin shouldn't see AWS_ACCESS_KEY_ID, GITHUB_TOKEN, or OPENAI_API_KEY unless it has a reason to. The allowlist covers POSIX basics, locale (LC_*), terminal/color (NO_COLOR, TERM, …), CI detection (CI, GITHUB_ACTIONS, …), proxies (both cases), SSH agent, and Windows essentials, plus the ENTIRE_*, LC_*, and XDG_* namespaces. Users opt names back in via ENTIRE_PLUGIN_ENV="AWS_*,GH_TOKEN". This is a deliberate divergence from kubectl and gh, which inherit unfiltered — neither treats env scrubbing as a security boundary, but the cost of doing it here is tiny and it makes the plugin contract explicit. Full list and rationale in docs/architecture/external-commands.md.

What's not in this PR

  • No entire plugin list discovery command. Lazy resolution only.
  • officialPlugins is empty for now
  • No managed install dir.
  • No auth callback (entire auth token) for plugins. If/when ENTIRE_AUTH_TOKEN exists, the gh-style pull pattern is the better fit and builds cleanly on top of the env filtering landed here.

Docs

  • New: docs/architecture/external-commands.md — resolution rules, author contract, telemetry posture, environment filtering, side-by-side comparison with the agent protocol
  • Updated: CLAUDE.md cross-link

Note

Medium Risk
Adds pre-Cobra dispatch that execs arbitrary entire-* binaries from PATH, changing CLI control flow, exit-code/signal behavior, and telemetry/version-check side effects. Risk is moderated by extensive unit/integration coverage, but failures could break command routing or process handling across platforms.

Overview
Adds kubectl-style external command support so entire <name> can resolve and exec an entire-<name> binary on PATH when no built-in subcommand matches, with stdio and exit codes passed through and reserved agent-* names explicitly skipped.

Introduces cli.MaybeRunPlugin/runPlugin with PATH resolution hardening (reject flags/path traversal, detect found-but-not-executable as a launch error), forwards ENTIRE_CLI_VERSION and (when available) ENTIRE_REPO_ROOT, and mirrors built-in behavior by running version-check notice and allowlist-only telemetry (cli_plugin_executed, no args/flags) on successful plugin runs.

Adds unit + end-to-end integration tests for routing precedence, arg passthrough, env forwarding, non-executable handling, and SIGINT propagation, plus docs in docs/architecture/external-commands.md and a small export of external.StripExeExt for cross-platform name handling.

Reviewed by Cursor Bugbot for commit e9b1392. Configure here.

Copilot AI review requested due to automatic review settings May 4, 2026 15:47
@Soph Soph requested a review from a team as a code owner May 4, 2026 15:47
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread cmd/entire/cli/integration_test/plugin_dispatch_test.go Outdated
ashtom added a commit that referenced this pull request May 5, 2026
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
@Soph
Copy link
Copy Markdown
Collaborator Author

Soph commented May 6, 2026

@BugBot review again

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 e9b1392. Configure here.

Comment thread cmd/entire/cli/plugin.go
Soph and others added 14 commits May 6, 2026 18:34
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
Cobra adds the `help` and `completion` commands to the tree inside
Execute, not in the constructor. resolvePlugin runs before Execute, so
Find(["help"]) was returning an "unknown command" error and the
"built-ins always win" guard never fired — letting an entire-help (or
entire-completion) binary on PATH shadow the built-in.

Call InitDefaultHelpCmd / InitDefaultCompletionCmd before Find. Both
are idempotent and Execute calls them again later. Adds regression
tests that fail on the unpatched code.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 2bff6dccfe6f
External commands previously inherited the parent's full environment.
That meant a binary on PATH saw AWS_ACCESS_KEY_ID, GITHUB_TOKEN,
OPENAI_API_KEY, and similar credentials whether or not it had any
business with them. kubectl and gh take the same approach, but neither
treats env scrubbing as a security boundary.

Defense in depth here is cheap: forward only OS-plumbing, locale,
terminal, CI-detection, proxy, SSH-agent, and Windows essentials by
default, plus the ENTIRE_*, LC_*, and XDG_* namespaces. Everything
else is dropped. Users can opt names back in via ENTIRE_PLUGIN_ENV
(comma-separated list of exact names or PREFIX_* wildcards) — for
example ENTIRE_PLUGIN_ENV=AWS_*,GH_TOKEN.

Caller-injected extras (ENTIRE_CLI_VERSION, ENTIRE_REPO_ROOT) are
appended last so they override any matching parent value, per
cmd/exec's last-wins contract.

Coverage: table-driven unit test in plugin_env_test.go for the
allowlist, prefix, override-exact, override-wildcard, whitespace,
malformed-entry, and last-wins cases; integration tests assert
end-to-end through the real entire binary that GITHUB_TOKEN /
AWS_ACCESS_KEY_ID are dropped and that ENTIRE_PLUGIN_ENV opens names
back up. Documents the behavior and the escape hatch in
docs/architecture/external-commands.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: dcce922e6495
@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