Skip to content

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

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

🤖 feat: Agent Plugins install/update UX (managed installs, v1)#3820
ThomasK33 wants to merge 23 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.

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

New AgentPluginInstallEntry schema persisted as a 'plugins' section in
~/.mux/config.json via Config's atomic writes. source.ref is the tracking
channel; lockedSha is what runs. Invalid entries are dropped lenient-on-read
(discovery stays the source of truth for what loads).
- discoverAgentPluginAt: public single-root discovery wrapper so the
  installer validates staged clones with the exact runtime validation
- extract normalizeRepoUrlForClone into src/node/utils/gitUrls.ts (shared
  with the project clone flow)
- sourceInput grammar: owner/repo[/subpath][@ref] shorthand + URL passthrough
- AgentPluginInstallService: stateless preview (temp shallow clone, consent
  payload with skills + MCP command lines), exact-SHA install with rollback,
  list (managed + unmanaged + missing), uninstall (override pruning,
  optional plugin-data purge), ls-remote update checks, swap-based update
- MCPServerManager.stopServersWithKeyPrefix: explicit plugin-server recycle
- plugins.* oRPC endpoints gated on the agent-plugins experiment
- PluginsSettingsSection: managed/unmanaged/missing list with update badges,
  Check for updates (on section open + explicit button only), two-phase Add
  flow (source input → consent preview listing every skill and MCP command
  line → install), inline uninstall confirm with unchecked-by-default
  'also delete stored plugin data' checkbox
- gated on the agent-plugins experiment (section, redirect, palette entry)
- mock oRPC client support + Storybook stories with play assertions
Also fix lint (unsafe any in registry normalization) and prettier.
@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
- Registry moved to standalone ~/.mux/plugins.json: older builds rebuild
  config.json from known fields on save (passthrough only affects schema
  validation), so a downgrade would drop an embedded registry; owning the
  file also lets writes THROW so install rolls back the promoted dir and
  uninstall/update surface persistence failures instead of silently
  succeeding (Q2's own contingency: 'migration to a separate file')
- Traversal safety: plugin-name grammar (shared via
  src/common/utils/agentPluginName.ts) enforced in the registry schema and
  asserted in targetPathFor, so a malformed entry named '.'/'..' can never
  resolve outside the container that uninstall deletes recursively
- Fallback clone: reset the staging dir before the branch-clone fallback
  (fetchExactSha leaves an initialized repo; git clone refuses non-empty)
- Keyboard rule: palette commands Install Agent Plugin… (opens the section
  with the add panel expanded), Check for Plugin Updates (toast + navigate),
  Update All Plugins (applies update-available; moved tags stay manual)
- New tests: registry survives config.json rewrites, traversal names dropped,
  registry-write failure rollback, SHA-fetch-refused fallback (file:// remote
  with uploadpack.allowAnySHA1InWant=false)
@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
- uninstall: stage the tree out (rename to staging) BEFORE the registry
  write; a locked/undeletable tree now fails cleanly with the install fully
  intact, and a failed registry write renames the tree back
- palette 'Install Agent Plugin…': mounted sections subscribe to the intent
  so invoking the command while Settings → Plugins is already open expands
  the add panel (initializer covers the fresh-mount path)
- section: mutation errors are re-asserted after refresh (refresh success
  cleared them); failed uninstall keeps the confirmation open
- palette check/update-all: per-plugin status:'error' entries no longer read
  as 'all up to date' — surface which plugins failed and navigate
@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
- palette 'Uninstall Agent Plugin…' with a managed-plugin select prompt;
  routes through the section's confirmation flow (plugin-data checkbox,
  destructive button) — the palette never uninstalls directly
- pluginsSectionIntents generalized to a typed intent bus (open-add-panel /
  confirm-uninstall / refresh); mounted sections subscribe, unmounted
  sections consume the buffered intent on mount
- 'Update All Plugins' publishes a refresh intent so an already-mounted
  section re-queries instead of showing stale versions/badges
- sourceInput expands ~/-relative local paths (git is spawned without a
  shell, so ~ never expands on its own)
@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
- update: stop the plugin's MCP servers BEFORE the old tree is renamed
  (live servers can lose files mid-swap on POSIX; open handles can fail the
  rename on Windows), and recycle again post-promote so content changed
  behind an unchanged command line still restarts — regression test snapshots
  the installed tree version at each recycle (pre-swap sees v1, post sees v2)
- uninstall: best-effort trash deletion (catch + log + leave for staging
  reclamation) so an undeletable staged tree cannot abort override pruning;
  update's replaced-tree deletion gets the same treatment — regression test
  forces EBUSY and verifies uninstall completes and reinstall works
- palette Update All / Check for Updates: check failures stay in the final
  summary even when other updates succeeded (mixed results toast as errors)
- section: update-check errors live in separate state from list/mutation
  errors, so the concurrent mount refresh can never clear an unreachable-
  remote warning (rendered as its own banner)
- sourceInput: expand ~\-style Windows home paths, not just ~/
@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
- P1 MCP startup race: stopServersWithKeyPrefix records an epoch-stamped
  prefix invalidation; getToolsForWorkspace snapshots the clock before
  reading config and closes matching instances at every publish point
  (fresh start, timed-out retry, mid-stream restart) instead of publishing
  them — an update/uninstall swap during an in-flight startup can no longer
  leave an old-tree server running. Race test gates startServers, swaps
  mid-flight, and asserts the instance is closed, not published
- registry rewrites are raw-preserving: mutations operate on the raw entry
  list (per-element validation on read, matched by name on write), so
  entries/fields from newer builds survive install/update/uninstall on this
  build; lifecycle test seeds an archive-source entry + unknown field
- managed list rows keep registry identity (update/uninstall look up by it);
  manifest-name drift is surfaced in the description instead of breaking
  repair from Settings
- update revalidates the resolved ref kind before cloning (deleted branch
  replaced by same-name tag → clear error, registry untouched)
- palette Check for Plugin Updates publishes a refresh intent so a mounted
  section's badges match the toast
- pinned phone-viewport story variant (Pixel matrix + mobile1 global) for
  the narrow-width row layout
@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
- P1: instances removed by mid-startup invalidation are queued in the
  timed-out retry list at every publish point — the entry is published under
  the unchanged full config signature, so without a retry marker the cached
  path would serve the reduced map and the updated plugin's tools would stay
  unavailable indefinitely; the race test now asserts the subsequent
  getToolsForWorkspace restarts the server from the new tree
- P2: stopServersWithKeyPrefix closes ONLY matching instances, preserving
  the rest of the workspace cache (an unrelated healthy client is no longer
  torn down under a live lease/mid tool call); removed keys go through the
  same retry markers; test covers two servers + a held lease
@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
- P1 uninstall re-invalidation: issue a second stopServersWithKeyPrefix
  AFTER the tree+registry removal (mirroring update's post-promote stop) so
  a startup that snapshotted the new epoch but discovered the plugin before
  the rename cannot keep a server running from the removed tree — test
  asserts the recycle pair observes tree-present then tree-gone
- P1 remove manual useCallback: PluginsSettingsSection handlers are plain
  functions (React Compiler repo); mount/subscription effects key on [api]
  with documented eslint-disable per repo precedent
- P2 nested raw preservation: update patches only owned fields (lockedSha,
  updatedAt, manifest version/description) into the RAW entry instead of
  spreading the Zod-parsed entry — unknown metadata inside source/manifest
  survives downgrade round-trips (tested)
- P2 stale update-check responses: generation counter; only the latest
  check commits updateChecks/checkingUpdates state
- P2 plugin-data staging: when deletion is requested, the data dir is
  staged out BEFORE the registry commit; failure aborts the uninstall with
  the row intact so the cleanup can be retried (tested), rollback restores
  both tree and data on registry-write failure
@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
- collision checks compare RAW entry names: an entry this build cannot
  parse (newer build's source kind) still owns its name, so install cannot
  filter it out and replace it during a downgrade (tested)
- only ENOENT means empty registry: other read failures (e.g. mode-000
  file) block mutations with a repair message instead of letting the atomic
  write replace the unreadable file and erase its entries; lenient reads
  still degrade to unmanaged rows (tested)
- install rollback test now injects the write failure via spy (the old
  dir-at-registry-path trick trips the strict read first)
@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
- Update All: moved tags join the final summary ('Tag moved for X — review
  in Settings → Plugins') and taint the toast; tag-moved-only results no
  longer report 'All plugins are up to date'
- Update All: the refresh intent publishes before any early return, so a
  mounted section picks up newly discovered moved tags / check errors even
  when no branch update applied; tag-moved / check-failure outcomes also
  navigate to 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: 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
uninstall runs the post-commit MCP invalidation BEFORE override pruning:
pruneWorkspaceOverrides can throw from getAllWorkspaceMetadata (outside its
per-workspace catch), and a pruning failure must not skip the invalidation
that catches servers started from the removed tree mid-uninstall (test:
metadata enumeration rejects → both stops ran, uninstall committed)
@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: 8581764548

ℹ️ 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/node/services/agentPlugins/installService.ts Outdated
- refresh() gets the same request-generation guard as checkForUpdates: an
  older overlapping list response can no longer resurrect removed rows or
  stale versions over a newer refresh
- uninstall enumerates its override-pruning targets (workspace metadata)
  BEFORE committing anything: enumeration is the only pruning step that can
  fail wholesale, and post-commit it would strand stale enabled-server
  overrides with no Settings row to retry from (reinstall reuses the same
  instance ID and would silently re-enable servers); per-workspace pruning
  stays best-effort post-commit (test: enumeration failure aborts fully
  intact with zero stops, retry completes with both invalidations)
@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: a22a9e0cc3

ℹ️ 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
failed per-workspace override prunes persist a retryable tombstone
(pendingOverridePrunes in the plugins.json envelope): retried on section
open (list), and a reinstall of the same name is hard-gated on the pending
prune for its instance-ID prefix — retry-then-refuse, so a stale
enabledServers key can never silently re-enable a reinstalled plugin's
server. Test: unavailable checkout → uninstall commits with tombstone,
reinstall refuses; checkout recovers → override pruned, tombstone cleared,
reinstall succeeds
@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: 09fbbc29e6

ℹ️ 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
Comment thread src/node/services/agentPlugins/installService.ts
- P1: retryPendingOverridePrunes runs its read-modify-write under the
  exclusive mutation queue — a section-open retry can no longer clobber a
  concurrent install/update/uninstall with its stale registry snapshot
  (race test: gated prune during list + concurrent install → entry survives,
  tombstone cleared)
- P2: the uninstall COMMIT write carries a pessimistic tombstone for every
  workspace to prune; the post-prune write only shrinks it (best-effort),
  so losing that write leaves the safe over-broad record instead of no
  record (test: prune + shrink-write both fail → tombstone durable,
  reinstall gated)
- P2: tombstone retries reconcile workspace IDs against current metadata
  and drop deleted workspaces — a permanently-missing checkout can no
  longer block reinstall forever (test: dead-ID tombstone → reinstall
  succeeds, tombstone retired)
@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: d005e8c877

ℹ️ 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
Comment thread src/node/services/agentPlugins/installService.ts Outdated
- WorkspaceMcpOverridesService.removeOverridesFile checks the rm exit code
  and throws: setOverridesForWorkspace must reject when clearing overrides
  fails, or the tombstone machinery would classify the prune as successful
  while the stale enabledServers key survives
- tombstone rewrites are raw-preserving: unrecognized tombstone variants
  pass through verbatim and recognized items keep unknown fields when their
  workspaceIds shrink (test: future variant + extra field survive a full
  uninstall cycle)
- the post-uninstall tombstone shrink re-reads the registry STRICT inside a
  try/catch: a lenient read degrading transient corruption to an empty
  document would have rewritten plugins.json with an empty plugin list; on
  failure the pessimistic commit-write tombstone simply stays
@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: 28d8bc6d0e

ℹ️ 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
- Consent preview now resolves symlinked skill dirs with allowMissing
  containment (matching runtime assertSkillDirValid), so escaping
  symlinks surface a warning instead of hiding behind ENOENT while
  in-root symlinked skills are disclosed.
- Plugin location/source lines wrap with break-all so max-length
  separator-free names cannot overflow the card at phone widths;
  pinned phone story covers a 64-char name with a scroll-width
  assertion.
@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: c62f9345a3

ℹ️ 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
Stale Workspace MCP dialog snapshots could restore plugin:<instanceId>:
override keys that an uninstall had just pruned, silently re-enabling a
reinstalled plugin's MCP server.

- workspace.mcp.get now returns { overrides, revision } where revision
  is a content hash of the normalized overrides.
- workspace.mcp.set requires expectedRevision and rejects with a
  conflict error when the stored overrides changed since that read;
  saves are serialized through a write queue so check-and-set is atomic.
- WorkspaceMCPModal passes the loaded snapshot's revision on save, so a
  stale save surfaces 'settings changed while this dialog was open'
  instead of clobbering the prune.
- The uninstaller's override prune passes expectedRevision too and
  re-reads + re-filters on conflict (bounded retries), so a concurrent
  dialog save cannot interleave with the prune's read-modify-write.
@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

this.workspaceServers.set(workspaceId, {

P1 Badge Make invalidation and cache publication atomic

Fresh evidence after the epoch fix is the await between the final epoch scan and this cache publication. If an update/uninstall's post-swap stopServersWithKeyPrefix continuation runs during that yield, it records the new epoch and scans before this entry exists; this continuation then publishes a server that discovered the old tree without rechecking the epoch, allowing an uninstalled or replaced plugin process to remain active. Recheck synchronously at publication or otherwise serialize the check-and-publish step with prefix invalidation.

AGENTS.md reference: AGENTS.md:L149-L149

ℹ️ 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".

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