[ISSUE #7561] Add reusable request, configuration and result bridge for wasm base plugins. - #7569

Large but well-scoped bridge. I built and ran the affected reactor locally on this head (JDK 21):

• wasm-api: AbstractWasmPluginTest 2/2, WasmLoaderTest 1/1. wasm-base: AbstractShenyuWasmPluginTest 7/7, WasmBridgeTest 15/15, WasmRequestBridgeTest 38/38, AbstractWasmPluginDataHandlerTest 12/12, AbstractWasmDiscoveryHandlerTest 2/2, AbstractWasmMetaDataHandlerTest 2/2 — cumulative 76/76, no failures; checkstyle 0 violations; apache-rat:check 0 unapproved on both modules.
• The protocol is defensive in the right places: strict UTF-8/JSON (duplicate keys and trailing documents rejected), 1 MiB cap enforced before allocation, long→int narrowing guarded by checkRange, framing / hop-by-hop headers refused, header names validated against the token grammar with case-insensitive duplicate detection, the result copied out of guest memory before the lock is released, invocation ids expired in finally, and get_args(id, 0, 0) size negotiation that writes nothing on an undersized buffer.

Coordination issue with the sibling WASM PRs (same base, same author). WasmLoader.getWasmExtern here is a third variant of the change also made by #7567 (draft) and #7568. This PR filters on ExternalType.FUNCTION and returns empty for a non-function export; #7568/#7567 let instance.export(...) throw InvalidException for that case. The expectations are mutually exclusive — I applied this PR and ran #7568's nonFunctionExportStillFailsTest: Expected com.dylibso.chicory.wasm.InvalidException to be thrown, but nothing was thrown. Whichever of the three merges second also conflicts on the same hunk. Please settle one lookup policy and land it once (a shared findWasmExtern helper in #7567, with #7568/#7569 rebased on top), and drop the duplicate here.

Behaviour-change notes that deserve a line in the description / a migration note, since AbstractShenyuWasmPlugin is a public base class:

• The no-argument constructor now installs the JSON bridge instead of the built-in no-op get_args/put_result. An external subclass that used the no-arg constructor now gets bridge semantics — strict JSON result parsing, 1 MiB cap, fail-closed HTTP 500 for a result the bridge cannot parse — rather than the previous no-op callbacks. There is no in-repo production subclass, but this is exactly the kind of change that needs to be called out.
• close() only runs if the Spring bean is registered with a destroy method; the README says so, but no starter wiring is added here, so the "idempotent shutdown cleanup" remains opt-in.
• The default 5-arg doExecute now continues the chain and getArgumentId is concrete. That is source/binary compatible for existing subclasses, and the Consumer<Store> constructor still preserves the eager, custom-adapter path (covered by customAdapterPreservesEagerInvocationTest), so this is fine — just note that the two constructors now mean "bridge" vs "custom adapter", not "default host functions" vs "custom host functions".

Notes (non-blocking):

• synchronized (wasmLoader) serialises every guest call for one plugin instance, hooks included. That is the safe choice for shared guest linear memory; a short comment on why the lock is on the loader (shared across the plugin, not per-bridge) would prevent it from being "optimised" later.
• WasmResult.apply mutates the response headers before the continuation runs; if the downstream chain then fails, the header changes stay applied. Probably intended, but worth a comment.
• customAdapterPreservesEagerInvocationTest proves eagerness through the mocked chain interaction; asserting the guest itself was entered (e.g. a counter in the fixture) would make the intent sharper.

Approve. This adds a reusable, well-designed JSON request/configuration/result bridge for WASM plugins, and I focused my review on the security and correctness of the bridge.

Key points verified in the diff:

• Export lookup correctness. WasmLoader.getWasmExtern now iterates the module ExportSection and only returns a function when export.exportType() == ExternalType.FUNCTION. This fixes a real bug where a non-function export like memory would have been treated as a callable (or thrown). A new WasmLoaderTest covers present/absent/non-function cases.
• Strong result validation. WasmResult.parse enforces: strict UTF-8, JSON object with version == 1, a closed set of fields, action ∈ {CONTINUE, REJECT}, REJECT requires an HTTP status in 400–599 and cannot set request headers, CONTINUE cannot set status/body, header names are validated (no framing headers such as Host/Content-Length, valid token pattern, unique, no control characters). Jackson is configured with FAIL_ON_TRAILING_TOKENS and STRICT_DUPLICATE_DETECTION.
• Lifecycle safety. AbstractShenyuWasmPlugin now implements AutoCloseable; the bridge serializes guest-memory access via synchronized (wasmLoader), and close() is idempotent and refuses further invocations. begin/end/close clear argument/result state, avoiding leaks.
• Backward compatibility. The Consumer<Store> constructor retains custom host adapters (bridge = null), and the legacy path is unchanged; existing custom-constructor Go/Rust tests remain as regression coverage.

Tests (customAdapterPreservesEagerInvocationTest, plus the referenced WasmBridgeTest/WasmRequestBridgeTest) exercise both the supported bridge and the legacy adapter, and the README documents the protocol, limits and fixture regeneration clearly.

Non-blocking: the README is extensive; please make sure the referenced CI command and wat2wasm fixture-regeneration steps are wired into the build/CI so the committed .wasm fixtures stay reproducible.

Summary

Fresh review of the current head 0b0f780fb02c8b65cdbf15b98764a9f430dac1ae (open, not draft, author yangjj-iso). Fixes/Closes #7561. Adds a reusable request/configuration/result bridge to AbstractShenyuWasmPlugin, plus shared fixture-verification hardening and E2E CI regression repairs.

What the change does

• AbstractShenyuWasmPlugin no-arg constructor now installs the JSON bridge (AbstractShenyuWasmPlugin.java:77), while the Consumer<Store> constructor keeps custom host adapters (:90). doExecute(..., Long) and getArgumentId(...) are no longer abstract; they are concrete with a default chain-continuation body (:126, :198). The class now implements AutoCloseable (:56, close() at :232). Bridge execution serializes on the loader and expires state in finally (:147–:169).
• WasmRequestBridge (new) registers shenyu.get_args/shenyu.put_result, emits a versioned UTF-8 JSON snapshot of request metadata, cached PluginData and matched SelectorData/RuleData, caps payloads at 1 MiB and validates buffer ranges and single-submit (WasmRequestBridge.java:87–:129).
• WasmResult (new) validates strict JSON (FAIL_ON_TRAILING_TOKENS + STRICT_DUPLICATE_DETECTION), version 1, a field whitelist, CONTINUE/REJECT semantics, 400–599 reject status, UTF-8 bodies and header names/values, and rejects framing headers (WasmResult.java:59–:163). Invalid results fail closed with HTTP 500.
• E2E/shared-infra: local httpbin fixture wired into six consuming Compose files with a healthcheck (compose/fixtures/httpbin.py, httpbin.yml), RocketMQ access-log consumption switched to a lite-pull consumer with topic-creation waiting (DividePluginCases.java:87), Spring Cloud route-readiness wait, and WaitForHelperTest rewritten against an ephemeral local HttpServer instead of postman-echo.
• CI hardening: verify-wasm-fixtures.py now enumerates wasm-* families and requires wasm-gateway/wasm-concurrent; new test-wasm-fixtures.py covers failure cleanup/restoration.

Verification

• Compared against master: master's AbstractShenyuWasmPlugin has no bridge (no-arg constructor used a null initializer → built-in no-op get_args/put_result), and doExecute(..., Long)/getArgumentId were abstract. Removing abstract is source- and binary-compatible for subclasses; the in-repo subclasses (ReferenceWasmPlugin, WasmTestPlugin) use the Consumer<Store> constructor and continue on the legacy path. WasmLoader.java is unchanged by this PR.
• Local: mvn -pl shenyu-plugin/shenyu-plugin-wasm-base -am test → BUILD SUCCESS. wasm-api 13/13 (AbstractWasmPluginTest 4, WasmLoaderTest 1, WasmLoaderConcurrentTest 8); wasm-base 97/97 (WasmBridgeTest 15, WasmRequestBridgeTest 38, AbstractShenyuWasmPluginTest 7, WasmGatewayTest 9, WasmLifecycleContractTest 4, handlers 24). Zero failures/errors/skips.
• CI: all relevant checks green — pr_build, rebuild-and-test, Analyze (java), check-license-header, rat_check, scope, sql-matrix, registry_dependencies, agent_gateway_migrations, k8s-examples-http, build-docker-images, and every e2e-case/e2e-storage. No red to attribute.

Notes (non-blocking)

• The no-arg constructor is a documented behavior change for external subclasses that relied on no-op callbacks; the super((Consumer<Store>) null) escape hatch and RELEASE-NOTES/README cover it. Acceptable for a minor release but worth calling out in the changelog (already done).
• The PR is broad: it bundles the #7561 bridge with E2E CI repairs and fixture-verifier hardening (shared with #7581). All parts were verified independently here and CI is green; merging #7581 first as the description requests keeps the shared files tidy.
• close()/AutoCloseable requires explicit destroy wiring (documented); no starter wiring is added, which matches the stated scope.

Conclusion

APPROVE. The bridge contract is strict and fail-closed, the legacy adapter path is preserved, the behavior change is documented with an escape hatch, and both CI and local module tests pass.

Re-approving. The only commit after my last review is 4f652e05, and it is a merge of master (4d33568d) into the branch — two parents, no hand-written changes on top of 0b0f780f. So the code is exactly what I reviewed and approved; the approval was dismissed by the push, not by a code change.

CI on 4f652e05 is green for everything that runs on this PR: 17 pass, 10 skipping, 0 pending, 0 fail.

One thing to be aware of when it lands: the merge brought in 251 files of master movement, so the squash commit will be large and the diff against master is no longer a clean representation of this PR's intent. If a maintainer wants a readable history entry, the PR description should carry the summary rather than relying on the commit diff.

Approved. The reusable JSON request/configuration/result bridge is well designed and security-conscious. WasmRequestBridge uses the standard grow-buffer get_args protocol, bounds-checks the guest address/length against memory and the 1 MiB payload cap, and rejects expired or unknown invocation IDs. WasmResult performs strict JSON parsing (fails on trailing tokens, duplicate keys, malformed UTF-8), requires version 1, restricts action to CONTINUE/REJECT, requires a 400-599 status on REJECT, forbids request-header changes on reject, blocklists framing/hop-by-hop headers (Host, Content-Length, Connection, ...) and control characters, and applies request-header changes through an immutable decorator. The behavioral change (no-arg constructor now selects the JSON bridge) is documented in RELEASE-NOTES with a migration path for subclasses needing the old no-op callbacks, and the protocol is fully documented in the module README. Tests cover the flow with real WASM fixtures. Nice work.

Reviewed the reusable Wasm request/configuration/result bridge and its interaction with the plugin execution path. No actionable finding.

Validation: 63 focused bridge/plugin/loader tests passed. The PR currently has merge conflicts; conflict resolution needs revalidation. Apache RAT passed locally. Scope: source review and targeted verification; full integration/e2e suites were not run locally for this review.

Re-reviewed the current integration and confirmed the previously reviewed Wasm bridge production logic is unchanged. Focused bridge/plugin/loader tests: 63 tests passed. No actionable finding. The sampled failing registry_dependencies CI job hits the existing Admin constructor-fixture mismatch tracked by #8247; required CI must be green before merging.

Checkstyle and a separate Apache RAT check passed locally. This review covers source and the stated targeted verification; full Docker integration/E2E suites were not run locally.

Stream Agent analysis

FINAL synthesis of PR #7569 (head 85d468a) from 7 partition reviews: part-0 docs PASS, part-1 test PASS, part-2 docs PASS, part-3 impl PASS, part-4 tests PASS, part-5 binary .wasm resources INCONCLUSIVE (no diff hunks), part-6 WAT/Makefile PASS. all_parts_passed false, complete_diff false, CI not passed. Verdict INCONCLUSIVE due to incomplete evidence.

Evidence

• Part-5 review node: INCONCLUSIVE; added binary .wasm resources with available:false, additions:0/deletions:0, no diff hunks (frozen attachment).
• CI object: check_count 10, complete true, passed false, legacy_state pending.
• complete_diff false, global_complete_diff false in frozen cross-file review attachment.

Recommended next steps

• Provide complete diff hunks for the added binary .wasm resources (or confirm they are generated from WAT sources in part-6).
• Ensure all CI checks pass (currently ci.passed=false) before final approval.
• Re-run FINAL review after complete_diff true and all partition uncertainties resolved.

Scope: captured diff and GitHub check metadata only; no local tests were executed.

Task: a70bc237-a2c3-4c31-bcfe-3348d29b3b67 · Commit: 85d468a095863d5d25e208146f5a827658ba0c47

Review verdict: INCONCLUSIVE

SYNTHESIS/FINAL scope: child part-5 returned INCONCLUSIVE due to missing/binary patch (no diff hunks available), making all_parts_passed false. complete_diff false and CI passed false. Rules forbid overriding INCONCLUSIVE with PASS and require all_parts_passed true, complete_diff true, CI passed for FINAL PASS. No HIGH/CRITICAL findings anchored in any child; uncertainty preserved through INCONCLUSIVE verdict.

Publication: COMMENT (INCONCLUSIVE_REVIEW)

Review details

This PR adds a reusable WASM JSON bridge. Partition reviews found no concrete blocking defects in supplied code/test/doc segments, but partition part-5 (added binary .wasm test resources) had no available diff hunks and is INCONCLUSIVE. Moreover complete_diff is false and CI is failing/pending. Therefore the final verdict is INCONCLUSIVE; please supply full diff including binary resource details and ensure CI passes before a complete review can be finalized.