diff --git a/SPEC.md b/SPEC.md index f9e2b63a14..87e6645dd4 100644 --- a/SPEC.md +++ b/SPEC.md @@ -435,6 +435,9 @@ fields locally if they want stricter startup checks. - Default: implementation-defined. - `turn_sandbox_policy` (Codex `SandboxPolicy` value) - Default: implementation-defined. + - Runtime note: when the policy type is `workspaceWrite`, implementations should ensure the + current issue workspace remains writable even when callers add extra `writableRoots` for + linked-worktree metadata or similar adjunct paths. - `turn_timeout_ms` (integer) - Default: `3600000` (1 hour) - `read_timeout_ms` (integer) diff --git a/elixir/README.md b/elixir/README.md index 603b4bb000..5992980568 100644 --- a/elixir/README.md +++ b/elixir/README.md @@ -116,9 +116,11 @@ Notes: - `codex.turn_sandbox_policy` defaults to a `workspaceWrite` policy rooted at the current issue workspace - Supported `codex.approval_policy` values depend on the targeted Codex app-server version. In the current local Codex schema, string values include `untrusted`, `on-failure`, `on-request`, and `never`, and object-form `reject` is also supported. - Supported `codex.thread_sandbox` values: `read-only`, `workspace-write`, `danger-full-access`. -- When `codex.turn_sandbox_policy` is set explicitly, Symphony passes the map through to Codex - unchanged. Compatibility then depends on the targeted Codex app-server version rather than local - Symphony validation. +- When `codex.turn_sandbox_policy` is set explicitly, Symphony forwards the configured map to + Codex, but for `workspaceWrite` policies it ensures the current issue workspace stays in + `writableRoots` at runtime. This allows adding extra writable paths without granting access to + sibling workspaces by default. Compatibility for the remaining fields still depends on the + targeted Codex app-server version rather than local Symphony validation. - `agent.max_turns` caps how many back-to-back Codex turns Symphony will run in a single agent invocation when a turn completes normally but the issue is still in an active state. Default: `20`. - If the Markdown body is blank, Symphony uses a default prompt template that includes the issue diff --git a/elixir/lib/symphony_elixir/config/schema.ex b/elixir/lib/symphony_elixir/config/schema.ex index 17ead4ad6b..5e3cf17819 100644 --- a/elixir/lib/symphony_elixir/config/schema.ex +++ b/elixir/lib/symphony_elixir/config/schema.ex @@ -308,7 +308,7 @@ defmodule SymphonyElixir.Config.Schema do def resolve_runtime_turn_sandbox_policy(settings, workspace \\ nil, opts \\ []) do case settings.codex.turn_sandbox_policy do %{} = policy -> - {:ok, policy} + {:ok, ensure_workspace_write_root(policy, workspace, opts)} _ -> workspace @@ -505,6 +505,35 @@ defmodule SymphonyElixir.Config.Schema do {:error, {:unsafe_turn_sandbox_policy, {:invalid_workspace_root, workspace_root}}} end + defp ensure_workspace_write_root(%{"type" => "workspaceWrite"} = policy, workspace, opts) do + case runtime_workspace_write_root(workspace, opts) do + nil -> + policy + + workspace_root -> + writable_roots = + policy + |> Map.get("writableRoots", []) + |> normalize_writable_roots() + |> List.insert_at(0, workspace_root) + |> Enum.uniq() + + Map.put(policy, "writableRoots", writable_roots) + end + end + + defp ensure_workspace_write_root(policy, _workspace, _opts), do: policy + + defp runtime_workspace_write_root(workspace, opts) + when is_binary(workspace) and workspace != "" do + if Keyword.get(opts, :remote, false), do: workspace, else: Path.expand(workspace) + end + + defp runtime_workspace_write_root(_workspace, _opts), do: nil + + defp normalize_writable_roots(roots) when is_list(roots), do: roots + defp normalize_writable_roots(_roots), do: [] + defp default_workspace_root(workspace, _fallback) when is_binary(workspace) and workspace != "", do: workspace diff --git a/elixir/test/symphony_elixir/app_server_test.exs b/elixir/test/symphony_elixir/app_server_test.exs index d03627f8b3..1cdce493a5 100644 --- a/elixir/test/symphony_elixir/app_server_test.exs +++ b/elixir/test/symphony_elixir/app_server_test.exs @@ -164,6 +164,18 @@ defmodule SymphonyElixir.AppServerTest do trace = File.read!(trace_file) lines = String.split(trace, "\n", trim: true) + {:ok, canonical_workspace} = + SymphonyElixir.PathSafety.canonicalize(Path.expand(workspace)) + + expected_policy = + case configured_policy do + %{"type" => "workspaceWrite"} -> + Map.put(configured_policy, "writableRoots", [canonical_workspace, "relative/path"]) + + _ -> + configured_policy + end + assert Enum.any?(lines, fn line -> if String.starts_with?(line, "JSON:") do line @@ -171,7 +183,7 @@ defmodule SymphonyElixir.AppServerTest do |> Jason.decode!() |> then(fn payload -> payload["method"] == "turn/start" && - get_in(payload, ["params", "sandboxPolicy"]) == configured_policy + get_in(payload, ["params", "sandboxPolicy"]) == expected_policy end) else false diff --git a/elixir/test/symphony_elixir/core_test.exs b/elixir/test/symphony_elixir/core_test.exs index dcbe12a271..51422ad53e 100644 --- a/elixir/test/symphony_elixir/core_test.exs +++ b/elixir/test/symphony_elixir/core_test.exs @@ -1760,7 +1760,7 @@ defmodule SymphonyElixir.CoreTest do codex_thread_sandbox: "workspace-write", codex_turn_sandbox_policy: %{ type: "workspaceWrite", - writableRoots: [Path.expand(workspace), workspace_cache] + writableRoots: [workspace_cache] } ) @@ -1793,9 +1793,12 @@ defmodule SymphonyElixir.CoreTest do end end) + assert {:ok, canonical_workspace} = + SymphonyElixir.PathSafety.canonicalize(Path.expand(workspace)) + expected_turn_policy = %{ "type" => "workspaceWrite", - "writableRoots" => [Path.expand(workspace), workspace_cache] + "writableRoots" => [canonical_workspace, workspace_cache] } assert Enum.any?(lines, fn line -> diff --git a/elixir/test/symphony_elixir/workspace_and_config_test.exs b/elixir/test/symphony_elixir/workspace_and_config_test.exs index 59ff0850b1..7005bc0312 100644 --- a/elixir/test/symphony_elixir/workspace_and_config_test.exs +++ b/elixir/test/symphony_elixir/workspace_and_config_test.exs @@ -793,7 +793,7 @@ defmodule SymphonyElixir.WorkspaceAndConfigTest do codex_thread_sandbox: "workspace-write", codex_turn_sandbox_policy: %{ type: "workspaceWrite", - writableRoots: [explicit_workspace, explicit_cache] + writableRoots: [explicit_cache] } ) @@ -1138,7 +1138,7 @@ defmodule SymphonyElixir.WorkspaceAndConfigTest do assert runtime_settings.turn_sandbox_policy == %{ "type" => "workspaceWrite", - "writableRoots" => ["relative/path"], + "writableRoots" => [issue_workspace, "relative/path"], "networkAccess" => true } @@ -1161,6 +1161,74 @@ defmodule SymphonyElixir.WorkspaceAndConfigTest do end end + test "runtime sandbox policy resolution leaves explicit workspaceWrite roots unchanged without a workspace" do + settings = %Schema{ + codex: %Codex{ + turn_sandbox_policy: %{ + "type" => "workspaceWrite", + "writableRoots" => "not-a-list", + "networkAccess" => true + } + }, + workspace: %Schema.Workspace{root: "/tmp/ignored"} + } + + assert {:ok, policy} = Schema.resolve_runtime_turn_sandbox_policy(settings, nil) + + assert policy == %{ + "type" => "workspaceWrite", + "writableRoots" => "not-a-list", + "networkAccess" => true + } + end + + test "runtime sandbox policy resolution normalizes explicit workspaceWrite roots when needed" do + issue_workspace = "/tmp/MT-201" + + settings = %Schema{ + codex: %Codex{ + turn_sandbox_policy: %{ + "type" => "workspaceWrite", + "writableRoots" => "not-a-list", + "networkAccess" => true + } + }, + workspace: %Schema.Workspace{root: "/tmp/ignored"} + } + + assert {:ok, policy} = Schema.resolve_runtime_turn_sandbox_policy(settings, issue_workspace) + + assert policy == %{ + "type" => "workspaceWrite", + "writableRoots" => [issue_workspace], + "networkAccess" => true + } + end + + test "runtime sandbox policy resolution preserves explicit workspaceWrite roots for remote workers" do + remote_workspace = "/remote/workspaces/MT-200" + + settings = %Schema{ + codex: %Codex{ + turn_sandbox_policy: %{ + "type" => "workspaceWrite", + "writableRoots" => ["relative/path"], + "networkAccess" => true + } + }, + workspace: %Schema.Workspace{root: "/tmp/ignored"} + } + + assert {:ok, policy} = + Schema.resolve_runtime_turn_sandbox_policy(settings, remote_workspace, remote: true) + + assert policy == %{ + "type" => "workspaceWrite", + "writableRoots" => [remote_workspace, "relative/path"], + "networkAccess" => true + } + end + test "path safety returns errors for invalid path segments" do invalid_segment = String.duplicate("a", 300) path = Path.join(System.tmp_dir!(), invalid_segment)