feat(plugin): replace simulateJSFilter with real QuickJS execution#133
Conversation
, #127) QuickJSRuntime now embeds a real QuickJS engine (compiled to WASM, running on wazero — zero CGo) via github.com/fastschema/qjs. Plugin JS files are evaluated in the QuickJS VM and CallFilter invokes the actual JS function instead of pattern-matching against known Go equivalents. - Init() creates qjs.Runtime, sets up alloy global with filter/shortcode/hook/on - EvalFile() transforms ES module syntax to IIFE, evaluates in QuickJS - CallFilter() invokes the registered JS function and converts the result - Removes simulateJSFilter, filterBodies, and all regex-based parsing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ee5ab15 to
f0de3c2
Compare
zeroedin
left a comment
There was a problem hiding this comment.
Review: PR #133 — feat(plugin): replace simulateJSFilter with real QuickJS execution
Scope: `internal/plugin/wasm.go` (145 added, 86 removed), `go.mod`, `go.sum`
What changed
Replaces the entire regex-parsing + pattern-matching scaffold with a real QuickJS engine:
-
`Init()` — Creates `qjs.Runtime` via `github.com/fastschema/qjs` (QuickJS compiled to WASM, running on wazero — pure Go, zero CGo). Sets up Go→JS callback bridges (`__registerFilter`, `__registerShortcode`, `__registerHook`) and creates the `alloy` global with JS-side methods that store callbacks in `__filters`/`__shortcodes`/`__hooks` and notify Go-side maps.
-
`EvalFile()` — Transforms `export default function(alloy)` module syntax to an IIFE via regex, then evaluates in the QuickJS VM. The JS execution itself handles filter/hook/shortcode registration through the `alloy` global — no more regex parsing of registrations.
-
`CallFilter()` — Sets input as `__callInput` global, evals `__filters"name"`, converts result via `jsValueToGo()`.
-
Removed: `simulateJSFilter`, `filterBodies`, `filterBodyRegex`, `filterRegex`, `shortcodeRegex`, `hookRegex`, `onRegex` — all regex-based parsing.
-
Added: `Close()` method, `jsValueToGo()` type converter, `moduleExportRegex` for IIFE transform.
Architecture
The two-layer bridge is clean:
- JS→Go (registration): `alloy.filter("name", fn)` stores `fn` in JS-side `__filters` dict AND calls Go-side `__registerFilter` to update `r.filters` map
- Go→JS (invocation): `CallFilter` sets `__callInput` global, evals `__filters"name"`, converts result back
Issues to flag
1. `Close()` is never called (resource leak)
`QuickJSRuntime.Close()` exists but is never invoked — not in `registry.go:LoadPlugins`, not in `build.go`, not in tests. Each `Init()` creates a wazero WASM instance. For a single build this is fine (process exits), but for `alloy serve` with rebuilds, runtimes accumulate. The registry stores all runtimes in `r.runtimes` but has no `Close()` or `Cleanup()` method.
Suggestion: Add `Registry.Close()` that iterates `r.runtimes` and calls `rt.Close()`, called from the pipeline on shutdown.
2. Filter name interpolation in `CallFilter` — JS injection via format string
fmt.Sprintf(\`__filters["%s"](__callInput)\`, name)If a plugin registers a filter name containing `"`, the generated JS breaks. Not a security escalation (the plugin already runs arbitrary JS), but a correctness issue — a filter named `my"filter` would cause a syntax error instead of a clear "filter not found" message.
Suggestion: Escape `name` or use bracket notation with a variable: set name as a global string, then `__filters__callFilterName`.
3. IIFE transform assumes single default export per file
The `moduleExportRegex.ReplaceAllString` replaces ALL matches, but the closing `)(alloy);` is appended only once at the end. If a file has two `export default function(alloy)` blocks (invalid JS, but), the transform produces broken output. Not a practical concern — this is invalid JS anyway.
4. `hasSyntaxError` pre-check — false positives on braces in strings
Pre-existing issue (not introduced by this PR): the brace-counting doesn't account for `{` inside string literals, template literals, comments, or regex patterns. A valid JS file with `"{"` would be rejected. Now that QuickJS provides real syntax error reporting, this pre-check could eventually be removed. Low priority.
Observations (non-blocking)
- `__callInput` persists between calls: The global isn't cleaned up after each filter call. Not a correctness issue (it's overwritten on next call) but worth knowing.
- `jsValueToGo` integer detection: `f == float64(int(f))` correctly handles JS's "all numbers are floats" model. The int32 range check prevents overflow.
- wazero is transitive: `github.com/tetratelabs/wazero v1.9.0` appears in `go.sum` as an indirect dep of `fastschema/qjs`. Pure Go, no CGo — correct per spec.
- `fsnotify` and `gorilla/websocket` moved from indirect to direct in `go.mod`: These are used by the server package. The move is cosmetically correct (they're imported directly, not transitively).
Verdict
LGTM — this is the right change. Real JS execution replaces a fundamentally broken pattern-matching hack. The `reverse.js` test that was impossible to pass now passes. Flag the `Close()` resource leak as a follow-up.
…Close() - CallFilter uses global variable for filter name instead of fmt.Sprintf interpolation, preventing JS syntax errors from names with special chars - Add Registry.Close() to release all QuickJS WASM runtimes on shutdown Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove hasSyntaxError brace-counting pre-check — QuickJS provides real syntax error reporting with filename and line numbers, and the old check had false positives on braces in strings/templates/comments - Clean up __callInput and __callFilterName globals after each CallFilter invocation to avoid stale references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin
left a comment
There was a problem hiding this comment.
Re-review: PR #133 — commits f59762d and b403d2d address all review items
Commit f59762d — safe filter name dispatch + Registry.Close()
Filter name injection (resolved):
// Before: fmt.Sprintf interpolation
fmt.Sprintf(`__filters["%s"](__callInput)`, name)
// After: global variable, no interpolation
r.ctx.Global().SetPropertyStr("__callFilterName", r.ctx.NewString(name))
r.ctx.Eval("filter-call.js", qjs.Code(`__filters[__callFilterName](__callInput)`))Filter names with special characters now work correctly — the name is passed as a JS string value via the QJS API, never interpolated into JS source.
Registry.Close() (resolved):
func (r *Registry) Close() {
for _, rt := range r.runtimes {
rt.Close()
}
r.runtimes = nil
}Iterates all loaded runtimes and releases them. Setting r.runtimes = nil prevents double-close. Pipeline/serve callers can now clean up WASM resources on shutdown.
Commit b403d2d — remove hasSyntaxError, clean up globals
hasSyntaxError removed (resolved): QuickJS provides real syntax errors with filenames and line numbers. The brace-counting pre-check with its false-positive risk is gone.
Global cleanup after CallFilter (resolved):
r.ctx.Global().SetPropertyStr("__callInput", r.ctx.NewUndefined())
r.ctx.Global().SetPropertyStr("__callFilterName", r.ctx.NewUndefined())Both globals are reset to undefined after each call, preventing stale references.
Verdict
LGTM — all three review items resolved cleanly. No new concerns.
Summary
Closes #103. Closes #127.
github.com/fastschema/qjs— QuickJS compiled to WASM, running on wazero (pure Go, zero CGo), per spec PLAN.md §5Init()creates aqjs.Runtime, sets up thealloyglobal object withfilter(),shortcode(),hook(),on()methods that store JS callbacks for later invocationEvalFile()transformsexport default function(alloy)module syntax to an IIFE, then evaluates in the QuickJS VM — JS code actually runs nowCallFilter()invokes the registered JS filter function in the VM and converts the result back to Go types (string, int, float, bool)simulateJSFilter,filterBodies,filterBodyRegex, and all regex-based name parsing — the JS execution itself handles registrationTest plan
reversefilter from spec PR)reversefilter (split("").reverse().join("")) now returns"olleh"for"hello"— proves real JS execution, not pattern matching🤖 Generated with Claude Code