fix(agent): extract and wire the pre-action verifier at the live tool seam (#14031)

[mrveiss]

Part of #14031.

Thinking Path

#14031 lists two extractions: the pre-action verifier and belief-state
tracking. This PR does only the first — PreActionVerifier — because its
pre_action_verifier_enabled defaults to True (agent_loop/types.py:266),
so today it reads as an active guard while executing on no request (AgentLoop
has no production caller). Belief-state's flag defaults False, so it is off
and unreachable — a smaller, separable problem left for a follow-up.

Template: #13590 (closed). Extract the decision surface into autobot_shared/,
leave the original module as a re-export (the fact_forcing precedent), call
it from the live seam in chat_workflow/tool_handler.py.

Unlike repetition_guard/fact_forcing_guard, the verifier's job is an LLM
call — it cannot be made fully pure. What moved to autobot_shared/ and IS
pure/unit-tested: action-class threshold resolution, response parsing, and
verdict determination (determine_verdict/panel_decision — the surface that
preserves the HARD_BLOCK semantics exactly). The LLM call orchestration
(PreActionVerifier.verify) moved alongside it so agent_loop/pre_action_verifier.py
can stay a thin re-export, same as fact_forcing.

What Changed

• New: autobot_shared/pre_action_verifier_guard.py — the full decision
  surface + PreActionVerifier class, dependency-free of agent_loop (only
  lazy-imports agent_loop.guard_profile/agent_loop.types inside
  pre_action_verifier_enabled(), mirroring repetition_guard.max_identical_tool_calls()).
• Changed: agent_loop/pre_action_verifier.py is now a re-export. Scalar
  constants (HARD_BLOCK, THRESHOLD_*, ...) are proxied through module
  __getattr__ rather than statically imported — agent_loop/loop.py
  re-imports HARD_BLOCK fresh on every call inside _check_approvals, and a
  static import would have frozen a stale copy, invisible to anything patching
  the source module (this broke one existing test during verification; fixed
  by proxying instead of copying).
• Changed: chat_workflow/tool_handler.py — new _enforce_pre_action_verifier
  enforcer, called from _dispatch_tool_call between fact-forcing and the
  work-item approval hold (matching where AgentLoop._check_approvals ran the
  verifier: before the approval step). Scoped to autobot_shared.tool_catalogue.SENSITIVE_TOOLS
  — the same set AgentLoop._sensitive_tool_name gated on — and gated on
  pre_action_verifier_enabled() (guard-profile resolved, defaults True). A
  BLOCK verdict with VERIFIER_HARD_BLOCK=1 hard-blocks the call, exactly
  preserving the original semantics; without it, the call is held
  pending_approval with the verifier's rationale attached, reusing the shape
  _enforce_work_item_approval already uses at this seam. A broken/unavailable
  verifier fails open (unchanged from the original).
• New tests: autobot_shared/pre_action_verifier_guard_test.py (pure
  decision-surface unit tests) and
  autobot-backend/tests/unit/chat_workflow/test_tool_handler_pre_action_verifier.py
  (seam wiring, including the required wiring assertion).

Seam insertion point

chat_workflow/tool_handler.py::_dispatch_tool_call, right after
_enforce_fact_forcing and before _enforce_work_item_approval. Traced the
enforcer chain end to end first: forbidden_work → config_protection →
fact_forcing → verifier (new) → work_item_approval → repetition, each an
early-return gate before the tool actually dispatches. This position mirrors
AgentLoop._check_approvals, where the verifier ran immediately before the
human-approval request — so a HARD_BLOCK short-circuits before any approval
plumbing, and a soft block's rationale is available to attach to the
subsequent hold.

What the now-live guard will newly block

Nothing was gated on PreActionVerifier in production before this PR — the
guard executed on no request. With this PR, pre_action_verifier_enabled
being True by default means every call to a SENSITIVE_TOOLS tool (31 names:
write_file, delete_file, bash/execute_command/shell, git_push,
git_force_push, deploy/kubectl/terraform, http_post, ...) now gets an
independent adversarial LLM pass before it executes. VERIFIER_HARD_BLOCK
defaults to 0, so by default a BLOCK verdict does not reject the call —
it holds it pending_approval with the rationale attached, which is new
behaviour for tools that previously had no approval gate at this seam at all
(e.g. write_file, git_push — only execute_command had its own approval
flow before).

Running the pure decision logic (determine_verdict + threshold_for_tool)
against representative refutation probabilities:

write_file       prob=0.10 threshold=0.50 -> PASS   (well-grounded, prior read confirmed path)
write_file       prob=0.60 threshold=0.50 -> BLOCK  (plausible but unconfirmed target)
delete_file      prob=0.70 threshold=0.50 -> BLOCK  (delete without a prior existence check)
execute_command  prob=0.20 threshold=0.50 -> PASS   (ls -la, read-only, low risk)
execute_command  prob=0.55 threshold=0.50 -> BLOCK  (rm -rf on an unconfirmed path)
git_push         prob=0.30 threshold=0.50 -> PASS   (push after tests referenced this turn)
git_force_push   prob=0.65 threshold=0.50 -> BLOCK  (force-push with no stated reason)
kubectl          prob=0.50 threshold=0.50 -> BLOCK  (kubectl apply, borderline)
http_post        prob=0.55 threshold=0.60 -> PASS   (below the 0.6 network threshold)
http_post        prob=0.65 threshold=0.60 -> BLOCK  (above the 0.6 network threshold)

Finding, not just a detail: execute_command is in SENSITIVE_TOOLS and
is almost certainly the highest-volume sensitive tool in production. It
already has its own approval flow (_handle_pending_approval, session-scoped
pending state). This PR adds the verifier's LLM round trip (up to
VERIFIER_TIMEOUT_S=30s, fails open on timeout/no-provider) in front of that
existing flow for every shell command, not just risky ones — matching the
original AgentLoop scope exactly (it verified every sensitive tool, not a
narrower "risky" subset), but that scope was never exercised before because
AgentLoop never ran in production. This is a real latency/cost tradeoff, not
a bug: I did not narrow the guard to work around it. If per-environment
latency turns out to be a problem, the options are (a) tune
VERIFIER_TIMEOUT_S/thresholds, (b) exempt read-only-flavoured execute_command
invocations via a cheap pre-check, or (c) accept it as the cost of the guard
finally running. That's a product call, not mine to make unilaterally, so I'm
flagging it here and on #14031 rather than quietly loosening the gate.

Verification

./venv/bin/python -m pytest autobot_shared/pre_action_verifier_guard_test.py \
  autobot-backend/tests/unit/chat_workflow/test_tool_handler_pre_action_verifier.py \
  autobot-backend/agent_loop/tests/test_pre_action_verifier.py -q
# 51 passed

./venv/bin/python -m pytest autobot-backend/agent_loop/ autobot-backend/chat_workflow/ -q
# 621 passed

./venv/bin/python -m pytest autobot_shared/ -q
# 1738 passed, 3 failed — pre-existing #14035 (editable-install layout), reproduced
# identically on main tree HEAD, unrelated to this change

./venv/bin/python -m pytest autobot-backend/tests/orchestration/test_agent_registry_fail_closed.py -q
# 27 passed (the suite a prior sweep-scope mistake broke — checked per the type
# of change, not just the touched directories)

Wiring-assertion mutation test: removed the
await self._enforce_pre_action_verifier(...) call site from
_dispatch_tool_call by direct file edit (not git stash), confirmed via
grep that the call site was gone, re-ran
test_the_guard_is_reached_from_the_dispatch_seam — it failed
(AssertionError: assert '_enforce_pre_action_verifier(' in '...'). Restored
the file from the pre-mutation copy, confirmed the call site was back via
grep, and reran the full targeted suite green (51 passed).

ruff check clean on every touched/added file.

Left for the follow-up (explicitly out of scope here)

• Extracting BeliefState/BeliefStateUpdater into autobot_shared/ and
  wiring it into the tool seam where results are already collected.
• Deciding belief_state_enabled's default deliberately once it is reachable.

Both tracked on #14031, which stays open until the belief-state slice lands.

Model Used

Sonnet 5

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review response — the wiring guard now guards wiring, and CI is root-caused

Pushed as 8c3ead6c1.

The wiring assertion was a presence check, not a reachability check

source = inspect.getsource(ToolHandlerMixin._dispatch_tool_call)
assert "_enforce_pre_action_verifier(" in source

That stays green if the call sits after an early return, behind a never-true condition, or is otherwise made dead while the text remains in the file. Since the whole point of #14031 is that a guard read as present while executing on no request, a presence assertion reproduces the failure mode instead of preventing it. The original mutation only tested deletion — the one case this style does catch, and the weakest.

Replaced with test_the_live_dispatch_seam_reaches_and_awaits_the_verifier, which drives the real _dispatch_tool_call end to end through the actual enforcer chain, patches PreActionVerifier.verify, and asserts it was awaited with the tool name, the args, and the task_id the seam should have passed. The BLOCK verdict short-circuits dispatch, so the single yielded approval_required message also proves the call was reached mid-seam rather than bypassed.

The source-text test is kept, relabelled as a rename guard, with a docstring saying explicitly that it is not relied on for wiring proof.

Mutation: dead rather than absent

MUTANT applied (file differs)
call-site text still in file: 1
FAILED ...::test_the_live_dispatch_seam_reaches_and_awaits_the_verifier
1 failed, 7 passed
restored -> 8 passed

The call-site text is still present and the rename guard is among the 7 that pass — only the behavioural test fails. That is the proof that was missing.

CI failures — root-caused, both mine

python-suite shard 10/12 — test_governed_execution.py::test_dispatch_holds_approval_gated_tool died with AttributeError: 'types.SimpleNamespace' object has no attribute 'session_id'. The new enforcer read ctx.session_id directly, and several existing seam tests use a ctx double without it. session_id is only an opaque trajectory identifier here, so it now reads through getattr(ctx, "session_id", None) — matching this file's own convention for optional ctx attributes (getattr(ctx, "requires_approval_before", None) at :3290, getattr(ctx, "work_item_id", None) at :3296).

This is the "sweep by the type, not the directory" lesson again: I swept agent_loop/ and chat_workflow/ but the affected test lives under tests/unit/chat_workflow/. Full sweep now: 957 passed across agent_loop/, chat_workflow/ and tests/unit/chat_workflow/.

code-quality — black wanted tool_handler.py reformatted (two log calls I had hand-wrapped). Fixed; black --check and isort --check-only both clean across autobot-backend/ and autobot_shared/.

Decision — verifier scope, recorded rather than decided silently

Wiring this in means every call to a SENSITIVE_TOOLS tool now makes an adversarial LLM call before dispatch, and that set includes execute_command — almost certainly the highest-volume tool on the list. That adds a round-trip and a token cost to every shell invocation, on the main model, inheriting its rate limits (#13593).

Shipping the full canonical scope. Reasons, in order:

1. Narrowing would mean introducing a second tool set beside autobot_shared.tool_catalogue.SENSITIVE_TOOLS. This codebase keeps having to un-fork exactly that shape, and the guard currently reuses the canonical set — the same one AgentLoop._sensitive_tool_name gated on. A parallel "initially verified" list buys latency at the cost of the SSOT.
2. The scope is not new behaviour I invented; it is the original design finally running.
3. It is reversible: pre_action_verifier_enabled resolves through the guard profile, so it can be turned off without a code change.
4. A broken or unavailable verifier fails open (SKIP/PASS) and never blocks the loop, so the risk is latency and tokens, not availability.

What should be measured before leaving it on: p50/p95 added latency on execute_command dispatch, and verifier token spend as a fraction of a session. If either is unacceptable, the lever is the guard profile, not a second tool list.

Labelling #14031 needs-decision so this is the owner's call to revisit rather than something buried in a PR body.

Left for the follow-up slice

BeliefState / BeliefStateUpdater extraction, and deciding belief_state_enabled's default now that it would be reachable. #14031 stays open for that — this PR says Part of, not Closes.

[mrveiss]

code-quality root-caused — a real type defect in the extracted guard, not a lint nit

autobot_shared/pre_action_verifier_guard.py:484: error: "BaseException" object is not iterable  [misc]
Found 1 error in 1 file (checked 165 source files)

autobot_shared/ is strictly typed, and mypy was pointing at something real. asyncio.gather(..., return_exceptions=True) returns list[T | BaseException], and the loop narrowed with isinstance(r, Exception) — which does not cover BaseExceptions that are not Exceptions. asyncio.CancelledError is exactly that. So a cancelled panellist fell through to prob, rat, p, m = r and would have raised a confusing unpack error at runtime.

Fixed by handling the two cases differently, because they mean different things:

• an Exception from one panellist is a degraded panel, not a failed one — panel_decision works on however many probabilities arrived, so it is skipped as before
• a BaseException that is not an Exception propagates. Swallowing a cancellation would keep this coroutine running after its caller gave up, which is the classic way a cancelled request turns into a leaked LLM call

Local mypy is 2.3.0 against CI's 1.16.0, which is why CI reported one error where the newer version reports eight. The other seven are pre-existing — verified by running the same command on the main tree at base:

worktree (this branch) : Found 8 errors in 4 files (checked 165 source files)
main tree (base)       : Found 8 errors in 4 files (checked 164 source files)
guard-file errors      : 0

Same eight, in rate_limiter.py, leader_lease.py and two others this PR does not touch. Nothing new introduced.

black and flake8 clean on the file; 392 passed across pre_action_verifier_guard_test.py, agent_loop/tests/test_pre_action_verifier.py and tests/unit/chat_workflow/.

Pushed as b7d037d70.