Repository navigation
fix(flows): gate live-run tool on flows_build builder path (#4593) - #4606
Conversation
…ild path (tinyhumansai#4593) The flows_build RPC runs the workflow_builder turn under AgentTurnOrigin::Cli so the approval gate does not fail-closed in a headless/streamed run. That same origin makes ApprovalGate auto-allow every external_effect tool, so a stray run_flow call during an authoring turn could execute a live saved flow (real Slack/Gmail/HTTP/code) with no HITL confirmation. Strip the live-run tool from the builder's callable belt on this direct RPC path via a new Agent::hide_tools helper, which drops the names from the (already narrow) visible belt and rebuilds the ToolPolicySession so they resolve to Deny at the tool-call boundary — a hard execution guarantee, not just prompt-hiding. Authoring/read tools are untouched so the turn still runs headless without fail-closing. run_flow stays available and properly gated (WebChat approval card) when workflow_builder is invoked as the build_workflow chat delegate. Covers both run_flow (current name) and legacy run_workflow for robustness. Adds Rust regression coverage on the flows_build path plus unit tests for both hide_tools branches.
📝 WalkthroughWalkthroughAdds a new ChangesTool hiding feature
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant flows_build
participant workflow_builder as Agent(workflow_builder)
participant restrict_builder_toolset
flows_build->>workflow_builder: construct agent
flows_build->>restrict_builder_toolset: restrict_builder_toolset(agent)
restrict_builder_toolset->>workflow_builder: hide_tools([run_workflow, run_flow])
workflow_builder->>workflow_builder: seed visible_tool_names if empty
workflow_builder->>workflow_builder: remove hidden names, rebuild tool policy session
flows_build->>workflow_builder: run agent (live-run tools denied)
Possibly related issues
Suggested labels: Suggested reviewers: 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: b7648c7576
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .collect(); | ||
| } | ||
| for name in names { | ||
| self.visible_tool_names.remove(*name); |
There was a problem hiding this comment.
If names covers the whole current belt — for example a custom workflow_builder override or any single-tool caller where only run_flow is visible — this removal leaves visible_tool_names empty. Elsewhere an empty set is the “no filter / all tools visible” sentinel (run_chat_turn_graph maps it to None, and ToolPolicyEngine only hides when the set is non-empty), so the rebuild immediately re-exposes the full registry instead of denying the hidden tools. That turns a safety restriction into a fail-open path; keep an explicit empty allowlist/sentinel instead of collapsing to the empty-set sentinel.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/agent/harness/session/tests.rs (1)
1259-1311: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTests only assert on
visible_tool_names_for_test, not the actual policy-enforcement outcome.The doc comment for
hide_toolspromises hidden names resolve toDenyat the tool-call boundary (via the rebuilt [ToolPolicySession]), not merely absent from the prompt — a hard execution guarantee even if the model requests the tool anyway. Both new tests here stop at inspecting the intermediatevisible_tool_namesset and never assert thattool_policy_session(or an equivalent decision API) actually denies a call to the hidden tool. Consider adding one assertion per test that exercises the policy session directly, so the "hard execution guarantee" claim is what's actually pinned by the test, not just the input to it.🤖 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/agent/harness/session/tests.rs` around lines 1259 - 1311, The new hide_tools tests only verify the visible tool allowlist and do not prove the rebuilt ToolPolicySession enforces the promised deny-at-call boundary. Update hide_tools_drops_named_from_existing_filter and hide_tools_seeds_allowlist_when_no_filter_present to also exercise the actual policy decision path on the agent’s ToolPolicySession (or equivalent decision API) and assert the hidden tool is denied. Keep the existing visible_tool_names_for_test checks, but add one policy-level assertion in each test so the hard execution guarantee is covered, not just the intermediate filter state.
🤖 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.
Inline comments:
In `@src/openhuman/agent/harness/session/runtime.rs`:
- Around line 256-268: `hide_tools` in `SessionRuntime` can turn an empty
`visible_tool_names` back into the “all visible” sentinel, which silently undoes
hiding when the last visible tool is removed. Update `hide_tools` so it
preserves the intended “none visible” state after removals, instead of relying
on an empty `HashSet` meaning both “unset” and “fully hidden”; add an explicit
guard or separate state handling before calling `rebuild_tool_policy_session()`.
Refer to `hide_tools`, `visible_tool_names`, and `rebuild_tool_policy_session`
when implementing the fix.
---
Nitpick comments:
In `@src/openhuman/agent/harness/session/tests.rs`:
- Around line 1259-1311: The new hide_tools tests only verify the visible tool
allowlist and do not prove the rebuilt ToolPolicySession enforces the promised
deny-at-call boundary. Update hide_tools_drops_named_from_existing_filter and
hide_tools_seeds_allowlist_when_no_filter_present to also exercise the actual
policy decision path on the agent’s ToolPolicySession (or equivalent decision
API) and assert the hidden tool is denied. Keep the existing
visible_tool_names_for_test checks, but add one policy-level assertion in each
test so the hard execution guarantee is covered, not just the intermediate
filter state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1745f83-acc6-4e9a-b9cc-08efd5f9bbad
📒 Files selected for processing (4)
src/openhuman/agent/harness/session/runtime.rssrc/openhuman/agent/harness/session/tests.rssrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rs
| pub fn hide_tools(&mut self, names: &[&str]) { | ||
| if self.visible_tool_names.is_empty() { | ||
| self.visible_tool_names = self | ||
| .tool_specs | ||
| .iter() | ||
| .map(|spec| spec.name.clone()) | ||
| .collect(); | ||
| } | ||
| for name in names { | ||
| self.visible_tool_names.remove(*name); | ||
| } | ||
| self.rebuild_tool_policy_session(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
hide_tools can silently re-expose everything if a caller ever hides the full belt.
Because empty HashSet is overloaded as the "all visible" sentinel, if names ever covers every tool currently in visible_tool_names (e.g. seeded set had only a handful of entries and all get removed), the resulting empty set flips meaning back to "all visible" instead of "nothing visible" — silently un-hiding tools instead of hiding them. This is explicitly called out as a caveat in the doc comment but nothing guards against it. Today's only caller (flows_build's restrict_builder_toolset) hides 2 of a larger belt so it's safe in practice, but the API itself is a footgun for any future caller (or a future edit to FLOWS_BUILD_HIDDEN_TOOLS) that could silently defeat the exact protection this PR adds.
🛡️ Suggested guard
pub fn hide_tools(&mut self, names: &[&str]) {
if self.visible_tool_names.is_empty() {
self.visible_tool_names = self
.tool_specs
.iter()
.map(|spec| spec.name.clone())
.collect();
}
for name in names {
self.visible_tool_names.remove(*name);
}
+ if self.visible_tool_names.is_empty() && !self.tool_specs.is_empty() {
+ log::warn!(
+ "[agent] hide_tools cleared the entire visible-tool set for {} — \
+ falling back to the 'all visible' sentinel, which defeats the hide",
+ self.agent_definition_name
+ );
+ }
self.rebuild_tool_policy_session();
}🤖 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/agent/harness/session/runtime.rs` around lines 256 - 268,
`hide_tools` in `SessionRuntime` can turn an empty `visible_tool_names` back
into the “all visible” sentinel, which silently undoes hiding when the last
visible tool is removed. Update `hide_tools` so it preserves the intended “none
visible” state after removals, instead of relying on an empty `HashSet` meaning
both “unset” and “fully hidden”; add an explicit guard or separate state
handling before calling `rebuild_tool_policy_session()`. Refer to `hide_tools`,
`visible_tool_names`, and `rebuild_tool_policy_session` when implementing the
fix.
Summary
flows_buildran theworkflow_builderturn underAgentTurnOrigin::Cli, whoseApprovalGatebranch auto-allows every external-effect tool — so a strayrun_flowcall during an authoring turn could execute a live saved flow (real Slack/Gmail/HTTP/code) with no HITL confirmation (issue flows_build builder turn can run a live flow without approval gating #4593).Agent::hide_tools(&[&str])that drops names from the session's visible belt and rebuilds theToolPolicySessionso they resolve toDenyat the tool-call boundary (a hard execution guarantee, not just prompt-hiding).flows_buildnow strips the live-run tool from the builder belt on this direct RPC path; authoring/read tools are untouched, so the turn still runs headless without fail-closing.run_flowstays available and properly gated (realWebChatapproval card) whenworkflow_builderruns as thebuild_workflowchat delegate — only the directflows_buildpath is restricted.Problem
flows_builddeliberately usesAgentTurnOrigin::Cliso the approval gate does not fail-closed in a headless/streamed run. ButApprovalGate::intercept_audited'sCliarm returnsAllowfor everyexternal_effecttool. Theworkflow_builderbelt includesrun_flow(flows::tools::RunFlowTool,permission_level = Write,external_effect = true), whoseexecute()callsflows_run(...)— a real run. Invoked from the copilot/prompt-bar with a savedflow_id, a mistakenrun_flowfires the flow with no gate. The only remaining safety was the flow's ownrequire_approval(only pauses outbound nodes if set) plus a soft "ask first" prompt rule — neither a real guarantee. This contradictsflows_build's contract: "This op never enables or runs a flow." Flagged P1 bychatgpt-codex-connectoron #4578.Note: the
run_workflow→run_flowrename referenced in the issue thread has already landed onmain; the belt listsrun_flowtoday. The duplicate-tool-name collision (harnessrun_workflowspawn tool vs. flows runner) is therefore already resolved and is out of scope here.Solution
Agent::hide_tools(agent/harness/session/runtime.rs): removes names fromvisible_tool_namesand callsrebuild_tool_policy_session(), so hidden names becomeDenyin the enforcedToolPolicySession(consumed byToolPolicyEnforcementon the tinyagents turn path). If the belt was unfiltered (empty = "all visible"), it first seeds from every spec so the hide actually restricts.flows_build(flows/ops.rs):restrict_builder_toolset(&mut agent)hidesFLOWS_BUILD_HIDDEN_TOOLS = ["run_workflow", "run_flow"]right after the agent is built.run_flowis the live-runner today;run_workflowis listed belt-and-braces (no-ops if absent).thread_id, no per-userclient_id; the dedicated flow-review surface "B3" isn't built), so a gated origin would park-then-TTL-deny (a 10-min hang). Making the tool unreachable matches the "never runs a flow" contract and keeps the gated chat-delegate path unchanged.Submission Checklist
flows_build_hides_the_live_run_tool_from_the_builder_beltand bothhide_tools_*unit tests.N/A: behaviour-hardening change to an existing feature (no new feature row)## Related—N/A: no matrix feature IDs affectedN/A: internal approval-gating hardening, no release-cut surface changeCloses #NNNin the## RelatedsectionImpact
flows_buildRPC path; thebuild_workflowchat-delegate path (properly gated) is unchanged.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/4593-flows-build-approval-gateValidation Run
pnpm --filter openhuman-app format:check—N/A: no frontend files changedpnpm typecheck—N/A: no TypeScript changedcargo test --lib flows_build_hides_the_live_run_tool_from_the_builder_belt,hide_tools_*— all passcargo fmt --checkclean;cargo checkcleanN/A: no app/src-tauri changesValidation Blocked
command:pnpm compile/ pre-push hook (frontend steps)error:tscfails inapp/src/components/flows/canvas/*andapp/src/pages/conversations/*withCannot find module '@xyflow/react' | 'rehype-highlight' | 'remark-gfm'— these deps are declared inapp/package.jsonbut were not installed in the local worktree's stalenode_modules. Pre-existing, in files this PR does not touch.impact:None on this change. This is a Rust-only PR;cargo fmt --check,cargo check(core + Tauri viapnpm rust:check), and all focused Rust tests pass. The pre-push hook was bypassed with--no-verifyfor this unrelated pre-existing frontend/tooling breakage per repo policy; CI (which installs fresh deps) validates the frontend.Behavior Changes
flows_buildbuilder turn can no longer call the live-run flow tool.Parity Contract
run_flowstays available and gated on thebuild_workflowchat-delegate path (WebChat origin → approval card).DenyviaToolPolicySession; authoring tools (external_effect == false) unaffected, so the turn never fail-closes.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes