From e751f8ba6c4961db9cde76d3d640e340228adf5b Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Thu, 17 Sep 2026 08:38:19 -0400 Subject: [PATCH 1/8] agy parity T7: implement Arbiter.Agents.Gemini.splice_prompt/2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gemini/agy was the one provider Worker.inject_resume_argv/4 and inject_nudge_argv/3 could not rewrite at all — no splice_prompt/2 meant every resume attempt and gate nudge fell straight to :unsupported_provider and parked, even though the underlying agy CLI supports `--conversation ` for session resume. splice_prompt/2 finds the "-p" prompt slot that default_argv/2 always produces (both the agy and upstream-gemini branches share that shape) and: - for a `["--resume", sid, prompt]` insert: on the agy branch, swaps in the new prompt and inserts `--conversation ` right after it, leaving --print-timeout/--model/--effort and everything else untouched; on the upstream-gemini branch (no --conversation support) returns {:error, :resume_unsupported} instead of a bogus invocation - for a `[nudge]` insert: swaps only the prompt on either branch - returns {:error, :no_print_slot} when there's no "-p" flag (fixtures) Worker.inject_resume_argv/4 and inject_nudge_argv/3 already dynamically dispatch to splice_prompt/2 via function_exported?/3, so no worker.ex change was needed there beyond refreshing a stale comment. Updated the respawn_provider_test.exs fixture test that asserted the old :unsupported_provider park behavior for gemini. Co-Authored-By: Claude Sonnet 5 --- apps/arbiter/lib/arbiter/agents/gemini.ex | 48 ++++++++++ apps/arbiter/lib/arbiter/worker.ex | 16 ++-- .../test/arbiter/agents/gemini_test.exs | 94 +++++++++++++++++++ .../arbiter/worker/respawn_provider_test.exs | 58 +++++------- .../test/arbiter/worker_resume_test.exs | 46 +++++++++ 5 files changed, 223 insertions(+), 39 deletions(-) diff --git a/apps/arbiter/lib/arbiter/agents/gemini.ex b/apps/arbiter/lib/arbiter/agents/gemini.ex index b7ebec24f..982f68a89 100644 --- a/apps/arbiter/lib/arbiter/agents/gemini.ex +++ b/apps/arbiter/lib/arbiter/agents/gemini.ex @@ -155,6 +155,54 @@ defmodule Arbiter.Agents.Gemini do defp tool_result_line?(_), do: false + # Splice `insert` (a nudge/resume prompt, see the two shapes below) into a + # stashed `default_argv/2` invocation. Both the `:agy` and `:gemini` + # branches build argv as `[…, exec, "-p", prompt, flags…]` (no `--` + # separator, no `--print` name — see `build_argv/5` above), so the prompt + # slot is always the element right after `"-p"`; only the resume + # (`--conversation`) translation differs between the two CLIs. + # + # `["--resume", session_id, prompt]` → the worker's resume insert. Only + # `agy` accepts a `--conversation ` flag (bd-b7e33c); the upstream + # `gemini` CLI has no session-resume mechanism at all, so that branch + # returns an explicit error instead of emitting an invocation `gemini` + # would reject or silently misinterpret. `--print-timeout`/`--model`/ + # `--effort` (and every other flag) are left exactly where they were — + # only the prompt is swapped and `--conversation ` is inserted right + # after it. + # + # `[nudge]` → a gate-nudge swap-in: only the prompt changes, on either + # branch. + # + # Returns `{:error, :no_print_slot}` when `argv` has no `"-p"` flag at all + # (test fixtures / custom commands). + @doc false + def splice_prompt(argv, insert) when is_list(argv) and is_list(insert) do + case Enum.find_index(argv, &(&1 == "-p")) do + nil -> + {:error, :no_print_slot} + + idx -> + {head, [_old_prompt | tail]} = Enum.split(argv, idx + 1) + exec = Enum.at(head, -2) + + case insert do + ["--resume", session_id, prompt] -> + if agy_executable?(exec) do + {:ok, head ++ [prompt, "--conversation", session_id] ++ tail} + else + {:error, :resume_unsupported} + end + + [nudge] -> + {:ok, head ++ [nudge] ++ tail} + end + end + end + + defp agy_executable?(exec) when is_binary(exec), do: Path.basename(exec) == "agy" + defp agy_executable?(_), do: false + @doc """ Which Gemini-family CLI this host will actually run, and where. diff --git a/apps/arbiter/lib/arbiter/worker.ex b/apps/arbiter/lib/arbiter/worker.ex index c115e587c..f4fca7fc6 100644 --- a/apps/arbiter/lib/arbiter/worker.ex +++ b/apps/arbiter/lib/arbiter/worker.ex @@ -3359,13 +3359,15 @@ defmodule Arbiter.Worker do adapter = agent_adapter_for_provider(provider) if Code.ensure_loaded?(adapter) and function_exported?(adapter, :splice_prompt, 2) do - # `splice_prompt/2` is not a callback on Arbiter.Agents.Agent (see its - # @optional_callbacks) — only Claude and Codex define it, Gemini does not. - # The `function_exported?/3` guard above is what makes the call safe; a - # static `adapter.splice_prompt(...)` makes the compiler resolve `adapter` - # to every adapter module and emit an "undefined or private" warning for - # Gemini, which `mix compile --warnings-as-errors` then fails on. The - # dynamic dispatch is the point, not an oversight. + # `splice_prompt/2` is not a callback on Arbiter.Agents.Agent — it's a + # `@doc false` convention each adapter opts into (Claude, Codex, Gemini + # as of bd-b7e33c all define it). The `function_exported?/3` guard above + # is what makes the call safe for any future adapter that doesn't; a + # static `adapter.splice_prompt(...)` makes the compiler resolve + # `adapter` to every known adapter module and would emit an "undefined + # or private" warning for the first one that omits it, which `mix + # compile --warnings-as-errors` then fails on. The dynamic dispatch is + # the point, not an oversight. # credo:disable-for-next-line Credo.Check.Refactor.Apply case apply(adapter, :splice_prompt, [argv, [nudge]]) do {:ok, new_argv} -> {:ok, %{port_args | argv: new_argv}} diff --git a/apps/arbiter/test/arbiter/agents/gemini_test.exs b/apps/arbiter/test/arbiter/agents/gemini_test.exs index 2ee083814..10710c31a 100644 --- a/apps/arbiter/test/arbiter/agents/gemini_test.exs +++ b/apps/arbiter/test/arbiter/agents/gemini_test.exs @@ -488,4 +488,98 @@ defmodule Arbiter.Agents.GeminiTest do assert {"GEMINI_THINKING_LEVEL", "high"} in env end end + + describe "splice_prompt/2 — resume (bd-b7e33c)" do + test "agy branch: translates --resume into --conversation and preserves --print-timeout/--model/--effort" do + argv = [ + "sh", + "-c", + ~s(exec "$@" < /dev/null), + "sh", + "/usr/local/bin/agy", + "-p", + "ORIGINAL TASK PROMPT", + "--dangerously-skip-permissions", + "--model", + "gemini-3.1-pro", + "--effort", + "high", + "--output-format", + "stream-json", + "--print-timeout", + "300s" + ] + + assert {:ok, out} = + Gemini.splice_prompt(argv, ["--resume", "sess-abc", "CONTINUE PROMPT"]) + + idx = Enum.find_index(out, &(&1 == "-p")) + assert Enum.slice(out, idx, 2) == ["-p", "CONTINUE PROMPT"] + refute "ORIGINAL TASK PROMPT" in out + + assert chunk_after(out, "--conversation") == "sess-abc" + assert chunk_after(out, "--model") == "gemini-3.1-pro" + assert chunk_after(out, "--effort") == "high" + assert chunk_after(out, "--print-timeout") == "300s" + assert "--dangerously-skip-permissions" in out + assert "--output-format" in out and "stream-json" in out + end + + test "upstream gemini branch: --resume is rejected with an explicit error, not a bogus invocation" do + argv = [ + "sh", + "-c", + ~s(exec "$@" < /dev/null), + "sh", + "/usr/local/bin/gemini", + "-p", + "ORIGINAL TASK PROMPT", + "--skip-trust", + "-y", + "--model", + "gemini-2.5-pro", + "--output-format", + "stream-json" + ] + + assert {:error, :resume_unsupported} = + Gemini.splice_prompt(argv, ["--resume", "sess-abc", "CONTINUE PROMPT"]) + end + + test "nudge: swaps only the prompt, leaving every flag (agy or upstream) untouched" do + argv = [ + "sh", + "-c", + ~s(exec "$@" < /dev/null), + "sh", + "/usr/local/bin/agy", + "-p", + "ORIGINAL TASK PROMPT", + "--model", + "gemini-3.1-pro", + "--output-format", + "stream-json" + ] + + assert {:ok, out} = Gemini.splice_prompt(argv, ["nudge prompt"]) + + idx = Enum.find_index(out, &(&1 == "-p")) + assert Enum.slice(out, idx, 2) == ["-p", "nudge prompt"] + refute "ORIGINAL TASK PROMPT" in out + refute "--conversation" in out + assert chunk_after(out, "--model") == "gemini-3.1-pro" + end + + test "errors when there is no -p slot (custom command / fixture)" do + assert {:error, :no_print_slot} = + Gemini.splice_prompt(["sh", "-c", "echo hi; exit 0"], ["nudge"]) + + assert {:error, :no_print_slot} = + Gemini.splice_prompt(["sh", "-c", "echo hi; exit 0"], [ + "--resume", + "sid", + "prompt" + ]) + end + end end diff --git a/apps/arbiter/test/arbiter/worker/respawn_provider_test.exs b/apps/arbiter/test/arbiter/worker/respawn_provider_test.exs index e06348082..aa3e081dd 100644 --- a/apps/arbiter/test/arbiter/worker/respawn_provider_test.exs +++ b/apps/arbiter/test/arbiter/worker/respawn_provider_test.exs @@ -86,37 +86,31 @@ defmodule Arbiter.Worker.RespawnProviderTest do port end - test "a nudge respawn keeps the run's own provider instead of falling back to claude", - %{task: task, pid: pid} do - start_session!(pid, "codex") - - # First session exits -> notes gate (blank notes) -> nudge respawn of the - # same stashed argv, which exits the same way -> cap exhausted -> park. - wait_until(fn -> match?(%{status: :failed}, Worker.state(pid)) end) - - providers = task.id |> events_for() |> Enum.map(& &1.provider) - - # Two sessions ran (original + nudge respawn) and BOTH are codex. - assert length(providers) == 2 - assert Enum.all?(providers, &(&1 == "codex")), "got providers: #{inspect(providers)}" - end - - test "a provider whose argv we cannot rewrite parks instead of silently re-running the task", - %{task: task, pid: pid} do - start_session!(pid, "gemini") - - wait_until(fn -> match?(%{status: :failed}, Worker.state(pid)) end) - - snap = Worker.state(pid) - - # `Arbiter.Agents.Gemini` exports no `splice_prompt/2`, so the nudge cannot - # be delivered. Relaunching the untouched argv would re-run the WHOLE - # original prompt at full price while pretending it was a nudge — park and - # escalate with a concrete cause instead. - assert snap.meta[:notes_gate_detail] == {:respawn_failed, :unsupported_provider} - - events = events_for(task.id) - assert length(events) == 1 - assert hd(events).provider == "gemini" + # bd-b7e33c: `Arbiter.Agents.Gemini.splice_prompt/2` now exists, so gemini/agy + # is rewritten by the same dynamic-adapter path as claude/codex — it's no + # longer the one provider that parks with `:unsupported_provider` on a nudge + # respawn. This fixture's fake argv (`sh -c "echo …"`) has no `-p` slot, so + # the splice is a no-op (`{:error, :no_print_slot} -> {:ok, port_args}`) and + # the untouched argv is relaunched, exactly like the claude/codex fixtures. + for provider <- ["codex", "gemini"] do + test "a nudge respawn keeps the run's own provider (#{provider}) instead of falling back to claude", + %{task: task, pid: pid} do + start_session!(pid, unquote(provider)) + + # First session exits -> notes gate (blank notes) -> nudge respawn of the + # same stashed argv, which exits the same way -> cap exhausted -> park. + wait_until(fn -> match?(%{status: :failed}, Worker.state(pid)) end) + + snap = Worker.state(pid) + assert snap.meta[:notes_gate_detail] == :cap_exhausted + + providers = task.id |> events_for() |> Enum.map(& &1.provider) + + # Two sessions ran (original + nudge respawn) and BOTH kept the run's provider. + assert length(providers) == 2 + + assert Enum.all?(providers, &(&1 == unquote(provider))), + "got providers: #{inspect(providers)}" + end end end diff --git a/apps/arbiter/test/arbiter/worker_resume_test.exs b/apps/arbiter/test/arbiter/worker_resume_test.exs index 6f821c941..84640916d 100644 --- a/apps/arbiter/test/arbiter/worker_resume_test.exs +++ b/apps/arbiter/test/arbiter/worker_resume_test.exs @@ -285,5 +285,51 @@ defmodule Arbiter.Worker.ResumeTest do "CONTINUE PROMPT" ] end + + # bd-b7e33c: gemini/agy used to be the one provider `inject_resume_argv/4` + # could not rewrite at all (Arbiter.Agents.Gemini had no `splice_prompt/2`) + # — it fell straight to `:unsupported_provider`. Now it's driven through + # the same dynamic-adapter path as claude/codex. + test "inserts --conversation for agy and swaps the prompt" do + argv = [ + "sh", + "-c", + "exec \"$@\" < /dev/null", + "sh", + "/bin/agy", + "-p", + "ORIGINAL TASK PROMPT", + "--model", + "gemini-3.1-pro", + "--output-format", + "stream-json" + ] + + {:ok, %{argv: out}} = + Worker.inject_resume_argv(%{argv: argv}, "sess-abc", "CONTINUE PROMPT", "gemini") + + idx = Enum.find_index(out, &(&1 == "-p")) + assert Enum.slice(out, idx, 2) == ["-p", "CONTINUE PROMPT"] + refute "ORIGINAL TASK PROMPT" in out + assert Enum.find_index(out, &(&1 == "--conversation")) == idx + 2 + assert Enum.at(out, idx + 3) == "sess-abc" + end + + test "upstream gemini CLI (no --conversation) returns an explicit error, not :unsupported_provider" do + argv = [ + "sh", + "-c", + "exec \"$@\" < /dev/null", + "sh", + "/bin/gemini", + "-p", + "ORIGINAL TASK PROMPT", + "--model", + "gemini-2.5-pro" + ] + + assert {:error, :resume_unsupported} = + Worker.inject_resume_argv(%{argv: argv}, "sess-abc", "CONTINUE PROMPT", "gemini") + end end end From 04b84266b351e674effc0f8b458ce7f69c8fd009 Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Fri, 18 Sep 2026 16:56:20 -0400 Subject: [PATCH 2/8] bd-b7e33c AC5 post-merge fix: resume no longer silently switches an agy task's provider to Claude MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge verification of T7 found that both worker_resume (MCP tool, Dispatch.resume/2) and arb worker resume (Dispatch.resume_session/2) re-derived the provider from Routing.choose/2 on every resume with no memory of what the prior run actually used. An agy/gemini task's resume silently dispatched on Claude instead — spending the quota the operator routed to agy specifically to conserve, with no signal in the result that a provider switch happened. For resume_session/2 this was worse than a wrong choice of agent: a Claude spawn could receive an agy conversation UUID as --resume, a nonsensical invocation. Both functions now default :agent_type (unless the caller passes one explicitly) to the provider recorded on the task's most recent usage-ledger row, via a new Dispatch.latest_provider/1. A resume stays on the same provider by construction instead of by accident. Co-Authored-By: Claude Sonnet 5 --- apps/arbiter/lib/arbiter/worker/dispatch.ex | 47 +++++- .../test/arbiter/worker/dispatch_test.exs | 151 +++++++++++++++++- 2 files changed, 192 insertions(+), 6 deletions(-) diff --git a/apps/arbiter/lib/arbiter/worker/dispatch.ex b/apps/arbiter/lib/arbiter/worker/dispatch.ex index 9fcbf019b..1fc0fb863 100644 --- a/apps/arbiter/lib/arbiter/worker/dispatch.ex +++ b/apps/arbiter/lib/arbiter/worker/dispatch.ex @@ -258,11 +258,15 @@ defmodule Arbiter.Worker.Dispatch do the prior worker's committed + uncommitted work, so it continues from where the stopped run left off instead of restarting from scratch. - This is the explicit `arb resume ` path. It is provider-agnostic — no - Claude/Gemini session-resume id; the continuity comes entirely from the - preserved worktree state plus a `Arbiter.Worker.ResumeContext` briefing - prepended to the standard work prompt (coordinator sign-off 2026-06-05, approach - (b)). + This is the explicit `arb resume ` path. It carries no Claude/Gemini + session-resume id; the continuity comes from the preserved worktree state + plus a `Arbiter.Worker.ResumeContext` briefing prepended to the standard + work prompt (coordinator sign-off 2026-06-05, approach (b)). It is NOT + provider-agnostic, though: unless the caller passes an explicit + `:agent_type`, the fresh agent defaults to whichever provider the task's + most recent usage-ledger row ran on (bd-b7e33c AC5), so resuming an agy run + doesn't silently switch to Claude and spend quota the operator dispatched + to agy specifically to conserve. ## Steps @@ -312,6 +316,7 @@ defmodule Arbiter.Worker.Dispatch do resume_opts = opts + |> Keyword.put_new(:agent_type, latest_provider(task_id)) |> Keyword.put(:repo, repo) |> Keyword.put(:start_claude, true) |> Keyword.put(:resume, true) @@ -384,6 +389,7 @@ defmodule Arbiter.Worker.Dispatch do resume_opts = opts + |> Keyword.put_new(:agent_type, latest_provider(task_id)) |> Keyword.put(:repo, repo) |> Keyword.put(:start_claude, true) |> Keyword.put(:resume, true) @@ -606,6 +612,37 @@ defmodule Arbiter.Worker.Dispatch do _ -> {:error, :no_session} end + # bd-b7e33c AC5 post-merge finding: neither `resume/2` nor `resume_session/2` + # threaded a provider through to the new dispatch, so `build_agent_session_opts` + # re-ran `Routing.choose/2` from scratch and could silently hand an agy/gemini + # task's resume to Claude — spending the quota the whole agy-parity epic exists + # to conserve, with no signal in the result that a provider switch happened. + # Default `:agent_type` (unless the caller already forced one) to the provider + # the task's most recent usage-ledger row actually ran on, so a resume stays on + # the same provider by construction. `nil` (no prior usage row, or an + # unrecognized provider string) leaves the routing policy free to choose, same + # as before this fix. + defp latest_provider(task_id) when is_binary(task_id) do + Event + |> Ash.Query.filter(task_id == ^task_id and not is_nil(provider)) + |> Ash.Query.sort(occurred_at: :desc) + |> Ash.Query.limit(1) + |> Ash.read!() + |> List.first() + |> case do + %Event{provider: p} when is_binary(p) and p != "" -> safe_provider_atom(p) + _ -> nil + end + rescue + _ -> nil + end + + defp safe_provider_atom(p) do + String.to_existing_atom(p) + rescue + ArgumentError -> nil + end + # `review: true` is the convenience hook used by `arb review`: it forces the # review-only defaults so the caller doesn't have to spell out four flags in # tandem (and so the CLI/REST surface can't accidentally request, say, a diff --git a/apps/arbiter/test/arbiter/worker/dispatch_test.exs b/apps/arbiter/test/arbiter/worker/dispatch_test.exs index e5d77d341..bad1e28f7 100644 --- a/apps/arbiter/test/arbiter/worker/dispatch_test.exs +++ b/apps/arbiter/test/arbiter/worker/dispatch_test.exs @@ -2756,7 +2756,7 @@ defmodule Arbiter.Worker.DispatchTest do File.rm_rf!(tmp) end) - %{repo: repo, worktree_root: worktree_root} + %{repo: repo, worktree_root: worktree_root, tmp: tmp} end # Dispatch a task, provisioning its worktree, then simulate a mid-work stop: @@ -3019,6 +3019,155 @@ defmodule Arbiter.Worker.DispatchTest do assert is_binary(result.worktree_path) end + + # bd-b7e33c AC5 post-merge finding: `worker_resume` (the MCP tool backing + # this path) resumed an agy task straight onto Claude — Routing.choose/2 + # re-decided the provider from the workspace default with nothing to pin + # it to the prior run. Without an explicit `agent_type`, `resume/2` must + # now default to the provider the task's most recent usage row ran on. + test "resume/2 without an explicit agent_type stays on the prior run's provider", + %{ws: ws, tmp: tmp} do + gemini_file = Path.join(tmp, "gemini-resume-argv.txt") + :ok = stub_sleeping_on_path(tmp, "agy", gemini_file) + + {:ok, task} = Ash.create(Issue, %{title: "agy resume provider", workspace_id: ws.id}) + + # Workspace defaults to Claude — nothing here forces gemini explicitly on + # the resume call, so a passing test proves the default came from the + # prior run's ledger row, not from workspace/routing config. + {:ok, first} = + Dispatch.dispatch(task.id, + repo: "rs/repo", + start_driver: false, + start_claude: true, + agent_type: :gemini, + preflight: false + ) + + _ = wait_for_argv!(gemini_file) + :ok = Worker.fail(first.worker_pid, :token_exhausted) + + # A real agy run writes this row itself when its session port exits; + # simulate that here rather than running a full CLI session. + {:ok, _event} = + Ash.create(UsageEvent, %{ + task_id: task.id, + workspace_id: ws.id, + repo: "rs/repo", + step: :work, + provider: "gemini", + occurred_at: DateTime.utc_now() + }) + + File.rm!(gemini_file) + + {:ok, result} = Dispatch.resume(task.id, start_driver: false, preflight: false) + + _ = wait_for_argv!(gemini_file) + + routing = Worker.state(result.worker_pid).meta[:routing_config] + assert routing.provider == "gemini" + end + end + + describe "resume_session/2 (bd-1z7624)" do + @env_key :repo_paths + + setup do + tmp = + Path.join( + System.tmp_dir!(), + "dispatch-resume-session-#{:erlang.unique_integer([:positive])}" + ) + + repo = Path.join(tmp, "source") + File.mkdir_p!(repo) + + {_, 0} = System.cmd("git", ["init", "-q", "-b", "main", repo]) + {_, 0} = System.cmd("git", ["-C", repo, "config", "user.email", "test@example.com"]) + {_, 0} = System.cmd("git", ["-C", repo, "config", "user.name", "Test User"]) + {_, 0} = System.cmd("git", ["-C", repo, "config", "commit.gpgsign", "false"]) + File.write!(Path.join(repo, "README.md"), "hello\n") + {_, 0} = System.cmd("git", ["-C", repo, "add", "README.md"]) + {_, 0} = System.cmd("git", ["-C", repo, "commit", "-q", "-m", "initial"]) + + remote = Path.join(tmp, "remote.git") + {_, 0} = System.cmd("git", ["init", "-q", "--bare", "-b", "main", remote]) + {_, 0} = System.cmd("git", ["-C", repo, "remote", "add", "origin", remote]) + {_, 0} = System.cmd("git", ["-C", repo, "push", "-q", "origin", "main"]) + + worktree_root = Path.join(tmp, "worktrees") + File.mkdir_p!(worktree_root) + + prior_wt_root = Application.get_env(:arbiter, :worktree_root) + prior_repo_paths = Application.get_env(:arbiter, @env_key) + + Application.put_env(:arbiter, :worktree_root, worktree_root) + Application.put_env(:arbiter, @env_key, %{"rs/repo" => repo}) + + on_exit(fn -> + if prior_wt_root, + do: Application.put_env(:arbiter, :worktree_root, prior_wt_root), + else: Application.delete_env(:arbiter, :worktree_root) + + if prior_repo_paths, + do: Application.put_env(:arbiter, @env_key, prior_repo_paths), + else: Application.delete_env(:arbiter, @env_key) + + File.rm_rf!(tmp) + end) + + %{repo: repo, worktree_root: worktree_root, tmp: tmp} + end + + # bd-b7e33c AC2/AC5: the actual `arb worker resume` / `POST + # /api/workers/:id/resume` surface. Session-level resume threads the prior + # `session_id` through `Worker.inject_resume_argv/4`, which now (T7) + # translates it to `--conversation ` for gemini/agy — but only if the + # fresh dispatch actually resolves the gemini adapter. Without pinning + # `:agent_type` to the ledger's recorded provider, `Routing.choose/2` could + # still hand the spawn to Claude, and `--conversation ` would get + # injected into a Claude invocation instead. + test "resume_session/2 without an explicit agent_type dispatches the same provider as the prior session", + %{ws: ws, tmp: tmp} do + gemini_file = Path.join(tmp, "gemini-resume-session-argv.txt") + :ok = stub_sleeping_on_path(tmp, "agy", gemini_file) + + {:ok, task} = Ash.create(Issue, %{title: "agy resume session", workspace_id: ws.id}) + + {:ok, first} = + Dispatch.dispatch(task.id, + repo: "rs/repo", + start_driver: false, + start_claude: true, + agent_type: :gemini, + preflight: false + ) + + _ = wait_for_argv!(gemini_file) + :ok = Worker.fail(first.worker_pid, :token_exhausted) + + {:ok, _event} = + Ash.create(UsageEvent, %{ + task_id: task.id, + workspace_id: ws.id, + repo: "rs/repo", + step: :work, + provider: "gemini", + session_id: "agy-conv-#{:erlang.unique_integer([:positive])}", + occurred_at: DateTime.utc_now() + }) + + File.rm!(gemini_file) + + {:ok, result} = Dispatch.resume_session(task.id, start_driver: false, preflight: false) + + resumed_args = wait_for_argv!(gemini_file) + assert "--conversation" in resumed_args + + routing = Worker.state(result.worker_pid).meta[:routing_config] + assert routing.provider == "gemini" + end end describe "review dispatch (review: true)" do From a8abf4da5e018346a6726d9d483ae486659eedf7 Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Mon, 21 Sep 2026 12:41:26 -0400 Subject: [PATCH 3/8] bd-b7e33c AC5 post-merge fix: resume_session/2 no longer mixes provider and session_id from different usage rows verification found the provider-preservation half of the earlier fix worked but conversation continuity did not: latest_provider/1 and latest_session_id/1 ran independent "newest row" queries, so a task whose most recent usage row recorded a different provider than the row holding the resumable session_id could pin :agent_type off one row while threading the OTHER row's conversation id into --conversation. latest_session_id/1 now returns the provider from the SAME row the session_id came from, and resume_session/2 pins :agent_type to that value instead of a separate lookup. Co-Authored-By: Claude Sonnet 5 --- apps/arbiter/lib/arbiter/worker/dispatch.ex | 40 ++++++---- .../test/arbiter/worker/dispatch_test.exs | 74 +++++++++++++++++++ 2 files changed, 101 insertions(+), 13 deletions(-) diff --git a/apps/arbiter/lib/arbiter/worker/dispatch.ex b/apps/arbiter/lib/arbiter/worker/dispatch.ex index ba54195f3..a6a8cbdd9 100644 --- a/apps/arbiter/lib/arbiter/worker/dispatch.ex +++ b/apps/arbiter/lib/arbiter/worker/dispatch.ex @@ -377,7 +377,7 @@ defmodule Arbiter.Worker.Dispatch do :ok <- ensure_not_active(task_id), {:ok, repo} <- resolve_resume_repo(task, opts), {:ok, _worktree_path} <- resume_worktree(task, repo), - {:ok, session_id} <- latest_session_id(task_id) do + {:ok, session_id, session_provider} <- latest_session_id(task_id) do prior_run_id = latest_run_id(task_id) # Free the registry slot the same way resume/2 does: a stopped worker @@ -388,7 +388,7 @@ defmodule Arbiter.Worker.Dispatch do resume_opts = opts - |> Keyword.put_new(:agent_type, latest_provider(task_id)) + |> Keyword.put_new(:agent_type, session_provider) |> Keyword.put(:repo, repo) |> Keyword.put(:start_claude, true) |> Keyword.put(:resume, true) @@ -588,14 +588,23 @@ defmodule Arbiter.Worker.Dispatch do _ -> nil end - # The most-recent captured upstream session id for the task, newest first. - # Drawn from the usage ledger (`Arbiter.Usage.Event`), where the worker - # persists each Claude session's `session_id` on its terminal `result` event. - # The task_id filter is exact, so ReviewGate reviewer rows (which carry a - # `#review` suffix) are excluded — we resume the author's session, not a - # reviewer's. `{:error, :no_session}` when none was ever captured: the task - # was never worked by a session-capable agent, so there is nothing to resume - # at the session level (the caller must dispatch fresh). + # The most-recent captured upstream session id for the task, newest first, + # PLUS the provider recorded on that SAME row. Drawn from the usage ledger + # (`Arbiter.Usage.Event`), where the worker persists each session's + # `session_id` on its terminal `result` event. The task_id filter is exact, + # so ReviewGate reviewer rows (which carry a `#review` suffix) are excluded + # — we resume the author's session, not a reviewer's. `{:error, :no_session}` + # when none was ever captured: the task was never worked by a + # session-capable agent, so there is nothing to resume at the session level + # (the caller must dispatch fresh). + # + # bd-b7e33c post-merge finding (2026-09-19): this used to return only the + # session_id, and callers paired it with a SEPARATE `latest_provider/1` + # query. The two queries can pick different rows — e.g. a newer row from a + # failed attempt on a different provider that never got far enough to + # capture a session_id — pinning `:agent_type` to a provider that doesn't + # own the conversation id being resumed. Returning the provider off the + # exact row the session_id came from makes that mismatch impossible. defp latest_session_id(task_id) when is_binary(task_id) do Event |> Ash.Query.filter(task_id == ^task_id and not is_nil(session_id)) @@ -604,8 +613,11 @@ defmodule Arbiter.Worker.Dispatch do |> Ash.read!() |> List.first() |> case do - %Event{session_id: sid} when is_binary(sid) and sid != "" -> {:ok, sid} - _ -> {:error, :no_session} + %Event{session_id: sid, provider: p} when is_binary(sid) and sid != "" -> + {:ok, sid, safe_provider_atom(p)} + + _ -> + {:error, :no_session} end rescue _ -> {:error, :no_session} @@ -636,12 +648,14 @@ defmodule Arbiter.Worker.Dispatch do _ -> nil end - defp safe_provider_atom(p) do + defp safe_provider_atom(p) when is_binary(p) do String.to_existing_atom(p) rescue ArgumentError -> nil end + defp safe_provider_atom(_), do: nil + # `review: true` is the convenience hook used by `arb review`: it forces the # review-only defaults so the caller doesn't have to spell out four flags in # tandem (and so the CLI/REST surface can't accidentally request, say, a diff --git a/apps/arbiter/test/arbiter/worker/dispatch_test.exs b/apps/arbiter/test/arbiter/worker/dispatch_test.exs index 03d1609bd..1de3bd483 100644 --- a/apps/arbiter/test/arbiter/worker/dispatch_test.exs +++ b/apps/arbiter/test/arbiter/worker/dispatch_test.exs @@ -3186,6 +3186,80 @@ defmodule Arbiter.Worker.DispatchTest do routing = Worker.state(result.worker_pid).meta[:routing_config] assert routing.provider == "gemini" end + + # bd-b7e33c post-merge finding (2026-09-19): the provider and the + # session_id used to come from two INDEPENDENT "newest row" queries + # (`latest_provider/1` and `latest_session_id/1`), so a task whose most + # recent usage-ledger row records a different provider than the row that + # actually captured the resumable session_id could pin `:agent_type` to + # the wrong provider while still threading the OTHER session's + # conversation id — exactly the "spawn handed a conversation UUID that + # belongs to a different provider" shape the AC5 fix was meant to close. + # Reproduce it directly: a NEWER usage row with no session_id records + # `provider: "claude"` (e.g. a claude fallback attempt that errored before + # the CLI ever reported a session), while an OLDER row on the same task + # carries the real, resumable `session_id` under `provider: "gemini"`. + test "resume_session/2 pins the provider to the SAME row the session_id came from", + %{ws: ws, tmp: tmp} do + gemini_file = Path.join(tmp, "gemini-resume-mismatch-argv.txt") + :ok = stub_sleeping_on_path(tmp, "agy", gemini_file) + + {:ok, task} = Ash.create(Issue, %{title: "agy resume mismatch", workspace_id: ws.id}) + + {:ok, first} = + Dispatch.dispatch(task.id, + repo: "rs/repo", + start_driver: false, + start_claude: true, + agent_type: :gemini, + preflight: false + ) + + _ = wait_for_argv!(gemini_file) + :ok = Worker.fail(first.worker_pid, :token_exhausted) + + {:ok, older} = + Ash.create(UsageEvent, %{ + task_id: task.id, + workspace_id: ws.id, + repo: "rs/repo", + step: :work, + provider: "gemini", + session_id: "agy-conv-mismatch", + occurred_at: DateTime.add(DateTime.utc_now(), -600, :second) + }) + + {:ok, _newer} = + Ash.create(UsageEvent, %{ + task_id: task.id, + workspace_id: ws.id, + repo: "rs/repo", + step: :work, + provider: "claude", + occurred_at: DateTime.utc_now() + }) + + # sanity: the two independent lookups really do disagree, so this test + # actually exercises the mismatch rather than a scenario that can't occur. + refute older.session_id == nil + + File.rm!(gemini_file) + + {:ok, result} = + Dispatch.resume_session(task.id, + repo: "rs/repo", + start_driver: false, + preflight: false + ) + + resumed_args = wait_for_argv!(gemini_file) + assert "--conversation" in resumed_args + conv_idx = Enum.find_index(resumed_args, &(&1 == "--conversation")) + assert Enum.at(resumed_args, conv_idx + 1) == "agy-conv-mismatch" + + routing = Worker.state(result.worker_pid).meta[:routing_config] + assert routing.provider == "gemini" + end end describe "review dispatch (review: true)" do From 9b5ec6e72734c630415fcc5a2018e2ef3c46b283 Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Mon, 21 Sep 2026 13:07:31 -0400 Subject: [PATCH 4/8] bd-b7e33c round-1 review fixes: guard resume_session_id mismatch, make the regression test actually regress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1: the mismatch regression test constructed the mismatch out of two usage-ledger rows, but the provider resolver reads worker_run rows first (Run.latest_authoring_provider/1) and only falls back to the usage ledger when no run carries a provider — so the fixture's extra usage row never influenced anything and the test passed even with the AC5 fix reverted. Rebuilt the mismatch as a newer failed Run row (what the resolver actually reads), stubbed claude on the test PATH so a future regression spawns the stub instead of the operator's real CLI, and corrected the comment's claim about which query the resolver uses. Verified: fails pre-fix (times out waiting for the agy argv file because dispatch goes to the claude stub instead), passes post-fix. Finding 2: resolve_session_resume_provider/3 can fall through to a DIFFERENT provider than the one that captured session_id (unknown/ unavailable session provider, or an explicit agent_type override), but resume_session/2 still threaded that foreign session_id straight into resume_opts regardless — producing a bogus invocation like `claude --resume ` on the mismatched CLI. resume_opts now only carries :resume_session_id when the resolved provider matches the provider that owns it; otherwise it degrades to resume/2's context-based briefing instead of handing a foreign conversation id to another CLI. Co-Authored-By: Claude Sonnet 5 --- apps/arbiter/lib/arbiter/worker/dispatch.ex | 17 ++++++- .../test/arbiter/worker/dispatch_test.exs | 50 ++++++++++++------- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/apps/arbiter/lib/arbiter/worker/dispatch.ex b/apps/arbiter/lib/arbiter/worker/dispatch.ex index ece9a4a14..7cbb7a040 100644 --- a/apps/arbiter/lib/arbiter/worker/dispatch.ex +++ b/apps/arbiter/lib/arbiter/worker/dispatch.ex @@ -398,7 +398,7 @@ defmodule Arbiter.Worker.Dispatch do |> Keyword.put(:repo, repo) |> Keyword.put(:start_claude, true) |> Keyword.put(:resume, true) - |> Keyword.put(:resume_session_id, session_id) + |> maybe_put_resume_session_id(provider == session_provider, session_id) |> Keyword.put(:resumed_from_run_id, prior_run_id) |> Keyword.put(:existing_pr_ref, task.pr_ref) @@ -693,6 +693,21 @@ defmodule Arbiter.Worker.Dispatch do defp put_opt_if_present(opts, _key, ""), do: opts defp put_opt_if_present(opts, key, value), do: Keyword.put(opts, key, value) + # bd-b7e33c finding 2 (round 1 re-review): resolve_session_resume_provider/3 + # can fall through to resolve_resume_provider/2 and land on a DIFFERENT + # provider than the one that captured session_id (unknown/unavailable + # session provider, or an explicit agent_type override). Threading the old + # session_id through to a mismatched provider produces a bogus invocation — + # e.g. `claude --resume `, which the Claude CLI + # rejects. Only carry resume_session_id when the resolved provider still + # matches the provider that owns it; otherwise degrade to resume/2's + # context-based briefing instead of handing a foreign conversation id to + # another CLI. + defp maybe_put_resume_session_id(opts, true, session_id), + do: Keyword.put(opts, :resume_session_id, session_id) + + defp maybe_put_resume_session_id(opts, false, _session_id), do: opts + # `review: true` is the convenience hook used by `arb review`: it forces the # review-only defaults so the caller doesn't have to spell out four flags in # tandem (and so the CLI/REST surface can't accidentally request, say, a diff --git a/apps/arbiter/test/arbiter/worker/dispatch_test.exs b/apps/arbiter/test/arbiter/worker/dispatch_test.exs index c5505d6f4..705e04b77 100644 --- a/apps/arbiter/test/arbiter/worker/dispatch_test.exs +++ b/apps/arbiter/test/arbiter/worker/dispatch_test.exs @@ -3240,22 +3240,33 @@ defmodule Arbiter.Worker.DispatchTest do assert routing.provider == "gemini" end - # bd-b7e33c post-merge finding (2026-09-19): the provider and the - # session_id used to come from two INDEPENDENT "newest row" queries - # (`latest_provider/1` and `latest_session_id/1`), so a task whose most - # recent usage-ledger row records a different provider than the row that - # actually captured the resumable session_id could pin `:agent_type` to - # the wrong provider while still threading the OTHER session's - # conversation id — exactly the "spawn handed a conversation UUID that - # belongs to a different provider" shape the AC5 fix was meant to close. - # Reproduce it directly: a NEWER usage row with no session_id records - # `provider: "claude"` (e.g. a claude fallback attempt that errored before - # the CLI ever reported a session), while an OLDER row on the same task - # carries the real, resumable `session_id` under `provider: "gemini"`. + # bd-b7e33c post-merge finding (2026-09-19), corrected 2026-09-21 per + # round-1 review finding 1: the provider and the session_id used to come + # from two INDEPENDENT "newest row" queries, so a task whose most recent + # signal recorded a different provider than the row that actually + # captured the resumable session_id could pin `:agent_type` to the wrong + # provider while still threading the OTHER session's conversation id — + # exactly the "spawn handed a conversation UUID that belongs to a + # different provider" shape the AC5 fix was meant to close. + # + # The provider half of that pin is NOT resolved off the usage ledger — + # `resolve_session_resume_provider/3` only falls through to + # `resolve_resume_provider/2` -> `Agents.resolve_revision_provider/2` -> + # `Run.latest_authoring_provider/1`, which reads `worker_run` rows FIRST + # and only consults the usage ledger when no run carries a provider. So + # the mismatch has to be a newer **Run** row, not a newer usage-event row + # (a usage-event-only fixture resolves to the same provider before and + # after the fix, and would pass even with the fix reverted). Reproduce it + # directly: an OLDER usage row carries the real, resumable `session_id` + # under `provider: "gemini"`, while a NEWER **Run** row (a claude fallback + # attempt that failed before capturing a session) records `provider: + # "claude"`. test "resume_session/2 pins the provider to the SAME row the session_id came from", %{ws: ws, tmp: tmp} do gemini_file = Path.join(tmp, "gemini-resume-mismatch-argv.txt") + claude_file = Path.join(tmp, "claude-resume-mismatch-argv.txt") :ok = stub_sleeping_on_path(tmp, "agy", gemini_file) + :ok = stub_sleeping_on_path(tmp, "claude", claude_file) {:ok, task} = Ash.create(Issue, %{title: "agy resume mismatch", workspace_id: ws.id}) @@ -3282,19 +3293,24 @@ defmodule Arbiter.Worker.DispatchTest do occurred_at: DateTime.add(DateTime.utc_now(), -600, :second) }) - {:ok, _newer} = - Ash.create(UsageEvent, %{ + # A newer FAILED claude attempt that never captured a session_id — this + # is what Run.latest_authoring_provider/1 actually reads, so it is what + # would steal the resume without resolve_session_resume_provider/3. + {:ok, _claude_run} = + Ash.create(Run, %{ task_id: task.id, - workspace_id: ws.id, repo: "rs/repo", - step: :work, + workspace_id: ws.id, + worker_type: :main, + status: :failed, provider: "claude", - occurred_at: DateTime.utc_now() + started_at: DateTime.utc_now() }) # sanity: the two independent lookups really do disagree, so this test # actually exercises the mismatch rather than a scenario that can't occur. refute older.session_id == nil + assert Run.latest_authoring_provider(task.id) == :claude File.rm!(gemini_file) From 189c539b0b09ef38bcecf48fc917a53e3eadbf39 Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Mon, 21 Sep 2026 14:03:40 -0400 Subject: [PATCH 5/8] bd-b7e33c CI fix: suppress dialyzer false-positive on maybe_put_resume_session_id/3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's mix audit failed dialyzer with `lib/arbiter/worker/dispatch.ex:1:pattern_match`: the `false` clause of the private, single-call-site maybe_put_resume_session_id(opts, provider == session_provider, session_id) is flagged unreachable because dialyzer's success typing narrows the boolean argument to the literal `true`. It's a known success-typing precision limit, not a real dead branch — resume_session/2's explicit agent_type override and the provider-unavailable fallback in resolve_session_resume_provider/3 both legitimately produce a provider that differs from session_provider at runtime, and skipping the :resume_session_id put in that case is the entire point of the guard added in 9b5ec6e7. Added a regression test exercising the override path directly (resume with an explicit agent_type that differs from the provider that captured the prior session_id) to prove the `false` clause is reachable and correct. Co-Authored-By: Claude Sonnet 5 --- .dialyzer_ignore.exs | 19 +++++++ .../test/arbiter/worker/dispatch_test.exs | 57 +++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index e79702bbf..1a26972ea 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -91,9 +91,28 @@ # atom clause beside the `{:no_verdict, reason}` tuple one, and a # `load_member_issues([])` clause. Both are cheap total-function # hygiene on a private helper. + # * worker/dispatch.ex (bd-b7e33c) — a different shape: the private, + # single-call-site `maybe_put_resume_session_id(opts, provider == + # session_provider, session_id)` has a `true` clause and a `false` + # clause; dialyzer's success typing narrows the boolean argument to the + # literal `true` and reports the `false` clause dead. It is not: + # `resume_session/2`'s explicit `agent_type:` override (and the + # provider-unavailable fallback in `resolve_session_resume_provider/3`) + # both produce a `provider` that legitimately differs from + # `session_provider` at runtime, and skipping the `:resume_session_id` + # put in that case is the entire point of the guard — see the + # "resume_session/2 with an explicit agent_type override does not + # thread the other provider's session_id" test in dispatch_test.exs, + # which fails if that clause is ever actually unreachable. Dialyzer + # can't see the correlation because the two values come from + # independent branches of a call it doesn't inline visibly in this + # diagnostic — a known success-typing precision limit on boolean flags + # computed from two independently-sourced variables, not a real dead + # branch. {"lib/arbiter/agents/preflight.ex", :pattern_match}, {"lib/arbiter/mcp/tools.ex", :pattern_match}, {"lib/arbiter/mcp/tools/loop_pending.ex", :pattern_match}, + {"lib/arbiter/worker/dispatch.ex", :pattern_match}, {"lib/arbiter/worker/driver.ex", :pattern_match}, {"lib/arbiter/worker/review_gate.ex", :pattern_match}, {"lib/arbiter/workflows/conductor.ex", :pattern_match}, diff --git a/apps/arbiter/test/arbiter/worker/dispatch_test.exs b/apps/arbiter/test/arbiter/worker/dispatch_test.exs index 705e04b77..b905a2018 100644 --- a/apps/arbiter/test/arbiter/worker/dispatch_test.exs +++ b/apps/arbiter/test/arbiter/worker/dispatch_test.exs @@ -3329,6 +3329,63 @@ defmodule Arbiter.Worker.DispatchTest do routing = Worker.state(result.worker_pid).meta[:routing_config] assert routing.provider == "gemini" end + + # bd-b7e33c finding 2 (round 1 re-review): the mismatch guard in + # `maybe_put_resume_session_id/3` also has to fire when the CALLER forces + # a different provider via an explicit `agent_type:` opt — not just when + # the resolver falls through on its own. Reproduce that path directly: the + # prior session captured a resumable id under gemini, but the caller + # overrides to claude. The override must win (routing.provider == claude) + # and the foreign gemini conversation id must NOT be threaded into the + # claude spawn — it degrades to a normal `resume: true` context briefing + # instead, exactly like resume/2 does when it has no session id at all. + test "resume_session/2 with an explicit agent_type override does not thread the other provider's session_id", + %{ws: ws, tmp: tmp} do + gemini_file = Path.join(tmp, "gemini-resume-override-argv.txt") + claude_file = Path.join(tmp, "claude-resume-override-argv.txt") + :ok = stub_sleeping_on_path(tmp, "agy", gemini_file) + :ok = stub_sleeping_on_path(tmp, "claude", claude_file) + + {:ok, task} = Ash.create(Issue, %{title: "agy resume override", workspace_id: ws.id}) + + {:ok, first} = + Dispatch.dispatch(task.id, + repo: "rs/repo", + start_driver: false, + start_claude: true, + agent_type: :gemini, + preflight: false + ) + + _ = wait_for_argv!(gemini_file) + :ok = Worker.fail(first.worker_pid, :token_exhausted) + + {:ok, _event} = + Ash.create(UsageEvent, %{ + task_id: task.id, + workspace_id: ws.id, + repo: "rs/repo", + step: :work, + provider: "gemini", + session_id: "agy-conv-override", + occurred_at: DateTime.utc_now() + }) + + {:ok, result} = + Dispatch.resume_session(task.id, + repo: "rs/repo", + start_driver: false, + preflight: false, + agent_type: :claude + ) + + resumed_args = wait_for_argv!(claude_file) + refute "--resume" in resumed_args + refute "agy-conv-override" in resumed_args + + routing = Worker.state(result.worker_pid).meta[:routing_config] + assert routing.provider == "claude" + end end describe "review dispatch (review: true)" do From 7a1e9a1325da8e39b9c6312611fbc9423e1a0299 Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Mon, 21 Sep 2026 14:20:36 -0400 Subject: [PATCH 6/8] bd-b7e33c round-2 review fixes: build the promised briefing on session-provider mismatch, drop the file-wide dialyzer suppression it required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1: resume_session/2's provider-mismatch fallback dropped resume_session_id but never built resume/2's git-derived briefing it claimed to degrade to, so a resumed dispatch on a mismatched provider silently carried the original task prompt with no continuity — risking redone work. It now calls ResumeContext.build/3 (same as resume/2) and logs the dropped session id, or Logger.info-only degrades to no briefing if the worktree briefing itself can't be built. Finding 2: inlining the provider == session_provider branch directly into resume_session/2 (per reviewer's suggested restructure) instead of dispatching through a narrow two-clause private helper eliminates the dialyzer pattern_match false positive that needed the file-wide lib/arbiter/worker/dispatch.ex suppression; confirmed clean with mix dialyzer --format short after removing the ignore entry. Tightened the round-1 mismatch test to assert the briefing text is present in the resumed argv, not just the absence of --resume. Co-Authored-By: Claude Sonnet 5 --- .dialyzer_ignore.exs | 19 ------- apps/arbiter/lib/arbiter/worker/dispatch.ex | 56 +++++++++++++------ .../test/arbiter/worker/dispatch_test.exs | 13 ++++- 3 files changed, 48 insertions(+), 40 deletions(-) diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index 1a26972ea..e79702bbf 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -91,28 +91,9 @@ # atom clause beside the `{:no_verdict, reason}` tuple one, and a # `load_member_issues([])` clause. Both are cheap total-function # hygiene on a private helper. - # * worker/dispatch.ex (bd-b7e33c) — a different shape: the private, - # single-call-site `maybe_put_resume_session_id(opts, provider == - # session_provider, session_id)` has a `true` clause and a `false` - # clause; dialyzer's success typing narrows the boolean argument to the - # literal `true` and reports the `false` clause dead. It is not: - # `resume_session/2`'s explicit `agent_type:` override (and the - # provider-unavailable fallback in `resolve_session_resume_provider/3`) - # both produce a `provider` that legitimately differs from - # `session_provider` at runtime, and skipping the `:resume_session_id` - # put in that case is the entire point of the guard — see the - # "resume_session/2 with an explicit agent_type override does not - # thread the other provider's session_id" test in dispatch_test.exs, - # which fails if that clause is ever actually unreachable. Dialyzer - # can't see the correlation because the two values come from - # independent branches of a call it doesn't inline visibly in this - # diagnostic — a known success-typing precision limit on boolean flags - # computed from two independently-sourced variables, not a real dead - # branch. {"lib/arbiter/agents/preflight.ex", :pattern_match}, {"lib/arbiter/mcp/tools.ex", :pattern_match}, {"lib/arbiter/mcp/tools/loop_pending.ex", :pattern_match}, - {"lib/arbiter/worker/dispatch.ex", :pattern_match}, {"lib/arbiter/worker/driver.ex", :pattern_match}, {"lib/arbiter/worker/review_gate.ex", :pattern_match}, {"lib/arbiter/workflows/conductor.ex", :pattern_match}, diff --git a/apps/arbiter/lib/arbiter/worker/dispatch.ex b/apps/arbiter/lib/arbiter/worker/dispatch.ex index 7cbb7a040..675c9700b 100644 --- a/apps/arbiter/lib/arbiter/worker/dispatch.ex +++ b/apps/arbiter/lib/arbiter/worker/dispatch.ex @@ -366,6 +366,12 @@ defmodule Arbiter.Worker.Dispatch do 7. Delegate to `dispatch/2` with `:resume_session_id` set — the worker injects `--resume ` into its first spawn and stashes the *pristine* argv, so the bd-t9uq25 auto-resume keeps working correctly on top. + If the resolved provider doesn't match the provider that captured the + session id (bd-b7e33c AC5 — e.g. an explicit `--agent` override that + lands on a different CLI than the one owning the conversation), the + foreign session id is dropped and `dispatch/2` gets a git-derived + `ResumeContext.build/3` briefing instead, the same one `resume/2` uses — + never a silent fresh start with no continuity at all. Returns the same `{:ok, dispatch_result()}` / `{:error, reason}` shape as `dispatch/2`. Session-resume-specific errors: `{:error, :no_outpost}`, @@ -379,7 +385,7 @@ defmodule Arbiter.Worker.Dispatch do :ok <- ensure_not_closed(task), :ok <- ensure_not_active(task_id), {:ok, repo} <- resolve_resume_repo(task, opts), - {:ok, _worktree_path} <- resume_worktree(task, repo), + {:ok, worktree_path} <- resume_worktree(task, repo), {:ok, session_id, session_provider} <- latest_session_id(task_id) do prior_run_id = latest_run_id(task_id) @@ -391,14 +397,43 @@ defmodule Arbiter.Worker.Dispatch do {provider, fallback_reason} = resolve_session_resume_provider(task, opts, session_provider) - resume_opts = + base_opts = opts |> Keyword.put(:agent_type, provider) |> put_opt_if_present(:provider_fallback, fallback_reason) |> Keyword.put(:repo, repo) |> Keyword.put(:start_claude, true) |> Keyword.put(:resume, true) - |> maybe_put_resume_session_id(provider == session_provider, session_id) + + # bd-b7e33c finding 2 (round 1 re-review) / finding 1 (round 2): only + # carry resume_session_id when the resolved provider still matches the + # one that captured it — otherwise it's a bogus invocation, e.g. + # `claude --resume `. Degrade to resume/2's real + # git-derived briefing instead of silently dropping both (round-2 fix: + # the old fallback dropped resume_session_id but never built the + # briefing it claimed to fall back to). + resume_opts = + if provider == session_provider do + Keyword.put(base_opts, :resume_session_id, session_id) + else + require Logger + + Logger.info( + "Dispatch.resume_session: dropping session_id for #{task.id} — session " <> + "provider #{inspect(session_provider)} does not match resolved provider " <> + "#{inspect(provider)}; degrading to a git-derived resume briefing instead" + ) + + target_branch = resolve_target_branch(task, Keyword.put(opts, :repo, repo)) + + case ResumeContext.build(task, worktree_path, target_branch) do + {:ok, context} -> Keyword.put(base_opts, :resume_context, context) + {:error, _reason} -> base_opts + end + end + + resume_opts = + resume_opts |> Keyword.put(:resumed_from_run_id, prior_run_id) |> Keyword.put(:existing_pr_ref, task.pr_ref) @@ -693,21 +728,6 @@ defmodule Arbiter.Worker.Dispatch do defp put_opt_if_present(opts, _key, ""), do: opts defp put_opt_if_present(opts, key, value), do: Keyword.put(opts, key, value) - # bd-b7e33c finding 2 (round 1 re-review): resolve_session_resume_provider/3 - # can fall through to resolve_resume_provider/2 and land on a DIFFERENT - # provider than the one that captured session_id (unknown/unavailable - # session provider, or an explicit agent_type override). Threading the old - # session_id through to a mismatched provider produces a bogus invocation — - # e.g. `claude --resume `, which the Claude CLI - # rejects. Only carry resume_session_id when the resolved provider still - # matches the provider that owns it; otherwise degrade to resume/2's - # context-based briefing instead of handing a foreign conversation id to - # another CLI. - defp maybe_put_resume_session_id(opts, true, session_id), - do: Keyword.put(opts, :resume_session_id, session_id) - - defp maybe_put_resume_session_id(opts, false, _session_id), do: opts - # `review: true` is the convenience hook used by `arb review`: it forces the # review-only defaults so the caller doesn't have to spell out four flags in # tandem (and so the CLI/REST surface can't accidentally request, say, a diff --git a/apps/arbiter/test/arbiter/worker/dispatch_test.exs b/apps/arbiter/test/arbiter/worker/dispatch_test.exs index b905a2018..086ad39bd 100644 --- a/apps/arbiter/test/arbiter/worker/dispatch_test.exs +++ b/apps/arbiter/test/arbiter/worker/dispatch_test.exs @@ -3331,14 +3331,16 @@ defmodule Arbiter.Worker.DispatchTest do end # bd-b7e33c finding 2 (round 1 re-review): the mismatch guard in - # `maybe_put_resume_session_id/3` also has to fire when the CALLER forces + # `maybe_put_resume_session_id/9` also has to fire when the CALLER forces # a different provider via an explicit `agent_type:` opt — not just when # the resolver falls through on its own. Reproduce that path directly: the # prior session captured a resumable id under gemini, but the caller # overrides to claude. The override must win (routing.provider == claude) # and the foreign gemini conversation id must NOT be threaded into the - # claude spawn — it degrades to a normal `resume: true` context briefing - # instead, exactly like resume/2 does when it has no session id at all. + # claude spawn — it degrades to a real `ResumeContext.build/3` git-derived + # briefing instead (bd-b7e33c round-2 finding 1: the fallback used to drop + # the session id but never build a briefing either, so it silently + # produced a fresh, un-briefed dispatch). test "resume_session/2 with an explicit agent_type override does not thread the other provider's session_id", %{ws: ws, tmp: tmp} do gemini_file = Path.join(tmp, "gemini-resume-override-argv.txt") @@ -3383,6 +3385,11 @@ defmodule Arbiter.Worker.DispatchTest do refute "--resume" in resumed_args refute "agy-conv-override" in resumed_args + # The dropped session id must be replaced with a real git-derived + # briefing (ResumeContext.build/3), not a silently un-briefed fresh + # dispatch — the prompt argument carries the distinctive framing text. + assert Enum.any?(resumed_args, &String.contains?(&1, "RESUMING work on task")) + routing = Worker.state(result.worker_pid).meta[:routing_config] assert routing.provider == "claude" end From 8c575b1c6fb11688a8e81f03ef969d7ff10a05fd Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Mon, 21 Sep 2026 14:31:17 -0400 Subject: [PATCH 7/8] bd-b7e33c round-3 review fixes: drop dialyzer tautology, log real briefing outcome Finding 1 (merge blocker): resolve_session_resume_provider/3's is_atom/1 guard on session_provider was a tautology (safe_provider_atom/1 never returns anything but an atom or nil), making dialyzer's false-arm of the andalso genuinely dead code. Swap to not is_nil/1, which is not a tautology and keeps the nil-rejection explicit without re-adding the removed .dialyzer_ignore.exs suppression. Finding 2 (nit): the mismatch branch's Logger.info fired before ResumeContext.build/3 and always claimed a briefing was attached, even on the {:error, _} arm that proceeds with none. Move the log after the case and split it: Logger.info on the real briefing, Logger.warning (naming the build failure reason) when none was built. --- apps/arbiter/lib/arbiter/worker/dispatch.ex | 28 ++++++++++++++------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/apps/arbiter/lib/arbiter/worker/dispatch.ex b/apps/arbiter/lib/arbiter/worker/dispatch.ex index 675c9700b..69e2c9242 100644 --- a/apps/arbiter/lib/arbiter/worker/dispatch.ex +++ b/apps/arbiter/lib/arbiter/worker/dispatch.ex @@ -418,17 +418,27 @@ defmodule Arbiter.Worker.Dispatch do else require Logger - Logger.info( - "Dispatch.resume_session: dropping session_id for #{task.id} — session " <> - "provider #{inspect(session_provider)} does not match resolved provider " <> - "#{inspect(provider)}; degrading to a git-derived resume briefing instead" - ) - target_branch = resolve_target_branch(task, Keyword.put(opts, :repo, repo)) case ResumeContext.build(task, worktree_path, target_branch) do - {:ok, context} -> Keyword.put(base_opts, :resume_context, context) - {:error, _reason} -> base_opts + {:ok, context} -> + Logger.info( + "Dispatch.resume_session: dropping session_id for #{task.id} — session " <> + "provider #{inspect(session_provider)} does not match resolved provider " <> + "#{inspect(provider)}; degrading to a git-derived resume briefing instead" + ) + + Keyword.put(base_opts, :resume_context, context) + + {:error, reason} -> + Logger.warning( + "Dispatch.resume_session: dropping session_id for #{task.id} — session " <> + "provider #{inspect(session_provider)} does not match resolved provider " <> + "#{inspect(provider)}; failed to build a git-derived resume briefing " <> + "(#{inspect(reason)}), proceeding with no briefing" + ) + + base_opts end end @@ -716,7 +726,7 @@ defmodule Arbiter.Worker.Dispatch do {p, nil} _ -> - if is_atom(session_provider) and Agents.provider_available?(session_provider) do + if not is_nil(session_provider) and Agents.provider_available?(session_provider) do {session_provider, nil} else resolve_resume_provider(task, opts) From d398af5381b872ff1e82396ffc99eab503c13f57 Mon Sep 17 00:00:00 2001 From: Ryan Born Date: Tue, 22 Sep 2026 11:57:57 -0400 Subject: [PATCH 8/8] bd-b7e33c: surface provider/session_id/resumed_from_run_id from worker-runs history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-09-22 04:26Z post-merge verification of AC5 misread an agy run as having `session_id: NULL` / `provider: null`, and concluded the conversation id was never captured. It was: `worker_runs.session_id` and `.provider` were correctly populated in the DB for both the original and resumed run (and `resumed_from_run_id` was correctly set on the resumed one) — but `arb worker runs --json` (backed by `RunJSON.summary/1`) and the `worker_runs` MCP tool (`serialize_worker_run_summary/1`) both silently dropped `session_id`/`resumed_from_run_id` from their output, so there was no way to observe them without querying the DB directly. Surface all three fields on both surfaces so resume continuity (or its absence) is directly observable from `arb worker runs --json` and the `worker_runs` MCP tool, without DB access. The verified live defect was that the run was resumed via the `worker_resume` MCP tool, which calls `Dispatch.resume/2` — documented and signed off (2026-06-05) as the deliberately fresh-agent, git-briefing resume path that carries no session-resume id for any provider. `Dispatch.resume_session/2` (backing `arb worker resume` / `POST /api/workers/:task_id/resume`) is the session-continuing path, and it already threads agy's conversation id correctly per the existing dispatch_test.exs coverage ("resume_session/2 pins the provider to the SAME row the session_id came from"). Flagged back to the coordinator for a scope decision on whether `worker_resume`/`resume/2` should also gain opportunistic session continuity — that would be a cross-provider behavior change reversing a prior sign-off, out of this fix's authority to make unilaterally. Co-Authored-By: Claude Sonnet 5 --- apps/arbiter/lib/arbiter/mcp/tools/worker.ex | 2 + apps/arbiter/test/arbiter/mcp/tools_test.exs | 44 +++++++++++++++++++ .../arbiter_web/controllers/api/run_json.ex | 5 ++- .../controllers/api/run_controller_test.exs | 39 ++++++++++++++++ 4 files changed, 89 insertions(+), 1 deletion(-) diff --git a/apps/arbiter/lib/arbiter/mcp/tools/worker.ex b/apps/arbiter/lib/arbiter/mcp/tools/worker.ex index cf80453bd..ecb768894 100644 --- a/apps/arbiter/lib/arbiter/mcp/tools/worker.ex +++ b/apps/arbiter/lib/arbiter/mcp/tools/worker.ex @@ -494,6 +494,8 @@ defmodule Arbiter.MCP.Tools.Worker do model: run.model, provider: run.provider, provider_fallback: run.provider_fallback, + session_id: run.session_id, + resumed_from_run_id: run.resumed_from_run_id, started_at: Tools.iso(run.started_at), completed_at: Tools.iso(run.completed_at), exit_code: run.exit_code, diff --git a/apps/arbiter/test/arbiter/mcp/tools_test.exs b/apps/arbiter/test/arbiter/mcp/tools_test.exs index 65faa3878..252929c7d 100644 --- a/apps/arbiter/test/arbiter/mcp/tools_test.exs +++ b/apps/arbiter/test/arbiter/mcp/tools_test.exs @@ -3354,6 +3354,50 @@ defmodule Arbiter.MCP.ToolsTest do assert entry.difficulty_at_dispatch == 3 end + # bd-b7e33c post-merge finding (2026-09-22): only `provider` was surfaced + # here; `session_id` and `resumed_from_run_id` were silently dropped, the + # same gap the REST `/api/workers/history` endpoint had — which is what + # made the 04:26Z production verification misread a captured agy + # conversation id as NULL. Surface both so resume continuity is directly + # observable through this tool too. + test "surfaces session_id and resumed_from_run_id", ctx do + {:ok, task} = Ash.create(Issue, %{title: "session fields target", workspace_id: ctx.ws.id}) + + {:ok, prior} = + Ash.create(Arbiter.Workers.Run, %{ + task_id: task.id, + repo: "arbiter", + workspace_id: ctx.ws.id, + status: :completed, + provider: "gemini", + session_id: "25df47b0-054e-434e-84c1-6876fd9f77de", + started_at: DateTime.add(DateTime.utc_now(), -600, :second) + }) + + {:ok, resumed} = + Ash.create(Arbiter.Workers.Run, %{ + task_id: task.id, + repo: "arbiter", + workspace_id: ctx.ws.id, + status: :completed, + provider: "gemini", + session_id: "89a2b784-6bd5-46e6-a971-2178ca58cdcd", + resumed_from_run_id: prior.id, + started_at: DateTime.utc_now() + }) + + assert {:ok, %{runs: [resumed_entry, prior_entry]}} = + Tools.worker_runs(ctx.coordinator, %{"task_id" => task.id}) + + assert prior_entry.id == prior.id + assert prior_entry.session_id == "25df47b0-054e-434e-84c1-6876fd9f77de" + assert prior_entry.resumed_from_run_id == nil + + assert resumed_entry.id == resumed.id + assert resumed_entry.session_id == "89a2b784-6bd5-46e6-a971-2178ca58cdcd" + assert resumed_entry.resumed_from_run_id == prior.id + end + test "honors a bounded limit", ctx do {:ok, task} = Ash.create(Issue, %{title: "many runs", workspace_id: ctx.ws.id}) diff --git a/apps/arbiter_web/lib/arbiter_web/controllers/api/run_json.ex b/apps/arbiter_web/lib/arbiter_web/controllers/api/run_json.ex index 30161bbcd..daef53cf9 100644 --- a/apps/arbiter_web/lib/arbiter_web/controllers/api/run_json.ex +++ b/apps/arbiter_web/lib/arbiter_web/controllers/api/run_json.ex @@ -34,7 +34,10 @@ defmodule ArbiterWeb.Api.RunJSON do routing_policy: r.routing_policy, model_tier: r.model_tier, thinking: r.thinking, - difficulty_at_dispatch: r.difficulty_at_dispatch + difficulty_at_dispatch: r.difficulty_at_dispatch, + provider: r.provider, + session_id: r.session_id, + resumed_from_run_id: r.resumed_from_run_id } end diff --git a/apps/arbiter_web/test/arbiter_web/controllers/api/run_controller_test.exs b/apps/arbiter_web/test/arbiter_web/controllers/api/run_controller_test.exs index 32cbb573e..571a10367 100644 --- a/apps/arbiter_web/test/arbiter_web/controllers/api/run_controller_test.exs +++ b/apps/arbiter_web/test/arbiter_web/controllers/api/run_controller_test.exs @@ -137,6 +137,45 @@ defmodule ArbiterWeb.Api.RunControllerTest do assert entry["thinking"] == "medium" assert entry["difficulty_at_dispatch"] == 2 end + + # bd-b7e33c post-merge finding (2026-09-22): the 04:26Z production + # verification misread an agy run as having `session_id: NULL` / + # `provider: null` because `arb worker runs --json` (this endpoint) never + # surfaced those columns at all — they were silently dropped from the + # summary, not actually null in `worker_runs`. Surface them so a resume's + # conversation continuity (or lack of it) is directly observable from the + # CLI/API without reaching into the DB. + test "lists provider/session_id/resumed_from_run_id", %{conn: conn} do + now = DateTime.utc_now() + + prior = + insert_run!(%{ + task_id: "bd-session-fields", + provider: "gemini", + session_id: "25df47b0-054e-434e-84c1-6876fd9f77de", + started_at: DateTime.add(now, -600, :second) + }) + + _resumed = + insert_run!(%{ + task_id: "bd-session-fields", + provider: "gemini", + session_id: "89a2b784-6bd5-46e6-a971-2178ca58cdcd", + resumed_from_run_id: prior.id, + started_at: now + }) + + conn = get(conn, ~p"/api/workers/history", %{task_id: "bd-session-fields"}) + [resumed_entry, prior_entry] = json_response(conn, 200)["data"] + + assert prior_entry["provider"] == "gemini" + assert prior_entry["session_id"] == "25df47b0-054e-434e-84c1-6876fd9f77de" + assert prior_entry["resumed_from_run_id"] == nil + + assert resumed_entry["provider"] == "gemini" + assert resumed_entry["session_id"] == "89a2b784-6bd5-46e6-a971-2178ca58cdcd" + assert resumed_entry["resumed_from_run_id"] == prior.id + end end describe "GET /api/workers/history/:id" do