feat(approval): gate external-effect tool calls until user approves (#1339)#2149
Merged
senamakel merged 19 commits intoMay 20, 2026
Merged
Conversation
Default-false hook that classifies tool calls producing outbound side effects (Slack/Telegram/email/calendar/webhook writes, etc.) distinct from local writes. Used by the agent harness to route gated calls through the approval gate before execute() runs. Refs tinyhumansai#1339.
composio, gmail_unsubscribe, pushover all fan out to outbound services on the user's behalf. Returning true here routes the call through the approval gate before execute() runs. composio is unconditional in v1; per-action write metadata mapping is tracked as tinyhumansai#1339-v2 in an inline comment. Refs tinyhumansai#1339.
…umansai#1339) Coordinates with tinyhumansai#1206. reply_speech is the user's own assistant speaking through the user's own speakers — no outbound side effect. Code default already false; doc comment locks the classification so a future Tool-wrap doesn't accidentally flip it. Refs tinyhumansai#1339.
…nyhumansai#1339) ApprovalRequested fires when the gate parks a tool-call future (carries redacted args + session_id). ApprovalDecided fires from the RPC handler after the parked future resolves. Both belong to the new 'approval' domain. Refs tinyhumansai#1339.
…inyhumansai#1339) Async middleware that parks tool-call futures whose Tool returns external_effect()==true. Pending rows persist in pending_approvals so they survive core restart. Decisions land via approval_list_pending / approval_decide RPC; the gate resumes the parked oneshot and the agent continues. Session allowlist (approve_always_for_tool) short-circuits subsequent calls to the same tool. Redactor scrubs PII / chat content from action_summary and args_redacted before persistence or broadcast. 51 unit tests cover gate (allow/deny/timeout/always-allowlist/ unknown id), store (insert/list/decide/purge_session/cross-handle survival), redactor (sensitive-key match/array-count/home-path scrub), types (decision serde), and RPC schemas. Refs tinyhumansai#1339.
Wires approval_list_pending + approval_decide into the global controller registry so they're discoverable via 'openhuman approval --help' and reachable over JSON-RPC. Refs tinyhumansai#1339.
…#1339) tool_loop + subagent_runner both check tool.external_effect() and route through ApprovalGate::intercept before tool.execute(). Deny is translated into a denied tool_result the agent can reason about. When the gate is not installed (try_global() == None), behavior matches the pre-tinyhumansai#1339 path exactly. Refs tinyhumansai#1339.
) Proactive sends to Telegram/Discord/Slack/etc. route through ApprovalGate before handing off to the channel implementation. Web delivery (step 1 of the subscriber) is internal and exempt. Refs tinyhumansai#1339.
Before invoking trigger_reactor / orchestrator on a React or Escalate triage decision, ApprovalGate intercepts so the user can decline the whole escalation up-front instead of one tool call at a time. Defense in depth alongside the per-tool gate — the session allowlist short-circuits subsequent prompts after the first approval. Denied dispatches publish TriggerEscalationFailed with the gate reason and abort the trigger cleanly. Refs tinyhumansai#1339.
…tinyhumansai#1339) Opt-in at boot via env var. When unset (default) the gate is never installed, try_global() returns None everywhere, and the agent harness behaves exactly as before — safe to merge ahead of the React UI that will dispatch approval_decide. When set, the gate is keyed on OPENHUMAN_CORE_TOKEN for session binding. Refs tinyhumansai#1339. Closes tinyhumansai#1339.
Contributor
|
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 (1)
📝 WalkthroughWalkthroughAdds a persistent ApprovalGate with redaction, SQLite-backed pending approvals, RPC/controllers for list/decide, controller schemas/events, tool external-effect classification, and integration that blocks or short-circuits external-effect actions in harness, subagent, triage, and proactive delivery. ChangesApproval gate system and integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/core/jsonrpc.rs`:
- Around line 1239-1242: The fallback to a random session_id when
OPENHUMAN_CORE_TOKEN is unset causes pending approvals to be hidden across
restarts; update the code around the session_id initialization (the variable
session_id where OPENHUMAN_CORE_TOKEN is read) to log the generated UUID at
debug or trace level when the env var is absent, e.g., detect when
unwrap_or_else branch runs and emit a debug/trace via the existing logger with
the resulting session_id; mention session_id and approval_list_pending in the
log context so operators can correlate restarts to stored approvals.
- Line 1243: The call to
crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id)
currently discards its Result, hiding initialization failures; change this to
capture the Result, check for Err, and emit a clear error log (e.g., using the
module's logger or tracing::error) that includes the error details and context
(cfg/session_id) before continuing or returning; specifically locate the
init_global invocation and replace the ignored-let with proper error handling
that logs the error if init_global() returns Err so failures aren't silently
swallowed.
In `@src/openhuman/agent/harness/subagent_runner/ops.rs`:
- Around line 1429-1470: The gate denial branch currently returns the raw denial
reason (gate_denial) which gets treated as a success later via the call_success
check, so change the denial return to a consistent error-formatted string (e.g.,
prefix with "Error: ") or alternatively return a tuple/struct with an explicit
success flag; specifically update the code around the gate_denial handling (the
variable gate_denial and the if let Some(reason) = gate_denial block that
currently returns reason) to return a string like format!("Error: {reason}") (or
propagate a success boolean alongside the result) so downstream logic (which
checks result_text.starts_with("Error")) will mark denied approvals as failures.
In `@src/openhuman/approval/gate.rs`:
- Around line 123-140: The code persists the pending row with
store::insert_pending before registering the oneshot waiter (tx) in
self.waiters, allowing a race where approval_decide marks the request approved
before a waiter exists; fix by creating the oneshot channel and inserting tx
into self.waiters (using request_id and waiters.lock()) before calling
store::insert_pending, and if insert_pending subsequently fails remove the tx
from waiters and return the GateOutcome::Deny as before; apply the same change
to the other occurrence (the block around lines 208-221) so both code paths
register the waiter prior to persisting.
- Around line 18-21: The module-level doc comment for the gate says
prior-session rows are purged on init but the init_global function's docs state
the opposite; pick the actual restart behavior and make both docs consistent:
either change the module header (the top comment) to state that prior-session
rows are preserved across restarts, or update the init_global doc to state that
init purges prior-session rows, ensuring both the module doc and the init_global
function comment refer to the same behavior and include the same wording about
per-launch session_id and stale-decision purging.
In `@src/openhuman/approval/rpc.rs`:
- Around line 18-25: Add structured tracing/logging to approval_list_pending: on
entry emit a debug/trace with stable prefix (e.g.
"rpc:approval_list_pending:entry") including a generated or passed request_id
and method="approval_list_pending"; when ApprovalGate::try_global() returns None
emit a debug log ("rpc:approval_list_pending:gate_missing") with request_id and
decision="empty" before returning; when calling gate.list_pending() wrap the
call to capture errors and on error emit an error/trace log
("rpc:approval_list_pending:error") with request_id, method, and the error; on
success emit an exit debug/trace ("rpc:approval_list_pending:exit") with
request_id, method, decision="ok", and rows.len() before returning
RpcOutcome::single_log(rows, log). Use the existing symbols
ApprovalGate::try_global, gate.list_pending, and RpcOutcome::single_log to
locate spots to insert logs.
In `@src/openhuman/tools/impl/network/composio.rs`:
- Around line 557-571: The current Composio implementation always returns true
from external_effect(), which forces approval even for read-only actions like
action=="list" or action=="connect"; update the gating so only write actions
require approval — either change the harness to perform args-aware gating for
the Composio tool or adapt the tool API (e.g., add an args-aware predicate or
replace external_effect() with external_effect_with_args(action: &str) that
returns false for read-only actions such as "list"/"connect" and true for write
actions (send/post/create)); ensure the unique symbol external_effect (or its
args-aware replacement) and the action values "list"/"connect" are used to
determine read vs write behavior so read-only flows bypass the approval prompt.
In `@src/openhuman/tools/impl/network/gmail_unsubscribe.rs`:
- Around line 37-40: The legacy UI-confirmation flow is still returning the
frontend pending_approval payload from execute(), so marking external_effect()
as true allows the gate to be cleared while the real unsubscribe still runs via
the old approval path (causing duplicate prompts); fix by either (A) keep this
tool non-external for now by changing external_effect() -> false in the
GmailUnsubscribe implementation so the legacy UI continues to control the
outbound effect, or (B) move the actual unsubscribe side-effect into execute()
(so execute() performs the outbound HTTP/mailto request only after approval and
clears the gate first), and ensure the gate-clearing happens before performing
the external request; update references to external_effect() and execute()
accordingly.
🪄 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: 548e9142-5b7c-4e07-b9e8-a8d6f15048b1
📒 Files selected for processing (19)
src/core/all.rssrc/core/event_bus/events.rssrc/core/jsonrpc.rssrc/openhuman/agent/harness/subagent_runner/ops.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/agent/triage/escalation.rssrc/openhuman/approval/gate.rssrc/openhuman/approval/mod.rssrc/openhuman/approval/redact.rssrc/openhuman/approval/rpc.rssrc/openhuman/approval/schemas.rssrc/openhuman/approval/store.rssrc/openhuman/approval/types.rssrc/openhuman/channels/proactive.rssrc/openhuman/tools/impl/network/composio.rssrc/openhuman/tools/impl/network/gmail_unsubscribe.rssrc/openhuman/tools/impl/system/pushover.rssrc/openhuman/tools/traits.rssrc/openhuman/voice/reply_speech.rs
| .ok() | ||
| .filter(|s| !s.is_empty()) | ||
| .unwrap_or_else(|| format!("session-{}", uuid::Uuid::new_v4())); | ||
| let _ = crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id); |
Contributor
There was a problem hiding this comment.
Log initialization errors before swallowing them.
The init_global result is silently dropped. If initialization fails (e.g., SQLite schema migration error, permission issue, or lock contention), the operator will see the "approval gate installed" log but the gate won't actually be functional, leading to confusing behavior where external-effect tools run unsupervised.
🛡️ Proposed fix
- let _ = crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id);
+ if let Err(e) = crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id.clone()) {
+ log::warn!(
+ "[runtime] approval gate init_global failed: {e} — external-effect tools will run unsupervised"
+ );
+ return;
+ }
log::info!(
- "[runtime] approval gate installed (OPENHUMAN_APPROVAL_GATE=1) — \
+ "[runtime] approval gate installed (OPENHUMAN_APPROVAL_GATE=1, session_id={session_id}) — \
external-effect tool calls will block until approval_decide"
);🤖 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/core/jsonrpc.rs` at line 1243, The call to
crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id)
currently discards its Result, hiding initialization failures; change this to
capture the Result, check for Err, and emit a clear error log (e.g., using the
module's logger or tracing::error) that includes the error details and context
(cfg/session_id) before continuing or returning; specifically locate the
init_global invocation and replace the ignored-let with proper error handling
that logs the error if init_global() returns Err so failures aren't silently
swallowed.
Contributor
Author
There was a problem hiding this comment.
Skipped — ApprovalGate::init_global returns Arc, not Result; lazy SQLite errors surface inside with_connection on first use, not at install. Addressed the spirit in 6d33ca2 by surfacing session_id on the bootstrap log line so operators can grep across restarts.
Contributor
There was a problem hiding this comment.
@oxoxDev, you're right — I was mistaken. Looking at the file content, init_global infallibly returns Arc<ApprovalGate> (no Result), so the Err handling in my suggestion was invalid. The lazy with_connection design also makes sense: SQLite errors at first use will surface in the relevant operation's error path rather than blocking boot.
The updated code in the file already addresses the observability concern well — the (session_id, ephemeral) tuple tracks whether the session was derived from OPENHUMAN_CORE_TOKEN or generated ephemerally, the debug log on the ephemeral path explains the cross-restart correlation caveat, and the info log on success includes session_id for grep-ability. That's a cleaner approach than what I originally suggested. Apologies for the noise.
🧠 Learnings used
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When using the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, don’t require individual execution-provider feature flags (e.g., `directml`, `coreml`, `cuda`) alongside `load-dynamic` to get EP registration code. The `ort` crate already compiles EP registration via `#[cfg(any(feature = "load-dynamic", feature = "<ep_name>"))]` guards, and adding per-EP feature flags can pull in static-linking dependencies that conflict with the dynamic loading approach. At runtime, EP availability is determined by what the dynamically loaded ONNX Runtime library (`onnxruntime.dll`/`.so`/`.dylib`) supports; ort docs indicate providers like `directml`/`xnnpack`/`coreml` are available in builds when the platform supports them.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When integrating the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, do NOT also require/enable individual provider EP Cargo features like `directml`, `coreml`, or `cuda`. In `ort` v2.x, EP registration for providers (e.g., DirectML, CoreML, CUDA, etc.) is already compiled in under source-level `#[cfg(any(feature = "load-dynamic", feature = "<provider>"))]` guards, such as in `ep/directml.rs`. Adding provider feature flags alongside `load-dynamic` can pull in static-linking dependencies that conflict with the dynamic-loading approach. Provider availability should be treated as runtime-determined by what the loaded `onnxruntime` library (`onnxruntime.dll`/`libonnxruntime.so`/`libonnxruntime.dylib`) actually supports.
Learnt from: oxoxDev
Repo: tinyhumansai/openhuman PR: 571
File: src/openhuman/local_ai/service/whisper_engine.rs:69-80
Timestamp: 2026-04-14T19:59:04.826Z
Learning: When reviewing Rust code in this repo that uses the upstream `whisper-rs` crate (v0.16.0), do not report `WhisperContextParameters::use_gpu(...)` or `WhisperContextParameters::flash_attn(...)` as missing/invalid APIs. These builder-style methods exist upstream and return `&mut Self`; they are not limited to `WhisperVadContextParams`.
Learnt from: senamakel
Repo: tinyhumansai/openhuman PR: 1173
File: tests/agent_memory_loader_public.rs:88-88
Timestamp: 2026-05-04T06:50:47.877Z
Learning: In this repository, the general camelCase naming guideline should not be applied to Rust source files. For all .rs files, Rust function (and related) names should use snake_case, and snake_case Rust function names should not be flagged—even for async test functions annotated with attributes like #[tokio::test]. This is consistent with Rust’s non_snake_case lint behavior.
…ai#1339) Two CodeRabbit findings on PR tinyhumansai#2149: - Race: insert_pending ran before the oneshot waiter was registered, so a fast approval_decide could mark the row approved while no waiter existed; the parked future then timed out and returned Deny. Register the waiter first, evict on persist failure. - Module header doc claimed prior-session rows are purged on init, contradicting init_global's own doc. Restart-survival is the intended contract — header now matches init_global.
Per CodeRabbit on PR tinyhumansai#2149: subagent path returned the raw denial reason, but downstream call_success uses !result_text.starts_with("Error"), so denied approvals showed up as successful tool executions in progress events and tool_result blocks. Prefix the denial string with "Error: " so the success computation classifies it correctly.
…nsai#1339) Per CodeRabbit on PR tinyhumansai#2149 and CLAUDE.md debug-logging rules. Adds debug entry/exit logs to approval_list_pending and approval_decide with stable [rpc:approval_*] prefixes plus correlation fields (request_id, decision, tool, row count). Failures get warn/error logs at the matching site so a missing gate / unknown request_id is debuggable without re-running.
…ansai#1339) Per CodeRabbit on PR tinyhumansai#2149: forcing the entire composio tool through the approval gate prompted users for action="list" (read-only discovery) and action="connect" (returns an OAuth URL the user manually visits — no outbound side effect of its own), conflicting with the goal to keep read flows approval-free. - Tool trait gains external_effect_with_args(&self, args) which defaults to external_effect() so existing static overrides keep working with no churn. - Composio overrides the args-aware variant to skip the gate when action == "list" or "connect"; everything else (action == "execute" or missing) still gates as a write. - Harness call sites (tool_loop, subagent_runner) now use the args-aware variant so per-call gating works.
…humansai#1339) Per CodeRabbit on PR tinyhumansai#2149: gmail_unsubscribe.execute() still returns a pending_approval JSON payload that the React UI intercepts with its own legacy confirmation flow — it does not perform the unsubscribe itself. Marking the tool external_effect = true would gate the call AND then immediately fall back into the old UI prompt, prompting the user twice while the real side effect remained outside core enforcement. Keep the tool at the trait default (false) for v1 and document the follow-up (move the actual unsubscribe into execute() so the gate is the single approval surface) in an inline comment.
…i#1339) Per CodeRabbit on PR tinyhumansai#2149: when OPENHUMAN_CORE_TOKEN is unset the bootstrap fell back to a fresh UUID with no diagnostic; an operator restarting without a stable token had no way to correlate pending approvals across launches. Now logs both the ephemeral fallback (at debug) and the active session_id on the bootstrap info line. Note: approval_list_pending is session-agnostic so pending rows from prior launches stay visible — this log is purely for audit-trail correlation.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/jsonrpc.rs (1)
1254-1259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog initialization failures before declaring success.
If
init_globalfails (e.g., SQLite schema error, permission issue), the result is silently discarded but the success log at line 1256 still fires. This misleads operators into thinking the gate is active when external-effect tools may run unsupervised.🛡️ Proposed fix
- let _ = - crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id.clone()); - log::info!( + if let Err(e) = crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id.clone()) { + log::warn!( + "[runtime] approval gate init_global failed: {e} — external-effect tools will run unsupervised" + ); + } else { + log::info!( - "[runtime] approval gate installed (OPENHUMAN_APPROVAL_GATE=1, session_id={session_id}) — \ - external-effect tool calls will block until approval_decide" - ); + "[runtime] approval gate installed (OPENHUMAN_APPROVAL_GATE=1, session_id={session_id}) — \ + external-effect tool calls will block until approval_decide" + ); + }🤖 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/core/jsonrpc.rs` around lines 1254 - 1259, The call to ApprovalGate::init_global currently discards its Result but still logs success; change it to handle the Result from crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(), session_id.clone()) — if Err, log the error with details (include the error value) and avoid emitting the success log (or propagate/return the error), otherwise on Ok emit the existing log referencing session_id; ensure you update the code paths around ApprovalGate::init_global and the log::info call so initialization failures are recorded before any success message.
🤖 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.
Duplicate comments:
In `@src/core/jsonrpc.rs`:
- Around line 1254-1259: The call to ApprovalGate::init_global currently
discards its Result but still logs success; change it to handle the Result from
crate::openhuman::approval::ApprovalGate::init_global(cfg.clone(),
session_id.clone()) — if Err, log the error with details (include the error
value) and avoid emitting the success log (or propagate/return the error),
otherwise on Ok emit the existing log referencing session_id; ensure you update
the code paths around ApprovalGate::init_global and the log::info call so
initialization failures are recorded before any success message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9143f06-05d3-475f-bd22-e53ee9f324f7
📒 Files selected for processing (8)
src/core/jsonrpc.rssrc/openhuman/agent/harness/subagent_runner/ops.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/approval/gate.rssrc/openhuman/approval/rpc.rssrc/openhuman/tools/impl/network/composio.rssrc/openhuman/tools/impl/network/gmail_unsubscribe.rssrc/openhuman/tools/traits.rs
✅ Files skipped from review due to trivial changes (1)
- src/openhuman/tools/impl/network/gmail_unsubscribe.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/openhuman/approval/rpc.rs
- src/openhuman/agent/harness/tool_loop.rs
- src/openhuman/tools/traits.rs
- src/openhuman/approval/gate.rs
Contributor
Author
|
@coderabbitai review All 8 inline findings addressed in commits ae871b0..6d33ca2 (push c2caeccc..6d33ca2). Per-comment summary in the inline replies. CI: 22/22 green including Coverage Gate ≥ 80% and PR Submission Checklist. Requesting fresh review to clear changes_requested. |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/openhuman/agent/triage/escalation.rs (1)
88-129:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis approval key won't suppress the later per-tool prompts.
This gate records approval against
triage.react/triage.escalate, but the actual external writes later run under different tool keys (pushover,channels.proactive_send, Composio action tools, etc.). So an approval here does not get consumed by the downstream execution paths, and users can still see one prompt for the escalation plus another prompt for the first external-effect tool it invokes. If the intent is an up-front “approve this escalation” decision, that approval needs to flow through as a decision/token the later tool gates honor; otherwise this layer is just an extra prompt.🤖 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/triage/escalation.rs` around lines 88 - 129, The current approval uses tool_key = format!("triage.{}", run.decision.action.as_str()) so ApprovalGate::intercept only records approval for "triage.*" but downstream tool gates look up keys like "pushover" or "channels.proactive_send", so the triage allow isn't consumed; change the flow so when ApprovalGate::intercept returns GateOutcome::Allow you create and attach a transferable approval token/allowlist entry (e.g., generate approval_key or approval_id) into the run or envelope state (e.g., envelope.metadata or run.session.allowlist) and ensure downstream tool invocation code/gates check and consume that token (or map triage approval to the actual tool keys) so approvals recorded in ApprovalGate::intercept are honored by the per-tool gates for "pushover", "channels.proactive_send", Composio tools, etc.
🧹 Nitpick comments (1)
src/openhuman/approval/store.rs (1)
212-216: 💤 Low valueSilent fallback to
Utc::now()may mask data corruption.If the stored timestamp is malformed, substituting
Utc::now()silently could produce misleadingcreated_at/expires_atvalues. Consider adding atracing::warn!on parse failure so corrupted rows are observable.🔧 Optional: add a warning log
fn parse_rfc3339(input: &str) -> DateTime<Utc> { DateTime::parse_from_rfc3339(input) .map(|t| t.with_timezone(&Utc)) - .unwrap_or_else(|_| Utc::now()) + .unwrap_or_else(|err| { + tracing::warn!(input = input, error = %err, "[approval::store] invalid RFC3339 timestamp, using now"); + Utc::now() + }) }🤖 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/approval/store.rs` around lines 212 - 216, The parse_rfc3339 function currently swallows parse errors and returns Utc::now(), which can hide corrupted timestamps; modify parse_rfc3339 to log parse failures using tracing::warn! (include the input string and the parse error) before falling back to Utc::now(), so malformed rows are observable; keep the existing return behavior but ensure the warning contains the function name or context, the offending input, and the error message to aid debugging.
🤖 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/approval/redact.rs`:
- Around line 112-137: scrub_paths currently only matches "{sep}Users{sep}"
(using needle), so Windows paths like "C:\Users\name\..." are missed; update
scrub_paths to locate "Users" occurrences rather than relying on a leading
MAIN_SEPARATOR and treat a match valid if it is preceded by a path separator or
by a Windows drive pattern (a letter + ':' immediately before "Users"); then
when you find such a "Users" match (use rest.find("Users") or similar), replace
the whole "Users/<username>" or "Users\<username>" segment with "<HOME>" the
same way you do now (use MAIN_SEPARATOR to find the end of the username and
advance rest accordingly). Use the existing symbols scrub_paths, rest, needle
(remove or replace its use), MAIN_SEPARATOR, and after to implement this
cross-platform matching and skipping logic.
In `@src/openhuman/tools/impl/system/pushover.rs`:
- Around line 114-118: The external_effect() implementation should not
unconditionally return true; update external_effect() to mirror the pre-checks
used by execute() so it returns false when the send is impossible (e.g., when
the action is read-only or rate-limited). Locate external_effect() in
pushover.rs and have it call the same helper or inline the same conditions used
by execute() (or a can_send()/is_read_only()/is_rate_limited() helper) so the
gate will not prompt for approval for requests that execute() will immediately
reject.
---
Duplicate comments:
In `@src/openhuman/agent/triage/escalation.rs`:
- Around line 88-129: The current approval uses tool_key = format!("triage.{}",
run.decision.action.as_str()) so ApprovalGate::intercept only records approval
for "triage.*" but downstream tool gates look up keys like "pushover" or
"channels.proactive_send", so the triage allow isn't consumed; change the flow
so when ApprovalGate::intercept returns GateOutcome::Allow you create and attach
a transferable approval token/allowlist entry (e.g., generate approval_key or
approval_id) into the run or envelope state (e.g., envelope.metadata or
run.session.allowlist) and ensure downstream tool invocation code/gates check
and consume that token (or map triage approval to the actual tool keys) so
approvals recorded in ApprovalGate::intercept are honored by the per-tool gates
for "pushover", "channels.proactive_send", Composio tools, etc.
---
Nitpick comments:
In `@src/openhuman/approval/store.rs`:
- Around line 212-216: The parse_rfc3339 function currently swallows parse
errors and returns Utc::now(), which can hide corrupted timestamps; modify
parse_rfc3339 to log parse failures using tracing::warn! (include the input
string and the parse error) before falling back to Utc::now(), so malformed rows
are observable; keep the existing return behavior but ensure the warning
contains the function name or context, the offending input, and the error
message to aid debugging.
🪄 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: a88a0bc5-1494-4571-a674-1d498db1046d
📒 Files selected for processing (19)
src/core/all.rssrc/core/event_bus/events.rssrc/core/jsonrpc.rssrc/openhuman/agent/harness/subagent_runner/ops.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/agent/triage/escalation.rssrc/openhuman/approval/gate.rssrc/openhuman/approval/mod.rssrc/openhuman/approval/redact.rssrc/openhuman/approval/rpc.rssrc/openhuman/approval/schemas.rssrc/openhuman/approval/store.rssrc/openhuman/approval/types.rssrc/openhuman/channels/proactive.rssrc/openhuman/tools/impl/network/composio.rssrc/openhuman/tools/impl/network/gmail_unsubscribe.rssrc/openhuman/tools/impl/system/pushover.rssrc/openhuman/tools/traits.rssrc/openhuman/voice/reply_speech.rs
…nsai#1339) Per CodeRabbit on PR tinyhumansai#2149: scrub_paths only matched {MAIN_SEPARATOR}Users{MAIN_SEPARATOR}, so Windows paths like C:\Users\<name>\... slipped through on Unix-built artifacts reading cross-platform log payloads. Rewrote to recognise the three real shapes: - /Users/<name>/... (macOS) - /home/<name>/... (Linux) - C:\Users\<name>\... (Windows, any drive letter, case-insensitive) Multiple home paths in the same string are all scrubbed (was a single-shot loop bug). Three new tests cover Windows, Linux, and multi-path strings.
…1339) Per CodeRabbit on PR tinyhumansai#2149: gate runs BEFORE execute(), so marking pushover external_effect=true raised approvals even for calls that execute() would immediately reject via security.can_act() (read-only autonomy) or security.record_action() (rate limit) — a dead-end approval flow for actions that can never run. Keep this tool at the trait default (false) until the gate can consult SecurityPolicy before parking. Inline comment notes the follow-up tinyhumansai#1339-v3.
10 tasks
graycyrus
reviewed
May 19, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — graycyrus
Solid implementation of the approval gate backend for #1339. The architecture is well-thought-out: defense in depth at four interception points (tool_loop, subagent_runner, proactive channels, triage escalation), fail-closed on every error path, and the OnceLock + Option pattern means the default-off path is truly zero-cost. The 46 unit tests cover the critical flows, and all CI checks pass cleanly.
CodeRabbit caught the significant issues (waiter-before-persist race, denial error formatting, docs alignment, composio read-only gating, cross-platform path scrubbing) — all addressed. Nice work responding to those.
Change summary
| File | Change | Description |
|---|---|---|
approval/gate.rs |
New | Core ApprovalGate — intercept, decide, session allowlist, timeout |
approval/store.rs |
New | SQLite persistence for pending approvals |
approval/redact.rs |
New | PII/path redaction for approval summaries |
approval/types.rs |
New | PendingApproval, ApprovalDecision, GateOutcome |
approval/rpc.rs |
New | approval_list_pending + approval_decide RPC ops |
approval/schemas.rs |
New | Controller registry wiring |
approval/mod.rs |
Modified | Module re-exports for new submodules |
core/all.rs |
Modified | Register approval controllers + schemas |
core/event_bus/events.rs |
Modified | ApprovalRequested + ApprovalDecided domain events |
core/jsonrpc.rs |
Modified | Bootstrap gate on OPENHUMAN_APPROVAL_GATE=1 |
tools/traits.rs |
Modified | external_effect() + external_effect_with_args() trait methods |
tools/impl/network/composio.rs |
Modified | Args-aware gating (list/connect exempt) |
tools/impl/network/gmail_unsubscribe.rs |
Modified | Documented as intentionally non-gated (legacy UI) |
tools/impl/system/pushover.rs |
Modified | Documented as intentionally non-gated (policy pre-check) |
agent/harness/tool_loop.rs |
Modified | Gate check before execute() in main agent loop |
agent/harness/subagent_runner/ops.rs |
Modified | Gate check in subagent tool execution |
agent/triage/escalation.rs |
Modified | Gate at escalation dispatch entry point |
channels/proactive.rs |
Modified | Gate before external-channel delivery |
voice/reply_speech.rs |
Modified | Doc comment: TTS is internal, never gated |
Two minor observations below — neither is blocking.
| /// or unknown id). | ||
| pub fn decide( | ||
| config: &Config, | ||
| request_id: &str, |
Contributor
There was a problem hiding this comment.
[minor] list_pending returns all rows with decided_at IS NULL but doesn't filter by expires_at. If the process crashes between insert_pending and the timeout handler marking the row decided, orphan rows with a past expires_at will appear as still-pending indefinitely.
The PR docs say orphans are intentionally preserved for audit — that's fine. But the query could add AND (expires_at IS NULL OR expires_at > datetime('now')) so the UI doesn't have to implement its own expiry filter. Alternatively, document that the UI must filter client-side.
Non-blocking — the follow-up React UI can handle this either way.
| .context("[approval::store] serialize args_redacted")?; | ||
| let created = pending.created_at.to_rfc3339(); | ||
| let expires = pending.expires_at.map(|t| t.to_rfc3339()); | ||
| conn.execute( |
Contributor
There was a problem hiding this comment.
[minor] The store uses the default SQLite journal mode. Since intercept and list_pending / decide can be called concurrently from async tasks (each opening their own connection via with_connection), WAL mode would reduce SQLITE_BUSY contention under load.
One line in the schema block:
PRAGMA journal_mode = WAL;Non-blocking — the per-call connection pattern limits concurrency pressure in practice, and this matches notifications/store.rs's current approach.
graycyrus
reviewed
May 19, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review — prior minor findings (expires_at filter in list_pending, WAL mode for the approval store) remain non-blocking and are documented as follow-up-safe. No new commits since last review, all CI green, all CodeRabbit issues addressed. Clean from a blocking-findings perspective.
Moving to approval queue.
12 tasks
12 tasks
This was referenced May 20, 2026
mtkik
pushed a commit
to mtkik/openhuman-meet
that referenced
this pull request
May 21, 2026
43 tasks
CodeGhost21
pushed a commit
to CodeGhost21/openhuman
that referenced
this pull request
May 22, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ApprovalGatemiddleware that parks any tool call whoseTool::external_effect()returnstrue(composio, pushover, gmail_unsubscribe, proactive external-channel sends, triage React/Escalate dispatch) until the user decides.pending_approvalsSQLite table so they survive a core restart; the gate exposesapproval_list_pending+approval_decideover JSON-RPC and emitsDomainEvent::ApprovalRequested/ApprovalDecidedon the event bus.OPENHUMAN_APPROVAL_GATE=1. When unset,ApprovalGate::try_global()returnsNoneand the agent harness behaves exactly as before — safe to merge ahead of the React UI.Problem
Per #1339, proactive triggers (
src/openhuman/channels/proactive.rs), agent side-effect tools (composio, pushover, gmail_unsubscribe), and triage React/Escalate dispatch all fire side effects without an explicit user-consent gate.PermissionLevelfilters tools but does not gate execution through an approval queue, so an autonomous agent can publish to Slack, send email, or create calendar events without surfacing the action to the user first.Solution
fn external_effect(&self) -> bool { false }tosrc/openhuman/tools/traits.rs. Composio, pushover, gmail_unsubscribe override totrue. Voicereply_speechis documented as internal (default false) so a future Tool-wrap doesn't accidentally flip it (Polish mascot-driven TTS and STT voice experience #1206 coordination).src/openhuman/approval/gate.rs) —ApprovalGate::intercept(tool, summary, redacted_args)checks the session-scoped allowlist (set by priorApproveAlwaysForTooldecisions), then persists a row, emitsDomainEvent::ApprovalRequested, and parks the future on aoneshot::Senderkeyed byrequest_id. Default TTL = 10 minutes; timeout →Deny.src/openhuman/approval/store.rs) — synchronousrusqlitewith the samewith_connectionpattern asnotifications/store.rs.list_pendingreturns every undecided row regardless of session so the UI can show orphans after restart per the acceptance criterion; decisions on orphan rows update the DB but no live waiter resumes, so no side effect can fire across processes.src/openhuman/approval/redact.rs) — case-insensitive sensitive-key match (body,message,token,recipient,name, …) plus home-path scrub; arrays/objects collapse to<redacted: array (N items)>markers. Perfeedback_redact_paths_and_ids_in_public.mdandfeedback_pr_no_chat_content.md.src/openhuman/approval/rpc.rs+schemas.rs) —openhuman.approval_list_pending+openhuman.approval_decidewired into the controller registry viasrc/core/all.rs. Decision values:approve_once,approve_always_for_tool,deny.tool_loop.rsandsubagent_runner/ops.rsroute external-effect tools through the gate;channels/proactive.rsgates external-channel sends (web exempt);agent/triage/escalation.rsgates React/Escalate dispatch at the entry point so the user can decline the whole escalation up-front. Defense in depth — the session allowlist short-circuits subsequent prompts after the first approval.src/core/jsonrpc.rs) — gate is installed only whenOPENHUMAN_APPROVAL_GATE=1and keyed onOPENHUMAN_CORE_TOKENfor session binding. Default off until the UI lands.Submission Checklist
openhuman::approvalcovering gate allow/deny/timeout/always-allowlist/unknown-id, store CRUD + cross-handle survival + purge_session, redactor sensitive-key match + array-count + home-path scrub, types decision serde, and RPC schemasdiff-cover) meet the gate enforced by.github/workflows/coverage.yml. Localcargo test --lib openhuman::approvalcovers gate + store + redactor + types + schemas (46/46 pass); CI will confirm cross-file diff coverage on the merged lcov.docs/TEST-COVERAGE-MATRIX.md. The follow-up UI PR will add the matrix entry when the toast + settings panel land.docs/RELEASE-MANUAL-SMOKE.mdupdate needed for this PR. Follow-up UI PR will add the smoke checklist entry.Closes #NNNin the## RelatedsectionImpact
OPENHUMAN_APPROVAL_GATEunset →ApprovalGate::try_global()returnsNone→ zero behavior change in production. Enabling the env var without a UI deployed will park tool calls until the 10-minute TTL elapses (Deny) — intentional safety default; documented in the bootstrap log line and the gate module header.OPENHUMAN_CORE_TOKEN(session_id field) for audit; decisions on orphan rows from prior launches update the DB but cannot resume a parked future across processes, so no side effect can replay. Redactor strips secrets before any data leaves the gate.CREATE TABLE IF NOT EXISTS), applied per-connection likenotifications/store.rs— no separate migration step.Live smoke (backend only — no app required)
list_pendingreturns redacted shape)decision="approve_once"→ escalation dispatched to orchestratordecision="deny"→triage::escalationlog:approval gate denied dispatch ... reason=User denied 'triage.escalate' execution.args_redactedreturns onlyaction / external_id / label / prompt_chars / target_agent; no email body, subject, sender, recipientRelated
PendingApprovalsToast+ApprovalsPanelsettings page + authenticated SSE subscription (will usebuildWebhookEventsUrlhelper from Security: SSE EventSource in useWebhooks.ts has no authentication #1922 once it lands)action=list/connectbypass the gate (#1339-v2inline comment incomposio.rs)current_sandbox_mode()task-local (coordinates with Make sandboxed agents use real OS-enforced workspace isolation #1401)AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/1339-triggers-approval-gatecaeccc9cb63c165eda55cf550a4c27e94f235eb0Validation Run
app/files touchedapp/files touchedcargo test --lib openhuman::approval→ 46 passedcargo fmt --check+cargo checkboth cleanapp/src-tauri/files touchedValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
OPENHUMAN_APPROVAL_GATE=1, agent tool calls withexternal_effect()==truepark untilapproval_decideresolves; default-off path is unchanged.approval_list_pending+approval_decide.Parity Contract
ApprovalGate::try_global() == Nonereturns the pre-Keep triggers read-only until users approve writes #1339Allowshort-circuit at every hook site; the existing CLIApprovalManagerpath intool_loopis untouched.try_global()inOptionso absence of the gate is a no-op;reply_speechdocumented as internal (default false) so the gate never fires on TTS.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Documentation