Repository navigation
fix(bridge): accept JSON array envelopes for parallel task dispatch - #136
Conversation
The bridge detector returned on the first parsed envelope and the
response builders hardcoded a single tool call at index 0, so fan-out
requests dispatched one task and silently dropped the rest.
Accept a JSON array of envelopes (all-or-nothing validation), emit all
calls with sequential stream indices, and document the array form in the
task bridge prompt. Only [{ opens a candidate so markdown that starts
with [ still streams through.
Nomadcxx
left a comment
Nomadcxx
left a comment
There was a problem hiding this comment.
Review
Task fan-out on the bridge-JSON path is actually fixed: a complete array of valid envelopes becomes N OpenAI tool_calls with sequential indices and unique ids, then the agent is killed. All-or-nothing validation, [{ vs markdown [, and both plugin copies look correct. Native parallel Cursor events staying out of scope is the right call.
Ready to merge. The comments below are prompt wording, a type smell at the emit boundary, and two test holes that would not catch a silent return to “dispatch 1, drop the rest.”
What works
- Array extract fails closed (
[]or any invalid entry →null/ passthrough). - Detector buffers
[and only becomes a candidate on[{;[docs](url)streams through. - Ids are
call_bridge_<hash>_<i>, so duplicate envelopes still unique. emitToolCallAndTerminateemits the full list, then kills. One SSE chunk with everytool_calls[i]matches the existing single-call builder.
| For Task only, the exact envelope below overrides the earlier generic "standard OpenAI tool_call" instruction. Do not add id, type, or function fields, and do not stringify arguments. | ||
| OpenCode owns the task tool. Do not invoke Cursor's built-in Task tool; it uses a different subagent list. To call OpenCode's task tool, respond with exactly one JSON object and no prose: | ||
| {"name":"task","arguments":{"description":"3-5 words","prompt":"task details","subagent_type":"one name listed in the OpenCode task description"}} | ||
| To dispatch several tasks in parallel, respond instead with exactly one JSON array of these objects and no prose: [{"name":"task","arguments":{...}},{"name":"task","arguments":{...}}] |
There was a problem hiding this comment.
Line 21 still says “exactly one JSON object”; this line says “respond instead with a JSON array.” Models that treat the first sentence as a hard rule will keep emitting one object and you get the old 1-of-N dispatch.
Collapse to one sentence, e.g. “respond with one JSON object, or one JSON array of those objects for parallel dispatch, and no prose.”
The prompt unit test (adds task bridge instructions only when task is offered) never asserts the array line, so deleting it would not fail CI. The live cursor-agent check in the PR body depended on this sentence.
| role: "assistant", | ||
| content: null, | ||
| tool_calls: [toolCall], | ||
| tool_calls: [toolCalls].flat(), |
There was a problem hiding this comment.
Extract already normalizes a singleton object to [call]. Reopening OpenAiToolCall | OpenAiToolCall[] here (and on ProviderBoundary / emitToolCallAndTerminate) is two in-memory shapes for the same wire value, plus [x].flat() copied into plugin log lines.
Take OpenAiToolCall[] only and wrap at the two native intercept sites: emitToolCallAndTerminate([toolCall]). Drops the union, the flats, and the empty-array hole (tool_calls: [] + finish_reason: "tool_calls" is type-legal today).
| extractBridgeToolCallsFromText(...args)?.[0] ?? null; | ||
| const extractBridgeToolCallFromStreamOutput = ( | ||
| ...args: Parameters<typeof extractBridgeToolCallsFromStreamOutput> | ||
| ) => extractBridgeToolCallsFromStreamOutput(...args)?.[0] ?? null; |
There was a problem hiding this comment.
These ?.[0] wrappers mean almost every existing extract test still passes if the new API starts returning only the first call — the original production bug.
Keep the wrappers if you want, but add one test that calls extractBridgeToolCallsFromStreamOutput (not the wrapper) on a 3-element NDJSON array and asserts .length === 3 and unique ids, then createToolCallCompletionResponse(meta, calls).choices[0].message.tool_calls length 3. That is the non-stream path this PR rewired in both plugin handlers and currently has zero coverage.
Also split the detector assert at L367 (expect(decision.action === "tool_call" && decision.toolCalls.length).toBe(2)): true && 2 happens to equal 2; it does not check names or ids.
| expect(toolCalls.map((call: any) => call.index)).toEqual([0, 1, 2]); | ||
| expect(toolCalls.map((call: any) => JSON.parse(call.function.arguments).subagent_type)) | ||
| .toEqual(["explore", "general", "explore"]); | ||
| expect(new Set(toolCalls.map((call: any) => call.id)).size).toBe(3); |
There was a problem hiding this comment.
Sibling reassembles streaming Task bridge JSON without leaking fragments also asserts finish_reason === "tool_calls" and that the JSON is not leaked as delta.content. This test would still pass if the array became three tool_calls and the buffered [/JSON flushed as text, or if finish_reason stayed stop. Copy those two assertions down.
| } | ||
| const toolCalls = parsed.map((entry, i) => | ||
| buildBridgeToolCall(entry, `${id}_${i}`, allowedToolNames, writeSchema)); | ||
| return toolCalls.length > 0 && toolCalls.every((call): call is OpenAiToolCall => call !== null) |
There was a problem hiding this comment.
All-or-nothing flips the failure mode without leaving any trace. If one envelope in an N-array fails buildBridgeToolCall (bad arg, unoffered tool, mixed task/write), the whole fan-out dequeues as assistant text: on the stream path containsCompleteJson → releaseBuffer() prints the raw JSON, and the non-stream path falls through to extractCompletionFromStream with the same text. That is arguably better than the old silent 1-of-N, but unlike a normal rejection there is no log signal at all — bridge-json.ts has no logger, so production triage of a "model spat JSON back at me" report has nothing to hook into. Two suggestions:
log.warn(with envelope count + first rejection reason) when an array is dropped — cheap and makes the fail-closed path observable.- A sanity cap on array length. Nothing prevents a model emitting a 100-element array → one chunk carrying
tool_calls[0..99]→ 100 parallel subagent spawns in OpenCode. Even a reasonable limit (say 10–20) with the same all-or-nothing fallback would bound blast radius.
Also note the write path is now array-capable via the same builder but completely untested — a 2-element write array (a plausible near-term use, "write these three files" in parallel) degrades to text if any entry fails validation.
| return null; | ||
| } | ||
|
|
||
| const id = `call_bridge_${shortHash(jsonText)}`; |
There was a problem hiding this comment.
Nit: content-derived ids are deterministic across turns in a resumed chat. call_bridge_<hash(jsonText)>_<i> is a pure function of the envelope text, so if the model re-emits the same array in a later turn (retry after a failed dispatch — exactly the issue #123 scenario), every id in the second batch duplicates the first. Single-object envelopes already had this property, but a retried fan-out now reuses the entire _0.._n id set, and tool results are correlated by id in the following request, so a long session can mis-associate results across turns. Cheap fix: salt with the per-request meta.id (or a module-level counter) so uniqueness no longer depends on envelope content differing. Non-blocking.
Nomadcxx
left a comment
Nomadcxx
left a comment
There was a problem hiding this comment.
Independent review — fix(bridge): accept JSON array envelopes for parallel task dispatch
Verified by cloning the head commit: unit suite tests/unit/proxy/bridge-json.test.ts (44 pass, incl. all 4 new tests), opencode-loop.integration.test.ts (24 pass / 1 skip, incl. the new array scenario), and npx tsc --noEmit reports identical 80 pre-existing errors on main and this branch (no new type errors — PR body claim checks out).
What works
- Array extraction is all-or-nothing and fail-closed:
[]and any invalid entry →null→ passthrough text. - Detector buffers
[and only[{opens a candidate;[docs](url)streams through (regression-tested). The old[{, ]shape check is replaced cleanly by/{1,2}|[\s*$/ + /({|```|[\s*{)/`. - Ids
call_bridge_<hash>_<i>keep duplicate envelopes unique;tool_callsindices0..n-1inside one delta chunk are legal per the wire spec and parsed per-index by the AI SDK (both standard SDKs accumulate byindex, so multiple entries in one chunk is fine). - Both plugin copies (SSE route + HTTP route) were updated consistently, and the widened
OpenAiToolCall | OpenAiToolCall[]keeps the native single-call intercept sites compiling untouched. tscparity + passing suites are the right evidence a one-commit PR like this needs.
Existing review comments (author self-review, all accurate — for the record)
- bridge-json.ts:23 — array instruction sits under “exactly one JSON object” → conflicting prompt rules. Agreed; addendum below.
- tool-loop.ts:214 —
OpenAiToolCall | OpenAiToolCall[]union is two in-memory shapes for one wire value + a type-legaltool_calls: []empty-array hole. Agreed —OpenAiToolCall[]only, wrap at the 3 native intercept sites. - bridge-json.test.ts:15 —
?.[0]wrappers mean single-call tests still pass if arrays regress to first-call-only. Agreed; also add your proposed directextractBridgeToolCallsFromStreamOutputarray assertion — the non-stream path is genuinely uncovered. - opencode-loop.integration.test.ts:949 — array scenario omits the sibling test’s
finish_reason+ content-leak assertions. Verified: sibling asserts both at L918/L924.
New comments posted
- bridge-json.ts:188 — partially-invalid array degrades the whole fan-out to visible JSON text with zero log signal (
bridge-json.tshas no logger); no array-length cap (a 100-element array → 100 parallel subagent spawns); write-array path untested. - bridge-json.ts:181 — content-hash ids are deterministic across resumed turns; a retried fan-out reuses the entire id set and can mis-associate tool results. Salt with
meta.idor a counter.
Addenda (could not thread-reply to your comments; API 404 on replies)
- To #1: the new array example is self-referential (
arguments:{...}shorthand) — the model must dereference the object example above it under conflicting “exactly one” rules. If you collapse to one sentence, keep one complete envelope inline in the array example. Confirmed your CI-gap claim: the prompt unit test asserts"name":"task"but no array fragment; consider addingtoContain("JSON array")so the line can’t silently die. - To #4: there is no
stream: falsearray scenario at all — a non-stream 2-event scenario assertingcreateNonStreamToolCallResponseoutput would close the loop on the other two handlers.
Verdict
No blocking issues. The fix is correct, minimal, and well-tested for the stream path; the riskier seams are the all-or-nothing drop (observability) and the contradictory prompt (effectiveness). I’d ship after: warn-logging/length cap for dropped arrays + the author’s four points (prompt rewrite, union→array type, the two test additions). Approve-as-comment.
Nomadcxx
left a comment
Nomadcxx
left a comment
There was a problem hiding this comment.
Independent follow-up review
Verified from a fresh clone of head 2a72ec5:
bun test tests/unit/proxy/bridge-json.test.ts→ 44 pass (incl. all 4 new tests)bun test tests/integration/opencode-loop.integration.test.ts→ 24 pass / 1 skip (incl. the newassistant-bridge-task-arrayscenario)npx tsc --noEmit→ 80 errors on bothmainand this branch — identical sets, no new type errors (PR body claim confirmed)- CI on the head commit: unit ✅, integration ✅
I also read the diff line-by-line. My verdict matches the prior reviews: the core fix is correct, minimal, and fail-closed — a complete array of valid envelopes becomes N tool_calls with sequential indices and per-entry ids; anything else (empty array, any invalid entry, mixed invalid) degrades to passthrough text rather than dispatching 1-of-N. The [ vs [{ discrimination, markdown-link passthrough, and consistency across both plugin handler copies all check out. Keeping native parallel Cursor events out of scope remains the right call given the --force double-execution hazard.
Existing comments — noted and confirmed
Six inline comments and three review bodies are already on this PR (all from @Nomadcxx: a self-review plus two summary reviews). For the record, I independently re-verified each claim and all are accurate:
- bridge-json.ts:23 (prompt contradiction) — confirmed: "exactly one JSON object" and the new "respond instead with exactly one JSON array" sit in the same block, and the array example's
{...}shorthand is self-referential. Confirmed the CI gap:bridge-json.test.ts:318-325asserts'"name":"task"'but never asserts the array line, so it could silently disappear. - tool-loop.ts:214 (union type) — confirmed:
OpenAiToolCall | OpenAiToolCall[]reopens two in-memory shapes for one wire value,[x].flat()is repeated in the plugin log lines, andtool_calls: []+finish_reason: "tool_calls"is currently type-legal (though unreachable today — both call sites guard on non-null extraction).OpenAiToolCall[]-only with wrapping at the native intercept sites is cleaner. - bridge-json.test.ts:15 (
?.[0]wrappers) — confirmed: every pre-existing single-call extract test still passes if the new API regressed to first-call-only, i.e. the original production bug is only weakly guarded. There is also still zero direct coverage ofcreateToolCallCompletionResponsewith an array (tool-loop.test.ts:415 only exercises the single-call object). - opencode-loop.integration.test.ts:949 — confirmed: the sibling test asserts
finish_reason === "tool_calls"and no JSON leak indelta.content; the new array test omits both. - bridge-json.ts:188 (observability + cap) — confirmed:
bridge-json.tshas no logger, so a dropped fan-out (including a 100-element array, which nothing bounds) is invisible in production. A warn-log with the rejection reason and a sanity cap (10–20) are cheap and worthwhile. - bridge-json.ts:181 (id determinism across turns) — confirmed:
call_bridge_<sha256(jsonText)[0:12]>_<i>is a pure function of envelope text, so a retried identical fan-out reuses the full_0.._nid set; tool results are correlated by id downstream. Salting withmeta.idor a counter is the right fix.
New, non-blocking observations (not covered above)
- tool-loop.ts:236 — all N tool_calls arrive in one delta. See inline comment. Optional.
- bridge-json.ts:122 (
reset()) — buffered text is silently discarded. With[now also buffered in the undecided state (alongside the pre-existing backtick buffering), a lone[or backtick followed by a nativetool_callevent loses that character instead of flushing it. Pre-existing pattern, marginally wider exposure now;reset()could return/flush the pending buffer. Trivial. - Latency note: any assistant message that starts with a complete object-array JSON literal (e.g. example data in an ordinary answer,
[{"id":1}]) now holds as a candidate until the JSON completes and is then flushed verbatim as one chunk — the same trade-off{...}already had, now extended to arrays. No loss, just delayed streaming; fine for bridge-sized payloads, worth remembering if envelopes grow. - Mixed task+write arrays are accepted by the builder (any allowed tool per entry) although the prompt documents task-only arrays — consistent with point 5's note that the write-array path is reachable but untested.
Verdict
No blocking issues. Same pre-merge list as the prior reviews: collapse the prompt sentences (and add toContain("JSON array") to the prompt test), collapse the union to OpenAiToolCall[], add the direct non-stream extract + array-builder tests and the missing finish_reason/leak assertions, and add the warn-log + length cap. Ship it after those.
| ...toolCall, | ||
| }, | ||
| ], | ||
| tool_calls: [toolCalls].flat().map((toolCall, index) => ({ index, ...toolCall })), |
There was a problem hiding this comment.
Nit / optional robustness: all N tool_calls are emitted inside a single delta. That's legal per the wire spec and the major SDKs accumulate per-index, but minimal OpenAI-compatible clients that only read delta.tool_calls[0] will silently drop calls 1..n. Emitting one chunk per call (N tool deltas, then the finish chunk) would mirror OpenAI's real streaming shape more closely. Not blocking — just cheap insurance if third-party clients hit this proxy.
- Single prompt rule: one object, or one array for parallel dispatch, with a complete inline array example. - Builders take OpenAiToolCall[] only; native sites wrap their call. - Stream one chunk per tool call, mirroring the upstream wire shape. - Warn-log when an array is dropped to passthrough text. - Random bridge call ids so retried envelopes do not reuse ids. - Tests: direct non-stream array extraction, array completion response, write arrays, retry ids, prompt array line, finish_reason and leak assertions, non-stream integration scenario.
1 participant
Addresses #123 (Task fan-out part). Native parallel tool events are not covered; see "Out of scope".
Problem
taskreaches OpenCode only through the bridge-JSON text envelope. The prompt asked for "exactly one JSON object",BridgeJsonStreamDetectorreturned on the first parsed object, andemitToolCallAndTerminatethen killed cursor-agent. The response builders also hardcoded one tool call atindex: 0. Asking for N parallel subagents dispatched 1 and silently dropped the rest.Change
src/proxy/bridge-json.ts: the Task bridge prompt documents a JSON array form for parallel dispatch.extractBridgeToolCallsFromText/extractBridgeToolCallsFromStreamOutputreturnOpenAiToolCall[]. Arrays are all-or-nothing: an empty array or any invalid entry falls back to passthrough text. Ids arecall_bridge_<hash>_<i>. Only[{(not a bare[) opens a candidate, so markdown that starts with a link still streams immediately.src/proxy/tool-loop.ts,src/provider/boundary.ts: the builders accept one call or an array and emit sequentialindexvalues.src/plugin.ts: both streaming handlers and both non-stream fallbacks pass every call through.Verification
[markdown](link)passthrough, stream indices 0..n-1.tool_callswith indices[0,1,2]and unique ids.tsc: no new errors vsmain.npm run buildpasses. The full suite's 40 failures are identical onmain(cross-file interference);opencode-loop.integration.test.tspasses on its own.Out of scope: native parallel tool calls
A capture of three parallel reads showed every
startedevent arriving before anycompletedevent, but about 1.07s apart. Batching them means waiting for the firstcompletedevent, and by then cursor-agent (running with--forceby default) has already executed the calls itself, so OpenCode would run shell commands and edits a second time. That path still stops at the first call.