Repository navigation
🤖 feat: record the reviewed fingerprint of each imported plugin component - #6534
Merged
Merged
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
Member
Author
|
@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: 3dba7cd744
ℹ️ 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".
ThomasK33
force-pushed
the
fix/plugin-import-fingerprints
branch
from
October 14, 2026 11:19
3dba7cd to
f6c32f7
Compare
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb57a9d14a
ℹ️ 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".
ThomasK33
added this pull request to stack #6558
October 14, 2026 11:25
This was referenced Oct 14, 2026
Member
Author
🛡️ 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: bb57a9d14a
ℹ️ 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".
Member
Author
Member
Author
|
Codex Review: Didn't find any major issues. Nice work! 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. |
Member
Author
|
Codex Review: Didn't find any major issues. Hooray! 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. |
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 14, 2026
… changes only (coder#6557) ## 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 coder#6498 (part 2 of the stack, on top of coder#6534). ## Background coder#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 coder#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 1. **Component review:** `previewComponentReview({ name })` returns the existing update review shape (`fromSha` equals `toSha`), 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 }`. 2. **Confirm:** `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. 3. **Updates:** `AgentPluginUpdateConsentSchema` gets an optional `reviewed` list, and each skill or server change row carries its component reference. - `update()` records consent only for recomputed changes that appear in `reviewed` at the same fingerprint. - Every other record is kept. - Without `reviewed`, the changed components need a component review afterwards (fails closed). 4. **Update diff:** the diff now uses the consent fingerprints. - A server change compares the whole normalized server plus the client-secret digest, so a parser widening also shows up in the update review. - A skill change compares its location and full frontmatter, and the summary says "changes the definition of skill" when the advertisement did not change. 5. **List rows:** managed rows get `reviewRequiredCount`, the number of imported components that need review. 6. **Consent merge:** writes merge the raw `componentConsent` object, 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.ts` cases: - A legacy install (no records) lists both components. Confirming after one of them changed records only the unchanged one. - An update records only the reviewed change, and the unreviewed one keeps its old record. - A manifest that names the final install path literally needs no review after install. ## Risks Low. Loading does not change yet. An update applied from the current UI, which does not send `reviewed` yet, 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`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=0.00 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 14, 2026
…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>
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 14, 2026
…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 -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Xum now records the fingerprint the user reviewed for each imported component of a managed plugin. Nothing reads the records yet, so loading does not change in this PR. A later PR in this stack uses them to refuse a component whose fingerprint changed. Refs #6498 (part 1 of 4).
Background
Saved imports in
plugins.jsonname a component, but they do not bind what the user reviewed. So when a newer Xum parses more of an installed package, a changed skill or MCP server can load under an approved name. #6498 has two examples: an extra Codex skills root, and a stdio server that an older parser skipped. The design and its security review are summarized in #6498.Implementation
componentConsent.tsbuilds the fingerprints. Review time and load time use the same functions.MCPServerInfo. ARecord<keyof …, "include" | "exclude">map names every field, so a new field is a compile error until someone decides. Without that, a future parser widening would slip past. Excluded fields are the ones the user owns (disabled,toolAllowlist), provenance, and the location of the OAuth client secret file. The plugin root and thePLUGIN_DATAdirectory become tags, so a staged clone and the installed tree hash alike. Literal%and<are escaped first, so a manifest cannot fake a tag.extra/foo) plus the wholeSKILL.mdtext.componentConsent: { skills, mcpServers }(name → fingerprint). It is top-level because older builds spreadimportedComponentson save and would leave a nested record stale.install()records the fingerprints of the imported components, because the consent preview showed them at the pinned SHA.setComponents()records a fingerprint only for components that are new to the selection. A component that stays selected keeps its old record, or stays without one. Without this rule, a component picker save could approve a component that changed since its review. Narrowing a legacy import-all entry approves nothing.getComponents()never sends fingerprints to the renderer.Accepted limitations, documented in
componentConsent.ts:PATHat launch../command, and the other files in a skill directory, are not hashed.Validation
componentConsent.test.tscover these cases:installService.test.tscases cover install, a picker save that adds a component while keeping a stale record, and a narrowed legacy install.Risks
Low. The new field is optional and nothing reads it yet. Older builds ignore it.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$0.00