Repository navigation
feat(workflows): add a canvas and a script for multi-step runs - #280
XuQianJin-Stars wants to merge 10 commits into
Conversation
github-actions
Bot
added
enhancement
 [High impact] Workflow runs reach the host and its connected client apps. The PR is not ready to merge because earlier execution, save, and approval defects remain. Findings
SummaryType: feature · Areas: host, iOS, Android, desktop, TUI, web, docs · Priority: P3: low Adds shared workflows with a desktop canvas, editable scripts, host execution, and phone and terminal controls.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| void (async () => { | ||
| try { | ||
| const client = await store.ready() | ||
| const { workflow } = await fetchWorkflow(client, selected, false) | ||
| if (cancel) return | ||
| adopt(workflow) | ||
| const page = await workflowRuns(client, selected, 20) |
There was a problem hiding this comment.
WorkflowsView owns host calls, save queues, revision changes, and run state inside the React component. The repository requires views to render state and send intents, with networking in clients and state logic in stores/models. Move this workflow controller into the store; the same layering issue appears in the new phone editor views. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
df1d4ec to
c00b828
Compare
| if edge.branch.is_some() && edge.branch != source.branch { | ||
| continue; | ||
| } | ||
| parts.push((edge_pos(graph, &edge.id), source.output.clone())); | ||
| } | ||
| parts.sort_by(|a, b| a.0.cmp(&b.0)); | ||
| if !ins.is_empty() && parts.is_empty() { | ||
| actions.push(Action::Skip { node_id: (*id).to_owned() }); |
There was a problem hiding this comment.
A compiled conditional step gets both an ordinary edge from its input variable and a conditional edge. When the condition is false, next_actions drops the conditional edge but still starts the step using the ordinary input. An output inside if (a === "ship") can therefore post to chat even when a is not "ship". Treat the conditional edge as a required gate before accepting other inputs.
How this was verified: The compiler adds both edges, and the walker starts the output whenever any input survives, even when its conditional edge does not match.
| lines.push(format!("if ({test}) {{")); | ||
| emit_list(yes, graph, params, vars, lines, seq, last); | ||
| lines.push("} else {".into()); | ||
| emit_list(no, graph, params, vars, lines, seq, last); | ||
| lines.push("}".into()); |
There was a problem hiding this comment.
emit_condition shares vars and last across both branches. generate then uses those names for later steps and the final return, outside the branch blocks. A graph with an assistant in a branch can generate const step2 inside an if and return step2 outside it. If a bot edits that script into a dynamic workflow, it fails because step2 is undefined. Generate an outer result binding and assign the selected branch's value to it.
| @@ -0,0 +1,140 @@ | |||
| //! Workflow create, save, conflict and a run that does not need an agent. | |||
There was a problem hiding this comment.
Validation reports unit tests and type checks, but no end-to-end walkthrough on the final host and affected clients. It also says Android was not built or checked in an emulator and desktop was not walked after replay. The repository requires real-host and client flow checks before merging external PRs. Walk the workflow flows on desktop, iPhone, Android and terminal, then record the results and any blocked checks in Validation. This requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1e52f56 to
cff8b6b
Compare
| called.push(func.call(&JsValue::undefined(), &[], context)?); | ||
| let () = context.run_jobs(); |
There was a problem hiding this comment.
parallel assigns call numbers while each branch runs, then runs its waiting code before invoking the next branch. Consider a first branch that awaits A and then calls C, alongside a second branch that calls B. The first pass records A as call-1 and B as call-2. After A finishes, replay reaches C first and gives it B's cached call-2 result. Required work can be skipped, and later steps receive the wrong text. Give calls stable identities across replay.
| let steps = load_steps(&hub.store, &run.id)?; | ||
| let status = if stopping | ||
| || (steps.iter().any(|s| s.status == StepStatus::Stopped) | ||
| && steps.iter().all(|s| s.status != StepStatus::Failed)) | ||
| { | ||
| RunStatus::Stopped | ||
| } else if let Some(failed) = steps.iter().find(|s| s.status == StepStatus::Failed) { | ||
| let _ = failed; | ||
| RunStatus::Failed | ||
| } else { | ||
| RunStatus::Done | ||
| }; | ||
| let error = steps.iter().find(|s| s.status == StepStatus::Failed).and_then(|s| s.error.clone()); |
There was a problem hiding this comment.
settle_run checks every recorded attempt when choosing the final status. A successful graph-step retry leaves its original failed row intact, so the run becomes failed again and keeps the old error even after the retry and later steps succeed. Choose the final status and error from each node's latest attempt.
| if !task.is_empty() && all.len() == 1 { | ||
| return format!("{} + {}", js_str(&format!("{task}\n\n")), all[0]); | ||
| } | ||
| let mut expr = String::new(); | ||
| for (index, part) in all.iter().enumerate() { |
There was a problem hiding this comment.
text_expr drops the assistant's task when it has multiple text inputs, including a trigger plus a prompt. The generated agent() call contains only the joined inputs; the task survives only as a label. Running an edited dynamic script or restoring that generated script therefore loses the instructions. Include the task before the joined inputs.
| let Some(previous) = tx.query_row( | ||
| "SELECT output, branch FROM workflow_steps WHERE workflow_id = ?1 AND node_id = ?2 AND status = 'done' ORDER BY ended_at DESC, id DESC LIMIT 1", | ||
| params![workflow, node], | ||
| |r| Ok((r.get::<_, Option<String>>(0)?, r.get::<_, Option<String>>(1)?)), | ||
| ).optional()? else { continue }; |
There was a problem hiding this comment.
Starting from a middle node without saved upstream results reports success without running that node. reuse silently skips missing results, but next_actions then treats the missing upstream node as due and blocks the requested node. With no actions or open steps, the run finishes as done. Reject the start with a clear missing-input error or include the required upstream work.
| if detail.running != nil { | ||
| let open = runs.first { $0.isOpen } | ||
| if let open { try await client.stopWorkflowRun(open.id) } |
There was a problem hiding this comment.
After a scheduled or remotely started run, the iPhone detail page changes its button to Stop but does not refresh runs on run events. The first tap finds no open run in that stale list and sends no stop request; it only reloads afterward. Users must tap again while the workflow keeps working. Refresh the list on workflowRunReload, or fetch the current open run before stopping.
| const { run: detail } = await fetchWorkflowRun(await store.ready(), id) | ||
| followRef.current = false | ||
| setVersionGraph(null) | ||
| setPreviewId(null) | ||
| previewRef.current = null | ||
| setViewed(detail) | ||
| sync(selected, dirtyRef.current) |
There was a problem hiding this comment.
Open a run from A, then switch to B while fetchWorkflowRun is pending. A's response sets viewed and turns off live following on B, showing A's run under B's title. Version previews and history-list requests lack the same check. Bind each request to the current workflow and ignore its response after a switch.
| if expanded.isEmpty { | ||
| expanded = Set(loaded.steps.compactMap { step in | ||
| guard let output = step.output, !output.isEmpty else { return nil } | ||
| return step.id | ||
| }) |
There was a problem hiding this comment.
After the user hides every output, the next live refresh sees an empty expanded set and opens them all again. Android does the same with openOutput. This reopens long replies and moves the page while a workflow runs. Track first load separately so refresh preserves the user's choice.
| Row(horizontalArrangement = Arrangement.spacedBy(10.dp), verticalAlignment = Alignment.Top) { | ||
| for (id in rank) { | ||
| val node = graph.nodes.firstOrNull { it.id == id } ?: continue | ||
| StepCard(node, names, numbers[id], graph.parentsOf(id).mapNotNull { numbers[it] }, Modifier.weight(1f).onGloballyPositioned { coords -> |
There was a problem hiding this comment.
Line mode places every step at the same depth in one phone-width row. With several parallel assistants, each card has too little room for its number, icon, padding and instructions. Both phones use this layout. Give cards a readable minimum width and allow horizontal scrolling or wrapping.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if !parents.is_empty() && y < inner.bottom().saturating_sub(2) { | ||
| let sources = source_line(&parents); | ||
| put( | ||
| buf, | ||
| x + 16, | ||
| y, | ||
| inner.width.saturating_sub(pad.saturating_add(16)), | ||
| &truncate(&sources, usize::from(inner.width.saturating_sub(pad.saturating_add(16)))), | ||
| t.dim.patch(t.panel), | ||
| ); | ||
| y += 1; |
There was a problem hiding this comment.
The new source labels add another screen line per step, but scrolling still assumes each step takes one line. On a long chain, moving down leaves the selected step below the drawing cutoff. Later steps remain unreadable even at the end of the list. Choose the visible window from the actual lines each step uses.
| contextScope.launch { | ||
| try { refreshWorkflows() } | ||
| catch (cancelled: CancellationException) { throw cancelled } | ||
| catch (_: Exception) { } |
There was a problem hiding this comment.
receiveWorkflows discards refresh failures without logging or showing them. The repository requires errors to be handled, passed on, or logged; an empty catch needs an explanation of why ignoring it is safe. The new recent-run fetch has the same pattern. Report these failures so stale rows have an explanation. This requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
| store.bots().ok()?.into_iter().find(|b| !b.deleted && b.config.lead).map(|b| b.config.id) | ||
| } | ||
|
|
||
| fn settle_run(hub: &Hub, run: &RunRow, steps: &[StepRow], effects: &mut Effects) -> Result<()> { |
There was a problem hiding this comment.
run.rs adds 1,478 lines covering run starts, execution, output delivery, database reads and retries. The repository requires new behavior past roughly 500 lines to move into sibling modules. This keeps unrelated changes easier to review and maintain. The same requirement applies to script_parse.rs and the newly enlarged Workflow.swift. Split these responsibilities before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
leepokai
left a comment
leepokai
left a comment
There was a problem hiding this comment.
Thanks @XuQianJin-Stars, this is a big piece of work, and the outline/canvas pairing is a nice idea. I did a security pass over the whole diff before anything else (nothing from the branch was run here). There's nothing malicious in it: the new crates all come from crates.io, scripts can't touch files, processes or the network, the SQL is parameterized, and the device permissions match routines. A few things do need changing before this can merge:
Security
- A script can hang the whole host (high).
SCRIPT_FUELcounts bytecode only, not native builtins, soargs.input.match("(a+)+$")(catastrophic regex backtracking) ors += sabout 30 times runs unbounded. The script runs underblock_in_placeon a tokio worker while holding the globalWorkflows.gate(script_run.rs:160/:189,service.rs:123/:235/:298), so stop, webhooks and the bot actors then block too, andrecover()reruns the same run at startup. Please run scripts off the async runtime (its own thread or process) with a wall-clock and memory limit, and never while holding the gate; boa'sRuntimeLimits(loop iterations, recursion, stack) instead of thefuzzfeature's instruction counter. - Every bot gets an unscoped workflows MCP (high).
session.rs:110attaches it unconditionally, andtools.rs:25/:34let any bot save and run any workflow, so a bot that read a prompt injection can writeagent("<any bot>", "...")and run it, or rewrite the user's scheduled or webhook workflows (new ones start enabled). The target bot gets the raw prompt (agent.rs:69) and runs under its own permission mode, Approve automatically and computer use included. Routines are scoped to their own bot (a bot never runs another bot's routine); please scope workflows the same way, ask the user before a bot saves or runs one, and mark cross-bot prompts with their source likemessage_botdoes. - Webhook payloads read as instructions (medium).
hooks.rs:51hands the raw body toargs.input, and it reaches the agent atagent.rs:69/:106-108unframed. Routines wrap it as untrusted trigger data (routines/runner.rs:70); please do the same. - Webhook checks in the wrong order (low).
hooks.rs:23-30takes the gate and answersPausedbefore checking the secret, so hook ids can be probed and unauthenticated requests hold the gate and a write transaction. Check the signature first, without the gate (asroutines/mod.rsdoes). Also: GitHub'spingstarts a run (routines ignore it), and without a delivery id the body hash dedupes forever, so the same body can never run twice. - Deep nesting (low, unverified). A deeply nested script near 64 KB is parsed on a 2 MiB worker stack (
script_parse.rs:122), andto_jsonon deep objects inreturned()/object_json()recurses too; please add a depth test. And the staticForbidwalk skipsfor/try/switchbodies (walk_stmt_loose), so it shouldn't be treated as the sandbox.
CI: the iOS Kit job fails: Swift 6.3.3 crashes in IR generation for CodyncUI/Workflows/WorkflowStepSheet.swift (a thunk for WorkflowNode with an isolated closure). It needs a workaround in that file so the Kit builds.
Validation: our rule for contributor PRs is end to end on the real apps against a real host built from the branch, on every client it touches: desktop (npm run dev), iPhone (iOS Simulator), Android (emulator) and the TUI, plus the after-change report if you use Claude Code. The Validation says the desktop wasn't walked again after the replay and Android didn't build.
Given the size (25k lines, 187 files), it may also be easier to land in parts, for example the host engine and API with the TUI first, then the desktop canvas, then the phones.
🤖 Drafted with Claude Code
b7130bb to
2fff06e
Compare
| // An order line only waits. An output has no text port, and can still precede the next step. | ||
| let ordered = edge.id.ends_with(":order"); | ||
| if (!ordered && outs.is_empty()) || ins.is_empty() { |
There was a problem hiding this comment.
The new :order edges let consecutive outputs save, but the outline readers ignore those edges. When an assistant feeds two consecutive outputs, both are placed directly after the assistant. Adding or moving a step on either phone rebuilds the graph without the output-to-output edge.
The next run can send the second output before the first finishes, or even if the first fails. Preserve ordering edges when rebuilding, or keep these graphs out of outline mode.
| Box(Modifier.fillMaxSize().tabLayer(tab == AppTab.Workflows)) { | ||
| val shown = states.firstOrNull { it.workflow != null } | ||
| ?: states.firstOrNull { it.computer.id == app.currentComputerId } | ||
| ?: states.firstOrNull() | ||
| val store = shown?.let { accounts.store(it.computer.id) } | ||
| if (shown != null && store != null) WorkflowsView(shown, store, states, barSpace) { accounts.focusWorkflows(it) } |
There was a problem hiding this comment.
Open a workflow detail, then open a bot through a notification or widget. Opening the bot leaves workflow unchanged, so the hidden Workflows tab keeps its two-page NavStack and active back handler. That handler is registered after the chat's handler and receives Back first.
The user stays in the chat while the hidden workflow goes back, requiring extra gestures to leave the chat. Disable the workflow's back handler while its tab is inactive.
ec784c2 to
4167844
Compare
|
The iOS Kit run was failing in |
|
Fixed the three review notes in acae368.
|
acae368 to
b3e6966
Compare
|
Fixed the schedule race in b3e6966 and rebased onto the latest Choosing Schedule still writes the daily default, but an edit made while that request is in flight is kept. The late default no longer puts 09:00 back over the time the user picked. The language conflicts with |
|
A later approval notification for the same step now opens that step. Android was remembering the node, so the second tap stayed on the detail page and the open sheet kept the first decision. Each notification is tracked by its step id, and the sheet resets when that id changes. The iPhone already rebuilds the workflows screen for each notification, so it shows the new step without this change. |
144c303 to
451599a
Compare
|
Rebased onto the latest
|
|
Rebased again onto the latest
|
451599a to
fb873ae
Compare
A workflow is a graph on the desktop and a script the host runs. Phones and the terminal use the same list, outline, and run. Scripts run in their own process, and a failed save, version, or schedule request stays visible.
The recovery test failed the signaling channel after the answer was installed. On a busy runner gathering could finish first, the session detached, and the test waited until its one-minute limit. The upload now fails while the route is still down, so recovery waits without spending an attempt.
The reconnect test waited on an animation notification and on a checkpoint opened after the new link could already be up. On a busy runner that wait ran out the one-minute limit. It now reads the connection itself and listens before the tap.
A script retry keeps the failed row's request id and inserts the new attempt under its own, so replay no longer hits the unique constraint. Every step that reads the trigger gets that payload. Choosing Schedule writes a default time on Android and the desktop, matching the iPhone.
The default request used to replace that choice with daily 09:00.
A run opened from history stayed on the frame from when it was opened, so Stop did nothing after the host had finished. The canvas follows an open run, Stop reloads it, and the wait clock starts when the agent does.
An ordering edge only waits, so regenerating a script no longer adds the earlier reply to the next prompt. A due tick claims that schedule under the same lock as a save, and does not start after the trigger becomes manual. A standalone approval can be approved or rejected, and its notification opens that step.
The approval sheet sat inside the editor body, which the compiler could not type-check.
A later approval for the same step was ignored after that node had been opened, and the sheet kept the previous decision. Each notification is tracked by its step, and the sheet shows that step.
A main-actor task group queued its wait behind the test, so a checkpoint that had not arrived yet never woke it and CI killed the test at one minute. The race now runs off the main actor.
fb873ae to
94a9dc7
Compare
Summary
Validation
tscpassed while the canvas was being built. Two host workflow tests (a_manual_run) passed on the earlier base.dev(2c94985a, the Android multi-computer merge). Android workflow screens were fitted onto that per-computer store. The Android SDK is not installed here, so the emulator and Gradle checks were not run. The desktop app was not walked again after that replay.What's New: iPhone
zh-Hant
en-US
What's New: Android
zh-Hant
en-US
MAC（PC）：
IPhone（Mobile）：