Skip to content

Add remote plugin install, index discovery, and plugin dependencies - #1422

Open
ashtom wants to merge 37 commits into
mainfrom
ashtom/plugin-index
Open

Add remote plugin install, index discovery, and plugin dependencies#1422
ashtom wants to merge 37 commits into
mainfrom
ashtom/plugin-index

Conversation

@ashtom

@ashtom ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member

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

Summary

Expands the kubectl-style external-command layer so users can discover and install plugins without cloning anything, from any git host. 18 files, ~5,540 insertions.

  • Remote installentire plugin install <name|url|path>. Newest stable semver tag via git ls-remote (forge-agnostic, inherits git auth and proxies; no forge REST client anywhere). Optional entire-plugin.yml metadata read via blobless shallow clone. Release assets download over HTTPS through a small per-host URL convention table (GitHub/Gitea-style, GitLab-style, download_url template escape hatch), selected and verified against the release's checksums.txt. Binaries land in pkg/<name>/ with a provenance manifest.yml; bin/ stays the only dispatch surface and the resolver in plugin.go is untouched.
  • Upgradeentire plugin upgrade [name|--all], --pin to hold a tag, next-highest-tag fallback for pushed-but-unpublished releases, Windows locked-binary rename-aside.
  • Discovery — krew-style git-synced index (entireio/plugin-index): shallow-cloned into the user cache keyed by URL hash, flock-guarded, 24h TTL, stale-copy-on-offline. Six new subcommands (upgrade, search, info, browse, doctor, index update) join the pre-existing install/list/remove; bare-name installs resolve through the index. Index URL: --index > ENTIRE_PLUGIN_INDEX_URL > built-in default.
  • Dependenciesentire-plugin.yml requires (name + optional min_version, no ranges), resolved by name through the index. Install-time transitive planning, apt-style single confirmation, remove guard for depended-on plugins, and entire plugin doctor. Dispatch stays zero-cost — no runtime dependency checks.

Security model

This code ends in downloading a binary and making it executable, so the trust boundaries are the design:

  • Transport. HTTPS required for anything off-machine, re-validated on every redirect hop (Go follows redirects across schemes; validating only the entry point is bypassable). Loopback exempt so httptest fixtures work.
  • Authentication. A release that publishes no checksums.txt for the platform is refused by default; --allow-unverified opts in, is recorded in the manifest, reported by doctor, and inherited by upgrade.
  • Integrity. The installed binary's digest is recorded as binary_sha256 and re-checked by doctor. (sha256 covers the downloaded asset, which is discarded with the staging dir — provenance only.)
  • URLs reaching git. Every repo/index URL is scheme-validated by one definition and every git invocation passes -- before positionals. Repo URLs are attacker-influenced (index entries, --index, env), and git reads an option-shaped positional as an option — --upload-pack's value is shell-interpreted.
  • The trust root is not repo-choosable. The index URL cannot be set from committed settings; see below.
  • Names. The installed name comes from the remote, so callers that already committed to a name pass it as a required argument and a mismatch is fatal.
  • Credentials. Userinfo and query strings are stripped from every URL before printing, logging, or persisting (.entire/logs/ is inside the working tree and doctor bundle collects it wholesale; manifest.yml is mode 0644).

Not claimed: authenticity. checksums.txt shares an origin with the asset, so it proves integrity in transit, not publisher identity. Real authenticity needs signatures.

Design notes

  • Built on the recorded founding decisions of this layer: PATH dispatch unchanged, built-ins win, agent- prefix reserved, env filtering untouched, no new telemetry.
  • The only forge-specific code is the download-URL convention table; version listing, metadata, and the index all ride on the git protocol. No go-github-class dependency (uses existing gopkg.in/yaml.v3 + golang.org/x/mod).
  • The plugin layer reads no settings at all. The settings package is byte-identical to main.

Changed since this description was first written

The branch was reviewed (Bugbot ×5 rounds, Copilot, four trail agent reviews, and a three-agent adversarial pass), then merged with main (2053 commits). Reviewers should know these are new:

Behavior changes

  1. Checksums are now required by default (--allow-unverified opts out). Previously a release without them installed silently.
  2. http:// and git:// are no longer accepted for repo or index URLs — unauthenticated transports, and the catalog decides what installs without a prompt.
  3. Prereleases are excluded from tag resolution. v2.0.0-rc1 outranks stable v1.9.0 in semver, so upgrade --all was migrating users onto release candidates.

Removals
4. plugins.index_url and plugins.index_ttl_hours are gone. .entire/settings.json is committed and resolved from the working directory, while an index-listed plugin installs with no prompt — so a cloned repo could redirect the catalog and get an attacker-chosen binary onto PATH silently. The setting's value was that contributors got a different catalog without knowing, which is the vulnerability stated as a feature. Internal catalogs now use ENTIRE_PLUGIN_INDEX_URL, which applies across repos and cannot be chosen by checked-out content. Go takes the same position with GOPROXY; npm's per-project registry is the cautionary counter-example.
5. requires[].repo_url is gone. Dependencies resolve by name through the index, so a dependency's URL comes from the curated catalog rather than the requiring plugin's author — planning contacted that URL before the confirmation prompt. A user can still install an out-of-catalog dependency themselves, with the usual prompt; the authority moved to the user.

Fixes worth naming
6. Argument injection into the git CLI allowing arbitrary command execution from a malicious index entry or dependency URL — verified live on the pre-fix tree, on a path that never prompts.
7. entire-plugin.yml now decodes leniently. Strict decoding meant the first author to adopt any future field would break installs on every older CLI, permanently. This is also what made removals 4 and 5 migration-free for already-published files.
8. Silent truncation of archive entries over 512 MiB, index-cache races (now flock-guarded), a diamond min_version skip, darwin_all universal binaries never matching, empty entire-plugin.yml being fatal, index_ttl_hours overflowing to "always stale", and credential leaks through download errors.

Testing

  • 77 unit tests against file:// git repos and httptest asset servers (zero real network in CI), covering tag resolution, asset selection/verification, extraction guards, index sync/TTL/offline/concurrency, dependency planning, doctor.
  • 4 integration tests drive the spawned binary end to end (install/dispatch, unlisted-URL gating, dependencies + remove guard, search/info): bare-name install → real dispatch → dependency install → remove guard → doctor exit code. They run the verified path — no test passes --allow-unverified, so their passing is the proof that verification succeeds.
  • Security regressions assert the stronger property where it matters: that an injected payload never executes and that credentials never appear, not merely that a call errors. Each was verified RED against the pre-fix tree.
  • mise run check green (fmt, lint, unit + integration + Vogon canary).

Docs

  • docs/architecture/external-commands.md — remote install, plugin index, dependencies, and the trust-boundary sections.
  • README — user-facing Plugins section.

Open, deliberately

  • browse and index.json's platforms are candidates to cut — browse adds no capability over search + install and installs unprompted; platforms warns but never enforces.
  • min_version is not enforced on the install outcome (the planner computes a constraint nothing downstream checks), and dependencies have no auto_installed marker, so orphans leak.
  • Two notions of "official": the officialPlugins telemetry allowlist and index.json's official flag.
  • entire upgrade (the self-update plugin) vs entire plugin upgrade (plugins).
  • Follow-up outside this branch: a credential-redaction pass at main.go's error print. %w-wrapped url.Error leaks the full URL regardless of call-site discipline, because net/url.Error.Error() does no redaction.
  • Outside this repo: graph should be added to entireio/plugin-index, or entire plugin install brain reports its dependency as unindexed.

🤖 Generated with Claude Code

ashtom and others added 3 commits June 12, 2026 13:36
Typed PluginSettings sub-struct on EntireSettings, whole-object merge in
the local-override path (parallel to investigate), and post-merge
validation. Settings configure discovery only; plugin state (installed
versions, pins) lives in the managed dir's manifests. Index URL
precedence is resolved in the cli package: --index flag >
ENTIRE_PLUGIN_INDEX_URL > settings > built-in default.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: b1d40d91c483
Expands the kubectl-style external-command layer so users can discover
and install plugins without cloning anything, on any git host:

- Remote install (entire plugin install <url>|<name>): newest semver tag
  via git ls-remote (forge-agnostic, inherits git auth), optional
  entire-plugin.yml metadata via blobless shallow clone, release asset
  download through a per-host URL convention table (GitHub/Gitea-style,
  GitLab-style, download_url template escape hatch), checksums.txt
  verification, tar.gz/zip/raw extraction with traversal guards, and a
  pkg/<name>/manifest.yml provenance record. bin/ stays the only
  dispatch surface; the resolver in plugin.go is untouched.
- Upgrade (entire plugin upgrade [name|--all]) with --pin skip and a
  next-highest-tag fallback for pushed-but-unpublished releases.
- Discovery: krew-style git-synced index (index.json in a git repo,
  shallow-cloned into the user cache keyed by URL hash, TTL refresh,
  stale-on-offline). New search/info/browse/index update subcommands;
  bare-name installs resolve through the index; non-index URLs require
  TTY confirmation or --yes.
- Dependencies: entire-plugin.yml requires (name, repo_url,
  min_version), install-time transitive planning with cycle bounds,
  apt-style confirmation, a remove guard for depended-on plugins, and
  entire plugin doctor (missing/outdated deps, manifest drift, dangling
  symlinks, macOS quarantine).

Unit tests run against file:// repos and httptest asset servers;
integration tests exercise the spawned binary end to end including
dispatch of an installed plugin. No forge REST API anywhere — version
listing, metadata, and the index all ride on the git protocol.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 012eb5e80390
Plugins section covering install sources, discovery via the plugin
index, dependencies, and the corporate index_url override; plugin row
in the commands reference.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: aa8b54a63324
Copilot AI review requested due to automatic review settings June 12, 2026 05:21
Comment thread cmd/entire/cli/plugin_group.go Outdated
Comment thread cmd/entire/cli/plugin_group.go Outdated
Comment thread cmd/entire/cli/plugin_group.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Expands the existing kubectl-style external-command layer to support a full plugin lifecycle: remote installs/upgrades (via git tag resolution + release asset download), discovery via a git-synced plugin index, and install-time dependency planning with a plugin doctor health check.

Changes:

  • Add remote plugin install/upgrade plumbing (git ls-remote tag resolution, optional entire-plugin.yml metadata fetch, release asset download + extraction, provenance manifests).
  • Add plugin index sync/search/info/browse/index-update flows with settings/env/flag precedence and TTL-based refresh.
  • Add dependency planning/execution + remove guard + plugin doctor, plus docs/README updates and new unit/integration tests.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
README.md Adds user-facing “Plugins” section and command examples.
docs/architecture/external-commands.md Documents remote install, index discovery, and dependency semantics.
cmd/entire/cli/settings/settings.go Adds repo-level plugin discovery settings (index URL + TTL) and validation/merge support.
cmd/entire/cli/settings/settings_plugins_test.go Unit tests for plugin settings validation/TTL and merge semantics.
cmd/entire/cli/plugin_manifest.go Introduces managed-install manifest + author metadata parsing (entire-plugin.yml).
cmd/entire/cli/plugin_manifest_test.go Tests manifest I/O and strict metadata parsing behavior.
cmd/entire/cli/plugin_install_remote.go Implements remote install orchestration, manifest writing, and upgrade path.
cmd/entire/cli/plugin_install_remote_test.go Unit tests for remote install, pinning, fallback, upgrade, and removal cleanup.
cmd/entire/cli/plugin_index.go Implements git-synced plugin index cache with TTL refresh and filtering.
cmd/entire/cli/plugin_index_test.go Tests index sync/refresh/offline behavior and install-arg classification.
cmd/entire/cli/plugin_group.go Adds Cobra commands/flags for install/upgrade/search/info/browse/doctor/index and dependency prompts.
cmd/entire/cli/plugin_gitremote.go Adds git-shellout helpers for semver tag listing and metadata fetch-at-tag.
cmd/entire/cli/plugin_gitremote_test.go Unit tests for tag sorting, metadata fetch, and repo-url name derivation.
cmd/entire/cli/plugin_fetch.go Adds release asset URL conventions, checksum handling, downloading, and archive extraction.
cmd/entire/cli/plugin_fetch_test.go Unit tests for URL conventions, checksum selection, extraction guards, and download verification.
cmd/entire/cli/plugin_deps.go Adds dependency planning/execution, remove guard helpers, and plugin doctor checks.
cmd/entire/cli/plugin_deps_test.go Unit tests for dependency planning, cycles, dependents, and doctor reporting.
cmd/entire/cli/login.go Minor constant usage change for GOOS comparison.
cmd/entire/cli/integration_test/plugin_remote_install_test.go End-to-end integration coverage for index install, URL confirmation gating, deps/remove-guard, and doctor exit behavior.
cmd/entire/cli/explain.go Introduces darwinGOOS constant for reuse.

Comment thread cmd/entire/cli/plugin_fetch.go Outdated
Comment thread cmd/entire/cli/plugin_index_test.go
- huh prompts use RunWithContext and map Ctrl+C/Esc through
  handleFormCancellation instead of conflating abort with decline;
  dependency-confirm failures are no longer reported as a skip the user
  chose. Non-interactive-without---yes gets a dedicated sentinel so the
  untrusted-install path fails while the post-install dependency path
  degrades to an informed skip.
- Context cancellation during sync/install/upgrade maps to SilentError
  per the clean.go/activity_cmd.go convention (silencePluginCancel,
  including the ctx.Err() check for killed git children).
- fetchAndVerify rejects asset names that could escape the staging dir
  (both separator kinds, dot segments) and removes partial downloads on
  checksum mismatch, oversize, or write failure.
- Index offline test uses os.RemoveAll instead of shelling to rm -rf
  (Windows portability).
- Strict-decode metadata test uses a non-near-miss unknown key; the
  previous deliberately misspelled field was silently corrected by a
  spell-fixing formatter pass, making the test vacuous.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 9770a0e2d1f8
@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Addressed all five review findings in cd5984b:

  • Huh forms skip RunWithContext (Bugbot): confirmPluginAction and plugin browse now use form.RunWithContext(ctx) and route errors through handleFormCancellation, matching the doctor.go convention.
  • Confirm abort treated as skip deps (Bugbot): a prompt error is no longer reported as a user-chosen skip. Ctrl+C/Esc prints "Dependency install cancelled." (clean exit, main install stands); real prompt failures propagate as errors. Non-interactive runs without --yes get a dedicated sentinel — fatal for untrusted-URL installs, informed skip for post-install dependency confirmation.
  • Install path ignores context cancel (Bugbot): new silencePluginCancel maps cancellation to NewSilentError per the clean.go/activity_cmd.go convention, applied across install/upgrade/search/info/browse/index-update. It also checks ctx.Err() directly because a killed git child surfaces as "signal: killed" rather than context.Canceled.
  • Asset path traversal + partial files (Copilot): fetchAndVerify now rejects asset names containing either separator kind or dot segments before joining into the staging dir (today's callers only pass internally-generated candidates, but the boundary is now enforced where it belongs), and removes the partial download on checksum mismatch, oversize, or write failure.
  • rm -rf in test (Copilot): replaced with os.RemoveAll.

New unit tests cover the unsafe-name rejection and mismatch cleanup. mise run check (fmt + lint + unit + integration + canary) is green.

🤖 Generated with Claude Code

@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

bugbot run

Comment thread cmd/entire/cli/plugin_deps.go
Comment thread cmd/entire/cli/plugin_index.go Outdated
- planDeps walks a satisfied managed dependency's recorded manifest
  requirements, so installing a parent repairs gaps deeper in the chain
  (e.g. a grandchild removed with --force since install). Offline —
  reads the manifest, no extra network during planning.
- SyncPluginIndex sweeps a partial cache dir (no .git) before the
  initial clone; previously an interrupted first clone wedged discovery
  until the cache was cleared by hand, since git refuses to clone into
  a non-empty directory.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: df589ff10cf4
@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Addressed both round-two findings in 6ee0a3b:

  • Skipped transitive dependency planning: planDeps now walks a satisfied managed dependency's recorded manifest requirements, so installing a parent repairs gaps deeper in the chain (e.g. a grandchild removed with --force after install). Planning stays offline — it reads the local manifest, no extra network. PATH/local-dev-satisfied deps have no manifest to walk; those remain plugin doctor's domain. Regression test: satisfied parent with a missing grandchild produces exactly the grandchild action.
  • Failed index clone blocks retries: the un-cloned branch of SyncPluginIndex sweeps the cache directory before cloning, so a partial dir left by an interrupted first clone (no .git) can't wedge discovery behind git's non-empty-target refusal. Regression test: sync recovers from a junk-filled cache dir.

mise run check green.

🤖 Generated with Claude Code

@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

bugbot run

Comment thread cmd/entire/cli/plugin_group.go Outdated
Comment thread cmd/entire/cli/plugin_group.go Outdated
- Declining the untrusted-install prompt now prints "Install
  cancelled." and exits 0, matching the Esc/Ctrl+C path and the
  handleFormCancellation convention used by every other confirm.
  Exit codes no longer differ by how the user said no; automation
  never reaches this prompt (non-interactive fails earlier with the
  --yes hint).
- classifyInstallArg no longer stats bare arguments: names always
  resolve through the index, local paths must be explicit (./ or a
  separator), git-style. A stray CWD file sharing a plugin's name can
  no longer shadow the index; the index-miss error hints at
  'install ./<name>' when a matching local file exists.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 3bf711f1f5a4
@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Addressed both round-three findings in 117b23b:

  • Install abort yields success exit: declining the untrusted-install prompt now prints "Install cancelled." and exits 0, identical to Esc/Ctrl+C — harmonized toward the handleFormCancellation convention every other confirm in the codebase uses. The automation concern can't arise in practice: non-interactive runs fail before the prompt with the --yes hint, so only interactive consistency was at stake.
  • Local file shadows index name: classifyInstallArg no longer stats bare arguments. Names always resolve through the index; local paths must be explicit (./entire-foo or any separator-containing path) — git-style disambiguation, and the two spaces are disjoint since plugin names can't contain separators. When an index lookup misses but a same-named local file exists, the error hints at entire plugin install ./<name>. Command help documents the rule.

mise run check green.

🤖 Generated with Claude Code

@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

bugbot run

Comment thread cmd/entire/cli/plugin_install_remote.go
UpgradeInstalledPlugin compared raw tag strings, so equivalent
spellings (v0.2.0 vs 0.2.0) triggered spurious reinstalls, and an
asset-less newest tag produced a misleading X → X upgrade line after
the install fell back to the already-installed version. Both
comparisons now go through semver.Compare on canonicalized tags: no
download when the newest tag is not strictly newer, and a fallback
that lands on the installed version reports up-to-date.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: db4754bb3db1
@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Addressed the round-four finding in 813670a:

  • Upgrade compares raw tag strings: both comparisons in UpgradeInstalledPlugin now use semver.Compare on canonicalized tags. Equivalent spellings (v0.2.0 vs 0.2.0) no longer reinstall — the check short-circuits before any download — and when an asset-less newest tag makes the install fall back to the already-installed version, the outcome reports up-to-date instead of an X → X upgrade line. Regression tests cover both (the spelling test runs against a repo with no asset server at all, so any download attempt would fail the test).

Known residual: a genuinely newer tag with an unpublished release still causes a re-download of the installed version on each upgrade run — avoiding that would require persisting known-asset-less-tag state, which doesn't seem worth it. Output is correct either way.

mise run check green.

🤖 Generated with Claude Code

@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 3 potential issues.

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 813670a. Configure here.

Comment thread cmd/entire/cli/plugin_fetch.go Outdated
Comment thread cmd/entire/cli/plugin_deps.go
Comment thread cmd/entire/cli/plugin_deps.go
- A checksum manifest that lists no asset for the current platform no
  longer aborts the download: selection continues through the other
  manifest candidates and the direct-probe fallback, so a stale or
  hand-written root checksums.txt can't mask an installable release.
  Verification is not weakened — an attacker controlling the manifest
  could list a malicious digest directly.
- Dependency planning warns when a scheduled dependency's tags or
  metadata can't be inspected, instead of silently omitting its nested
  requirements from the confirmed plan.
- Document why doctor's quarantine probe on the bin entry is
  sufficient: macOS xattr follows symlinks by default, so the check
  (and the suggested xattr -d fix) already operates on the pkg/
  target. Verified empirically; the round-five finding claiming
  otherwise was a false positive.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: a11bfa5352d7
@ashtom

ashtom commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Round five: two fixed, one rebutted, in 6da333c.

  • Checksum miss aborts other manifests (fixed): a manifest listing no asset for the platform now falls through to the remaining checksumCandidates names and the direct-probe fallback instead of aborting, so a stale or hand-written root checksums.txt can't mask an installable release. No verification weakening — an attacker who controls the manifest could list a malicious digest directly. Regression test: stale root manifest + published conventional asset installs via the probe.
  • Planning skips failed transitive fetch (fixed): when a scheduled dependency's tags or metadata can't be inspected, the plan now carries an explicit warning naming the plugin and pointing at entire plugin doctor, instead of silently omitting nested requirements. Regression test: untagged dependency repo plans the action plus the warning.
  • Quarantine check ignores symlink target (not a bug): macOS xattr follows symlinks by default — -s is the flag to act on the link itself. Verified empirically: xattr -p com.apple.quarantine <bin-symlink> returns the attribute set on the pkg/ target, exit 0. Doctor's existing probe on the bin entry therefore already covers the real binary, and the suggested xattr -d fix also operates on the target. Added a code comment documenting this so it doesn't resurface.

mise run check green. Findings across rounds: 3 → 2 → 2 → 1 → 2+1 false positive — at this point the remaining surface looks like speculative edge cases, so leaving further review to humans unless something specific comes up.

🤖 Generated with Claude Code

Soph and others added 4 commits August 4, 2026 20:34
Entire-Checkpoint: 01KZ70X4NPK7Y5KP254BR17S2Q
main's "lint build-tagged files" change brought the integration_test
package under golangci-lint, which this file predates:

- goconst: use the package-level windowsGOOS constant instead of four
  literal "windows" comparisons.
- unparam: pluginTestEnv's second return (the plugin dir) was discarded
  at every call site; drop it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ73R4DNYE7J4QDA5ZBQ1PX1
A fully-specified download_url (no {asset} placeholder) took its staging
filename from path.Base of the whole URL. path.Base only splits on "/",
so a query string folded into the name: a signed or proxied URL like
.../entire-run.tar.gz?token=abc yielded "entire-run.tar.gz?token=abc",
which then missed extractPluginBinary's archive-extension sniff and was
written out verbatim as the binary — a silently broken install. On
Windows the "?" also makes the staging file uncreatable.

assetNameFromURL parses the URL and uses the path component, falling
back to a query/fragment trim when the URL won't parse.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ73R7MYNJ9A4V7FDVTKE00D
Repo URLs are attacker-influenced: they arrive from index.json entries,
from another plugin's entire-plugin.yml requires[].repo_url, and from
--index / ENTIRE_PLUGIN_INDEX_URL. They were passed to git ls-remote and
git clone as bare positionals with no validation.

git parses an option-shaped positional as an option, and --upload-pack's
value is shell-interpreted. With no positional repository left, git falls
back to the ambient repo's origin and runs the payload, so an index entry
of "--upload-pack=curl ... | sh; git-upload-pack" executed arbitrary
commands during 'entire plugin install <name>' — a path that is treated
as trusted and never prompts. Dependency planning was tighter still: it
inspects requires[].repo_url before the install confirmation prompt.

Two independent layers, either of which is sufficient:

- validatePluginRepoURL gates every URL to https/http/ssh/git/file
  schemes, git's user@host:path scp-like form, or an absolute local path.
  This also rejects git's command-executing ext:: transport, which a "--"
  separator alone would not stop (current git blocks ext:: by default,
  but the allowlist doesn't rely on that policy).
- Every git invocation now passes "--" before its positionals.

Validation is applied at the sinks (listRemoteSemverTags,
fetchPluginMetadataAtTag, SyncPluginIndex) so all callers are covered,
and at the parse boundaries for better errors: bad index entries are
dropped like invalid names, so one hostile row can't take out the
catalog, while an invalid requires[].repo_url fails metadata parsing and
surfaces the author's mistake at install time.

This mirrors validatePluginName, which already refuses a leading '-' on
names for exactly this reason.

The regression test asserts the stronger property — that the payload
never executes, not merely that the call errors. Verified RED against
the pre-fix tree, where it reports the marker file being created.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ73RNANM74SEZW1S132RYS9
Soph and others added 2 commits August 4, 2026 22:01
loadPluginIndexFromDir rejected any index whose version wasn't exactly 1.
That guarded a migration that can never happen: the index is one shared
resource read by every CLI version ever shipped, so bumping it would break
discovery fleet-wide with no gradual rollout and no undo for binaries
already installed. An incompatible schema ships at a new path
(index-v2.json, another branch, another repo), which needs no gate here.

Since the version would never be bumped deliberately, enforcement only
ever fired by accident — most painfully on an index that simply omits the
field, which decodes to 0 and produced "declares unsupported version 0
(this CLI understands 1; upgrade entire?)", telling the author to upgrade
the CLI when the fix was in their own file. Hand-written internal
catalogs are exactly what the repo-level plugins.index_url setting is
for.

The changes that actually happen were already absorbed without it:
decoding ignores unknown fields, so new fields are free, and unreadable
entries are dropped individually. Degrading per entry beats refusing the
catalog for a discovery feature.

A declared version above what this CLI reads now logs a warning and the
catalog still loads. The field stays in the schema, so branching on it
later remains possible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ75W2KK5YTZPCRZ9P1KV4QZ
Three gaps in the remote-install supply chain, all on the path that ends in
making a downloaded file executable.

HTTPS is now required. Plaintext HTTP was accepted, and because the asset
and the checksums.txt authenticating it come from the same origin, an
attacker who could rewrite one could rewrite both — checksum verification
proved nothing. Enforced at the HTTP boundary (httpGetSmall and
fetchAndVerify) so it also covers the author-declared download_url escape
hatch, not just the derived forge URLs. Loopback is exempt: httptest
fixtures and local forge experiments have no network attacker to defend
against.

Unauthenticated downloads are refused by default. Previously a release
publishing no checksums.txt silently fell through to probing candidate
names and installing whatever arrived. Now that needs --allow-unverified,
which is recorded in the manifest, reported by doctor until the author
publishes checksums, and inherited by upgrade — so a knowingly-unverified
plugin doesn't start failing on upgrade, and a verified one can't silently
become unverified. The same gate covers a download_url with no {asset}
placeholder, which is unverifiable by construction.

The candidate probe still runs when verification is required, because it
separates "no release published for this tag yet" (errAssetNotFound, worth
walking down to the next tag) from "release exists but publishes no
checksums" (errUnverifiedAsset, which an older tag would not fix).
Collapsing those would report a missing release for a plugin that simply
ships no checksums.

Installed binaries are now digested and re-checked. manifest.SHA256 covers
the downloaded asset — usually an archive, discarded with the staging dir —
so it recorded provenance but could never detect tampering of the thing
actually executed, which is why doctor had no integrity check. Installs now
also record BinarySHA256 for the binary under pkg/<name>/, and doctor
re-hashes it to catch a binary modified or replaced outside entire. A
manifest without the field predates this and is skipped rather than nagged
about.

The unit and integration release servers now publish checksums.txt, so the
end-to-end tests exercise the verified path that production uses rather
than the refusal path.

Also collapses three copies of the entire-<name>[.exe] construction into
pluginBinaryName, which the .exe literal count surfaced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ8DFPNRF5TZ1J4BJP2SAT1T
@Soph
Soph marked this pull request as ready for review August 5, 2026 07:40
@Soph
Soph requested a review from a team as a code owner August 5, 2026 07:40
@Soph

Soph commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Un-drafted after a review pass and a merge with main

This sat for ~7.5 weeks, so I reviewed the implementation, merged current main into it, and fixed what the review turned up. The Summary and Testing sections above describe the pre-merge state and are now stale — this comment is the current picture.

main moved 2053 commits in the meantime. Both merge conflicts were "both sides added a line in the same place": Plugins next to main's new Checkpoints field in EntireSettings, and the entire plugin README row next to main's new org/project/repo/grant rows.

⚠️ Please read ac0d13e29 closely — argument injection into the git CLI

Repo URLs were passed to git ls-remote and git clone as bare positionals with no validation. They are attacker-influenced: they arrive from index.json entries, from another plugin's requires[].repo_url, and from --index / ENTIRE_PLUGIN_INDEX_URL.

git parses an option-shaped positional as an option, and --upload-pack's value is shell-interpreted. With no positional repository left, git falls back to the ambient repo's origin and runs the payload. So an index entry of --upload-pack=curl … | sh; git-upload-pack executed arbitrary commands during entire plugin install <name> — a path that is treated as trusted and never prompts. Dependency planning was tighter still: it inspects requires[].repo_url before the install confirmation.

I confirmed this was live on the pre-fix tree before fixing it — the regression test asserts the payload never executes, not merely that the call errors, and it reports the marker file being created when run against the old code.

Two independent layers now, either sufficient alone: a scheme allowlist (validatePluginRepoURL) applied at the sinks and at the parse boundaries, plus -- before every git positional. This mirrors validatePluginName, which already refused a leading - on names for exactly this reason.

Behavior change worth a decision: checksums now required by default (26b10db99)

A release publishing no checksums.txt previously fell through to probing candidate names and installing whatever arrived. That now needs --allow-unverified, recorded in the manifest, reported by doctor, and inherited by upgrade. Plaintext HTTP is also refused for non-loopback hosts, because the asset and the checksums.txt authenticating it share an origin — an attacker who can rewrite one can rewrite both.

This is the change most likely to want discussion. It's easy to soften to warn-only, or to require checksums only for index-listed plugins. Flagging it rather than assuming.

Also in that commit: manifest.SHA256 covers the downloaded asset, which is usually an archive discarded with the staging dir — so it recorded provenance but could never detect tampering of the binary actually executed, which is why doctor had no integrity check. Installs now also record BinarySHA256 for the binary under pkg/<name>/, and doctor re-hashes it.

Other commits

  • 0636c621b — the index version != 1 gate is now advisory. It guarded a migration that can never happen (the index is one shared resource read by every shipped CLI, so a bump breaks discovery fleet-wide), while punishing the case that does: an index omitting the field decoded to 0 and told the author to upgrade the CLI. Hand-written internal catalogs are exactly what the repo-level plugins.index_url setting is for.
  • 56a063e4b — a fixed download_url carrying a query string yielded entire-run.tar.gz?token=abc from path.Base, missing the archive-extension sniff and getting written out verbatim as the binary.
  • 2ac8323f9 — two lint errors from main's bf0c56b1c, which brought build-tagged files under golangci-lint after this branch was written.

Testing

mise run check green locally; all GitHub checks green here, including test-windows and both test-canary variants, which did not exist when this branch was last touched. Both release test servers now publish checksums.txt, so the end-to-end tests exercise the verified path production uses — no test passes --allow-unverified, so their passing is the proof that verification succeeds.

Still open (deliberately not addressed)

  • Dependency upgrades install the newest tag without confirming min_version was actually reached; only doctor notices.
  • Two sources of truth for "official": main's officialPlugins telemetry allowlist (now holding "ci") vs the index's official flag.
  • entire upgrade (CLI self-update, special-cased in plugin.go since main's 5e12b0cac) vs entire plugin upgrade (plugins) — collision-free mechanically, confusable in docs.
  • The index should probably now list ci and upgrade, both of which shipped while this was open.

🤖 Generated with Claude Code

Soph and others added 23 commits August 5, 2026 12:19
Two medium findings from the agent review, both verified against the code
before fixing and both covered by tests that fail without the fix.

Silent truncation in archive extraction. extractFromTarGz/extractFromZip
capped the copy with io.LimitReader(entry, maxPluginAssetSize) and never
checked whether the entry had actually exceeded it. io.Copy through a bare
LimitReader returns a nil error at the limit, so an archive entry larger
than 512 MiB was truncated to exactly the cap and written out as the plugin
binary with the install reporting success. Reachable: the download cap
bounds the compressed archive, not what it expands to, so a few-KB
tar.gz of compressible data clears it.

The interaction with binary_sha256 made this worse than it looks — the
digest is computed from the bytes on disk, so doctor would have confirmed
the truncated binary as intact.

The cap now lives in writeExecutable, which reads one byte past it and
errors on overflow, mirroring fetchAndVerify's existing +1 pattern. Callers
hand over the raw reader rather than pre-limiting it, so all three
extraction paths (tar.gz, zip, raw binary) inherit the check.
writeExecutableLimited takes the bound explicitly so the oversize path is
testable without materializing 512 MiB.

No locking on the shared plugin-index cache. SyncPluginIndex mutated the
per-URL cache dir destructively — RemoveAll + clone cold, fetch + reset
--hard warm — with no lock, unlike discovery/cache.go which flock-guards
its shared cache files. Concurrent `entire plugin` invocations could race:
one cloning while another reads index.json surfaces a spurious "no readable
index.json", and two cold syncs racing leave exactly the half-created
directory the existing sweep was added to recover from.

The whole sync-and-read now runs under flock.AcquireContext, which honors
ctx so an interrupted wait still cancels. The lock file is a sibling of the
cache dir rather than inside it, since RemoveAll on the dir would otherwise
delete the lock while it is held. Six concurrent cold syncs fail without
this ("destination path already exists and is not an empty directory") and
pass with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ8PZKR5P4TGDGNQ4NZ060YT
planDeps kept a name-only visited set and marked a dependency handled before
evaluating the current requirement's min_version. In a diamond where two
requirers demand different minimums of the same plugin — A needs sem >= v1.0.0,
B needs sem >= v2.0.0, with sem installed at v1.5.0 — the first, satisfied
requirement closed the name off and B's stricter one was skipped entirely: no
action, no warning, install completes with B running against a too-old sem.

Verified before fixing: the plan came back with zero actions and zero
warnings. The originating finding also claimed doctor would never flag it;
that part is wrong — RunPluginDoctor walks each manifest's requirements with
no visited set and does report "requires sem >= v2.0.0 but v1.5.0 is
installed". So the gap was late discovery rather than silence, which is why
this is a correctness fix and not an escalation: the install should plan the
upgrade instead of deferring to doctor.

Planning now records the strictest min_version considered per name and
re-evaluates only when a later requirer demands more. upsertDepAction keeps
one action per plugin so the reverse order (strict first) doesn't double-plan,
and addDepWarning dedupes observations that a re-visit can re-derive.
Termination still rests on maxDepDepth, which already existed as the backstop
for metadata cycles; the cycle-breaking test is unchanged and still passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ8S211MVJ04494Y2ME739VM
DepAction.FromIndex recorded whether a dependency's repo URL came from the
catalog, but nothing in production ever read it — a tracked trust signal that
was never wired up. So a dependency whose repo_url is declared by the
requiring plugin's author reached the git CLI and downloaded a binary behind
only the generic apt-style "Install them now?", while the direct path
(`entire plugin install <url>`) prompts explicitly for a URL the index doesn't
list. Same class of attacker-influenced input, less visibility.

The apt-style single confirmation stays — batching is the point. What changes
is that it is now informed: unlisted actions are marked "← not in the plugin
index" in the listing, upgrades show the repo they will reinstall from, and a
warning names how many resolve outside the catalog.

FromIndex is now derived from the resolved URL for every branch rather than
only the two that consulted the index. Previously an upgrade action never set
it, so it would have reported as unlisted no matter where the installed plugin
came from. A nil index — the offline case during a URL install — makes
everything unlisted, which is the conservative answer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ8SXB81TV8ZCZSC32T8XGDH
Defect fixes from an adversarial review. Each was verified against the code
before changing it; the trust-model and scope questions the review also raised
are deliberately not touched here.

Integration-test hermeticity, first so the rest can be believed. The plugin
integration tests built their child env from os.Environ() instead of
testutil.GitIsolatedEnv(), and GitIsolatedEnv is the only thing that makes the
process-wide ENTIRE_TEST_GIT_HERMETIC=1 bite — it writes the per-host
http.<url>.proxy entries that route github.com/gitlab.com at a dead loopback
address. Flag set, tripwire absent, real global git config inherited. So if
index-URL resolution had regressed to the built-in default, all four tests
would have cloned the real entireio/plugin-index and passed. cmd.Dir was also
unset, leaving the child standing in the real repo reading its committed
.entire/settings.json.

HTTPS enforcement was bypassable by redirect. pluginHTTPClient had no
CheckRedirect, and Go's default policy follows up to ten hops across schemes
while only the initial URL was ever checked — so an https:// entry point could
deliver the asset and its checksums.txt over plaintext, the exact outcome
requireSecureAssetURL was written to prevent. Every hop is now re-validated;
same-transport redirects (CDN hops) still work.

One URL validator, narrowed. validatePluginRepoURL and
PluginSettings.Validate disagreed about http://, git:// and bare absolute
paths, so `--index /srv/idx` was accepted while the equivalent committed
setting was a hard load failure — and the comment claimed to mirror an
allowlist it contradicted. Both now call settings.ValidateGitURL. http:// and
git:// are dropped: they are unauthenticated and unencrypted, and the catalog
fetched over them decides what installs without a prompt, so rewriting the
transport chooses the binary. Bare absolute paths are dropped in favor of
file://, which is unambiguous.

Prereleases are no longer resolved as newest. semver ranks v2.0.0-rc1 above
stable v1.9.0, so `plugin upgrade --all` moved every user onto a release
candidate as soon as an author pushed one. --pin bypasses listing and is the
opt-in. Tag sorting is now stable so semver-equal spellings resolve
deterministically.

macOS universal binaries could never install. "all" sat in the OS slot,
generating _all_<arch>, a spelling no forge produces; goreleaser puts it in the
arch slot as darwin_all — the fallback the docs already promised.

Binary writes are atomic and do not follow symlinks. writeExecutable opened the
destination with O_CREATE|O_TRUNC and no O_EXCL, so a symlink planted at
pkg/<name>/entire-<name> redirected the write anywhere the user can write, and
O_TRUNC destroyed a working binary before the new bytes were known good — on
the cross-device path, which is the normal one when /tmp is a separate
filesystem. Now a sibling temp file plus rename.

Credentials no longer reach logs, output, or disk. Userinfo is stripped from
every repo/index URL before printing, logging, or persisting, and git's stderr
is scrubbed for the same pattern. .entire/logs/ lives in the working tree and
is collected wholesale by `doctor bundle`; manifest.yml is mode 0644.

Bounded git subprocesses. The root context is cancellable but has no deadline,
so an unreachable host hung the command indefinitely with no output — worst
during dependency planning, which fans out to author-controlled URLs before any
confirmation. Two minutes per invocation, matching the download cap's intent.
LC_ALL=C in the same env, because the "no metadata file" branch substring-matches
git's English stderr and NLS builds translate it; ssh BatchMode so a key
passphrase prompt cannot block CI.

Empty entire-plugin.yml is no longer fatal. An empty or comment-only file
decodes to io.EOF; the file is documented as optional and a missing one is
handled, so a committed placeholder must not abort every install. Explicit
"---" already behaved this way.

index_ttl_hours is bounded. Past ~2.5M hours the nanosecond conversion wrapped
negative, i.e. always stale — the opposite of the large value requested.
Validate rejects out-of-range, IndexTTL saturates. A sync marker dated in the
future no longer reads as fresh forever (clock rollback, restored backup).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ943B4CJ61WHSMNXTA7GCFG
plugins.index_url was read from .entire/settings.json — a committed file
resolved from the working directory — while an index-listed repository installs
with no confirmation prompt. Composed, that meant cloning a repo containing four
lines of JSON and running `entire plugin install <any-name-in-their-catalog>`
downloaded an attacker-chosen binary, chmod 0755, and linked it onto the user's
PATH with no prompt, no --yes, and no warning. `plugin browse` did it on one
keypress.

The setting cannot be kept and made safe. Its whole value was that contributors
got a different catalog *without knowing* — that is the vulnerability stated as
a feature. The options were to make the redirect visible (prompt per unseen
index) or impossible; this takes the second, because it deletes code instead of
adding trust state, and because it fails safe: a missing override now falls back
to the curated default rather than to someone else's catalog.

Organizations wanting an internal catalog set ENTIRE_PLUGIN_INDEX_URL, which
applies across repositories rather than per-repository and cannot be chosen by
content the user merely checked out. Go takes the same position — nothing
committed can redirect GOPROXY — and npm's per-project registry override is the
cautionary counter-example.

plugins.index_ttl_hours goes with it: `plugin index update` already forces a
refresh and stale-on-offline already covers a failed one, so the knob tuned a
problem solved twice while costing a settings load per sync. The TTL is now a
fixed 24h constant.

Between them the whole PluginSettings struct, its validator, its merge path and
its tests are gone — 320 lines, and the plugin layer now reads no settings at
all, so resolvePluginIndexURL no longer needs a context. The URL validator moves
back to the cli package as the single definition, which is where its only
remaining callers live.

A deprecated, ignored Plugins field is retained for one reason: without it a
settings file left over from when the keys existed fails the strict loader, and
that breaks every command that loads settings — `entire status` included — with
an opaque "unknown field" error. Same tolerance the removed Strategy key gets.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ9AZ23QCVADS8VETZGFW96N
ParsePluginMetadata used strict decoding, the exact inverse of the choice made
for index.json in this same branch — where the reasoning is that an artifact
read by every shipped CLI version must degrade rather than refuse.

entire-plugin.yml is that same class of artifact and has no version field to
gate on, so the first plugin author to adopt any future field (min_cli_version,
bin_name, …) would break installs on every older CLI, permanently, with no way
to fix it for those users.

The trade is asymmetric in the other direction too: strict decoding caught
author typos, which costs the author one confused test run against their own
plugin. A forward-compatibility break is unfixable and fleet-wide.
Author-side validation belongs in a lint command, not in the path every user's
install runs through.

Timing is why this goes in now rather than later: once third-party
entire-plugin.yml files exist, older CLIs stay broken forever.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ9AZ4K1M2NWAC91K6N3HTZY
Reverts the deprecated tolerance field added alongside the index_url removal.

The Strategy precedent it was modelled on doesn't apply: that field tolerates
config which actually shipped, whereas plugins.index_url and
plugins.index_ttl_hours never appeared in a release. Nobody outside this branch
can have them, and the handful who do can delete one line.

Tolerating the key was also worse for the one case that mattered. The removed
setting's advertised purpose was for a company to commit an internal catalog;
silently ignoring it would have left such a repo on the public index while its
authors believed otherwise. An "unknown field" error forces the migration to
ENTIRE_PLUGIN_INDEX_URL to be noticed. For a security-relevant redirect, loud
beats quiet.

The settings package is now byte-identical to main — the plugin layer reads no
settings, so it adds no schema.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ9CN1ADA6GZ6463WAPFV9FR
Completes the credential scrubbing an earlier commit claimed to have finished.
That commit covered the git paths (gitremote, index, manifest, install) and
missed the HTTP one, so its message — "credentials no longer reach logs, output,
or disk" — was not true.

releaseAssetBaseURL derives the asset URL from the repo URL and url.String()
re-serializes embedded userinfo, so a private-forge remote like
https://user:token@host/o/r yields a credentialed download URL. Thirteen error
paths in plugin_fetch.go then interpolated it verbatim, and main.go prints
command errors straight to stderr — so any ordinary download failure (unreachable
host, 5xx, checksum mismatch, oversize) printed the token to the terminal or a CI
log.

The request keeps its credentials, since that is how a download authenticates to
a private forge; only the messages are scrubbed.

Worth noting Go had already masked its own nested error to bob:***@host — the
leak was entirely our own interpolation around it, which is exactly why the
regression test asserts on the full error string rather than trusting the
transport layer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ9GC3EV14ERWJF6BTE1XXD3
The installed plugin name came from the remote — entire-plugin.yml's name:, else
the repo basename — and was never reconciled with the name the caller had
already committed to. Three consequences, found independently by three
reviewers:

  - An index entry named "safe" could install entire-hijack. Index-resolved
    installs are treated as trusted and never prompt, so nothing tied the name
    the user typed to what landed on PATH.
  - --force escalated across plugins: the already-installed check tested the
    *remote-declared* name, so `install <A> --force` let A's repo declare
    name: B and replace an unrelated installed plugin B.
  - A dependency installed under another name never satisfied its requirement,
    so dependencySatisfied and doctor reported it missing forever and every
    future parent install silently re-attempted it.

RemoteInstallOptions.ExpectedName carries the committed name and a mismatch is
fatal. Three callers set it — an index-resolved install (the catalog entry), a
dependency install (the requirement), an upgrade (the plugin being upgraded).
A bare `install <url>` sets nothing, because there the repository legitimately
names itself; that path is unchanged and still covered by the existing
end-to-end test, where metadata names a plugin its repo basename could not.

Fatal rather than a warning: every caller that sets an expectation has already
made a trust decision about that name, and silently honoring a different one
voids it. A genuine rename is a catalog entry or requirement to fix, and the
message says so.

Also finishes the credential redaction from the previous commit across the
remaining user-visible URL output — the dependency-plan listing, the
uninspectable-dependency warning, doctor's reinstall suggestions, and `plugin
info`. Manifest URLs are already stripped at write time; redacting on the way
out too means no caller has to reason about which sources are pre-sanitized.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ9K067CSPVM91XB8DS42CVK
requires[] loses repo_url. A missing dependency now resolves by name through
the plugin index and nowhere else.

The old field let a plugin author decide that installing their plugin also
fetched and executed a binary from a URL of their choosing — and dependency
planning contacted that URL before the confirmation prompt. URL validation and
the unlisted-dependency warning added earlier were mitigations for that; they
did not remove it. This removes it.

The capability is not gone, the authority moved. A user can still install an
out-of-catalog dependency by URL themselves, with the usual untrusted-source
prompt, after which the requirement is satisfied and planning schedules nothing.
Consent belongs to the user, not the plugin author.

It also makes the trust model self-consistent. "Index-listed means trusted, so
no prompt" previously coexisted with "a dependency can name any URL", which
contradict each other. Now anything installed without the user typing a URL came
from the curated catalog.

Falls out of it: DepAction.FromIndex is always true for fresh installs, so the
field, the per-entry "not in the plugin index" annotation and its warning are
gone — the mitigation disappears because the hazard did. PluginRequirement
collapses to {name, min_version}, and requirement URLs no longer need validating
or credential-stripping.

The upgrade branch still reinstalls from the recorded manifest URL rather than
re-resolving through the index: that provenance was established and accepted at
the dependency's own install time, and re-resolving would break upgrading a
dependency the user deliberately installed by URL. The index requirement bites
only when a dependency is missing.

Migration is a no-op for published files. entire-plugin.yml decodes leniently as
of an earlier commit, so entireio/entire-brain's existing requires[].repo_url is
ignored rather than rejected. The one required step is outside this repo: add
graph to entireio/plugin-index, without which `install brain` reports the
dependency as unindexed. Had strict decoding been kept, this schema change would
have broken every older CLI against that published file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZ9PNTV3XPDJ6Y3PSEGH0Q87
… spots

Quality pass over the plugin diff. Two items were regressions rather than
tidiness, and both are fixed here.

Credential redaction now delegates to gitremote.RedactURL, the repo-wide helper
that already existed. The local implementation stripped userinfo but kept the
query string, so a signed CDN redirect — which is exactly what release hosts
serve asset downloads through — would have leaked ?token= / X-Amz-Signature=
into an error message. Five call sites had also been missed: the untrusted-source
confirm prompt, the install success line, the index-update summary, and both of
validatePluginRepoURL's own rejection messages, which echo the URL being
rejected. Test cases pin the query-string behavior.

runGitQuiet no longer clobbers GIT_SSH_COMMAND. Setting it unconditionally
discarded a user's own ssh invocation — jump hosts, an explicit identity — and
core.sshCommand with it, breaking clones that only work through them. It is now
supplied only when unset, which costs the passphrase-prompt guard in the one
case where the user has chosen their own command. LANG=C joins LC_ALL=C to match
the existing gitPlumbingEnv precedent.

dependencySatisfied returns the manifest it loaded, as a depStatus. Every caller
needed it and every caller re-read the file: twice per requirement in planDeps
(once in each branch of the same if) and again in doctor's unsatisfied branch.
Three reads per requirement become one, and two duplicated four-line blocks go.

downloadPluginAsset resolves the asset URL prefix once. It never depended on the
asset name, so it was re-parsing the repo URL on every candidate — up to 36 times
in the probe loop — and carrying an error return through three call sites for a
failure that can only happen at resolution. The probe loop leads with its
keep-walking case, dropping a nesting level.

--index was registered five times with the same help string; one addIndexFlag
helper now owns it. runRemoteInstall synced the index as the first statement of
both branches of an if/else, which forced three pre-declared vars; hoisted.

The dependency integration fixture still carried a requires[].repo_url after the
field was removed, which read as though the field still worked and proved
nothing. It now points at an unreachable host, so the test fails loudly if the
leftover is ever honored instead of being ignored in favor of the index.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBFC3SP3Z5VRT747B2WWCT4
All seven plugin test files are new on this branch, so this duplication was
self-inflicted rather than shared with pre-existing suites. This is the
in-package half; the unit/integration sharing that would need a home in
testutil is deliberately left alone.

One item was a latent bug, not tidiness. Three `git tag` shell-outs and the
bare-repo init ran without cmd.Env, so they inherited the developer's global git
config — a `tag.gpgSign = true` there fails these tests locally while CI stays
green. They now go through runGitIsolated, which sets testutil.GitIsolatedEnv()
the way the pre-existing testutil.CreateBranch and GitReset do.

updateRepoMetadata hand-rolled `git add` plus a `git -c user.name=… commit
--no-gpg-sign` in 15 lines, while the same file already did that correctly in
five with testutil.WriteFile/GitAdd/GitCommit. It now uses the helpers, and the
inline copy routes through it.

pluginReleaseServer registers t.Cleanup itself, so eight `defer srv.Close()`
lines go. newDemoPluginRepo absorbs the four-line preamble eight tests opened
with, taking the `download_url` format string from ten copies to two.

plugin_fetch_test.go had thirteen hand-rolled httptest servers in three shapes;
staticServer/assetServer/notFoundServer replace seven of them, and the
`//nolint:errcheck // test server write` comment drops from eleven sites to four.
The three redirect servers stay inline — their behavior is the point of the test.

withIsolatedPluginEnv replaces the withPluginDir+withIsolatedPath pair that
appeared 21 times and never once separately. withIndexCache replaces nine copies
of the XDG_CACHE_HOME line. Two hand-rolled os.MkdirAll+os.WriteFile pairs become
testutil.WriteFile, which creates parents.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBKRH4TF8Y5R98KSX6BJKDG
RemoteInstallOptions carried RepoURL and ExpectedName as fields. Neither is an
option: one is the target, the other is a security decision.

ExpectedName as a field could be silently omitted, and omitting it reopens the
no-prompt name-substitution hole the field exists to close — an index entry
named "safe" installing entire-hijack, or --force on plugin A replacing an
unrelated installed B. No test can catch the absence of a field in a caller that
does not exist yet. As a required argument, "" is still expressible but is now a
visible choice at the call site rather than an omission.

RepoURL moves for the plain reason that options configure how an install
behaves, not what it installs. The struct is down to the three genuine options:
Pin, Force, AllowUnverified.

All three production callers and every test call site are in this branch, so the
change is self-contained.

Considered and not taken: splitting into InstallKnownPlugin and
InstallPluginFromURL so the compiler forces the choice. Two of three callers
always know the name, but runRemoteInstall legitimately handles both cases, so
the split would move the "" into a branch there rather than remove it — the same
residual expressiveness for twice the exported surface.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBPBKK5E8XRCKDE984Q3EX4
classifyInstallArg tested for a literal "git@" prefix while
validatePluginRepoURL's scpLikeGitURL regex accepts any SSH username. Two
definitions of "scp-like git URL" that disagreed, and the broader one was added
later in this branch, so the classifier was left behind.

The consequence was a confusing failure for anyone whose forge uses a
non-default user. deploy@git.corp.io:group/entire-foo.git is a URL the validator
would install from, but the classifier saw the path separator, sent it down the
local-path branch, and the command failed with

  install plugin: stat source: stat /tmp/deploy@git.corp.io:group/entire-foo.git:
  no such file or directory

The classifier now shares scpLikeGitURL, so there is one definition. The
scp-like test runs before the separator test because these URLs contain a
separator too.

Verified against the built binary: both git@ and deploy@ forms now reach
git ls-remote, and ./entire-localdemo still installs as a local path. Test cases
cover a non-default username, a username with a hyphen, and ./deploy@host:x —
which is a relative path, not scp-like, because the regex anchors the username
at the start.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBXJTB9W65HCP04PHYQJM33
classifyInstallArg and validatePluginRepoURL answered overlapping questions in
two places, and they drifted: the classifier matched a literal "git@" prefix
while the validator's scpLikeGitURL accepts any SSH username, so a
deploy@host:path URL the validator would install from was routed to the
filesystem instead. That was fixed by sharing the regex; this removes the
possibility of it recurring, because there is now one function and it cannot
disagree with itself.

The argument was also being classified twice — once in the install command to
split off the path case, once in runRemoteInstall to split off the index case —
each re-deriving the kind from the raw string. parseInstallSource returns a
typed installSource that is parsed once at the command boundary and carried.

Validating at the boundary is a free error-message upgrade: a reserved or
malformed name now says so instead of being handed to the catalog and coming
back "not in the index". `install agent-evil` reports the reserved prefix.

An option-shaped argument is now refused for every kind. It is never a
legitimate source — the URL validator rejects it, validatePluginName rejects it,
and a path would be written ./-foo — and previously anything containing a
separator reached the path branch and failed as "stat: no such file".

Two shapes are deliberately still accepted by the parser, with a test comment
recording why: "ext::sh -c whoami" has no separator and no scheme, so it is an
index name that misses the catalog, and anything with a separator is a path that
fails at stat. Neither can reach git as a repository URL, which is the property
that matters. validatePluginName permits spaces and colons — pre-existing
looseness, harmless on those two routes, and tightening it would change
local-install behavior outside this branch.

Not done here: moving the source vocabulary into its own file. It is currently
spread over four (validator and scp regex in plugin_gitremote.go, the parser in
plugin_group.go, normalizeRepoURL in plugin_index.go, asset URLs in
plugin_fetch.go). Co-location would make a future drift visible; one decision
makes it impossible, so this was the half worth doing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZC4XYYYKSRZG4A34FG08TYR
…utcomes

Two findings from the agent review of the current head.

The manifest is now written immediately after the binary swap, before the bin/
link. replaceBinary has already mutated pkg/<name>/ by that point, and until the
manifest catches up it records the *previous* tag and binary_sha256 while the new
binary is on disk — which checkManagedBinaryIntegrity reads as tampering. A
failure or interrupt in any of the three steps that used to sit between the swap
and the manifest write left a permanent false "modified or replaced outside
entire" alarm on a perfectly good install, and a stale tag in `plugin list`.

Reordering leaves only the local re-hash in that window, and turns the remaining
failure mode into an accurate one: if the bin/ link then fails, the manifest is
correct and doctor reports "has an install manifest but no entry in the managed
bin dir" with a fix that works, instead of accusing the user of tampering.
SavePluginManifest also writes through jsonutil.WriteFileAtomic now, so a torn
manifest cannot be produced either — the same shared helper the settings and
checkpoint stores already use.

ExecuteDepPlan verifies the outcome rather than the attempt. It installs the
newest published tag, which may still be below the minimum the plan computed —
if a dependency hasn't cut a release meeting the parent's requirement, the loop
reported success and printed "Installed dependency", silently defeating the
guarantee PlanDependencyInstalls had just derived. It now fails with both
versions named. The check runs after the install, so the dependency is left on
disk at the too-old version: deliberate, because doctor then reports "requires
demo >= v9.0.0 but v0.1.0 is installed", which is accurate and actionable rather
than leaving nothing to diagnose.

This closes the min_version-enforcement gap that had been listed as knowingly
open since the dependency system was first reviewed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZC6NGK5C4G1FPGY39FV53KA
`plugin browse` installed on a single keypress. Index-resolved installs are
treated as trusted and never prompt inside runRemoteInstall, so the picker's
select *was* the confirmation: arrow to a row, press Enter, and a binary is
downloaded and linked onto PATH. The picker also shows only a name and
description, so the repository the binary actually comes from was never named.

It now confirms, naming that repository, and follows the same
handleFormCancellation convention as the untrusted-URL prompt so Esc and "No"
both exit cleanly with "Install cancelled."

Also gives browse a Long description — it had only a one-line Short — stating
that it needs a terminal and pointing at 'search' plus 'install <name>' for
scripts, which is the non-interactive equivalent the agent-safe-fallback rule
requires.

The interactive prompt itself is not unit-testable without a TTY harness, which
is true of every huh prompt here; the test covers the documented no-terminal
path and asserts the error names the alternative, matching how the
untrusted-URL prompt is covered.

This addresses the confirmation half of the standing question about browse. The
larger one — whether it earns its place at all, given `search` plus
`install <name>` does the same thing — is still open and is Thomas's call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZC7HEZC0REF76NM7ZD3GNE8
ParsePluginMetadata validated each requirement's name but not its min_version.
x/mod/semver ranks an invalid version string below every valid one, so
semver.Compare(installedTag, garbage) >= 0 is always true and
dependencySatisfied reported any installed version as acceptable — the floor
disappeared instead of the error surfacing. Verified: "vtypo", "latest",
">=1.0" and "1.x" all compare as satisfied.

The likeliest author mistake is range syntax, since the field is deliberately a
minimum only, and that is exactly the shape that silently disabled the check.

Rejecting this is consistent with the lenient decoding a few lines above, not in
tension with it. That leniency is about unknown *keys*: they are a newer CLI's
fields, and refusing them breaks older binaries permanently for everyone on that
version. A malformed *value of a known field* is an author error no CLI version
will ever accept — which is why the requirement's name is already validated in
the same loop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZC8CD1ECZQP8G6TANN6BCQG
…essages

Three findings from the whole-change review.

--force can replace a plugin the user never named. On a bare `install <url>`,
expectedName is empty, so the reconciliation guard is skipped and the remote's
entire-plugin.yml decides which pkg/<name>/ and bin/entire-<name> get
overwritten — under a confirmation that named only a URL. Install does not
execute the binary, so the payoff is that the *next* invocation of the displaced
plugin runs the new one.

Refusing on a repository mismatch was the wrong fix: --force is precisely for
replacing, and a plugin moving repositories is legitimate — entireio/entire-sem
and entireio/entire-graph are the same plugin today. The defect is uninformed
replacement, not replacement. Two changes instead:

  - A URL that is listed in the index now takes its expected name from the
    catalog entry (FindByRepoURL). That path installs with no prompt at all, so
    the entry was the only thing that could tie the request to what lands on
    PATH; the existing guard now applies there.
  - An unlisted URL still prompts, and a --force install that displaces a plugin
    from a different repository now reports what it replaced and where that came
    from — the URL to put things back.

A URL install with an unreachable index hard-failed after the plugin was
already installed. idx is nil when SyncPluginIndex fails; trusted short-circuits
on the error but idx was still handed to dependency planning, where Find returns
nil on a nil receiver and every dependency is reported "not in the plugin index"
— blaming the catalog for a fetch that never happened, and exiting non-zero
after the install printed success. That contradicted the contract stated four
lines below the call site. Dependency planning is now skipped with a warning.

Doctor handed out a command that cannot work. Its three reinstall suggestions
re-run with checksum verification required, which fails with errUnverifiedAsset
for exactly the plugins installed via --allow-unverified — the ones doctor also
flags. reinstallCommand carries the flag through when the manifest records it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZE0CQSQD9MZQYHAAWRSBBN4
file:// was in the production allowlist purely to let the test suite build
fixture repositories. Nothing shipped needs it: a file:// plugin repository
cannot even complete an install, because releaseAssetBaseURL requires an http(s)
repo URL to locate the release asset, and `install ./path` is the supported way
to use a local build. A security allowlist should not be widened to pay for test
convenience.

Replacing it with a git daemon — the obvious alternative — would have been
worse: git daemon serves git://, unauthenticated and unencrypted, which is
exactly the scheme this allowlist rejects. Smart-HTTP would need
git-http-backend plus http:// back in the list, and ssh:// needs an sshd. Every
test transport needs some scheme; file:// is the least dangerous of them, and
gating it is better than promoting a worse one.

The gate is testing.Testing(), plus ENTIRE_TEST_ALLOW_FILE_REMOTES for the
integration harness, which spawns the real binary where testing.Testing() is
false — the same shape the harness already uses for config, cache and token
isolation. Being able to set that variable already implies code execution, so it
is not a weakening. What it buys is that a hostile catalog entry can no longer
name a local path and use the CLI to probe the filesystem; it could not have
installed anything either way.

The gate is a var rather than a func so the shipped rejection path is reachable
from a test — testing.Testing() is otherwise always true where the test runs.
Same seam as postPluginVersionCheck in this package.

Scope note: this was raised as skepticism about supporting file:// at all, and
the stricter reading — test-only everywhere — is what landed. An earlier
proposal kept file:// for the index URL in production on the grounds that a
file:// index does work end-to-end and is user-supplied. That was dropped: it
needed two validation policies over one core, and two policies drifting is the
exact failure this branch already hit twice. An air-gapped organization needs an
internal git server for the plugin repositories regardless, since assets require
http(s), so hosting the index there too costs nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZEBAKRR4JQ93HDXE8D2VK92
Two review points, both wider than the sites they were raised on.

Lstat instead of Stat for the plugin-index cache checks. Both ask whether *our*
own files exist — the .git marking a directory as a clone, and the sync marker
whose mtime drives the TTL. Stat follows symlinks, so a .git symlink pointing
elsewhere would answer "yes, this is a clone" for a directory that is not one.
Matches the convention 062d8bb set for existence checks on files we own. The
one remaining Stat is on a user-supplied path, where following a symlink to a
real file is the helpful behavior.

Bounded reads, which turned out to matter most where os.ReadFile was not
involved. os.ReadFile sizes its buffer from the file and reads to EOF, so
callers inherit whatever is on disk; readFileLimited now caps index.json (8 MiB
— cloned from a remote catalog) and manifest.yml (1 MiB — ours, but in a
user-writable directory), erroring rather than truncating, since a truncated
file parses as valid-but-wrong.

The larger exposure was cmd.Output(), which buffers stdout with no ceiling at
all, on two calls that read remote-controlled data: ls-remote returns whatever
refs a repository advertises, and `show <tag>:entire-plugin.yml` returns
whatever an author committed. A hostile repository listed in a catalog could
hand back an arbitrarily large response. runGitQuiet now takes an explicit cap
per call — 64 KiB for metadata, which holds a name, a description and a short
requires list; 16 MiB for refs, still hundreds of thousands of tags; 4 KiB for
--quiet commands that should print nothing.

That meant reading stdout through a pipe rather than cmd.Output(), so stderr is
captured directly and folded into the error. stderrSuffix went with it — it
only ever existed to dig detail out of ExitError.Stderr, which is empty once
cmd.Stderr is set, and it was the sixth copy of that pattern in the repo. The
benign "no metadata file" branch now matches on the error text, which carries
the same stderr; TestFetchPluginMetadataAtTag_NoFileIsNilNil covers it.

readFileLimited deliberately returns os.Open's error unwrapped, because
LoadPluginManifest tests it with errors.Is against os.ErrNotExist to report an
absent manifest as (nil, nil).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZECCDJAM5SE1FM7KK0X617P
The guarantee asked about already held, in three independent places, but
nothing checked it — and writing the check found a real bug next to it.

What holds: the archive entry's name only *selects* what to extract. The
destination is built from the validated plugin name, always
entire-<name>[.exe], so what lands in pkg/<name>/ and bin/ cannot be anything
else; validatePluginName has already rejected separators, . and .., a leading
-, and the reserved agent- prefix. Every extraction branch returns on its first
match, so one install writes one binary, and a differently-named entire-* entry
in the same archive is ignored. Local installs are gated the same way, in
plugin_store.go. All of that was emergent from how the code is written rather
than asserted anywhere, so a refactor using the archive's own name, or one that
kept looping, could have let a plugin put a second command on PATH — including
somebody else's.

The bug the test caught: selection was "first basename match in archive order",
and the test's fixture (a Go map, iterated randomly) picked bin/entire-mine over
the root entire-mine. Matching is by basename, so an archive legitimately holds
several candidates — the binary at the root, the same name nested under a
versioned directory from goreleaser's wrap_in_directory, and unrelated files
like completions/entire-<name>. Which one got installed depended on the order
the author's tar was built in.

preferredArchiveEntry now chooses shallowest-first, then lexicographic, which
encodes the convention: the real binary is at the root or one directory down,
and a same-named file deeper in the tree is a completion script or a doc. For
tar this needs a second pass, since the stream cannot seek backwards; the cost
is one extra inflation of an archive already capped at maxPluginAssetSize.

Tests cover the invariant end to end (an install writes exactly the binary plus
its manifest, and one bin entry), the selection rule as a table, and the
extraction repeated eight times against a randomly-ordered archive to catch the
non-determinism directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01KZEDKAB9JG7NY62Y6BXCNVZT
…n memory

The previous pass bounded stdout, files, and downloads. Reviewing that work
turned up three places the bound did not actually hold:

- git's stderr was still a plain bytes.Buffer. cmd.Output() had supplied a
  32 KiB ceiling for free, and replacing it dropped that silently — on a
  stream carrying the remote's sideband. limitedWriter caps both channels,
  with the overflow policy that fits each: stdout fails (a truncated payload
  is not the payload), stderr absorbs the excess (diagnostics must never fail
  a command git itself completed). Feeding it to cmd.Stdout instead of
  reading a pipe also drops a two-minute hang: os/exec closes the pipe as
  soon as a write fails, where returning early from a pipe read left Wait
  blocked on a full one until the timeout.
- tarEntryNames collected every entry name before filtering. Entry headers
  compress well enough that the 512 MiB asset cap admits tens of millions of
  them, so bounding the read still allowed gigabytes of heap — to choose
  between the one or two entries a real release holds. A running minimum
  (archiveEntryPicker) does the same job in one string.
- httpGetSmall truncated checksums.txt at 1 MiB instead of erroring, which
  would drop the line covering our asset and turn a verifiable download into
  an unverified one for a reason nobody could see.

readFileLimited now delegates to readWithinLimit, which already existed in
this package and was already tested; the idiom had been written three times.

The file:// seam test replaced allowLocalGitURL with its own reimplementation
and asserted against that, so it proved nothing about shipped code. The
policy is now a pure function taking both inputs, and the test exercises the
spawned-binary case that testing.Testing() masks in-process.

Two suggestions repaired: a dependency install that replaced a plugin from a
different repo said nothing, though the top-level install names it and the
dependency path is the one confirmed as a batch; and the doctor repair
command dropped --pin, so fixing a broken install would silently unpin a
plugin held at a version on purpose.

Also: extract walkTarGz (the two tar passes duplicated their scaffolding
verbatim), archiveEntryPath (selection and extraction must normalize
identically), and confirmInstallOrCancel; make extractFromZip reach its entry
directly rather than rescanning with a Close deferred inside a loop; add
runGitDiscard for the five --quiet call sites; drop HasRepoURL, which had no
callers left outside its own test. Two doc comments had been orphaned by
insertions that landed between a comment and its function, and two claimed
Output() was still in use.

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