Implement WASM ProductAdapter component runtime - #3583
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the wasmtime component-model runtime for IronClaw Reborn product adapters, transitioning from a stub implementation to a functional WASM sandbox. It introduces the ProductAdapterComponentRuntime for managing component lifecycles, resource limiting via WasmResourceLimiter, and integration with WIT bindings. Feedback focuses on addressing a potential thread leak in the epoch ticker and improving performance by caching the Linker instead of recreating it for every instantiation.
|
|
||
| let engine = Engine::new(&wasmtime_config) | ||
| .map_err(|error| RuntimeError::EngineCreationFailed(error.to_string()))?; | ||
| spawn_epoch_ticker(engine.clone())?; |
There was a problem hiding this comment.
| StoreData::new(limits.memory_bytes, limits.timeout), | ||
| ); | ||
| configure_store(&mut store, limits)?; | ||
| let linker = create_linker(&self.engine)?; |
There was a problem hiding this comment.
The Linker is recreated for every instantiate call, which is inefficient. Since Linker is Clone, consider creating it once in new and storing it in ProductAdapterComponentRuntime to reuse it. This avoids unnecessary heap allocations, which is important for performance in WASM.
References
- To improve performance in WASM, avoid unnecessary heap allocations.
This commit addresses every code review comment from PR #3590 and lands the P1 follow-ups for the Telegram v2 tracer (durable storage backends fully tested, migration smoke tests, boundary entry, real bot identity, ledger settle verification, docs). ## Review comments addressed | # | Reviewer | Change | |---|---|---| | 1 | gemini-code-assist | Centralize `derive_user_id` — new `identifiers.rs` module; libSQL and Postgres bindings share one implementation. Removes the documented/implementation drift the reviewer flagged. | | 2 | gemini-code-assist | `parse_phase` exhaustive `match` in both ledger impls (replaces the fragile serde-quoted-string trick); paired comment on `phase_to_str` so any future `ActionPhase` variant fails closed. | | 3 | gemini-code-assist | Postgres binding upsert is now a single-roundtrip `INSERT ... ON CONFLICT ... DO UPDATE SET adapter_id = EXCLUDED.adapter_id RETURNING thread_id` (was: separate `INSERT ... ON CONFLICT DO NOTHING` + `SELECT thread_id`). | | 4 | gemini-code-assist | V28 `product_inbound_actions.action_id` is `UUID` (was `TEXT`); Postgres ledger binds `uuid::Uuid` directly via tokio-postgres `with-uuid-1` feature. libSQL keeps `TEXT` (no UUID type). | | 5 | gemini-code-assist | Runner map keyed by the validated `installation_id.as_str()` (was: raw `installation_id_str` from env). Webhook lookup now matches what the adapter sees. | | 6 | serrrfirat | Extracted Reborn boot orchestration out of `src/main.rs` into `src/channels/reborn/registry.rs::register_reborn_channels`. Main binary now has one function call + a `RebornChannelWiringInputs` struct; no direct references to Reborn crate types. This is interpretation (A) of the comment — orchestration-only extraction. If the reviewer's intent is interpretation (B), full crate-level separation of `src/channels/reborn/` is a follow-up. | ## P1 batch (per pre-agreed in-scope follow-ups) * **#15** — Postgres contract tests: 4 tests for ledger (begin/settle/replay/release) and binding (idempotent + per-actor distinct) under `--features postgres`, gated on `IRONCLAW_PRODUCT_STORAGE_POSTGRES_URL` (or `DATABASE_URL`). Skip-clean when no Postgres available. * **#16** — Migration smoke tests in `src/db/libsql_migrations.rs`: applies V26 against fresh in-memory libSQL, asserts both tables + all required columns + recording in `_migrations`. * **#17** — `ironclaw_product_workflow_storage` added to architecture boundary rules forbidding `dispatcher` / `extensions` / `host_runtime` / `mcp` / `wasm` / `scripts` / `network` / `engine` / `gateway` / `secrets` / `authorization` / `capabilities` / `reborn` / `reborn_cli`. * **#18** — `getMe` at boot resolves real `bot_user_id` + `bot_username` from `api.telegram.org` (was: hardcoded placeholders). Fail-soft on network/token errors: warn + use safe placeholders, so the binary still starts. * **#19** — `accepted_inbound_settles_the_ledger_row` e2e test SELECTs the DB row after a webhook completes, asserts `phase='settled'` + `outcome_json` is non-null + `settled_at` is set. Proves `DefaultProductWorkflow::accept_inbound` actually invokes `ledger.settle` end-to-end. * **#20** — Documentation: new `src/channels/reborn/CLAUDE.md` (file map, call paths, bridge seams, migration path post-#3583, operator setup, known gaps) + paragraph in top-level `CLAUDE.md` mentioning `REBORN_TELEGRAM_V2_ENABLED` and pointing at the module guide. ## Verification cargo fmt --all -- --check # clean cargo clippy --bin ironclaw --features libsql --tests -- -D warnings # clean cargo clippy -p ironclaw_product_workflow_storage --features libsql,postgres --tests -- -D warnings # clean cargo test -p ironclaw_product_workflow_storage --features libsql # 13/13 IRONCLAW_SKIP_POSTGRES_TESTS=1 cargo test --test postgres_contract -p ironclaw_product_workflow_storage --features libsql,postgres # 4/4 cargo test --test reborn_telegram_v2_e2e --features libsql --no-default-features # 7/7 cargo test --lib --no-default-features --features libsql migration_smoke # 2/2 cargo test -p ironclaw_architecture # 13/13 ## P2 deferred to follow-up PRs Per the in-scope vs follow-up split agreed during triage: * #21 multi-installation support — own PR (env vs registry design) * #22 setWebhook helper — own PR (CLI subcommand) * #23 slash command routing — own PR (couples to Epic Child 9) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Sandbox core: epoch ticker now holds Weak<Engine> and exits when every Engine clone is dropped. Eliminates the per-runtime thread + Engine clone leak that affected tests and any long-running host that rebuilds runtimes (config reloads, per-installation, etc.). Adds a regression test that drops the engine and asserts the weak handle releases. - ProductAdapter runtime: render_outbound now returns the host's validated typed EgressRequest instead of the raw component JSON. The host built the typed value via the same EgressMethod::new, EgressPath::new, EgressHeader::new, EgressRequest::with_body constructors the production HTTP egress path will use, so the validation is now the single source of truth callers see. - ProductAdapter runtime: extract_manifest checks the epoch deadline before inspecting the call result, so timeouts surface as clean "deadline exceeded" errors instead of raw wasmtime trap text. Matches the existing parse_inbound / render_outbound ordering. - ProductAdapter runtime: introduces MAX_COMPONENT_JSON_BYTES (1 MiB) and rejects any component-returned (or host-supplied) JSON above it before serde walks the document. Decouples host-side serde allocations from any future raise of the WASM memory_bytes cap. - WIT + Rust: document now-millis as wall-clock UTC milliseconds, NOT monotonic. Adapter authors must not build TTL/idempotency checks on it. - Egress validation: explicit comment that the EgressPolicy::check inside validate_rendered_egress_request is defense-in-depth: it is structurally a no-op today (the policy is built from the same vec we just indexed into) but locks the manifest <-> policy symmetry for future readers and divergence. Tests added: - epoch_ticker_exits_when_engine_is_dropped in sandbox core. - render_rejects_oversized_host_envelope_before_serde in ProductAdapter contract tests. Validation: cargo fmt; cargo clippy -p ironclaw_wasm_product_adapters -p ironclaw_wasm_sandbox_core -p ironclaw_architecture --all-targets -- -D warnings; cargo test on those three crates (--locked).
Code Review — WASM ProductAdapter Component RuntimeSummaryIntroduces Strengths
IssuesMajor
Minor
Suggestions
VerdictREQUEST CHANGES — primarily for issue 2 (deadline-check ordering in Overall this is a well-scoped, well-documented PR that does what the description says, respects every guardrail in the crate-level |
henrypark133
left a comment
There was a problem hiding this comment.
Review: contract mismatch in WASM ProductAdapter egress
What looks good:
- The runtime validates component-returned parsed inbound and rendered egress JSON back into typed host DTOs.
- Malformed components, undeclared render targets, host-managed headers, and bad JSON paths have contract coverage.
- CI is green on the current head.
Findings:
- High -
crates/ironclaw_wasm_product_adapters/src/store.rs:101:http_egressis hard-coded toPolicyDenied, while the WIT and product-adapter contract say component HTTP egress is the only network capability and that the host executes rendered outbound through it.
Why it matters: the new WASM ProductAdapter runtime cannot support adapters that need host-mediated protocol HTTP egress, despite the contract promising declared-host validation, credential resolution, response leak scanning, and delivery reporting. The mismatch is visible in wit/product_adapter.wit, which describes http-egress as the network boundary, and in docs/reborn/contracts/product-adapters.md, which says the host sends the request through that path.
Expected fix direction: either wire a real host-mediated egress implementation through the component store while keeping the default fail-closed when no egress service is injected, or explicitly narrow this PR/docs/WIT to say this slice is parse/render-only and defer component http-egress to a follow-up.
Low-priority notes:
- Hostile output-size, timeout/fuel, and log-isolation tests would strengthen the runtime boundary.
- I did not treat the epoch ticker as a blocker: the current sandbox core uses
Engine::weak()and the ticker exits once owned engines drop.
Summary:
- Recommended verdict: Request changes
- Review coverage: worktree-backed review of the WASM component runtime, store, WIT, product-adapter docs, tests, CI, and prior review threads.
95f5e0f to
a489837
Compare
|
Addressed review feedback in a489837. Summary:
Local: full |
|
Addressed the review feedback in
Verification run:
Known unrelated local checks:
|
Re-review — WASM ProductAdapter component runtimeFollowing up on my prior review (REQUEST CHANGES, primarily on issues 2 and 3). Re-reviewed against the three new commits ( Per-item status
New items observed
Minor follow-ups (non-blocking)
VerdictAPPROVE. Both REQUEST CHANGES items (deadline-ordering in |
…ironclaw into feat/wasm-product-adapter-loader
SummaryReviewed PR #3583 only. Base PR implements WASM ProductAdapter component runtime. Boundary shape is mostly good: fail-closed egress, sealed auth evidence, resource caps during execution, and host-side egress revalidation. Merge stance: two Medium pre-execution/control-plane gaps remain. Findings
Security/data-flow notes
Missing tests
|
…583-conflicts # Conflicts: # Cargo.toml # crates/ironclaw_wasm_product_adapters/src/lib.rs
…loader Implement WASM ProductAdapter component runtime
Summary
ProductAdapterComponentRuntimewasmtime component loader forwit/product_adapter.witEgressPolicy, and callparse-inbound/render-outboundProtocolAuthEvidencefor parse calls; validate render egress target index before exposing component outputTesting
cargo fmt --checkcargo test -p ironclaw_wasm_sandbox_core --lockedcargo test -p ironclaw_product_adapters --features test-support --lockedcargo test -p ironclaw_product_adapter_registry --lockedcargo test -p ironclaw_wasm_product_adapters --lockedcargo test -p ironclaw_architecture --lockedcargo clippy -p ironclaw_product_adapters -p ironclaw_product_adapter_registry -p ironclaw_wasm_sandbox_core -p ironclaw_wasm_product_adapters -p ironclaw_architecture --all-targets --all-features --locked -- -D warningsNotes