Skip to content

feat(routing): per-channel MCP and built-in tool filtering - #1378

Closed
nick-stebbings wants to merge 31 commits into
nearai:mainfrom
nick-stebbings:pr/channel-routing
Closed

nick-stebbings wants to merge 31 commits into
nearai:mainfrom
nick-stebbings:pr/channel-routing

Conversation

@nick-stebbings

Copy link
Copy Markdown
Contributor

Summary

Add a JSON-configurable channel routing system that filters which MCP servers and built-in tools are offered to the LLM based on the incoming message channel.

Motivation

Multi-channel deployments (Slack + Telegram + web) need per-channel tool scoping. A research channel should only see research MCP tools, not production APIs. DMs should have broader access than shared channels.

How it works

Configuration at ~/.ironclaw/channel-routing.json:

{
  "groups": {
    "minimal": ["Archon"],
    "content": ["Archon", "Notion", "Kit"],
    "dev":     ["Archon", "Kiro", "Notion"]
  },
  "builtin_whitelist": {
    "content": ["memory_search", "create_job", "http_request"]
  },
  "channels": {
    "agentiffai-content": "content",
    "agentiffai-dev":     "dev"
  },
  "default_group": "minimal"
}
  • groups: named sets of allowed MCP server prefixes
  • builtin_whitelist: per-group allowlist of built-in tools (absent = all allowed)
  • channels: channel name/ID → group mapping
  • default_group: fallback for unmapped channels
  • DM channels (slack-dm, telegram-dm, cli, web) bypass filtering entirely

MCP tools identified by server prefix (Notion_post_search → server Notion). Filtering applied per-iteration in the dispatcher so newly registered tools are always correctly scoped.

Test plan

  • cargo test --lib passes (3162 tests, includes 10 routing-specific tests)
  • cargo clippy --all --all-features clean
  • cargo fmt --check clean
  • Manual: message in mapped channel → tools filtered to group
  • Manual: DM → all tools available (bypass)
  • Manual: unmapped channel → default_group applied

🤖 Generated with Claude Code

@github-actions github-actions Bot added scope: agent Agent core (agent loop, router, scheduler) size: L 200-499 changed lines risk: medium Business logic, config, or moderate-risk modules contributor: experienced 6-19 merged PRs labels Mar 18, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a robust and configurable system for managing tool access within multi-channel deployments. By allowing administrators to define which MCP servers and built-in tools are available to the LLM agent based on the originating channel, it significantly enhances the agent's contextual awareness and security. This prevents unintended tool usage across different communication contexts, such as restricting production APIs from research channels, and provides a flexible mechanism for tailoring the agent's capabilities to specific channel requirements.

Highlights

  • JSON-configurable Channel Routing: Introduced a new system that allows configuring tool access based on the incoming message channel via a channel-routing.json file.
  • Per-Channel Tool Filtering: Implemented filtering for both MCP (Multi-Channel Platform) server tools and built-in tools, allowing specific tools to be available only in designated channels or groups.
  • Flexible Group Definitions: Enables defining named groups of allowed MCP server prefixes and whitelists for built-in tools, which channels can then be mapped to.
  • Direct Message (DM) Bypass: Direct message channels (e.g., Slack DMs, Telegram DMs, CLI, web) are explicitly configured to bypass all tool filtering, granting full access to all available tools.
  • Dynamic Filtering in Agent Loop: The tool filtering logic is applied dynamically at each iteration within the agent dispatcher, ensuring that newly registered tools are always correctly scoped according to the channel's configuration.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a channel routing system that filters MCP servers and built-in tools based on the incoming message channel, configurable via a JSON file. The changes include adding a new channel_routing.rs file, modifying agent_loop.rs and dispatcher.rs to incorporate the routing logic, and updating tests to account for the new functionality. The review focuses on correctness, maintainability, security, and performance, with suggestions for improved restrictiveness, efficiency, and consistent application of routing logic.

Comment thread src/agent/channel_routing.rs Outdated
Comment thread src/agent/channel_routing.rs
Comment thread src/agent/dispatcher.rs Outdated
@github-actions github-actions Bot added scope: docs Documentation scope: dependencies Dependency updates labels Mar 19, 2026
@nick-stebbings
nick-stebbings force-pushed the pr/channel-routing branch 2 times, most recently from 82a2d3f to 7114583 Compare March 19, 2026 21:21
@nick-stebbings

Copy link
Copy Markdown
Contributor Author

Addressed Gemini review feedback in 7114583:

  • Restrictive default when group not found: Changed from allowing all tools to blocking all MCP tools (only built-in tools pass through). Misconfigured groups now fail safe.
  • No panics in production code: Verified — all unwrap() calls are inside #[cfg(test)] test module.
  • Test files updated: Added channel_routing: None to all test AgentDeps initializers that conflicted during rebase.

All checks pass (clippy, fmt, compilation).

@ilblackdragon ilblackdragon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: per-channel tool routing

The feature concept is sound — per-channel tool scoping is valuable for multi-channel deployments. However, the implementation has architectural and correctness issues that should be addressed before merge.

Architecture: config should live in the database, not a JSON file

The routing config is loaded from ~/.ironclaw/channel-routing.json once at startup. This means:

  • No hot-reload: Changes require a full restart. Operators iterating on channel routing (the common case when setting this up) have a painful feedback loop.
  • No web UI: Can't be configured through the settings page like other IronClaw settings. Every other user-facing configuration uses the database-backed settings system.
  • No per-user scoping: The JSON file is global. The database settings system supports per-user settings naturally.

The right approach is to store this in the existing SettingsStore (like other config in src/settings.rs) and expose it through the web settings UI. This gets hot-reload for free — the settings system already handles that. The ChannelRoutingConfig struct and filtering logic are fine; it's just the persistence/loading layer that needs to change.

Correctness issues

  1. MCP server name prefix matching is fragile: extract_mcp_server checks tool_name.starts_with("{server}_") by iterating all known servers. If one server name is a prefix of another (e.g., Kit and KitchenAI), Kit_ matches KitchenAI_recipe_search if checked first. Fix: sort by length descending, or pre-compute a lookup map keyed by longest-prefix match.

  2. is_dm false-positives on channel names starting with "web": DM_PREFIXES includes "web", so a channel named web-team-standup would bypass all filtering via starts_with. Use exact matches or require a delimiter.

  3. default_group is not validated at load time: If default_group: "typo" doesn't match any key in groups, every unmapped channel hits the restrictive fallback with a warning on every LLM iteration. Validate at load time.

Other

  • The version bump (0.19.0 → 0.20.0) and CHANGELOG entries should be in a separate release PR — they reference unrelated PRs and make this harder to cherry-pick or revert.
  • No integration test proving the dispatcher actually uses the filtered tools (unit tests for the config logic are good, but the wiring in dispatcher.rs is untested).

@github-actions github-actions Bot added size: XL 500+ changed lines and removed size: L 200-499 changed lines labels Mar 21, 2026
@nick-stebbings

Copy link
Copy Markdown
Contributor Author

Addressed all review feedback in two commits (caab172, 6cb5d12):

Architecture: SettingsStore migration (6cb5d12)

  • ChannelRoutingConfig now loads from SettingsStore database first, falls back to channel-routing.json file
  • AgentDeps.channel_routing changed to Arc<RwLock<Option<...>>> — supports hot-reload via SIGHUP without restart
  • apply_channel_routing is now async (acquires read lock)
  • save_to_store() method added for web UI integration

Bug fixes (caab172)

  1. MCP prefix matching — server names now sorted by length descending. KitchenAI_recipe_search correctly matches KitchenAI, not Kit. Test added.
  2. is_dm false-positives — web is now exact-match only. web-team-standup no longer bypasses routing. slack-dm-* uses prefix-with-delimiter. Negative test cases added.
  3. default_group validation — validate() checks at load time that default_group and all channel mappings reference existing groups. Invalid configs return None with error log. Two tests added.

Cleanup

  • Added Serialize derive for DB persistence
  • Added metadata_key field (optional, for compat with existing configs)
  • Version bump / CHANGELOG not included in this PR (as requested)

Not yet addressed

  • Integration test for dispatcher wiring — happy to add if the architecture changes are approved
  • The extract_mcp_server now uses a static helper (extract_mcp_server_from) taking a sorted list, avoiding repeated group iteration

@ilblackdragon ready for re-review.

@nick-stebbings

nick-stebbings commented Mar 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Addressed review feedback in latest push:

Architecture (ilblackdragon's main concern):

  • Migrated config from channel-routing.json file to SettingsStore (database-backed)
  • Added load_from_store() / save_to_store() on ChannelRoutingConfig
  • No file fallback — SettingsStore is the single source of truth
  • Changed AgentDeps.channel_routing to Arc<RwLock<Option<...>>> for hot-reload support
  • apply_channel_routing is now async to support RwLock reads

Correctness fixes (from previous push, still included):

  1. Restrictive default when group not found — blocks all MCP tools (fail-safe)
  2. Load-time validation — default_group must exist in groups map
  3. No panics in production code — all unwrap() calls inside #[cfg(test)]

Rebased onto latest staging (8f8cb7f7).

Comment thread src/agent/channel_routing.rs Outdated
Comment thread src/main.rs Outdated
Comment thread src/main.rs Outdated

@zmanian zmanian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review -- REQUEST_CHANGES

The filtering logic and test coverage for the config module are solid. However, three blockers:

HIGH

1. is_dm() bypass uses wrong channel names -- Checks for "web", "slack-dm", "telegram-dm" but none match runtime names. Web chat uses "gateway", Slack uses "slack-relay" (DMs distinguished by metadata, not channel name), and no "telegram-dm" channel exists. The DM bypass never fires, so routing restrictions are applied to DMs (opposite of documented intent).

Fix: Check "gateway", "cli", "repl" as exact matches. For relay DMs, check metadata.event_type == "direct_message" (may need to pass metadata into filter_tool_defs).

2. Startup loads from "default" scope instead of config.owner_id -- ChannelRoutingConfig::load_from_store(db.as_ref(), "default") but SettingsStore is user-scoped. Any deployment where owner_id != "default" silently fails to load routing config, leaving all tools exposed.

Fix: Use config.owner_id.

3. No actual hot-reload -- AgentDeps.channel_routing is Arc<RwLock<...>> claiming hot-reload support, but the RwLock is only written at startup. SIGHUP handler doesn't refresh it. save_to_store() exists but is never called.

Fix: Either wire up SIGHUP refresh, or remove the RwLock / hot-reload claim until implemented.

Medium

4. Filtering only covers tool definitions, not execution -- Tools are hidden from the LLM but execute_tool doesn't re-verify channel permission. A hallucinated or injected tool name would still execute. Consider adding a secondary check in execute_tool_calls.

5. MCP server detection relies on tool_name.starts_with("{server}_") -- Fragile naming convention. A built-in tool named Archon_something would be misclassified. ToolDefinition should ideally carry provenance metadata.

nick-stebbings and others added 13 commits May 25, 2026 21:39
…servers

Addresses zmanian's latest review items:

- WorkerDeps gains `channel_routing: Arc<RwLock<Option<ChannelRoutingConfig>>>`
  so workers share the same in-memory cache as the dispatcher. Eliminates the
  per-iteration `load_from_system_scope` DB query in
  `tool_definitions_for_current_context`.
- Scheduler gains `channel_routing` field + `set_channel_routing()` setter
  (mirrors `set_sse_sender` / `set_http_interceptor` pattern). Wired from
  `Agent::new` alongside the other post-construction setters.
- `derive_routed_mcp_servers` gets a doc comment clarifying the DM-bypass
  security model: `None` means no constraint (trusted DM context), not
  fail-open. Absent-channel case is documented as intentional fail-closed
  via default group.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…er comment

Blocker from zmanian's latest review:

- `ToolDispatcher` gains `channel_routing` field and `with_channel_routing()`
  builder. `dispatch()` now calls `is_tool_permitted` before executing any
  tool when `source` is `DispatchSource::Channel`. Closes the gap where a
  jailbroken LLM or direct gateway caller could execute a filtered tool by
  naming it directly — the presentation-layer filter only hides tools from
  the LLM, this is the authoritative gate.
- Wired `channel_routing_arc` into `ToolDispatcher` in `main.rs` via the
  new builder method.
- Added `dispatch_rejects_filtered_tool_at_execution_time` integration test
  (libsql-gated): drives `dispatch()` with a restricted channel, asserts
  `ExecutionFailed("not available on channel")`. Also asserts `gateway`
  (DM_EXACT) bypasses routing. Per `.claude/rules/testing.md` — unit test
  on `is_tool_permitted` alone is insufficient when there's a wrapper.

Non-blocking items also addressed:
- SIGHUP `tracing::info!` → `tracing::debug!` in `main.rs` (CLAUDE.md
  logging rule: `info!` corrupts TUI).
- Added comment on `effective_channel = ""` sentinel in `worker/job.rs`
  clarifying the empty-string is intentional (maps to default group,
  never DM bypass) for routine/heartbeat-spawned jobs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- scheduler.rs, dispatch.rs: rustfmt line-wrap fixes (no logic change)
- TestRigBuilder::with_local_jobs_only() — forces job tools to use the
  local ContextManager-only path, preventing background worker loops from
  consuming replay-LLM trace steps meant for the foreground turn
- Wire with_local_jobs_only() into 6 e2e_builtin_tool_coverage tests
  that use a replaying backend (avoids flaky trace-step exhaustion)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… debug! SIGHUP

Blocker: routed_jobs_filter_tools_by_originating_channel passed none_arc()
(None) to WorkerDeps while the actual routing config lived only in the DB.
tool_definitions_for_current_context early-returns the full tool list when
the Arc contains None, so Archon_search was never filtered and the assert
would fail (or the test was never reached in CI).

Fix: call ChannelRoutingConfig::reload_from_store after save_to_store to
populate the Arc before handing it to the Worker. Test now passes.

Also: downgrade SIGHUP "config reloaded" log from info! to debug! — per
CLAUDE.md, info! in non-user-facing background paths corrupts the TUI.

Add: TODO comment on the libsql-only test acknowledging the missing
postgres-parity coverage per .claude/rules/database.md.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dd missing AgentDeps field

LLM types were extracted to ironclaw_llm crate in upstream (PR #3387).
Update all crate::llm::ToolDefinition references in our routing code:
- agent/channel_routing.rs: use ironclaw_llm::ToolDefinition
- agent/dispatcher.rs: apply_channel_routing signature
- worker/job.rs: tool_definitions_for_current_context return type

Also: add channel_routing: ChannelRoutingConfig::none_arc() to the
AgentDeps initializer in commands.rs test helper (new required field).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clippy (--deny warnings in CI) flags nested if let blocks that can be
collapsed with if-let chains. Merge the outer two into a single guard.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… tool

register_sync adds tools to builtin_tool_names. is_tool_permitted's builtin
branch defaults to allowing builtins when no whitelist is configured for the
group — so the dispatch-time enforcement test was passing the tool through
instead of rejecting it.

MCP tools in production are registered via the async register() path, which
keeps them out of builtin_tool_names. Using register_sync in a test that
asserts MCP-tool rejection is therefore testing the wrong code path.

Fix: switch both register_sync calls in the test to registry.register().await,
which causes the tool to fall through to the else { false } arm in
is_tool_permitted and be correctly rejected.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Same background-worker trace-step race as the other 6 tests. The
scheduler spawns a worker on create_job which consumes the cancel_job
replay step before the main turn can use it.

CancelJobTool and ListJobsTool both operate on the ContextManager only,
so local_jobs_only is safe here: create_job still returns a valid job_id,
list_jobs finds the job, and cancel_job cancels it — all without a
background worker racing on the replay LLM.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… loop

The dispatch-time gate only fired for DispatchSource::Channel (the gateway
path). The agent's LLM-driven worker loop calls Tool::execute via
worker/job.rs::execute_tool_inner, bypassing ToolDispatcher::dispatch — so a
jailbroken LLM could run a routing-hidden tool by naming it directly. Close
that gap and deduplicate the routing logic across its three call sites.

- Add an execution-time is_tool_permitted check in execute_tool_inner (the
  choke point for both single and parallel tool paths), fed the job's
  notify_channel/notify_metadata so it agrees with the worker presentation
  filter, including DM bypass. Routine/heartbeat jobs (no channel) fall to the
  default group, fail-closed — matching the presentation filter, no regression.
- Extract permit_in_group() (per-tool allow/deny) and routing_group_for()
  (group resolution + DM bypass). filter_tool_defs, is_tool_permitted, and
  derive_routed_mcp_servers now route through them — one implementation each.
- is_tool_permitted gains a metadata arg: the dispatcher passes Value::Null
  (byte-identical to its prior DM_EXACT-only behavior), the worker passes the
  job's real metadata.
- Make extract_mcp_server's byte-index invariant explicit with
  debug_assert!(is_char_boundary) (NOT is_ascii — multibyte server names work).
- Document the dual-gate boundary (dispatch + worker) on ToolDispatcher.

Tests: is_tool_permitted_metadata_dm_bypass, routing_group_for_dm_and_fallback,
and routed_jobs_block_filtered_tool_at_execution_time (drives execute_tool, not
just the helper).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- permit_in_group: check builtin_names before MCP prefix extraction so an
  MCP server named "memory" cannot grant the "memory_search" built-in;
  deny-by-default when no builtin_whitelist entry exists (was allow-all)
- reload_from_store: retain last-known-good config on parse failure instead
  of silently writing None and disabling routing for the session
- dispatcher ChatDelegate preflight: add is_tool_permitted gate so the chat
  path (execute_tool_calls) enforces routing at execution time, matching the
  worker path — a jailbroken LLM cannot replay a hidden tool call
- derive_routed_mcp_servers: intersect explicit mcp_servers with the channel's
  routing-allowed set; create_job callers can no longer widen MCP access past
  the channel allowlist by naming servers explicitly
- worker planning: use tool_definitions_for_current_context() instead of the
  unfiltered tool_definitions() so the plan is aware of routing restrictions
- tests: update three assertions that relied on the old allow-by-default
  built-in behaviour; 30/30 channel_routing tests pass

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@zmanian zmanian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — round 3 (head 44c34524f, 2026-05-25)

What changed since my 2026-05-24 review

One substantive commit on top of the routing work: 44c34524f fix(routing): address round-3 review: close all 6 blocking/strong gaps. It touches channel_routing.rs, dispatcher.rs, builtin/job.rs, worker/job.rs. The disjoint-history problem from last time is gone — the branch now shares a merge-base with main (030cfeb0c, May 20). The commit message claims all six gaps closed; I verified each against the source. Five are genuinely fixed in the code; the security-critical one is partially fixed (right idea, wrong layer) and the privilege-escalation fix is untested.

Prior finding status

1. (BLOCKING) Execution gate gap on the chat path — PARTIAL.
A gate was added, but not at the single canonical executor I asked for. The new is_tool_permitted check lives in ChatDelegate::execute_tool_calls' preflight loop (src/agent/dispatcher.rs:987-1005): filtered tools get PreflightOutcome::Rejected + continue before being pushed to runnable, so Phase-2 execution is correctly skipped, and it passes self.message.metadata so the DM bypass works. For the legacy v1 chat turn, the gap is closed. But the fix was placed in the delegate, not in execute_tool_with_safety, which I'd specifically recommended because it's the one executor every path crosses. As a result, three other channel-reachable execution paths still call execute_tool_with_safety / execute_chat_tool_standalone with no is_tool_permitted gate:

  • src/agent/thread_ops.rs:1815 — process_approval (approval-resume). The ChatDelegate gate fires before approval queuing, so a filtered tool can't normally reach here — but a config hot-reload that narrows policy between queue and resume, or a directly-submitted ExecApproval JSON, resumes execution with no re-check.
  • src/agent/scheduler.rs:582 — execute_tool_task (subtask/ToolExec). Arguably system/operator context, lower risk.
  • src/bridge/effect_adapter.rs:1612 — the entire engine v2 path. Engine v2 has zero channel-routing references (git grep channel_routing src/bridge/ → only test-helper none_arc() in router.rs): no presentation filter, no execution gate. On any deployment running engine v2, channel routing is silently a no-op. This wasn't called out in my 2026-05-24 review and may be a deliberate v1-only scope — but the permit_in_group doc comment's "what the LLM can see is exactly what it can run" guarantee is false on v2, and per "Everything Goes Through Tools" it deserves at minimum an explicit documented boundary, ideally the same gate. Please state the intended scope (v1-only?) in the PR body and on with_channel_routing, or extend the gate.

2. (BLOCKING) create_job MCP escalation — RESOLVED (code), UNTESTED.
derive_routed_mcp_servers (src/tools/builtin/job.rs:305-360) no longer returns caller-supplied mcp_servers verbatim. Explicit servers are now intersected with the resolved group's allowlist (explicit.into_iter().filter(|s| allowed_set.contains(...))), DM/no-config returns explicit unchanged, absent-channel falls back to the default group. The escalation-by-delegation path is closed. However there is no test for the intersection — job.rs' test module has no derive_routed_mcp_servers case. Per "Test Through the Caller, Not Just the Helper," a security-boundary predicate with a wrapper computing its inputs needs a caller-level regression test. Please add one (restricted channel + explicit out-of-group server ⇒ filtered out).

3. (strong) Built-ins allow-by-default — RESOLVED.
permit_in_group (channel_routing.rs:397-431): a group with no builtin_whitelist entry now returns None => false (deny). Unknown-group branch gates built-ins by the default group's whitelist via is_some_and and blocks everything else. Three unit tests updated to assert deny (test_filter_denies_builtins_when_no_whitelist, plus the full-scenario and is_tool_permitted assertions). Fail-open on absent config is unchanged-by-design (Arc None ⇒ no filtering), but invalid-config-after-hot-reload now retains last-known-good (reload_from_store, channel_routing.rs:129-135: if new_routing.is_none() && guard.is_some() { return false }) instead of silently disabling routing. Good.

4. (should) Classification order + worker planning — RESOLVED.
Built-in-name check now runs before MCP prefix extraction (channel_routing.rs:400-417), so an MCP server named memory can no longer authorize built-in memory_search via the MCP allowlist. Worker planning now seeds reason_ctx.available_tools from tool_definitions_for_current_context() (filtered) instead of tool_definitions() (worker/job.rs:403).

5. Rebase — PARTIAL.
The disjoint-history blocker is resolved (merge-base 030cfeb0c now exists). But the branch was rebased onto a May-20 main and main has moved since; GitHub reports mergeStateStatus: DIRTY / mergeable: CONFLICTING and git merge-tree shows conflicts. Needs another rebase onto current main before merge.

New findings (from the round-3 commit)

  • (should) No caller-level test for either execution gate. The new ChatDelegate gate (dispatcher.rs:992) and the worker gate (worker/job.rs:614) have only the is_tool_permitted unit test and the dispatch()-path integration test (dispatch.rs:957) behind them — and dispatch() is a different executor than the one the chat/worker turns use. The exact "wrapper between helper and side effect, tested only at the helper" shape the testing rule names. Add a test that drives execute_tool_calls/the worker preflight with a routing config and asserts a filtered tool is never executed.
  • (nit) info! on a relay auto-deny path at dispatcher.rs:995 ("Auto-denying approval-requiring tool…") — pre-existing, not from this commit, but it's on a channel turn path and info! corrupts the TUI per CLAUDE.md. Worth a debug! downgrade while nearby.

Test note

channel_routing.rs unit coverage is strong (deny-by-default, unknown-group, multibyte prefix, reload-detects-changes, DM bypass, libsql roundtrip). The two gaps both sit at the caller layer: (a) the chat/worker execution gates, (b) the create_job MCP intersection. Both are security boundaries with computed inputs — both need a caller-level regression test.

Recommendation

Comment, leaning request-changes. Findings #3 and #4 are cleanly resolved; #2's logic is correct; #5's disjoint history is fixed. The two things keeping this from approve:

  1. The execution gate is per-executor, not at the single canonical point — process_approval, scheduler subtasks, and (notably) the whole engine v2 path remain ungated. Either fold the check into execute_tool_with_safety so every path inherits it, or explicitly document the v1-only scope on with_channel_routing and in the PR body and confirm engine v2 is intentionally out of scope.
  2. Add caller-level regression tests for the chat/worker gate and for the create_job MCP intersection.
  3. Rebase onto current main to clear the conflict.

Happy to pair on where the single gate lands given the v1/v2 split.

Re-reviewed on behalf of @zmanian.

…ersection test

- process_approval (thread_ops.rs): re-check is_tool_permitted before
  executing the resumed tool — guards against hot-reload policy narrowing
  between approval-queue and approval-resume, and against directly-submitted
  ExecApproval JSON that bypasses the ChatDelegate preflight gate
- dispatch.rs with_channel_routing doc: explicitly list the paths covered
  (dispatch, chat preflight, approval-resume, worker loop) and the two
  intentionally out-of-scope paths (engine v2 / EffectBridgeAdapter, and
  Scheduler::execute_tool_task operator subtasks)
- dispatcher.rs: downgrade info! → debug! on the relay auto-deny log line
  (was on a per-channel-turn hot path; info! corrupts TUI per CLAUDE.md)
- builtin/job.rs: add caller-level regression test for derive_routed_mcp_servers
  — asserts that explicit mcp_servers are intersected with the channel group
  allowlist (Kiro filtered from research-only group), fallback to group set
  when no explicit list, empty result when all explicit servers are out-of-group

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nick-stebbings

Copy link
Copy Markdown
Contributor Author

Addressed all round-4 findings. Pushed 62855e48e.

Execution gate — approval-resume path

Added is_tool_permitted re-check in Agent::process_approval (thread_ops.rs) immediately before execute_chat_tool. This guards the two specific vectors you named:

  • Hot-reload policy narrowing: a config reload between approval-queue and approval-resume now causes a clean denial instead of executing a freshly-restricted tool.
  • Direct ExecApproval JSON: a crafted client submitting ExecApproval directly hits process_approval without going through the ChatDelegate preflight; the re-check closes that path.

The denial returns a SubmissionResult::ok_with_message (visible to user) rather than an error, matching the existing pattern for stale/duplicate approvals on this path.

Execution gate — scope documentation

Added explicit scope note to ToolDispatcher::with_channel_routing listing the four covered paths and the two intentionally out-of-scope paths:

  • Out of scope: engine v2 (EffectBridgeAdapter::execute_action) — has its own event-sourced audit trail; channel routing integration is a v2 follow-up.
  • Out of scope: scheduler subtasks (Scheduler::execute_tool_task) — operator-initiated subtasks run outside the user's channel context.

create_job MCP intersection — regression test

Added test_derive_routed_mcp_servers_intersects_with_channel_allowlist to tools::builtin::job::tests. Tests three cases against a research group that allows [Archon, Serpstat]:

  1. Explicit [Serpstat, Kiro] → [Serpstat] (Kiro filtered)
  2. Explicit [Kiro] → [] (all out-of-group)
  3. No explicit list → [Archon, Serpstat] (routing-derived set)

Nit

info! → debug! on the relay auto-deny log line in dispatcher.rs (was on a per-channel-turn hot path; info! corrupts TUI per CLAUDE.md).

CI

  • cargo test --lib tools::builtin::job::tests::test_derive_routed: 1 passed
  • cargo test channel_routing: 30/30
  • cargo clippy --all --all-features: 0 warnings
  • cargo fmt --check: clean

kuchikikiki 0.9.2 was yanked from crates.io; cargo deny fails with
[yanked] error. Downgrade to 0.9.1 (latest non-yanked compatible).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nick-stebbings
nick-stebbings requested a review from zmanian May 25, 2026 21:56

@zmanian zmanian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — round 4 (head 85f2b1c0f, 2026-05-25T21:51)

What changed since my 2026-05-25T19:47 review

Two commits on top of 44c34524f:

  • 62855e48e fix(routing): close approval-resume gate, document scope, add MCP intersection test — the substantive one, directly targeting my three open items.
  • 85f2b1c0f chore(deps): kuchikikiki 0.9.2 → 0.9.1 — unblocks cargo-deny (0.9.2 was yanked). No routing impact.

The disjoint-history / rebase conflict is gone: mergeable: MERGEABLE. mergeStateStatus: BLOCKED is review-gating, not a conflict.


Open-item status

1. (BLOCKING) Execution gate per-executor, not canonical — RESOLVED via documented scope + approval-resume gate.
The author chose the "document the boundary + gate the reachable paths" route rather than folding into a single execute_tool_with_safety, which is a legitimate resolution of what I asked for. Verified:

  • Approval-resume now gated. src/agent/thread_ops.rs:1644-1666 (process_approval) re-checks is_tool_permitted(&message.channel, &message.metadata, &pending.tool_name, &builtin_names) before execute_chat_tool, returning a user-facing "no longer permitted" message on deny. This closes the hot-reload-narrowing and directly-submitted-ExecApproval holes I flagged. Placement is correct (after status emit, before execution) and it passes real message.metadata so DM bypass still works.
  • Scope explicitly documented. src/tools/dispatch.rs:104-118 (with_channel_routing doc) now enumerates the four covered paths (dispatch, chat preflight, approval-resume, worker loop) and the two intentionally-out-of-scope paths: engine v2 (EffectBridgeAdapter::execute_action, deferred to a v2 follow-up with its own event-sourced audit trail) and Scheduler::execute_tool_task (operator subtasks outside user channel context). This is the explicit boundary I requested; the permit_in_group "what the LLM sees is what it can run" guarantee now has a stated v1-agent-loop scope. Acceptable — engine v2 routing is a fair follow-up, not a silent no-op anymore.

2. (should) create_job MCP intersection UNTESTED — RESOLVED.
src/tools/builtin/job.rs:2729-2766 adds test_derive_routed_mcp_servers_intersects_with_channel_allowlist, a caller-level test driving CreateJobTool::derive_routed_mcp_servers (not just the helper). It covers the three cases: explicit [Serpstat, Kiro] on a research-group channel → [Serpstat] (Kiro filtered); explicit [Kiro] only → [] (not the full group — the key escalation case); None → group-derived [Archon, Serpstat]. Matches the live signature and the routing_group_for resolution path. This is exactly the regression coverage I asked for.

3. Rebase — RESOLVED. mergeable: MERGEABLE; the yanked-dep cargo-deny failure that would have blocked is fixed by 85f2b1c0f.


New finding (introduced by this round)

  • (BLOCKING) CI red: the new MCP-intersection test trips clippy -D warnings. src/tools/builtin/job.rs:2746-2747 uses the let mut ctx = JobContext::default(); ctx.metadata = … field-reassign pattern, which clippy flags as clippy::field_reassign_with_default (denied via -D warnings). Both Clippy (all-features) and Code Style (fmt + clippy) jobs are FAILURE on 85f2b1c0f for this exact line. Ironically it's the test that resolved item #2. Fix: construct with the field set, e.g.
    let ctx = JobContext {
        metadata: serde_json::json!({ "notify_channel": "research-channel" }),
        ..Default::default()
    };
    All other checks (Tests all-features, Heavy Integration, fmt, no-panics, cargo-deny, E2E) are green.

Recommendation

Comment, very close to approve. All three of my prior open items (canonical-gate scope, MCP-intersection test, rebase) are genuinely resolved — the approval-resume gate and the documented v1/v2 boundary are the right calls, and the intersection test is precisely the caller-level coverage the testing rule wants. The only thing standing between this and approve is the self-inflicted clippy field_reassign_with_default failure in the new test (job.rs:2746). Push the struct-literal fix above and re-run; once CI is green I expect to approve with no further blockers.

Re-reviewed on behalf of @zmanian.

…ign_with_default

CI runs clippy with -D warnings; field-then-reassign is denied.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nick-stebbings
nick-stebbings requested a review from zmanian May 25, 2026 22:35

@zmanian zmanian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at head a8642771d (one commit since my last pass): the struct-initializer fix for the field_reassign_with_default clippy failure I flagged is in, and both "Clippy (all-features)" and "Code Style (fmt + clippy)" are now green; PR is MERGEABLE.

That clears the last open item. Status of everything from my review rounds:

  • Execution gate on reachable channel paths (chat preflight + approval-resume) — resolved; scheduler/engine-v2 explicitly documented out-of-scope (v2 follow-up).
  • create_job mcp-server intersection — resolved + caller-level test.
  • Built-ins deny-by-default, classification order, worker-planning filter, last-known-good on bad hot-reload — resolved.
  • Rebase onto main — resolved.
  • clippy — now green.

No remaining blocking items from my side. Recommend approve (leaving the formal approval click to a maintainer). Nice iteration closing these out.

Re-reviewed on behalf of @zmanian.

@zmanian zmanian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. All findings from the review rounds are resolved and CI is green at a8642771d (both Clippy checks pass, MERGEABLE):

  • Execution gate enforced on reachable channel paths (chat preflight + approval-resume); scheduler/engine-v2 explicitly documented out-of-scope (v2 follow-up).
  • create_job mcp-server intersection closed + caller-level test.
  • Built-ins deny-by-default, built-in-vs-MCP classification order, worker-planning filter, last-known-good on invalid hot-reload.
  • Rebased onto current main; clippy field_reassign_with_default fixed.

Thanks for working through all six original items plus the follow-up rounds.

Approved on behalf of @zmanian.

@zmanian

zmanian commented May 25, 2026

Copy link
Copy Markdown
Collaborator

@ilblackdragon — your 2026-03-20 CHANGES_REQUESTED is now the only thing holding this PR's review state at CHANGES_REQUESTED (it's otherwise MERGEABLE with CI green at a8642771d, and I've approved it on behalf of @zmanian). Since that review the PR has been through ~8 weeks of revisions and several review rounds — the correctness items from across reviewers are resolved:

  • Execution gate now enforced on the reachable channel paths (chat preflight + approval-resume); scheduler/engine-v2 explicitly documented out-of-scope (v2 follow-up).
  • create_job MCP-server escalation closed + caller-level test.
  • Built-ins now deny-by-default per group; built-in-vs-MCP classification order fixed; worker-planning tools filtered; last-known-good retained on invalid hot-reload.
  • Rebased onto current main; clippy/fmt green.

Your original architectural point — routing config in the DB rather than ~/.ironclaw/channel-routing.json — I don't believe was changed (it's still file-loaded). Could you take another pass and either dismiss the stale review, or restate which of your concerns you still consider blocking vs. acceptable-as-is (e.g. is the file-based config OK for now with DB as a follow-up)? Just want to unblock the merge state on what's a long-evolved PR. Thanks!

@nick-stebbings

Copy link
Copy Markdown
Contributor Author

@ilblackdragon Are you still wanting to integrate this feature?
If so I am happy to rebase once more.

@serrrfirat

Copy link
Copy Markdown
Collaborator

Thank you for the work and investigation in this PR. We are closing it as part of the transition from the legacy IronClaw runtime to the Reborn architecture.

This JSON router filters v1 tool lists by channel name. Reborn capability visibility is constructed from authenticated actor scope, extension ownership, product surface, authorization policy, and the filtered capability catalog; channel strings are not sufficient authority.

Per-product restrictions can be added as typed capability-surface policy using verified ProductAdapter/install/conversation context, with tests proving shared-channel users cannot see or invoke private capabilities.

We would be glad to review a focused Reborn contribution along those lines. Please start from the current crate contracts and production composition rather than rebasing the legacy implementation mechanically. Thank you again for contributing and for helping expose the underlying product need.

@serrrfirat serrrfirat closed this Jul 12, 2026
@serrrfirat

Copy link
Copy Markdown
Collaborator

Thank you for the work in this PR. We are closing it as part of the transition from the legacy IronClaw runtime to Reborn.

This filters v1 tools by channel strings. Reborn visibility derives from verified actor/install/conversation scope, extension ownership, product surface, and authorization policy.

Add per-product restrictions as typed capability-surface policy, proving shared-channel users cannot see or invoke private capabilities.

We would be glad to review a focused Reborn contribution built on the current crate contracts and production composition. Thank you again for contributing and for identifying the underlying need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: experienced 6-19 merged PRs ironclaw v1 risk: medium Business logic, config, or moderate-risk modules scope: agent Agent core (agent loop, router, scheduler) scope: ci CI/CD workflows scope: dependencies Dependency updates scope: docs Documentation scope: tool/builtin Built-in tools scope: tool Tool infrastructure scope: worker Container worker size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants