revert(flows): restore require_approval default to true (merge LAST, after approval surfacing)#4672
Conversation
…ansai#4651 + tinyhumansai#4668 band-aids) tinyhumansai#4651 + tinyhumansai#4668 defaulted require_approval to false as a workaround for the parked-run deadlock (a flow's approval couldn't surface with no thread/client). Now that the approval-surfacing work makes flow approvals answerable (run-details + chat + notifications, approve once/always), restore the SAFE default of true. - flows/tools.rs (ProposeWorkflowTool): unwrap_or(false) -> unwrap_or(true) + schema default. - flows/builder_tools.rs (ReviseWorkflowTool): same. - WorkflowPromptBar.tsx: createFlow(..., false) -> true. - ChatRuntimeProvider.tsx: require_approval === true -> !== false (default true). - tests reverted to assert the true default; explicit true/false round-trip tests kept. Byte-for-byte revert of the two band-aid diffs (verified against 25ce5b7 + 8018eeb). cargo check/fmt/flows:: green; typecheck + WorkflowPromptBar 5/5 + ChatRuntimeProvider 59/59. MUST MERGE LAST — after the approval-surfacing PRs, or it reintroduces the deadlock.
📝 WalkthroughWalkthroughThis PR flips the default value of Changesrequire_approval default flip
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/flows/tools.rs (1)
141-245: 🧹 Nitpick | 🔵 TrivialDefault flip is consistent, but confirm merge sequencing.
The
unwrap_or(true)change and the resultingrequire_approvalechoed in the response (Line 241) are internally consistent with the schema description and withrevise_workflow's matching change inbuilder_tools.rs. Per the PR stack context, this revert is intended to be merged last, after the approval-surfacing work is in place, to avoid reintroducing the parked-run deadlock for flows awaiting approval. Please confirm that dependency has actually landed before merging this cohort.🤖 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/tools.rs` around lines 141 - 245, The `execute` flow in `propose_workflow` now flips `require_approval` to default true and returns it in the proposal payload, so make sure the approval-surfacing changes it depends on are already merged before this lands. Verify the matching logic in `builder_tools.rs` and the surrounding workflow approval path is present so the default flip does not reintroduce the parked-run deadlock for pending flows.
🤖 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/tools.rs`:
- Around line 141-245: The `execute` flow in `propose_workflow` now flips
`require_approval` to default true and returns it in the proposal payload, so
make sure the approval-surfacing changes it depends on are already merged before
this lands. Verify the matching logic in `builder_tools.rs` and the surrounding
workflow approval path is present so the default flip does not reintroduce the
parked-run deadlock for pending flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eabea221-f0b1-459c-9c05-1ef2ce3d4bc6
📒 Files selected for processing (7)
app/src/components/flows/WorkflowPromptBar.test.tsxapp/src/components/flows/WorkflowPromptBar.tsxapp/src/providers/ChatRuntimeProvider.tsxsrc/openhuman/flows/builder_tools.rssrc/openhuman/flows/builder_tools_tests.rssrc/openhuman/flows/tools.rssrc/openhuman/flows/tools_tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c26c39f74d
ℹ️ 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".
| name, | ||
| createBlankWorkflowGraph(name, t('flows.nodeKind.trigger')), | ||
| false | ||
| true |
There was a problem hiding this comment.
Avoid parking prompt-authored flows on an unsurfaced gate
With this default, every prompt-created flow saved by the builder carries require_approval=true, so a Run-button or scheduled run that reaches an outbound HTTP/tool/code node enters ApprovalGate's workflow branch. That branch explicitly publishes a generic ApprovalRequested with no chat routing, while the current UI still does not route approval attention actions (TinyPlaceOrchestrationTab only handles open-session; FlowApprovalCard calls flows_resume, not approval_decide). In those non-chat runs the tool call therefore waits until the gate TTL and fails instead of executing, which regresses the default prompt-authoring path for any real outbound automation.
Useful? React with 👍 / 👎.
Restores
require_approval's safe default oftrueby reverting the two band-aid PRs (#4651 + #4668), which had defaulted it tofalsepurely to dodge the parked-run deadlock.Merge this only after the approval-surfacing PRs (run-details + chat + notifications, approve once/always) are in. Merging it before them reintroduces the deadlock (flows would default to require-approval again, but the approval still couldn't surface).
Changes (exact reverts of #4651 + #4668)
flows/tools.rs(ProposeWorkflowTool) —.unwrap_or(false)→.unwrap_or(true)+ schema defaultflows/builder_tools.rs(ReviseWorkflowTool) — sameWorkflowPromptBar.tsx—createFlow(..., false)→trueChatRuntimeProvider.tsx—require_approval === true→!== falsetruedefault (explicit true/false round-trip tests kept)Verified byte-for-byte against the pre-band-aid originals (
25ce5b723,8018eeb6e).Verification
cargo check/fmtclean ·flows::green ·typecheckclean · WorkflowPromptBar 5/5 · ChatRuntimeProvider 59/59.Summary by CodeRabbit