Repository navigation
fix(flows): gate flows_create so saving a scheduled/side-effecting flow can't silently arm it - #4835
Conversation
…ow can't silently arm it Issue B29 (save/enable safety): saving a flow used to arm it live and unattended by default. `store::create_flow` hardcoded `enabled: true`, and `require_approval` defaulted to `false` on most creation paths — so a scheduled flow that posts to Slack could go from draft to armed-and-acting the moment the user clicked Save, with no confirmation. `flows_create` now enforces two server-side rules, authoritative regardless of what the caller passed, so no creation path (prompt bar, scratch/template modal, proposal "save & enable", copilot save_workflow) can bypass them: - Rule 1: a graph whose trigger fires without a human in the loop (schedule/app_event/webhook) persists disabled. The user arms it explicitly via the existing flows_set_enabled toggle. A manual trigger (or no trigger_kind discriminator) still persists enabled — it only ever runs via explicit flows_run, so there's no surprise. - Rule 2: a graph containing any outbound side-effect node (tool_call/http_request/code) forces require_approval: true, overriding whatever the caller passed. Read-only graphs are unaffected. store::create_flow now takes an explicit `enabled` parameter instead of hardcoding true. SaveWorkflowTool (the copilot's save path, which updates an existing flow rather than creating one, so Rule 1 doesn't apply there) now emits a loud warning when it saves onto a flow that is already enabled with an automatic trigger, so the agent relays the live-arming risk to the user. Three pre-existing tests asserted the old (unsafe) behavior directly — that flows_create bound a schedule flow's cron job immediately because it defaulted to enabled. Updated them to enable explicitly via flows_set_enabled first, matching the new contract. Added 9 ops-level tests covering both rules (including the exact ticket scenario: a scheduled Slack-posting flow saved with require_approval:false must come back both disabled and require_approval:true) plus 7 inline tests for the two new helpers.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughFlow creation now disables automatically triggered flows, forces approval for outbound side effects, and binds dispatch triggers only when enabled. Workflow saving reports automatic-trigger warnings, with tests covering gating, binding, approval, and updated storage calls. ChangesFlow creation and persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant flows_create
participant FlowGraph
participant FlowStore
participant DispatchTrigger
Client->>flows_create: create flow with graph and options
flows_create->>FlowGraph: classify trigger and node side effects
FlowGraph-->>flows_create: enabled and approval decisions
flows_create->>FlowStore: persist flow settings
flows_create->>DispatchTrigger: bind trigger when flow is enabled
DispatchTrigger-->>Client: creation result and logs
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![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: c983829ce0
ℹ️ 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".
|
|
||
| // Rule 1: automatic triggers create DISABLED — the user must arm them | ||
| // explicitly. | ||
| let enabled = !trigger_is_automatic(&graph); |
There was a problem hiding this comment.
When this branch is taken for scheduled/app_event/webhook graphs, flows_create now returns a disabled flow and only explains the change through RpcOutcome.logs. Existing UI callers discard those logs in createFlow (app/src/services/api/flowsApi.ts:270), and the proposal card's “Save & enable” path clears the card immediately after createFlow (app/src/components/chat/WorkflowProposalCard.tsx:84-85), so a user saving an automatic proposal gets no enable prompt and the flow never fires despite the CTA. Please make the create path surface this state or update the caller to explicitly notify/enable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da760be (fix(flows): make \"Save & enable\" actually enable a B29-disabled auto-trigger flow).
Good catch — kept B29 Rule 1's server-side guarantee intact (flows_create still always persists an automatic-trigger graph disabled, so a copilot save_workflow autosave can never silently arm one) but fixed the client so the explicit-arming path actually arms:
WorkflowProposalCard.save now checks the returned Flow.enabled after createFlow, and when Rule 1 left it disabled, follows up with an explicit setFlowEnabled(id, true) call — the same flows_set_enabled toggle the Workflows list page uses. This is legitimate because the user just clicked "Save & enable" themselves; it is NOT the silent copilot auto-arm Rule 1 exists to prevent (that path — the agent's save_workflow tool — has no such follow-up and stays disabled, as intended).
If the follow-up enable call itself fails, the flow stays saved (disabled) and the card keeps the flow id around so a retry only retries the enable step rather than re-creating (which would have duplicated the flow). Surfaced via a new chat.flowProposal.enableError string (added to all locales) shown inline on the card.
Added vitest coverage for both the disabled-return-then-enable path and the enable-fails-then-retry path (app/src/components/chat/WorkflowProposalCard.test.tsx), and confirmed the Rust B29 Rule 1 tests are untouched and still green (cargo test --lib openhuman::flows::, 287 passed).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…trigger flow B29 Rule 1 makes flows_create always persist an automatic-trigger graph (schedule/app_event/webhook) disabled, regardless of caller intent, so a copilot save_workflow autosave can't silently arm an unattended automation. But WorkflowProposalCard's "Save & enable" button is the user's own explicit arming click, not a silent autosave — and it discarded flows_create's logs and cleared the card immediately, so the CTA's own label lied: the flow was saved but never armed. WorkflowProposalCard.save now checks the returned Flow.enabled and, when Rule 1 left it disabled, follows up with an explicit setFlowEnabled(id, true) call — the same flows_set_enabled toggle used everywhere else. If that follow-up call itself fails, the flow stays saved (disabled) and the card keeps the id around so a retry only retries the enable step, never re-creates (which would duplicate the flow). Addresses @chatgpt-codex-connector on src/openhuman/flows/ops.rs:1547 (PR tinyhumansai#4835).
…ow can't silently arm it (tinyhumansai#4835)\n
Summary
flows_createused to hand back a live, unattended automation the moment a user hit Save:store::create_flowhardcodedenabled: true, andrequire_approvaldefaulted tofalseon most creation paths.schedule/app_event/webhook) now persists disabled on create. The user arms it explicitly via the existingflows_set_enabledtoggle. Amanualtrigger (or notrigger_kindat all) still persists enabled — it only ever runs via explicitflows_run, so there's no surprise.tool_call/http_request/code) now forcesrequire_approval: trueon create, regardless of what the caller passed. Read-only graphs are unaffected.SaveWorkflowTool(the copilot's save path, which updates an existing flow rather than creating one — Rule 1 doesn't cover it) now emits a loud warning when it saves a graph onto a flow that is already enabled with an automatic trigger, so the agent relays the live-arming risk to the user.store::create_flownow takes an explicitenabled: boolparameter instead of hardcodingtrue; all call-sites updated.Problem
A scheduled flow that posts to Slack (or any other outbound action) could go from draft to armed and acting the instant the user clicked Save, with no confirmation step and no forced approval gate — because nothing in
flows_createinspected the graph's trigger kind or its outbound nodes before decidingenabled/require_approval. Every creation path (prompt bar, scratch/template modal, "save & enable" proposal card, copilotsave_workflow) was exposed identically.Solution
Two server-side rules are enforced inside
ops::flows_createitself — authoritative regardless of which frontend path called it, so no caller can accidentally bypass them:trigger_is_automatic(&graph)— new helper,schedule/app_event/webhooktrigger kinds count as automatic (deliberately broader than the existingtrigger_kind_fires, sincewebhookisn't wired to auto-dispatch yet but will be, and must not be handed out pre-armed).graph_has_outbound_side_effect(&graph)— new helper, true when the graph contains atool_call/http_request/codenode.flows_createcomputesenabled = !trigger_is_automatic(&graph)and forcesrequire_approvaltrue whengraph_has_outbound_side_effectis true, logging both decisions loudly in the RPC outcome'slogs(and viatracing::info!) so a caller that got overridden can see why.store::create_flowno longer decidesenableditself — it takes it as a parameter, decided byflows_create's two rules.flows_set_enabled,flows_update,flows_duplicate, and the copilot'spropose_workflow/revise_workflowtools are all unchanged — this only tightens the initial-creation default.Existing-test impact
Three pre-existing tests directly asserted the old, unsafe behavior — that
flows_createbound a schedule flow's cron job immediately because creation defaulted toenabled: true:flows_create_binds_schedule_cron_job_for_an_enabled_flowflows_delete_unbinds_schedule_cron_jobflows_update_rebinds_schedule_cron_job_when_trigger_schedule_changesflows_update_does_not_rebind_when_graph_is_not_suppliedUpdated all four to call
flows_set_enabled(..., true)explicitly afterflows_create(the real caller-facing arming path) before exercising cron-binding behavior — same coverage, now against the corrected contract. No other existing test depended on schedule/app_event/webhook-trigger flows or outbound-node graphs coming back enabled/require_approval-unforced fromflows_create; verified by auditing everyflows_createandstore::create_flowcall-site in the crate.Submission Checklist
ops_tests.rscases (Rule 1, Rule 2, and the exact ticket scenario combining both) + 7 inline helper tests + 4 existing schedule/cron tests updated to the new contract.cargo test --lib openhuman::flows:: -- --test-threads=1passes 287/287.pnpm test:coveragenot applicable (no frontend lines changed).docs/TEST-COVERAGE-MATRIX.mdhas no Workflows/flows section yet (no existing feature-ID rows to update); this is a bug fix to existing internal behavior, not a new/removed/renamed feature leaf.## Related— no matrix rows exist for this area (see above).enabled/require_approval), no release-cut UI surface changed.Impact
src/openhuman/flows/), no UI changes, no RPC contract changes (sameflows_createparams; response now carries extra informationallogsentries when a rule fires).flows_createcalls.Related
useCreateFlow.tscould passrequireApproval: trueconsistently for defense-in-depth (low priority, server now enforces it regardless), and a confirmation dialog when enabling an automatic-trigger flow for the first time.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/flows-save-enable-safetyValidation Run
pnpm --filter openhuman-app format:check— N/A (no frontend files changed);cargo fmt+pnpm formatrun (Rust formatting).pnpm typecheck— N/A (no TypeScript files changed).cargo test --manifest-path Cargo.toml --lib openhuman::flows:: -- --test-threads=1→ 287 passed, 0 failed.cargo fmt --manifest-path Cargo.tomlclean;GGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlpasses (warnings only, pre-existing/unrelated).GGML_NATIVE=OFF cargo check --manifest-path app/src-tauri/Cargo.tomlpasses (no flows-domain references in the Tauri shell; ran anyway for completeness).Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
flows_createnow creates automatic-trigger flows disabled and forcesrequire_approvalon graphs with outbound side-effect nodes, instead of defaulting both to the unsafe "live and unattended" state.Summary by CodeRabbit
New Features
Bug Fixes