feat(flow): dynamic workflows — goja-based multi-agent orchestration#117
Merged
Conversation
Add internal/flow: a JavaScript workflow engine that runs user- or agent-authored orchestration scripts (`export const meta` + plain-JS body) to fan work out across many subagents. Embeds goja + goja_nodejs/eventloop; each agent() returns a promise settled back on the loop goroutine, so `await Promise.all([...])` fans out real parallel subagents while the JS stays deterministic. Primitives: agent/parallel/pipeline/phase/log/workflow/args/budget. Structured output via opts.schema; determinism guards (Date.now, Math.random, argless new Date throw); concurrency (16) + hard per-run (1000) agent caps; wall-clock watchdog with run-scoped cancel. - internal/flow: engine, async host bridge, loader, prelude, and 3 builtins (repo-audit, pr-review, roundtable). Pure package — the dependency runs one way: tools -> flow -> config. - internal/tools: flow_spawn (adk agent adapter) + workflow_run tool, registered across TUI/Web/ACP so the agent can write-and-run inline. - `jcode flow list|show|run` CLI (--args / --timeout / --concurrency). - Docs: site/docs/overview/workflows.md + changelog; internal design doc. Hardened by an adversarial multi-agent review and a real head-to-head comparison run (Claude vs glm-5.2 on the same workflows over this repo): fixed timeout not unblocking the run, a vmReady ctx guard, stripExports over-matching strings, and a run-scoped cancel so in-flight agents abort on timeout and OnAgentDone always pairs with OnAgentStart (no phantom progress). 24 tests, -race clean. Generated with Jack AI bot
📝 WalkthroughWalkthroughThis PR adds a dynamic workflow engine, workflow loader, built-in workflow scripts, CLI and tool integrations, and user-facing documentation, along with dependency updates for the new execution stack. ChangesDynamic Workflow Engine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as workflow CLI/Tool
participant Engine
participant Run as run (VM state)
participant Spawn as FlowSpawn
User->>CLI: workflow run <name> --args
CLI->>Engine: New(spawn, sink, opts).Run(ctx, wf, opts)
Engine->>Run: configure VM, compile & start
Run->>Run: execute agent()/phase()/workflow() calls
Run->>Spawn: spawn(AgentSpec)
Spawn-->>Run: AgentResult
Run->>Run: settle Promise via RunOnLoop
Run-->>Engine: flowOutcome (result/error)
Engine-->>CLI: result, error
CLI-->>User: printed/formatted result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.1)internal/flow/builtin/pr-review.jsFile contains syntax errors that prevent linting: Line 64: Illegal return statement outside of a function internal/flow/builtin/roundtable.jsFile contains syntax errors that prevent linting: Line 15: Illegal return statement outside of a function; Line 56: Illegal return statement outside of a function Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- flow.Validate(source): parse meta + compile the whole script as JS without running it. workflow_run tool and `jcode flow run` now pre-flight every inline/file script, so a syntax error returns to the agent to fix and spawns zero agents (costs no tokens). - New `jcode flow validate <name|file>` to check a script standalone. - Docs: new "Writing Workflows" chapter (site/docs/overview/writing-workflows.md) — full syntax reference: meta fields, every primitive signature, control-flow patterns, determinism, validation, common mistakes, a complete example. Cross-linked from the Dynamic Workflows overview; changelog updated. Generated with Jack AI bot
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
internal/flow/engine_test.go (1)
416-423: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleep with polling.
Line 417 can flake on slow CI. Poll until the sink observes the paired events or a deadline expires.
Proposed fix
- // Give the settle goroutine a moment to emit the (now out-of-loop) sink event. - time.Sleep(100 * time.Millisecond) - sink.mu.Lock() - starts, dones := sink.agentStarts, sink.agentDones - sink.mu.Unlock() - if starts != 1 || dones != 1 { - t.Fatalf("agent starts/dones = %d/%d, want 1/1 (phantom agent: unpaired start)", starts, dones) + deadline := time.After(2 * time.Second) + tick := time.NewTicker(10 * time.Millisecond) + defer tick.Stop() + for { + sink.mu.Lock() + starts, dones := sink.agentStarts, sink.agentDones + sink.mu.Unlock() + if starts == 1 && dones == 1 { + break + } + select { + case <-deadline: + t.Fatalf("agent starts/dones = %d/%d, want 1/1 (phantom agent: unpaired start)", starts, dones) + case <-tick.C: + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flow/engine_test.go` around lines 416 - 423, The settle-goroutine check in engine_test.go is flaky because it relies on a fixed time.Sleep before reading sink.agentStarts and sink.agentDones. Replace that sleep with polling in the same test block, repeatedly locking sink.mu and checking the counters until they reach 1/1 or a deadline expires, then fail with the same fatal message if the expected paired events never appear. Keep the logic localized around the sink inspection so the test remains stable on slow CI.internal-doc/dynamic-workflow-design.md (2)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPick one execution-path story for
agent().Line 4 says
agent()ridesSubagentTaskManager, but the later mapping table says v1 is intentionally decoupled and uses a semaphore/goroutine pair instead. Please align those sections so implementers don't follow two different contracts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal-doc/dynamic-workflow-design.md` at line 4, The `agent()` execution-path description is inconsistent across the document: one section says it rides `SubagentTaskManager`, while the mapping table says v1 is decoupled and uses a semaphore/goroutine pair. Update the `agent()` story so both the architecture summary and the v1 mapping table describe the same execution contract, using the same terms for the chosen implementation path.
109-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify whether structured output is schema-validated in v1.
Section 5 says
opts.schemareturns a validated object, but section 8 later marks schema conformity as an explicit non-goal. Please reconcile those sections so the shipped contract is unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal-doc/dynamic-workflow-design.md` around lines 109 - 110, Section 5 and section 8 currently conflict on whether `opts.schema` in the structured output flow is schema-validated in v1, so make the contract consistent. Update the `submit_structured_output`/`opts.schema` description and the non-goal statement to clearly say either validation is enforced or only best-effort parsing is done, and ensure the wording around agent retries and `error_max_structured_output_retries` matches that final behavior.go.mod (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake these new runtime deps direct.
internal/flowimportsgithub.com/dop251/gojaandgithub.com/dop251/goja_nodejs/eventloopdirectly, so theserequireentries shouldn’t be marked// indirect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` around lines 60 - 61, The go.mod dependency entries for github.com/dop251/goja and github.com/dop251/goja_nodejs are marked indirect even though internal/flow imports them directly. Update the require declarations in go.mod to make these runtime dependencies direct by removing the indirect markers, and keep the entries aligned with the actual imports used by the internal/flow package and its eventloop usage.internal/flow/loader.go (1)
45-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent failures on
ReadFilehide load bugs.
ReadDir/ReadFileerrors inloadBuiltin(lines 46-49, 55-57) andscanDir(line 73-75) are swallowed without logging, unlike the parse-error path inadd()which logs viaconfig.Logger(). A permission error or corrupted embed would silently drop a workflow with no diagnostic trail.♻️ Suggested fix
data, err := builtinFS.ReadFile(filepath.Join("builtin", e.Name())) if err != nil { + config.Logger().Printf("[flow] read builtin %s: %v", e.Name(), err) continue }Also applies to: 62-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flow/loader.go` around lines 45 - 60, The builtin loading paths are swallowing `ReadDir`/`ReadFile` failures without any diagnostic output, so update `Loader.loadBuiltin` and `Loader.scanDir` to log these errors instead of just continuing or returning silently. Use the existing `config.Logger()` pattern from `Loader.add` to report failures with enough context to identify whether the issue happened during builtin directory scanning or file reading, while keeping the current fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/flow/builtin/pr-review.js`:
- Around line 13-22: The `pr-review.js` diff command builder is interpolating
`args.base` directly into the shell string used by the `explore` agent, which
creates a command injection risk. Update the `base` handling in the `diffCmd`
construction to either validate `args.base` against a strict safe git-ref
pattern or stop passing it through shell interpolation entirely. Keep the change
localized around the `base` and `diffCmd` logic so the `agent(...)` call still
receives a safe, prebuilt command string.
In `@internal/flow/builtin/roundtable.js`:
- Around line 47-54: The `roundtable.js` synthesis prompt is misaligning
rebuttals because `critiques.filter(Boolean)[i]` reindexes a filtered array
while `panel.map((p, i) => ...)` still uses the original panel index. Update the
`Synthesize` prompt assembly to keep `panel` and `critiques` aligned by index in
the same iteration, or precompute a paired list before mapping, so each panelist
gets their own matching critique without shifting.
In `@internal/flow/engine.go`:
- Around line 99-103: The nested workflow path in runChild and run is resetting
resource tracking instead of sharing it, which lets child workflow() calls
bypass the intended WithConcurrency and WithMaxAgents caps. Thread the parent
limiter/counter state through runChild into run so child executions reuse the
same accounting rather than initializing fresh concurrency and agent counters
for each nested run. Update the run initialization logic and any related state
passed from runChild to ensure nested runs contribute to the same global limits.
In `@internal/flow/loader.go`:
- Around line 45-60: The builtin workflow loader uses an embedded filesystem
path built with filepath.Join in loadBuiltin, which can emit backslashes on
Windows and break embed.FS reads. Update Loader.loadBuiltin to construct the
ReadFile path with path.Join instead, while keeping the existing builtin entry
filtering and add call unchanged.
In `@internal/flow/parse.go`:
- Around line 28-47: ParseMeta currently evaluates extracted meta as JavaScript
via vm.RunString, which can hang indefinitely on untrusted input; add a timeout
guard around the evaluation in ParseMeta using goja Runtime.Interrupt with
time.AfterFunc, or replace the JS execution path entirely. Make sure the timeout
is wired around the vm.RunString call before ExportTo, and that any timeout or
interrupt is returned as an error from ParseMeta.
In `@internal/flow/run.go`:
- Around line 107-118: The agent() opts decode in runAgent currently logs
vm.ExportTo failures and continues with a zero-value AgentSpec, which can mask
malformed model/schema/agentType inputs. Update the opts handling in runAgent so
an ExportTo error rejects the promise or returns an error immediately instead of
proceeding, and keep the valid path unchanged by only assigning spec.Prompt
after successful decoding.
In `@internal/tools/flow_spawn.go`:
- Around line 55-60: The structured-output path in extractFlowJSON only
validates JSON syntax and silently falls back when json.Marshal(spec.Schema)
fails, so it can return malformed Structured data; update extractFlowJSON to
validate the parsed object against spec.Schema before returning, and propagate
an error from the schema-marshaling step instead of disabling structured mode
when json.Marshal(spec.Schema) fails. Keep the fix centered around
extractFlowJSON and the spec.Schema handling so the opts.schema contract always
returns a validated object.
- Around line 77-83: NewFlowSpawn is creating subagents with raw tool access but
never attaching the approval middleware used in normal sessions. Update
FlowSpawnDeps to carry the approval callback/handler and thread it into
adk.NewChatModelAgent when building the flow-agent, or switch to the approved
agent constructor used elsewhere so workflow_run subagents stay within the same
tool-permission boundary.
- Around line 136-173: runFlowAgent is swallowing execution and stream errors,
which lets NewFlowSpawn treat a failed agent run as a successful partial result.
Update runFlowAgent to return an error alongside the text/token usage, and
propagate any event.Err or MessageStream.Recv failure instead of breaking
silently. Then adjust the call site in NewFlowSpawn to handle the returned error
and fail the workflow with the real cause.
In `@internal/tools/workflow_tool.go`:
- Around line 92-94: The validation in workflow_tool.go currently only rejects
empty inputs, but it still allows both name and script to be set and then
silently prefers the script. Update the input checks in the workflow execution
path (including the shared validation used around the current Name/Script
handling in the tool logic) so callers must provide exactly one of the two
fields, returning an error when both are supplied. Keep the behavior aligned
with the tool’s name/script contract by enforcing this in the same place as the
existing validation and any related parsing/setup code.
---
Nitpick comments:
In `@go.mod`:
- Around line 60-61: The go.mod dependency entries for github.com/dop251/goja
and github.com/dop251/goja_nodejs are marked indirect even though internal/flow
imports them directly. Update the require declarations in go.mod to make these
runtime dependencies direct by removing the indirect markers, and keep the
entries aligned with the actual imports used by the internal/flow package and
its eventloop usage.
In `@internal-doc/dynamic-workflow-design.md`:
- Line 4: The `agent()` execution-path description is inconsistent across the
document: one section says it rides `SubagentTaskManager`, while the mapping
table says v1 is decoupled and uses a semaphore/goroutine pair. Update the
`agent()` story so both the architecture summary and the v1 mapping table
describe the same execution contract, using the same terms for the chosen
implementation path.
- Around line 109-110: Section 5 and section 8 currently conflict on whether
`opts.schema` in the structured output flow is schema-validated in v1, so make
the contract consistent. Update the `submit_structured_output`/`opts.schema`
description and the non-goal statement to clearly say either validation is
enforced or only best-effort parsing is done, and ensure the wording around
agent retries and `error_max_structured_output_retries` matches that final
behavior.
In `@internal/flow/engine_test.go`:
- Around line 416-423: The settle-goroutine check in engine_test.go is flaky
because it relies on a fixed time.Sleep before reading sink.agentStarts and
sink.agentDones. Replace that sleep with polling in the same test block,
repeatedly locking sink.mu and checking the counters until they reach 1/1 or a
deadline expires, then fail with the same fatal message if the expected paired
events never appear. Keep the logic localized around the sink inspection so the
test remains stable on slow CI.
In `@internal/flow/loader.go`:
- Around line 45-60: The builtin loading paths are swallowing
`ReadDir`/`ReadFile` failures without any diagnostic output, so update
`Loader.loadBuiltin` and `Loader.scanDir` to log these errors instead of just
continuing or returning silently. Use the existing `config.Logger()` pattern
from `Loader.add` to report failures with enough context to identify whether the
issue happened during builtin directory scanning or file reading, while keeping
the current fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e0d287d-c849-45be-86a7-26975b36e2c5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
cmd/jcode/main.gogo.modinternal-doc/dynamic-workflow-design.mdinternal/command/acp.gointernal/command/interactive.gointernal/command/web.gointernal/command/workflow.gointernal/flow/builtin/pr-review.jsinternal/flow/builtin/repo-audit.jsinternal/flow/builtin/roundtable.jsinternal/flow/builtins_run_test.gointernal/flow/engine.gointernal/flow/engine_test.gointernal/flow/loader.gointernal/flow/loader_test.gointernal/flow/parse.gointernal/flow/parse_test.gointernal/flow/prelude.jsinternal/flow/run.gointernal/flow/types.gointernal/tools/flow_spawn.gointernal/tools/workflow_tool.gosite/docs/changelog.mdsite/docs/overview/workflows.md
Comment on lines
+99
to
+103
| // runChild runs a nested workflow (called from workflow()). It shares the sink but | ||
| // gets its own loop; depth guards against deeper nesting. | ||
| func (e *Engine) runChild(ctx context.Context, wf Workflow, args interface{}, runID string, depth int) (interface{}, error) { | ||
| return e.run(ctx, wf, args, runID, depth, 0) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Share resource accounting with nested workflow runs.
runChild() creates a fresh run, and Line 128/Line 131 initialize independent concurrency and agent counters. A parent workflow can therefore fan out many workflow() calls and multiply WithConcurrency/WithMaxAgents beyond the intended run cap. Thread shared limiter/counter state into child runs instead of resetting it per child. This directly affects the PR’s concurrency/cap guarantees.
Also applies to: 120-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/flow/engine.go` around lines 99 - 103, The nested workflow path in
runChild and run is resetting resource tracking instead of sharing it, which
lets child workflow() calls bypass the intended WithConcurrency and
WithMaxAgents caps. Thread the parent limiter/counter state through runChild
into run so child executions reuse the same accounting rather than initializing
fresh concurrency and agent counters for each nested run. Update the run
initialization logic and any related state passed from runChild to ensure nested
runs contribute to the same global limits.
Comment on lines
+55
to
+60
| schemaJSON := "" | ||
| if spec.Schema != nil { | ||
| if b, mErr := json.Marshal(spec.Schema); mErr == nil { | ||
| schemaJSON = string(b) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate structured output against the provided JSON Schema.
extractFlowJSON only checks that the response is syntactically valid JSON, but the contract says opts.schema returns a validated object. This lets malformed-but-valid JSON pass as Structured; also return an error when json.Marshal(spec.Schema) fails instead of silently disabling structured mode.
Suggested direction
- if spec.Schema != nil {
- if b, mErr := json.Marshal(spec.Schema); mErr == nil {
- schemaJSON = string(b)
- }
+ if spec.Schema != nil {
+ b, mErr := json.Marshal(spec.Schema)
+ if mErr != nil {
+ return flow.AgentResult{}, fmt.Errorf("marshal structured output schema: %w", mErr)
+ }
+ schemaJSON = string(b)
}
...
- if v, ok := extractFlowJSON(text); ok {
+ if v, ok := extractFlowJSON(text); ok && validateAgainstFlowSchema(v, schemaJSON) == nil {
structured = v
break
}Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tools/flow_spawn.go` around lines 55 - 60, The structured-output
path in extractFlowJSON only validates JSON syntax and silently falls back when
json.Marshal(spec.Schema) fails, so it can return malformed Structured data;
update extractFlowJSON to validate the parsed object against spec.Schema before
returning, and propagate an error from the schema-marshaling step instead of
disabling structured mode when json.Marshal(spec.Schema) fails. Keep the fix
centered around extractFlowJSON and the spec.Schema handling so the opts.schema
contract always returns a validated object.
Comment on lines
+77
to
+83
| ag, aerr := adk.NewChatModelAgent(ctx, &adk.ChatModelAgentConfig{ | ||
| Name: "flow-agent", | ||
| Description: spec.Label, | ||
| Instruction: instruction, | ||
| Model: cm, | ||
| ToolsConfig: adk.ToolsConfig{ToolsNodeConfig: compose.ToolsNodeConfig{Tools: agentTools}}, | ||
| MaxIterations: flowAgentMaxIter, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File list\n'
git ls-files 'internal/tools/*' 'internal/**/flow*' 'internal/**/agent*' | sed -n '1,200p'
printf '\n## Outline: internal/tools/flow_spawn.go\n'
ast-grep outline internal/tools/flow_spawn.go --view expanded || true
printf '\n## Relevant search hits\n'
rg -n --hidden --no-ignore-vcs -S \
'FlowSpawnDeps|NewChatModelAgent|ToolsConfig|approval|permission|permission callback|approve|Execute|execute|edit|write|general|coordinator|explore' \
internal -g '!**/*_test.go' | sed -n '1,240p'
printf '\n## flow_spawn.go excerpt\n'
nl -ba internal/tools/flow_spawn.go | sed -n '1,220p'
printf '\n## Candidate approved agent path files\n'
rg -n --hidden --no-ignore-vcs -S \
'approved agent|approval handler|permission handler|New.*Agent|ChatModelAgentConfig|ToolsNodeConfig' \
internal | sed -n '1,240p'Repository: cnjack/jcode
Length of output: 24887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## internal/tools/flow_spawn.go\n'
sed -n '1,260p' internal/tools/flow_spawn.go
printf '\n## internal/team/manager.go (agent construction area)\n'
sed -n '540,640p' internal/team/manager.go
printf '\n## internal/runner/approval.go\n'
sed -n '1,260p' internal/runner/approval.go
printf '\n## internal/flow/types.go\n'
sed -n '1,180p' internal/flow/types.go
printf '\n## FlowSpawnDeps / NewFlowSpawn references\n'
rg -n --hidden --no-ignore-vcs -S 'FlowSpawnDeps|NewFlowSpawn|flow.SpawnFunc|flowTools\(' internal | sed -n '1,200p'Repository: cnjack/jcode
Length of output: 26016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## internal/command/workflow.go (spawn wiring)\n'
sed -n '160,260p' internal/command/workflow.go
printf '\n## internal/tools/workflow_tool.go\n'
sed -n '1,240p' internal/tools/workflow_tool.go
printf '\n## internal/tools/execute.go\n'
sed -n '1,260p' internal/tools/execute.go
printf '\n## internal/tools/write.go\n'
sed -n '1,260p' internal/tools/write.go
printf '\n## internal/tools/edit.go\n'
sed -n '1,260p' internal/tools/edit.go
printf '\n## approval-related tool/handler references in internal/tools\n'
rg -n --hidden --no-ignore-vcs -S 'Approval|approval|Handler|middleware|Hook|permission|pre-approve|preapprove|ask_user' internal/tools internal/runner internal/hooks internal/handler | sed -n '1,260p'Repository: cnjack/jcode
Length of output: 49103
Thread approval middleware into workflow subagents. NewFlowSpawn builds adk.ChatModelAgent instances with raw tool access (execute, plus edit/write for general/coordinator) but never threads in the approval handler/middleware used by normal sessions. That lets workflow_run spawn subagents outside the usual tool-permission boundary. Pass the approval callback through FlowSpawnDeps and into adk.NewChatModelAgent, or reuse the approved agent constructor here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tools/flow_spawn.go` around lines 77 - 83, NewFlowSpawn is creating
subagents with raw tool access but never attaching the approval middleware used
in normal sessions. Update FlowSpawnDeps to carry the approval callback/handler
and thread it into adk.NewChatModelAgent when building the flow-agent, or switch
to the approved agent constructor used elsewhere so workflow_run subagents stay
within the same tool-permission boundary.
Real bugs: - roundtable.js: rebuttal misalignment — critiques.filter(Boolean)[i] reindexed a filtered array against the original panel index; use critiques[i] (parallel preserves order). - loader.go: use path.Join (not filepath.Join) for embed.FS reads so builtins load on Windows; log previously-swallowed ReadDir/ReadFile errors. - flow_spawn.go: runFlowAgent now returns an error and NewFlowSpawn fails the workflow on a real agent-run/stream error instead of returning partial text as success. - pr-review.js: validate args.base against a safe git-ref charset before interpolating it into the command the review agent runs (injection). - workflow_tool.go: reject when both name and script are supplied. - run.go: reject the agent() promise on a malformed opts object instead of silently running with defaults. - parse.go: guard ParseMeta with an Interrupt timeout so a pathological meta literal (e.g. a looping getter) can't hang the parse. Nitpicks: - go.mod: mark goja / goja_nodejs as direct deps (imported by internal/flow). - engine_test.go: poll instead of a fixed sleep (slow-CI stability). - design doc: reconcile the agent() execution-path and structured-output sections (semaphore+goroutine, not SubagentTaskManager; structured output is best-effort JSON, schema conformance not validated in v1). Deferred (documented known limitations): nested-workflow cap sharing, schema-conformance validation, and approval middleware (matches the existing subagent path, which also builds agents without it). Generated with Jack AI bot
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/command/workflow.go (2)
175-179: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSilently ignored
os.Getwd()error.
pwd, _ := os.Getwd()swallows the error; if the current directory is unreadable/removed,pwdbecomes""and is passed straight intobuildFlowSpawn, which resolves provider/model/tools env from it. This can silently produce a wrongly-rooted spawn context instead of a clear error.🛠️ Proposed fix
- pwd, _ := os.Getwd() + pwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("get working directory: %w", err) + } spawn, resolver, err := buildFlowSpawn(ctx, pwd)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/command/workflow.go` around lines 175 - 179, The workflow spawn setup in buildFlowSpawn is ignoring the error from os.Getwd(), which can pass an empty or invalid working directory into the spawn context. Update the os.Getwd() call in workflow.go to handle its error explicitly and return it before calling buildFlowSpawn, so the provider/model/tools resolution never proceeds with a bad cwd.
128-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrefer saved-workflow lookup for bare names
isFlowFiletreats any existing non-directory path as a file, so a same-named file in the current directory will shadow a saved workflow and run/validate the wrong source. Try the loader first when the argument doesn’t end in.js, or require an explicit path for file-based workflows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/command/workflow.go` around lines 128 - 165, The workflow resolution in newFlowRunCmd currently lets isFlowFile shadow saved workflows whenever a same-named local file exists. Update RunE so bare names are resolved through newFlowLoader().Get first unless the target explicitly looks like a file path (for example, ends with .js), and only fall back to reading a local file for explicit file inputs. Keep the existing validation and flow.Validate/flow.ParseMeta path handling, but ensure saved workflows win for plain names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/command/workflow.go`:
- Around line 175-179: The workflow spawn setup in buildFlowSpawn is ignoring
the error from os.Getwd(), which can pass an empty or invalid working directory
into the spawn context. Update the os.Getwd() call in workflow.go to handle its
error explicitly and return it before calling buildFlowSpawn, so the
provider/model/tools resolution never proceeds with a bad cwd.
- Around line 128-165: The workflow resolution in newFlowRunCmd currently lets
isFlowFile shadow saved workflows whenever a same-named local file exists.
Update RunE so bare names are resolved through newFlowLoader().Get first unless
the target explicitly looks like a file path (for example, ends with .js), and
only fall back to reading a local file for explicit file inputs. Keep the
existing validation and flow.Validate/flow.ParseMeta path handling, but ensure
saved workflows win for plain names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93c39173-7d21-4958-9f10-0c3affd09f4f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.modinternal-doc/dynamic-workflow-design.mdinternal/command/workflow.gointernal/flow/builtin/pr-review.jsinternal/flow/builtin/roundtable.jsinternal/flow/engine_test.gointernal/flow/loader.gointernal/flow/parse.gointernal/flow/parse_test.gointernal/flow/run.gointernal/tools/flow_spawn.gointernal/tools/workflow_tool.gosite/docs/changelog.mdsite/docs/overview/workflows.mdsite/docs/overview/writing-workflows.md
✅ Files skipped from review due to trivial changes (2)
- site/docs/changelog.md
- site/docs/overview/workflows.md
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/flow/builtin/roundtable.js
- go.mod
- internal/flow/builtin/pr-review.js
- internal/tools/flow_spawn.go
- internal/tools/workflow_tool.go
- internal/flow/loader.go
- internal/flow/parse_test.go
- internal/flow/run.go
- internal/flow/engine_test.go
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.
What
A new dynamic workflow engine: user- or agent-authored JavaScript orchestration scripts that fan work out across many subagents, run them in parallel/pipeline, and collect structured results — the plan lives in code (à la Claude Code Dynamic Workflows / Qoder CLI Workflows).
A saved workflow is a
.jsfile:export const meta = {...}+ a plain-JS body with top-levelawait, using injected primitives.Why / design
The engine is a thin goja (pure-Go JavaScript) control-flow shell over a hand-written Go execution core. goja has no built-in event loop and its Promise resolve/reject aren't goroutine-safe, so it's driven by a
goja_nodejsevent loop:agent()returns a pending promise immediately, the blocking subagent runs on a worker goroutine, and the promise is settled back on the loop goroutine viaRunOnLoop. That gives realawait Promise.all([...])parallelism while the JS itself stays deterministic (required for journaled resume).internal/flowis a pure package — the dependency runs one way (tools → flow → config); the adk agent adapter lives ininternal/tools/flow_spawn.go.What's in it
internal/flow/— engine + async host bridge + loader +prelude.js+ 3 builtins (repo-audit,pr-review,roundtable).agent/parallel(barrier) /pipeline(streaming, no barrier) /phase/log/workflow(nested, one level) /args/budget.opts.schema; determinism guards (Date.now/Math.random/arglessnew Date()throw); concurrency (16) + hard per-run (1000) agent caps; wall-clock watchdog with run-scoped cancel.workflow_runtool registered across TUI / Web / ACP so the agent can write-and-run a workflow inline; intermediate agent work stays out of the conversation.jcode flow list | show | runCLI (--args/--timeout/--concurrency).site/docs/overview/workflows.md+ changelog; internal design doc.Loads from
~/.jcode/workflows/and<project>/.jcode/workflows/(project wins), mirroring the skills loader.Testing & hardening
-raceclean — real-parallelism barrier, caps, pipeline streaming, structured output, determinism throws, cancellation, timeout-unblock, nested + depth guard, meta brace-matching, all 3 builtins end-to-end (fake spawn, in-process per the sandbox limits).finish+ interrupt), not just interrupts an idle loop;vmReadyhandshake guarded byctx;stripExportsno longer over-matchesexportinside strings;OnAgentDonealways pairs withOnAgentStart— no phantom "running" agent in a progress UI.Follow-ups (documented, not in this PR)
Rich live-progress UI (TUI
/workflowspanel, WebWorkflowsView+ WSflow_progress) — the sink isNopSinktoday, soflow runstreams CLI progress and returns results, but the frontends don't yet render live phase/agent trees. Also: schema-conformance validation for structured output, journaled resume,isolation:'worktree'.Generated with Jack AI bot
Summary by CodeRabbit
workflow/workflows/flowcommand suite withlist,show,validate, andrunto discover, inspect, validate, and execute dynamic workflows.repo-audit,pr-review, androundtable.