Repository navigation
P4b1: agent turn contract with cancellable turns - #1301
Merged
Merged
Conversation
Contributor
Scoreboard
This change makes keystroke work as a share of drawing the whole first frame 0.7% higher (worse) (1 metric is new and not compared).
master: 48bb721 on Elixir |
Owner
Author
Adversarial reviewReviewer: a sub-agent with three personas (Saboteur, New Hire, Security Auditor), reviewing head
The false comment at Still open:
Verification at
|
The test depended on the turn reaching its approver inside the 1 s
Comm.call timeout. With the session suspended, a zero timeout expires
before anything can answer, so the session always sees the prompt and
then its cancel: the turn is answered {:error, :cancelled}, the agent
drops it, and its approver is never asked. Killing a live turn on cancel
stays covered by the explicit-cancel tests.
Also drop a dead send to an already-killed asker and say why the setup
starts UserPreferences and Raxol.DynamicSupervisor.
A Comm.call timeout cancels the turn, so the 5 s default would kill
any prompt that runs longer. Comm.call/2 now refuses an
{:acp_prompt, text, approver} with {:error, :timeout_required} without
sending it; other messages keep the 5 s default (owner decision
2026-10-25).
Comm.send(id, {:call, pid, ref, {:acp_prompt, ...}}, from: ...) reached
update/2 untracked, so the agent handed its turn function to the sender:
no caller monitor, no cancel, no prompt validation. The session now
starts a turn for that form too, keeping the metadata (from, causation)
on the dispatch.
Both take the pid in the prompt's caller position, which is always the session standing in for the real caller; calling it caller misled.
A standalone session whose lifecycle went down only cleared
lifecycle_pid: its turns were never answered and their processes ran
on. It now answers each turn {:error, :agent_down} and aborts its
process. A turn whose process crashes or is killed from outside is
answered {:error, :turn_failed} and reaches update/2 as
{:acp_cancel, ref}, as a cancel does, so the agent drops what it tracks
for the turn. Turn and lifecycle failures are logged by shape
(exception module or leading atom), never by content.
The reference agent applied any {:counter_change, ...} it received, so
any process could change the count without an approved turn. It now
records the session for each change turn it starts and applies a change
only for a ref it still tracks, forgetting it once applied, denied, or
cancelled or failed ({:acp_cancel, ref}). The change turn also monitors
the agent, so it ends {:error, :agent_down} instead of waiting forever
for a count from a dead agent. Counter names the prompt's session pid
session, like Comm.run_turn/3.
Comm moduledoc, AGENT_FRAMEWORK.md and the CHANGELOG entry: the
required prompt timeout, metadata-form prompts, {:acp_cancel, ref} on
any turn that ends without its reply, {:error, :agent_down}, Counter's
tracked changes, and that the reply shape is not checked at the
boundary.
A turn whose work raised let its worker crash, and the emulator's crash report logged the exception message (observed: token=sk-live-123 in 20 of 20 raises). The worker now catches the failure and reports only its shape (exception module, or exit tag) to the owner, which logs that and fails the turn as before.
{:acp_turn_failed, ref} was taken from any sender, and fail_turn
assumed the turn had a process: a forged report crashed the session for
a turn not yet run (demonitor of nil), and on a live hold answered the
caller while the worker ran on, orphaned. The owner now reports
{:acp_turn_failed, ref, self()}, the session accepts it only from the
turn's own process and aborts it before answering, and fail_turn skips
the demonitor when there is no process.
Re-review LOWs in the same place: a call-form cancel sent with
Comm.send(..., from:) is now handled like the bare form, and a prompt
envelope whose caller is not a pid or whose ref is not a reference is
dropped in the session (logged by shape) instead of reaching update/2
and the logs with its text.
A hold the approver denied stayed in the model, so holds piled up and release counted them (released 5 after five denied holds). The denied hold turn now tells the agent, which drops it. The moduledoc no longer claims no other process can change the count: a live turn's ref is the only credential, and whoever knows it can still send that turn's change.
The agent now sees a ref the session makes for each turn, never the
caller's, so a process that knows the caller's ref (enough to cancel)
can no longer answer the turn, start its work or message its process.
The turn's owner reports its reply as {:acp_turn_reply, ref, owner,
reply}, checked against the owner pid. An inline reply while the
turn's process runs kills that process first and forwards
{:acp_cancel, ref} to the agent.
Also documents the reserved message shapes the session intercepts and
that a cancel does not roll back a change already sent to the agent.
No code handles the two-element {:acp_turn_failed, ref} any more, and
the two ensure_running/1 clauses did the same thing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / why
P4b1 of the OMP pivot (ADR-0042): the agent-side turn contract that
raxol acp --agent(P4b2) will drive.Comm.call(agent, {:acp_prompt, text, approver}, timeout)asks a session for one turn; the agent answers{:ok, text, usage}(so the call returns{:ok, {:ok, text, usage}};Comm.callstill wraps replies as before).approveris an arity-3:tool_authorizer(Raxol.Agent.ToolPolicyshape). The reference agent passes it toToolConverter.dispatch_tool_call/3, so the turn's tool calls must pass it (P4b2 plugsPermission.authorizer/2in here).Raxol.Agent.Sessionstands in as the prompt's caller and tracks in-flight turns. The agent sees a ref the session makes for each turn, never the caller's: the caller's ref cancels a turn but cannot answer it, start its work or message its process. The agent answers inline withComm.reply/3, or with the newComm.run_turn/3, which runs the work in a session-owned turn process (an owner that traps exits plus a linked worker; the worker also dies with the session).{:acp_cancel, ref}kills that process (not just a dropped reply), answers the prompt{:error, :cancelled}, then forwards the cancel toupdate/2. Sent viaComm.call, it replies:okonly after the worker is dead ({:error, :not_found}when no turn has that ref). Late replies from a cancelled turn are dropped, so each prompt gets exactly one answer.Comm.call/4takesref:so a caller knows the ref to cancel (owner chose option A).Comm.send_turn/3messages a running turn.Raxol.Agent.Reference.Counter(lib):increment,get, sensitivereset, andhold, which blocks until areleaseprompt or a cancel. Integrator decision: P4b1 introduces the Counter with only what this contract needs. P4a adds the plainComm.callsurface, the "journal continues, state fresh" moduledoc and:mcp_spawnable_apps.Plan refs: TODO bullet "P4b1 agent turn contract"; review-3 R5 :1793, R4 P4b row :1524 (deterministic
hold); review-2 H4 (approver installed as the turn's:tool_authorizer).Sizing: lib +839/-28 at
29d40cd55(git diff --shortstat 0c739138b 29d40cd55 -- packages/raxol_agent/lib), about 4.2x the +200 row and past the lane rule's 1.5x stop-and-report line (+776/-25, 3.9x, ate5b7f1387; +671/-25, 3.4x, atcb91e970c). 178 of those lines are the Counter, which the integrator moved in from P4a; without it +661. Most of the growth since the first push is review fixes (abnormal turn ends, owner checks, the agent ref, the review-4 monitor index) and their comments. The PR was kept whole as the integrator's call, pending the owner's decision on splitting it.Adversarial review 2 (2026-10-25, three personas)
Fixes are new commits on this branch (
ad3c8f8f1..999e9b648). RED lines were recorded againstad3c8f8f1with the new tests or copies of the persona probes.lifecycle_pid; turns never answered, workers ran on,turnsleaked, Counterchange_turnwaited forever{:error, :agent_down}and its process aborted; the Counter change turn monitors the agent ({:error, :agent_down}if it dies)d720dcb30,7daf666abTask.await(held)timed out (** (EXIT) time out); probe:after lifecycle kill: worker alive true, turns 1,infinity caller after agent death (2s): nil; turns 1; asker alive true. GREEN:held caller result: {:ok, {:error, :agent_down}}; worker alive ...: false,infinity caller ...: {:ok, {:ok, {:error, :agent_down}}}; turns 0; asker alive falseComm.calltimeout cancels promptsComm.call/2refuses an{:acp_prompt, ...}with{:error, :timeout_required}without sending it; other messages keep 5 s. Every in-tree caller migrated (only this suite'sprompt/3helper used the default)fabb89be0left: {:error, :timeout_required}/right: {:error, :timeout}:sys.suspended, so a zero timeout always expires before anything answers; on resume the session sees the prompt, then its cancel. The test asserts the stray{:error, :cancelled}, the turn gone, the agent's hold dropped and the approver never asked. A timeout that fires only once the worker is live cannot be forced without a test hook inComm.call(the cancel comes from the caller's own timer); killing a live turn on cancel stays covered by the explicit-cancel tests08b40cc1d1_000to1):no matching message after 5000ms ... {:authorizing, Counter.Hold, worker}. GREEN:--repeat-until-failure 20: 21 x17 passedtoken=sk-live-123)d720dcb30,999e9b648failed: {%RuntimeError{message: "token=sk-live-123"}, ...}; crash-report probe:lines with the secret: 20(of 20 raises). GREEN:failed: RuntimeError,lines with the secret: 0; the log test--repeat-until-failure 100: 101 x pass. The core Dispatcher debug log of prompts is not in this PR's diff and is left aloneupdate/2update/2as{:acp_cancel, ref}, as a cancel does; documented in the contractd720dcb30,998f7b3deright: {:ok, {:ok, "released 1", ...}}; GREEN:released 0{:command_result, {:counter_change, ...}}applied{:acp_cancel, ref})7daf666abafter forged change, get -> {:ok, {:ok, "count: 0", ...}}; GREEN:count: 1Comm.send(..., from:)) bypassed turn tracking68dba7739no matching message after 5000ms ... {:authorizing, Counter.Hold, worker}(probe:caller received: {:acp_run, ...}, cancel{:error, :not_found}); GREEN: probecancel of that ref: {:ok, :ok}run_turn/3/send_turn/3takesession; the deadsend(asker, :approve)is deleted; a setup comment says why UserPreferences andRaxol.DynamicSupervisorrun; the moduledoc and AGENT_FRAMEWORK.md say the reply shape is not checked at the boundaryfa2f4c050,08b40cc1d,998f7b3derun_turnregisters is droppedagent_bridge.exis not in this PR's diff; left to the integratorRe-review (999e9b6, CONCERNS)
Fixed in
999e9b648..cb91e970c. Before results are the new tests run against999e9b648's lib.{:acp_turn_failed, ref}accepted from any sender;fail_turnassumed a turn process{:acp_turn_failed, ref, self()}, the session accepts it only when the report names the turn's owner pid (a claim, not a sender check: like the ref, the pid is not a secret; review 3 adds the agent ref) and aborts that process before answering;fail_turnguards the demonitor (if turn.mref) and documents that the process is down firstd6087dee0right: {:error, :agent_down}(session crashed,ArgumentErrorfromdemonitor); live hold,right: {:ok, {:ok, "released 0", ...}}(caller answered, hold dropped). After: session alive, worker alive,released 1, thenreleasedComm.send(..., from:)not interceptedd6087dee0no matching message after 5000ms ... {:agent_reply, ^call_ref, :ok}. After::ok, worker:killed, prompt{:error, :cancelled}update/2and the logs with its textd6087dee0hold token=sk-live-777. After:dropped a prompt ..., no secret in the logefc5942cereleased 3after 3 denied holds. After:released 0(smoke:holds: MapSet.new([])after 5)efc5942ce,cb91e970cAdversarial review 3 (2026-10-25, three personas)
Fixed in
cb91e970c..e5b7f1387. RED lines are the new or changed tests run againstcb91e970c's lib (git stashofpackages/raxol_agent/lib):Result: 21/25 passed. One commit followed before review 4:1920524e9adds the Session moduledoc paragraph on the agent ref's in-node visibility (it travels in the Dispatcher's:turn_startedevent and the journal; the node is the trust boundary).{:agent_reply, ref, _}answered the caller and orphaned the turn's worker (not cancellable, not released); a forged{:acp_run, ref, fun}sent before the agent's own ran attacker work;{:acp_turn_message, ...}was relayed from anyoneagent_refsmaps it to the caller's ref), so reply,acp_run, turn message and turn-failed messages are only accepted for that ref; a caller's ref, enough to cancel, matches nothing. The owner's reply is now{:acp_turn_reply, ref, owner, reply}, taken only when it namesturn.pid. An inlineComm.replywhile the turn's process runs aborts it and forwards{:acp_cancel, agent_ref}. Mechanism differs from the suggested pid-claim sender check: an inline reply is plainComm.reply/3({:agent_reply, ref, r}, the same wire everyComm.callcaller receives), so a sender claim there would change that public shape. Residual: the agent ref is in the Dispatcher's durable:turn_startedevent (it carries the prompt), so in-node EmitBus/journal readers can still learn itbebf3424cleft: {:ok, :ok}/right: {:ok, {:error, :not_found}}(cancel after the forged reply); forgedacp_runno matching message after 5000ms ... {:authorizing, Counter.Hold, _worker}; inline reply while runningno matching message after 5000ms ... {:DOWN, ^worker_mon, :process, ^worker, :killed}; forged turn-failed naming the real ownerassert Process.alive?(worker)failed. GREEN:25 passed,--repeat-until-failure 20: 21 x25 passed{:error, :cancelled}does not roll back a change the turn already sent to the agentbebf3424cbebf3424c{:acp_turn_failed, ref}sends; duplicateensure_running/1clausesensure_running/1is one clausebebf3424c,e5b7f1387TODO.md:981-984{:acp_cancel, ref}and the call-form cancel, and says an unmatched{:acp_cancel, x}is swallowedbebf3424clib/raxol/core/runtime/events/dispatcher.ex:482,494):turn_startedresidual above remainsbebf3424cAdversarial review 4 (2026-10-26, pre-merge)
Verdict CLEAN; the three LOWs are fixed in
1920524e9..29d40cd55. RED lines are the new test or a copy of the persona probe run against1920524e9.{:DOWN, _, :process, lifecycle_pid, _}(any ref) answered every turn{:error, :agent_down}, killed their workers and nulledlifecycle_pidfor good (next prompt:no_dispatcher); only the public lifecycle pid was neededlifecycle_mref, set only when it owns and monitors the lifecycle) and matches the agent-down clause on it (when is_reference(mref), so a forgednilref on a tree-owned session matches nothing either). New test: a forged DOWN with another ref, and one withnil, on a supervised and on a standalone session leave the held turn running, thenreleaseanswersreleased 1f5d42c598Expected truthy, got false/code: assert Process.alive?(worker)(turn_contract_test.exs:289). Probe after:turn worker alive after forged DOWN? true; lifecycle really alive? true,lifecycle_pid now: #PID<0.672.0>; turn still tracked? true, caller{:ok, {:ok, "count: 1", ...}}. The real agent-down test (standalone, lifecycle killed) still answers{:error, :agent_down}Enum.findover all turns per DOWN;PromptTurn.abort/2anddemonitor(..., [:flush])each scanned a mailbox full of DOWNs)monitorsindexes each turn's caller monitor and owner monitor to its ref, so a DOWN finds its turn in O(1);abort/2waits on a monitor made just before its receive (the receive skips older messages); turn monitors are removed without:flush, and a DOWN whose monitor left the index falls to the catch-all. Behaviour unchanged: existing suite, no timing assert51698fb0a/tmp/saboteur4-turns-scale.exscopy, Counterholdturns, all callers killed), same machine: beforen=1000 65 ms, n=4000 478 ms, n=16000 9380 ms; after39 ms, 128 ms, 484 ms;agent holds=0 agent_refs=0each. Index only (no abort/flush change) measured57, 364, 3346 ms. Mixed-load probe after:session turns=0 agent_refs=0 monitor_index=0 monitors=0 msgq=0,PromptTurn procs alive=0acp_cancel{:acp_cancel, ref}is not the bare:acp_cancelatom thatRaxol.AgentClientProtocol.Sessionsends to aRaxol.Agent.ClientProtocol.TurnRunnerprocess, and that the session neither sends nor intercepts that atom29d40cd55e5b7f13871920524e9, sizing and evidence are at29d40cd55MIX_ENV=testgets{:error, :registry_not_running}fromstart_sessioniex -S mix)Evidence
Refreshed at
999e9b648(review-2 fixes); review-4 checks at29d40cd55, review-3 checks ate5b7f1387and re-review checks atcb91e970clisted first. All commands were run inpackages/raxol_agentunless noted.29d40cd55:mix format --check-formattedclean;MIX_ENV=test mix compile --warnings-as-errorsandmix compile --warnings-as-errors(dev) exit 0, rootMIX_ENV=test mix compile --warnings-as-errorsexit 0;mix credo --stricton touched files only the oldstop_emit_bridgenesting;turn_contract_test.exs --repeat-until-failure 20: 21 x26 passed; turn_contract + comm + session + session_supervisor + turn:Result: 71 passed;mix test:Result: 3028 passed (19 properties, 3009 tests), 53 excluded; prose lint exit 0;scripts/check-quality-ratchet.sh(root):IMPROVED credo: 1169 < baseline 1182 (-13),IMPROVED dialyzer: 106 < baseline 118 (-12), exit 0.f5d42c598and51698fb0aeach compile and passturn_contract_test.exs(26 passed; 45 withsession_test.exs). No rootlib/change, so nomix raxol.checke5b7f1387:mix format --check-formattedclean;MIX_ENV=test mix compile --warnings-as-errors --forceclean (package) and rootMIX_ENV=test mix compile --warnings-as-errorsexit 0;mix credo --stricton touched files only the oldstop_emit_bridgenesting;turn_contract_test.exs --repeat-until-failure 20: 21 x25 passed;mix test:Result: 3027 passed (19 properties, 3008 tests), 53 excluded; prose lint exit 0;scripts/check-quality-ratchet.sh(root):IMPROVED credo: 1169 < baseline 1182 (-13),IMPROVED dialyzer: 106 < baseline 118 (-12), exit 0. No rootlib/change, so nomix raxol.checkcb91e970c:mix format --check-formattedclean;mix compile --warnings-as-errorsclean;mix credo --stricton touched files only the oldstop_emit_bridgenesting;turn_contract_test.exs --repeat-until-failure 20: 21 x22 passed;mix test:Result: 3024 passed (19 properties, 3005 tests), 53 excluded; prose lint exit 0;d6087dee0alone compiles and passes its 21 turn testsmix compile --warnings-as-errors(MIX_ENV=test): clean; rootMIX_ENV=test mix compile --warnings-as-errors:Generated raxol appmix format --check-formatted: cleanmix credo --stricton the changed files: onlysession.exstop_emit_bridgenesting, which was already there atad3c8f8f1(unchanged code)scripts/check-quality-ratchet.sh --gate credo(root):IMPROVED credo: 1169 < baseline 1182 (-13)scripts/check-quality-ratchet.sh --gate dialyzer(root):IMPROVED dialyzer: 106 < baseline 118 (-12)mix test:Result: 3019 passed (19 properties, 3000 tests), 53 excluded. Two earlier full runs each had unrelated failures (StorageInvariantsTestI8,Journal.ChainTestdamage,PermissionWireTestreject; none touch the turn code); those three files then passed 6 x--repeat-until-failure, and the final run above is greenmix test test/raxol/agent/turn_contract_test.exs --repeat-until-failure 20: 21 x17 passed; each intermediate commit compiles with--warnings-as-errorsand passes the suite as it stands at that commitgit ls-files -z '*.md' | xargs -0 elixir scripts/prose_lint.exs): exit 0New tests (
test/raxol/agent/turn_contract_test.exs, Counter started viaSession.Supervisor.start_session/2, unique ids, tmp journal dir):{:ok, "count: N", usage}.deny_allanddeny_sensitiverefusereset(count unchanged),allow_allgrants it.Comm.call(id, {:acp_cancel, ref})returns:ok. The worker (found through the approver) goes DOWN:killed, the prompt gets{:error, :cancelled}, a second cancel gets:not_found, and the next prompt succeeds.releaseprompt: the hold repliesreleasedand the worker exits:normal.Review-2 tests (same file): a prompt without a timeout is refused (
:timeout_required); the caller-timeout cancel, made deterministic with:sys.suspend; a failed turn logs the reason's shape, not its content; a turn killed from outside answers:turn_failedand the agent drops it; a standalone session answers its held turn{:error, :agent_down}and kills the worker when its lifecycle is killed; a forged change no live turn sent is dropped; a prompt sent withfrom:is a session-owned, cancellable turn.Smoke (throwaway script,
mix run --no-start, real Registry/DynSup,start_session/2):Review-2 smoke (throwaway script,
MIX_ENV=test mix run --no-start, real Registry/DynSups,start_session/2plus a standaloneSession.start_link; debug/info lines filtered):Re-review smoke (
MIX_ENV=test mix run --no-start, same setup):Review-3 smoke (
MIX_ENV=test mix run --no-start /tmp/fix1301r3-smoke.exs, real Registry/DynSup,start_session/2; forgeries use the caller's ref):Review-4 smoke (dev,
mix run, real Registry/DynSup,start_session/2; scripts are copies of the persona probes). Manual-testing steps 1-3 (/tmp/newhire4-counter.exscopy):Forged agent-down (
/tmp/security4-turn_probe_test.exscopy, run as a package test; the held turn is approved after the checks):Scale and mixed load (
/tmp/saboteur4-turns-scale.exs,/tmp/saboteur4-turns.exscopies):Model spend: none.
Manual testing
Run these in dev:
iex -S mixinpackages/raxol_agent. UnderMIX_ENV=testnothing startsRaxol.Agent.Registry(the suites start their own), sostart_sessionreturns{:error, :registry_not_running}.Session.Supervisor.start_session(Raxol.Agent.Reference.Counter, id: "c1", session_id: "c1")and check thatComm.call("c1", {:acp_prompt, "increment", ToolPolicy.allow_all()}, 5_000)returns{:ok, {:ok, "count: 1", _}}, and that the same call without a timeout returns{:error, :timeout_required}holdfrom a Task withComm.call/4ref: ref, thenComm.call("c1", {:acp_cancel, ref}): the Task gets{:error, :cancelled}and the next prompt answersresetwithToolPolicy.deny_sensitive(): the reply says denied andgetshows the count unchangedSession.start_link(app_module: Raxol.Agent.Reference.Counter, id: "s2", session_id: "s2"), hold a turn from a Task with:infinity, kill the lifecycle (Registry.lookup(Raxol.Agent.Registry, {:lifecycle, "s2"})): the Task gets{:ok, {:error, :agent_down}}and the hold's worker is dead{:error, :turn_failed}andreleaseanswersreleased 0"token=..."logsfailed: RuntimeErrorand no line with the tokenComm.send("c1", {:call, self(), ref, {:acp_prompt, "hold", approver}}, from: "editor")runs the hold in a session-owned worker (no{:acp_run, ...}reaches the sender) and{:acp_cancel, ref}kills itref), send{:agent_reply, ref, {:ok, "forged", %{}}},{:acp_turn_message, ref, :release}and{:acp_turn_failed, ref, owner}({:links, [owner]} = Process.info(worker, :links)): the Task is not answered, the hold keeps running, andComm.call("c1", {:acp_cancel, ref})still answers:okand the Task{:error, :cancelled}Comm.sendaholdprompt withref, send the session{:acp_run, ref, fn -> ... end}, resume: the forged function never runs and the agent's own hold doesholdprompts withToolPolicy.deny_all(:x), thenrelease:released 0{:DOWN, make_ref(), :process, lifecycle, :forged}([{lifecycle, _}] = Registry.lookup(Raxol.Agent.Registry, {:lifecycle, "c1"})): the hold keeps running andreleaseanswersreleased 1