diff --git a/.github/workflows/sandbox-isolation.yml b/.github/workflows/sandbox-isolation.yml index 5239cfb9d..d55a5efad 100644 --- a/.github/workflows/sandbox-isolation.yml +++ b/.github/workflows/sandbox-isolation.yml @@ -234,8 +234,8 @@ jobs: executed=$(grep -oE 'executed="[0-9]+"' "$trx" | head -1 | grep -oE '[0-9]+') passed=$(grep -oE 'passed="[0-9]+"' "$trx" | head -1 | grep -oE '[0-9]+') echo "executed=${executed:-0} passed=${passed:-0}" - if [ "${executed:-0}" -lt 90 ]; then - echo "::error::Expected >=90 sandbox isolation tests to run (bwrap/prlimit confinement + cap-drop + cgroup-namespace re-root + egress-allowlist filter + cgroup resource cap + durable-launch cgroup wiring + argv/envp per-string kernel ceiling + a prompt past it riding stdin + a read-only workspace mount + a network-off run reaching its broker through the relay and nothing else + an allowlist run relayed to its broker + a read-only reviewer reading its diff with the real CLIs + a bwrap probe that runs the launch argv + the MCP helper bound file by file behind a read-only socket dir + a CLI reaching its broker socket through the relay + a severed child reaching its broker over the lease socket across a worker restart + a pre-relay namespaced run re-bound at its gateway and torn down with its seal + an allowlist run's veth guarded both ways, a flow the worker opened before the run included + forwarding a root worker may not write named before an allowlist is planned + an allowlist run's port 53 open only at the resolvers of the resolv.conf its namespace reads, and still to a resolver address the worker's own NAT rewrites before its forward and its input hooks + a target repository's own CLI config kept out of the run with the real CLIs, every repository's memory read in a multi-repo workspace included), but only ${executed:-0} did — the Category=Sandbox filter matched too few (trait regression?). If a case was deliberately removed, lower this number in the same PR." + if [ "${executed:-0}" -lt 92 ]; then + echo "::error::Expected >=92 sandbox isolation tests to run (bwrap/prlimit confinement + cap-drop + cgroup-namespace re-root + egress-allowlist filter + cgroup resource cap + durable-launch cgroup wiring + argv/envp per-string kernel ceiling + a prompt past it riding stdin + a read-only workspace mount + a network-off run reaching its broker through the relay and nothing else + an allowlist run relayed to its broker + a read-only reviewer reading its diff with the real CLIs + a bwrap probe that runs the launch argv + the MCP helper bound file by file behind a read-only socket dir + a CLI reaching its broker socket through the relay + a severed child reaching its broker over the lease socket across a worker restart + a pre-relay namespaced run re-bound at its gateway and torn down with its seal + an allowlist run's veth guarded both ways, a flow the worker opened before the run included + forwarding a root worker may not write named before an allowlist is planned + an allowlist run's port 53 open only at the resolvers of the resolv.conf its namespace reads, and still to a resolver address the worker's own NAT rewrites before its forward and its input hooks + a target repository's own CLI config kept out of the run with the real CLIs, every repository's memory read in a multi-repo workspace included + a multi-repo Codex run starting at a workspace root that is no git repository and loading no config from it + a repo-less Codex run starting in a scratch directory that is no git repository), but only ${executed:-0} did — the Category=Sandbox filter matched too few (trait regression?). If a case was deliberately removed, lower this number in the same PR." exit 1 fi @@ -268,12 +268,12 @@ jobs: print(f'All {len(arms)} reviewer E2E arms ran and passed.') # The repository-config E2E is armed by the same CLI pins and returns early the same way; require each arm's marker. - for arm in ('claude-code single-repo Confined', 'claude-code multi-repo Confined', 'codex-cli'): + for arm in ('claude-code single-repo Confined', 'claude-code multi-repo Confined', 'codex-cli single-repo', 'codex-cli multi-repo', 'codex-cli scratch'): assert f'[repo-config-e2e] ran {arm}' in text, f'repository-config E2E arm "{arm}" did not run — check CODESPACE_REQUIRE_REVIEW_CLIS and the CLI install step' - for method in ('A_claude_run_ignores_the_settings_its_repository_commits_and_still_reads_its_memory', 'A_multi_repo_claude_run_reads_every_repositorys_memory_and_none_of_its_settings', 'A_codex_run_ignores_the_config_and_hooks_its_repository_commits_and_still_reads_its_agents_md'): + for method in ('A_claude_run_ignores_the_settings_its_repository_commits_and_still_reads_its_memory', 'A_multi_repo_claude_run_reads_every_repositorys_memory_and_none_of_its_settings', 'A_codex_run_ignores_the_config_and_hooks_its_repository_commits_and_still_reads_its_agents_md', 'A_multi_repo_codex_run_starts_at_a_workspace_root_that_is_no_repository_and_loads_no_config_from_it', 'A_repo_less_codex_run_starts_in_a_scratch_directory_that_is_no_repository'): cases = [r for r in results if 'RepositoryConfigE2ETests.' + method in r.get('testName', '')] assert len(cases) == 1 and cases[0].get('outcome') == 'Passed', f'{method}: must pass' - print('All 3 repository-config E2E arms ran and passed.') + print('All 5 repository-config E2E arms ran and passed.') # The sealed-egress E2E returns early on a host that cannot confine, which reads as Passed; require each arm's marker. for arm in ('durable', 'non-durable', 'relay-ipv6', 'restart', 'relay-refused', 'relay-policy-route'): @@ -398,10 +398,11 @@ jobs: # docker-compose.yml ships the worker image with none of the grants bubblewrap needs: uid 1654 with ip and nft # installed, nothing confining, RequireConfinement off. A confining worker severs an allowlist it cannot filter; # here nothing would enforce that, so Category=SandboxUnconfined pins that such a run is still planned into its - # namespace and aborted at the setup, never launched on the worker's network. It runs as that uid with - # CODESPACE_BWRAP_PATH naming no binary and RequireConfinement unset — the one lane that must not confine — - # and each arm first asserts exactly that posture. Reuses the home, packages and build output the non-root step - # made readable to that uid. + # namespace and aborted at the setup, never launched on the worker's network. Unconfined, Codex's own sandbox is + # the only boundary its commands meet, so the lane also pins that a multi-repo Codex run's agent can write no + # repository's .git or .codex. It runs as that uid with CODESPACE_BWRAP_PATH naming no binary and + # RequireConfinement unset — the one lane that must not confine — and each arm first asserts exactly that + # posture. Reuses the home, packages and build output the non-root step made readable to that uid. shell: bash run: | set -euo pipefail @@ -427,9 +428,9 @@ jobs: root = ET.parse(path).getroot() counters = root.find('.//{*}Counters') executed, passed = int(counters.get('executed')), int(counters.get('passed')) - assert executed >= 3 and passed == executed, f'expected all 3 unconfined arms to run and pass, got executed={executed} passed={passed}' + assert executed >= 4 and passed == executed, f'expected all 4 unconfined arms to run and pass, got executed={executed} passed={passed}' text = open(path, encoding='utf-8').read() - for marker in ('[unconfined-e2e] ran allowlist-never-unfiltered durable uid=1654', '[unconfined-e2e] ran allowlist-never-unfiltered one-shot uid=1654', '[unconfined-e2e] ran admission uid=1654'): + for marker in ('[unconfined-e2e] ran allowlist-never-unfiltered durable uid=1654', '[unconfined-e2e] ran allowlist-never-unfiltered one-shot uid=1654', '[unconfined-e2e] ran admission uid=1654', '[repo-config-e2e] ran unconfined codex-cli multi-repo metadata-read-only uid=1654'): assert marker in text, f'unconfined arm marker "{marker}" is missing — the arm returned early or did not run as the worker uid' print(f'All {executed} unconfined arms ran as uid 1654 and passed.') PY diff --git a/backend/src/CodeSpace.Core/Services/Agents/Harnesses/Codex/CodexHarness.cs b/backend/src/CodeSpace.Core/Services/Agents/Harnesses/Codex/CodexHarness.cs index 77e9fa14c..91a71f8cc 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Harnesses/Codex/CodexHarness.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Harnesses/Codex/CodexHarness.cs @@ -145,6 +145,20 @@ public sealed class CodexHarness : IAgentHarness, IAgentHarnessBinary, IAgentHar /// public const int MaxInputCharacters = 1_048_576; + /// + /// Lets Codex start in a directory that is not inside a git repository. Two kinds of run have such a cwd: a + /// multi-repo run whose cwd mode is Auto or WorkspaceRoot, which works at the workspace root holding + /// each repository in a folder of its own, and a repo-less run, which works in a scratch directory with no git + /// anywhere above it. (A single-repo workspace is cloned into its root, so its cwd is a repository in every mode.) + /// Without this flag the pinned 0.142.2 refuses both before any model request: exit 1, "Not inside a trusted + /// directory and --skip-git-repo-check was not specified". It does so for exec and exec resume <id> + /// alike, and no projects trust entry lifts the refusal. Every run passes it, because CodeSpace decides which + /// directory a run works in, not Codex's git heuristic. Inside a repository it changes nothing the model is sent: the + /// repository's AGENTS.md and skills still load, and still keeps its + /// project config out. Verified against 0.142.2, which accepts the flag after resume <id> too. + /// + private const string SkipGitRepoCheck = "--skip-git-repo-check"; + public SandboxSpec BuildInvocation(AgentTask task) { EnsureWithinInputCap(task.Goal); @@ -153,8 +167,8 @@ public SandboxSpec BuildInvocation(AgentTask task) // prior thread. The subcommand must follow `exec` directly; --model, the `-c` overrides (incl. the sandbox on // the resume path — see AppendSandbox), and the stdin `-` positional follow. Null (a fresh run) → the plain seed. var args = task.ResumeFromSessionId is { Length: > 0 } resumeThreadId - ? new List { "exec", "resume", resumeThreadId, "--json" } - : new List { "exec", "--json" }; + ? new List { "exec", "resume", resumeThreadId, "--json", SkipGitRepoCheck } + : new List { "exec", "--json", SkipGitRepoCheck }; // task.Tools is intentionally NOT projected here: Codex has no global tool allow-list (it restricts via // --sandbox + per-MCP-server enabled_tools), so a Claude-Code-style tool list has no faithful Codex flag. @@ -170,6 +184,7 @@ public SandboxSpec BuildInvocation(AgentTask task) } AppendSandbox(args, task); + AppendRepositoryWritableRoots(args, task); // Point Codex at a custom gateway (when one was projected) BEFORE the `-` positional — Codex parses `-c` // overrides as flags, so they must precede it. @@ -646,6 +661,32 @@ private static string SandboxMode(AgentPermissions permissions) => private static string[] SandboxFragment(AgentTask task, string mode) => task.ResumeFromSessionId is { Length: > 0 } ? new[] { "-c", $"sandbox_mode={mode}" } : new[] { "--sandbox", mode }; + /// + /// Name every repository below the cwd to Codex's workspace-write sandbox as a writable root of its own. That sandbox + /// keeps .git, .codex and .agents read-only only at the top of each writable root, so at a + /// multi-repo root, whose repositories sit below the cwd rather than at the top of a root, each repository's + /// .git/hooks and .git/config were writable to the agent. The platform's own commit and push then run + /// git in each repository with the run's credential. Named as roots, the pinned 0.142.2 refuses those writes, on + /// exec and exec resume alike, and the agent can still change each repository's files (observed under + /// macOS's sandbox; the unconfined sandbox lane runs the same check on Linux). No write access is added, since each + /// directory is already inside the cwd; a repository outside it is left out, because naming it would widen the + /// sandbox. A single-repo run's repository is its cwd, so its argv is unchanged. A read-only run has no writable root + /// to add to. Under our confinement Codex's sandbox is stood down () and ignores this + /// table. + /// + private static void AppendRepositoryWritableRoots(List args, AgentTask task) + { + if (task.Permissions.WriteScope == AgentWriteScope.ReadOnly || string.IsNullOrWhiteSpace(task.WorkspaceDirectory)) return; + + var inside = Path.TrimEndingDirectorySeparator(task.WorkspaceDirectory) + Path.DirectorySeparatorChar; + var repositories = (task.WorkspaceRepositoryDirectories ?? []).Where(directory => directory.StartsWith(inside, StringComparison.Ordinal)).Distinct(StringComparer.Ordinal).ToList(); + + if (repositories.Count == 0) return; + + args.Add("-c"); + args.Add($"sandbox_workspace_write.writable_roots=[{string.Join(',', repositories.Select(McpDeclarationWriter.TomlString))}]"); + } + /// What the runner swaps the sandbox fragment for where it confines the run: the same spelling, carrying . private static ArgsSubstitution SandboxStandDown(AgentTask task) => new() { Replace = SandboxFragment(task, SandboxMode(task.Permissions)), With = SandboxFragment(task, ConfinedSandboxMode) }; diff --git a/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexInjectionE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexInjectionE2ETests.cs index e9526e7ef..0a552d152 100644 --- a/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexInjectionE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexInjectionE2ETests.cs @@ -216,7 +216,7 @@ private async Task SeedAgentCredentialAsync(Guid teamId, string baseUrl, s return credId; } - /// A fresh git-initialised temp workspace — codex exec refuses to run outside a trusted git repo, and the executor provisions NO workspace for a no-repo task, so the test supplies one (mirrors ). Tracked for teardown. + /// A fresh git-initialised temp workspace the test supplies (mirrors ). Tracked for teardown. The repository is this test's choice, not the CLI's: every Codex run passes --skip-git-repo-check, so a repo-less run's scratch directory starts too. private string NewGitWorkspace() { var ws = Path.Combine(Path.GetTempPath(), "cs-codex-inject-" + Guid.NewGuid().ToString("N"), "ws"); diff --git a/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexStopHookE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexStopHookE2ETests.cs index 884943967..e7a3110e8 100644 --- a/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexStopHookE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Workflows/RealModelCodexStopHookE2ETests.cs @@ -145,7 +145,7 @@ private async Task SeedAgentCredentialAsync(Guid teamId, string baseUrl, s return credId; } - /// A fresh git-initialised temp workspace — codex exec refuses to run outside a trusted git repo (mirrors 's own helper). Tracked for teardown. + /// A fresh git-initialised temp workspace the test supplies (mirrors 's own helper). Tracked for teardown. The repository is this test's choice, not the CLI's: every Codex run passes --skip-git-repo-check. private string NewGitWorkspace() { var ws = Path.Combine(Path.GetTempPath(), "cs-codex-stophook-" + Guid.NewGuid().ToString("N"), "ws"); diff --git a/backend/tests/CodeSpace.SandboxTests/RepositoryConfigE2ETests.cs b/backend/tests/CodeSpace.SandboxTests/RepositoryConfigE2ETests.cs index 730d419b7..5b51a30a9 100644 --- a/backend/tests/CodeSpace.SandboxTests/RepositoryConfigE2ETests.cs +++ b/backend/tests/CodeSpace.SandboxTests/RepositoryConfigE2ETests.cs @@ -8,6 +8,7 @@ using CodeSpace.Core.Services.Agents.Harnesses.Codex; using CodeSpace.Core.Services.Agents.Sandbox.Isolation; using CodeSpace.Core.Services.Agents.Sandbox.Runners; +using CodeSpace.Core.Services.Agents.Workspace; using CodeSpace.Messages.Agents; using CodeSpace.Messages.Enums; using Shouldly; @@ -21,7 +22,12 @@ namespace CodeSpace.SandboxTests; /// Codex's .codex/config.toml and .codex/hooks.json — and the target repository is untrusted input. A file /// committed there must not take the run's model call off its broker, and must not run a command the model never /// chose. The repository's own instructions (CLAUDE.md, AGENTS.md) are context, not config, and must -/// still reach the model — from every repository of a multi-repo workspace. +/// still reach the model. For Claude that holds for every repository of a multi-repo workspace. The multi-repo Codex +/// arm asserts less: the run starts at the workspace root, reaches its broker and loads no config from that root. It +/// does not assert that the AGENTS.md of a repository below that root reaches the model. A repo-less Codex run +/// must likewise start in its scratch directory, and where nothing of ours confines a multi-repo run, Codex's own +/// sandbox must keep every repository's .git and .codex read-only +/// (, run by the unconfined lane). /// /// Fidelity: 🟢 HIGH for everything but the model. The pinned CLI binaries, the production harness argv /// (), the production (bubblewrap where the @@ -29,10 +35,11 @@ namespace CodeSpace.SandboxTests; /// broker () and the endpoint the repository names, a loopback listener that only /// counts connections. /// -/// Each workspace is laid out as the executor hands it over: a single repository is the workspace itself, and a -/// multi-repo workspace is a root holding each repository in a directory of its own, both named to the harness the way -/// the executor names them. Every path is the temp path as Path.GetTempPath() spells it and never resolved, as -/// production never resolves it: on macOS that path runs through the /var symlink. +/// Each workspace is laid out as the executor hands it over: a repo-less run's is the scratch directory the +/// executor makes, a single repository is the workspace itself, and a multi-repo workspace is a root holding each +/// repository in a directory of its own, each named to the harness the way the executor names them. Every path is the +/// temp path as Path.GetTempPath() spells it and never resolved, as production never resolves it: on macOS that +/// path runs through the /var symlink. /// /// What it found on the pinned CLIs before the fix: Claude 2.1.263, launched without --setting-sources user, /// sent its model call to the repository's env.ANTHROPIC_BASE_URL with the repository's token, ran the @@ -41,15 +48,20 @@ namespace CodeSpace.SandboxTests; /// repository's [mcp_servers] on every run and ran its hooks on every acceptance-bearing run, whose /// --dangerously-bypass-hook-trust waives review for every enabled hook; its model routing was never exposed, /// because Codex ignores model_provider in project-local config. Codex keys that trust entry on the physical -/// directory it resolves as its cwd, so an entry keyed only by a workspace path under a symlink matched nothing. +/// directory it resolves as its cwd, so an entry keyed only by a workspace path under a symlink matched nothing. Codex +/// also refused to start at a multi-repo workspace's root, or in a repo-less run's scratch directory, neither of which +/// is a git repository: without --skip-git-repo-check it exits 1 before any model request. A single-repo arm +/// cannot see that. Once it started at a multi-repo root, its own sandbox kept .git and .codex read-only +/// only at that root, so where nothing of ours confined the run, each repository's .git/hooks and +/// .git/config were writable to the agent. /// /// Each arm runs the posture its CLI can run in its lane. In this root lane the Claude arms are Confined: the /// pinned CLI refuses bypassPermissions (a Standard run's mode) to uid 0. A Confined run can write nothing the /// test reads back, so its hooks and MCP servers are observed where the CLI itself reports them — the hook_* and /// init lines of its stream-json, and hook output reaching the model. The non-root lane runs the shipped posture, /// Standard as the worker's uid (), where every command a repository plants also -/// leaves a marker file in the workspace that run may write. The Codex arm is Standard and acceptance-bearing, the -/// posture in which a loaded repository hook would run unreviewed; its markers are files in the workspace too. +/// leaves a marker file in the workspace that run may write. The Codex arms are Standard and acceptance-bearing, the +/// posture in which a loaded repository hook would run unreviewed; their markers are files in the workspace too. /// /// Armed exactly like (, /// or a harness's own command override for a local run); each arm that ran prints , which the @@ -106,7 +118,130 @@ public async Task A_codex_run_ignores_the_config_and_hooks_its_repository_commit File.Exists(ownHook).ShouldBeTrue($"the platform's own Stop hook must still run with the repository's hooks shut out — a distrust that also silenced it would disable in-loop acceptance. {Diagnosis(harnessKind, spec, run, upstream)}"); upstream.Requests.ShouldContain(r => r.Body.Contains($"PROJECT-DOC-{repo.Nonce}", StringComparison.Ordinal), $"the repository's AGENTS.md is context, not config — it must still reach the model. {Diagnosis(harnessKind, spec, run, upstream)}"); - output.WriteLine($"{RanMarker} codex-cli confined={BubblewrapSandbox.Available is not null} hostileConnections={hostile.Connections}"); + output.WriteLine($"{RanMarker} codex-cli single-repo confined={BubblewrapSandbox.Available is not null} hostileConnections={hostile.Connections}"); + } + + /// + /// A multi-repo Codex run's cwd is the workspace root, which holds each repository in a folder of its own and is no + /// repository itself. The pinned 0.142.2 refuses such a cwd unless told to skip its git check: exit 1 before any model + /// request. The run must start there and reach its broker. Codex reads project config from its cwd upward, never from + /// below it, so in this layout the root is where config would load from: hostile config planted there must not load, + /// which holds only while the distrust covers a cwd that is no repository. Each repository below commits hostile + /// config too. Codex never reads it from there, so those markers only catch a future CLI that reads config below its + /// cwd; the distrust of a repository's own committed config is pinned by + /// . Which + /// instructions reach the run from the repositories below its cwd is not asserted here. + /// + [Fact] + public async Task A_multi_repo_codex_run_starts_at_a_workspace_root_that_is_no_repository_and_loads_no_config_from_it() + { + const string harnessKind = CodexHarness.HarnessKind; + var harness = ReviewerReadsItsDiffE2ETests.HarnessFor(harnessKind); + + if (!ReviewerReadsItsDiffE2ETests.Armed(harnessKind) || OperatingSystem.IsWindows()) return; + + await ReviewerReadsItsDiffE2ETests.RequirePinnedBinaryAsync(harness, harnessKind); + + using var hostile = new ConnectionCounter(); + var workspace = NewWorkspace(repositories: 2); + var rootMarkers = new Markers(workspace.Directory, workspace.Nonce, "workspace root", "mcp-server", "session", "prompt", "stop"); + var markers = workspace.Repositories.Select(repo => new Markers(repo, "mcp-server", "session", "prompt", "stop")).ToList(); + var ownHook = Path.Combine(workspace.Directory, $"marker-own-stop-hook-{workspace.Nonce}"); + + WriteCodexConfig(workspace.Directory, hostile, rootMarkers); + + foreach (var (repo, marked) in workspace.Repositories.Zip(markers)) PlantCodexConfig(repo, hostile, marked); + + GitRepositoryHolding(workspace.Directory).ShouldBeNull("fixture check: the workspace root must sit in no git repository, or the CLI's git check passes and this says nothing"); + + // Acceptance-bearing, like the single-repo arm: the posture in which a project hook, if loaded, would run unreviewed. + var (spec, run, upstream) = await RunAsync(harness, workspace, AgentAutonomyLevel.Standard, task => task with { Acceptance = new SupervisorAcceptanceSpec { Command = ["sh", "-c", $"printf ran > '{ownHook}'"] } }); + + spec.WorkingDirectory.ShouldBe(workspace.Directory, "fixture check: the run must start at the workspace root, where a multi-repo run's cwd is"); + spec.Args.ShouldContain("--dangerously-bypass-hook-trust", "fixture check: the arm must carry the bypass an acceptance-bearing run carries, or a project hook not running proves nothing"); + + var violations = BrokerViolations(run, upstream, hostile, workspace).Concat(rootMarkers.Ran()).Concat(markers.SelectMany(marked => marked.Ran())).ToList(); + + violations.ShouldBeEmpty(Diagnosis(harnessKind, spec, run, upstream)); + File.Exists(ownHook).ShouldBeTrue($"the platform's own Stop hook must run at the workspace root too. {Diagnosis(harnessKind, spec, run, upstream)}"); + + output.WriteLine($"{RanMarker} codex-cli multi-repo confined={BubblewrapSandbox.Available is not null} hostileConnections={hostile.Connections}"); + } + + /// + /// A repo-less run works in the scratch directory the executor makes for it (), + /// which has no git anywhere above it. The pinned 0.142.2 refused that cwd as it refused a multi-repo root, so every + /// repo-less Codex run failed at start. The run must start there and reach its broker. + /// + [Fact] + public async Task A_repo_less_codex_run_starts_in_a_scratch_directory_that_is_no_repository() + { + const string harnessKind = CodexHarness.HarnessKind; + var harness = ReviewerReadsItsDiffE2ETests.HarnessFor(harnessKind); + + if (!ReviewerReadsItsDiffE2ETests.Armed(harnessKind) || OperatingSystem.IsWindows()) return; + + await ReviewerReadsItsDiffE2ETests.RequirePinnedBinaryAsync(harness, harnessKind); + + using var hostile = new ConnectionCounter(); + var workspace = NewWorkspace(repositories: 0); + + GitRepositoryHolding(workspace.Directory).ShouldBeNull("fixture check: the scratch directory must sit in no git repository, or the CLI's git check passes and this says nothing"); + + var (spec, run, upstream) = await RunAsync(harness, workspace, AgentAutonomyLevel.Standard, task => task); + + spec.WorkingDirectory.ShouldBe(workspace.Directory, "fixture check: the run must start in its scratch directory"); + BrokerViolations(run, upstream, hostile, workspace).ShouldBeEmpty(Diagnosis(harnessKind, spec, run, upstream)); + + output.WriteLine($"{RanMarker} codex-cli scratch confined={BubblewrapSandbox.Available is not null}"); + } + + /// + /// Where nothing of ours confines a run, Codex's own workspace-write sandbox is its only boundary, and that sandbox + /// keeps .git and .codex read-only only at the top of each writable root. A multi-repo run's cwd is the + /// workspace root, so the harness names each repository below it as a root of its own. The scripted model asks for one + /// command that appends to each repository's .git/hooks/pre-push, .git/config and + /// .codex/config.toml, and to the one file each repository is meant to have changed. The platform's own commit + /// and push run git in each repository with the run's credential, so a hook or config the agent planted there would + /// run outside any sandbox. Under our confinement Codex's sandbox is stood down, and what an agent can write under + /// .git there is pinned by ReviewerReadsItsDiffE2ETests.A_standard_codex_writes_its_workspace_under_our_confinement_but_not_the_system_root; + /// this arm is the unconfined lane's (). + /// + internal async Task CodexKeepsEveryRepositorysMetadataReadOnlyAsync(string lane) + { + const string harnessKind = CodexHarness.HarnessKind; + var harness = ReviewerReadsItsDiffE2ETests.HarnessFor(harnessKind); + + if (!ReviewerReadsItsDiffE2ETests.Armed(harnessKind) || OperatingSystem.IsWindows()) return; + + await ReviewerReadsItsDiffE2ETests.RequirePinnedBinaryAsync(harness, harnessKind); + + BubblewrapSandbox.Available.ShouldBeNull("fixture check: this arm is about a host where nothing of ours confines the run, so Codex's own sandbox is its boundary"); + + using var hostile = new ConnectionCounter(); + var workspace = NewWorkspace(repositories: 2); + var probe = $"cs-probe-{workspace.Nonce}"; + + // Every target's directory exists before the run, so a write that fails can only have been refused, never sent nowhere. + foreach (var repo in workspace.Repositories) + { + repo.Commit(".codex/config.toml", "# the repository's own Codex config\n"); + Directory.CreateDirectory(Path.Combine(repo.Directory, ".git", "hooks")); + } + + var targets = workspace.Repositories.SelectMany(repo => new[] { ".git/hooks/pre-push", ".git/config", ".codex/config.toml", "app.txt" }.Select(relative => Path.Combine(repo.Directory, relative))).ToList(); + var changes = workspace.Repositories.Select(repo => Path.Combine(repo.Directory, "app.txt")).ToList(); + var command = string.Join("; ", targets.Select(path => $"printf '\\n# {probe}\\n' >> '{path}'")); + + var (spec, run, upstream) = await RunAsync(harness, workspace, AgentAutonomyLevel.Standard, task => task, [command]); + + var written = targets.Where(path => File.Exists(path) && File.ReadAllText(path).Contains(probe, StringComparison.Ordinal)).ToList(); + + BrokerViolations(run, upstream, hostile, workspace).ShouldBeEmpty(Diagnosis(harnessKind, spec, run, upstream)); + changes.ShouldAllBe(path => written.Contains(path), $"fixture check: the command must have run and changed each repository's file, or a refusal below proves nothing. {Diagnosis(harnessKind, spec, run, upstream)}"); + written.Except(changes).ShouldBeEmpty($"the agent wrote a repository's git metadata or Codex config, which the platform's own credentialed git or a later run would honour. {Diagnosis(harnessKind, spec, run, upstream)}"); + + output.WriteLine($"{RanMarker} {lane} codex-cli multi-repo metadata-read-only uid={NonRootWorker.EffectiveUid()}"); } /// @@ -201,8 +336,25 @@ private static IEnumerable ClaudeConfigViolations(Run run, ScriptedModel if (echoed.Count > 0) yield return $"repository hook output reached the model in {echoed.Count} request(s)"; } - /// A project config naming a model provider at and an MCP server that writes a marker when spawned, plus a hooks file with a marker at every hook point. + /// The project config of , committed the way a repository ships it. private static void PlantCodexConfig(Repository repo, ConnectionCounter hostile, Markers markers) + { + foreach (var (relativePath, content) in CodexConfigFiles(hostile, markers)) repo.Commit(relativePath, content); + } + + /// The project config of , written straight into , which no repository holds. + private static void WriteCodexConfig(string directory, ConnectionCounter hostile, Markers markers) + { + foreach (var (relativePath, content) in CodexConfigFiles(hostile, markers)) + { + var path = Path.Combine(directory, relativePath); + Directory.CreateDirectory(Path.GetDirectoryName(path)!); + File.WriteAllText(path, content); + } + } + + /// A project config naming a model provider at and an MCP server that writes a marker when spawned, plus a hooks file with a marker at every hook point. + private static IEnumerable<(string RelativePath, string Content)> CodexConfigFiles(ConnectionCounter hostile, Markers markers) { var toml = $""" model_provider = "repo" @@ -219,16 +371,16 @@ private static void PlantCodexConfig(Repository repo, ConnectionCounter hostile, """; var hooks = new JsonObject { ["hooks"] = new JsonObject { ["SessionStart"] = CodexHook(markers.Command("session")), ["UserPromptSubmit"] = CodexHook(markers.Command("prompt")), ["Stop"] = CodexHook(markers.Command("stop")) } }; - repo.Commit(".codex/config.toml", toml + "\n"); - repo.Commit(".codex/hooks.json", hooks.ToJsonString()); + yield return (".codex/config.toml", toml + "\n"); + yield return (".codex/hooks.json", hooks.ToJsonString()); } private static JsonArray CodexHook(string command) => new(new JsonObject { ["hooks"] = new JsonArray(new JsonObject { ["type"] = "command", ["command"] = command }) }); - /// The run launched the way the executor launches it, in and at 's production permissions, against a scripted model that answers at once. - private async Task<(SandboxSpec Spec, Run Run, ScriptedModelUpstream Upstream)> RunAsync(IAgentHarness harness, Workspace workspace, AgentAutonomyLevel tier, Func shape) + /// The run launched the way the executor launches it, in and at 's production permissions, against a scripted model that asks for each of in turn and then answers. + private async Task<(SandboxSpec Spec, Run Run, ScriptedModelUpstream Upstream)> RunAsync(IAgentHarness harness, Workspace workspace, AgentAutonomyLevel tier, Func shape, IReadOnlyList? commands = null) { - var upstream = new ScriptedModelUpstream([], $"DONE-{workspace.Nonce}"); + var upstream = new ScriptedModelUpstream(commands ?? [], $"DONE-{workspace.Nonce}"); using var broker = LoopbackModelCredentialBroker.ForTest(upstream); var permissions = AgentAutonomyPolicy.Derive(tier); var brokered = await OpenLeaseAsync(broker, permissions); @@ -282,12 +434,19 @@ private async Task OpenLeaseAsync(LoopbackModelCredenti } /// - /// A workspace laid out as the executor hands it over, at the temp path as Path.GetTempPath() spells it: one - /// repository is the workspace itself; several sit each in a directory of its own under a root that holds only the - /// manifest, which is where a multi-repo run's cwd is. + /// A workspace laid out as the executor hands it over, at the temp path as Path.GetTempPath() spells it: no + /// repository is the scratch directory a repo-less run gets; one repository is the workspace itself; several sit each + /// in a directory of its own under a root that holds only the manifest, which is where a multi-repo run's cwd is. /// private Workspace NewWorkspace(int repositories) { + if (repositories == 0) + { + var scratch = ScratchWorkspaceHandle.Create(Guid.NewGuid()); + _directories.Add(scratch.Directory); + return new Workspace(scratch.Directory, [], Guid.NewGuid().ToString("N")); + } + if (repositories == 1) { var repo = NewRepository(NewDirectory("repo-config-repo")); @@ -314,6 +473,19 @@ private static Repository NewRepository(string directory) return new Repository(directory, Guid.NewGuid().ToString("N")); } + /// The nearest directory at or above holding a .git, the way git finds the repository a cwd sits in; null when there is none. + private static string? GitRepositoryHolding(string directory) + { + for (var current = new DirectoryInfo(directory); current is not null; current = current.Parent) + { + var dotGit = Path.Combine(current.FullName, ".git"); + + if (Directory.Exists(dotGit) || File.Exists(dotGit)) return current.FullName; + } + + return null; + } + private string NewDirectory(string label) { var directory = Path.Combine(Path.GetTempPath(), $"cs-{label}-{Guid.NewGuid():N}"); @@ -355,14 +527,17 @@ public void Commit(string relativePath, string content) } } - /// One file per command the repository planted, inside the repository — writable to a Standard run, so a command that did run there could not fail to leave its mark. - private sealed class Markers(Repository repo, params string[] names) + /// One file per command planted in , inside it — writable to a Standard run, so a command that did run there could not fail to leave its mark. + private sealed class Markers(string directory, string nonce, string owner, params string[] names) { - private readonly Dictionary _paths = names.ToDictionary(name => name, name => Path.Combine(repo.Directory, $"marker-{name}-{repo.Nonce}")); + private readonly Dictionary _paths = names.ToDictionary(name => name, name => Path.Combine(directory, $"marker-{name}-{nonce}")); + + /// The markers of the commands a repository plants. + public Markers(Repository repo, params string[] names) : this(repo.Directory, repo.Nonce, "repository", names) { } public string Command(string name) => $"printf ran > '{_paths[name]}'"; - public IEnumerable Ran() => _paths.Where(p => File.Exists(p.Value)).Select(p => $"the CLI ran the repository's {p.Key} command"); + public IEnumerable Ran() => _paths.Where(p => File.Exists(p.Value)).Select(p => $"the CLI ran the {owner}'s {p.Key} command"); } /// A loopback listener that accepts and drops every connection, counting them — the endpoint a repository's config names, which a pinned run must never dial. diff --git a/backend/tests/CodeSpace.SandboxTests/ReviewerReadsItsDiffE2ETests.cs b/backend/tests/CodeSpace.SandboxTests/ReviewerReadsItsDiffE2ETests.cs index 77ca11a75..18d872bc4 100644 --- a/backend/tests/CodeSpace.SandboxTests/ReviewerReadsItsDiffE2ETests.cs +++ b/backend/tests/CodeSpace.SandboxTests/ReviewerReadsItsDiffE2ETests.cs @@ -77,9 +77,15 @@ public async Task The_pinned_codex_accepts_the_resume_spelling_of_its_stand_down await RequirePinnedBinaryAsync(harness, CodexHarness.HarnessKind); - // A real repository, as every production workspace is: Codex refuses an untrusted non-git directory before it gets anywhere near the rollout. - var task = new AgentTask { Goal = "resume", Harness = CodexHarness.HarnessKind, WorkspaceDirectory = NewReviewRepository().Directory, ResumeFromSessionId = Guid.NewGuid().ToString(), TimeoutSeconds = 60, Environment = new Dictionary { [CodexHarness.ApiKeyEnvVar] = "sk-review-e2e-resume", ["HOME"] = NewDirectory("resume-home") } }; + // At a directory laid out like a multi-repo run's workspace root, which is no repository: Codex refuses such a cwd + // before it gets anywhere near the rollout unless the resume seed carries --skip-git-repo-check. The repositories + // below it ride the argv as writable roots of their own, so the pinned binary must accept those on resume too. + var (root, repositories) = NewWorkspaceRoot(); + var task = new AgentTask { Goal = "resume", Harness = CodexHarness.HarnessKind, WorkspaceDirectory = root, WorkspaceRepositoryDirectories = repositories, ResumeFromSessionId = Guid.NewGuid().ToString(), TimeoutSeconds = 60, Environment = new Dictionary { [CodexHarness.ApiKeyEnvVar] = "sk-review-e2e-resume", ["HOME"] = NewDirectory("resume-home") } }; var spec = AgentRunExecutor.ApplyWriteScope(harness.BuildInvocation(task), task.Permissions); + + spec.Args.ShouldContain(arg => arg.StartsWith("sandbox_workspace_write.writable_roots=", StringComparison.Ordinal), "fixture check: the repositories below the root must ride the resume argv, or this does not show the pinned binary accepts them"); + var bogus = spec with { Args = spec.Args.Select(arg => arg.StartsWith("sandbox_mode=", StringComparison.Ordinal) ? "sandbox_mode=not-a-mode" : arg).ToList(), WhenRunnerConfines = null }; var accepted = await new LocalProcessRunner().RunAsync(spec, CancellationToken.None); @@ -393,6 +399,23 @@ private ReviewRepository NewReviewRepository() return new ReviewRepository(RealPath(directory), head0, GitOut(directory, "rev-parse HEAD"), nonce); } + /// A directory laid out like a multi-repo run's workspace root: a manifest and two repositories below it, and no repository itself. + private (string Root, IReadOnlyList Repositories) NewWorkspaceRoot() + { + var root = NewDirectory("resume-workspace"); + var repositories = new[] { "repo-1", "repo-2" }.Select(alias => Path.Combine(root, alias)).ToList(); + + File.WriteAllText(Path.Combine(root, "WORKSPACE.md"), "# Workspace\n\nThis is a MULTI-REPO workspace; each repository is a folder below.\n"); + + foreach (var repository in repositories) + { + Directory.CreateDirectory(repository); + Git(repository, "init -q -b main"); + } + + return (root, repositories); + } + private string NewDirectory(string label) { var directory = Path.Combine(Path.GetTempPath(), $"cs-{label}-{Guid.NewGuid():N}"); diff --git a/backend/tests/CodeSpace.SandboxTests/UnconfinedWorkerE2ETests.cs b/backend/tests/CodeSpace.SandboxTests/UnconfinedWorkerE2ETests.cs index c4b3bff3d..85c1e326f 100644 --- a/backend/tests/CodeSpace.SandboxTests/UnconfinedWorkerE2ETests.cs +++ b/backend/tests/CodeSpace.SandboxTests/UnconfinedWorkerE2ETests.cs @@ -16,9 +16,14 @@ namespace CodeSpace.SandboxTests; /// on the binaries alone, as it always has, so the setup's refusal aborts the launch — durable or not — and the /// admission predicts that same namespace. /// +/// Unconfined, Codex's own sandbox is the only boundary its commands meet, so this lane also runs the +/// repository-config arm that needs exactly that posture: a multi-repo Codex run whose agent must not be able to write +/// any repository's .git or .codex (). +/// /// Selected by its trait alone (--filter Category=SandboxUnconfined); the lane runs it as uid 1654 with /// CODESPACE_BWRAP_PATH naming no binary and Sandbox__RequireConfinement unset. Each arm asserts that -/// posture first () and prints , which the lane requires. +/// posture first () and prints , or for the Codex arm +/// , which the lane requires. /// [Trait("Category", Category)] public sealed class UnconfinedWorkerE2ETests(ITestOutputHelper output) @@ -74,6 +79,15 @@ public void The_admission_predicts_the_namespace_the_launch_plans() output.WriteLine($"{RanMarker} admission uid={NonRootWorker.EffectiveUid()}"); } + [Fact] + public async Task A_multi_repo_codex_run_cannot_write_a_repositorys_git_metadata_where_its_own_sandbox_is_the_boundary() + { + if (!RequirePosture()) return; + + using var arms = new RepositoryConfigE2ETests(output); + await arms.CodexKeepsEveryRepositorysMetadataReadOnlyAsync("unconfined"); + } + /// /// True on Linux once the posture is proved; false on any other OS (Rule 12.1). On Linux a missing piece FAILS the /// test: an arm that ran where bubblewrap confines, or as root, proves nothing about the posture it is named for. diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/CodexHarnessTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/CodexHarnessTests.cs index a35ff39af..8160cf51e 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/CodexHarnessTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/CodexHarnessTests.cs @@ -276,11 +276,66 @@ public void Builds_a_codex_exec_json_invocation_from_the_task() var spec = Harness.BuildInvocation(Task()); spec.Command.ShouldBe("codex"); - spec.Args.ShouldBe(new[] { "exec", "--json", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }); + spec.Args.ShouldBe(new[] { "exec", "--json", "--skip-git-repo-check", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }); spec.WorkingDirectory.ShouldBe("/tmp/ws"); spec.TimeoutSeconds.ShouldBe(900); } + [Theory] + [InlineData(null, new[] { "exec", "--json", "--skip-git-repo-check" })] + [InlineData("thr-resume-1", new[] { "exec", "resume", "thr-resume-1", "--json", "--skip-git-repo-check" })] + public void Every_run_may_start_in_a_workspace_root_that_is_not_a_git_repository(string? resumeFromSessionId, string[] seed) + { + // A multi-repo run's cwd is the workspace root, which holds each repository in a folder of its own and is no + // repository itself; a repo-less run's is a scratch directory with no git above it. Without this flag the pinned + // 0.142.2 refuses either cwd before any model request, for `exec` and `exec resume ` alike: exit 1, "Not + // inside a trusted directory and --skip-git-repo-check was not specified". A trust entry does not lift it. + // Inside a repository the flag changes nothing the model is sent. + var args = Harness.BuildInvocation(Task() with { ResumeFromSessionId = resumeFromSessionId }).Args; + + args.Take(seed.Length).ShouldBe(seed); + args.Count(a => a == "--skip-git-repo-check").ShouldBe(1); + } + + [Theory] + [InlineData(null, new[] { "exec", "--json", "--skip-git-repo-check", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", "sandbox_workspace_write.writable_roots=[\"/tmp/ws/api\",\"/tmp/ws/web\"]", "-c", WorkspaceDistrust, "-" })] + [InlineData("thr-resume-1", new[] { "exec", "resume", "thr-resume-1", "--json", "--skip-git-repo-check", "--model", "gpt-5.3-codex", "-c", "sandbox_mode=workspace-write", "-c", "sandbox_workspace_write.writable_roots=[\"/tmp/ws/api\",\"/tmp/ws/web\"]", "-c", WorkspaceDistrust, "-" })] + public void A_run_at_a_workspace_root_names_each_repository_below_it_as_a_writable_root_of_its_own(string? resumeFromSessionId, string[] expected) + { + // Codex's workspace-write sandbox keeps .git, .codex and .agents read-only only at the top of each writable root. + // At a multi-repo root the repositories sit below the cwd, not at the top of a root, so each one's .git/hooks and + // .git/config were writable to the agent, and the platform's own commit and push run git in each repository with + // the run's credential. Named as roots of their own, the real 0.142.2 refuses those writes and still lets the agent + // change the repositories' files (observed on macOS; UnconfinedWorkerE2ETests runs it on Linux). No write access + // is added: each one is already inside the cwd. + var task = Task() with { ResumeFromSessionId = resumeFromSessionId, WorkspaceRepositoryDirectories = ["/tmp/ws/api", "/tmp/ws/web"] }; + + Harness.BuildInvocation(task).Args.ShouldBe(expected); + } + + public static TheoryData RunsWithNoRepositoryBelowAWritableCwd() => new() + { + { "single repo, which is the cwd itself", Task() with { WorkspaceRepositoryDirectories = ["/tmp/ws"] } }, + { "scratch, which holds no repository", Task() with { WorkspaceRepositoryDirectories = [] } }, + { "no workspace materialised", Task() }, + { "a repository beside a cwd at the primary one, which a root would widen the sandbox to", Task() with { WorkspaceDirectory = "/tmp/ws/api", WorkspaceRepositoryDirectories = ["/tmp/ws/api", "/tmp/ws/web"] } }, + { "a directory that only shares the cwd's prefix", Task() with { WorkspaceRepositoryDirectories = ["/tmp/ws-other"] } }, + { "read-only, whose sandbox has no writable root", Task(scope: AgentWriteScope.ReadOnly) with { WorkspaceRepositoryDirectories = ["/tmp/ws/api"] } }, + }; + + [Theory] + [MemberData(nameof(RunsWithNoRepositoryBelowAWritableCwd))] + public void A_run_with_no_repository_below_a_writable_cwd_names_no_writable_root(string shape, AgentTask task) => + Harness.BuildInvocation(task).Args.ShouldNotContain(a => a.StartsWith("sandbox_workspace_write.", StringComparison.Ordinal), $"{shape}: nothing to carve out"); + + [Fact] + public void Each_writable_root_is_a_quoted_toml_string_so_a_quote_in_its_path_cannot_end_it() + { + var args = Harness.BuildInvocation(Task() with { WorkspaceRepositoryDirectories = ["/tmp/ws/a\"b\\c"] }).Args; + + args.ShouldContain("sandbox_workspace_write.writable_roots=[\"/tmp/ws/a\\\"b\\\\c\"]"); + } + public static TheoryData EveryRunShape() => new() { { "fresh", Task() }, @@ -373,7 +428,7 @@ public void Builds_a_resume_invocation_when_a_prior_session_is_set() // while -c is accepted on it and sandbox_mode is the config key the flag maps to. The Goal stays last. var spec = Harness.BuildInvocation(Task() with { ResumeFromSessionId = "thr-resume-1" }); - spec.Args.ShouldBe(new[] { "exec", "resume", "thr-resume-1", "--json", "--model", "gpt-5.3-codex", "-c", "sandbox_mode=workspace-write", "-c", WorkspaceDistrust, "-" }); + spec.Args.ShouldBe(new[] { "exec", "resume", "thr-resume-1", "--json", "--skip-git-repo-check", "--model", "gpt-5.3-codex", "-c", "sandbox_mode=workspace-write", "-c", WorkspaceDistrust, "-" }); } [Fact] @@ -425,7 +480,7 @@ public void Omits_the_resume_subcommand_when_no_prior_session() var spec = Harness.BuildInvocation(Task() with { ResumeFromSessionId = null }); spec.Args.ShouldNotContain("resume"); - spec.Args.ShouldBe(new[] { "exec", "--json", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }); + spec.Args.ShouldBe(new[] { "exec", "--json", "--skip-git-repo-check", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }); } [Fact] @@ -496,7 +551,7 @@ public void Omits_the_model_flag_when_no_model_is_set(string? model) { var spec = Harness.BuildInvocation(Task(model: model)); - spec.Args.ShouldBe(new[] { "exec", "--json", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }, + spec.Args.ShouldBe(new[] { "exec", "--json", "--skip-git-repo-check", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }, customMessage: "a blank model must omit --model entirely (not emit `--model \"\"`, which Codex rejects) so the CLI uses its default"); } @@ -508,7 +563,7 @@ public void Tools_are_not_projected_codex_has_no_global_allow_list() var withTools = Harness.BuildInvocation(Task() with { Tools = new[] { "Read", "Grep" } }); withTools.Args.ShouldNotContain("--allowed-tools"); - withTools.Args.ShouldBe(new[] { "exec", "--json", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }, + withTools.Args.ShouldBe(new[] { "exec", "--json", "--skip-git-repo-check", "--model", "gpt-5.3-codex", "--sandbox", "workspace-write", "-c", WorkspaceDistrust, "-" }, customMessage: "a tools list must not change the Codex invocation — it has no faithful projection there"); }