fix(dashboard): read the agent tool policy off the event loop

[leonlaiyc]

Problem / Motivation

GET /api/session-tool-policy resolves the calling session's agent config on the gateway event loop. api_session_tool_policy does the whole filesystem transaction inline:

agent_path = kiro_agents_dir() / f"{agent_name}.json"
if not agent_path.is_file():                                    # stat
    return web.json_response({})

try:
    config = json.loads(agent_path.read_text(encoding="utf-8"))  # read + parse
except (OSError, json.JSONDecodeError):
    return web.json_response({})

A stat, a read and a JSON parse, none of them offloaded, on the single loop every other gateway request shares.

Why it matters

This is not a startup path. Managed MCP servers (kirocrew-core, kirocrew-cron) call this endpoint to filter their tool lists per agent — docs/architecture/mcp.md lists it as a gateway round-trip on the MCP path — so it runs on ordinary request traffic.

The directory it reads is a real user directory (<kiro home>/agents), not a cache, so its reads are exactly as slow as that filesystem is: a network home, a cold page cache, or a Windows indexer or AV holding the file each turn a "cheap" read into loop time that every other request waits behind. Nothing about the call site bounds that cost.

The repo already treats this as a defect class of its own (no-blocking-call-on-event-loop), with the same correction landed at other call sites — #4118, #3803, #4550.

What changed (motivation → approach → change)

Symptom — an MCP tool-list filter request performs blocking filesystem work on the gateway loop.

Root cause — the existence probe, the read and the parse are written inline in an async def, so they execute on whatever thread the coroutine is running on, which is the loop's.

Change — extract the three into one sync helper and invoke it through asyncio.to_thread:

policy = await asyncio.to_thread(_read_managed_tool_policy_sync, agent_path)
if policy is None:
    return web.json_response({})

The whole transaction moves, not just the read. Offloading read_text alone would leave the stat and the json.loads on the loop — the same defect in a smaller form, and a thread-identity test would then prove the fix incomplete rather than prove it correct.

None is deliberately distinct from {}. The helper answers None for every case that already returned an empty policy without an audit record — missing file, OSError, malformed JSON, non-dict policy — and a dict for a config that was read and understood. That split is load-bearing: a policy of {} because the key is absent is an agent whose config was parsed, which is what the SEL ok record attests, while an unreadable file is not. Folding the two together would start logging success for files that were never read. Two tests pin both halves.

Not moved: kiro_agents_dir() stays on the loop — it is kiro_home() / "agents", pure path arithmetic with no I/O, and moving it would be style rather than a fix. The path-traversal guard still runs before any filesystem work, exactly as before.

One production file; no route, contract, or response-shape change.

Tests

Test	Behavior locked in
test_the_agent_config_read_runs_off_the_event_loop	the thread performing the read is not the loop's
test_a_missing_agent_config_is_an_empty_policy	missing file → {}
test_an_unparseable_agent_config_is_an_empty_policy	malformed JSON → {}, not a raise
test_a_non_dict_policy_is_an_empty_policy	wrong-shaped policy → {}
test_an_agent_without_a_policy_key_is_reported_as_read	a parsed config with no policy key still logs SEL ok
test_an_unread_config_is_not_logged_as_ok	an unread config logs no success
test_a_traversing_agent_name_is_still_refused	the path-traversal guard precedes any filesystem work
test_a_missing_session_key_is_refused	deny-by-default on identity is unchanged

The proof is thread identity at the real filesystem seam — Path.read_text itself — not an assertion that asyncio.to_thread was called. A spy on the offload would keep passing if the call were later moved back inline behind another wrapper; the thread the read actually runs on cannot be faked. Same shape as the accepted proof in #4118.

Fail-before / pass-after on c940485dc, with only sessions.py reverted:

=== sessions.py = origin/main ===
FAILED test_the_agent_config_read_runs_off_the_event_loop
  AssertionError: the agent config was read on the event-loop thread: the stat,
  the read and the JSON parse all block every other request on that loop
1 failed, 7 passed

=== fix applied ===
8 passed

The seven behavioural tests pass on both sides, which is the point: they demonstrate that observable behaviour is preserved rather than merely re-stated.

test_session_tool_policy_off_loop.py + test_mcp_shared_cache.py + test_mcp_call_site_auth_coverage.py: 36 passed.

Gates: flake8 · isort · scripts/check_black_formatting.py · scripts/check_brand_name.py.

Manual verification

N/A — unit coverage sufficient: the change is entirely about which thread performs the filesystem work, and the test measures that directly on the production handler with only the seam instrumented. Observing it in a running gateway would mean timing loop stalls under a slow filesystem, which is precisely the non-deterministic evidence the thread-identity assertion replaces.

Related Issues

Fixes #4945.

Same defect class as #4118, #3803 and #4550.

Checklist

• Single commit with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A: docs/architecture/mcp.md documents this endpoint's contract (it returns the session's managedToolPolicy.exclude), which is unchanged; only the thread the read runs on differs
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 520290395d0c6bdc5ff672bcf4db779264f85d71 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5202903

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 520290395d0c6bdc5ff672bcf4db779264f85d71 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix matching the repo's established to_thread pattern for this exact defect class, with behavior preserved and pinned at the real seam.

[DESIGN-REVIEWED] 5202903

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 520290395d0c6bdc5ff672bcf4db779264f85d71 via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate file's discovery pass found no candidates. I've verified the diff independently: this is a behavior-preserving refactor that moves the stat/read/parse off the event loop into asyncio.to_thread. The None-vs-{} distinction correctly preserves the SEL ok logging semantics, asyncio (line 5) and Any imports are present, and all branch outcomes map identically to the base. No grounded defect exists in the changed lines.

No findings.

[OPUS-REVIEWED] 5202903

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 520290395d0c6bdc5ff672bcf4db779264f85d71 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Reading complete — I reviewed the patch, the intent file, and the base tree (the handler at sessions.py:1675-1687, the repo's existing agent-config readers, and the sibling blocking reads in dashboard/handlers/). Here is the review in the contract's required shape:

First-Principles-Verdict: CONCERNS

The fix is real and derived, but its new bare reader is a fourth spelling of a job agent_discovery._read_agent_spec already does hardened.

What this change ships

Intent: stop GET /api/session-tool-policy from doing blocking filesystem work on the gateway event loop — a FIX.

1. The agent-config stat/read/parse now runs on a worker thread, not the loop — justified (documented no-blocking-call-on-event-loop defect class, e.g. server.py:236)
2. New sync helper _read_managed_tool_policy_sync — one consumer, but it is the offload unit, not generalized surface
3. Eight new tests pinning thread identity and preserved behavior — declared
4. No route, response-shape, or SEL-logging change — verified against the base handler

Watch

• Second spelling of an existing reader. The repo already has three private "parse agent JSON → dict|None" readers: agent_discovery._read_agent_spec:147 (self-described "the one reader for both scopes", with size cap, symlink-target and AppleDouble guards, justified because "the agents directories are user-writable"), apps/bridges.py:671 _read_agent_config, and mcp_gateway/session_servers.py:95. This helper is the fourth, and the bare-read_text one — no size cap on a user-writable dir. The extraction moment was the moment to converge on the hardened one.
• Point patch, siblings counted. Grepping read_text\( under src/kiro_crew/dashboard/handlers/ finds 40+ inline reads, many in async handlers on the same loop (memory.py:373, security.py:139, themes.py:98, knowledge.py:87, messaging.py:2402). The repo fixes this class site-by-site (fix(dashboard): offload eager spawn's agent-binding config load #4118, fix(autopilot): offload orchestrator config load #3803, fix(apps): keep the app-config writes off the event loop #4550), so this is accepted-and-deferred — but the description claims no sibling scope, and a human should know the class remains open.

Subtractions

• Replace the stat/read/parse core of _read_managed_tool_policy_sync with a call to agent_discovery._read_agent_spec (keep only the managedToolPolicy extraction), deleting the fourth reader and inheriting the size-cap and symlink guards the agents dir was given them for.

[FIRST-PRINCIPLES-REVIEWED] 5202903

[bolichen97]

Approved after a description-vs-diff consistency review: every claim in the PR description is backed by the diff, and the diff carries no material change the description leaves unmentioned.