Repository navigation
🤖 feat: review unconsented plugin components; updates record reviewed changes only - #6557
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
changed the title
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d247fd36e
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d247fd36e
ℹ️ 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".
4d247fd to
c52c7df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c52c7df62d
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a52d03c84
ℹ️ 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".
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
… keep a committed review successful when reconcile fails
…d return a cleanup warning from a review
0ca7146 to
7816ba8
Compare
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…6570) ## Summary Settings → Plugins now shows which plugins have skills or MCP servers the user has not reviewed on this install. The user can approve them in the existing update review panel. Updates applied from that panel now send the reviewed component list. Refs coder#6498 (part 3 of the stack, on top of coder#6557). ## Background coder#6557 added `previewComponentReview`, `confirmComponentReview`, `reviewRequiredCount` and the `reviewed` list in the update consent. The load-time gate comes in the next PR. On the first run after that gate lands, every managed plugin's skills and MCP servers stop loading until the user reviews them once. This UI makes that state obvious and gives a direct way out. ## Implementation 1. **Badge and button:** a plugin row with `reviewRequiredCount > 0` shows a "needs review" badge and a **Review** button. 2. **Notice:** one notice above the list tells the user what to do: "Some plugins have skills or MCP servers that you have not reviewed on this install. Select Review on each plugin marked "needs review" to approve them." The next PR adds the sentence about loading, because it only becomes true there. 3. **Review panel:** **Review** opens the existing `UpdateReviewPanel` in a `components` mode. The rows, the Before/After layout and the Cancel button are the same as in the update review. Only the intro text and the confirm label ("Approve components") change. Approving sends the shown component references to `confirmComponentReview`, and the list refreshes. 4. **Updates:** an update confirmed in the panel sends `reviewed` (the component references of its rows). Only those changes get consent. 5. **Keyboard:** a new palette command, "Review Plugin Components…", opens the review for a plugin that needs it. 6. **Structure:** the review state and its handlers live in a `useComponentReview` hook, with small `ComponentReviewNotice`, `NeedsReviewBadge`, `ReviewComponentsButton` and `ComponentReviewRow` components. The section's intent switch gains one case. The commit carries a `Crap-Debt` trailer for that case (CC 98 → 99). ## Validation - New stories `ComponentReview` and `ComponentReviewPhoneViewport` (Pixel disabled because of the snapshot budget) have play tests. The plays check the badge and the notice, open the review, check both row states, approve, and check that the badge and notice disappear. At 390 px they also check that the card does not overflow. All 15 plays in the file pass (`test-storybook`). Phone width (390 px), review open:  Desktop, after approving:  ## Risks Low. Until the gate PR lands, the review only records consent, and loading does not change. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$0.00`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=0.00 -->
…view (coder#6587) ## Upgrade effect (read first) After this PR, every managed plugin's skills and MCP servers stop loading once, right after the upgrade, until the user reviews them. "Managed" means a plugin installed from **Settings → Plugins** (a row in `plugins.json`). Unmanaged plugins (`~/.agents/plugins`, repo plugins) are not affected. 1. Settings → Plugins shows a "Needs review" badge on each affected plugin and one notice that says the components do not load until reviewed, with a **Review** button (coder#6570). The command palette has the same review command. 2. One review per plugin restores loading. The review uses the existing update review panel and lists each skill and MCP server. 3. Agents in a workspace see fewer skills and MCP tools until then. Nothing is deleted: imports, enablement and plugin data stay as they are. 4. Downgrade: older builds ignore the consent records and load by name, as before. Upgrading again re-applies the gate. 5. Release text: the settings notice says why the components stop loading and that each plugin needs one review after the upgrade. The Agent Skills docs have a "Component review" section, and the MCP servers docs link to it. ## Why no backfill coder#6498 is about a newer Xum parsing more of an installed package than the user saw at install time: an extra Codex skills root, or a stdio server that an older parser skipped. Those components load under a name the user approved. A first-run backfill would fingerprint whatever the current parser sees, which is exactly the content the user never reviewed. It would turn the bug into a stored approval. So an install without consent records counts as "nothing reviewed", and the user reviews once. ## Summary A managed plugin's skill or MCP server now loads only when a stored consent record matches its current fingerprint. Closes coder#6498 (part 4 of the stack, on top of coder#6570). ## Implementation 1. Skills: one predicate, `isPluginSkillConsented` (`componentConsent.ts`), fingerprints the skill's directory (relative to the plugin root) plus its parsed frontmatter, as the install review does. The skill index, `agent_skill_read`, `agent_skill_list` and `skill://` reads all call it. A refused skill is skipped, so a same-name skill from another source can still serve. 2. MCP servers: `loadPluginMcpServers` stores `consentFingerprint` on the plugin provenance. `isPluginMcpServerAllowed` (`registry.ts`) requires a matching record, in addition to the import and enablement checks. The provider drops an unreviewed server from the list, and the manager checks the same rule again at launch. 3. Records come from coder#6534 (install), coder#6557 (review and update) and coder#6570 (UI). ## Validation New tests cover refusal and acceptance in the registry, the real MCP provider (including an `args` change under a reviewed name), the skill index, `agent_skill_list` and an end-to-end install. The end-to-end test installs a plugin, sees its skill and server load, strips the consent records, sees both stop loading, runs `confirmComponentReview`, and sees both load again. Each new test fails with the gate turned off. Existing fixtures now record real consent through the production fingerprint code. ## Risks 1. The upgrade effect above is intended. It is user-visible and needs one review per managed plugin. 2. A fingerprint mismatch between the install review and the load path would block a component the user approved. The fixtures compute consent through the same production functions as the load path, and the end-to-end test covers the full cycle. 3. A body-only update of a skill (its `SKILL.md` instructions) still applies without a review, as on main. The consent fingerprint covers the skill's folder and frontmatter, which closes the coder#6498 case. Body review is a separate product proposal: coder#6617. Size: about 400 production lines changed, the rest is tests and fixtures. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$42.85`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=42.85 --> --------- Signed-off-by: Thomas Kosiewski <tk@coder.com>
…stom_header without headerName (coder#6487) ## Summary Xum now loads three Codex `.mcp.json` shapes that it used to skip or misreport. A stdio server with `env_vars` loads and receives the named host variables. A remote entry with a `url` and no `type` loads as streamable HTTP. An `api_key` auth block with `headerScheme: custom_header` and no `headerName` is reported as malformed. Refs coder#6297. This PR covers three of its items: "Codex `env_vars` on stdio servers", "URL entries without `type`" and "`custom_header` without `headerName`". ## Background Codex review found these gaps after track A merged main (review threads `PRRT_kwDOPxxmWM6rv0UV`, `PRRT_kwDOPxxmWM6rv0Ua` and `PRRT_kwDOPxxmWM6rv0Uh`). 1. Codex defines `env_vars` as the way to pass selected host environment variables to a stdio server ([OpenAI plugins guide](https://developers.openai.com/api/docs/guides/agents-api/tools/plugins)). Xum rejected the field as unknown, so the server did not load. 2. The Codex plugin parser reads a `url` entry without `type` as streamable HTTP ([`codex-mcp/src/plugin_config.rs`](https://github.com/openai/codex/blob/main/codex-rs/codex-mcp/src/plugin_config.rs)). Xum skipped it as an unknown type. 3. OpenAI's auth reference requires `headerName` for `custom_header` ([fixed bearer tokens and API keys](https://developers.openai.com/plugins/build/plugins#fixed-bearer-tokens-and-api-keys)). Xum listed the block as an unsupported setting instead of a malformed one. ## Implementation All three changes apply to the Codex dialect only. A root `mcp.json` stays strict: `env_vars` and an untyped `url` are still errors there. A new test covers that. **`env_vars`.** The Codex translator turns `env_vars` into a new `forwardEnv` list of names on the stdio server record. Xum copies the semantics from Codex: - `env_vars: null` and `source: null` count as absent, as Codex's optional fields do. - An item is a bare name or `{ name, source }` (`McpServerEnvVar` in [`config/src/mcp_types.rs`](https://github.com/openai/codex/blob/main/codex-rs/config/src/mcp_types.rs)). - Xum refuses `source: "remote"`. Codex refuses it for a plugin that runs in the local environment (`bind_environment_env_vars` in `plugin_config.rs`). - Xum refuses `env_vars` on a remote server. Codex does the same (`throw_if_set` in `mcp_types.rs`). - At launch, `prepareStdioLaunch` copies only the named variables from the host. It reads only own properties of the host environment, so a name such as `constructor` is not an inherited value. It skips a name that the host does not set, as Codex's `create_env_for_mcp_server` does ([`rmcp-client/src/utils.rs`](https://github.com/openai/codex/blob/main/codex-rs/rmcp-client/src/utils.rs)). A configured `env` value wins over a host value. - Xum refuses `PLUGIN_ROOT` and `PLUGIN_DATA` in `env_vars`, as it already does in `env`. Xum reads only the names at load time. It never stores or shows a host value. The install consent preview shows the names, for example `node (host env: 'API_TOKEN')`. The consent fingerprint includes the names in the order they first appear (duplicates are dropped), so an update that adds, removes or reorders a name needs new consent. The server signature includes them too, so a changed list restarts the server. **Untyped `url`.** The translator infers `streamable-http` when an entry has a `url` and no `type`. This mirrors how it already infers `stdio` from a `command`. **`custom_header` without `headerName`.** `readAuth` now adds a warning that `headerName` is required, and it drops `headerScheme`. A bad `headerName` already used this warning class. ## Validation - I wrote each test first and saw it fail on `origin/main`. Then I made it pass. - `bun test` (pinned Bun 1.4.3) of `mcpConfig.test.ts`, `openaiMcpAuth.test.ts`, `installService.test.ts` and `mcpServerManager.test.ts`: 558 pass, 0 fail. - `make static-check` passes. ## Resolved since the hold - Consent for newly recognized servers is fixed on main by coder#6498 (stack coder#6534, coder#6557, coder#6570, coder#6587). A managed plugin's server loads only when its consent fingerprint matches what the user reviewed. This branch merges that main: `forwardEnv` is an included field in the fingerprint table (`componentConsent.ts`), so adding an `env_vars` name changes the fingerprint and needs a review. The old update-only `stdioFingerprint` helper is gone, because main's update review uses the same consent fingerprint. - The launch environment is fixed on main by coder#6581 (coder#6509). A plugin stdio server starts from a minimal host environment. `env_vars` names are copied into the server's declared `env`, so they reach the server on top of that base. A new test launches a plugin server through the real host runtime and checks that a named variable arrives and an unnamed one does not. - Docs: docs/config/mcp-servers.mdx ("Plugin server environment") now describes `env_vars`. ## Risks The risk is low. The `env_vars` and untyped-`url` changes apply only to Codex `.mcp.json` files, and those entries failed to load before. A stdio server without `env_vars` keeps the same launch environment, fingerprint and signature, because each of them leaves out an unset `forwardEnv`. Adding `forwardEnv` to the consent fingerprint does not change the fingerprint of an existing server, because `pick` skips an unset field. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$0.00`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=0.00 -->
1 participant
Summary
This PR adds the backend half of the plugin component review. A new review entry point lists the imported skills and MCP servers whose current fingerprint the user has not reviewed, and a confirm call records consent for them. Updates now record consent only for the changes the user saw. Nothing gates loading yet, so this PR does not change what loads. Refs #6498 (part 2 of the stack, on top of #6534).
Background
#6534 stores a reviewed fingerprint for each imported component. A later PR will refuse a component whose fingerprint has no matching record. Before that gate can land, users need a way to review components. Every install from before #6498 has no records, so every managed plugin needs one review after the upgrade. This PR adds that review and makes updates keep the records current.
Implementation
previewComponentReview({ name })returns the existing update review shape (fromShaequalstoSha), so Settings can reuse the update review panel. Each row says "Not reviewed on this install" or "Changed since you reviewed it". It shows the same display text as the update review and carries a component reference{ kind, name, fingerprint }.confirmComponentReview({ name, expectedLockedSha, reviewed })recomputes the rows under the mutation lock. It records only the references that still match, so a component that changed after the review keeps needing review. It then reconciles MCP servers.AgentPluginUpdateConsentSchemagets an optionalreviewedlist, and each skill or server change row carries its component reference.update()records consent only for recomputed changes that appear inreviewedat the same fingerprint.reviewed, the changed components need a component review afterwards (fails closed).reviewRequiredCount, the number of imported components that need review.componentConsentobject, so consent kinds that a newer build stored survive.The renderer changes (the "Needs review" badge, a notice, and the review panel reuse that sends
reviewed) come in the next PR of the stack.Validation
New
installService.test.tscases:Risks
Low. Loading does not change yet. An update applied from the current UI, which does not send
reviewedyet, leaves the changed components counted as needing review. Nothing shows that count until the next PR.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$0.00