neuron-technologies/neuron
2
0
Fork 0
Code Issues 2 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

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
will.anderson commented 2026-06-15 18:04:47 +00:00
Owner
Copy Link
Copy Source

Unifies the two separate suspension systems so tool approval works for all modern agentic_loop sessions.

Unifies the two separate suspension systems so tool approval works for all modern agentic_loop sessions.
will.anderson added 1 commit 2026-06-15 18:04:58 +00:00
fix(sessions): unify dual suspension systems in handle_session_approve
Neuron Soul CI / build (pull_request) Failing after 11m26s
Details
7c7dc310a0 
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.
will.anderson commented 2026-06-17 17:48:41 +00:00
Author
Owner
Copy Link
Copy Source

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

The refactor's goal is correct and the overall structure is cleaner. Removing the inline agentic loop from handle_session_approve and routing through agentic_resume is the right call. The legacy-path bridge-blob synthesis is a reasonable migration strategy. However there are two blockers and a few warnings.

Blocker 1 — Modern path dispatches tool without validating tool_name

approve_tool_name comes from json_get(body, "tool_name"), which returns "" if the client omits the field. The code reaches dispatch_tool(approve_tool_name, eff_input) without checking. dispatch_tool("") returns "unknown tool: " silently — no error, just a garbage string injected as the tool result into the resumed conversation.

Add a guard before the dispatch block:

if str_eq(approve_tool_name, "") {
    return "{\"error\":\"tool_name is required\"}"
}

Blocker 2 — Modern path calls dispatch_tool for MCP tools that the client should execute

agentic_loop suspends specifically when !is_builtin_tool(tool_name) — i.e. for mcp__* tools — so the client can execute them. When the client posts to /approve, it should be providing the already-executed result via a content field (same contract as /tool_result). Instead, the new code calls dispatch_tool(approve_tool_name, eff_input) server-side, re-running the tool and ignoring whatever the client did. This makes the approval flow semantically identical to /tool_result except that it executes the tool twice (or not at all, if the client already ran it). The fix is to accept json_get(body, "content") as the tool result for MCP tools, identical to handle_tool_result.

Warning — always action is silently broken

state_set("always_allow_" + session_id, ...) is written correctly, but nothing in agentic_loop reads it. The old inline loop that checked this list was the code deleted in this PR. tool_auto_approved() in chat.el is defined but never called. A user choosing "Always allow" gets this call approved but every subsequent call to the same tool will still suspend for approval.

Either wire agentic_loop to check always_allow_<session_id> in its bridge-decision logic, or document "always" as not yet implemented and return a clear error.

Warning — Legacy path hardcodes agentic_tools_literal()

The original session may have started with agentic_tools_with_web() or agentic_tools_all(). Resuming with agentic_tools_literal() means the LLM sees a different tool schema than when it made the tool call — Anthropic's API requires tool_use IDs to match tools actually present in the tools array. Consider storing a tools_variant field in the pending_tool_* blob, or storing the full tools_json literal (already done in the bridge blob format).

Nit — Double truncation

sessions.el truncates dispatch_tool output to 6000 chars before passing to agentic_resume, which truncates again. Pass the raw result to agentic_resume and let its truncation path handle it once.

The happy path for the modern path (builtin tool, allow, tool_name present) should work. The blocker issues surface when clients omit tool_name or when the suspended tool is an MCP connector (which is the most common case for the approval flow).

## Review: fix(sessions): unify dual suspension systems, wire approve to agentic_resume The refactor's goal is correct and the overall structure is cleaner. Removing the inline agentic loop from `handle_session_approve` and routing through `agentic_resume` is the right call. The legacy-path bridge-blob synthesis is a reasonable migration strategy. However there are two blockers and a few warnings. ### Blocker 1 — Modern path dispatches tool without validating `tool_name` `approve_tool_name` comes from `json_get(body, "tool_name")`, which returns `""` if the client omits the field. The code reaches `dispatch_tool(approve_tool_name, eff_input)` without checking. `dispatch_tool("")` returns `"unknown tool: "` silently — no error, just a garbage string injected as the tool result into the resumed conversation. Add a guard before the dispatch block: ```el if str_eq(approve_tool_name, "") { return "{\"error\":\"tool_name is required\"}" } ``` ### Blocker 2 — Modern path calls `dispatch_tool` for MCP tools that the client should execute `agentic_loop` suspends specifically when `!is_builtin_tool(tool_name)` — i.e. for `mcp__*` tools — so the *client* can execute them. When the client posts to `/approve`, it should be providing the already-executed result via a `content` field (same contract as `/tool_result`). Instead, the new code calls `dispatch_tool(approve_tool_name, eff_input)` server-side, re-running the tool and ignoring whatever the client did. This makes the approval flow semantically identical to `/tool_result` except that it executes the tool twice (or not at all, if the client already ran it). The fix is to accept `json_get(body, "content")` as the tool result for MCP tools, identical to `handle_tool_result`. ### Warning — `always` action is silently broken `state_set("always_allow_" + session_id, ...)` is written correctly, but nothing in `agentic_loop` reads it. The old inline loop that checked this list was the code deleted in this PR. `tool_auto_approved()` in `chat.el` is defined but never called. A user choosing "Always allow" gets this call approved but every subsequent call to the same tool will still suspend for approval. Either wire `agentic_loop` to check `always_allow_<session_id>` in its bridge-decision logic, or document "always" as not yet implemented and return a clear error. ### Warning — Legacy path hardcodes `agentic_tools_literal()` The original session may have started with `agentic_tools_with_web()` or `agentic_tools_all()`. Resuming with `agentic_tools_literal()` means the LLM sees a different tool schema than when it made the tool call — Anthropic's API requires `tool_use` IDs to match tools actually present in the `tools` array. Consider storing a `tools_variant` field in the `pending_tool_*` blob, or storing the full `tools_json` literal (already done in the bridge blob format). ### Nit — Double truncation `sessions.el` truncates `dispatch_tool` output to 6000 chars before passing to `agentic_resume`, which truncates again. Pass the raw result to `agentic_resume` and let its truncation path handle it once. --- The happy path for the modern path (builtin tool, allow, tool_name present) should work. The blocker issues surface when clients omit `tool_name` or when the suspended tool is an MCP connector (which is the most common case for the approval flow).
will.anderson added 1 commit 2026-06-17 17:59:05 +00:00
fix(sessions): resolve blockers and warnings in handle_session_approve
Neuron Soul CI / build (pull_request) Failing after 9m3s
Details
91902d6bf2 
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)
will.anderson merged commit fc74bd2a4b into main 2026-06-17 18:06:06 +00:00
will.anderson deleted branch fix/agentic-tool-approval-unification 2026-06-17 18:06:11 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
tim.lingo (Tim Lingo) will.anderson (Will Anderson)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: neuron-technologies/neuron#18
Delete Branch "fix/agentic-tool-approval-unification"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?