fix(sessions): unify dual suspension systems, wire approve to agentic_resume #18

Merged

will.anderson merged 2 commits from fix/agentic-tool-approval-unification into main 2026-06-17 18:06:06 +00:00

Conversation 1 Commits 2 Files Changed 3

+331 -148

2 Commits

Author	SHA1	Message	Date
will.anderson	91902d6bf2	fix(sessions): resolve blockers and warnings in handle_session_approve ... BLOCKER 1 (sessions.el, modern path): Add guard that rejects allow action when tool_name is missing from the body. Previously, omitting tool_name caused dispatch_tool("", ...) to return "unknown tool: " and silently inject a corrupted tool_result into the conversation. BLOCKER 2 (sessions.el, modern path): Stop re-executing client-side tools server-side. When the client provides body["content"], use it directly as the tool result (matching the handle_tool_result contract). Only fall back to dispatch_tool for builtin tools when no content is present. Non-builtin tools with no client content now return a clear error instead of a broken dispatch attempt. WARNING 1 (chat.el, agentic_loop): Wire always_allow_<session_id> state into the bridge-suspension decision. When a tool is in the session's always-allow list, treat it as locally dispatchable (like a builtin) and skip the bridge pause, so the approval UI is never shown again for that tool in that session. WARNING 2 (sessions.el, legacy path): Read a "tools_variant" field from the legacy pending blob when present, and call the corresponding agentic_tools_*() variant on resume. Falls back to agentic_tools_literal() for blobs written before this field existed. tests/test_sessions_approve.el: Add 10-case test suite covering: - empty session_id / missing call_id / missing action guards - no pending tool returns correct error - missing tool_name on allow returns error (BLOCKER 1) - deny action does not require tool_name - legacy call_id mismatch returns mismatch error - always action records tool_name in always_allow state - allow with client content skips re-execution (BLOCKER 2)	2026-06-17 12:58:44 -05:00
will.anderson	7c7dc310a0	fix(sessions): unify dual suspension systems in handle_session_approve ... The approve endpoint was permanently broken for all sessions going through the modern agentic_loop path. agentic_loop suspends via bridge_save() into mcp_bridge:<session_id>, but handle_session_approve was reading from pending_tool_<session_id> — a different key — so it always returned "no pending tool for session". Replace the body of handle_session_approve with a two-path design: Modern path: check mcp_bridge:<session_id> first. If the blob is there, dispatch_tool() on allow (or build the denial string), then delegate to agentic_resume() which re-enters agentic_loop from the exact suspension point. This is the path all live sessions take. Legacy path: if only pending_tool_<session_id> exists (in-flight session from before this deploy), synthesise a bridge blob from the stored messages_so_far and route through agentic_resume() as well. The stale inline agentic loop (90 lines, agentic_tools_literal only, no MCP connector support, no bridge suspension) is removed entirely. routes.el already calls handle_session_approve correctly — no change needed.	2026-06-15 13:03:15 -05:00