fix(sdk/graph,scriptnode): repair built-in JS scripts against v0.3 bindings#89
Merged
Merged
Conversation
…ndings
Several built-in node scripts under sdk/graph/node/scripts referenced
removed v0.2.x globals or otherwise drifted out of sync with the
v0.3.0 bindings:
- answer.js called stream.emit("answer", ...). The stream binding was
retired in v0.3 along with workflow.StreamCallback, so the script
threw ReferenceError on every default invocation.
- approval.js read config.__node_id for approval_request.node_id, but
nothing in scriptnode ever injected that reserved key — the field was
permanently undefined.
- iteration.js passed a stale third argument to runtime.execScript,
silently dropped child signals, and leaked the previous iteration's
__iteration_result when the body skipped setting it.
Fixed by routing emission through the canonical per-node publisher and
adding the missing identity bridge:
- bindings.NewHostBridge gains a StreamEmitter parameter exposing
host.emit(type, payload) — backed by ctx.Publisher, which the
executor pre-bakes with runID/nodeID so subjects land on the
canonical engine.run.<runID>.stream.<nodeID>.delta channel. No
separate stream bridge: host already speaks engine.Publisher.
- New graph-layer bridge scriptnode/bridge_node.go exposes node.id() /
node.type(). NodeID is per-step and graph-specific, so the bridge
lives in scriptnode rather than in bindings (which by design knows
nothing about graph; see sdk/script/bindings/doc.go).
- agent.RunInfo stays per-run immutable. NewRunInfoBridge is unchanged
on the script side.
- scriptnode wires NewRunInfoBridge(agent.RunInfo{RunID: ctx.RunID})
alongside the new node bridge so scripts read run vs node identity
from clearly-scoped globals.
- iteration.js clears __iteration_result before each body run, checks
the child signal before pushing results, and propagates
signal.interrupt / error / done to the parent loop.
- register.go renames needsShellFS -> needsShell and tightens the doc
string ("only shell is the extra bridge", fs is unconditional).
Adds an integration-style test file (builtins_test.go) that drives
answer / approval / iteration / node-bridge through ScriptNode with a
recording publisher and a recording host so future binding renames
trip the suite instead of breaking silently at runtime.
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several built-in JS node scripts under
sdk/graph/node/scriptshad silently drifted out of sync with the v0.3 binding surface:answer.jsreferencedstream.emit("answer", ...)— thestreamglobal was retired in v0.3 along withworkflow.StreamCallback, so the node threwReferenceErroron every default invocation.approval.jsreadconfig.__node_idforapproval_request.node_id, but nothing in scriptnode ever injected that reserved key. The field was permanentlyundefined.iteration.jspassed a stale third argument toruntime.execScript, silently dropped child signals, and leaked the previous iteration's__iteration_resultwhen the body skipped setting it.Approach
Restored emission through the executor's canonical per-node channel and added the missing identity bridge, while keeping each layer aware of only its own concepts:
host.emit(type, payload)bindings.NewHostBridge(gainsStreamEmitterparameter, backed byctx.Publisher)run.*)bindings.NewRunInfoBridge(agent.RunInfo{...})— unchanged on the script sidenode.id(),node.type())sdk/graph/node/scriptnode/bridge_node.go(kept out ofbindingsper its no-graph-deps invariant)host.publish,askUser,checkInterrupt,reportUsage)bindings.NewHostBridgeKey invariants preserved:
agent.RunInfostays per-run immutable — no per-step fields added.sdk/script/bindingsremains zero-dependency onsdk/graph(seebindings/doc.go).host.emituses a localStreamEmitterduck-typed interface thatgraph.StreamPublishersatisfies, so the executor's per-node publisher wiring carries the run/node identity inside the subject without leaking it back through host.Other fixes folded in
iteration.js: clear__iteration_resultper iteration, check child signal before pushing, propagatesignal.interrupt/error/doneto the parent loop.scriptnode/register.go: renameneedsShellFS→needsShelland tighten the comment ("only shell is the extra bridge"; fs is unconditional).Test coverage
New
sdk/graph/node/scriptnode/builtins_test.godrivesanswer/approval/iterationand the newnodebridge throughScriptNodewith a recordinggraph.StreamPublisherand anenginetest.MockHost, so future binding renames trip the suite at compile/test time instead of breaking silently in production.bridge_host_test.gogains coverage forhost.emitforwarding and the nil-emitter no-op contract.Test plan
cd sdk && go build ./...cd sdk && go test ./...— all packages greenanswer/approval/iterationnodes once mergedMade with Cursor