Repository navigation
feat(mcpx): WASM tool-source spike — wazero host, capability test (#1970) - #2002
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 34 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 019ee02b90
ℹ️ 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".
| for _, fn := range []string{inputPtrFn, outputPtrFn, listFn, callFn} { | ||
| if mod.ExportedFunction(fn) == nil { | ||
| rt.Close(ctx) | ||
| return nil, fmt.Errorf("wasmtools: guest does not export %s", fn) | ||
| } |
There was a problem hiding this comment.
If a guest exports all four required names but omits linear memory or uses an incompatible result signature, Load succeeds even though subsequent calls can panic the Go host. For example, a valid WASM module whose mcpx_list_tools returns no values reaches res[0] in ListTools, while a module without memory reaches a nil memory dereference in readOutput or CallTool. Validate the required memory and function parameter/result types before returning the source so malformed guests produce load errors rather than host panics.
Useful? React with 👍 / 👎.
| // grant set — WASI included — makes Load fail. | ||
| func Load(ctx context.Context, src []byte, grants ...string) (*Source, error) { | ||
| s := &Source{} | ||
| rt := wazero.NewRuntime(ctx) |
There was a problem hiding this comment.
If a guest tool enters an infinite loop, canceling or timing out the context passed to ListTools or CallTool will not interrupt it, leaving the caller goroutine executing indefinitely. The wazero v1.9.0 runtime configuration documentation explicitly states that execution-termination checks are disabled by default, which is the configuration selected by NewRuntime here. Construct the runtime with WithCloseOnContextDone(true) so callers can actually bound guest execution through the contexts these methods accept.
Useful? React with 👍 / 👎.
|
🤖 Agent note · parked
|
Spike for #1970 (queue task 2907): Approach B — a core-WASM
.wasmmodule as an mcpx tool source, hosted by wazero.What this adds
pkgs/mcpx/internal/wasmtools: a standalone wazero host for a core-WASM tool source. It instantiates no WASI at all, so a guest gets no filesystem, network, clock, or environment by default; the only imports that can exist are host grants (today:mcpx.log). Any ungranted import — WASI included — fails instantiation: deny-by-default is a property of the loader.mcpclient.Client(ListTools/CallToolshapes ininternal/mcpclient/client.go), shaped so a component-model ABI can replace it underneath later (Approach C)..wasmintestdata/):echo(lists/calls one tool, logs via the granted import) andgreedy(importsmcpx.read_file, which the host never offers).Interface gap (per the spike brief)
There is no registration point yet for a new tool-source kind: upstreams are concrete transports in
internal/mcpclientandinternal/adapter, and the tool-source interface is still tracked by #1885. So this package is deliberately a standalone spike, not a parallel/fifth composition path. Wiring it in waits for #1885's interface; the ABI above is designed to sit behind it unchanged.Capability test (acceptance)
TestListAndCallWithGrantedLog— grantedlogimport works; tool list + call round-trip.TestSameModuleWithoutGrantIsDenied— same module without the grant fails to instantiate.TestUngrantedCapabilityImportIsDenied—greedy.wasmis denied even with every grantable capability granted; the error names the missingread_fileimport.Verified on the laptop worktree:
go test -count=1 ./internal/wasmtools/— PASS.Constraints
Gating research answer
Posted on #1970: wazero does not host Component Model components at acceptable maturity today (component support is still under active development upstream); the mature component host is wasmtime, whose Go bindings require cgo — which would forfeit the pure-Go premise. Recommendation: proceed with B on core WASM now; defer C until wazero component hosting matures or a pure-Go lowering path lands.
Refs #1970, #1885
Need help on this PR? Tag
@codesmith-botwith what you need.