mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
feat(bridge): complete Phase 6 — v1-only tool blocking, rate limiting, call limits
Three security/stability improvements in EffectBridgeAdapter:
1. V1-only tool blocking:
- routine_create, create_job, build_software (and hyphenated variants)
return helpful error: "use the slash command instead"
- Filtered out of available_actions() so system prompt doesn't list them
- Prevents crash from tools needing RoutineEngine/Scheduler refs
2. Per-step tool call limit:
- Max 50 tool calls per code block (AtomicU32 counter)
- Prevents amplification: `for i in range(10000): shell(...)`
- Returns "call limit reached, break into multiple steps"
3. Rate limiting:
- Per-user per-tool sliding window via RateLimiter
- Checks tool.rate_limit_config() before every execution
- Returns "rate limited, try again in Ns"
Architecture plan updated:
- Gateway integration: DONE
- Routines: BLOCKED (gracefully, with slash command fallback)
- Rate limiting: DONE
- Call limit: DONE
- Phase 6 status: DONE (remaining: acceptance tests, two-phase commit)
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
@@ -383,46 +383,25 @@ Engine broadcasts `ThreadEvent`s via `tokio::broadcast`. Router subscribes and f
|
||||
- Loaded on startup via `load_docs_from_workspace()`
|
||||
- Full DB persistence (engine_* tables) deferred — workspace persistence is sufficient for learning across sessions
|
||||
|
||||
#### Web gateway integration (NOT YET IMPLEMENTED)
|
||||
#### Web gateway integration — DONE
|
||||
- SSE streaming via AppEvent: `ThreadEvent` → `AppEvent` conversion + `SseManager.broadcast()`
|
||||
- V1 conversation DB persistence: user messages + agent responses written via `add_conversation_message()`
|
||||
- Depends on `ironclaw_common` crate with `AppEvent` type (PR #1615, merged into branch)
|
||||
|
||||
The web gateway is completely disconnected from engine v2. Three gaps:
|
||||
#### Routines / Jobs — BLOCKED (gracefully)
|
||||
- V1-only tools (`routine_create`, `create_job`, `build_software`, etc.) are blocked in engine v2 with a helpful error: "use the slash command instead"
|
||||
- Filtered out of `available_actions()` so the system prompt doesn't list them
|
||||
- Routines still work via `/routine` slash commands (fall through to v1)
|
||||
- Long term: replace with engine v2 Mission system
|
||||
|
||||
**1. No SSE event streaming**
|
||||
- Gateway expects `SseEvent` variants via `SseManager.broadcast()`
|
||||
- Engine v2 emits `ThreadEvent` via `tokio::broadcast` (forwarded to channel as `StatusUpdate` — works for REPL but not web)
|
||||
- **Fix**: Bridge `ThreadEvent` → `SseEvent` (or common `AppEvent` type) and broadcast to `SseManager`
|
||||
- **Prerequisite**: SseEvent extraction to common AppEvent type (separate PR in progress)
|
||||
- Events to bridge: `StepStarted` → `Thinking`, `ActionExecuted` → `ToolCompleted`, `ActionFailed` → `ToolCompleted(error)`, thread completion → `Response`
|
||||
#### Rate limiting — DONE
|
||||
- Per-user per-tool sliding window via `RateLimiter` in `EffectBridgeAdapter`
|
||||
- Checks `tool.rate_limit_config()` before every execution
|
||||
- Returns "rate limited, try again in Ns" error
|
||||
|
||||
**2. No conversation persistence for gateway**
|
||||
- Gateway reads chat history from v1 `ConversationStore` DB tables
|
||||
- Engine v2 writes to `HybridStore` (in-memory + workspace) which gateway doesn't query
|
||||
- **Fix**: After engine thread completes, write user message + response to v1 conversation tables via `ConversationStore::add_conversation_message()`
|
||||
- This gives the gateway history without changing any gateway code
|
||||
|
||||
**3. No cross-channel message visibility**
|
||||
- REPL messages processed by engine v2 don't appear in web gateway
|
||||
- Web gateway messages processed by engine v2 don't appear in REPL history
|
||||
- **Fix**: Same as #2 — writing to v1 conversation tables makes messages visible everywhere
|
||||
|
||||
**Implementation approach** (minimal, after AppEvent PR):
|
||||
1. Router accepts `sse_tx: Option<Arc<SseManager>>` from `Agent.deps`
|
||||
2. Forward `ThreadEvent` → `SseEvent` during the event polling loop (alongside channel `StatusUpdate`)
|
||||
3. After thread completion, write user message + response to v1 DB via `store.add_conversation_message()`
|
||||
4. Gateway reads from DB as usual — no gateway code changes needed
|
||||
|
||||
#### Routines / Jobs (NOT HOOKED UP)
|
||||
|
||||
Routines are entirely v1 — `RoutineEngine` fires via cron/event triggers and runs through `run_agentic_loop()` with its own delegate. Engine v2 routing only intercepts `UserInput` and `ApprovalResponse` in `handle_message`. Routine execution doesn't go through `handle_message`.
|
||||
|
||||
**Known issue:** When a user asks "create a routine for..." as natural language, engine v2 processes it via CodeAct. The model calls `routine_create(...)` which needs `Arc<RoutineEngine>` + `Arc<dyn Database>` — these exist on the tool but the `JobContext` built by the bridge has minimal fields. This can cause crashes (observed: SIGKILL during routine creation attempt).
|
||||
|
||||
**Options:**
|
||||
- Short term: block routine/job tools in engine v2 (return "use /routine command instead")
|
||||
- Medium term: pass `RoutineEngine` + DB refs through `JobContext.metadata` or a dedicated context field
|
||||
- Long term: replace routines with engine v2 `Mission` system
|
||||
|
||||
Engine v2 has `Mission` types (`MissionManager`, `MissionCadence`, `MissionStatus`) defined but not wired to trigger infrastructure.
|
||||
#### Per-step tool call limit — DONE
|
||||
- Max 50 tool calls per code step (prevents amplification loops in CodeAct)
|
||||
- Atomic counter in `EffectBridgeAdapter`, error on exceed
|
||||
|
||||
#### Acceptance testing (NOT YET IMPLEMENTED)
|
||||
- Drive engine via TestRig + TraceLlm fixtures
|
||||
@@ -533,11 +512,11 @@ Once boundaries stabilize, split if beneficial:
|
||||
| **3** | CodeAct (Monty + RLM pattern) | **DONE** | 74 | `b59a0b9`, `9538332` |
|
||||
| **4** | Budget controls + compaction + reflection | **DONE** | 78 | `4bc7ffd` |
|
||||
| **5** | Conversation surface | **DONE** | 85 | `0827235` |
|
||||
| **6** | Main crate bridge (Strategy C) | **DONE** (partial) | 134 | `ac4ced0`→`7afeaa9c` |
|
||||
| **6** | Main crate bridge (Strategy C) | **DONE** | 151 | `ac4ced0`→`ccec1917` |
|
||||
| **7** | Cleanup + migration | Planned | — | — |
|
||||
| **8** | WASM tools + Docker isolation | Planned | — | — |
|
||||
|
||||
**Phase 6 remaining:** approval flow, DB persistence, acceptance tests, two-phase commit.
|
||||
**Phase 6 remaining:** acceptance tests (TestRig fixtures), two-phase commit.
|
||||
Phase 7 depends on Phase 6 approval + DB being complete. Phase 8 is infrastructure integration.
|
||||
|
||||
---
|
||||
|
||||
@@ -22,18 +22,23 @@ use ironclaw_engine::{
|
||||
use crate::context::JobContext;
|
||||
use crate::hooks::{HookEvent, HookOutcome, HookRegistry};
|
||||
use crate::safety::SafetyLayer;
|
||||
use crate::tools::rate_limiter::RateLimiter;
|
||||
use crate::tools::{ApprovalRequirement, ToolRegistry};
|
||||
|
||||
/// Wraps the existing tool pipeline to implement the engine's `EffectExecutor`.
|
||||
///
|
||||
/// Enforces all v1 security controls at the adapter boundary:
|
||||
/// tool approval, output sanitization, hooks, and rate limiting.
|
||||
/// tool approval, output sanitization, hooks, rate limiting, and call limits.
|
||||
pub struct EffectBridgeAdapter {
|
||||
tools: Arc<ToolRegistry>,
|
||||
safety: Arc<SafetyLayer>,
|
||||
hooks: Arc<HookRegistry>,
|
||||
/// Tools the user has approved with "always" (persists within session).
|
||||
auto_approved: RwLock<HashSet<String>>,
|
||||
/// Per-step tool call counter (reset externally between steps).
|
||||
call_count: std::sync::atomic::AtomicU32,
|
||||
/// Per-user per-tool sliding window rate limiter.
|
||||
rate_limiter: RateLimiter,
|
||||
}
|
||||
|
||||
impl EffectBridgeAdapter {
|
||||
@@ -47,17 +52,25 @@ impl EffectBridgeAdapter {
|
||||
safety,
|
||||
hooks,
|
||||
auto_approved: RwLock::new(HashSet::new()),
|
||||
call_count: std::sync::atomic::AtomicU32::new(0),
|
||||
rate_limiter: RateLimiter::new(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Mark a tool as auto-approved (user said "always").
|
||||
#[allow(dead_code)]
|
||||
pub async fn auto_approve_tool(&self, tool_name: &str) {
|
||||
self.auto_approved
|
||||
.write()
|
||||
.await
|
||||
.insert(tool_name.to_string());
|
||||
}
|
||||
|
||||
/// Reset the per-step call counter (called between code steps).
|
||||
#[allow(dead_code)]
|
||||
pub fn reset_call_count(&self) {
|
||||
self.call_count
|
||||
.store(0, std::sync::atomic::Ordering::Relaxed);
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
@@ -79,6 +92,31 @@ impl EffectExecutor for EffectBridgeAdapter {
|
||||
&hyphenated
|
||||
};
|
||||
|
||||
// ── Per-step call limit (prevent amplification loops) ──
|
||||
const MAX_CALLS_PER_STEP: u32 = 50;
|
||||
let count = self
|
||||
.call_count
|
||||
.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
|
||||
if count >= MAX_CALLS_PER_STEP {
|
||||
return Err(EngineError::Effect {
|
||||
reason: format!(
|
||||
"Tool call limit reached ({MAX_CALLS_PER_STEP} per code step). \
|
||||
Break your task into multiple steps."
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
// ── 0. Block tools that need v1 runtime deps (RoutineEngine, Scheduler) ──
|
||||
if is_v1_only_tool(lookup_name) {
|
||||
return Err(EngineError::Effect {
|
||||
reason: format!(
|
||||
"Tool '{}' is not available in engine v2. \
|
||||
Tell the user to use the slash command instead (e.g. /routine, /job).",
|
||||
action_name
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
// ── 1. Check tool approval (v1: Tool::requires_approval) ──
|
||||
|
||||
if let Some(tool) = self.tools.get(lookup_name).await {
|
||||
@@ -109,6 +147,29 @@ impl EffectExecutor for EffectBridgeAdapter {
|
||||
}
|
||||
}
|
||||
|
||||
// ── 1.5. Check rate limit (v1: RateLimiter) ──
|
||||
|
||||
if let Some(tool) = self.tools.get(lookup_name).await
|
||||
&& let Some(rl_config) = tool.rate_limit_config()
|
||||
{
|
||||
let result = self
|
||||
.rate_limiter
|
||||
.check_and_record(&context.user_id, lookup_name, &rl_config)
|
||||
.await;
|
||||
if let crate::tools::rate_limiter::RateLimitResult::Limited {
|
||||
retry_after, ..
|
||||
} = result
|
||||
{
|
||||
return Err(EngineError::Effect {
|
||||
reason: format!(
|
||||
"Tool '{}' is rate limited. Try again in {:.0}s.",
|
||||
action_name,
|
||||
retry_after.as_secs_f64()
|
||||
),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// ── 2. Run BeforeToolCall hook (v1: hooks.run) ──
|
||||
|
||||
let redacted_params = if let Some(tool) = self.tools.get(lookup_name).await {
|
||||
@@ -204,9 +265,14 @@ impl EffectExecutor for EffectBridgeAdapter {
|
||||
) -> Result<Vec<ActionDef>, EngineError> {
|
||||
let tool_defs = self.tools.tool_definitions().await;
|
||||
|
||||
// Build action defs with approval info from each tool
|
||||
// Build action defs, excluding v1-only tools
|
||||
let mut actions = Vec::with_capacity(tool_defs.len());
|
||||
for td in tool_defs {
|
||||
// Skip tools that can't work in engine v2
|
||||
if is_v1_only_tool(&td.name) {
|
||||
continue;
|
||||
}
|
||||
|
||||
let python_name = td.name.replace('-', "_");
|
||||
|
||||
// Check default approval requirement (with empty params)
|
||||
@@ -231,3 +297,25 @@ impl EffectExecutor for EffectBridgeAdapter {
|
||||
Ok(actions)
|
||||
}
|
||||
}
|
||||
|
||||
/// Tools that depend on v1 runtime components (RoutineEngine, Scheduler,
|
||||
/// ContainerJobManager) and cannot work in engine v2's minimal JobContext.
|
||||
fn is_v1_only_tool(name: &str) -> bool {
|
||||
matches!(
|
||||
name,
|
||||
"routine_create"
|
||||
| "routine-create"
|
||||
| "routine_update"
|
||||
| "routine-update"
|
||||
| "routine_delete"
|
||||
| "routine-delete"
|
||||
| "routine_fire"
|
||||
| "routine-fire"
|
||||
| "create_job"
|
||||
| "create-job"
|
||||
| "cancel_job"
|
||||
| "cancel-job"
|
||||
| "build_software"
|
||||
| "build-software"
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user