Skip to content

🤖 feat: manage installed plugin component selections - #4213

Merged
ThomasK33 merged 12 commits into
mainfrom
plugin-components-h6c8
Sep 11, 2026
Merged

🤖 feat: manage installed plugin component selections#4213
ThomasK33 merged 12 commits into
mainfrom
plugin-components-h6c8

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Sep 11, 2026

Copy link
Copy Markdown
Member

Summary

Replace installed-plugin Add components with Manage components. Add or remove imported skills and MCP servers, including clearing both groups, without uninstalling the plugin or deleting its files, data, or saved workspace MCP preferences.

Implementation

  1. Replace the internal additive API with an atomic setComponents operation. Validate the reviewed Git SHA, full-tree content receipt, and nullable raw selection baseline before writing the desired set. Preserve legacy import-all behavior on no-op.
  2. Use the existing managed registry as MCP component-policy authority. Gate held tools and actual stdio/HTTP/SSE startup under the existing admission locks, including retries/fallback. Retire only removed clients; retained plugin and unrelated clients remain connected.
  3. Persist before runtime reconciliation. No-op saves do nothing. Failed cleanup reports a saved-state warning and remains retryable. Re-adding a server honors its saved workspace opt-in; new imports remain disabled until opted in.
  4. Add editable imported checkboxes, group selection controls, pending-change counts, empty saves, cancellation, and conflict/lost-response recovery. Refresh both editor and card counts after a stale save is rejected. Update the palette, full-App stories, and existing user documentation.

Validation

Verified on f2609e183f50f3a11af1215ec4342f9b45a74667:

  • 930 unit tests passed, one Windows-only local skip, across 25 affected/sibling suites. 15 real-IPC/full-App UI tests passed.
  • make static-check, make static-check-full, and git diff --check passed on this head. The independent final code review found no concrete blocker. Current-head external CI and code/security reviews remain separate merge gates.
  • Test-first regressions exercise actual stdio/HTTP/SSE/fallback launch admission, delayed launch/lock release, live MCP admission during blocked inventory hashing, complete real uninstall/reinstall with identical receipt bytes, and pending installer journals. Read-only inventory uses the existing journal/epoch bracket; save-time SHA/content/selection validation remains locked.
  • UI regressions cover an acknowledged save followed by failed inventory confirmation, and two superseding-writer schedules. Counts/current checkboxes refresh, conflicts do not show success, and nothing is automatically resubmitted.
  • Recorded isolated, provider-free desktop and 375×812 UAT verified removal, mixed re-add, current counts, open-panel MCP availability, and stale-tab rejection. Keeper PID 353850 survived throughout; echo 353844 disappeared on removal and was replaced only on re-add by 383254. Stale rejection changed neither PID nor registry, workspace MCP preferences, data, or observed source hashes. Mobile document width stayed 375.
  • Desktop/mobile recordings and six full-App Storybook interactions cover 668094672; UI code is unchanged on this head. Recorded desktop 240.3s and mobile 121.0s final frames were checked. Deterministic failure-injection races are automated-test evidence, not claimed as manual reproductions.
  • Six test-first startup-close regressions cover initial, additive, retry, leased restart, retired-only, and re-add publication. A rejected close remains tracked by object identity without active tools/stats; existing reconciliation, idle, or prefix cleanup retries it successfully. Retained clients survive and re-addition uses a fresh client.

Current-head startup-cleanup regression evidence

Recorded the actual six-case failure-injection test run on f2609e183: 6 passed, 0 failed, exit 0. The accelerated recording is 5.0s; its final frame was checked. This is recorded automated regression evidence, not a manually induced process-close failure.

Six startup-cleanup regressions pass on the final commit

Recorded startup-cleanup regression run:

regression.webm

Desktop/mobile UAT (668094672; UI unchanged)

Saved mixed selection with retained MCP connection

375px stale-save rejection with refreshed selection and counts

Desktop removal, mixed re-add, and open-panel runtime observations:

desktop-runtime.webm

Mobile stale-save rejection and refresh:

mobile-conflict.webm
Earlier implementation evidence

Earlier validation

  • Local affected unit and sibling suites on the rebased head: 717 passed, one platform skip.
  • Real IPC/full-App UI: 10 passed.
  • Full-App Storybook interactions: six passed after warming the server. Fresh-server navigation initially exceeded the default timeout; unchanged warm runs passed.
  • make static-check, make static-check-full, and git diff --check passed again on the rebased publication head before push.
  • Recorded isolated, provider-free UAT covered desktop and 375px layouts, keyboard operation, add/remove/re-add, no-op, Cancel, empty selection, persistence, and two-tab conflicts.
  • Actual process evidence: keeper PID 3152939 survived echo removal and re-add. On the corrected build, keeper PID 3413952 survived the competing removal/re-add; rejecting the stale save changed neither PID nor registry/configuration bytes.
  • Recordings show the implemented 52046f230 build. Its feature commits were rebased without conflicts onto current main before publication; the feature patch is unchanged.

Screenshots

Manage components with pending additions and removals

Desktop and mobile conflict recovery

Rejected stale save refreshes the editor and surrounding card counts

375px conflict recovery with matching counts and disabled Save

The attached videos cover selective removal/re-add, Cancel/empty selection, and corrected mobile stale-save recovery.

Earlier recordings

final-mixed-selection.webm
final-cancel-empty.webm
corrected-mobile-conflict.webm

Runtime boundary

Only skills and MCP component selection changes. Initial install, update, uninstall, agents, hooks, workflows, and slash commands retain their existing behavior. Calls already admitted before the selection commit may finish normally. Idle sibling processes reconcile at their next MCP boundary; immediate cross-process process termination is not promised.

Regression risk: Managed MCP admission and cleanup are the sensitive paths. Real mutations can temporarily deny admission; read-only inventory must not. Deterministic transport/installer races and retained-process UAT cover those boundaries. No new persisted fields, journal files, or locking subsystem are introduced by these review fixes.


📋 Implementation Plan

The plan below is the accepted implementation snapshot. Publication and conditional merge-queue submission were authorized afterward.

Reversible installed-plugin component management

Goal and scope

Replace the installed-plugin Add components panel with Manage components. Users can add components or deselect previously imported skills and MCP servers, then explicitly save the new selection.

Assumption: removing a component means removing it from Xum's available plugin components, not deleting the plugin's installed source files. Keeping the plugin installed makes re-adding possible without fetching again. Empty selections are valid. Initial installation remains unchanged. Scope matches the existing chooser: skills and MCP servers only; agents, hooks, workflows, and slash commands are unchanged.

Preserve workspace MCP enablement, tool allowlists, identity, and data. New MCP imports remain disabled without a workspace opt-in. Re-adding a previously enabled server can restore that saved opt-in; the panel must say so.

Verified foundations

  • PluginsSettingsSection.tsx: ComponentChooser locks imported checkboxes; AddComponentsPanel models additions only.
  • agentPlugins/installService.ts: addComponents unions selections under runExclusive; the registry already supports explicit empty allowlists.
  • agentPluginInstalls.ts: absent importedComponents means legacy import-all. Cancel/no-op must preserve that distinction.
  • getComponents returns the installed SHA, full-tree content hash, inventory, and current imported selection. SHA/hash do not change when another panel changes only the registry selection.
  • Skill discovery and loadPluginMcpServers already filter by the selected allowlist. Active MCP connections require additional targeted reconciliation; blanket plugin teardown would disrupt retained servers.

User experience

  1. Rename the installed-plugin action to Manage components. Keep the existing inline panel; no new settings page or modal.
  2. Initialize checkboxes from the current imported set. All available skills and MCP servers are toggleable. Select all and Clear operate on the whole group.
  3. Show one concise pending-change summary, such as “Add 1 skill · Remove 1 MCP server.” Use Save changes and Cancel. Disable Save only while busy or unchanged—not when the selection is empty.
  4. Saving updates the card counts. Cancel discards the draft. Reopening loads current state. An empty selection explains that no skills or MCP servers are imported, while the plugin remains installed. Uninstall remains separate.
  5. Explain that deselection preserves source files, data, and workspace MCP preferences. New imports need workspace enablement; re-added servers retain previous preferences.

Implementation sequence

Deliver two dependent, reviewable increments. Stabilize increment 1 before exposing removal in increment 2. No PR creation or publishing is authorized by this plan.

1. Make MCP component removal safe at runtime

Estimated net product code: +180–300 LoC. No UI changes in this prerequisite.

Files: src/node/services/mcpServerManager.ts, src/node/services/agentPlugins/registry.ts, src/node/services/agentPlugins/mcpConfig.ts, and CoreServicesRootLive in src/node/services/di/layers/core.ts (the existing pluginInvalidation wiring).

  1. Write failing runtime tests first. Cover removal with a retained sibling and unrelated server, held tools in a second manager sharing the registry, pending startups, active leases, and simultaneous add/remove with equal counts.
  2. Use a canonical component-policy snapshot from the existing atomic plugins.json as the cross-process authority. Include managed installation identity and selected MCP names. Extend the existing stable-scan/call-time invalidation seam; do not create another epoch file, use mtime-only checks, or bump the global tree epoch.
  3. Recheck that policy at tool authorization and across config discovery/startup publication. On a policy change, reconcile against fresh configuration and retry stale publication. Carry managed-policy provenance from discovery; use the unqualified info.plugin.serverName, not the composite key, for allowlist membership. Reuse existing managed-home/alias identity rules. A missing registry entry must not reclassify an already managed server as unmanaged. Unreadable managed policy fails closed without restricting genuinely unmanaged plugins. Cover those boundaries in tests.
  4. Extend the existing additive-retention path to handle plugin-selection removals and mixed additions/removals when retained configs are unchanged. Compare sets, not counts. Reuse computeSignatureEntries; do not manually patch signature strings. Leave existing behavior for unrelated configuration changes intact.
  5. Revoke removed keys from tool authorization, availability, and retry queues. Fence in-flight startup publication. Retire only removed clients, respecting existing lease cleanup. Retained plugin and unrelated clients must stay connected. Never mark a deselected server for retry.

Runtime contract: An invocation whose call-time authorization check occurs after the selection commit must reject a deselected server, even through an old tool object in another process. “Admitted” means an actual invocation passed that gate—not that its tool definition was advertised. Already-admitted calls may finish or fail normally. Existing prompts cannot be retroactively edited. Idle sibling processes reconcile at their next MCP boundary; immediate cross-process process termination is not promised. Keep registry reads within the existing bounded gate and check their latency in the targeted runtime tests.

Gate: The targeted MCP/registry/discovery tests pass. Prove retained connection identity, not merely that a new connection works. No API for MCP removal ships before these checks are green.

2. Ship replacement selection and the management panel

Estimated net product code: +90–170 LoC. Depends on increment 1. Includes backend, UI, tests, and updated user docs.

  1. Write behavioral tests first. Exercise subset replacement, mixed add/remove, empty selection, cancel/no-op, legacy import-all, invalid component names, stale selection, same-SHA file edits, and failed persistence.
  2. Replace the internal additive API with agentPlugins.setComponents. Update src/common/orpc/schemas/api.ts, src/node/orpc/router.ts, and src/node/services/agentPlugins/installService.ts. Frontend/backend are version-synchronized, so do not retain an unused additive compatibility endpoint.
    • Input: plugin name, expectedLockedSha, expectedContentHash, required nullable expectedImportedComponents, and desired importedComponents.
    • null baseline means the legacy absent field. Compare selections as normalized sets; preserve absent versus explicit empty.
    • Under runExclusive, read strict registry state, check all three baselines, validate desired names against the verified installed inventory, and atomically replace only importedComponents.
    • Reuse full-tree content-consent checks, including verification around inventory reads. No remote fetch, tree rewrite, data deletion, identity change, override pruning, or migration.
    • Preserve legacy import-all on no-op. A changed legacy selection becomes an explicit allowlist. Later plugin updates preserve that selection and do not auto-import new components.
  3. Publish disk state before runtime effects. After persistence and release of the mutation lock, invalidate skill availability and reconcile local MCP managers through increment 1. Reconciliation reads current policy rather than replaying a stale removal list, so rapid remove→re-add cannot close a newly valid client. A failed write changes neither selection nor runtime. If cleanup fails after persistence, do not roll back or pretend nothing saved: show the saved selection, surface a cleanup warning, and retry retirement at the next reconciliation. Authoritative call-time gating must still deny removed tools. Keep any result-envelope extension local to this operation.
  4. Update the UI in PluginsSettingsSection.tsx. Rename AddComponentsPanel, use a full-selection draft, unlock imported rows, and implement the interaction above. Keep the shared initial-install chooser behavior intact. Capture the raw baseline separately from the displayed effective selection.
    • Refetch after success; clear pending state on close/reopen.
    • On a stale SHA/content/selection conflict, refresh inventory and baseline, reset the draft, and require a new explicit Save. Never auto-resubmit a stale choice.
    • On ordinary failure, retain the draft and show an actionable error. If a response is lost after a possible commit, refetch before describing the persisted result.
  5. Update access paths and fixtures. Rename the command-palette label and matching intent in src/browser/utils/commands/sources.ts, pluginsSectionIntents.ts, and their command-ID definition/callsites. Preserve Tab/Space/Enter operation. Update src/browser/stories/mocks/orpc.ts and src/browser/stories/App.pluginImports.stories.tsx to use replacement semantics.
  6. Update existing docs. In docs/agents/agent-skills.mdx, replace the one-way-import limitation. Explain deselection, preserved data/preferences, empty selections, and re-add behavior. Do not add a new documentation page.

Gate: Backend, real-IPC UI tests, full-App Storybook interactions, and static checks pass. Opening the panel without edits must not mutate the registry. Initial installation, update, and uninstall regression tests remain green.

Acceptance criteria

  1. Users can add and remove imported skills/MCP servers in one panel, including removing all of them without uninstalling.
  2. Save persists exactly the chosen set. Cancel and no-op leave disk and runtime untouched. Reopening and restarting show the saved choice.
  3. Deselected skills disappear from fresh skill discovery. Deselected MCP tools are unavailable, including through stale held tools; retained servers are not restarted.
  4. Installed files, instance identity, data, saved MCP preferences, and unrelated plugins are unchanged. Newly added MCP servers do not gain unsolicited workspace enablement.
  5. Concurrent edits and changed installed content cannot silently overwrite the user's reviewed selection. Invalid input and write failure leave the prior state intact.
  6. Desktop and narrow layouts support keyboard use without horizontal overflow. Initial install/update/uninstall retain their current behavior.

Validation and dogfooding

Automated gates

Run src/** suites with Bun. Run tests/** suites with Jest. Include sibling suites for every touched production module.

bun test src/node/services/agentPlugins/installService.test.ts \
  src/node/services/agentPlugins/registry.test.ts \
  src/node/services/agentPlugins/discovery.test.ts \
  src/node/services/agentPlugins/mcpConfig.test.ts \
  src/node/services/mcpServerManager.test.ts \
  src/node/services/agentSkills/agentSkillsService.test.ts

TEST_INTEGRATION=1 bun x jest tests/ipc/agentPlugins.test.ts \
  tests/ui/config/pluginImports.test.ts --runInBand

# Start a persistent Storybook server with make storybook, then:
bun x test-storybook --url http://127.0.0.1:6006 \
  --maxWorkers 1 App.pluginImports.stories

# Run after tests, not concurrently with temporary test-module creation.
make static-check
make static-check-full

Extend full-App stories only. Cover saved removals, empty selection, and conflict recovery. Pin the phone story with parameters.pixel.matrix.viewports: ["phone"] and matching globals.viewport. Keep breakpoint assertions valid in the desktop-sized Storybook test runner by using the existing fixed-width/guarded pattern.

Hands-on walkthrough

  1. Create a disposable local Git plugin fixture outside the checkout, following installService.test.ts fixture patterns. Include two skills and two harmless local MCP servers with observable process identities. Add a separate temporary workspace/project. No provider credentials are needed.
  2. Start an isolated server on free ports:
    make dev-server-sandbox \
      DEV_SERVER_SANDBOX_ARGS="--clean-providers --clean-projects"
    Use the printed URL/root. Enable Agent Plugins in that sandbox. Install the local fixture through Add plugin using its absolute Git path. Do not seed or edit the personal Xum root.
  3. Load agent-browser skills get core, open the sandbox, and start a video recording before interaction. Use snapshot -i for functional checks. Import a subset; enable one imported MCP server in the test workspace.
  4. Manage the plugin: add another component, deselect an existing one, save, reopen, and reload. Verify counts and actual skill/MCP availability. Verify the retained MCP process did not restart. Re-add the previously enabled server and verify the documented saved-preference behavior.
  5. Test Cancel, no-op, Clear both groups, and re-add after an empty selection. Open two panels in separate browser tabs; save one, then verify the other's stale save is rejected and refreshed.
  6. Repeat at desktop and approximately 375px width. Use the Storybook manager's pinned phone variant as the separate visual gate. Capture screenshots of pending removals, the saved/empty state, and phone layout. Stop the recording and use attach_file for screenshots and video.
  7. Stop only owned sandbox/browser/Storybook processes. If GitHub publication is later requested, upload the evidence with gh ... --attach; otherwise keep delivery local.

Final gate: Run the complete relevant tests and static checks on the final code. Get one clean-context independent readiness recommendation from the diff, tests, and dogfood evidence. Record the verdict and any concrete blocker. Do not claim implementation success if required checks or evidence are missing.

Planning review disposition

One independent review accepted the UI/API design and recommended reducing runtime scope. The plan limits reconciliation to plugin-selection changes and makes runtime tests a prerequisite. Retain cross-process checks: process-local invalidation alone misses held tools in sibling processes. Verified existing call gates already await disk-backed epoch/override readers; extend that bounded seam rather than introducing a separate synchronization system. Planning left the source tree clean. Implementation and test execution remain pending.

Total size and exclusions

Recommended approach: approximately +270–470 net product LoC, excluding tests, stories, and docs. This includes the runtime protections; merely unlocking checkboxes is not sufficient. Re-estimate at the first gate if the existing reconciliation seam cannot support the bounded change.

No new component types, plugin-file deletion, workspace-setting cleanup, generic plugin lifecycle framework, or unrelated MCP refactor. Planning is read-only; the commands above are implementation-time gates, not tests already run.


Generated with xum • Model: coder:openai/gpt-6-astra • Thinking: xhigh • Cost: $264.74

Use canonical plugins.json selection snapshots at discovery, startup, and
call authorization boundaries. Preserve retained server connections and
retire only removed components, deferring leased clients until release.
Remember managed installation provenance so missing registry rows cannot
reclassify hidden sibling components as unmanaged.

Validation: 583 unit tests passed (1 Windows-only skip); typecheck, touched
ESLint, Prettier, and git diff --check passed. No UI or public API changes.

---

_Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$4.69`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=4.69 -->
Replace additive imports with full, consent-bound selections for installed plugins.
Preserve plugin trees, data, identities and workspace MCP preferences while
reporting post-save cleanup failures as warnings. Add reversible inline
management, conflict/lost-response recovery, full-App stories and documentation.

Validation: 591 unit/sibling tests passed (1 platform skip), 10 IPC/UI tests,
6 Storybook interactions, make static-check and make static-check-full passed.
Recorded desktop/mobile sandbox UAT remains with the integrating parent.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh -->
Skip runtime reconciliation when component selection is effectively unchanged.
Keep failed retired clients owned for retry, report cleanup failures from
explicit saves, and leave automatic MCP boundaries non-disruptive.

Validation: 589 tests passed (1 Windows-only skip), typecheck, touched
ESLint, formatting, and diff checks passed.

---

_Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$24.47`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=24.47 -->
Keep card counts in sync with the refreshed editor after a stale selection
is rejected. Cover both skill and MCP counts with the real-IPC UI regression.

Validation: 716 unit tests (1 platform skip), 10 IPC/UI tests, 6 warmed
Storybook interactions, static-check, and static-check-full passed.

---

_Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$76.33`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=76.33 -->
@mintlify

mintlify Bot commented Sep 11, 2026

Copy link
Copy Markdown

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

Project Status Preview Updated
Mux 🟢 Ready View Preview Sep 11, 2026, 12:15 PM

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

@chatgpt-codex-connector

This comment has been minimized.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security 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: 6deceb6213

ℹ️ 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/mcpConfig.ts
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/node/services/mcpServerManager.ts
Comment thread src/node/services/mcpServerManager.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 Security Review

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

Reviewed commit: 6deceb6213

ℹ️ 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/mcpServerManager.ts Outdated
Refresh availability consumers and outer plugin counts when the selection write
loses its response and the confirming inventory read also fails. Retain the
uncertain status and pending draft, and do not invalidate definite failed saves.

Add real-IPC full-App regressions for persisted removals and definite failures,
including availability, counts, reopen behavior and no automatic resubmission.

Validation: 11 plugin UI tests passed; make typecheck, scoped ESLint, Prettier
and git diff --check passed. Full integration/static gates remain parent-owned.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$24.35`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=24.35 -->
Extend the installed-plugin regression to prove that updates preserve raw
selections for absent components, unavailable desired names remain invalid,
and explicit management replaces those selections. A later update restoring
the component must not silently re-import it. Document the Clear/empty-consent
rationale beside effectiveImports without changing selection semantics.

Validation: 134 installer tests and 11 full-App plugin UI tests passed;
make typecheck, scoped ESLint, Prettier and git diff --check passed.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$36.19`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=36.19 -->
Stop retired leased clients during matching plugin tree invalidations without
adding deselected restart candidates. Use existing idle ticks to retry failed
retirement before evicting its ownership. Recheck managed component policy
after named-test preparation and before admitting the connection test.

Validation: manager and plugin/config/DI sibling suites, repository typecheck,
touched ESLint, formatting, and diff checks passed.

---

_Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$37.78`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=37.78 -->
Hold the shared plugin mutation lock from a fresh policy read through synchronous tool, prompt, and named-test dispatch. Try it under the override fence so a plugin writer can still prune overrides; deny managed admission when the fence is missing or contended.

Cover real old-inode reads against component setters, override-prune progress, and exactly-once cleanup after read failure, timeout, abort, and late lock acquisition.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$73.96`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=73.96 -->
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security 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: f5cc72a292

ℹ️ 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/node/services/mcpServerManager.ts
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/mcpServerManager.ts
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.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: f5cc72a292

ℹ️ 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/mcpServerManager.ts
@ThomasK33

ThomasK33 commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

Status: merge blocked; review loop paused

Current head: f5cc72a292ad2f485b9cf1340487002ed04ff781.

Functional CI and the final-head local/UAT gates passed, but the latest code/security reviews found four verified defects. This reaches the six-round review limit, with further findings in the same runtime area. No merge-queue submission, new fixes, thread resolutions, or additional review requests were made after this cycle.

Finding disposition

Finding Assessment / next action
3989562090 Blocker. Normal stdio/HTTP/SSE launches lack component-policy admission after semaphore/preparation waits. Post-start cleanup cannot undo execution or credential-bearing initialization.
3989559740 Blocker. Opening the read-only inventory holds the mutation lock and causes concurrent managed MCP admissions to fail. Inventory consistency must remain intact when fixing this.
3989559728 Valid UI defect. A confirmed write followed by failed inventory refresh does not refresh the parent card.
3989559750 Valid UI defect. A concurrent post-save overwrite can be shown as a successful active selection instead of a conflict.
3989559711 The supplied schedule starts the skill read before the commit. The accepted contract covers fresh discovery, not retroactive prompt/content removal. No post-commit read violation is demonstrated.
3989559718 The supplied schedule launches/publishes before removal. Delayed sibling cleanup is within the next-boundary contract; later invocation is still policy-gated. This does not establish the same violation as the late-launch blocker above.

All six new threads remain open while this pause is resolved. None of these deferred defects is claimed fixed.

Proposed bounded continuation

  1. Keep this PR unmerged. Reassess the existing launch/admission and inventory-lock interaction first. Fence actual launch/retry/fallback, including the existing bounded delayed-stdio behavior, without a new generation file or locking subsystem. Add deterministic transport-level and concurrent-inventory regressions.
  2. Handle the two UI confirmation defects as a separate small change, with real-IPC tests. These are owned defects of this PR, not unrelated feature work.
  3. After explicit approval to continue beyond the review limit, rerun the final gates and obtain fresh code/security approval before queueing. If a safe runtime fix requires new persistent state or a subsystem, reduce/split scope instead of expanding this PR.

The independent readiness pass found no blocker from the earlier evidence. The later external findings supersede that recommendation. The strategic advisor recommends this pause, not an unreviewed late security patch.

Verified evidence on this exact head (not a readiness claim)

  • 758 unit tests passed, one Windows-only local skip; 12 IPC/UI and 6 Storybook checks passed. make static-check, make static-check-full, and git diff --check passed. Remote functional CI, including Windows tests, passed. Codex Comments and the umbrella Required check remain failed; review is not clean.
  • Isolated provider-free UAT: keeper PID 53525 survived removal and mixed re-add; echo changed from 53532 to 82240 only on re-add. Stale rejection preserved both PIDs, registry/config bytes, source hashes, and plugin data. At 375×812, document width remained 375.
  • The new recordings contain the final actions: desktop 157.6s; mobile stale conflict 117.0s. Browser, recorders, and owned sandbox servers are stopped.
Final-head UAT screenshots and recordings

Mixed selection saved with retained MCP sibling

375px stale save rejected and counts refreshed

Desktop removal and mixed re-add recording:

runtime-desktop.webm

Mobile stale-save conflict recording:

mobile-stale-conflict.webm

Generated with xum • Model: coder:openai/gpt-6-astra • Thinking: xhigh • Cost: $166.82

Reuse the component writer try-lock after the override fence so queued
stdio and remote startups cannot launch a removed managed component.
Retain the bounded stdio abort and remote initiation lock lifetimes.

Validation: 674 tests passed, one Windows-only skip; typecheck, targeted
ESLint, Prettier and diff checks passed.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh -->
Bracket component inventory with the existing discovery journal/epoch gate
instead of holding the plugin writer lock across full-tree hashes. Reject
installer transactions overlapping the scan, including complete reinstall
with an unchanged final content receipt. Keep component updates under the
existing SHA, content-hash and selection compare-and-swap lock.

Validation: 678 tests passed, one Windows-only skip; final make static-check
passed with one ESLint worker. Regression tests reproduce both contention
and concurrent-installer receipt invalidation.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$103.63`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=103.63 -->
Refresh outer plugin counts when an acknowledged selection write cannot be
confirmed by the inventory read. Confirm the current tree and raw persisted
selection before showing success; if another writer supersedes the save,
show its current selection and counts without automatically resubmitting.

Add deterministic full-App regressions for failed confirmation and subsequent
writers, including restoration of the original baseline and explicit retry.

Validation: 15 plugin UI/IPC tests passed; make typecheck, scoped ESLint,
Prettier and git diff --check passed. Both defects were reproduced test-first.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$50.51`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=50.51 -->
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security 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: 6680946720

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

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 6680946720

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.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 6680946720

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.

@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: 6680946720

ℹ️ 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
Transfer deselected startup clients whose close fails into the published
workspace entry's existing retired-client set. Cover initial, additive,
timed-out retry and leased restart publication without exposing retired
clients or reusing them when a component is re-added.

Validation: six deterministic regressions failed before the fix; 684
manager/sibling tests passed with one Windows-only skip. Sequential
make typecheck and make static-check passed.

---

_Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$119.29`_

<!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=119.29 -->
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: f2609e183f

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

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: f2609e183f

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.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: f2609e183f

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.

@ThomasK33

Copy link
Copy Markdown
Member Author

The Codex aggregate summary still includes a historical advisory appendix. Both linked findings are fixed and their actual threads are resolved: selection-read fence and normal MCP launch fence. The latter’s summary badge is stale.

For unchanged commit f2609e183f50f3a11af1215ec4342f9b45a74667, code review is clean, fresh security review is clean, both summary rows are completed, and all 14 review threads remain resolved after the six-minute late-finding audit.

I am marking only that stale aggregate comment as resolved (minimized). The repository’s comment gate otherwise treats any historical findings appendix as an unresolved regular comment. No finding or clean verdict is being removed, and no gate code or policy is being changed. The original links above preserve the review record. I will rerun the local review gates and required CI before queueing the merge.


Generated with xum • Model: coder:openai/gpt-6-astra • Thinking: xhigh • Cost: $264.74

@ThomasK33
ThomasK33 added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit b9bc394 Sep 11, 2026
36 of 39 checks passed
@ThomasK33
ThomasK33 deleted the plugin-components-h6c8 branch September 11, 2026 21:00
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