Repository navigation
feat(mcpx): recipes as MCP tools + runtime on adapter declarations (#2117) - #2504
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 51 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
![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: f76452c780
ℹ️ 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".
…ers (#2117) - recipe_tools: run recipes under the MCP caller's session (a.mcpCaller(ctx).SessionID), not the daemon process session, so session-scoped namespaces stay isolated per client (P1). - recipe_tools: a nonzero recipe exit now returns an error (output retained), so the MCP result is a tool failure, not a success (P2). - adapter/runtime: when Dir is set and the script is absent beside the declaration, fall back to resolving the bare name on PATH (P2); regression test probes+runs a PATH script with Dir elsewhere. - recipes: placeholder discovery (refs) skips // @tool marker lines as it already skipped @PARAM declarations, so opting out of MCP exposure no longer invents a required 'tool' input (P2); regression test added.
|
@codex review Codex findings on f76452c dispositioned in f495c8e (all fixed, replied on their threads, threads resolved):
Laptop verification: go test ./internal/recipes/ ./internal/adapter/ pass; go test ./internal/cli/ -run Recipe pass; go vet clean on all three packages. Please re-review the new head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f495c8e5c6
ℹ️ 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".
- adapter: resolve a relative executable against Dir when no runtime is declared (preserves pre-resolution exec behaviour for command: ./tool with dir set) - mcpserver: keep a failed tool's returned text in the isError result so a failed recipe's stdout/stderr reaches the caller alongside the error - regression tests for both
Closes #2117 (slices 1+3 of #1875, split by triage task 2769).
What
1. Recipes are individual MCP tools. A saved recipe now appears in
tools/listasrecipe_<name>, with its// @paramheaders turned into the tool's JSON Schema by the existingrecipes.FormSchema. Exposure is opt-out per recipe (// @tool false, default on). Registration happens where adapter/OpenAPI//v1extras are already assembled (cli.MCPServer→mcpserver.RecipeTools), so recipe tools reach/mcp,mcpx servestdio and the plain-POST projection alike. A tool call routes to the existingPOST /v1/recipes/{name}/runpath — no second runner.Name rules are the adapter rules (one namespace prefix + member name), not a new scheme. A recipe whose
recipe_<name>is already taken by an adapter//v1/OpenAPI tool is skipped, never shadowed, and named inmcpx catalog, which now ends with a recipe-tool block: exposed / budget (25) / opted out / every skip with its reason.2.
runtimeon adapter declarations.bash,js,ts,pyare accepted, resolved to bash/sh, deno|bun|node, python3|python (deno getsrun), withcommandresolved as an absolute path,dir-relative, or via PATH. Declarations withoutruntimespawn exactly as before, byte for byte;mcpx adapter serveneeds no change because it goes throughSpec.Call. Invalid values fail validation with the accepted list.Tests
New: recipe tool schema/opt-out/collision/budget/idempotence (recipes), tools-list presence + call + collision + re-listing (mcpserver), real-spawn bash and python adapters served and answering,
dir-relative script resolution, catalog skip reporting, no-runtime unchanged, runtime validation (adapter).go build ./...,go vet ./...cleango test ./...inpkgs/mcpx: all pass except two pre-existing failures proven identical onmainat7d1b081c—internal/settings TestEveryDocumentHasAChapter(11 files absent fromcompendium.manifest.json) and 8internal/e2efailures that are runtime-environmental (nodenoon PATH). None touch tools/list, serve, adapters or recipes.Known gaps (deliberately out of scope)
--allow-*must be declared through a wrapper.Need help on this PR? Tag
@codesmith-botwith what you need.