fix(flows): correct save_workflow + workflow_builder contract text (disarm/create accuracy)#4904
Conversation
…ate accuracy) + surface disarm logs - save_workflow's tool description and inline comment claimed it "never changes the flow's enabled state" — stale since tinyhumansai#4889 made flows_update auto-disable a flow on a manual->automatic trigger transition. Update both to state the disarm behavior accurately. - save_workflow dropped flows_update's outcome.logs, so the model never saw the "auto-disabled because trigger changed manual->automatic" explanation. Thread outcome.logs into the returned warnings (minus the boilerplate "flow updated: <id>" line). - workflow_builder's agent.toml (when_to_use + [tools] comments) and prompt.md claimed the agent "can never create a new flow" / had "no tool that does" — false: create_workflow/duplicate_flow are on the belt and safe because created flows are always born disabled. Corrected the descriptive text; tool belt and born-disabled behavior are unchanged. Tests: extended save_workflow test coverage with a disarm-on-save regression test asserting flows_update's log surfaces in the response warnings; extended builder_prompt.rs's standing-prompt contract test with negative assertions against the stale "never create a flow" phrasing and positive assertions for the accurate create_workflow/born-disabled contract.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe workflow builder prompt now requires explicit creation requests and disabled initial flows. ChangesWorkflow builder safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SaveWorkflowTool
participant flows_update
participant FlowStorage
SaveWorkflowTool->>flows_update: Save graph with automatic trigger
flows_update->>FlowStorage: Persist disabled enabled-state
flows_update-->>SaveWorkflowTool: Return disarm logs
SaveWorkflowTool-->>SaveWorkflowTool: Append logs to warnings
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6053088359
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| You **can** create a new flow (`create_workflow`) or clone one | ||
| (`duplicate_flow`), but only when the user explicitly asks — and every flow | ||
| you create is always born **DISABLED**. Enabling a flow is not a tool you |
There was a problem hiding this comment.
When the user explicitly asks the builder to create/save a brand-new flow and there is no existing flow_id, this new permission conflicts with the still-present save-only guidance in the same prompt: it calls save_workflow the “ONE persistence tool” and says if there is “no flow yet” to hand back the proposal instead of creating. Since prompt.md is the agent’s system prompt and create_workflow is actually on the belt, the model can still refuse or skip the new create path for exactly the scenario this commit is trying to enable; make the save section route explicit new-flow requests to create_workflow/duplicate_flow and reserve save_workflow for existing flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 47da50f — split the "user says save it" guidance in prompt.md into existing-flow (save_workflow) vs brand-new-flow (create_workflow/duplicate_flow) cases, so an explicit "create and save this as a new flow" ask now routes to create_workflow instead of falling through to propose-only.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/flows/builder_tools_tests.rs (1)
1776-1822: 📐 Maintainability & Code Quality | 🔵 TrivialTest doesn't verify the boilerplate line is actually excluded.
The test asserts an "auto-disabled" warning is present, but never asserts the redundant
"flow updated: {flow_id}"boilerplate is absent fromwarnings— the specific exclusion behavior this PR claims to add. See the related comment onsrc/openhuman/flows/builder_tools.rs#L3391-3402for the consolidated fix.🤖 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 `@src/openhuman/flows/builder_tools_tests.rs` around lines 1776 - 1822, The test save_workflow_surfaces_auto_disarm_warning_on_manual_to_automatic_transition must also verify that warnings excludes the redundant “flow updated: {flow_id}” boilerplate. Keep the existing auto-disabled warning assertion and add an assertion over parsed["warnings"] confirming no warning contains the flow-updated message for flow_id.
🤖 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.
Nitpick comments:
In `@src/openhuman/flows/builder_tools_tests.rs`:
- Around line 1776-1822: The test
save_workflow_surfaces_auto_disarm_warning_on_manual_to_automatic_transition
must also verify that warnings excludes the redundant “flow updated: {flow_id}”
boilerplate. Keep the existing auto-disabled warning assertion and add an
assertion over parsed["warnings"] confirming no warning contains the
flow-updated message for flow_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1445408-7c5a-4ce9-a03e-ab0bcea1f320
📒 Files selected for processing (5)
src/openhuman/flows/agents/workflow_builder/agent.tomlsrc/openhuman/flows/agents/workflow_builder/builder_prompt.rssrc/openhuman/flows/agents/workflow_builder/prompt.mdsrc/openhuman/flows/builder_tools.rssrc/openhuman/flows/builder_tools_tests.rs
…ngs (addresses @coderabbitai on builder_tools_tests.rs:1776-1822) CodeRabbit nitpick: the auto-disarm regression test asserted the "auto-disabled" warning was present but never asserted the redundant "flow updated: <id>" boilerplate line was excluded — the specific filtering behavior SaveWorkflowTool::execute claims to add.
…in prompt.md (addresses @chatgpt-codex-connector on prompt.md:13) The invariants section already said the agent CAN create_workflow/ duplicate_flow on explicit ask, but the "Saving your work" section below it only described save_workflow and told the agent to fall back to propose-only when there's no flow_id yet — even when the user explicitly asked to create-and-save a brand-new flow. Split the "user says save it" guidance into existing-flow (save_workflow) vs brand-new-flow (create_workflow/duplicate_flow) cases so the model doesn't skip the create path for exactly the scenario this PR enables.
|
Addressed CodeRabbit's nitpick (review 4704530374) on |
1 participant
Summary
Two small contract-accuracy fixes in
workflow_builder(tinyflows agent) — no behavior/tool-belt changes, only descriptive text + one bug where an explanatory log was silently dropped.Fix 1 —
save_workflowdescription/comments stale after #4889 (+ dropped disarm logs)#4889madeflows_updateauto-disable a flow whenever its trigger transitions from manual to an automatic kind (schedule/webhook/app_event) on an already-enabled flow — but never on its own for an existing automatic→automatic re-save.SaveWorkflowTool::description()(src/openhuman/flows/builder_tools.rs) still said save_workflow "never changes the flow's enabled state" — now corrected to state the auto-disarm behavior.flows_update"preserves whatever enabled state the flow already had" (unconditionally) — corrected to describe the disarm-on-transition case first, then the preserved-state case.save_workflowextractedoutcome.valuefromflows_update'sRpcOutcomebut droppedoutcome.logs— so when a save disarmed a flow, the model never sawflows_update's explanation ("Flow was auto-disabled because its trigger changed from manual to automatic..."). Now threaded into the tool'swarningsarray (skipping the boilerplate"flow updated: <id>"line, which just duplicates fields already in the response).Fix 2 —
workflow_builderclaimed it "can never create a flow"agent.toml'swhen_to_useand[tools]comments, plusprompt.md's invariants section, claimed the agent "can never create a new flow" / "has no tool that does" — false:create_workflowandduplicate_floware on the belt (namedlist inagent.toml). They're safe because every flow they create is force-disabled at creation (CreateWorkflowTool,duplicate_flow's born-disabled clone). The contradiction was visible withinprompt.mditself (the invariants section at the top said "cannot create a flow", while a later section accurately describedcreate_workflow/duplicate_flow).Updated the descriptive text in both files to state the accurate contract: the agent can create a flow on explicit ask, but every created flow is born DISABLED, and it can never enable one. No change to the tool belt or to the born-disabled persistence behavior.
Files changed
src/openhuman/flows/builder_tools.rs—SaveWorkflowTool::description(), inline comment, threadoutcome.logsintowarnings.src/openhuman/flows/builder_tools_tests.rs— new regression testsave_workflow_surfaces_auto_disarm_warning_on_manual_to_automatic_transition.src/openhuman/flows/agents/workflow_builder/agent.toml—when_to_use,[tools]/sandbox comments.src/openhuman/flows/agents/workflow_builder/prompt.md— invariants section +save_workflowguidance paragraph.src/openhuman/flows/agents/workflow_builder/builder_prompt.rs— extendedstanding_prompt_teaches_plain_language_and_readonly_memorywith negative assertions against the stale "never create a flow" phrasing and positive assertions for the accuratecreate_workflow/born-disabled contract.Test plan
GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml— clean.GGML_NATIVE=OFF cargo test --lib openhuman::flows::— 384 passed, 0 failed.save_workflow_surfaces_auto_disarm_warning_on_manual_to_automatic_transitionpasses — seeds an enabled manual-trigger flow, saves a schedule-trigger graph onto it, asserts the flow ends up disabled and the disarm log appears inwarnings.standing_prompt_teaches_plain_language_and_readonly_memorypasses — asserts the stale "can never create a flow" phrasing is gone and the accuratecreate_workflow/born-disabled contract is taught.cargo fmtclean (no diff).Summary by CodeRabbit