Skip to content

🤖 feat: Agent Plugins install/update UX (managed installs, v1) - #3820

Open
ThomasK33 wants to merge 19 commits into
mainfrom
agent-plugin-install-ux
Open

🤖 feat: Agent Plugins install/update UX (managed installs, v1)#3820
ThomasK33 wants to merge 19 commits into
mainfrom
agent-plugin-install-ux

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

v1 of the Agent Plugins install/update UX ("Option B: managed installs"): paste a git URL or owner/repo[@ref] into Settings → Plugins, get a consent preview of everything the plugin contributes (manifest, every skill, every MCP server command line), and install into ~/.mux/plugins with provenance recorded in a managed-install registry. Update badge + manual update, uninstall with override pruning, all behind the existing agent-plugins experiment.

Background

PR #3815 shipped Agent Plugins 1.0.0 as discovery-only: users had to git clone into container dirs by hand, with no provenance, no update signal, no uninstall, and no list surface. The design doc (docs/research/agent-plugin-integration-options.md on branch research-agent-plugin-ux) compared five options; Thomas signed off on Option B (managed installs) with the §6 proposed decisions ratified.

Approved decisions implemented here

  1. Registry — a standalone ~/.mux/plugins.json owned by the install service (atomic, throwing writes; in-process serialized mutations). Lenient-on-read: invalid entries are dropped with a warning, and plugin names are pattern-validated so a malformed entry can never resolve a path outside the container. (Deviation from §6-Q2's letter, following its own contingency: Codex review demonstrated that .passthrough() only affects schema validation — older builds rebuild config.json from known fields on save, so a downgrade would drop an embedded registry section. Q2 priced exactly this: "Cost if wrong: a one-time migration to a separate file." A file older builds never rewrite is the only mechanism that actually survives downgrade round-trips, and owning the write path also makes registry-persistence failures observable for rollback.)
  2. Tracking semanticssource.ref is the tracking channel, lockedSha is what runs. No ref given ⇒ record the remote default branch + pin its current SHA. Tag/SHA refs are pinned (moved tags surface a tag moved warning badge). Nothing auto-applies, ever.
  3. Consent preview — temp shallow clone to ~/.mux/plugin-staging (never inside a discovery container), validated with the same validatePluginManifest + discovery code the runtime uses, listing manifest metadata, every skill name+description, and every MCP command line (rendered against the final install path, incl. PLUGIN_DATA expansion). Cancelling writes nothing — the preview is stateless; install re-fetches the exact consented SHA and fails loudly if the remote moved.
  4. Update — badge + manual only; checks run on Settings-section open and on the explicit button (git ls-remote vs lockedSha, no fetch, no timers). Applying = temp clone at the new SHA → re-validate → wholesale directory swap (rename-old → promote-new → delete-old, with rollback) → bump lockedSharecycle that plugin's running MCP servers via the new MCPServerManager.stopServersWithKeyPrefix (content can change behind an unchanged stdio command line, so the config-signature check cannot notice). Local edits to a managed plugin dir are discarded on update (documented).
  5. Uninstall — deletes plugin dir + registry entry + prunes that plugin's plugin:<instanceId>:* keys from every local workspace's MCP overrides (reinstall re-attaches the same instanceId, so stale overrides would silently re-enable servers). ~/.mux/plugin-data/<instanceId> is preserved behind an "also delete stored plugin data" checkbox, unchecked by default.
  6. Scope — global-only; the installer never writes into a project checkout.
  7. Human-only surfaces — Settings section + palette commands (Settings: Plugins, Install Agent Plugin…, Check for Plugin Updates, Update All Plugins — keyboard rule). No agent-facing installer tool.
  8. Name collisions — existing registry entry or target dir ⇒ clear error; the installer never overwrites.
  9. Subpath grammar, not subpath installsowner/repo/sub/path[@ref] parses and the subpath field is persisted in the source descriptor, but installs reject with "monorepo subpath installs land in v2". Claude Code plugin/marketplace repos fail with a clear message naming the limitation (source stays a discriminated union so an import adapter is additive).

Implementation

  • Step 1 — registry schema: src/common/config/schemas/agentPluginInstalls.ts (entry + tagged-union source + plugins.json file schema); name grammar shared with the manifest validator via src/common/utils/agentPluginName.ts.
  • Step 2 — service + oRPC: discoverAgentPluginAt (public single-root wrapper over the existing per-entry discovery, so staged clones get the exact runtime validation); normalizeRepoUrlForClone extracted to src/node/utils/gitUrls.ts (shared with the project clone flow); sourceInput.ts grammar; AgentPluginInstallService (preview/install/list/uninstall/checkUpdates/update, mutations serialized on an internal queue, staging under ~/.mux/plugin-staging with stale-dir reclamation, GIT_TERMINAL_PROMPT=0 + SSH BatchMode so private repos without auth fail fast instead of hanging); plugins.* oRPC namespace returning Result values; MCPServerManager.stopServersWithKeyPrefix recycle hook. Backend gating mirrors the MCP provider: the service is constructed with isEnabled: () => experimentsService.isExperimentEnabled(AGENT_PLUGINS).
  • Step 3 — UI: PluginsSettingsSection (list with unmanaged/missing/update available/tag moved/pinned badges, two-phase add flow, inline uninstall confirm), experiment-gated section registration + redirect + palette entry.
  • Step 4/5 — docs, stories, tests: docs additions in docs/config/mcp-servers.mdx + docs/agents/agent-skills.mdx; Storybook stories with play assertions (consent preview, update states, unchecked-by-default checkbox); unit tests for the input grammar, registry round-trip/self-heal, and the full service lifecycle against real local git remotes (hermetic — local-path remotes exercise the same clone/ls-remote plumbing).

Validation

  • make static-check green (typecheck, ESLint, prettier, docs links); targeted suites: 393 tests across the touched areas (agentPlugins, config, schemas, SettingsPage, palette sources, MCPServerManager, oRPC router, projectService) all pass; test-storybook passes for the new stories.
  • Live dogfooding in a make dev-server-sandbox instance (screenshots in the workspace transcript): enabled the experiment via Settings → Experiments (Plugins section appeared immediately), installed a local fixture repo through the full preview → consent → install flow, verified the on-disk registry entry + plugin tree (no .git), advanced the fixture remote → update available badge appeared on "Check for updates" → Update bumped lockedSha/version/updatedAt, uninstall (checkbox unchecked) removed dir + registry but preserved plugin-data, and the reinstalled plugin's MCP server surfaced in Settings → MCP as plugin · … default-disabled/read-only.

Risks

  • Config surface: none — the registry is a standalone ~/.mux/plugins.json; config.json load/save is untouched. Malformed registry entries degrade to "unmanaged dir" rather than errors; downgrade-safe because older builds never touch the file.
  • MCP recycle: stopServersWithKeyPrefix only stops matching workspaces' server sets; they restart lazily on next use, same as the idle-timeout path. No behavior change for non-plugin servers.
  • Everything is experiment-gated: with agent-plugins off, the service throws, the section/palette entry hide, and no new code paths run.

Judgement calls

  • install re-fetches the exact consented SHA (direct SHA fetch, falling back to branch clone + HEAD verification) rather than keeping the preview clone on disk between preview and confirm — a stateless preview means cancel/crash cannot leave partial state, at the cost of a second shallow clone on confirm.
  • The installed tree drops .git (plain content snapshot): the registry holds all provenance, updates replace the dir wholesale, and a live checkout would only invite in-place edits that updates discard.
  • Update refuses upstream renames (plugin.json#name changed): container-entry names are identity (instanceId → PLUGIN_DATA, workspace overrides), so renames require uninstall/reinstall.
  • Uninstall stops that plugin's running MCP servers before deleting the tree, mirroring the update-recycle rationale.
  • Update All Plugins applies only update-available entries; moved tags stay per-plugin manual (a mutated tag deserves the section's warning, not a bulk apply).

Deferred (per §5/§6 of the design)

  • v2: monorepo subpath installs (sparse checkout; grammar + schema already in place), content-addressed store + symlinked container entries, dev-mode/local-path installs, unmanaged-dir adoption ("convert to managed"), Pin row action, bun run debug plugin … CLI + /plugin slash command.
  • v3: repo-declared prompt-on-trust team plugins, archive+sha256 / seed dirs for air-gap, restore-from-lock, catalogs/marketplace (Claude marketplace import adapter only on demonstrated demand).
  • Explicit non-goals: background/auto-update (per-entry autoUpdate boolean reserved in the schema, unused), agent-facing install tool, Claude Code marketplace compatibility.

Post-review hardening (Codex rounds 1–20)

20 review rounds of fixes folded into this diff

Highlights beyond the original plan (full round-by-round history in the PR review threads):

  • Registry durability: standalone plugins.json with lossless raw-document writes (unknown envelope/entry/tombstone fields from newer builds survive rewrites), strict-mode reads for mutations vs lenient reads for list, raw-entry collision checks, non-ENOENT read errors refuse mutations.
  • Uninstall override pruning: pre-commit workspace enumeration, persisted pendingOverridePrunes tombstones (pessimistic commit, retry on list, reconciliation against deleted workspaces, reinstall gate) so a temporarily unreachable checkout can never let a reinstall silently re-enable a pruned server.
  • Workspace MCP overrides optimistic concurrency: workspace.mcp.get returns { overrides, revision }; set requires expectedRevision and rejects stale dialog snapshots (serialized check-and-set), and the uninstaller's prune retries on conflict — a stale open dialog can no longer resurrect pruned plugin: keys.
  • MCP recycle vs in-flight startups: monotonic prefix-invalidation epochs; closeInvalidatedInstancesThenPublish re-scans until the invalidation clock is stable and publishes synchronously in the same continuation, closing the microtask window where a mid-startup plugin swap could publish (and keep alive) a server from a deleted tree. Regression test interleaves the swap into the exact yield window.
  • Consent preview parity: symlinked skill dirs are disclosed (with containment warnings for escaping symlinks), matching runtime discovery.
  • Mobile: break-all on plugin name/location/source lines + pinned 390px story with a max-length-name overflow assertion.

Two P2 follow-ups are documented (not in this PR) in this comment: stale workspace-switch modal loads, and prefix-stop retry-marker publish ordering.

Rebase + experimental label

  • Rebased onto main (squashed to one feature commit): merged this branch's install contract with main's independently-landed agentPlugins.ts oRPC schemas (slash commands/composition inspector), re-ported the stable-clock publication onto the MCP SDK v2 mcpServerManager, and adopted main's SSH→HTTPS clone fallback into the extracted gitUrls.ts (installer records the primary URL only, documented).
  • The Plugins section is now marked experimental the same way as Backup: FlaskConical nav icon + in-section warning banner.
  • Re-dogfooded the full lifecycle post-rebase in a fresh dev-server-sandbox (install → consent preview incl. a deliberately-invalid mcp.json diagnostic → update badge → atomic update → read-only MCP row → uninstall with data-preservation default); screenshots in the workspace transcript.

Generated with mux • Model: anthropic:claude-fable-5 • Thinking: xhigh • Cost: $327.63

@mintlify

mintlify Bot commented Aug 8, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
Mux 🟢 Ready View Preview Aug 8, 2026, 6:37 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b9d7245ce

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/common/config/schemas/appConfigOnDisk.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all five Codex findings in b9c9a10:

  • Traversal names (P1) — the plugin-name grammar (§5 pattern, now shared via src/common/utils/agentPluginName.ts) is enforced in the registry entry schema and asserted in targetPathFor before any filesystem mutation; entries named ./../a/../b are dropped on read and can never resolve outside the container. Test: "registry survives config.json rewrites and drops traversal names on read".
  • Downgrade preservation (P1) — correct: .passthrough() only affects schema validation; older builds rebuild config.json from known fields on save. Followed §6-Q2's own contingency ("migration to a separate file"): the registry now lives in a standalone ~/.mux/plugins.json that older builds never rewrite. Test: registry survives editConfig config.json rewrites.
  • Registry write observability (P2) — solved by the same move: the service owns the file and its atomic write throws, so install rolls back the promoted dir ("Failed to persist the plugin registry"), uninstall writes the registry before deleting the tree, and a failed update write keeps the stale lockedSha (badge stays, retry self-heals). Test: "install rolls back the promoted dir when the registry write fails".
  • Fallback clone into non-empty dir (P1) — the staging dir is reset before the branch-clone fallback. Test: "falls back to a branch clone when the remote refuses direct SHA fetches" (file:// remote with uploadpack.allowAnySHA1InWant=false).
  • Keyboard rule (P1) — added palette commands: Install Agent Plugin… (opens Settings → Plugins with the add form expanded), Check for Plugin Updates (toast + navigates when updates exist), Update All Plugins (applies update-available; moved tags intentionally stay per-plugin manual since mutated tags deserve the section's warning). Uninstall/per-plugin update remain reachable via standard focus navigation within the section.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9c9a1062a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/utils/commands/sources.ts Outdated
Comment thread src/browser/utils/commands/sources.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all four round-2 findings in edcdfa0:

  • Registry restore on removal failure — uninstall now stages the tree out of the container (rename into the staging root) before the registry write: a locked/undeletable tree fails the rename with the install fully intact, and a failed registry write renames the tree back. Deleting the staged tree is best-effort (stale-dir reclamation covers leftovers). Test: "uninstall restores the registry entry when the tree cannot be staged out" (read-only container forces the rename failure, then the retry succeeds).
  • Add panel with section already mounted — the intent module now supports subscription; the mounted section subscribes and expands the add panel immediately, while the useState initializer still covers the palette → fresh-mount path.
  • Mutation errors clobbered by refresh — update/uninstall re-assert the operation error after the refresh (whose success path clears error state); a failed uninstall also keeps the confirmation open instead of dismissing it.
  • Per-plugin check errors — both Check for Plugin Updates and Update All Plugins now distinguish status: "error" entries: unreachable remotes surface as "Update check failed for …" (with navigation to the section) instead of masquerading as "All plugins are up to date."

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: edcdfa05fe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/browser/utils/commands/sources.ts
Comment thread src/node/services/agentPlugins/sourceInput.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all three round-3 findings:

  • Keyboard path for uninstall — new Uninstall Agent Plugin… palette command using the palette's select prompt (async getOptions over agentPlugins.list(), managed entries only). Submission publishes a confirm-uninstall intent and opens the section, landing the user in the existing confirmation flow with the plugin-data checkbox — the palette never deletes directly.
  • Mounted-section staleness after bulk updates — the intent module is now a typed bus (open-add-panel / confirm-uninstall / refresh); Update All Plugins publishes refresh after its mutations, so a mounted section re-queries list + update checks instead of showing stale versions/badges. Unmounted sections still consume the buffered intent on mount.
  • Tilde expansion~/~/… sources resolve against os.homedir() before git sees them (git is spawned via execFile, no shell). Grammar test added.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a6503d2fe4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/utils/commands/sources.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/node/services/agentPlugins/sourceInput.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc68e771d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/mcpServerManager.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/utils/commands/sources.ts
Comment thread src/node/services/agentPlugins/installService.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a2bd105f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/mcpServerManager.ts
Comment thread src/node/services/mcpServerManager.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0443e1af3e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc141c4c7e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6daf6aec0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1fd47e8cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6965bc298

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/utils/commands/sources.ts Outdated
Comment thread src/browser/utils/commands/sources.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e576538423

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85f0aeb3e6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/workspaceMcpOverridesService.ts
Reject duplicate JSONC properties before path-based override pruning:
jsonc.parse exposes the last value of a duplicated property while
jsonc.modify resolves the first matching path, so the edit loop could
spin forever on an entry it can never remove (duplicate
enabledServers/disabledServers) or declare success while a stale
plugin key survives in the shadowing property (duplicate toolAllowlist
or duplicate keys inside it). Detection walks the JSONC syntax tree;
rejection keeps the caller's retry tombstone. Also assert each
targeted edit actually changes the text so a parse/modify disagreement
can never loop silently.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 3b030057e1

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b030057e1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/workspaceMcpOverridesService.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
- Security: updates reject capability increases/changes (new hooks.js,
  expanded hook tool grants, added or changed MCP servers) instead of
  silently applying them; uninstall + reinstall routes through the full
  install consent preview.
- Consent preview renders stdio argv shell-quoted per token exactly like
  the runtime, so argument boundaries cannot be concealed or faked.
- Reject Windows-reserved device names (con, prn, aux, nul, com1-9,
  lpt1-9, with or without extension) in the shared plugin-name validator
  on every platform.
- Bound untrusted git subprocess output (10 MiB) so a noisy remote
  cannot exhaust main-process memory before the timeout.
- checkUpdates reads the registry strictly (corrupted registry surfaces
  as the update-check error state, not a false all-clear) and bounds
  concurrent ls-remote lookups to 4.
- prunePluginOverrideKeys rejects opaque enabledServers/disabledServers/
  toolAllowlist shapes from newer builds so tombstones stay retryable
  instead of retiring against uninspectable content.
- Use shared warning color tokens (bg-warning/text-warning) instead of
  fixed yellow-500 utilities in the Plugins section.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5c7136d8f2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/utils/commands/sources.ts
- Include stdio cwd in the update capability fingerprint: a cwd-only
  change (e.g. plugin root -> writable PLUGIN_DATA) silently moves
  relative module/config resolution, so it requires re-consent.
- Settings copy shows the backend-provided managed plugin container
  path (new agentPlugins.containerLocation route) instead of a
  hardcoded ~/.mux/plugins: the active root is config-derived.
- checkUpdates errors when the registry holds entries this build
  cannot parse (newer source kinds / corruption) instead of silently
  skipping them and reporting a false all-clear.
- Uninstall surfaces a failed user-requested plugin-data deletion
  (with the staged path for manual cleanup) instead of confirming
  success; staging reclamation may otherwise never run.
- New 'Update Agent Plugin…' palette command: a per-plugin selector so
  keyboard-only users can apply moved-tag updates after reviewing the
  warning (bulk update intentionally excludes them).
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fc62ab47ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/workspaceMcpOverridesService.ts Outdated
- Install rollback retries the promoted-tree deletion after stopping
  the plugin's MCP servers (a running server can hold the lock), and
  quarantines a still-undeletable tree into the staging root so global
  discovery cannot load it as an unmanaged plugin after a failed
  install.
- prunePluginOverrideKeys rejects a non-object override document root
  (a newer build's representation) the same way it rejects opaque
  owned-field shapes, keeping the uninstall tombstone retryable
  instead of retiring it against uninspectable content.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d18a2d3158

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/mcpServerManager.ts
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/mcpConfig.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.stories.tsx Outdated

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: d18a2d3158

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/agentPlugins/installService.ts

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d18a2d3158

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
- Recheck the workspace removal stop-epoch inside the synchronous MCP
  cache publication callback: a stopServers(workspaceId) landing while
  the awaited invalidation scan yielded found no cache entry to close,
  so publishing would resurrect processes for a removed workspace.
  Skip publication and close the late clients instead.
- Sort env entries in the update capability fingerprint: env is an
  unordered map, so an upstream property reordering must not be
  rejected as a capability change.
- Validate added plugin override keys PER FIELD: a stale key surviving
  only in toolAllowlist must not smuggle that key into enabledServers
  without discovery validation.
- Stories: accept a single props parameter instead of destructuring
  (repo rename-friendly convention).
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8a0da25b9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1664 to +1668
await this.closeInvalidatedInstancesThenPublish(
retriedInstances,
startupEpoch,
workspaceId,
(invalidatedRetryKeys) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recheck cache ownership inside retry publication

When a removal-style stopServers(workspaceId) lands after the currentEntry check but while closeInvalidatedInstancesThenPublish() is awaiting its scan, it deletes and closes the cached entry; this callback then merges the newly started clients into the detached existing object and returns their tools, leaving those processes with no cache owner to clean them up. Fresh evidence beyond the fixed cold-start publication path is that this timed-out retry path does not repeat its cache-entry or stop-epoch check inside the synchronous callback; recheck there and close/discard the retry instances when ownership changed (the active-lease restart path has the same publication window).

Useful? React with 👍 / 👎.

- Security: enforce an aggregate quota (100 MiB / 10,000 files, .git
  excluded, symlinks not followed) on every staged clone checkout
  before validation reads it — --depth 1 and the subprocess output cap
  do not bound checkout bytes from an untrusted remote.
- Consent preview discloses full stdio env assignments (quoted
  KEY='value'), not just key names: values like
  NODE_OPTIONS=--require=./payload.js change what executes without
  appearing in the argv.
- Uninstall data rollback re-invalidates the plugin's MCP prefix and
  removes a recreated (empty) data dir before restoring the staged
  original, so a late server launch cannot make the restore
  EEXIST-fail and strand the user's data in staging.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Unknown error
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffa0312519

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}
const sha = (await runGit(["-C", dir, "rev-parse", "HEAD"])).trim();
assert(isFullCommitSha(sha), "cloneResolved: rev-parse HEAD must be a full SHA");
await this.assertStagedTreeWithinQuota(dir);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Enforce the checkout quota before Git fills the disk

When a malicious or accidentally huge repository exceeds available disk space during checkout, this quota cannot protect the app because runGit(["clone", ...]) has already completed before the tree is measured here. Fresh evidence after the earlier staged-data finding is that the new check remains post-clone, so it can only reject and clean up if Git successfully materializes the entire tree; enforce the limit while cloning/checking out in a quota-constrained location instead.

Useful? React with 👍 / 👎.

Comment on lines +442 to +444
const stat = await fsPromises.stat(entryPath);
if (now - stat.mtimeMs > STALE_STAGING_MAX_AGE_MS) {
await fsPromises.rm(entryPath, { recursive: true, force: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Protect live staging directories from age cleanup

When an installed tree older than one hour has just been renamed to trashDir during uninstall and a concurrent preview calls createStagingDir(), the rename preserves the tree's old mtime, so this cleanup can delete the active rollback copy as “stale.” If data staging or the registry write then fails, restoreTree cannot restore the install, leaving a failed uninstall with a missing plugin tree; track active staging ownership or stamp newly staged trash deterministically rather than relying on inherited age.

AGENTS.md reference: AGENTS.md:L211-L211

Useful? React with 👍 / 👎.


Stdio plugin servers launch with the spec's `PLUGIN_ROOT` and `PLUGIN_DATA` environment variables; per-plugin data directories live under `~/.shux/plugin-data/`.

**Settings → Plugins** installs plugins from git into `~/.mux/plugins` (paste a git URL or `owner/repo[@ref]`). Before anything is written, a consent preview lists the plugin's manifest, every skill, and every MCP server command line. Installs are pinned to the resolved commit; update checks compare the tracked branch or tag against the pinned commit and never auto-apply. Applying an update replaces the plugin directory wholesale — local edits to a managed plugin directory are discarded — and restarts that plugin's running MCP servers. Uninstalling removes the directory, the registry entry, and the plugin's per-workspace server overrides, but keeps `~/.mux/plugin-data/` unless you opt in to deleting it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Document the active Shux plugin directories

On a normal Shux installation, managed plugins and their data live under ~/.shux/plugins and ~/.shux/plugin-data, while custom or legacy-compatible roots may differ, so this new paragraph directs users to two legacy ~/.mux locations that the installer does not use. Replace these paths with the canonical Shux locations or explain that they derive from the active Shux home.

AGENTS.md reference: docs/AGENTS.md:L22-L23

Useful? React with 👍 / 👎.

Comment on lines +509 to +510
if (branchSha !== undefined) {
return { ref, refType: "branch", sha: branchSha };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve the tracked ref kind when names are ambiguous

When a plugin tracks a tag and the remote later adds a branch with the same name while retaining that tag, both refs are returned here but the unconditional branch-first return makes checkUpdates() report that the tag became a branch and makes update() refuse to proceed. Resolve update checks using the entry's stored refType so a newly added same-name branch does not break a still-valid tracked tag.

Useful? React with 👍 / 👎.

Comment on lines +331 to +333
const parsed = AgentPluginInstallEntrySchema.safeParse(rawEntry);
if (parsed.success) {
entries.push(parsed.data);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Canonicalize duplicate managed plugin identities

When a corrupted or newer-written registry contains two schema-valid entries with the same plugin name, both pass this parser and strict reads accept them. update() then selects the first entry as its source but patches every raw entry with that name, leaving duplicate entries with different sources and the same SHA; subsequent update checks can show a badge that retries can never clear because update keeps returning early for the first entry. Detect and sanitize or reject duplicate names before list/check/mutation logic consumes them.

AGENTS.md reference: AGENTS.md:L110-L110

Useful? React with 👍 / 👎.

await this.removeDir(path.join(stagedDir, ".git"));

await fsPromises.mkdir(this.containerDir, { recursive: true });
await fsPromises.rename(stagedDir, targetPath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recover installs interrupted after promotion

When the Electron process exits or the machine loses power after this rename but before the registry write, the promoted tree survives without a registry entry. On restart it is shown only as unmanaged, assertNoCollision() blocks reinstalling it, and uninstall refuses it because it is not managed, so the user can recover only by manually deleting the directory. Distinct from a handled registry-write rejection, process termination bypasses the rollback catch entirely; persist a promotion journal or reconcile orphaned promoted trees on the next load.

AGENTS.md reference: AGENTS.md:L110-L110

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant