From 8509c8c6ca8d850cf1c22bc9f12cf483f8228a78 Mon Sep 17 00:00:00 2001 From: CodeGhost21 <164498022+CodeGhost21@users.noreply.github.com> Date: Tue, 7 Jul 2026 03:39:43 +0530 Subject: [PATCH] fix(flows): keep seeded build turn propose-only (#4596) (#4607) --- .../flows/WorkflowCopilotPanel.test.tsx | 10 ++- .../components/flows/WorkflowCopilotPanel.tsx | 9 +-- .../__tests__/ChatRuntimeProvider.test.tsx | 9 ++- .../flows/agents/workflow_builder/agent.toml | 2 +- .../agents/workflow_builder/builder_prompt.rs | 72 +++++++++++++------ .../flows/agents/workflow_builder/prompt.md | 33 +++++---- src/openhuman/flows/schemas.rs | 13 ++-- src/openhuman/tools/ops.rs | 8 +-- 8 files changed, 97 insertions(+), 59 deletions(-) diff --git a/app/src/components/flows/WorkflowCopilotPanel.test.tsx b/app/src/components/flows/WorkflowCopilotPanel.test.tsx index 6318e6fc4..31d38f207 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.test.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.test.tsx @@ -229,13 +229,11 @@ describe('WorkflowCopilotPanel', () => { ); expect(hookState.send).toHaveBeenCalledTimes(1); const arg = hookState.send.mock.calls[0][0]; - // The user's description reads as their own first turn in the transcript, - // while the real prompt injects the blank graph + flow id and asks for the - // full build → dry-run → save arc onto the already-created flow. + // The user's description reads as their own first turn in the transcript; + // the structured build request carries the blank graph + flow id so the + // server's brief asks for a build → dry-run → propose arc (propose-only — + // see #4596; persistence still waits on Accept + Save). expect(arg.displayText).toBe('digest my Slack every morning'); - // The user's description reads as their own first turn; the structured - // build request carries the blank graph + flow id so the server's brief - // asks for the full build → dry-run → save arc onto the created flow. expect(arg.request.mode).toBe('build'); expect(arg.request.instruction).toBe('digest my Slack every morning'); expect(arg.request.graph).toEqual(baseGraph); diff --git a/app/src/components/flows/WorkflowCopilotPanel.tsx b/app/src/components/flows/WorkflowCopilotPanel.tsx index 38611b874..5209dcb7e 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.tsx @@ -153,10 +153,11 @@ export default function WorkflowCopilotPanel({ // Auto-send the build turn once when opened from the prompt bar's // instant-create path: the user's description becomes the first user turn on - // this thread, and the prompt asks for the full build → dry-run → save arc - // against the just-created flow (its proposal still lands as the usual - // Accept/Reject diff preview). Falls back to a propose-only revise turn if - // the flow id is somehow missing (a draft canvas has nothing to save onto). + // this thread, and the prompt asks for the full build → dry-run → PROPOSE + // arc against the just-created flow. Persistence still stays behind the + // usual Accept + canvas Save; `mode: 'build'` intentionally does NOT save + // the graph (issue #4596 — a Reject used to leave the graph persisted). + // Falls back to a plain revise turn if the flow id is somehow missing. const buildSentRef = useRef(false); useEffect(() => { if (!buildSeed || buildSentRef.current) return; diff --git a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx index 2570fcfa2..319494d98 100644 --- a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx +++ b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx @@ -1019,14 +1019,13 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledWith( 't-interim', - expect.objectContaining({ - content: 'Let me check your calendar first.', - sender: 'agent', - }) + expect.objectContaining({ content: 'Let me check your calendar first.', sender: 'agent' }) ) ); // …and cleared from the live preview so it isn't shown twice. - expect(store.getState().chatRuntime.streamingAssistantByThread['t-interim']?.content).toBe(''); + expect(store.getState().chatRuntime.streamingAssistantByThread['t-interim']?.content).toBe( + '' + ); }); it('dedupes a re-delivered interim event by round', async () => { diff --git a/src/openhuman/flows/agents/workflow_builder/agent.toml b/src/openhuman/flows/agents/workflow_builder/agent.toml index 93f46a1c9..2f0e31672 100644 --- a/src/openhuman/flows/agents/workflow_builder/agent.toml +++ b/src/openhuman/flows/agents/workflow_builder/agent.toml @@ -1,7 +1,7 @@ id = "workflow_builder" display_name = "Workflow Builder" delegate_name = "build_workflow" -when_to_use = "Workflow authoring specialist — owns building tinyflows automation graphs from a natural-language description. Route any request to 'set up a workflow that…', 'automate…', 'when X happens do Y', 'build/create/edit an automation or flow', or to iterate on a proposed workflow here. It grounds nodes in real tool slugs + connections, dry-runs a draft in a sandbox to self-check, and returns a workflow PROPOSAL for review. When the host hands it an existing flow id it may also SAVE the built graph onto that flow (save_workflow) and, with the user's explicit confirmation, test-run it — but it can never create a new flow or enable/disable one. Not for running an already-saved flow (that is a direct flows_run) and not for the legacy skill 'workflows'." +when_to_use = "Workflow authoring specialist — owns building tinyflows automation graphs from a natural-language description. Route any request to 'set up a workflow that…', 'automate…', 'when X happens do Y', 'build/create/edit an automation or flow', or to iterate on a proposed workflow here. It grounds nodes in real tool slugs + connections, dry-runs a draft in a sandbox to self-check, and returns a workflow PROPOSAL for review. Persistence stays with the user's Accept + Save; when the user explicitly asks the agent to save (or test-run) onto an existing flow id it may `save_workflow` / `run_flow` — but never on its own, and it can never create a new flow or enable/disable one. Not for running an already-saved flow (that is a direct flows_run) and not for the legacy skill 'workflows'." temperature = 0.2 max_iterations = 12 iteration_policy = "extended" diff --git a/src/openhuman/flows/agents/workflow_builder/builder_prompt.rs b/src/openhuman/flows/agents/workflow_builder/builder_prompt.rs index d7d0443d7..da728c3ed 100644 --- a/src/openhuman/flows/agents/workflow_builder/builder_prompt.rs +++ b/src/openhuman/flows/agents/workflow_builder/builder_prompt.rs @@ -7,12 +7,13 @@ //! directly (like the Flow Scout), instead of the frontend crafting delegate //! strings and relying on the chat orchestrator to route them. //! -//! Persistence contract, unchanged: `create`/`revise`/`repair` ask for a -//! PROPOSAL only — saving stays behind the user's explicit action. -//! [`BuildMode::Build`] is the instant-create path (the host has already made -//! the blank flow), so its brief tells the agent to finish the job: build, -//! dry-run, and `save_workflow` onto that flow id. Enabling/disabling a flow is -//! never in scope here. +//! Persistence contract: every mode is PROPOSE-ONLY — saving always stays +//! behind the user's explicit action (the review card's Accept, then the +//! canvas's own Save). [`BuildMode::Build`] is the instant-create path (the +//! host already made the blank flow), so its brief injects that flow id as +//! future-turn context but explicitly forbids `save_workflow` on this turn: +//! rejecting the proposal must leave the flow's persisted graph untouched +//! (see issue #4596). Enabling/disabling a flow is never in scope here. use serde::Deserialize; use serde_json::Value; @@ -29,7 +30,8 @@ pub enum BuildMode { /// Diagnose a failed run and propose a corrected graph. Repair, /// Instant-create: the flow already exists (blank), so build → dry-run → - /// `save_workflow` onto `flow_id`, end to end. + /// propose against `flow_id`. Persistence still waits on the user's + /// explicit Accept + Save; the agent must NOT `save_workflow` here. Build, } @@ -66,11 +68,12 @@ pub struct BuilderRequest { impl BuilderRequest { /// Validates a builder-turn request before prompt rendering. /// - /// [`BuildMode::Build`] acts on an existing saved flow — its brief tells the - /// agent to `save_workflow` onto `flow_id`. A missing or blank `flow_id` - /// would otherwise render `The flow's id is ``.` into the brief and let the - /// agent save onto nothing, so reject it here (the RPC path deserializes - /// `BuilderRequest` directly, where only `mode` is required). + /// [`BuildMode::Build`] injects a `flow_id` as context for future turns + /// (the user may later ask the agent to save/test that flow). A missing or + /// blank `flow_id` would render `The flow's id is ``.` into the brief and + /// contradict the "instant-create flow already exists" framing, so reject + /// it here (the RPC path deserializes `BuilderRequest` directly, where + /// only `mode` is required). pub fn validate(&self) -> Result<(), String> { if self.mode == BuildMode::Build && self @@ -96,10 +99,11 @@ const DIRECTIVE_REVISE: &str = "Revise this tinyflows automation and return the disable anything. You may run_flow the SAVED flow to test it, but ONLY if I ask and only after you \ confirm with me first."; -const DIRECTIVE_BUILD_AND_SAVE: &str = "Build this tinyflows automation END-TO-END. The flow already exists \ - (created blank just now) — design the graph, verify it with dry_run_workflow, return the workflow \ - proposal, then SAVE it onto the flow id below with save_workflow. Do not enable or disable anything, and \ - do not run_flow a real test unless I explicitly confirm first. Tell me what you saved when you are done."; +const DIRECTIVE_BUILD_PROPOSE_ONLY: &str = "Build this tinyflows automation END-TO-END and return the workflow \ + proposal. The flow already exists (created blank just now) — design the graph and verify it with \ + dry_run_workflow, then return the proposal for me to review. Do NOT save_workflow in this turn: I will \ + Accept the proposal explicitly and the canvas's Save persists it. Do not enable, disable, or run_flow \ + anything unless I explicitly confirm first."; /// Serialize a graph compactly for injection as agent context. fn serialize_graph(graph: &Value) -> String { @@ -142,9 +146,12 @@ pub fn render_prompt(req: &BuilderRequest) -> String { BuildMode::Build => { let flow_id = req.flow_id.as_deref().unwrap_or(""); [ - DIRECTIVE_BUILD_AND_SAVE, + DIRECTIVE_BUILD_PROPOSE_ONLY, "", - &format!("The flow's id is `{flow_id}`. Its current (blank) graph is:"), + &format!( + "The flow's id is `{flow_id}` (kept for future turns — do not save_workflow it here). \ + Its current (blank) graph is:" + ), "```json", &req.graph .as_ref() @@ -241,12 +248,37 @@ mod tests { } #[test] - fn build_asks_to_save_onto_flow_id() { + fn build_is_propose_only_and_injects_flow_id_as_context() { + // Regression for #4596: the instant-create build turn must NOT + // instruct the agent to `save_workflow`. Rejecting the proposal has + // to leave the created-blank flow's persisted graph untouched, so + // persistence stays behind the user's explicit Accept + Save. The + // flow id is still injected as context for future turns. let mut r = req(BuildMode::Build); r.flow_id = Some("flow_9".into()); r.graph = Some(json!({ "nodes": [], "edges": [] })); let p = render_prompt(&r); - assert!(p.contains("save_workflow")); + // Positive: the new directive explicitly forbids save_workflow on + // this turn. + assert!( + p.contains("Do NOT save_workflow"), + "build directive must forbid save_workflow explicitly (#4596)" + ); + // Negative: none of the old imperative-save phrasings survive + // (any of them would put us back in the auto-save bug). + for banned in [ + "then SAVE", + "with save_workflow", + "SAVE it onto", + "save_workflow onto", + ] { + assert!( + !p.contains(banned), + "build directive must not carry auto-save phrasing `{banned}` (#4596)" + ); + } + // Context is still injected so the user can later ask the agent to + // save/test that specific flow. assert!(p.contains("flow_9")); assert!(p.contains("END-TO-END")); } diff --git a/src/openhuman/flows/agents/workflow_builder/prompt.md b/src/openhuman/flows/agents/workflow_builder/prompt.md index acab92c3e..292409eb9 100644 --- a/src/openhuman/flows/agents/workflow_builder/prompt.md +++ b/src/openhuman/flows/agents/workflow_builder/prompt.md @@ -24,24 +24,31 @@ click in the review card persists a flow (via `flows_create`, which re-validates server-side). If a user says "just turn it on for me", explain that enabling stays in their hands. -## Saving your work: `save_workflow` (finish the job — don't hand it back) +## Saving your work: `save_workflow` (only on the user's explicit ask) -When the request gives you a **flow id to build into** (the Flows page's prompt -bar creates the flow first and delegates with its id; the canvas copilot passes -the saved flow's id), the user expects you to **finish**: build, verify, and -**save** — not to tell them to go save it themselves. The arc: +Every authoring turn — including a **build** turn seeded from the Flows +prompt bar (which creates the flow first and delegates with its id) and every +**revise** turn on the canvas copilot — is **propose-only** by default. Your +arc is: 1. Ground + build the graph (below), `dry_run_workflow` until it's clean. 2. `revise_workflow` / `propose_workflow` so the user gets the reviewable - proposal card. -3. **`save_workflow { flow_id, graph, name? }`** to persist it onto that flow, - then tell the user plainly what you saved (trigger, steps, and — if the flow - is enabled with a schedule/app_event trigger — that it is now live and will - fire on its own). + proposal card. **Stop there** and hand back — the user's Accept + the + canvas's Save persist the graph, not you. -Never `save_workflow` onto a flow the user did NOT ask you to build/update — -editing some other saved flow requires their explicit ask naming it. It cannot -create flows, and it never changes `enabled` or the approval gate. +**Do NOT auto-`save_workflow` when the request carries a `flow_id`.** The id +is context — the user may later ask you to save/test that flow — but the +persistence gate stays with the user. Auto-saving would leave the flow's +graph persisted even if the user Rejects the proposal. + +Use **`save_workflow { flow_id, graph, name? }`** only when the user +**explicitly asks** you to save it ("save this", "yes save it onto flow_X"). +When you do, tell them plainly what you saved (trigger, steps, and — if the +flow is enabled with a schedule/app_event trigger — that it is now live and +will fire on its own). Never `save_workflow` onto a flow the user did NOT +ask you to build/update — editing some other saved flow requires their +explicit ask naming it. It cannot create flows, and it never changes +`enabled` or the approval gate. ## Testing a saved flow: `run_flow` (ask first!) diff --git a/src/openhuman/flows/schemas.rs b/src/openhuman/flows/schemas.rs index 9151858ec..0736d3a27 100644 --- a/src/openhuman/flows/schemas.rs +++ b/src/openhuman/flows/schemas.rs @@ -748,12 +748,13 @@ pub fn schemas(function: &str) -> ControllerSchema { description: "Run the workflow_builder agent for one authoring turn. `mode` selects \ create (first draft from `instruction`), revise (refine the injected \ `graph`), repair (diagnose a failed `run_id` and fix), or build \ - (instant-create: build + dry-run + save_workflow onto `flow_id`). The \ - server renders the agent's brief — the frontend no longer crafts prompts. \ - Returns `{ proposal, assistant_text, error }`, where `proposal` is the \ - `{ type: 'workflow_proposal', name, graph, require_approval, summary, \ - warnings }` the agent produced (or null). Only `build` may persist (via \ - save_workflow onto an existing flow); it never enables or runs a flow.", + (instant-create: build + dry-run + propose against `flow_id`; \ + propose-only, see #4596). The server renders the agent's brief — the \ + frontend no longer crafts prompts. Returns `{ proposal, assistant_text, \ + error }`, where `proposal` is the `{ type: 'workflow_proposal', name, \ + graph, require_approval, summary, warnings }` the agent produced (or \ + null). No mode auto-persists a graph; save/enable/run stay behind the \ + user's explicit action.", inputs: vec![ FieldSchema { name: "mode", diff --git a/src/openhuman/tools/ops.rs b/src/openhuman/tools/ops.rs index c573eb83e..a771c0218 100644 --- a/src/openhuman/tools/ops.rs +++ b/src/openhuman/tools/ops.rs @@ -300,10 +300,10 @@ pub fn all_tools_with_runtime( // workflow-builder prompt requires it to ask the user for confirmation // first, and the flow's own approval gate still pauses outbound nodes. Box::new(RunFlowTool::new(config.clone())), - // Persist a built graph onto an EXISTING saved flow (Write). The one - // deliberate carve-out from the belt's propose-only origin: the Flows - // prompt bar creates the flow first and hands the agent its id, so the - // copilot can finish the "build → dry-run → save" arc itself. It can + // Persist a built graph onto an EXISTING saved flow (Write). Used only + // when the USER explicitly asks the agent to save; the seeded build + // turn from the Flows prompt bar is propose-only (see #4596) — Accept + // + the canvas's own Save persist the graph. The tool itself can // never create a flow or change enabled/require_approval. Box::new(SaveWorkflowTool::new(config.clone())), // Flow Scout discovery: the `flow_discovery` agent's terminal emit