feat(runtime-api): plugin + marketplace management over /v1/apps (Engine side) - #5842
feat(runtime-api): plugin + marketplace management over /v1/apps (Engine side)#5842Hmbown wants to merge 1 commit into
Conversation
…ine side)
Expose the Engine's plugin authority to app clients over the runtime API
the desktop shell already spawns (codewhale app-server --http → serve):
- GET /v1/apps/plugins, GET /v1/apps/plugins/{selector} with structured
capability review payload + review token (env/header values redacted)
- POST install / update / DELETE uninstall through plugins::mutation
(installs land disabled+untrusted; network policy enforced)
- POST trust (hash-bound token) / enable / disable / revoke through the
registry receipt flow
- Marketplace CRUD + candidate install sharing one catalog loader
(plugins/marketplace/document.rs) with /plugin marketplace
- RuntimeCapabilities.plugin_management advertised by /v1/runtime/info
- install:: name_conflict callbacks now &(dyn Fn + Send + Sync) so the
install future is Send (required by async axum handlers)
Local gates: cargo clippy -p codewhale-tui --lib --tests clean;
new tests 8/8 pass (capability, full lifecycle over HTTP, 404s,
marketplace add/list/install/remove, symlink refusal); plugins + command
suites pass except two stack-overflow crashers reproduced on pristine
origin/main (kimi_plan_codes_resolve_at_render_time,
marketplace_add_list_show_remove_roundtrip) — pre-existing, not from
this change; documented in handoff.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6d40c2b4-c5e8-4965-9edb-43ee68523b0c) |
|
Claude finished @Hmbown's task in 3m 7s —— View job Review complete
SummaryThis is a clean, well-scoped diff. It follows the ponytail rule 2 (reuse) well: rather than reimplementing catalog loading and trust-token logic for the new Correctness: no Provider/model/route safety: not applicable, no model/provider selection logic is touched by this diff. Tests: One point worth confirming with the author rather than a blocking bug: Nit: I could not run Good choices to call out: the doc comment on |
There was a problem hiding this comment.
🟡 Changes recommended
The new name_conflict: ...(Send + Sync) requirement in plugin install APIs breaks existing non-Sync callers (e.g., the registry-capturing closure in plugins/mutation.rs), which is a likely compile blocker.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Engine-side plugin bundle and marketplace lifecycle management to the Runtime HTTP API under /v1/apps/plugins and /v1/apps/marketplaces, and advertises the feature via a new plugin_management runtime capability.
Changes:
- Introduces new Runtime API handlers for plugin list/detail, install/update/uninstall, trust/enable/disable/revoke, and marketplace catalog add/list/detail/remove/install.
- Extracts local marketplace catalog document loading + candidate install resolution into a shared
plugins::marketplace::documentmodule used by both TUI commands and the Runtime API. - Adds integration tests for plugin lifecycle and marketplace flows; adds
LoadedPlugin::review_token()and wires capability advertisement through protocol + runtime info.
File summaries
| File | Description |
|---|---|
| crates/tui/src/runtime_api/tests.rs | Adds Runtime API integration tests covering plugin and marketplace flows. |
| crates/tui/src/runtime_api/plugins.rs | New Axum handlers + DTOs for plugin and marketplace management over /v1/apps/*. |
| crates/tui/src/runtime_api.rs | Registers new routes and advertises plugin_management in default capabilities. |
| crates/tui/src/plugins/types.rs | Adds LoadedPlugin::review_token() used by both TUI and Runtime API trust flows. |
| crates/tui/src/plugins/marketplace/mod.rs | Exposes new shared document module and updates module docs. |
| crates/tui/src/plugins/marketplace/document.rs | New shared loader for local catalog documents + install-spec resolution. |
| crates/tui/src/plugins/install/mod.rs | Tightens name_conflict callback bounds to Send + Sync. |
| crates/tui/src/commands/groups/plugins/render.rs | Delegates review-token formatting to LoadedPlugin::review_token(). |
| crates/tui/src/commands/groups/plugins/marketplace.rs | Refactors TUI marketplace command to use shared document loader + resolver. |
| crates/protocol/src/runtime/mod.rs | Adds plugin_management to RuntimeCapabilities with serialization tests. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| network: &NetworkPolicy, | ||
| update: bool, | ||
| name_conflict: &dyn Fn(&str) -> Option<String>, | ||
| name_conflict: &(dyn Fn(&str) -> Option<String> + Send + Sync), | ||
| ) -> Result<PluginInstallOutcome> { |
| //! execution. Every fetch happens through the existing reviewed installer | ||
| //! when an operator explicitly installs a candidate. | ||
| //! when an operator explicitly installs a candidate. The one filesystem | ||
| //! seam — reading a local catalog document a operator pointed at — lives in |
| let add_resp = client | ||
| .post(&base) | ||
| .json(&serde_json::json!({ | ||
| "name": "team", | ||
| "path": catalog_path.display().to_string() | ||
| })) | ||
| .send() | ||
| .await?; | ||
| let add: serde_json::Value = add_resp.json().await?; |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// Resolve a user-supplied document path to an existing regular file without | ||
| /// following a final symlink (the document is untrusted input). | ||
| fn canonical_document(path: &Path) -> Result<PathBuf, String> { | ||
| let metadata = std::fs::symlink_metadata(path) |
| } | ||
|
|
||
| fn read_bounded(path: &Path) -> Result<String, String> { | ||
| let file = std::fs::File::open(path) |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR adds plugin and marketplace lifecycle endpoints to the Runtime API and extracts a shared local-catalog loader. The API shapes are generally consistent with TUI review flows, but the new run_registry_mutation has a compile-blocking move error, several API messages leak literal {name} placeholders, and the review payload may expose unredacted MCP URLs.
Findings
- [ERROR]
run_registry_mutationconsumesmutationtwice withoutCopy(crates/tui/src/runtime_api/plugins.rs:468)
RegistryMutationdoes not deriveCopy/Clone, but it is matched by value at lines 460-465 to computeactionand then matched again at lines 468-473 to dispatch the registry mutation. The first match moves the parameter, so the second match is a use-after-move and this file will not compile. Add#[derive(Clone, Copy)]toRegistryMutation, or match by reference. - [WARNING] Placeholder
{name}reaches API clients unformatted (crates/tui/src/runtime_api/plugins.rs:418)
The install note at line 418, the token-mismatch error at line 455, and the enabled-untrusted note at line 492 embed literal{name}text. SincePluginMutationResponse.noteandPluginActionResponse.noteareOption<&'static str>and the error is not passed throughformat!, clients receive the literal placeholder instead of the actual plugin name. These should either useOption<String>withformat!or avoid the placeholder. - [WARNING] MCP server review payload exposes raw URL (
crates/tui/src/runtime_api/plugins.rs:273)
mcp_server_reviewcopiescfg.urldirectly intoPluginMcpServerReview.url. If remote MCP URLs can contain userinfo or query-string credentials, the structured trust review leaks them despite the comment claiming secret-bearing maps are reduced to key names. Redact userinfo/sensitive query parameters or confirm this URL type cannot carry credentials. - [INFO] Missing coverage for several lifecycle and marketplace paths (
crates/tui/src/runtime_api/tests.rs)
Tests cover capability advertisement, install→trust→enable→disable→uninstall, and basic marketplace add/list/install/remove. There are no HTTP tests forupdate,revoke, install withexpected_content_hash, marketplace detail/remove-not-found/add-invalid-name, or install of an unsupported candidate.
Suggestions
-
crates/tui/src/runtime_api/plugins.rs:511— MakeRegistryMutationCopyso it can safely be matched twice, or alternatively match by reference the first time. All fields areCopy.#[derive(Clone, Copy)] enum RegistryMutation<'a> {
Assessment
The feature is well-structured and shares security-sensitive logic with the TUI, but the compile-blocking RegistryMutation move issue must be fixed before merge. The literal placeholders and unredacted MCP URL should also be corrected, and the missing API lifecycle tests should be added to fully cover the new surface.
Advisory review by Codewhale (codewhale review --pr 5842 --post, head 6865dfb4cd68aebf6a05f4790b6d0dfaa7d01ed6). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| }; | ||
|
|
||
| let mut registry = (*registry).clone(); | ||
| let result = match mutation { |
There was a problem hiding this comment.
[ERROR] run_registry_mutation consumes mutation twice without Copy
RegistryMutation does not derive Copy/Clone, but it is matched by value at lines 460-465 to compute action and then matched again at lines 468-473 to dispatch the registry mutation. The first match moves the parameter, so the second match is a use-after-move and this file will not compile. Add #[derive(Clone, Copy)] to RegistryMutation, or match by reference.
| let note = match receipt.outcome { | ||
| PluginMutationOutcome::Installed => Some( | ||
| "Installed disabled and untrusted. Review the capability payload \ | ||
| (GET /v1/apps/plugins/{name}), then trust and enable it.", |
There was a problem hiding this comment.
[WARNING] Placeholder {name} reaches API clients unformatted
The install note at line 418, the token-mismatch error at line 455, and the enabled-untrusted note at line 492 embed literal {name} text. Since PluginMutationResponse.note and PluginActionResponse.note are Option<&'static str> and the error is not passed through format!, clients receive the literal placeholder instead of the actual plugin name. These should either use Option<String> with format! or avoid the placeholder.
| kind: if cfg.url.is_some() { "remote" } else { "stdio" }, | ||
| command: cfg.command.clone(), | ||
| args: cfg.args.clone(), | ||
| url: cfg.url.clone(), |
There was a problem hiding this comment.
[WARNING] MCP server review payload exposes raw URL
mcp_server_review copies cfg.url directly into PluginMcpServerReview.url. If remote MCP URLs can contain userinfo or query-string credentials, the structured trust review leaks them despite the comment claiming secret-bearing maps are reduced to key names. Redact userinfo/sensitive query parameters or confirm this URL type cannot carry credentials.
| }) | ||
| } | ||
|
|
||
| enum RegistryMutation<'a> { |
There was a problem hiding this comment.
Make RegistryMutation Copy so it can safely be matched twice, or alternatively match by reference the first time. All fields are Copy.
| enum RegistryMutation<'a> { | |
| #[derive(Clone, Copy)] | |
| enum RegistryMutation<'a> { |
Engine side of the gated local plugin system. Single commit off current tip. Gates: fmt clean, dead-code budget PASS at 425. App side follows separately. Full matrix via CI.
No-Issue: feature branch tracked in 0.9.12 milestone, no single issue.