diff --git a/CHANGELOG.md b/CHANGELOG.md index e6641a6ce..7dc627e31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,12 @@ upgrade, is in ### Fixed +- A teammate can be moved to a different environment or vault. Fountain retires + the computer the old binding named, so the teammate's next message builds one + from the new pair, and it refuses the move while a turn is running on that + computer. A move onto an environment and vault the agent already has a + computer for is refused rather than merged onto it (#1636). + - A conversation that shares a sandbox follows the replacement machine only when it declares the same environment and vault. The replacement is built from the waking conversation's pair, so a co-tenant that named a different diff --git a/apps/fountain/lib/fountain/conversations.ex b/apps/fountain/lib/fountain/conversations.ex index 11c803414..d18426b61 100644 --- a/apps/fountain/lib/fountain/conversations.ex +++ b/apps/fountain/lib/fountain/conversations.ex @@ -2807,8 +2807,9 @@ defmodule Fountain.Conversations do `opts[:reason]` says *why*, and reaches every transcript on the machine and the audit row: `"home_reset"` (the owner asked — the default), - `"environment_changed"`, `"environment_deleted"` or `"vault_deleted"` when - the identity moved out from under the home (#1084). + `"environment_changed"`, `"environment_deleted"`, `"vault_deleted"` or + `"teammate_rebound"` when the identity moved out from under the home + (#1084, #1636). See `create_agent/2` for the rest of `opts` (`:actor`, `:request_ip`). """ @@ -2915,6 +2916,11 @@ defmodule Fountain.Conversations do defp reset_message("vault_deleted"), do: "The vault this machine was built for was deleted. " <> @reset_tail + defp reset_message("teammate_rebound"), + do: + "The teammate moved to a different environment or vault, so this machine is no " <> + "longer its " <> @reset_tail + defp reset_message(_owner), do: "The sandbox was reset by its owner. " <> @reset_tail # A conversation on a machine the caller already has (ADR 0023 gate 3). diff --git a/apps/fountain/lib/fountain/team.ex b/apps/fountain/lib/fountain/team.ex index 773789ca9..2b77df166 100644 --- a/apps/fountain/lib/fountain/team.ex +++ b/apps/fountain/lib/fountain/team.ex @@ -41,7 +41,7 @@ defmodule Fountain.Team do require Logger alias Fountain.{Agents, Audit, Conversations, Repo} - alias Fountain.Conversations.{Conversation, ConversationServer, Turn} + alias Fountain.Conversations.{Conversation, ConversationServer, Sandbox, Turn} @channel "fountain:team" @@ -448,6 +448,194 @@ defmodule Fountain.Team do end end + # What a teammate is, in one map: the public attribute name, and the + # conversation column it lands on. + @bindings [{"name", :title}, {"environment_id", :environment_id}, {"vault_id", :vault_id}] + + @doc """ + Reconcile the teammate for `agent_id` against `attrs` (#1636). + + `attrs` is string-keyed and takes the same three keys `add_teammate/4` + does: `"name"`, `"environment_id"` and `"vault_id"`. A key that is absent + leaves that binding alone; a blank value clears it, which for the two ids + means the agent's own environment and no vault. Bulk apply + (`Fountain.Manifest`) always sends all three, so a Teammate document that + names no environment clears the override rather than keeping the last one. + Both ids go through the agent's allowlists, as an add does, so a teammate + cannot be bound to an environment or vault its agent refuses. + + The three live on the teammate's current conversation, where they already + are, so `open_fresh_conversation/3` and `start_fresh/6` build the next + computer from them. + + A home is keyed on `(user, agent, environment, vault)`, so moving either id + moves the teammate's computer out from under it: the next launch looks + under the new key, finds nothing and provisions a fresh machine, while the + old one stays `ready` holding a concurrency slot and a disk carrying the + old environment's secrets (#1084). This is the hazard + `Fountain.Agents.update_agent/3` refuses, and it is refused the same way — + `{:error, :sandbox_mid_turn}` while a turn is running on that machine, and + the orphan retired through `reset_sandbox/2` once the new binding is the + committed one. An ephemeral computer is a conversation's own and is left + alone. + + A rebinding onto an identity the agent already has a live home for is + refused with `{:error, :destination_home_occupied}`, and nothing is written. + There is one home per `(user, agent, environment, vault)`, and the wake path + builds a home rather than attaching to one, so writing the binding anyway + would leave a teammate that cannot wake at all. Merging the teammate onto + the machine that is already there is deliberately not done here; it needs + the readiness, runtime and quota checks `attach_conversation/3` makes. + + Returns `{:ok, conv, :updated}`, or `{:ok, conv, :unchanged}` when `attrs` + matched the teammate already. `{:error, :not_found}` when the agent is not + on the team, `{:error, :environment_not_allowed}` / `{:error, + :vault_not_allowed}` when an id is not the caller's own or not on the + agent's allowlist, `{:error, :sandbox_mid_turn}` and `{:error, + :destination_home_occupied}` as above. Audited as `team.updated` with the + changed field names, and nothing is recorded when nothing changed. + + The second argument is the agent's id, or the teammate map a caller already + holds from `get_teammate/2` or `list_teammates/1` for the same `user_id` — + listing the roster is several queries, and bulk apply has just done it. + """ + def update_teammate(user_id, agent_or_teammate, attrs, opts \\ []) + + def update_teammate(user_id, agent_id, attrs, opts) + when is_binary(user_id) and is_binary(agent_id) and is_map(attrs) do + case get_teammate(user_id, agent_id) do + nil -> {:error, :not_found} + teammate -> update_teammate(user_id, teammate, attrs, opts) + end + end + + def update_teammate(user_id, %{agent: agent, conversation: conv}, attrs, opts) + when is_binary(user_id) and is_map(attrs) and is_list(opts) do + changes = binding_changes(attrs, conv) + identity = effective_identity(conv, agent, changes) + # Ownership: `conv` and `agent` came from the scoped get_teammate. + orphans = homes_orphaned_by_rebinding(conv, identity) + + with :ok <- bindings_allowed(user_id, agent, changes), + :ok <- destination_free(user_id, agent, conv, identity), + :ok <- no_home_mid_turn(orphans) do + write_bindings(user_id, conv, changes, orphans, opts) + end + end + + # Only what actually moves: a value the conversation already holds is not a + # change, which is what lets a re-apply say it wrote nothing. + defp binding_changes(attrs, %Conversation{} = conv) do + @bindings + |> Enum.filter(fn {key, _field} -> Map.has_key?(attrs, key) end) + |> Enum.map(fn {key, field} -> {field, blank_to_nil(attrs[key])} end) + |> Enum.reject(fn {field, value} -> Map.get(conv, field) == value end) + |> Map.new() + end + + # The pair a machine for this teammate would be built from once `changes` + # land. A cleared override falls back to the agent's own environment, which + # is what a sandbox row carries and what `_unsafe_find_home/4` looks up by. + defp effective_identity(%Conversation{} = conv, %Agents.Agent{} = agent, changes) do + {Map.get(changes, :environment_id, conv.environment_id) || agent.environment_id, + Map.get(changes, :vault_id, conv.vault_id)} + end + + # The teammate's computer, when the new binding no longer names it. Nothing + # moves for a name-only change, and nothing is orphaned by a rebinding that + # keeps the same pair. + defp homes_orphaned_by_rebinding(%Conversation{sandbox: %Sandbox{} = home}, identity) do + if home.mode == "persistent" and home.status not in ["terminated", "failed"] and + {home.environment_id, home.vault_id} != identity do + [home] + else + [] + end + end + + defp homes_orphaned_by_rebinding(_conv, _identity), do: [] + + # One live home per identity, enforced by `sandboxes_home_identity_index`. + # If the agent already has a home for the pair this rebinding moves to, the + # teammate would be written onto an identity it cannot wake into: the wake + # path provisions a *new* home rather than attaching to an existing one, and + # the index rejects the insert, so the teammate is stranded and re-applying + # the same manifest reports `unchanged` and does not recover it. + # + # Refused rather than merged. Attaching to the machine that is already there + # is the other half of this and needs its own change — readiness, the + # runtime the disk was shaped for, and the quota a second tenant of that + # machine implies are all checks `attach_conversation/3` makes and this + # function does not. Refusing cannot strand anybody; attaching wrongly can. + # + # Ownership: `agent` and `conv` came from the scoped get_teammate, and a + # home carries the same `user_id` as the identity it is keyed on. + defp destination_free(user_id, %Agents.Agent{} = agent, %Conversation{} = conv, identity) do + {env_id, vault_id} = identity + + if identity == effective_identity(conv, agent, %{}) do + :ok + else + case Conversations._unsafe_find_home(user_id, agent.id, env_id, vault_id) do + nil -> :ok + %Sandbox{id: id} when id == conv.sandbox_id -> :ok + %Sandbox{} -> {:error, :destination_home_occupied} + end + end + end + + # Asked before anything is written, so a mid-turn refusal costs the caller + # nothing. Ownership: the homes came from the scoped get_teammate's + # conversation. + defp no_home_mid_turn(homes) do + if Conversations._unsafe_any_home_mid_turn?(homes), + do: {:error, :sandbox_mid_turn}, + else: :ok + end + + defp bindings_allowed(user_id, %Agents.Agent{} = agent, changes) do + options = addable_options(user_id, agent) + + with :ok <- + binding_allowed( + changes, + :environment_id, + options.environments, + :environment_not_allowed + ) do + binding_allowed(changes, :vault_id, options.vaults, :vault_not_allowed) + end + end + + defp binding_allowed(changes, field, allowed, refusal) do + case Map.get(changes, field) do + nil -> :ok + id -> if Enum.any?(allowed, &(&1.id == id)), do: :ok, else: {:error, refusal} + end + end + + defp write_bindings(_user_id, conv, changes, _orphans, _opts) when map_size(changes) == 0, + do: {:ok, conv, :unchanged} + + defp write_bindings(user_id, conv, changes, orphans, opts) do + case Conversations.update_conversation(conv, changes) do + {:ok, updated} -> + fields = for {key, field} <- @bindings, Map.has_key?(changes, field), do: key + record(user_id, "team.updated", updated, opts, %{"fields" => fields}) + + # Only once the new binding is the committed one: a machine torn down + # against a write that then failed would be rebuilt for nothing. + # Ownership: established above, by the scoped get_teammate. + _ = Conversations._unsafe_retire_orphaned_homes(orphans, "teammate_rebound", opts) + + broadcast_changed(user_id) + {:ok, updated, :updated} + + {:error, _} = err -> + err + end + end + @doc """ Open a fresh conversation for the teammate on its current computer. diff --git a/apps/fountain/test/fountain/audit_guardrail_test.exs b/apps/fountain/test/fountain/audit_guardrail_test.exs index b7eb0cd60..75badca8c 100644 --- a/apps/fountain/test/fountain/audit_guardrail_test.exs +++ b/apps/fountain/test/fountain/audit_guardrail_test.exs @@ -87,6 +87,7 @@ defmodule Fountain.AuditGuardrailTest do {"team member add", &__MODULE__.do_team_add/1, "team.member.added"}, {"team member remove", &__MODULE__.do_team_remove/1, "team.member.removed"}, {"team member rename", &__MODULE__.do_team_rename/1, "team.renamed"}, + {"team member rebind", &__MODULE__.do_team_update/1, "team.updated"}, {"team conversation rotate", &__MODULE__.do_team_rotate/1, "team.conversation.rotated"}, # Team schedules: a cron that runs a teammate with a prompt. A run leaves # conversation events underneath; `.fired` is the schedule-side record. @@ -456,6 +457,21 @@ defmodule Fountain.AuditGuardrailTest do {:ok, _} = Fountain.Team.rename_teammate(user.id, agent.id, "Renamed") end + def do_team_update(user) do + agent = insert_agent(user_id: user.id) + vault = insert_vault(user_id: user.id) + + insert_conversation( + user_id: user.id, + agent: agent, + status: "idle", + channel_id: Fountain.Team.channel() + ) + + {:ok, _, :updated} = + Fountain.Team.update_teammate(user.id, agent.id, %{"vault_id" => vault.id}) + end + def do_team_rotate(user) do agent = insert_agent(user_id: user.id) sandbox = insert_sandbox(user_id: user.id, status: "ready") diff --git a/apps/fountain/test/fountain/team_test.exs b/apps/fountain/test/fountain/team_test.exs index 14b9af0c9..822fc731b 100644 --- a/apps/fountain/test/fountain/team_test.exs +++ b/apps/fountain/test/fountain/team_test.exs @@ -192,6 +192,356 @@ defmodule Fountain.TeamTest do end end + describe "update_teammate/4" do + test "moves the name, the environment and the vault, and records the fields" do + user = insert_verified_user() + agent = insert_agent(user_id: user.id, name: "Ada") + env = insert_env(user_id: user.id) + vault = insert_vault(user_id: user.id) + conv = insert_teammate_conv(user, agent) + + assert {:ok, updated, :updated} = + Team.update_teammate( + user.id, + agent.id, + %{ + "name" => " Ada (staging) ", + "environment_id" => env.id, + "vault_id" => vault.id + }, + actor: "api" + ) + + assert updated.id == conv.id + assert updated.title == "Ada (staging)" + assert updated.environment_id == env.id + assert updated.vault_id == vault.id + + assert [%{name: "Ada (staging)"}] = Team.list_teammates(user.id) + + assert event = + Enum.find(Audit.list_recent_for_user(user.id, 20), &(&1.action == "team.updated")) + + assert event.actor == "api" + assert Enum.sort(event.metadata["fields"]) == ["environment_id", "name", "vault_id"] + end + + test "a blank value clears the binding; an absent key leaves it alone" do + user = insert_verified_user() + agent = insert_agent(user_id: user.id) + env = insert_env(user_id: user.id) + vault = insert_vault(user_id: user.id) + insert_teammate_conv(user, agent, environment_id: env.id, vault_id: vault.id, title: "Ada") + + assert {:ok, updated, :updated} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => ""}) + + assert updated.environment_id == nil + assert updated.vault_id == vault.id + assert updated.title == "Ada" + end + + test "records nothing when nothing moves" do + user = insert_verified_user() + agent = insert_agent(user_id: user.id) + insert_teammate_conv(user, agent, title: "Ada") + before = length(Audit.list_recent_for_user(user.id, 50)) + + assert {:ok, _, :unchanged} = Team.update_teammate(user.id, agent.id, %{"name" => "Ada"}) + assert length(Audit.list_recent_for_user(user.id, 50)) == before + end + + test "the agent's allowlists gate the environment and the vault" do + user = insert_verified_user() + env = insert_env(user_id: user.id) + vault = insert_vault(user_id: user.id) + + agent = + insert_agent(user_id: user.id, allowed_environment_ids: [], allowed_vault_ids: []) + + insert_teammate_conv(user, agent) + + assert {:error, :environment_not_allowed} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => env.id}) + + assert {:error, :vault_not_allowed} = + Team.update_teammate(user.id, agent.id, %{"vault_id" => vault.id}) + end + + test "another tenant's environment or vault is refused" do + user = insert_verified_user() + other = insert_verified_user() + agent = insert_agent(user_id: user.id) + insert_teammate_conv(user, agent) + + assert {:error, :environment_not_allowed} = + Team.update_teammate(user.id, agent.id, %{ + "environment_id" => insert_env(user_id: other.id).id + }) + + assert {:error, :vault_not_allowed} = + Team.update_teammate(user.id, agent.id, %{ + "vault_id" => insert_vault(user_id: other.id).id + }) + end + + # #1084 from the teammate's side: a home is keyed on (user, agent, + # environment, vault), so rebinding a teammate moves its computer out from + # under it. Refused mid-turn, and retired once the new binding is written. + test "the computer the binding moved away from is retired" do + user = insert_active_user() + env = insert_env(user_id: user.id) + other = insert_env(user_id: user.id) + agent = insert_agent(user_id: user.id, environment_id: env.id, sandbox_mode: "persistent") + + home = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + insert_teammate_conv(user, agent, sandbox: home, environment_id: env.id) + + test = self() + + stub(Managoat.Sandbox.Sprites, :destroy, fn h -> send(test, {:destroyed, h.name}) && :ok end) + + assert {:ok, _, :updated} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => other.id}) + + assert_received {:destroyed, name} + assert name == home.sprite_name + assert Conversations._unsafe_get_sandbox!(home.id).status == "terminated" + end + + test "a rebinding is refused while a turn runs on that computer" do + user = insert_active_user() + env = insert_env(user_id: user.id) + other = insert_env(user_id: user.id) + agent = insert_agent(user_id: user.id, environment_id: env.id, sandbox_mode: "persistent") + + home = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + conv = + insert_teammate_conv(user, agent, + sandbox: home, + environment_id: env.id, + status: "running" + ) + + insert_turn(conv, status: "running") + + assert {:error, :sandbox_mid_turn} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => other.id}) + + # The refusal is the whole answer: the binding did not move and the + # machine is still there. + assert Conversations.get_conversation(conv.id, user.id).environment_id == env.id + assert Conversations._unsafe_get_sandbox!(home.id).status == "ready" + end + + test "a name-only change leaves the computer alone, mid-turn or not" do + user = insert_active_user() + env = insert_env(user_id: user.id) + agent = insert_agent(user_id: user.id, environment_id: env.id, sandbox_mode: "persistent") + + home = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + conv = + insert_teammate_conv(user, agent, + sandbox: home, + environment_id: env.id, + status: "running" + ) + + insert_turn(conv, status: "running") + + assert {:ok, _, :updated} = Team.update_teammate(user.id, agent.id, %{"name" => "Ada"}) + assert Conversations._unsafe_get_sandbox!(home.id).status == "ready" + end + + test "an ephemeral computer is not a home and is left standing" do + user = insert_active_user() + env = insert_env(user_id: user.id) + other = insert_env(user_id: user.id) + agent = insert_agent(user_id: user.id, environment_id: env.id) + + sandbox = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "ephemeral", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + insert_teammate_conv(user, agent, sandbox: sandbox, environment_id: env.id) + + assert {:ok, _, :updated} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => other.id}) + + assert Conversations._unsafe_get_sandbox!(sandbox.id).status == "ready" + end + + # #1636: the teammate's home may be shared. Retiring it must not let + # whichever conversation wakes first decide what the other one runs on. + test "a co-tenant of the retired computer keeps its own binding, either wake order" do + for order <- [:teammate_first, :cotenant_first] do + user = insert_active_user() + {:ok, user} = Fountain.Accounts.update_sandbox_limit(user, 10) + env = insert_env(user_id: user.id) + other = insert_env(user_id: user.id) + + agent = + insert_agent( + user_id: user.id, + runtime: "claude", + environment_id: env.id, + sandbox_mode: "persistent" + ) + + home = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + mate = insert_teammate_conv(user, agent, sandbox: home) + + cotenant = + insert_conversation(user_id: user.id, agent: agent, sandbox: home, status: "idle") + + stub(Managoat.Sandbox.Sprites, :destroy, fn _h -> :ok end) + + assert {:ok, _, :updated} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => other.id}) + + [first, second] = + if order == :teammate_first, do: [mate, cotenant], else: [cotenant, mate] + + {:ok, _} = Conversations.wake_conversation(first.id) + {:ok, _} = Conversations.wake_conversation(second.id) + + mate_sandbox = + Conversations._unsafe_get_conversation!(mate.id).sandbox_id + |> Conversations._unsafe_get_sandbox!() + + cotenant_sandbox = + Conversations._unsafe_get_conversation!(cotenant.id).sandbox_id + |> Conversations._unsafe_get_sandbox!() + + assert mate_sandbox.environment_id == other.id, + "#{order}: the teammate ran on #{inspect(mate_sandbox.environment_id)}" + + assert cotenant_sandbox.environment_id == env.id, + "#{order}: the co-tenant ran on #{inspect(cotenant_sandbox.environment_id)}" + + refute mate_sandbox.id == cotenant_sandbox.id + end + end + + # One live home per (user, agent, environment, vault). Writing the binding + # anyway would leave a teammate whose next wake cannot insert its home and + # cannot attach to the one that is there, so the rebind is refused whole. + test "a rebinding onto an identity that already has a computer is refused" do + user = insert_active_user() + env = insert_env(user_id: user.id) + other = insert_env(user_id: user.id) + + agent = + insert_agent(user_id: user.id, environment_id: env.id, sandbox_mode: "persistent") + + home = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + occupied = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: other.id, + provider: "sprites" + ) + + conv = insert_teammate_conv(user, agent, sandbox: home) + + assert {:error, :destination_home_occupied} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => other.id}) + + # Nothing was written and nothing was retired. + assert Conversations.get_conversation(conv.id, user.id).environment_id == nil + assert Conversations._unsafe_get_sandbox!(home.id).status == "ready" + assert Conversations._unsafe_get_sandbox!(occupied.id).status == "ready" + assert [%{name: name}] = Team.list_teammates(user.id) + assert name == agent.name + end + + test "a rebinding onto the identity of the computer it is already on is allowed" do + user = insert_active_user() + env = insert_env(user_id: user.id) + agent = insert_agent(user_id: user.id, sandbox_mode: "persistent") + + home = + insert_sandbox( + user_id: user.id, + status: "ready", + mode: "persistent", + agent_id: agent.id, + environment_id: env.id, + provider: "sprites" + ) + + insert_teammate_conv(user, agent, sandbox: home, environment_id: env.id) + + # Naming the same environment explicitly is not a move, so the home it + # is sitting on is not "occupied by something else". + assert {:ok, _, :unchanged} = + Team.update_teammate(user.id, agent.id, %{"environment_id" => env.id}) + + assert Conversations._unsafe_get_sandbox!(home.id).status == "ready" + end + + test "an agent that is not on the team is not found" do + user = insert_verified_user() + agent = insert_agent(user_id: user.id) + + assert {:error, :not_found} = Team.update_teammate(user.id, agent.id, %{"name" => "x"}) + end + end + describe "addable_options/2" do test "the user's environments and vaults, narrowed by the agent's allowlists" do user = insert_verified_user()