Skip to content

fix(acp): replace global approval callback with task-scoped registry#9157

Open
Kewe63 wants to merge 2 commits intoNousResearch:mainfrom
Kewe63:fix/acp-approval-callback-race-condition
Open

fix(acp): replace global approval callback with task-scoped registry#9157
Kewe63 wants to merge 2 commits intoNousResearch:mainfrom
Kewe63:fix/acp-approval-callback-race-condition

Conversation

@Kewe63
Copy link
Copy Markdown
Contributor

@Kewe63 Kewe63 commented Apr 13, 2026

Problem

terminal_tool.py exposed a single process-wide _approval_callback variable.
server.py's prompt() method set this global via set_approval_callback() before
running each agent, then restored the previous value afterward.

With up to 4 concurrent ACP sessions sharing the same ThreadPoolExecutor, the
following race was possible:

  1. Session A registers its callback → _approval_callback = cb_A
  2. Session B starts, overwrites it → _approval_callback = cb_B
  3. Session A's terminal tool fires a dangerous-command approval dialog
  4. It reads _approval_callback and gets cb_B — session B's permission dialog
    handles session A's command silently

This means one session could silently grant or deny permissions on behalf of
another, breaking the security boundary between concurrent users.

Root Cause

_approval_callback in terminal_tool.py is a module-level global. Any session
that calls set_approval_callback() clobbers every other session's callback for
the duration of its run. The old "save and restore" pattern in _run_agent()'s
finally block was not atomic — another thread could overwrite between the save
and the restore.

Fix

tools/terminal_tool.py

  • Added a _task_approval_callbacks: dict registry protected by a
    threading.Lock.
  • Added three public helpers:
    • register_task_approval_callback(task_id, cb) — store a per-session callback
    • unregister_task_approval_callback(task_id) — remove it (called in finally)
    • get_task_approval_callback(task_id) — return the task's callback or fall back
      to the global _approval_callback (CLI sessions are unaffected)
  • Updated _check_all_guards(command, env_type, task_id="") to accept a
    task_id parameter and look up the correct callback via get_task_approval_callback.
  • Passed task_id=effective_task_id at the single _check_all_guards() call site
    inside terminal_tool().

acp_adapter/server.py

  • Replaced set_approval_callback(approval_cb) + save/restore with:
    • register_task_approval_callback(session_id, approval_cb) before _run_agent()
    • unregister_task_approval_callback(session_id) inside the finally block of
      _run_agent() — guaranteed cleanup even on exception
  • The global _approval_callback and set_approval_callback() are untouched; CLI
    sessions continue to work exactly as before.

Tests

Added tests/acp/test_approval_callback_race.py with 14 tests:

Class What it covers
TestTaskApprovalRegistry register, unregister, fallback, isolation, overwrite
TestApprovalCallbackConcurrency 10 threads × 100 reads — no cross-session leakage
TestServerUsesTaskScopedAPI AST checks: set_approval_callback not called; register/unregister called; unregister is inside a finally block
TestCheckAllGuardsTaskScoped _check_all_guards routes to task cb, falls back correctly

All 14 tests pass.

Backwards Compatibility

  • set_approval_callback() / _approval_callback unchanged — CLI and any other
    caller is unaffected.
  • No changes to the public tool schema or ACP protocol.

Kewe63 added 2 commits April 13, 2026 19:32
Concurrent ACP sessions shared a single process-wide `_approval_callback`
in `terminal_tool.py`. Every new `prompt()` call in `server.py` overwrote
this global with `set_approval_callback()`, so when two sessions ran in
parallel the last writer's callback would handle *all* approval dialogs —
meaning session A could silently receive session B's permission prompt.

Changes:
- Add `register_task_approval_callback(task_id, cb)`,
  `unregister_task_approval_callback(task_id)`, and
  `get_task_approval_callback(task_id)` to `terminal_tool.py`.
  The registry is a `dict` protected by a `threading.Lock`; the global
  `_approval_callback` / `set_approval_callback` are kept for CLI use.
- Update `_check_all_guards()` to accept `task_id` and look up the
  per-task callback via the new getter, falling back to the global.
- Pass `task_id=effective_task_id` at the single `_check_all_guards()`
  call site inside `terminal_tool()`.
- Rewrite the callback wiring in `server.py prompt()`:
  - `register_task_approval_callback(session_id, approval_cb)` before
    `_run_agent()` (instead of the old `set_approval_callback()`).
  - `unregister_task_approval_callback(session_id)` in the `finally`
    block of `_run_agent()` so cleanup is guaranteed even on exception.
- Add `tests/acp/test_approval_callback_race.py` with 14 tests covering
  the registry API, concurrent isolation, AST checks on `server.py`,
  and the `_check_all_guards` routing logic.
…level

The previous lazy initialisation pattern was not thread-safe:

    _task_approval_lock = None

    def _get_task_approval_lock():
        global _task_approval_lock
        if _task_approval_lock is None:          # two threads can pass here simultaneously
            _task_approval_lock = threading.Lock()  # each creates a separate Lock object
        return _task_approval_lock

Two threads racing through the None-check could each create a distinct
Lock instance and return different objects, making the mutex useless.

Fix: initialise _task_approval_lock directly at module load time so there
is exactly one Lock object from the start. Replace all _get_task_approval_lock()
call sites with direct _task_approval_lock usage. Remove the now-unnecessary
helper function entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant