Repository navigation
Install Graph on demand with visible plugin installation progress - #2324
Merged
Merged
Conversation
Show installation progress and announce the forwarded command. Update the terminal renderer so completed confirmation prompts are cleared, with regression coverage for terminal cleanup and plugin install/dispatch behavior. Entire-Checkpoint: 01M211S73J1V1YPNB123826DE8
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new stderr “Running plugin with command …” line prints user-supplied arguments and can expose secrets and add noisy output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enhances the CLI’s external-command dispatch to support on-demand installation of the entire-graph plugin when users run entire graph ... and the plugin is not present, while also adding visible install progress reporting (on stderr) throughout the managed plugin install flow.
Changes:
- Special-cases missing
entire-graphduring plugin resolution to offer an interactive install (or a non-interactive install hint) and then runs the plugin with the original args. - Adds context-driven, stderr-only install progress stages (spinner on styled terminals; plain lines for non-TTY/accessibility).
- Adds documentation and test coverage for the on-demand install path and progress output, plus bumps
ultraviolet.
File summaries
| File | Description |
|---|---|
| go.mod | Bumps github.com/charmbracelet/ultraviolet indirect dependency. |
| go.sum | Updates checksums for the ultraviolet bump. |
| docs/architecture/external-commands.md | Documents the new “missing graph offers installation” rule and stderr progress behavior. |
| cmd/entire/cli/plugin.go | Enables the on-demand install flow when graph is missing and dispatches after install. |
| cmd/entire/cli/plugin_progress.go | Introduces context-carried progress writer + startPluginStep helper. |
| cmd/entire/cli/plugin_progress_test.go | Verifies immediate plain progress output and that stages land on stderr while stdout stays clean. |
| cmd/entire/cli/plugin_on_demand.go | Implements the interactive install prompt + managed-install handoff for missing plugins. |
| cmd/entire/cli/plugin_on_demand_test.go | Tests non-interactive hinting, prompt behavior, install/run sequencing, and arg forwarding. |
| cmd/entire/cli/plugin_install_remote.go | Adds progress stages around tag lookup, metadata fetch, and install step. |
| cmd/entire/cli/plugin_group.go | Wires progress reporting into remote installs and dependency planning. |
| cmd/entire/cli/plugin_fetch.go | Adds progress stages for locating assets, downloading, and checksum verification. |
| cmd/entire/cli/uiform/prompt_terminal_test.go | Ensures terminal prompt rendering clears answered confirmations (PTY-based test). |
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 5c185dd. Configure here.
Entire-Checkpoint: 01M231BF0DXEPSR8G9AWF7NCMC
Entire-Checkpoint: 01M23DNRDKQ7Z7SYTXZ4PBT5SB
Two defects, each with a regression test verified red without its fix. An `entire-graph` already in the managed directory but unreachable through PATH — a local-dev symlink whose target moved, or a managed bin dir that could not be prepended at startup — dead-ended: the user was prompted, accepted, and after three network round-trips got "already installed; use --force to replace", because the on-demand path passes no --force. The managed entry is now found before the prompt and executed, which is what installMissingPlugin's own return contract already promised. A confirmation rendered to the supplied writer even when that writer was not a terminal. `entire graph 2>log` therefore put zero bytes on the terminal while holding it in raw mode waiting for a keypress, and with Yes as the default an Enter authorized a download-and-exec nobody was shown. The opener now keeps the TTY's output handle instead of closing it, and renders there when the caller's writer is not itself a terminal. Also narrows the plugin signal exit. It fired whenever any signal had reached this process, which discarded the plugin's own exit code — Ctrl-C reaches the whole foreground process group, so a TUI plugin that handles it and exits 0 was reported as killed. It is now gated on the plugin's outcome via ExitPluginSignalled, which incidentally fixes os.Exit(-1) truncating to 255 for a signalled child. Smaller: the announcement names the command rather than only its arguments (a bare `entire graph` printed "Running plugin with command:" and stopped); `plugin upgrade` opts into the install progress its own startPluginStep calls were emitting nowhere; Browse cancellation follows its prompt to stderr; the graph name gets a constant; the progress docs move out of the managed-install -directory section; and the prompt-clearing assertion stops pinning the row count of a huh form. TestMaybeRunPlugin_MissingGraphNonInteractive gains plugin-dir isolation: consulting the managed directory before the prompt made it read the developer's real plugins, and pluginParentDir is not covered by the testdirs fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M25T41HTVXGY36EF3QXAF9YP
…uments
Two review findings on the on-demand install path.
FindInstalledPlugin reports what Lstat finds, so a local-dev symlink whose
target moved is listed like any other install. Short-circuiting to it
exec'd the missing target and failed with a fork/exec ENOENT naming a path
the user never chose — and the comment and docs claimed such an entry was
executed, which was true only when the target still existed. The entry is
now checked with os.Stat first; a missing target or a directory in its place
is reported with the entry's path and an 'entire plugin install <name>
--force' remedy. Reinstalling automatically is deliberately not done:
replacing a developer's symlink with a released binary is their call.
The remedy is offered only for the two conditions identified positively. A
stat failure that is not ENOENT gets the error and no advice — reinstalling
into the managed directory does not fix a permission failure on it, and a
remedy hung off an unmatched error is how advice comes to name the wrong
cause, as writeEntireDirRemedy documents for `.entire`.
The post-install announcement echoed every argument verbatim to stderr. They
are the user's own command line, already on screen, so they add nothing —
and they carry whatever they hold into stderr and anything capturing it. The
regression test drives a token, an ESC sequence and an embedded newline
through it; before the fix stderr read:
Running entire graph --token s3cr3t \x1b[1A\x1b[2Kforged line
break
The announcement is now the binary's name alone: "Running entire-graph".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M25WMATX7WCBP326ZXSRZGSY
A signalled child reports ExitPluginSignalled either way, so the previous
commit re-raised only when this process had also received a signal and exited
1 otherwise — losing the signal exactly when the plugin was the only one to
get it. Ctrl-C reaches the whole foreground process group, but `kill -TERM`
aimed at the plugin does not, and neither does a SIGPIPE from `entire graph |
head -1`. Both are outcomes kubectl-style dispatch has to pass through.
runPlugin now reads the signal off the child's wait status and MaybeRunPlugin
returns it, so main.go re-raises the child's own signal, preferring it over
one we received. `kill -TERM` inside a plugin, measured end to end:
PR head exit=255 (os.Exit(-1), truncated)
previous commit exit=1 (signal lost)
now exit=143 and bash reports "Terminated: 15"
pluginTerminatingSignal is a build-tagged pair rather than a runtime.GOOS
branch: syscall.WaitStatus on Windows is a bare struct with an ExitCode field
and no Signaled/Signal methods, so a GOOS branch would not compile. Windows
reports a killed child as an ordinary exit code, so it never reaches this path
at all; the remaining "-1 with no signal on either side" case exits 1 rather
than inventing a signal to raise.
MaybeRunPlugin's third result is what unparam flagged as unused, correctly:
nothing in the package checked that the signal survived the trip out of the
dispatcher, which is the half main.go depends on. Both levels are covered now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M25YSY75V465CHD9DXK30WG5
The prompt is the only human checkpoint before a remote binary is downloaded
and executed — an index-listed install never prompts inside runRemoteInstall,
because the catalog is the trust decision — and it named neither its catalog
nor its repository while defaulting to Yes. confirmInstallOrCancel names
redactURL(repoURL) for an unlisted repo and defaults to No, so the one
unprompted-command-to-exec path described itself least. It now resolves the
index entry first and asks:
Install the entire-graph plugin from https://github.com/entireio/entire-graph?
Resolving first also removes a prompt-then-fail: a name the index does not
carry cannot be installed at all, so it is now reported instead of offered,
the same shape as the already-installed dead end. Not an extra round-trip —
SyncPluginIndex touches a freshness marker and runRemoteInstall's own call
moments later reads the clone without fetching (pluginIndexTTL) — and the
progress step stops before the prompt.
The docs claimed "EOF declines" for both prompt modes. Measured, with an
input that EOFs immediately:
accessible returns at once, ok=false err=nil (declines)
default (tea) blocks to the deadline, "cancelled" (never answers)
Both fail closed, so this was doc accuracy rather than a hole, but only one
of them declines. The sentence now says which does what and why.
The on-demand tests gain a local file:// index, since the prompt text now
depends on a resolved entry: without it they would consult the real published
catalog over the network.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M25ZHG577Q1PD3S30XEV1P5Q
… repo
Two review findings, both on commits earlier in this branch.
The signal precedence was backwards. runPlugin's cmd.Cancel sends the child
SIGINT whatever Entire was sent, so when Entire is signalled the child's
signal is Entire's own signal laundered — and laundered lossily. Preferring
the child's therefore reported the wrong thing for exactly the case
dieFromSignal's doc comment calls out: "a SIGTERM (from a supervisor /
container stop) must exit 143, not masquerade as a SIGINT 130". Measured with
a plugin that ignores SIGINT and so outlives WaitDelay:
before supervisor SIGTERM -> parent dies of SIGKILL (137)
after supervisor SIGTERM -> parent dies of SIGTERM (143)
The child's signal is now used only when Entire received none, which is
precisely when nothing laundered it: `kill -TERM` at the plugin still exits
143, SIGPIPE from a closed pipe still exits 141.
Naming the repository in the prompt introduced a TOCTOU on the trust
decision it exists to make. runRemoteInstall re-resolved the name through a
second SyncPluginIndex, and the two reads can disagree: a refresh that fails
leaves the freshness marker untouched, so the next call retries the fetch and
may succeed with different content, and a concurrent `plugin index update
--force` rewrites the clone under the lock either way. The prompt could name
repository A while repository B was downloaded and executed. installSource
gains a Resolved entry that travels with the request, so nothing is
re-resolved after the user has agreed. An idxErr stops being fatal on that
path, since the caller has already done the lookup and the index is needed
afterwards only for dependency planning, which already degrades to a warning.
TestExternalCommand_ParentsSignalOutranksTheChilds covers the precedence
against the real binary — main.go had no automated coverage for this at all —
and fails with "parent died of killed, want SIGTERM" against the old order.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M261WZ3GJH3BBPSSY3TJ0TNX
Soph
marked this pull request as ready for review
September 10, 2026 17:11
gtrrz-victor
previously approved these changes
Sep 11, 2026
Entire-Checkpoint: 01M27VYXMNDNPV2B2JGFW182H2
gtrrz-victor
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/1267
Summary
Install the Graph external command on demand when a user runs
entire graphandentire-graphis missing.graphthrough the configured plugin index and show the repository URL in a Yes-default confirmation.entire plugin install graphhint instead of prompting.PATH; diagnose dangling links or directory entries with an actionable--forcereinstall command.Install experience
Remote plugin installs now report index lookup, release discovery, metadata fetch, download and verification, placement, and dependency-planning stages on stderr. Styled terminals get a spinner; accessibility mode and non-terminal writers get plain status lines. Stdout remains available for plugin output and JSON pipelines.
Confirmations read from the controlling terminal rather than stdin, preserving piped plugin input. If stderr is redirected, the prompt is rendered on the terminal where the answer is read. Cancellation is context-aware, and accessible-mode EOF fails closed.
The post-install handoff announces only
Running entire-graph; user arguments are never echoed to stderr.Reliability
Validation
go test ./cmd/entire/cli ./cmd/entire/cli/uiform ./cmd/entire