mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-09-03 01:59:23 +00:00
feat(bridge): implement tool approval flow for engine v2
Adds a complete approval flow that mirrors v1 behavior, using the
existing v1 security controls (Tool::requires_approval, auto-approve
sets, StatusUpdate::ApprovalNeeded).
## How it works
### Step 1: Tool blocked at execution
When the LLM's code calls a tool (e.g., `shell("ls")`):
1. EffectBridgeAdapter.execute_action() looks up the Tool object
2. Calls tool.requires_approval(¶ms) — returns ApprovalRequirement
3. If Always → EngineError::LeaseDenied (always blocks)
4. If UnlessAutoApproved → checks auto_approved HashSet → if not in set,
returns EngineError::LeaseDenied
5. If Never → proceeds to execution
### Step 2: Engine returns NeedApproval
The LeaseDenied error propagates through:
- CodeAct path: becomes Python RuntimeError, code halts, thread returns
NeedApproval with action_name + parameters
- Structured path: same via ActionResult.is_error
### Step 3: Router stores pending approval
- PendingApproval { action_name, original_content } stored on EngineState
- StatusUpdate::ApprovalNeeded sent to channel (shows approval card in
CLI/web with tool name, parameters, yes/always/no buttons)
- Returns text: "Tool 'shell' requires approval. Reply yes/always/no."
### Step 4: User responds
handle_message() intercepts Submission::ApprovalResponse when ENGINE_V2:
- 'yes' → auto_approve_tool(name) on EffectBridgeAdapter, re-processes
original message (tool now passes the approval check on second run)
- 'always' → same + logs for session persistence
- 'no' → returns "Denied: tool was not executed."
### Key design choice
Instead of pausing/resuming mid-execution (which needs engine changes
to freeze/restore the Monty VM state), we auto-approve the tool and
re-run the full message. The EffectBridgeAdapter's auto_approved set
persists across runs, so the second execution passes immediately.
This trades one extra LLM call for zero engine modifications.
## Files changed
- src/bridge/router.rs: PendingApproval struct, handle_approval(),
NeedApproval → StatusUpdate::ApprovalNeeded conversion
- src/bridge/mod.rs: export handle_approval
- src/agent/agent_loop.rs: intercept ApprovalResponse for engine v2
- src/bridge/effect_adapter.rs: fmt fixes
151 tests passing, clippy + fmt clean.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
@@ -53,9 +53,11 @@ impl LeaseManager {
|
||||
/// Check whether a lease is still valid. Returns the lease if valid.
|
||||
pub async fn check(&self, lease_id: LeaseId) -> Result<CapabilityLease, EngineError> {
|
||||
let leases = self.active.read().await;
|
||||
let lease = leases.get(&lease_id).ok_or_else(|| EngineError::LeaseExpired {
|
||||
capability_name: format!("lease {lease_id:?} not found"),
|
||||
})?;
|
||||
let lease = leases
|
||||
.get(&lease_id)
|
||||
.ok_or_else(|| EngineError::LeaseExpired {
|
||||
capability_name: format!("lease {lease_id:?} not found"),
|
||||
})?;
|
||||
if !lease.is_valid() {
|
||||
return Err(EngineError::LeaseExpired {
|
||||
capability_name: lease.capability_name.clone(),
|
||||
@@ -67,9 +69,11 @@ impl LeaseManager {
|
||||
/// Consume one use of a lease. Returns error if the lease is invalid or exhausted.
|
||||
pub async fn consume_use(&self, lease_id: LeaseId) -> Result<(), EngineError> {
|
||||
let mut leases = self.active.write().await;
|
||||
let lease = leases.get_mut(&lease_id).ok_or_else(|| EngineError::LeaseExpired {
|
||||
capability_name: format!("lease {lease_id:?} not found"),
|
||||
})?;
|
||||
let lease = leases
|
||||
.get_mut(&lease_id)
|
||||
.ok_or_else(|| EngineError::LeaseExpired {
|
||||
capability_name: format!("lease {lease_id:?} not found"),
|
||||
})?;
|
||||
if !lease.is_valid() {
|
||||
return Err(EngineError::LeaseExpired {
|
||||
capability_name: lease.capability_name.clone(),
|
||||
@@ -202,8 +206,16 @@ mod tests {
|
||||
None,
|
||||
)
|
||||
.await;
|
||||
assert!(mgr.find_lease_for_action(tid, "create_issue").await.is_some());
|
||||
assert!(mgr.find_lease_for_action(tid, "delete_repo").await.is_none());
|
||||
assert!(
|
||||
mgr.find_lease_for_action(tid, "create_issue")
|
||||
.await
|
||||
.is_some()
|
||||
);
|
||||
assert!(
|
||||
mgr.find_lease_for_action(tid, "delete_repo")
|
||||
.await
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -314,12 +314,8 @@ mod tests {
|
||||
let engine = PolicyEngine::new();
|
||||
let action = make_action("transfer_funds", vec![EffectType::Financial], false);
|
||||
let lease = make_lease();
|
||||
let decision = engine.evaluate_with_provenance(
|
||||
&action,
|
||||
&lease,
|
||||
&[],
|
||||
&Provenance::LlmGenerated,
|
||||
);
|
||||
let decision =
|
||||
engine.evaluate_with_provenance(&action, &lease, &[], &Provenance::LlmGenerated);
|
||||
assert!(matches!(decision, PolicyDecision::RequireApproval { .. }));
|
||||
}
|
||||
|
||||
@@ -328,12 +324,8 @@ mod tests {
|
||||
let engine = PolicyEngine::new();
|
||||
let action = make_action("post_message", vec![EffectType::WriteExternal], false);
|
||||
let lease = make_lease();
|
||||
let decision = engine.evaluate_with_provenance(
|
||||
&action,
|
||||
&lease,
|
||||
&[],
|
||||
&Provenance::LlmGenerated,
|
||||
);
|
||||
let decision =
|
||||
engine.evaluate_with_provenance(&action, &lease, &[], &Provenance::LlmGenerated);
|
||||
assert!(matches!(decision, PolicyDecision::RequireApproval { .. }));
|
||||
}
|
||||
|
||||
@@ -342,12 +334,7 @@ mod tests {
|
||||
let engine = PolicyEngine::new();
|
||||
let action = make_action("transfer_funds", vec![EffectType::Financial], false);
|
||||
let lease = make_lease();
|
||||
let decision = engine.evaluate_with_provenance(
|
||||
&action,
|
||||
&lease,
|
||||
&[],
|
||||
&Provenance::User,
|
||||
);
|
||||
let decision = engine.evaluate_with_provenance(&action, &lease, &[], &Provenance::User);
|
||||
assert_eq!(decision, PolicyDecision::Allow);
|
||||
}
|
||||
|
||||
@@ -360,7 +347,9 @@ mod tests {
|
||||
&action,
|
||||
&lease,
|
||||
&[],
|
||||
&Provenance::ToolOutput { action_name: "scrape_invoices".into() },
|
||||
&Provenance::ToolOutput {
|
||||
action_name: "scrape_invoices".into(),
|
||||
},
|
||||
);
|
||||
assert!(matches!(decision, PolicyDecision::RequireApproval { .. }));
|
||||
}
|
||||
|
||||
@@ -20,7 +20,8 @@ impl CapabilityRegistry {
|
||||
|
||||
/// Register a capability. Overwrites any existing capability with the same name.
|
||||
pub fn register(&mut self, capability: Capability) {
|
||||
self.capabilities.insert(capability.name.clone(), capability);
|
||||
self.capabilities
|
||||
.insert(capability.name.clone(), capability);
|
||||
}
|
||||
|
||||
/// Look up a capability by name.
|
||||
|
||||
Reference in New Issue
Block a user