From 6d13c8e09d1f04f0b875438c645bb6b83ad682a4 Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 13:40:04 +0200 Subject: [PATCH 01/16] refactor(sandbox): extract the constrained-execution boundary into Shared Move ContainerCodeRunner's argument-building, bounded output reading, timeout, and container lifecycle into Shared/Sandbox (SandboxOptions, SandboxResult, SandboxRunner) so CodeAct is no longer the only sample with a real isolation boundary; MCP and Stigmergic can reuse SandboxRunner instead of inventing a weaker one. ContainerCodeRunner.BuildRunArguments keeps its exact signature and now maps CodeExecutionOptions onto SandboxOptions, producing the same docker argument list element-for-element and in the same order. Task 2.1 of docs/superpowers/sdd/2026-08-25-second-review-remediation. --- .../CodeActExecutionTests.cs | 1 + AgenticPatterns.Tests/SandboxArgumentTests.cs | 45 ++++ .../Execution/ContainerCodeRunner.cs | 145 +++++-------- .../Execution/UnsafeHostCodeRunner.cs | 1 + .../Sandbox}/BoundedReader.cs | 4 +- Shared/Sandbox/SandboxOptions.cs | 25 +++ Shared/Sandbox/SandboxRunner.cs | 200 ++++++++++++++++++ 7 files changed, 324 insertions(+), 97 deletions(-) create mode 100644 AgenticPatterns.Tests/SandboxArgumentTests.cs rename {CodeAct.AgentFramework/Execution => Shared/Sandbox}/BoundedReader.cs (92%) create mode 100644 Shared/Sandbox/SandboxOptions.cs create mode 100644 Shared/Sandbox/SandboxRunner.cs diff --git a/AgenticPatterns.Tests/CodeActExecutionTests.cs b/AgenticPatterns.Tests/CodeActExecutionTests.cs index 948d862..5d134e7 100644 --- a/AgenticPatterns.Tests/CodeActExecutionTests.cs +++ b/AgenticPatterns.Tests/CodeActExecutionTests.cs @@ -1,4 +1,5 @@ using CodeAct.AgentFramework.Execution; +using Shared.Sandbox; using Xunit; #pragma warning disable CS0618 // testing the deliberately-[Obsolete] unsafe runner is the point diff --git a/AgenticPatterns.Tests/SandboxArgumentTests.cs b/AgenticPatterns.Tests/SandboxArgumentTests.cs new file mode 100644 index 0000000..cb8f47e --- /dev/null +++ b/AgenticPatterns.Tests/SandboxArgumentTests.cs @@ -0,0 +1,45 @@ +using Shared.Sandbox; +using Xunit; + +namespace AgenticPatterns.Tests; + +public class SandboxArgumentTests +{ + [Fact] + public void DefaultsDenyNetworkAndCapabilitiesAndWrites() + { + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img"), ["echo", "hi"]).ToList(); + Assert.Equal("none", args[args.IndexOf("--network") + 1]); + Assert.Contains("--read-only", args); + Assert.Contains("--cap-drop", args); + Assert.Contains("--pids-limit", args); + Assert.DoesNotContain("--privileged", args); + } + + [Fact] + public void NoHostEnvironmentIsInherited() + { + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img"), ["echo"]); + // The container only sees variables passed explicitly; none were passed here. + Assert.DoesNotContain("--env", args); + Assert.DoesNotContain("-e", args); + } + + // Controller ruling: BuildRunArguments emits "--mount type=bind,src=...,dst=...[,readonly]", + // not "-v host:container:ro" — CodeActExecutionTests.OnlyThePerRunDirectoryIsMountedAndReadOnly + // already pins this shape and cannot change. + [Fact] + public void MountsAreReadOnlyWhenRequested() + { + var options = new SandboxOptions("img", Mounts: [("/host/src", "/src", true)]); + Assert.Contains(SandboxRunner.BuildRunArguments(options, ["echo"]), + a => a == "type=bind,src=/host/src,dst=/src,readonly"); + } + + [Fact] + public void EnablingTheNetworkIsExplicit() + { + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", Network: true), ["echo"]); + Assert.DoesNotContain("none", args); + } +} diff --git a/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs b/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs index be490f5..6d62c32 100644 --- a/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs +++ b/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using Shared.Sandbox; namespace CodeAct.AgentFramework.Execution; @@ -10,54 +11,51 @@ namespace CodeAct.AgentFramework.Execution; /// mount of the per-run script directory and a bounded tmpfs for build artifacts. /// This demonstrates the required isolation boundary; it is NOT a production-grade /// sandbox for adversarial or multi-tenant workloads (use a disposable VM/microVM -/// isolation service for that). +/// isolation service for that). The isolation boundary itself lives in +/// (Shared.Sandbox) so other samples can reuse it; this +/// class owns only what is specific to CodeAct: the sandbox image, per-run script +/// staging, and mapping onto . /// public sealed class ContainerCodeRunner(CodeExecutionOptions options) : IGeneratedCodeRunner { + private static readonly IReadOnlyList RunScriptCommand = ["dotnet", "run", "/workspace/script.cs"]; + /// True when the runtime CLI exists AND its daemon answers. - public static bool IsAvailable(string containerRuntime) - { - try - { - var (exitCode, _, _) = RunRuntimeCommandAsync(containerRuntime, - ["version", "--format", "{{.Server.Version}}"], TimeSpan.FromSeconds(10)) - .GetAwaiter().GetResult(); - return exitCode == 0; - } - catch (Exception e) when (e is System.ComponentModel.Win32Exception or PlatformNotSupportedException) - { - return false; // CLI not on PATH - } - } + public static bool IsAvailable(string containerRuntime) => SandboxRunner.IsAvailable(containerRuntime); /// /// The whole security posture, as one pure function so tests can pin every flag. - /// Deny everything; allow only what `dotnet run script.cs` needs. + /// Deny everything; allow only what `dotnet run script.cs` needs. Thin mapping onto + /// — the argument construction itself + /// lives there now. /// public static IReadOnlyList BuildRunArguments( string containerName, string runDirectory, CodeExecutionOptions options) => - [ - "run", "--rm", - "--name", containerName, // unique name so a timeout can kill THIS container - "--network", "none", // no network, not even DNS - "--read-only", // immutable container filesystem - "--cap-drop", "ALL", // no Linux capabilities - "--security-opt", "no-new-privileges=true", // setuid binaries cannot escalate - "--pids-limit", "128", // fork bombs die early - "--memory", "1g", // enough for Roslyn to compile, nothing runaway - "--cpus", "1", - "--user", "65532:65532", // non-root, no matching user on the host - "--tmpfs", "/tmp:rw,exec,nosuid,nodev,size=512m", // the ONLY writable path: bounded, for build - // artifacts; exec because the compiled script - // binary lives (and must run) here - "--mount", $"type=bind,src={runDirectory},dst=/workspace,readonly", // per-run dir only, read-only - "--env", "HOME=/tmp", // no host env is forwarded; these four are the - "--env", "DOTNET_CLI_HOME=/tmp/dotnet", // complete environment the SDK needs to run - "--env", "DOTNET_NOLOGO=1", // as an unknown non-root user offline - "--env", "DOTNET_CLI_TELEMETRY_OPTOUT=1", - options.ContainerImage, - "dotnet", "run", "/workspace/script.cs" - ]; + SandboxRunner.BuildRunArguments(ToSandboxOptions(containerName, runDirectory, options), RunScriptCommand); + + private static SandboxOptions ToSandboxOptions( + string containerName, string runDirectory, CodeExecutionOptions options) => new( + Image: options.ContainerImage, + ContainerRuntime: options.ContainerRuntime, + Network: false, + Memory: "1g", // enough for Roslyn to compile, nothing runaway + Cpus: "1", + PidsLimit: 128, // fork bombs die early + Timeout: options.ExecutionTimeout, + MaxOutputCharacters: options.MaxOutputCharacters, + Environment: new Dictionary // no host env is forwarded; these four are the + { // complete environment the SDK needs to run + ["HOME"] = "/tmp", // as an unknown non-root user offline + ["DOTNET_CLI_HOME"] = "/tmp/dotnet", + ["DOTNET_NOLOGO"] = "1", + ["DOTNET_CLI_TELEMETRY_OPTOUT"] = "1", + }, + Mounts: [(runDirectory, "/workspace", true)], // per-run dir only, read-only + ContainerName: containerName, // unique name so a timeout can kill THIS container + User: "65532:65532", // non-root, no matching user on the host + Tmpfs: "/tmp:rw,exec,nosuid,nodev,size=512m"); // the ONLY writable path: bounded, for build + // artifacts; exec because the compiled script + // binary lives (and must run) here public async Task RunAsync(string sourceCode, CancellationToken cancellationToken) { @@ -82,40 +80,12 @@ await WriteWorldReadableAsync(Path.Combine(runDirectory, "NuGet.config"), await EnsureImageAsync(cancellationToken); - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - timeoutCts.CancelAfter(options.ExecutionTimeout); - - using var process = StartRuntimeProcess(options.ContainerRuntime, - BuildRunArguments(containerName, runDirectory, options)); - try - { - var stdoutTask = BoundedReader.ReadBoundedAsync( - process.StandardOutput, options.MaxOutputCharacters, timeoutCts.Token); - var stderrTask = BoundedReader.ReadBoundedAsync( - process.StandardError, options.MaxOutputCharacters, timeoutCts.Token); - - await process.WaitForExitAsync(timeoutCts.Token); - - return new ExecutionResult(process.ExitCode, await stdoutTask, await stderrTask, TimedOut: false); - } - catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) - { - // Timeout, not caller cancellation. Kill by NAME: cancelling the client - // process does not guarantee the containerized process has stopped. - await KillContainerAsync(containerName); - if (!process.HasExited) process.Kill(entireProcessTree: true); - return new ExecutionResult( - ExitCode: -1, - StandardOutput: "", - StandardError: "Execution exceeded the configured time limit.", - TimedOut: true); - } - // Caller cancellation propagates as OperationCanceledException — it is never - // converted into an ordinary failure result. The finally still cleans up. + var sandboxOptions = ToSandboxOptions(containerName, runDirectory, options); + var result = await SandboxRunner.RunAsync(sandboxOptions, RunScriptCommand, stdin: null, cancellationToken); + return new ExecutionResult(result.ExitCode, result.StdOut, result.StdErr, result.TimedOut); } finally { - await RemoveContainerAsync(containerName); try { Directory.Delete(runDirectory, recursive: true); } catch (IOException) { } catch (UnauthorizedAccessException) { } @@ -162,39 +132,24 @@ private async Task EnsureImageAsync(CancellationToken cancellationToken) $" {options.ContainerRuntime} build -t {options.ContainerImage} CodeAct.AgentFramework/Sandbox\n{buildErr}"); } - private async Task KillContainerAsync(string containerName) => - await RunRuntimeCommandAsync(options.ContainerRuntime, - ["kill", containerName], TimeSpan.FromSeconds(30)); - - private async Task RemoveContainerAsync(string containerName) - { - // Belt and braces next to --rm; a missing container is the expected happy path. - try - { - await RunRuntimeCommandAsync(options.ContainerRuntime, - ["rm", "-f", containerName], TimeSpan.FromSeconds(30)); - } - catch (System.ComponentModel.Win32Exception) { } - } - - private static Process StartRuntimeProcess(string containerRuntime, IReadOnlyList arguments) + // ponytail: duplicates SandboxRunner's private process-exec helper. `docker image + // inspect`/`docker build` aren't sandboxed container runs, so SandboxRunner's public + // surface (fixed by the task interface) has no method for them; a ~20-line local + // helper is cheaper than adding a new public "run arbitrary runtime command" API + // that nothing else needs yet. Promote to a shared helper if a third caller appears. + private static async Task<(int ExitCode, string Stdout, string Stderr)> RunRuntimeCommandAsync( + string containerRuntime, IReadOnlyList arguments, TimeSpan timeout, + CancellationToken cancellationToken = default) { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(timeout); var startInfo = new ProcessStartInfo(containerRuntime) { RedirectStandardOutput = true, RedirectStandardError = true }; foreach (var argument in arguments) startInfo.ArgumentList.Add(argument); - return Process.Start(startInfo)!; - } - - private static async Task<(int ExitCode, string Stdout, string Stderr)> RunRuntimeCommandAsync( - string containerRuntime, IReadOnlyList arguments, TimeSpan timeout, - CancellationToken cancellationToken = default) - { - using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(timeout); - using var process = StartRuntimeProcess(containerRuntime, arguments); + using var process = Process.Start(startInfo)!; var stdoutTask = process.StandardOutput.ReadToEndAsync(cts.Token); var stderrTask = process.StandardError.ReadToEndAsync(cts.Token); try diff --git a/CodeAct.AgentFramework/Execution/UnsafeHostCodeRunner.cs b/CodeAct.AgentFramework/Execution/UnsafeHostCodeRunner.cs index a703178..5592c91 100644 --- a/CodeAct.AgentFramework/Execution/UnsafeHostCodeRunner.cs +++ b/CodeAct.AgentFramework/Execution/UnsafeHostCodeRunner.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using Shared.Sandbox; namespace CodeAct.AgentFramework.Execution; diff --git a/CodeAct.AgentFramework/Execution/BoundedReader.cs b/Shared/Sandbox/BoundedReader.cs similarity index 92% rename from CodeAct.AgentFramework/Execution/BoundedReader.cs rename to Shared/Sandbox/BoundedReader.cs index 3c265f0..8c212db 100644 --- a/CodeAct.AgentFramework/Execution/BoundedReader.cs +++ b/Shared/Sandbox/BoundedReader.cs @@ -1,8 +1,8 @@ using System.Text; -namespace CodeAct.AgentFramework.Execution; +namespace Shared.Sandbox; -internal static class BoundedReader +public static class BoundedReader { /// /// Reads a stream keeping at most characters, but keeps diff --git a/Shared/Sandbox/SandboxOptions.cs b/Shared/Sandbox/SandboxOptions.cs new file mode 100644 index 0000000..a2581a2 --- /dev/null +++ b/Shared/Sandbox/SandboxOptions.cs @@ -0,0 +1,25 @@ +namespace Shared.Sandbox; + +/// +/// The constrained-execution boundary shared by every sample that runs untrusted, +/// model-generated work: deny everything by default (no network, no capabilities, no +/// host filesystem, no host environment, no root) and grant back only what the caller +/// explicitly asks for via and . +/// +public sealed record SandboxOptions( + string Image, + string ContainerRuntime = "docker", + bool Network = false, + string Memory = "512m", + string Cpus = "1", + int PidsLimit = 128, + TimeSpan Timeout = default, + int MaxOutputCharacters = 65_536, + IReadOnlyDictionary? Environment = null, + IReadOnlyList<(string Host, string Container, bool ReadOnly)>? Mounts = null, + string? ContainerName = null, + string? User = "65532:65532", + string? Tmpfs = null, + bool Interactive = false); + +public sealed record SandboxResult(int ExitCode, string StdOut, string StdErr, bool TimedOut); diff --git a/Shared/Sandbox/SandboxRunner.cs b/Shared/Sandbox/SandboxRunner.cs new file mode 100644 index 0000000..2c66ea5 --- /dev/null +++ b/Shared/Sandbox/SandboxRunner.cs @@ -0,0 +1,200 @@ +using System.Diagnostics; + +namespace Shared.Sandbox; + +/// +/// Runs a command inside a locked-down local container. This is the constrained-execution +/// boundary itself: least privilege throughout, nothing granted back except what an +/// individual explicitly asks for. It demonstrates the required +/// isolation boundary; it is NOT a production-grade sandbox for adversarial or multi-tenant +/// workloads (use a disposable VM/microVM isolation service for that). +/// +public static class SandboxRunner +{ + /// True when the runtime CLI exists AND its daemon answers. + public static bool IsAvailable(string containerRuntime) + { + try + { + var (exitCode, _, _) = RunRuntimeCommandAsync(containerRuntime, + ["version", "--format", "{{.Server.Version}}"], TimeSpan.FromSeconds(10)) + .GetAwaiter().GetResult(); + return exitCode == 0; + } + catch (Exception e) when (e is System.ComponentModel.Win32Exception or PlatformNotSupportedException) + { + return false; // CLI not on PATH + } + } + + /// + /// The whole security posture, as one pure function so tests can pin every flag. + /// Deny everything by default; grant back only what asks for. + /// + public static IReadOnlyList BuildRunArguments(SandboxOptions options, IReadOnlyList command) + { + List args = ["run", "--rm"]; + + if (options.ContainerName is not null) + { + args.Add("--name"); + args.Add(options.ContainerName); + } + if (options.Interactive) args.Add("-i"); + + if (!options.Network) + { + args.Add("--network"); + args.Add("none"); + } + + args.Add("--read-only"); + args.Add("--cap-drop"); + args.Add("ALL"); + args.Add("--security-opt"); + args.Add("no-new-privileges=true"); + args.Add("--pids-limit"); + args.Add(options.PidsLimit.ToString()); + args.Add("--memory"); + args.Add(options.Memory); + args.Add("--cpus"); + args.Add(options.Cpus); + + if (options.User is not null) + { + args.Add("--user"); + args.Add(options.User); + } + + if (options.Tmpfs is not null) + { + args.Add("--tmpfs"); + args.Add(options.Tmpfs); + } + + if (options.Mounts is not null) + { + foreach (var (host, container, readOnly) in options.Mounts) + { + args.Add("--mount"); + args.Add($"type=bind,src={host},dst={container}" + (readOnly ? ",readonly" : "")); + } + } + + if (options.Environment is not null) + { + foreach (var (name, value) in options.Environment) + { + args.Add("--env"); + args.Add($"{name}={value}"); + } + } + + args.Add(options.Image); + args.AddRange(command); + return args; + } + + /// + /// Runs inside the sandbox and returns its output, bounded + /// per . On timeout, kills the container + /// by name (cancelling the client process does not guarantee the containerized process + /// has stopped) and reports instead of throwing. + /// Caller cancellation is never converted into a timeout result — it propagates as + /// . + /// + public static async Task RunAsync( + SandboxOptions options, IReadOnlyList command, string? stdin, CancellationToken cancellationToken) + { + using var process = StartRuntimeProcess(options.ContainerRuntime, + BuildRunArguments(options, command), redirectStandardInput: stdin is not null); + try + { + if (stdin is not null) + { + await process.StandardInput.WriteAsync(stdin); + process.StandardInput.Close(); + } + + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + // default(TimeSpan) means "no timeout configured", not "cancel immediately". + if (options.Timeout > TimeSpan.Zero) timeoutCts.CancelAfter(options.Timeout); + + try + { + var stdoutTask = BoundedReader.ReadBoundedAsync( + process.StandardOutput, options.MaxOutputCharacters, timeoutCts.Token); + var stderrTask = BoundedReader.ReadBoundedAsync( + process.StandardError, options.MaxOutputCharacters, timeoutCts.Token); + + await process.WaitForExitAsync(timeoutCts.Token); + + return new SandboxResult(process.ExitCode, await stdoutTask, await stderrTask, TimedOut: false); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // Timeout, not caller cancellation. + if (options.ContainerName is not null) await KillContainerAsync(options.ContainerRuntime, options.ContainerName); + if (!process.HasExited) process.Kill(entireProcessTree: true); + return new SandboxResult( + ExitCode: -1, + StdOut: "", + StdErr: "Execution exceeded the configured time limit.", + TimedOut: true); + } + // Caller cancellation propagates as OperationCanceledException — it is never + // converted into an ordinary failure result. The finally still cleans up. + } + finally + { + if (options.ContainerName is not null) await RemoveContainerAsync(options.ContainerRuntime, options.ContainerName); + } + } + + private static async Task KillContainerAsync(string containerRuntime, string containerName) => + await RunRuntimeCommandAsync(containerRuntime, ["kill", containerName], TimeSpan.FromSeconds(30)); + + private static async Task RemoveContainerAsync(string containerRuntime, string containerName) + { + // Belt and braces next to --rm; a missing container is the expected happy path. + try + { + await RunRuntimeCommandAsync(containerRuntime, ["rm", "-f", containerName], TimeSpan.FromSeconds(30)); + } + catch (System.ComponentModel.Win32Exception) { } + } + + private static Process StartRuntimeProcess( + string containerRuntime, IReadOnlyList arguments, bool redirectStandardInput = false) + { + var startInfo = new ProcessStartInfo(containerRuntime) + { + RedirectStandardOutput = true, + RedirectStandardError = true, + RedirectStandardInput = redirectStandardInput + }; + foreach (var argument in arguments) startInfo.ArgumentList.Add(argument); + return Process.Start(startInfo)!; + } + + private static async Task<(int ExitCode, string Stdout, string Stderr)> RunRuntimeCommandAsync( + string containerRuntime, IReadOnlyList arguments, TimeSpan timeout, + CancellationToken cancellationToken = default) + { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(timeout); + using var process = StartRuntimeProcess(containerRuntime, arguments); + var stdoutTask = process.StandardOutput.ReadToEndAsync(cts.Token); + var stderrTask = process.StandardError.ReadToEndAsync(cts.Token); + try + { + await process.WaitForExitAsync(cts.Token); + } + catch (OperationCanceledException) + { + if (!process.HasExited) process.Kill(entireProcessTree: true); + throw; + } + return (process.ExitCode, await stdoutTask, await stderrTask); + } +} From a4b01353ee9441de3bf2cad9f1763b14dc3627fd Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 13:51:04 +0200 Subject: [PATCH 02/16] fix(sandbox): never let optional fields turn off safety machinery Review fix round 1 on the sandbox extraction. Every finding shared one root cause: unconditional safety behaviour in the original ContainerCodeRunner became conditional on optional SandboxOptions fields defaulting to off. - C1 (critical): RunAsync generated no container name when ContainerName was null, so timeout cleanup degraded from kill-by-name to killing only the docker-run CLI process, leaking the still-running container. RunAsync now always generates a name when the caller omits one. - I1: Timeout <= TimeSpan.Zero now clamps to a 3-minute default instead of disabling the timeout outright (also fixes a real regression: ExecutionTimeout = TimeSpan.Zero used to fail fast and now would have run unbounded). - I2: stdin is written after the output readers start and the timeout is armed, and uses a cancellable WriteAsync overload, closing a pipe-deadlock/ uncancellable-hang path. - I3: Interactive is now derived as `Interactive || stdin is not null` so a redirected pipe is never silently discarded by a missing -i. - M3: PidsLimit <= 0 clamps to 128 instead of reading to docker as unlimited. - M2: strengthened two weak test assertions. - I4: added pure BuildRunArguments tests for User/Interactive/ContainerName/ Tmpfs, plus a Docker-gated RunAsync test proving the I1 clamp. ContainerCodeRunner.cs and both pinned test files (CodeActExecutionTests.cs, CodeActSandboxSmokeTests.cs) are untouched by this round. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- AgenticPatterns.Tests/SandboxArgumentTests.cs | 72 ++++++++++++++++++- Shared/Sandbox/SandboxRunner.cs | 47 ++++++++---- 2 files changed, 105 insertions(+), 14 deletions(-) diff --git a/AgenticPatterns.Tests/SandboxArgumentTests.cs b/AgenticPatterns.Tests/SandboxArgumentTests.cs index cb8f47e..c25547b 100644 --- a/AgenticPatterns.Tests/SandboxArgumentTests.cs +++ b/AgenticPatterns.Tests/SandboxArgumentTests.cs @@ -22,7 +22,6 @@ public void NoHostEnvironmentIsInherited() var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img"), ["echo"]); // The container only sees variables passed explicitly; none were passed here. Assert.DoesNotContain("--env", args); - Assert.DoesNotContain("-e", args); } // Controller ruling: BuildRunArguments emits "--mount type=bind,src=...,dst=...[,readonly]", @@ -39,7 +38,76 @@ public void MountsAreReadOnlyWhenRequested() [Fact] public void EnablingTheNetworkIsExplicit() { + // M2: assert intent (no --network flag at all), not just the absence of the + // string "none" — a weaker assertion would pass even if the flag leaked through + // with some other value. var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", Network: true), ["echo"]); - Assert.DoesNotContain("none", args); + Assert.DoesNotContain("--network", args); + } + + // ---- I4: the four fields ruling 2 added, pinned individually ---- + + [Fact] + public void NullUserOmitsTheFlagAndItsValue() + { + // Task 2.2 depends on this exact behaviour: User: null defers to the image's own USER. + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", User: null), ["echo"]); + Assert.DoesNotContain("--user", args); + Assert.DoesNotContain("65532:65532", args); + } + + [Fact] + public void InteractiveFlagIsPlacedBeforeTheImage() + { + // MCP's stdio transport depends on -i being a docker FLAG, i.e. before the image + // argument, not appended after it (where docker would pass it to the command instead). + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", Interactive: true), ["echo"]).ToList(); + Assert.True(args.IndexOf("-i") < args.IndexOf("img")); + } + + [Fact] + public void NullContainerNameOmitsTheFlag() + { + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", ContainerName: null), ["echo"]); + Assert.DoesNotContain("--name", args); + } + + [Fact] + public void NullTmpfsOmitsTheFlag() + { + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", Tmpfs: null), ["echo"]); + Assert.DoesNotContain("--tmpfs", args); + } + + [Fact] + public void NonPositivePidsLimitFallsBackToTheSafeDefault() + { + // M3: 0 (or negative) reads to docker as "unlimited" — the same fail-open-on-a-bound + // shape as an unset Timeout must not be reachable through PidsLimit either. + var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", PidsLimit: 0), ["echo"]).ToList(); + Assert.Equal("128", args[args.IndexOf("--pids-limit") + 1]); + } +} + +// I1 has no coverage from BuildRunArguments alone — the clamp lives in RunAsync's +// CancelAfter call, so proving it needs a real run. Gated on Docker like +// CodeActSandboxSmokeTests: passes vacuously without it rather than failing the suite. +public class SandboxTimeoutTests +{ + private static readonly bool DockerAvailable = SandboxRunner.IsAvailable("docker"); + + [Fact] + public async Task ZeroTimeoutFallsBackToTheSafeDefaultInsteadOfCancellingImmediately() + { + if (!DockerAvailable) return; + + // Timeout: default (TimeSpan.Zero) would cancel a CancellationTokenSource + // instantly if taken literally; RunAsync must clamp it to a safe positive bound + // instead, so an ordinary, fast command still completes with TimedOut: false. + var options = new SandboxOptions("agentic-patterns-codeact-sandbox", + ContainerName: $"sandbox-timeout-test-{Guid.NewGuid():N}"); + var result = await SandboxRunner.RunAsync(options, ["dotnet", "--version"], stdin: null, CancellationToken.None); + + Assert.False(result.TimedOut); } } diff --git a/Shared/Sandbox/SandboxRunner.cs b/Shared/Sandbox/SandboxRunner.cs index 2c66ea5..5ae5bf2 100644 --- a/Shared/Sandbox/SandboxRunner.cs +++ b/Shared/Sandbox/SandboxRunner.cs @@ -54,7 +54,9 @@ public static IReadOnlyList BuildRunArguments(SandboxOptions options, IR args.Add("--security-opt"); args.Add("no-new-privileges=true"); args.Add("--pids-limit"); - args.Add(options.PidsLimit.ToString()); + // M3: 0 (or negative) reads to docker as "unlimited" — the same fail-open-on-a-bound + // shape as an unset Timeout. Never let "unset" mean "no limit" on a security boundary. + args.Add((options.PidsLimit > 0 ? options.PidsLimit : 128).ToString()); args.Add("--memory"); args.Add(options.Memory); args.Add("--cpus"); @@ -106,19 +108,28 @@ public static IReadOnlyList BuildRunArguments(SandboxOptions options, IR public static async Task RunAsync( SandboxOptions options, IReadOnlyList command, string? stdin, CancellationToken cancellationToken) { + // C1: kill-by-name must never be optional. SIGKILLing the docker-run CLI process + // does not stop the daemon-side container, so timeout cleanup below has to kill BY + // NAME — generate one when the caller didn't supply one rather than silently + // degrading to a leaked container. + var containerName = options.ContainerName ?? $"sandbox-{Guid.NewGuid():N}"; + options = options with + { + ContainerName = containerName, + // I3: a caller that redirects stdin but forgot -i gets a pipe docker never + // attaches to — input is silently discarded and the callee sees EOF. + Interactive = options.Interactive || stdin is not null, + }; + using var process = StartRuntimeProcess(options.ContainerRuntime, BuildRunArguments(options, command), redirectStandardInput: stdin is not null); try { - if (stdin is not null) - { - await process.StandardInput.WriteAsync(stdin); - process.StandardInput.Close(); - } - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - // default(TimeSpan) means "no timeout configured", not "cancel immediately". - if (options.Timeout > TimeSpan.Zero) timeoutCts.CancelAfter(options.Timeout); + // I1: on a type whose whole purpose is bounding untrusted work, "caller forgot + // to set Timeout" must never mean "no bound" — fall back to the same safe + // default CodeAct always passes explicitly. + timeoutCts.CancelAfter(options.Timeout > TimeSpan.Zero ? options.Timeout : TimeSpan.FromMinutes(3)); try { @@ -127,14 +138,26 @@ public static async Task RunAsync( var stderrTask = BoundedReader.ReadBoundedAsync( process.StandardError, options.MaxOutputCharacters, timeoutCts.Token); + // I2: readers must already be draining, and the timeout must already be + // armed, before we write. A child that prints more than the output bound + // before reading its stdin blocks on a full stdout pipe; writing stdin + // before the drain starts (and with no cancellation token) would then hang + // forever, uncancellable by either the caller's token or the sandbox timeout. + if (stdin is not null) + { + await process.StandardInput.WriteAsync(stdin.AsMemory(), timeoutCts.Token); + process.StandardInput.Close(); + } + await process.WaitForExitAsync(timeoutCts.Token); return new SandboxResult(process.ExitCode, await stdoutTask, await stderrTask, TimedOut: false); } catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { - // Timeout, not caller cancellation. - if (options.ContainerName is not null) await KillContainerAsync(options.ContainerRuntime, options.ContainerName); + // Timeout, not caller cancellation. Kill by NAME: cancelling the client + // process does not guarantee the containerized process has stopped. + await KillContainerAsync(options.ContainerRuntime, containerName); if (!process.HasExited) process.Kill(entireProcessTree: true); return new SandboxResult( ExitCode: -1, @@ -147,7 +170,7 @@ public static async Task RunAsync( } finally { - if (options.ContainerName is not null) await RemoveContainerAsync(options.ContainerRuntime, options.ContainerName); + await RemoveContainerAsync(options.ContainerRuntime, containerName); } } From f7d3b99c7c16e1ff417532afd59a1e5255ed8867 Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 13:56:58 +0200 Subject: [PATCH 03/16] test(sandbox): assert the timeout/pids-limit clamps by value, not by waiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix round 2: the round-1 Docker-gated timeout test was vacuous — it ran a fast command and asserted TimedOut: false, which passes identically whether Timeout <= 0 clamps to 3 minutes or disables the timeout outright, since a fast command finishes long before either bound expires. Extract SandboxRunner.EffectiveTimeout(TimeSpan) and EffectivePidsLimit(int) as pure static methods; RunAsync and BuildRunArguments now call through them instead of inlining the ternary. New pure tests assert the clamp values directly, so reverting either clamp fails immediately with no Docker or timing involved. Renamed the surviving Docker-gated test to what it actually proves (ZeroTimeoutDoesNotCancelTheRunImmediately). ContainerCodeRunner.cs and both pinned test files are untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- AgenticPatterns.Tests/SandboxArgumentTests.cs | 39 +++++++++++++++---- Shared/Sandbox/SandboxRunner.cs | 24 ++++++++---- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/AgenticPatterns.Tests/SandboxArgumentTests.cs b/AgenticPatterns.Tests/SandboxArgumentTests.cs index c25547b..da097df 100644 --- a/AgenticPatterns.Tests/SandboxArgumentTests.cs +++ b/AgenticPatterns.Tests/SandboxArgumentTests.cs @@ -87,23 +87,48 @@ public void NonPositivePidsLimitFallsBackToTheSafeDefault() var args = SandboxRunner.BuildRunArguments(new SandboxOptions("img", PidsLimit: 0), ["echo"]).ToList(); Assert.Equal("128", args[args.IndexOf("--pids-limit") + 1]); } + + // ---- fix round 2: assert the clamps as pure values, not by waiting for them ---- + // A test that runs a fast command and asserts TimedOut: false cannot fail if the clamp + // is reverted — a fast command finishes long before either an unbounded wait or a + // 3-minute bound expires. Assert the decision directly instead. + + [Fact] + public void EffectiveTimeoutClampsNonPositiveValuesToThreeMinutes() + { + Assert.Equal(TimeSpan.FromMinutes(3), SandboxRunner.EffectiveTimeout(TimeSpan.Zero)); + Assert.Equal(TimeSpan.FromMinutes(3), SandboxRunner.EffectiveTimeout(TimeSpan.FromSeconds(-5))); + } + + [Fact] + public void EffectiveTimeoutNeverOverridesAnExplicitCallerValue() => + Assert.Equal(TimeSpan.FromSeconds(30), SandboxRunner.EffectiveTimeout(TimeSpan.FromSeconds(30))); + + [Fact] + public void EffectivePidsLimitClampsNonPositiveValuesTo128() + { + Assert.Equal(128, SandboxRunner.EffectivePidsLimit(0)); + Assert.Equal(128, SandboxRunner.EffectivePidsLimit(-1)); + } + + [Fact] + public void EffectivePidsLimitNeverOverridesAnExplicitCallerValue() => + Assert.Equal(64, SandboxRunner.EffectivePidsLimit(64)); } -// I1 has no coverage from BuildRunArguments alone — the clamp lives in RunAsync's -// CancelAfter call, so proving it needs a real run. Gated on Docker like -// CodeActSandboxSmokeTests: passes vacuously without it rather than failing the suite. +// The zero-Timeout clamp value itself is asserted directly above (EffectiveTimeoutClamps...). +// What that pure test cannot prove is that RunAsync actually WIRES the clamp into +// CancelAfter instead of, say, ignoring Timeout entirely — that needs a real run. Gated on +// Docker like CodeActSandboxSmokeTests: passes vacuously without it rather than failing the suite. public class SandboxTimeoutTests { private static readonly bool DockerAvailable = SandboxRunner.IsAvailable("docker"); [Fact] - public async Task ZeroTimeoutFallsBackToTheSafeDefaultInsteadOfCancellingImmediately() + public async Task ZeroTimeoutDoesNotCancelTheRunImmediately() { if (!DockerAvailable) return; - // Timeout: default (TimeSpan.Zero) would cancel a CancellationTokenSource - // instantly if taken literally; RunAsync must clamp it to a safe positive bound - // instead, so an ordinary, fast command still completes with TimedOut: false. var options = new SandboxOptions("agentic-patterns-codeact-sandbox", ContainerName: $"sandbox-timeout-test-{Guid.NewGuid():N}"); var result = await SandboxRunner.RunAsync(options, ["dotnet", "--version"], stdin: null, CancellationToken.None); diff --git a/Shared/Sandbox/SandboxRunner.cs b/Shared/Sandbox/SandboxRunner.cs index 5ae5bf2..35407da 100644 --- a/Shared/Sandbox/SandboxRunner.cs +++ b/Shared/Sandbox/SandboxRunner.cs @@ -27,6 +27,21 @@ public static bool IsAvailable(string containerRuntime) } } + /// + /// The effective pids-limit clamp, as a pure value so tests can assert it directly instead + /// of inferring it from a timing-dependent run. 0 or negative reads to docker as "unlimited" + /// — never let "unset" mean "no limit" on a security boundary. + /// + public static int EffectivePidsLimit(int configured) => configured > 0 ? configured : 128; + + /// + /// The effective timeout clamp, as a pure value so tests can assert it directly instead of + /// inferring it from a timing-dependent run. On a type whose whole purpose is bounding + /// untrusted work, "caller forgot to set Timeout" must never mean "no bound". + /// + public static TimeSpan EffectiveTimeout(TimeSpan configured) => + configured > TimeSpan.Zero ? configured : TimeSpan.FromMinutes(3); + /// /// The whole security posture, as one pure function so tests can pin every flag. /// Deny everything by default; grant back only what asks for. @@ -54,9 +69,7 @@ public static IReadOnlyList BuildRunArguments(SandboxOptions options, IR args.Add("--security-opt"); args.Add("no-new-privileges=true"); args.Add("--pids-limit"); - // M3: 0 (or negative) reads to docker as "unlimited" — the same fail-open-on-a-bound - // shape as an unset Timeout. Never let "unset" mean "no limit" on a security boundary. - args.Add((options.PidsLimit > 0 ? options.PidsLimit : 128).ToString()); + args.Add(EffectivePidsLimit(options.PidsLimit).ToString()); args.Add("--memory"); args.Add(options.Memory); args.Add("--cpus"); @@ -126,10 +139,7 @@ public static async Task RunAsync( try { using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - // I1: on a type whose whole purpose is bounding untrusted work, "caller forgot - // to set Timeout" must never mean "no bound" — fall back to the same safe - // default CodeAct always passes explicitly. - timeoutCts.CancelAfter(options.Timeout > TimeSpan.Zero ? options.Timeout : TimeSpan.FromMinutes(3)); + timeoutCts.CancelAfter(EffectiveTimeout(options.Timeout)); try { From 0c9555ea30d7816192a8c401a99b211f72a068fb Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 14:09:14 +0200 Subject: [PATCH 04/16] fix(mcp): pin, sandbox and allowlist the MCP server The sample no longer runs npx -y @modelcontextprotocol/server-everything (unpinned, latest-at-run-time, executed on the host with the app's own environment, every discovered tool bound). The server is now pinned to an exact version baked into a Docker image, launched through the same locked-down container boundary CodeAct uses (Shared.Sandbox.SandboxRunner), and only an explicit allowlist (add, echo) of its discovered tools is ever bound to the agent via the new McpToolBinding.SelectAuthorized, which fails closed if an allowlisted tool is missing. Both the Agent Framework and Semantic Kernel flavors get the same treatment and fail closed with no silent host fallback when no container runtime is available, mirroring CodeAct.AgentFramework's CodeRunnerFactory double opt-in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- .../AgenticPatterns.Tests.csproj | 5 + AgenticPatterns.Tests/McpToolBindingTests.cs | 18 ++++ MCP.AgentFramework/McpToolBinding.cs | 17 ++++ MCP.AgentFramework/Program.cs | 45 ++++++--- MCP.AgentFramework/Sandbox/Dockerfile | 16 ++++ MCP.SemanticKernel/McpToolBinding.cs | 17 ++++ MCP.SemanticKernel/Program.cs | 53 ++++++---- PatternExplorer/patterns/MCP.md | 96 ++++++++++++++----- .../patterns/ProgressiveToolDisclosure.md | 6 +- README.md | 23 ++++- 10 files changed, 237 insertions(+), 59 deletions(-) create mode 100644 AgenticPatterns.Tests/McpToolBindingTests.cs create mode 100644 MCP.AgentFramework/McpToolBinding.cs create mode 100644 MCP.AgentFramework/Sandbox/Dockerfile create mode 100644 MCP.SemanticKernel/McpToolBinding.cs diff --git a/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj b/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj index 360e758..917e359 100644 --- a/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj +++ b/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj @@ -21,6 +21,11 @@ + + diff --git a/AgenticPatterns.Tests/McpToolBindingTests.cs b/AgenticPatterns.Tests/McpToolBindingTests.cs new file mode 100644 index 0000000..52ce4f3 --- /dev/null +++ b/AgenticPatterns.Tests/McpToolBindingTests.cs @@ -0,0 +1,18 @@ +using MCP.AgentFramework; +using Xunit; + +namespace AgenticPatterns.Tests; + +public class McpToolBindingTests +{ + static readonly HashSet Allowed = new(["add", "echo"], StringComparer.Ordinal); + + [Fact] + public void OnlyAllowlistedToolsAreBound() => + Assert.Equal(["add", "echo"], + McpToolBinding.SelectAuthorized(["add", "echo", "printEnv", "sampleLLM"], Allowed).Order()); + + [Fact] + public void AMissingAllowlistedToolFailsClosed() => + Assert.Throws(() => McpToolBinding.SelectAuthorized(["echo"], Allowed)); +} diff --git a/MCP.AgentFramework/McpToolBinding.cs b/MCP.AgentFramework/McpToolBinding.cs new file mode 100644 index 0000000..43c690b --- /dev/null +++ b/MCP.AgentFramework/McpToolBinding.cs @@ -0,0 +1,17 @@ +namespace MCP.AgentFramework; + +public static class McpToolBinding +{ + /// Discovery and authorization are separate steps: discovering a tool never grants it. + /// Fails closed - a missing allowlisted tool means the server is not the one we pinned. + public static IReadOnlyList SelectAuthorized(IEnumerable discovered, + IReadOnlySet allowed) + { + var found = discovered.Where(allowed.Contains).ToList(); + var missing = allowed.Except(found).ToList(); + if (missing.Count > 0) + throw new InvalidOperationException( + $"The MCP server does not advertise the required tool(s): {string.Join(", ", missing)}."); + return found; + } +} diff --git a/MCP.AgentFramework/Program.cs b/MCP.AgentFramework/Program.cs index 84bba2e..fb0324a 100644 --- a/MCP.AgentFramework/Program.cs +++ b/MCP.AgentFramework/Program.cs @@ -1,27 +1,46 @@ +using MCP.AgentFramework; using Microsoft.Agents.AI; using Microsoft.Extensions.AI; using ModelContextProtocol.Client; using Shared; +using Shared.Sandbox; -// 1) Connect to the official MCP demo server (stdio, no credentials needed) -await using var mcpClient = await McpClient.CreateAsync( - new StdioClientTransport(new StdioClientTransportOptions - { - Name = "MCPServer", - Command = "npx", - Arguments = ["-y", "@modelcontextprotocol/server-everything"] - })); +const string image = "agentic-patterns/mcp-server-everything:2025.8.18"; +var allowed = new HashSet(["add", "echo"], StringComparer.Ordinal); -// 2) Discover tools -var tools = await mcpClient.ListToolsAsync().ConfigureAwait(false); -Console.WriteLine($"MCP tools: {string.Join(", ", tools.Select(t => t.Name))}"); +// Fail closed: no sandbox, no MCP server. Same double opt-in as CodeAct for the unsafe path. +if (!SandboxRunner.IsAvailable("docker")) +{ + Console.Error.WriteLine( + "No container runtime available. This sample runs a third-party MCP server, which is " + + "untrusted code; it will not be started on the host. Install Docker, or set " + + "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION=true and " + + "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION=I_UNDERSTAND_THIS_RUNS_UNTRUSTED_CODE_ON_MY_HOST."); + return 1; +} + +// The server speaks stdio, so the container IS the transport: no network, no host environment, +// no credentials, read-only filesystem, dropped capabilities, bounded pids and memory. +var sandbox = new SandboxOptions(image, Network: false, Memory: "256m", PidsLimit: 64, Interactive: true, + User: null); +await using var mcpClient = await McpClient.CreateAsync(new StdioClientTransport(new StdioClientTransportOptions +{ + Name = "MCPServer", + Command = "docker", + Arguments = [.. SandboxRunner.BuildRunArguments(sandbox, [])], +})); + +var discovered = await mcpClient.ListToolsAsync(); +Console.WriteLine($"Discovered: {string.Join(", ", discovered.Select(t => t.Name))}"); +var authorized = McpToolBinding.SelectAuthorized(discovered.Select(t => t.Name), allowed).ToHashSet(); +Console.WriteLine($"Bound to the agent: {string.Join(", ", authorized)}"); -// 3) McpClientTool derives from AIFunction, so MCP tools plug straight into the agent var agent = new ChatClientAgent(Settings.ChatClient, "Use MCP tools when needed. Be concise and cite tool results in your reasoning.", - tools: [.. tools.Cast()]); + tools: [.. discovered.Where(t => authorized.Contains(t.Name)).Cast()]); var prompt = "Use the 'add' tool to compute 1234 + 5678, then use the 'echo' tool to repeat the result."; var response = await agent.RunAsync(prompt); Console.WriteLine(response); +return 0; diff --git a/MCP.AgentFramework/Sandbox/Dockerfile b/MCP.AgentFramework/Sandbox/Dockerfile new file mode 100644 index 0000000..401cfd1 --- /dev/null +++ b/MCP.AgentFramework/Sandbox/Dockerfile @@ -0,0 +1,16 @@ +# Sandbox image for the third-party MCP demo server. +# +# The upstream sample spawns `npx -y @modelcontextprotocol/server-everything`, which +# downloads an UNPINNED package at run time and executes it on the host with the +# application's environment. This image replaces that: the exact server version is +# baked in at BUILD time (the only moment network access is legitimate), so the +# sandbox can run with `--network none` and no host credentials at all. The security +# boundary itself is enforced at `docker run` time by +# Shared.Sandbox.SandboxRunner.BuildRunArguments (no network, read-only rootfs, +# dropped capabilities, non-root user, pids/memory/cpu limits). +FROM node:22-alpine +ARG SERVER_VERSION=2025.8.18 +RUN npm install -g @modelcontextprotocol/server-everything@${SERVER_VERSION} \ + && addgroup -S mcp && adduser -S -G mcp mcp +USER mcp +ENTRYPOINT ["mcp-server-everything"] diff --git a/MCP.SemanticKernel/McpToolBinding.cs b/MCP.SemanticKernel/McpToolBinding.cs new file mode 100644 index 0000000..e609d9e --- /dev/null +++ b/MCP.SemanticKernel/McpToolBinding.cs @@ -0,0 +1,17 @@ +namespace MCP.SemanticKernel; + +public static class McpToolBinding +{ + /// Discovery and authorization are separate steps: discovering a tool never grants it. + /// Fails closed - a missing allowlisted tool means the server is not the one we pinned. + public static IReadOnlyList SelectAuthorized(IEnumerable discovered, + IReadOnlySet allowed) + { + var found = discovered.Where(allowed.Contains).ToList(); + var missing = allowed.Except(found).ToList(); + if (missing.Count > 0) + throw new InvalidOperationException( + $"The MCP server does not advertise the required tool(s): {string.Join(", ", missing)}."); + return found; + } +} diff --git a/MCP.SemanticKernel/Program.cs b/MCP.SemanticKernel/Program.cs index d9c4135..3f50d73 100644 --- a/MCP.SemanticKernel/Program.cs +++ b/MCP.SemanticKernel/Program.cs @@ -1,32 +1,50 @@ +using MCP.SemanticKernel; using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.Agents; using Microsoft.SemanticKernel.Connectors.OpenAI; using ModelContextProtocol.Client; using Shared; +using Shared.Sandbox; #pragma warning disable SKEXP0001 -await using var mcpClient = await McpClient.CreateAsync( - new StdioClientTransport(new StdioClientTransportOptions - { - Name = "MCPServer", - Command = "npx", - // Official MCP demo server: runs over stdio, needs no credentials. - Arguments = ["-y", "@modelcontextprotocol/server-everything"] - })); +const string image = "agentic-patterns/mcp-server-everything:2025.8.18"; +var allowed = new HashSet(["add", "echo"], StringComparer.Ordinal); -// 2) Discover tools -var tools = await mcpClient.ListToolsAsync().ConfigureAwait(false); +// Fail closed: no sandbox, no MCP server. Same double opt-in as CodeAct for the unsafe path. +if (!SandboxRunner.IsAvailable("docker")) +{ + Console.Error.WriteLine( + "No container runtime available. This sample runs a third-party MCP server, which is " + + "untrusted code; it will not be started on the host. Install Docker, or set " + + "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION=true and " + + "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION=I_UNDERSTAND_THIS_RUNS_UNTRUSTED_CODE_ON_MY_HOST."); + return 1; +} -// 3) Register MCP tools as SK functions (agent can tool-call) -var kernel = Settings.Kernel; +// The server speaks stdio, so the container IS the transport: no network, no host environment, +// no credentials, read-only filesystem, dropped capabilities, bounded pids and memory. +var sandbox = new SandboxOptions(image, Network: false, Memory: "256m", PidsLimit: 64, Interactive: true, + User: null); +await using var mcpClient = await McpClient.CreateAsync(new StdioClientTransport(new StdioClientTransportOptions +{ + Name = "MCPServer", + Command = "docker", + Arguments = [.. SandboxRunner.BuildRunArguments(sandbox, [])], +})); -// This pattern (MCP tools -> Kernel functions) is used in the official SK MCP sample. +var discovered = await mcpClient.ListToolsAsync().ConfigureAwait(false); +Console.WriteLine($"Discovered: {string.Join(", ", discovered.Select(t => t.Name))}"); +var authorized = McpToolBinding.SelectAuthorized(discovered.Select(t => t.Name), allowed).ToHashSet(); +Console.WriteLine($"Bound to the agent: {string.Join(", ", authorized)}"); + +// Register MCP tools as SK functions (agent can tool-call) - allowlisted tools only. +var kernel = Settings.Kernel; kernel.Plugins.AddFromFunctions( "McpTools", - tools.Select(aiFunction => aiFunction.AsKernelFunction())); + discovered.Where(t => authorized.Contains(t.Name)).Select(aiFunction => aiFunction.AsKernelFunction())); -// 4) Enable auto function calling +// Enable auto function calling var exec = new OpenAIPromptExecutionSettings { FunctionChoiceBehavior = FunctionChoiceBehavior.Auto( @@ -36,7 +54,7 @@ }) }; -// 5) Agent uses MCP tools as needed +// Agent uses MCP tools as needed var agent = new ChatCompletionAgent { Name = "McpAgent", @@ -48,4 +66,5 @@ var prompt = "Use the 'add' tool to compute 1234 + 5678, then use the 'echo' tool to repeat the result."; await foreach (var response in agent.InvokeAsync(prompt)) - Console.WriteLine(response.Message.Content); \ No newline at end of file + Console.WriteLine(response.Message.Content); +return 0; diff --git a/PatternExplorer/patterns/MCP.md b/PatternExplorer/patterns/MCP.md index 86267b0..99ee716 100644 --- a/PatternExplorer/patterns/MCP.md +++ b/PatternExplorer/patterns/MCP.md @@ -1,12 +1,12 @@ --- { "title": "Model Context Protocol", - "summary": "Discover tools from an external MCP server at runtime instead of compiling them in.", + "summary": "Discover tools from a pinned, sandboxed MCP server at runtime, but bind only an explicit allowlist.", "category": "Orchestration", - "risk": "Downloads and runs an external MCP server via npx (unpinned); its tools act with your local privileges.", + "risk": "Runs a third-party MCP server; discovery and authorization are kept separate so only an explicit allowlist is ever bound.", "projects": [ - { "flavor": "AgentFramework", "path": "MCP.AgentFramework", "note": "Needs npx on PATH - the MCP server is fetched with npx on first run." }, - { "flavor": "SemanticKernel", "path": "MCP.SemanticKernel", "note": "Needs npx on PATH - the MCP server is fetched with npx on first run." } + { "flavor": "AgentFramework", "path": "MCP.AgentFramework", "note": "Needs Docker or Podman - the pinned server runs in a locked-down container, not on the host." }, + { "flavor": "SemanticKernel", "path": "MCP.SemanticKernel", "note": "Needs Docker or Podman - the pinned server runs in a locked-down container, not on the host." } ] } --- @@ -34,17 +34,23 @@ untrusted instruction source. ## How the demo works -Both samples spawn the official reference server, `@modelcontextprotocol/server-everything`, over -stdio via `npx -y` — no credentials needed, but **npx must be on PATH** and the first run -downloads the package. They call `ListToolsAsync()`, register everything it returns with the -agent, and send one prompt: *"Use the 'add' tool to compute 1234 + 5678, then use the 'echo' tool -to repeat the result."* Two chained calls against tools that were unknown at compile time. +Both samples run the official reference server, `@modelcontextprotocol/server-everything`, over +stdio — but inside the same locked-down local container the **CodeAct** sample uses, not on the +host. They call `ListToolsAsync()`, but unlike the pre-fix version they do **not** register +everything the server returns: discovery and authorization are two separate steps, and only an +explicit allowlist (`add`, `echo`) is bound to the agent. Then they send one prompt: *"Use the +'add' tool to compute 1234 + 5678, then use the 'echo' tool to repeat the result."* Two chained +calls against tools that were unknown at compile time — and unreachable if they weren't +allowlisted. ```mermaid flowchart LR - P[Program starts] --> S[npx spawns MCP server
server-everything over stdio] - S --> L[ListToolsAsync] - L --> A[Agent registered with
discovered tools] + P[Program starts] --> D{Docker/Podman
available?} + D -- no --> X[Fail closed - exit] + D -- yes --> S[docker run spawns MCP server
server-everything over stdio] + S --> L[ListToolsAsync: many tools] + L --> AL[SelectAuthorized:
allowlist add, echo only] + AL --> A[Agent registered with
allowlisted tools only] A -->|add 1234 and 5678| S A -->|echo the result| S S --> R[Final answer 6912] @@ -57,25 +63,69 @@ into the `ChatClientAgent` constructor. Semantic Kernel converts each one with `FunctionChoiceBehavior.Auto` with `RetainArgumentTypes = true` — without that, the numeric arguments reach the `add` tool as strings and the call fails schema validation. +## Security: pin, sandbox, and allowlist the server + +An MCP server is a third-party tool provider: its binary runs on your machine (or one you +control) and its tool descriptions land straight in your prompt. The pre-fix shape of this +sample — `npx -y @modelcontextprotocol/server-everything` (an **unpinned**, latest-at-run-time +package), executed directly **on the host with the application's own environment**, with +**every discovered tool bound to the agent** — is exactly what this sample now exists to *not* +do. The same rule this repo applies to every pattern that executes untrusted work: + +> **The model proposes. A constrained host validates and executes. Untrusted execution +> never inherits the application's authority.** + +Concretely: + +- **Pin the server.** `MCP.AgentFramework/Sandbox/Dockerfile` bakes in an exact version + (`@modelcontextprotocol/server-everything@2025.8.18`) at build time — no "whatever is + latest today" resolved at run time. +- **Run it in the same constrained container as CodeAct.** The pinned server is launched with + `Shared.Sandbox.SandboxRunner.BuildRunArguments`, the identical locked-down-container boundary + the **CodeAct** sample uses for model-generated code — see that pattern's security section for + the full flag-by-flag walkthrough. +- **Pass no host environment or credentials.** The container gets nothing from the host process; + the server never sees an API key, a token, or a host env var it wasn't explicitly handed. +- **Deny network unless the chosen server needs it.** This demo server only needs stdio, so + `Network: false` — `--network none`. A server that legitimately calls out (a real GitHub or + database MCP server) would need that grant made explicit and justified, not defaulted on. +- **Keep discovery and authorization separate.** `ListToolsAsync()` still returns everything the + server advertises — discovering a tool never grants it. +- **Bind an explicit allowlist.** `McpToolBinding.SelectAuthorized` filters the discovered list + down to exactly `add` and `echo` before anything reaches the agent, and **fails closed** — + throwing `InvalidOperationException` — if an allowlisted tool goes missing, on the theory that + a missing expected tool means the server isn't the one that was pinned. +- **Fail closed when the boundary is unavailable.** No Docker or Podman means no sandbox, which + means no MCP server — the sample exits with an explanatory message rather than falling back to + running the third-party server on the host. Same double opt-in as CodeAct + (`AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION` + `AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION`) + would be required to override that, and this sample does not wire that override up. + +The bundled Dockerfile/sandbox is a teaching boundary, not a production one — see CodeAct's +"Included sandbox ≠ production-ready sandbox" for the caveats, which apply here unchanged. + ## Key APIs | Agent Framework | Semantic Kernel | |---|---| | `McpClient.CreateAsync(new StdioClientTransport(...))` | `McpClient.CreateAsync(new StdioClientTransport(...))` | | `mcpClient.ListToolsAsync()` | `mcpClient.ListToolsAsync()` | -| `tools.Cast()` into `ChatClientAgent` | `kernel.Plugins.AddFromFunctions("McpTools", tools.Select(f => f.AsKernelFunction()))` | +| `McpToolBinding.SelectAuthorized(discovered, allowed)` | `McpToolBinding.SelectAuthorized(discovered, allowed)` | +| `tools.Where(authorized).Cast()` into `ChatClientAgent` | `kernel.Plugins.AddFromFunctions("McpTools", tools.Where(authorized).Select(f => f.AsKernelFunction()))` | | — no conversion, `McpClientTool` is an `AIFunction` | `FunctionChoiceBehaviorOptions { RetainArgumentTypes = true }` | -`StdioClientTransportOptions` is what launches the process: `Command = "npx"` with -`Arguments = ["-y", "@modelcontextprotocol/server-everything"]`. Point it at any other executable -and the rest of the code is unchanged. +`StdioClientTransportOptions` is what launches the process: `Command = "docker"` with +`Arguments = SandboxRunner.BuildRunArguments(sandbox, [])` — an empty command list, because the +image's own `ENTRYPOINT` is the pinned server binary. Point `SandboxOptions.Image` at any other +sandboxed server and the rest of the code is unchanged. ## What to watch in the output -The Agent Framework sample prints `MCP tools: ` followed by the full discovered list — `echo`, -`add`, `longRunningOperation`, `printEnv` and the rest — which is the whole point of the pattern -made visible; nothing in the source names them. Then comes the agent's answer containing -**6912**, a number only the remote `add` tool produced. The Semantic Kernel sample skips the -listing and prints just the streamed answer. If the run hangs or dies at startup, npx is missing -or still downloading. Compare with **ToolUse** for locally compiled tools, and **HostedTools** -for tools the model provider runs on your behalf. +Both samples print `Discovered: ` followed by the full list the container hands back — `echo`, +`add`, `longRunningOperation`, `printEnv` and the rest — then `Bound to the agent: ` showing +exactly `echo, add`, the visible proof that discovery and authorization are different steps. Then +comes the agent's answer containing **6912**, a number only the remote `add` tool produced. If the +run exits immediately with "No container runtime available", Docker/Podman isn't installed or the +daemon isn't running. Compare with **ToolUse** for locally compiled tools, **HostedTools** for +tools the model provider runs on your behalf, and **CodeAct** for the container sandbox this +sample reuses. diff --git a/PatternExplorer/patterns/ProgressiveToolDisclosure.md b/PatternExplorer/patterns/ProgressiveToolDisclosure.md index 8d860f3..f972a64 100644 --- a/PatternExplorer/patterns/ProgressiveToolDisclosure.md +++ b/PatternExplorer/patterns/ProgressiveToolDisclosure.md @@ -67,5 +67,7 @@ answered with `[tool definitions sent to the model: 1 of 16 available]`: the mod only `search_tools`, used it, and confirmed what it loaded. Turn two shows `3 of 16` — search_tools plus the two discovered tools — and the actual answers. The closing line names what was loaded on demand and notes that the other 13 definitions never entered -the context. **MCP** is the contrast case, binding every discovered tool up front; -**SkillLearning** applies the same frontmatter-first trick to learned procedures. +the context. **MCP** is a related contrast case: it still sends every *bound* tool's +definition on every call rather than searching on demand, though it narrows which +discovered tools get bound at all with an explicit allowlist. **SkillLearning** applies +the same frontmatter-first trick to learned procedures. diff --git a/README.md b/README.md index 5fa7c1b..725d140 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ the catalog together; each result states its scope limits and cites a primary so | Handoff | Agents transferring the conversation to each other | | HostedTools | Server-side code interpreter and web search tools | | InterAgentCommunication.A2A | Agent-to-agent communication over the A2A protocol | -| MCP | Consuming Model Context Protocol tool servers | +| MCP | Consuming Model Context Protocol tool servers, sandboxed and allowlisted | | Magentic | Manager-driven open-ended multi-agent orchestration | | MultiAgentCollaboration | Group-chat orchestration | | OrchestratorWorkers | Dynamic decomposition into validated tasks for a fixed worker registry | @@ -157,9 +157,10 @@ the catalog together; each result states its scope limits and cites a primary so ## Setup -Requires the .NET 10 SDK and an Azure OpenAI deployment. The `CodeAct` sample additionally -requires Docker or Podman — it sandboxes the code the model writes and refuses to run -without isolation (see the security section below). +Requires the .NET 10 SDK and an Azure OpenAI deployment. The `CodeAct` and `MCP` samples +additionally require Docker or Podman — both sandbox untrusted execution (model-generated +code for `CodeAct`, a third-party MCP server for `MCP`) and refuse to run without isolation +(see the security section below). Configuration is read from `settings/appsettings.json` (linked into every project), environment variables, and user secrets. **Don't put your API key in `appsettings.json`** — it's tracked in git. Use user secrets instead: @@ -210,6 +211,20 @@ Concretely, for the `CodeAct` sample: no ambient credentials. Never execute model-generated code in the application process or on the application host. +The same rule applies to the `MCP` sample: a third-party MCP server is untrusted code too. +`@modelcontextprotocol/server-everything` is pinned at an exact version and baked into an +image at build time, run in the same locked-down container as `CodeAct` (no network, no +host environment or credentials, read-only filesystem, dropped capabilities, non-root), +and only an explicit allowlist (`add`, `echo`) of its discovered tools is ever bound to the +agent — discovery and authorization are kept separate. Build the image once before running +either flavor: + +```bash +docker build -t agentic-patterns/mcp-server-everything:2025.8.18 MCP.AgentFramework/Sandbox +``` + +See `PatternExplorer/patterns/MCP.md` for the full walkthrough. + The A2A samples need the server running first: ```bash From af79fc55060fe5435f8a84b1cd383d87620cdaaa Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 14:25:09 +0200 Subject: [PATCH 05/16] fix(mcp): fix round 1 - remove false override hint, name containers, fix docs - Fail-closed message in both Program.cs no longer advertises the AGENTIC_PATTERNS_* override: neither variable is ever read by the MCP samples (only CodeRunnerFactory reads them), so the old message told a Docker-less user to do something that does nothing. - SandboxOptions now gets an explicit ContainerName on the stdio path so the container is nameable/killable even though McpClient (not SandboxRunner.RunAsync) owns the process lifecycle here. - McpToolBinding.SelectAuthorized takes HashSet instead of IReadOnlySet so allowed.Comparer governs both the match and the missing-check consistently, and both filter and diff run through Distinct/Except with that same comparer so duplicate discovered names don't leak into the result. Mirrored byte-identically into the SK twin. Added tests for case-insensitive allowlists and duplicate tool names. - README.md now says plainly that MCP cannot run inside the Pattern Explorer container (no docker client/socket there) instead of shipping a silently-broken run button. - MCP.md now says the sandbox image must be built manually first (unlike CodeAct, which builds its image on first run). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- AgenticPatterns.Tests/McpToolBindingTests.cs | 11 +++++++++++ MCP.AgentFramework/McpToolBinding.cs | 9 ++++++--- MCP.AgentFramework/Program.cs | 16 ++++++++++------ MCP.SemanticKernel/McpToolBinding.cs | 9 ++++++--- MCP.SemanticKernel/Program.cs | 16 ++++++++++------ PatternExplorer/patterns/MCP.md | 9 +++++++-- README.md | 6 +++++- 7 files changed, 55 insertions(+), 21 deletions(-) diff --git a/AgenticPatterns.Tests/McpToolBindingTests.cs b/AgenticPatterns.Tests/McpToolBindingTests.cs index 52ce4f3..1e24309 100644 --- a/AgenticPatterns.Tests/McpToolBindingTests.cs +++ b/AgenticPatterns.Tests/McpToolBindingTests.cs @@ -15,4 +15,15 @@ public void OnlyAllowlistedToolsAreBound() => [Fact] public void AMissingAllowlistedToolFailsClosed() => Assert.Throws(() => McpToolBinding.SelectAuthorized(["echo"], Allowed)); + + [Fact] + public void CaseInsensitiveAllowlistUsesTheAllowlistsOwnComparer() + { + var allowed = new HashSet(["Add", "Echo"], StringComparer.OrdinalIgnoreCase); + Assert.Equal(["add", "echo"], McpToolBinding.SelectAuthorized(["add", "echo"], allowed).Order()); + } + + [Fact] + public void DuplicateDiscoveredNamesAreNotDuplicatedInTheResult() => + Assert.Equal(["add", "echo"], McpToolBinding.SelectAuthorized(["add", "add", "echo"], Allowed).Order()); } diff --git a/MCP.AgentFramework/McpToolBinding.cs b/MCP.AgentFramework/McpToolBinding.cs index 43c690b..4ca0e7d 100644 --- a/MCP.AgentFramework/McpToolBinding.cs +++ b/MCP.AgentFramework/McpToolBinding.cs @@ -4,11 +4,14 @@ public static class McpToolBinding { /// Discovery and authorization are separate steps: discovering a tool never grants it. /// Fails closed - a missing allowlisted tool means the server is not the one we pinned. + /// Takes a HashSet (not IReadOnlySet) so its own Comparer governs both the match and the + /// missing-check - one comparer for the whole function, not the set's for one direction + /// and the default for the other. public static IReadOnlyList SelectAuthorized(IEnumerable discovered, - IReadOnlySet allowed) + HashSet allowed) { - var found = discovered.Where(allowed.Contains).ToList(); - var missing = allowed.Except(found).ToList(); + var found = discovered.Where(allowed.Contains).Distinct(allowed.Comparer).ToList(); + var missing = allowed.Except(found, allowed.Comparer).ToList(); if (missing.Count > 0) throw new InvalidOperationException( $"The MCP server does not advertise the required tool(s): {string.Join(", ", missing)}."); diff --git a/MCP.AgentFramework/Program.cs b/MCP.AgentFramework/Program.cs index fb0324a..b435a3a 100644 --- a/MCP.AgentFramework/Program.cs +++ b/MCP.AgentFramework/Program.cs @@ -8,21 +8,25 @@ const string image = "agentic-patterns/mcp-server-everything:2025.8.18"; var allowed = new HashSet(["add", "echo"], StringComparer.Ordinal); -// Fail closed: no sandbox, no MCP server. Same double opt-in as CodeAct for the unsafe path. +// Fail closed: no sandbox, no MCP server. Unlike CodeAct, this sample has no host-execution +// fallback at all - there is nothing to opt into, so the message must not imply otherwise. if (!SandboxRunner.IsAvailable("docker")) { Console.Error.WriteLine( "No container runtime available. This sample runs a third-party MCP server, which is " + - "untrusted code; it will not be started on the host. Install Docker, or set " + - "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION=true and " + - "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION=I_UNDERSTAND_THIS_RUNS_UNTRUSTED_CODE_ON_MY_HOST."); + "untrusted code; it will not be started on the host. Install Docker or Podman to run it."); return 1; } // The server speaks stdio, so the container IS the transport: no network, no host environment, -// no credentials, read-only filesystem, dropped capabilities, bounded pids and memory. +// no credentials, read-only filesystem, dropped capabilities, bounded pids and memory. Named +// explicitly (not just left to SandboxRunner.RunAsync's own naming, which this stdio path +// doesn't go through) so the container can be torn down by name if the process is killed - +// SIGKILLing the `docker run` CLI does not stop the daemon-side container. +// ponytail: no automatic kill-by-name wired up on this path (McpClient owns the process, not +// RunAsync) - add it if this sample stops being a short-lived demo. var sandbox = new SandboxOptions(image, Network: false, Memory: "256m", PidsLimit: 64, Interactive: true, - User: null); + User: null, ContainerName: $"mcp-sandbox-{Guid.NewGuid():N}"); await using var mcpClient = await McpClient.CreateAsync(new StdioClientTransport(new StdioClientTransportOptions { Name = "MCPServer", diff --git a/MCP.SemanticKernel/McpToolBinding.cs b/MCP.SemanticKernel/McpToolBinding.cs index e609d9e..5822ce7 100644 --- a/MCP.SemanticKernel/McpToolBinding.cs +++ b/MCP.SemanticKernel/McpToolBinding.cs @@ -4,11 +4,14 @@ public static class McpToolBinding { /// Discovery and authorization are separate steps: discovering a tool never grants it. /// Fails closed - a missing allowlisted tool means the server is not the one we pinned. + /// Takes a HashSet (not IReadOnlySet) so its own Comparer governs both the match and the + /// missing-check - one comparer for the whole function, not the set's for one direction + /// and the default for the other. public static IReadOnlyList SelectAuthorized(IEnumerable discovered, - IReadOnlySet allowed) + HashSet allowed) { - var found = discovered.Where(allowed.Contains).ToList(); - var missing = allowed.Except(found).ToList(); + var found = discovered.Where(allowed.Contains).Distinct(allowed.Comparer).ToList(); + var missing = allowed.Except(found, allowed.Comparer).ToList(); if (missing.Count > 0) throw new InvalidOperationException( $"The MCP server does not advertise the required tool(s): {string.Join(", ", missing)}."); diff --git a/MCP.SemanticKernel/Program.cs b/MCP.SemanticKernel/Program.cs index 3f50d73..aab6486 100644 --- a/MCP.SemanticKernel/Program.cs +++ b/MCP.SemanticKernel/Program.cs @@ -11,21 +11,25 @@ const string image = "agentic-patterns/mcp-server-everything:2025.8.18"; var allowed = new HashSet(["add", "echo"], StringComparer.Ordinal); -// Fail closed: no sandbox, no MCP server. Same double opt-in as CodeAct for the unsafe path. +// Fail closed: no sandbox, no MCP server. Unlike CodeAct, this sample has no host-execution +// fallback at all - there is nothing to opt into, so the message must not imply otherwise. if (!SandboxRunner.IsAvailable("docker")) { Console.Error.WriteLine( "No container runtime available. This sample runs a third-party MCP server, which is " + - "untrusted code; it will not be started on the host. Install Docker, or set " + - "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION=true and " + - "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION=I_UNDERSTAND_THIS_RUNS_UNTRUSTED_CODE_ON_MY_HOST."); + "untrusted code; it will not be started on the host. Install Docker or Podman to run it."); return 1; } // The server speaks stdio, so the container IS the transport: no network, no host environment, -// no credentials, read-only filesystem, dropped capabilities, bounded pids and memory. +// no credentials, read-only filesystem, dropped capabilities, bounded pids and memory. Named +// explicitly (not just left to SandboxRunner.RunAsync's own naming, which this stdio path +// doesn't go through) so the container can be torn down by name if the process is killed - +// SIGKILLing the `docker run` CLI does not stop the daemon-side container. +// ponytail: no automatic kill-by-name wired up on this path (McpClient owns the process, not +// RunAsync) - add it if this sample stops being a short-lived demo. var sandbox = new SandboxOptions(image, Network: false, Memory: "256m", PidsLimit: 64, Interactive: true, - User: null); + User: null, ContainerName: $"mcp-sandbox-{Guid.NewGuid():N}"); await using var mcpClient = await McpClient.CreateAsync(new StdioClientTransport(new StdioClientTransportOptions { Name = "MCPServer", diff --git a/PatternExplorer/patterns/MCP.md b/PatternExplorer/patterns/MCP.md index 99ee716..544a075 100644 --- a/PatternExplorer/patterns/MCP.md +++ b/PatternExplorer/patterns/MCP.md @@ -5,8 +5,8 @@ "category": "Orchestration", "risk": "Runs a third-party MCP server; discovery and authorization are kept separate so only an explicit allowlist is ever bound.", "projects": [ - { "flavor": "AgentFramework", "path": "MCP.AgentFramework", "note": "Needs Docker or Podman - the pinned server runs in a locked-down container, not on the host." }, - { "flavor": "SemanticKernel", "path": "MCP.SemanticKernel", "note": "Needs Docker or Podman - the pinned server runs in a locked-down container, not on the host." } + { "flavor": "AgentFramework", "path": "MCP.AgentFramework", "note": "Needs Docker or Podman, and the sandbox image built first (docker build -t agentic-patterns/mcp-server-everything:2025.8.18 MCP.AgentFramework/Sandbox) - unlike CodeAct, this sample does not build it for you." }, + { "flavor": "SemanticKernel", "path": "MCP.SemanticKernel", "note": "Needs Docker or Podman, and the sandbox image built first (docker build -t agentic-patterns/mcp-server-everything:2025.8.18 MCP.AgentFramework/Sandbox) - unlike CodeAct, this sample does not build it for you." } ] } --- @@ -34,6 +34,11 @@ untrusted instruction source. ## How the demo works +Build the sandbox image once before running either flavor — +`docker build -t agentic-patterns/mcp-server-everything:2025.8.18 MCP.AgentFramework/Sandbox`. +Unlike **CodeAct**, this sample does **not** build its image automatically on first run; without +it, `docker run` exits 125 and the stdio transport dies on a pipe nothing is writing to. + Both samples run the official reference server, `@modelcontextprotocol/server-everything`, over stdio — but inside the same locked-down local container the **CodeAct** sample uses, not on the host. They call `ListToolsAsync()`, but unlike the pre-fix version they do **not** register diff --git a/README.md b/README.md index 725d140..423baef 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,11 @@ non-root user; the command above also binds only to loopback and drops Linux cap not expose Pattern Explorer directly to the internet: its run endpoints intentionally execute samples with the supplied credentials. Enabling the two optional CodeAct variables runs generated code directly inside this outer container instead of a nested Docker sandbox. The generated code -therefore shares the container's credentials, filesystem, and network access. +therefore shares the container's credentials, filesystem, and network access. The `MCP` sample +cannot run inside this container at all: it needs a container runtime of its own to sandbox the +MCP server, and this image ships neither a Docker client nor a daemon socket — running it here +always hits the fail-closed path and exits. Run `MCP` from a terminal with Docker/Podman +installed instead. Running a sample from the UI spawns `dotnet run` for that project and calls your Azure OpenAI deployment, exactly as running it from the terminal would. Samples that ask for approval get an From 649ae7dd036348c055aeab311e714bc2928d76e1 Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 14:45:42 +0200 Subject: [PATCH 06/16] fix(stigmergic): run the build gate inside the sandbox with limits and cleanup Compiling untrusted C# is still running untrusted code (build tasks, source generators, MSBuild targets), so the stigmergic build gate now runs through Shared.Sandbox instead of a bare host `dotnet build`, fails closed with no container runtime (same double opt-in AGENTIC_PATTERNS_* fallback as CodeAct), and guarantees workspace cleanup via try/finally around the round loop (including the success-path return that used to leak it). New BuildGate.cs extracts the testable pieces; SandboxRunner reads stdout/stderr concurrently, removing the sequential-ReadToEndAsync pipe deadlock. Mounts the writable tmpfs at /tmp (not /build) - verified by hand that /build alone still fails, because the .NET CLI's first-run mutex is hardcoded under /tmp/.dotnet/shm regardless of HOME/DOTNET_CLI_HOME/TMPDIR. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- .../AgenticPatterns.Tests.csproj | 1 + .../StigmergicBuildGateTests.cs | 218 ++++++++++++++++++ .../patterns/StigmergicCoordination.md | 18 +- .../BuildGate.cs | 149 ++++++++++++ .../Program.cs | 91 ++++---- 5 files changed, 431 insertions(+), 46 deletions(-) create mode 100644 AgenticPatterns.Tests/StigmergicBuildGateTests.cs create mode 100644 StigmergicCoordination.AgentFramework/BuildGate.cs diff --git a/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj b/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj index 917e359..5142e47 100644 --- a/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj +++ b/AgenticPatterns.Tests/AgenticPatterns.Tests.csproj @@ -37,6 +37,7 @@ + |SloganModule.cs| WS W2 -->|PricingModule.cs| WS W3 -->|BriefAssembler.cs| WS - WS --> G{dotnet build
mechanical gate} + WS --> G{sandboxed dotnet build
mechanical gate} G -->|error CS0535| W2 G -->|all contracts satisfied| DONE[0 messages exchanged] ``` The model's code is compiled but **never executed** — the compiler is the integration test. -That keeps the sample inside the repository's untrusted-execution rule; a production version -would add behavioral contract tests running in a sandbox (see **CodeAct** for what that takes). +But compiling is not risk-free either: build tasks, source generators, and MSBuild targets all +run as part of a build, not just at execution time, so `dotnet build` still runs inside the +same locked-down container boundary **CodeAct** uses for model-generated code — no network, +read-only source mount, a bounded writable build directory, capped CPU/memory/pids, a +wall-clock timeout, and bounded output. The sample fails closed with no fallback to a host +build unless the same double opt-in CodeAct offers is set (and even then the timeout and +source-size cap still apply). A production version would go further and add behavioral +contract tests running in that same sandbox. ## Key APIs @@ -85,8 +91,8 @@ no workflow. The coordination machinery is the environment itself. - `ChatClientAgent(client, instructions, name)` — one plain agent per worker, no tools. - `Task.WhenAll(...)` — workers run concurrently precisely because they share no channel. - `File.WriteAllText` into a shared workspace — the write *is* the coordination act. -- `Process.Start("dotnet", "build ...")` — the mechanical gate; its `error CS*` lines are - parsed and routed to the worker owning the failing file. +- `BuildGate.RunAsync` / `SandboxRunner.RunAsync` — the mechanical gate, run inside a + container; its `error CS*` lines are parsed and routed to the worker owning the failing file. ## What to watch in the output diff --git a/StigmergicCoordination.AgentFramework/BuildGate.cs b/StigmergicCoordination.AgentFramework/BuildGate.cs new file mode 100644 index 0000000..b324ea3 --- /dev/null +++ b/StigmergicCoordination.AgentFramework/BuildGate.cs @@ -0,0 +1,149 @@ +using Shared.Sandbox; + +namespace StigmergicCoordination.AgentFramework; + +/// +/// The mechanical gate: `dotnet build` over the shared workspace, run inside the SAME +/// constrained-execution boundary CodeAct uses for model-generated code. Compiling untrusted +/// source is still running untrusted code — build tasks, source generators, and MSBuild +/// targets all execute as part of a build, not just at run time — so the workspace is +/// compiled in a locked-down container (no network, read-only source mount, bounded writable +/// build directory, capped CPU/memory/pids, wall-clock timeout, bounded output), never +/// directly on the host. See the repository's untrusted-execution rule. +/// +public static class BuildGate +{ + public const long MaxSourceBytes = 64 * 1024; + private const string ContainerImage = "mcr.microsoft.com/dotnet/sdk:10.0"; + private const int MaxOutputCharacters = 65_536; + + // Same two variables CodeAct's CodeRunnerFactory reads (Execution/CodeRunnerFactory.cs) - + // duplicated here rather than referenced across projects, since no sample in this repo + // references another sample's project. Only THIS sample's fail-closed path offers them, + // because only this sample actually reads them - unlike MCP, which has no host fallback. + public const string UnsafeEnableVariable = "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION"; + public const string UnsafeEnableValue = "true"; + public const string UnsafeAcknowledgementVariable = "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION"; + public const string UnsafeAcknowledgementValue = "I_UNDERSTAND_THIS_RUNS_UNTRUSTED_CODE_ON_MY_HOST"; + + private const string NuGetConfig = + """ + + + + + + + """; + + public static string FailClosedMessage => + $""" + No container runtime available. This sample compiles model-generated C# files, which + is untrusted code - build tasks, source generators, and MSBuild targets all run during + a build, not just at execution. It will not be compiled on the host. + + Install Docker or Podman to run it, or explicitly opt into an unsandboxed host build + (the build still gets a timeout and a source size cap, but none of the container + isolation) with both: + 1. {UnsafeEnableVariable}={UnsafeEnableValue} + 2. {UnsafeAcknowledgementVariable}={UnsafeAcknowledgementValue} + """; + + /// Double opt-in, same shape as CodeAct - one variable alone is never enough. + public static bool IsUnsafeHostBuildRequested() => + Environment.GetEnvironmentVariable(UnsafeEnableVariable) == UnsafeEnableValue && + Environment.GetEnvironmentVariable(UnsafeAcknowledgementVariable) == UnsafeAcknowledgementValue; + + /// Oversized files never reach the compiler (sandboxed or not) - checked up front. + public static string? OversizedSourceError(string workspace) + { + foreach (var file in Directory.GetFiles(workspace, "*.cs")) + if (new FileInfo(file).Length > MaxSourceBytes) + return $"{Path.GetFileName(file)}: error AP0001: source exceeds {MaxSourceBytes} bytes"; + return null; + } + + public static List ParseErrors(string combinedOutput) => + [.. combinedOutput.Split('\n').Where(l => l.Contains(": error ")).Select(l => l.Trim()).Distinct()]; + + /// + /// `SandboxOptions` defaults to `--read-only`, so copying the workspace out of the + /// read-only `/src` mount needs a writable mount - a bounded tmpfs. The tmpfs (and HOME / + /// DOTNET_CLI_HOME) land on `/tmp`, exactly where `ContainerCodeRunner` puts them - NOT on + /// a separate `/build` tmpfs, which was tried first and measured to fail: the .NET CLI's + /// interprocess "first run" mutex is hardcoded to `/tmp/.dotnet/shm`, ignoring HOME and + /// TMPDIR, so a writable `/build` with a still-read-only `/tmp` fails on every run with + /// "mkdir(/tmp/.dotnet/shm/session1) == -1; errno == EROFS" before the compiler ever sees + /// the source. Mounting the tmpfs at `/tmp` is what `ContainerCodeRunner` already does, is + /// verified working here, and keeps the same environment variables it passes: what lets + /// the SDK run offline as a non-root user with no writable home. + /// + public static SandboxOptions SandboxedOptions(string workspace) => new( + Image: ContainerImage, + Network: false, Memory: "1g", Cpus: "2", PidsLimit: 256, + Timeout: TimeSpan.FromMinutes(3), + Tmpfs: "/tmp:rw,exec,nosuid,nodev,size=1g", + Environment: new Dictionary + { + ["HOME"] = "/tmp", + ["DOTNET_CLI_HOME"] = "/tmp/dotnet", + ["DOTNET_NOLOGO"] = "1", + ["DOTNET_CLI_TELEMETRY_OPTOUT"] = "1", + }, + Mounts: [(workspace, "/src", true)]); + + /// + /// Runs the gate. is decided ONCE by the caller (before any + /// worker runs) rather than re-checked every round, mirroring CodeAct's one-time runner + /// selection - availability cannot silently change mid-run into a different security + /// posture. + /// + public static async Task> RunAsync( + string workspace, bool useSandbox, CancellationToken cancellationToken) + { + var oversized = OversizedSourceError(workspace); + if (oversized is not null) return [oversized]; + + // Ruling 3: written every round (cheap, idempotent) so a restore that ever tries to + // reach a feed fails loudly instead of silently depending on Network: false alone. + await File.WriteAllTextAsync(Path.Combine(workspace, "NuGet.config"), NuGetConfig, cancellationToken); + + if (!useSandbox) return await HostBuildAsync(workspace, cancellationToken); + + var result = await SandboxRunner.RunAsync(SandboxedOptions(workspace), + ["sh", "-c", "cp -r /src /tmp/build && cd /tmp/build && dotnet build -nologo --verbosity quiet"], + stdin: null, cancellationToken); + + if (result.TimedOut) return ["error AP0002: the build gate timed out"]; + return ParseErrors(result.StdOut + result.StdErr); + } + + // ponytail: unsandboxed - only reachable behind the double opt-in in IsUnsafeHostBuildRequested, + // same shape as CodeAct's UnsafeHostCodeRunner. Upgrade path is the same: delete this method + // once every environment running the sample has a container runtime. + private static async Task> HostBuildAsync(string workspace, CancellationToken cancellationToken) + { + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(TimeSpan.FromMinutes(3)); + + using var process = System.Diagnostics.Process.Start( + new System.Diagnostics.ProcessStartInfo("dotnet", "build -nologo --verbosity quiet") + { + WorkingDirectory = workspace, + RedirectStandardOutput = true, + RedirectStandardError = true, + })!; + try + { + var stdoutTask = BoundedReader.ReadBoundedAsync(process.StandardOutput, MaxOutputCharacters, timeoutCts.Token); + var stderrTask = BoundedReader.ReadBoundedAsync(process.StandardError, MaxOutputCharacters, timeoutCts.Token); + await process.WaitForExitAsync(timeoutCts.Token); + return ParseErrors(await stdoutTask + await stderrTask); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + if (!process.HasExited) process.Kill(entireProcessTree: true); + return ["error AP0002: the build gate timed out"]; + } + } +} diff --git a/StigmergicCoordination.AgentFramework/Program.cs b/StigmergicCoordination.AgentFramework/Program.cs index 79749a7..e1cd1ad 100644 --- a/StigmergicCoordination.AgentFramework/Program.cs +++ b/StigmergicCoordination.AgentFramework/Program.cs @@ -1,13 +1,28 @@ -using System.Diagnostics; using System.Text.RegularExpressions; using Microsoft.Agents.AI; using Shared; +using Shared.Sandbox; +using StigmergicCoordination.AgentFramework; // Stigmergic coordination: N workers build components of one system WITHOUT exchanging // a single message. All coordination flows through the shared environment — a workspace // directory, compiler-enforced C# contracts, and a build gate. The orchestrator only // launches workers and runs the gate; it never relays information between agents. // Contrast MultiAgentCollaboration, where the same domain task is coordinated by dialogue. +// +// The gate compiles model-written files, which is untrusted code — build tasks, source +// generators, and MSBuild targets all run during a build. It runs inside the same +// constrained-execution boundary CodeAct uses (see BuildGate.cs) and FAILS CLOSED when no +// container runtime is available, unless the same double opt-in CodeAct offers is set. + +// Fail closed BEFORE creating a workspace or spending a single model call: no container +// runtime and no explicit double opt-in means the sample refuses to compile anything. +var useSandbox = SandboxRunner.IsAvailable("docker"); +if (!useSandbox && !BuildGate.IsUnsafeHostBuildRequested()) +{ + Console.Error.WriteLine(BuildGate.FailClosedMessage); + return 1; +} var workspace = Directory.CreateDirectory( Path.Combine(Path.GetTempPath(), "stigmergy", Guid.NewGuid().ToString("N"))).FullName; @@ -101,49 +116,45 @@ async Task ProduceAsync((string File, string Role, string Brief) worker, string Console.WriteLine($"[worker] {worker.Role} -> {worker.File} ({code.Split('\n').Length} lines, no messages to other workers)"); } -// ---- The mechanical gate: dotnet build over the shared workspace ---- +// ---- The mechanical gate: BuildGate.RunAsync, sandboxed unless the opt-in fallback fired ---- -async Task> BuildAsync() +try { - var process = Process.Start(new ProcessStartInfo("dotnet", "build -nologo --verbosity quiet") - { - WorkingDirectory = workspace, - RedirectStandardOutput = true, - RedirectStandardError = true, - })!; - var output = await process.StandardOutput.ReadToEndAsync() + await process.StandardError.ReadToEndAsync(); - await process.WaitForExitAsync(); - return [.. output.Split('\n').Where(l => l.Contains(": error ")).Select(l => l.Trim()).Distinct()]; -} - -await Task.WhenAll(workers.Select(w => ProduceAsync(w, ""))); + await Task.WhenAll(workers.Select(w => ProduceAsync(w, ""))); -for (var round = 1; round <= 3; round++) -{ - Console.WriteLine($"\n=== Build gate: round {round} ==="); - var errors = await BuildAsync(); - if (errors.Count == 0) + for (var round = 1; round <= 3; round++) { - Console.WriteLine("PASSED — every component satisfies the shared contracts."); - Console.WriteLine("\n---- Files in the shared environment ----"); - foreach (var f in Directory.GetFiles(workspace, "*.cs").Order()) - Console.WriteLine($"\n>>> {Path.GetFileName(f)}\n{File.ReadAllText(f).Trim()}"); - Console.WriteLine("\nMessages exchanged between workers: 0. The workspace did all the talking."); - return; + Console.WriteLine($"\n=== Build gate: round {round} ==="); + var errors = await BuildGate.RunAsync(workspace, useSandbox, CancellationToken.None); + if (errors.Count == 0) + { + Console.WriteLine("PASSED — every component satisfies the shared contracts."); + Console.WriteLine("\n---- Files in the shared environment ----"); + foreach (var f in Directory.GetFiles(workspace, "*.cs").Order()) + Console.WriteLine($"\n>>> {Path.GetFileName(f)}\n{File.ReadAllText(f).Trim()}"); + Console.WriteLine("\nMessages exchanged between workers: 0. The workspace did all the talking."); + return 0; + } + + foreach (var error in errors) Console.WriteLine($" {error}"); + + // Errors route by file name to the worker that owns the file — the trace in the + // environment is the only feedback channel, and it carries the REAL contract with it. + foreach (var group in errors.GroupBy(e => workers.FirstOrDefault(w => e.Contains(w.File)).File).Where(g => g.Key is not null)) + { + var worker = workers.First(w => w.File == group.Key); + Console.WriteLine($" -> gate feedback for {worker.Role}: rework {worker.File} against the real contract"); + await ProduceAsync(worker, + $"Your previous {worker.File} failed the build gate:\n{string.Join("\n", group)}\n\n" + + $"The authoritative shared contract file Contracts.cs is:\n{Contracts}\nRewrite the complete file so it compiles against it."); + } } - foreach (var error in errors) Console.WriteLine($" {error}"); - - // Errors route by file name to the worker that owns the file — the trace in the - // environment is the only feedback channel, and it carries the REAL contract with it. - foreach (var group in errors.GroupBy(e => workers.FirstOrDefault(w => e.Contains(w.File)).File).Where(g => g.Key is not null)) - { - var worker = workers.First(w => w.File == group.Key); - Console.WriteLine($" -> gate feedback for {worker.Role}: rework {worker.File} against the real contract"); - await ProduceAsync(worker, - $"Your previous {worker.File} failed the build gate:\n{string.Join("\n", group)}\n\n" + - $"The authoritative shared contract file Contracts.cs is:\n{Contracts}\nRewrite the complete file so it compiles against it."); - } + Console.WriteLine("\nFAILED — components still do not satisfy the contracts after 3 rounds."); + return 1; +} +finally +{ + // Guaranteed cleanup: the workspace must not survive a crash or the success return above. + try { Directory.Delete(workspace, recursive: true); } catch (IOException) { } } - -Console.WriteLine("\nFAILED — components still do not satisfy the contracts after 3 rounds."); From ec57258b9b1a01744b6b8b6f660e864f3930daac Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 15:05:36 +0200 Subject: [PATCH 07/16] fix(stigmergic): stop discarding the gate's exit code, fix umask-dependent perms C1: BuildGate.InterpretResult stops treating a nonzero exit with no parsed compiler diagnostic as PASSED (permission failures, pids-limit kills, and similar now surface as a synthetic AP0003 error) - reproduced live with PidsLimit: 1 ("Cannot fork") and pinned as a Docker-gated regression test. C2: workspace directories/files are now forced world-readable via explicit File.SetUnixFileMode after creation. Directory.CreateDirectory(path, mode) alone - the same call ContainerCodeRunner.CreateRunDirectory makes - is NOT enough: mkdir()'s mode argument is itself masked by the process umask, verified by hand (0755 requested, 0700 back under umask 077). chmod() is not subject to umask, so an explicit SetUnixFileMode call after creation is what actually makes the permissions umask-independent. I3: added real tests that actually enter HostBuildAsync (a genuinely broken file, and cancellation of a live process), replacing one that threw before reaching it. Fixed the underlying orphan: HostBuildAsync now kills the process on caller cancellation too, not just on timeout. Minor: SIGINT now deletes the workspace directly (the round loop's try/finally can't be trusted to run on a signal); the finally/handler also catch UnauthorizedAccessException; docs note the stock vs. offline-cached image difference. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- .../StigmergicBuildGateTests.cs | 142 +++++++++++++++++- .../patterns/StigmergicCoordination.md | 13 +- .../BuildGate.cs | 80 +++++++++- .../Program.cs | 28 +++- 4 files changed, 242 insertions(+), 21 deletions(-) diff --git a/AgenticPatterns.Tests/StigmergicBuildGateTests.cs b/AgenticPatterns.Tests/StigmergicBuildGateTests.cs index 37ca1c7..f3b1b2d 100644 --- a/AgenticPatterns.Tests/StigmergicBuildGateTests.cs +++ b/AgenticPatterns.Tests/StigmergicBuildGateTests.cs @@ -121,16 +121,120 @@ public void FailClosedMessageNamesBothOverrideVariables() Assert.Contains(BuildGate.UnsafeAcknowledgementVariable, BuildGate.FailClosedMessage); } - // ---- host fallback wiring: caller cancellation must not be reported as a timeout ---- + // ---- C1: the exit code is never discarded - a nonzero exit with no parsed compiler + // diagnostic must not read as a pass. Pure, so no Docker/host build needed to pin it. ---- [Fact] - public async Task CallerCancellationDuringHostBuildIsNotConvertedIntoATimeoutResult() + public void NonzeroExitWithNoParsedErrorBecomesASyntheticGateError() + { + var result = new SandboxResult(1, "", "cp: cannot access '/src': Permission denied\n", TimedOut: false); + var errors = BuildGate.InterpretResult(result); + Assert.Single(errors); + Assert.Contains("error AP0003", errors[0]); + Assert.Contains("Permission denied", errors[0]); + } + + [Fact] + public void ZeroExitWithNoOutputIsAGenuinePass() => + Assert.Empty(BuildGate.InterpretResult(new SandboxResult(0, "", "", TimedOut: false))); + + [Fact] + public void NonzeroExitWithAParsedCompilerErrorReportsOnlyTheCompilerError() + { + var result = new SandboxResult(1, "Foo.cs(1,1): error CS0535: whatever\n", "", TimedOut: false); + var errors = BuildGate.InterpretResult(result); + Assert.Single(errors); + Assert.Contains("CS0535", errors[0]); + Assert.DoesNotContain(errors, e => e.Contains("AP0003")); + } + + [Fact] + public void TimedOutTakesPriorityOverExitCode() => + Assert.Equal(["error AP0002: the build gate timed out"], + BuildGate.InterpretResult(new SandboxResult(1, "", "", TimedOut: true))); + + // ---- C2: workspace/file permissions must not depend on the operator's umask ---- + + [Fact] + public void CreateWorkspaceDirectoryIsWorldReadableAndTraversable() + { + if (OperatingSystem.IsWindows()) return; // UnixFileMode is a no-op there + var parent = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); + var path = Path.Combine(parent, Guid.NewGuid().ToString("N")); + try + { + BuildGate.CreateWorkspaceDirectory(path); + var mode = File.GetUnixFileMode(path); + Assert.True(mode.HasFlag(UnixFileMode.OtherRead) && mode.HasFlag(UnixFileMode.OtherExecute)); + } + finally { Directory.Delete(parent, recursive: true); } + } + + [Fact] + public async Task WriteWorldReadableAsyncMakesTheFileReadableByOthers() + { + if (OperatingSystem.IsWindows()) return; + var dir = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))).FullName; + try + { + var path = Path.Combine(dir, "f.cs"); + await BuildGate.WriteWorldReadableAsync(path, "class C {}", CancellationToken.None); + Assert.True(File.GetUnixFileMode(path).HasFlag(UnixFileMode.OtherRead)); + } + finally { Directory.Delete(dir, recursive: true); } + } + + // ---- host fallback: I3 wants a test that actually ENTERS HostBuildAsync, not one that + // throws before the NuGet.config write even completes. Both below run a real `dotnet + // build` on the host - no Docker needed, since this is the unsandboxed path. ---- + + [Fact] + public async Task HostFallbackActuallyCompilesOnTheHostAndReportsTheCompilerError() { var workspace = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))).FullName; try { + File.WriteAllText(Path.Combine(workspace, "Broken.cs"), "this is not C#;"); + File.WriteAllText(Path.Combine(workspace, "Broken.csproj"), + """ + + + net10.0 + + + """); + + var errors = await BuildGate.RunAsync(workspace, useSandbox: false, CancellationToken.None); + + Assert.NotEmpty(errors); + Assert.Contains(errors, e => e.Contains(": error ")); + } + finally { Directory.Delete(workspace, recursive: true); } + } + + [Fact] + public async Task CancellationDuringHostBuildPropagatesInsteadOfHangingOrFalselyPassing() + { + var workspace = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))).FullName; + try + { + File.WriteAllText(Path.Combine(workspace, "Program.cs"), "System.Console.WriteLine(\"hi\");"); + File.WriteAllText(Path.Combine(workspace, "Program.csproj"), + """ + + + Exe + net10.0 + + + """); + // Cancels after the NuGet.config write but almost certainly while `dotnet build` + // is still running (process startup alone dominates 50ms) - I3: this must + // propagate as cancellation (not hang, not report a false PASSED/timeout). + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(50)); + await Assert.ThrowsAnyAsync(() => - BuildGate.RunAsync(workspace, useSandbox: false, new CancellationToken(canceled: true))); + BuildGate.RunAsync(workspace, useSandbox: false, cts.Token)); } finally { Directory.Delete(workspace, recursive: true); } } @@ -215,4 +319,36 @@ public sealed class PricingModule : IPricingModule } finally { Directory.Delete(workspace, recursive: true); } } + + // C1 regression, reproduced the same way the reviewer did: an artificially starved + // pids-limit kills `dotnet build` (fork fails) before it ever prints a line containing + // ": error " - verified by hand first ("sh: 1: Cannot fork", exit 2). InterpretResult + // must not read that silence as PASSED. + [Fact] + public async Task PidsLimitKillIsNotSilentlyReadAsPassed() + { + if (!DockerAvailable) return; + + var workspace = CreateWorkspace(); + try + { + File.WriteAllText(Path.Combine(workspace, "PricingModule.cs"), + """ + namespace Campaign; + public sealed class PricingModule : IPricingModule + { + public int[] GetTiers(ProductSpec spec) => [10, 20, 30]; + } + """); + + var options = BuildGate.SandboxedOptions(workspace) with { PidsLimit = 1 }; + var result = await SandboxRunner.RunAsync(options, + ["sh", "-c", "cp -r /src /tmp/build && cd /tmp/build && dotnet build -nologo --verbosity quiet"], + stdin: null, CancellationToken.None); + + Assert.NotEqual(0, result.ExitCode); + Assert.NotEmpty(BuildGate.InterpretResult(result)); + } + finally { Directory.Delete(workspace, recursive: true); } + } } diff --git a/PatternExplorer/patterns/StigmergicCoordination.md b/PatternExplorer/patterns/StigmergicCoordination.md index 9d86ac5..619a237 100644 --- a/PatternExplorer/patterns/StigmergicCoordination.md +++ b/PatternExplorer/patterns/StigmergicCoordination.md @@ -78,10 +78,15 @@ But compiling is not risk-free either: build tasks, source generators, and MSBui run as part of a build, not just at execution time, so `dotnet build` still runs inside the same locked-down container boundary **CodeAct** uses for model-generated code — no network, read-only source mount, a bounded writable build directory, capped CPU/memory/pids, a -wall-clock timeout, and bounded output. The sample fails closed with no fallback to a host -build unless the same double opt-in CodeAct offers is set (and even then the timeout and -source-size cap still apply). A production version would go further and add behavioral -contract tests running in that same sandbox. +wall-clock timeout, and bounded output. The image differs, though: this sample pulls the stock +`mcr.microsoft.com/dotnet/sdk` image from the network on first use, rather than CodeAct's +repo-controlled image with an offline package cache baked in — same isolation flags, different +image provenance. The sample fails closed with no fallback to a host build unless the same +double opt-in CodeAct offers is set (and even then the timeout and source-size cap still +apply). A nonzero exit with no compiler diagnostic — a permission mismatch on the mount, a +resource-limit kill, an image-pull failure — is reported as a gate error too, never as a +silent pass. A production version would go further and add behavioral contract tests running +in that same sandbox. ## Key APIs diff --git a/StigmergicCoordination.AgentFramework/BuildGate.cs b/StigmergicCoordination.AgentFramework/BuildGate.cs index b324ea3..729a270 100644 --- a/StigmergicCoordination.AgentFramework/BuildGate.cs +++ b/StigmergicCoordination.AgentFramework/BuildGate.cs @@ -14,6 +14,13 @@ namespace StigmergicCoordination.AgentFramework; public static class BuildGate { public const long MaxSourceBytes = 64 * 1024; + + // ponytail: the stock SDK image, pulled from the network on first `docker run` - unlike + // CodeAct's ContainerCodeRunner, which builds a repo-controlled image with an offline + // package cache baked in (CodeAct.AgentFramework/Sandbox/Dockerfile). Same isolation + // flags, different image provenance: a first-run pull failure is a real failure mode here + // that CodeAct doesn't have. Upgrade path: bake a similar offline image if this sample + // needs to run with zero host network access, including for the initial pull. private const string ContainerImage = "mcr.microsoft.com/dotnet/sdk:10.0"; private const int MaxOutputCharacters = 65_536; @@ -66,6 +73,25 @@ public static bool IsUnsafeHostBuildRequested() => public static List ParseErrors(string combinedOutput) => [.. combinedOutput.Split('\n').Where(l => l.Contains(": error ")).Select(l => l.Trim()).Distinct()]; + /// + /// C1: a nonzero exit with no parsed compiler diagnostic is NOT a pass. `cp` permission + /// failures, a `--pids-limit` kill, a daemon hiccup, or an image-pull failure all exit + /// nonzero without ever printing a line containing ": error " - discarding the exit code + /// (as the pre-sandbox `BuildAsync` did) turns every one of those into a silent PASSED. + /// Single source of truth for both the sandboxed and host-fallback paths. + /// + public static List InterpretResult(SandboxResult result) + { + if (result.TimedOut) return ["error AP0002: the build gate timed out"]; + var errors = ParseErrors(result.StdOut + result.StdErr); + if (errors.Count == 0 && result.ExitCode != 0) + { + var detail = string.IsNullOrWhiteSpace(result.StdErr) ? result.StdOut : result.StdErr; + return [$"error AP0003: the build gate could not run (exit {result.ExitCode}): {detail.Trim()}"]; + } + return errors; + } + /// /// `SandboxOptions` defaults to `--read-only`, so copying the workspace out of the /// read-only `/src` mount needs a writable mount - a bounded tmpfs. The tmpfs (and HOME / @@ -106,7 +132,8 @@ public static async Task> RunAsync( // Ruling 3: written every round (cheap, idempotent) so a restore that ever tries to // reach a feed fails loudly instead of silently depending on Network: false alone. - await File.WriteAllTextAsync(Path.Combine(workspace, "NuGet.config"), NuGetConfig, cancellationToken); + // World-readable: this file rides the read-only /src bind mount into the sandbox too. + await WriteWorldReadableAsync(Path.Combine(workspace, "NuGet.config"), NuGetConfig, cancellationToken); if (!useSandbox) return await HostBuildAsync(workspace, cancellationToken); @@ -114,8 +141,44 @@ public static async Task> RunAsync( ["sh", "-c", "cp -r /src /tmp/build && cd /tmp/build && dotnet build -nologo --verbosity quiet"], stdin: null, cancellationToken); - if (result.TimedOut) return ["error AP0002: the build gate timed out"]; - return ParseErrors(result.StdOut + result.StdErr); + return InterpretResult(result); + } + + /// + /// C2: the container runs as uid 65532, an unrelated uid on the host. The default + /// directory/file permissions `Directory.CreateDirectory`/`File.WriteAllText` produce + /// depend on the caller's umask - restrictive enough (0700, common with `umask 077`) and + /// the bind mount is unreadable to the sandbox, `cp` fails, and (pre-C1-fix) that silently + /// read as PASSED. Verified by hand that `Directory.CreateDirectory(path, unixCreateMode)` + /// ALONE is not enough: `mkdir()`'s mode argument is itself masked by the process umask + /// (0077 in, 0700 out, even though 0775 was requested) - the same call + /// `ContainerCodeRunner.CreateRunDirectory` makes, and the same gap. An explicit + /// `File.SetUnixFileMode` afterwards is what actually forces the bits: `chmod()`, unlike + /// `mkdir()`, is not subject to umask. + /// + public static string CreateWorkspaceDirectory(string path) + { + if (OperatingSystem.IsWindows()) return Directory.CreateDirectory(path).FullName; + + const UnixFileMode mode = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | + UnixFileMode.GroupRead | UnixFileMode.GroupExecute | + UnixFileMode.OtherRead | UnixFileMode.OtherExecute; + var parent = Path.GetDirectoryName(path)!; + Directory.CreateDirectory(parent); + File.SetUnixFileMode(parent, mode); + Directory.CreateDirectory(path); + File.SetUnixFileMode(path, mode); + return path; + } + + /// Mirrors ContainerCodeRunner.WriteWorldReadableAsync - every file written + /// into the workspace has to be readable by uid 65532, not just the directory. + public static async Task WriteWorldReadableAsync(string path, string content, CancellationToken cancellationToken) + { + await File.WriteAllTextAsync(path, content, cancellationToken); + if (!OperatingSystem.IsWindows()) + File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite | + UnixFileMode.GroupRead | UnixFileMode.OtherRead); } // ponytail: unsandboxed - only reachable behind the double opt-in in IsUnsafeHostBuildRequested, @@ -138,12 +201,17 @@ private static async Task> HostBuildAsync(string workspace, Cancell var stdoutTask = BoundedReader.ReadBoundedAsync(process.StandardOutput, MaxOutputCharacters, timeoutCts.Token); var stderrTask = BoundedReader.ReadBoundedAsync(process.StandardError, MaxOutputCharacters, timeoutCts.Token); await process.WaitForExitAsync(timeoutCts.Token); - return ParseErrors(await stdoutTask + await stderrTask); + return InterpretResult(new SandboxResult(process.ExitCode, await stdoutTask, await stderrTask, TimedOut: false)); } - catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + catch (OperationCanceledException) { + // I3: kill on EITHER a timeout or caller cancellation - an unsandboxed host + // `dotnet build` left running after this method returns/throws is an orphaned + // process, not just a discarded result (contrast SandboxRunner's kill-by-name). if (!process.HasExited) process.Kill(entireProcessTree: true); - return ["error AP0002: the build gate timed out"]; + process.WaitForExit(); // release file handles so the workspace can be deleted + if (cancellationToken.IsCancellationRequested) throw; // caller cancellation stays caller cancellation + return InterpretResult(new SandboxResult(-1, "", "", TimedOut: true)); } } } diff --git a/StigmergicCoordination.AgentFramework/Program.cs b/StigmergicCoordination.AgentFramework/Program.cs index e1cd1ad..5fab78c 100644 --- a/StigmergicCoordination.AgentFramework/Program.cs +++ b/StigmergicCoordination.AgentFramework/Program.cs @@ -1,3 +1,4 @@ +using System.Runtime.InteropServices; using System.Text.RegularExpressions; using Microsoft.Agents.AI; using Shared; @@ -24,10 +25,20 @@ return 1; } -var workspace = Directory.CreateDirectory( - Path.Combine(Path.GetTempPath(), "stigmergy", Guid.NewGuid().ToString("N"))).FullName; +// C2: world-readable so the sandbox's non-root uid can read the bind mount, regardless of +// the operator's umask - see BuildGate.CreateWorkspaceDirectory. +var workspace = BuildGate.CreateWorkspaceDirectory( + Path.Combine(Path.GetTempPath(), "stigmergy", Guid.NewGuid().ToString("N"))); Console.WriteLine($"Workspace: {workspace}\n"); +// Minor: a plain `return` inside the round loop below cannot outrun Ctrl-C, so the SIGINT +// handler deletes the workspace directly rather than relying on the try/finally to run. +using var sigint = PosixSignalRegistration.Create(PosixSignal.SIGINT, _ => +{ + try { Directory.Delete(workspace, recursive: true); } + catch (IOException) { } catch (UnauthorizedAccessException) { } +}); + // ---- The environment: contracts + integration gate, written by the HOST ---- const string Contracts = @@ -69,9 +80,9 @@ public static (ISloganModule, IPricingModule, IBriefAssembler) Wire() => } """; -File.WriteAllText(Path.Combine(workspace, "Contracts.cs"), Contracts); -File.WriteAllText(Path.Combine(workspace, "IntegrationGate.cs"), Gate); -File.WriteAllText(Path.Combine(workspace, "Campaign.csproj"), +await BuildGate.WriteWorldReadableAsync(Path.Combine(workspace, "Contracts.cs"), Contracts, CancellationToken.None); +await BuildGate.WriteWorldReadableAsync(Path.Combine(workspace, "IntegrationGate.cs"), Gate, CancellationToken.None); +await BuildGate.WriteWorldReadableAsync(Path.Combine(workspace, "Campaign.csproj"), """ @@ -79,7 +90,7 @@ public static (ISloganModule, IPricingModule, IBriefAssembler) Wire() => enable - """); + """, CancellationToken.None); // ---- The workers: one file each, briefed from the environment, never from each other ---- @@ -112,7 +123,7 @@ async Task ProduceAsync((string File, string Role, string Brief) worker, string var agent = new ChatClientAgent(Settings.ChatClient, worker.Brief, worker.Role); var code = (await agent.RunAsync($"{TaskBrief}\n{extraContext}")).Text.Trim(); code = Regex.Replace(code, @"^```\w*\n|\n?```$", ""); // strip fences if the model adds them anyway - File.WriteAllText(Path.Combine(workspace, worker.File), code); + await BuildGate.WriteWorldReadableAsync(Path.Combine(workspace, worker.File), code, CancellationToken.None); Console.WriteLine($"[worker] {worker.Role} -> {worker.File} ({code.Split('\n').Length} lines, no messages to other workers)"); } @@ -156,5 +167,6 @@ await ProduceAsync(worker, finally { // Guaranteed cleanup: the workspace must not survive a crash or the success return above. - try { Directory.Delete(workspace, recursive: true); } catch (IOException) { } + try { Directory.Delete(workspace, recursive: true); } + catch (IOException) { } catch (UnauthorizedAccessException) { } } From 4fe16e0366123ab57104b779ed391791eb0d6403 Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 15:20:50 +0200 Subject: [PATCH 08/16] fix(codeact): propagate cancellation into script execution AIFunctionFactory.Create injects the invocation's CancellationToken into ExecuteCSharp's trailing parameter, so cancelling the agent run now cancels the underlying container run instead of using CancellationToken.None. Proven with a fake IGeneratedCodeRunner recording the token it received, invoked through AIFunctionFactory.Create with an already-cancelled token. --- .../CodeActExecutionTests.cs | 57 +++++++++++++++++++ AgenticPatterns.Tests/Fakes.cs | 13 +++++ CodeAct.AgentFramework/Program.cs | 4 +- 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/AgenticPatterns.Tests/CodeActExecutionTests.cs b/AgenticPatterns.Tests/CodeActExecutionTests.cs index 5d134e7..074a8b0 100644 --- a/AgenticPatterns.Tests/CodeActExecutionTests.cs +++ b/AgenticPatterns.Tests/CodeActExecutionTests.cs @@ -1,4 +1,5 @@ using CodeAct.AgentFramework.Execution; +using Microsoft.Extensions.AI; using Shared.Sandbox; using Xunit; @@ -155,6 +156,35 @@ public void HostCannotBeAskedToRunAnythingButTheScript() Assert.Equal([Options.ContainerImage, "dotnet", "run", "/workspace/script.cs"], Args()[^4..]); } + // ---- run-directory permissions must not depend on the operator's umask ---- + + // Directory.CreateDirectory(path, mode)'s mode is a mkdir(2) mode, masked by the + // process umask like any mkdir call - a restrictive umask (e.g. 077, common in CI/hardened + // hosts) silently drops the group/other bits the container's uid 65532 needs to traverse + // the bind mount and read script.cs, and CodeAct fails loudly with "Script failed" inside + // the container. This asserts the ACTUAL mode via File.GetUnixFileMode - regardless of + // whatever umask the test process happens to run under - rather than reasoning about the + // code, matching StigmergicBuildGateTests.CreateWorkspaceDirectoryIsWorldReadableAndTraversable. + [Fact] + public void RunDirectoryIsWorldReadableAndTraversableRegardlessOfUmask() + { + if (OperatingSystem.IsWindows()) return; // UnixFileMode is a no-op there + var runId = Guid.NewGuid().ToString("N"); + var path = ContainerCodeRunner.CreateRunDirectory(runId); + try + { + var mode = File.GetUnixFileMode(path); + Assert.True(mode.HasFlag(UnixFileMode.OtherRead) && mode.HasFlag(UnixFileMode.OtherExecute)); + Assert.True(mode.HasFlag(UnixFileMode.GroupRead) && mode.HasFlag(UnixFileMode.GroupExecute)); + + // The shared parent ("codeact") must be traversable too, or uid 65532 cannot + // even reach the per-run directory beneath it. + var parentMode = File.GetUnixFileMode(Path.GetDirectoryName(path)!); + Assert.True(parentMode.HasFlag(UnixFileMode.OtherRead) && parentMode.HasFlag(UnixFileMode.OtherExecute)); + } + finally { Directory.Delete(path, recursive: true); } + } + // ---- output and cancellation lifecycle ---- [Fact] @@ -182,4 +212,31 @@ public async Task CallerCancellationIsNotConvertedIntoATimeoutResult() await Assert.ThrowsAnyAsync(() => runner.RunAsync("Console.WriteLine();", new CancellationToken(canceled: true))); } + + // ---- cancellation plumbing: the agent-invocation token must reach the runner ---- + + // Same shape as Program.cs's ExecuteCSharp local function (a trailing CancellationToken + // parameter, not exposed to the model as a JSON-schema argument): proves + // AIFunctionFactory.Create injects the AIFunction invocation's token into that parameter + // for exactly this delegate shape, and that it is the SAME token RunAsync receives. + [Fact] + public async Task ExecuteCSharpToolForwardsTheInvocationTokenToTheRunner() + { + var runner = new RecordingCodeRunner(); + + async Task ExecuteCSharp(string code, CancellationToken cancellationToken) + { + var execution = await runner.RunAsync(code, cancellationToken); + return execution.StandardOutput; + } + + var tool = AIFunctionFactory.Create(ExecuteCSharp, "execute_csharp", "test tool"); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await tool.InvokeAsync(new AIFunctionArguments { ["code"] = "Console.WriteLine();" }, cts.Token); + + Assert.Equal(cts.Token, runner.ReceivedToken); + Assert.True(runner.ReceivedToken.IsCancellationRequested); + } } diff --git a/AgenticPatterns.Tests/Fakes.cs b/AgenticPatterns.Tests/Fakes.cs index 8cea0c4..e63b9ce 100644 --- a/AgenticPatterns.Tests/Fakes.cs +++ b/AgenticPatterns.Tests/Fakes.cs @@ -1,7 +1,20 @@ +using CodeAct.AgentFramework.Execution; using Microsoft.Extensions.AI; namespace AgenticPatterns.Tests; +/// Records the token it was invoked with, instead of actually running anything. +internal sealed class RecordingCodeRunner : IGeneratedCodeRunner +{ + public CancellationToken ReceivedToken { get; private set; } + + public Task RunAsync(string sourceCode, CancellationToken cancellationToken) + { + ReceivedToken = cancellationToken; + return Task.FromResult(new ExecutionResult(0, "", "", TimedOut: false)); + } +} + /// Returns pre-canned responses in order and counts calls. internal sealed class ScriptedChatClient(params ChatResponse[] responses) : IChatClient { diff --git a/CodeAct.AgentFramework/Program.cs b/CodeAct.AgentFramework/Program.cs index 15df82c..de63105 100644 --- a/CodeAct.AgentFramework/Program.cs +++ b/CodeAct.AgentFramework/Program.cs @@ -67,7 +67,7 @@ findings. Then answer the user from the script output. return; -async Task ExecuteCSharp(string code) +async Task ExecuteCSharp(string code, CancellationToken cancellationToken) { // Models sometimes wrap code in markdown fences despite instructions. code = code.Trim(); @@ -84,7 +84,7 @@ async Task ExecuteCSharp(string code) // The action API is appended below the model's code: local functions may follow the // top-level statements, and are callable from them regardless of declaration order. - var execution = await runner.RunAsync(code + "\n\n" + ActionApiSource, CancellationToken.None); + var execution = await runner.RunAsync(code + "\n\n" + ActionApiSource, cancellationToken); var result = execution switch { From e47aa5027ffa601d2558ca3a76d2765bd356a57b Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 15:20:50 +0200 Subject: [PATCH 09/16] fix(codeact): force run-directory permissions instead of relying on umask Directory.CreateDirectory(path, mode)'s mode is a mkdir(2) mode, masked by the process umask like any mkdir call (verified: umask 077 turns a requested 0755 into 0700). Under a restrictive umask the container's uid 65532 couldn't traverse the per-run directory or read the bind-mounted script, so CodeAct failed loudly with "Script failed (exit N)". Mirrors StigmergicCoordination.AgentFramework/BuildGate.cs CreateWorkspaceDirectory, which hit and fixed the identical gap: create the directory, then force the bits with File.SetUnixFileMode (chmod is not subject to umask). CreateRunDirectory is now internal so a test can call it directly and assert the actual on-disk mode, independent of whatever umask the test process runs under. --- .../Execution/ContainerCodeRunner.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs b/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs index 6d62c32..bb8eb2f 100644 --- a/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs +++ b/CodeAct.AgentFramework/Execution/ContainerCodeRunner.cs @@ -100,18 +100,28 @@ private static async Task WriteWorldReadableAsync(string path, string content, C UnixFileMode.GroupRead | UnixFileMode.OtherRead); } - private static string CreateRunDirectory(string runId) + // World-readable/traversable so container uid 65532 can read the bind mount. + // Directory.CreateDirectory's mode parameter is a mkdir(2) mode, itself masked by the + // process umask (verified: umask 077 turns a requested 0755 into 0700) - passing `mode` + // to CreateDirectory alone is not enough. An explicit File.SetUnixFileMode afterwards is + // what actually forces the bits, since chmod(2) is not subject to umask. Mirrors + // StigmergicCoordination.AgentFramework/BuildGate.cs CreateWorkspaceDirectory, which hit + // and fixed the identical gap. + internal static string CreateRunDirectory(string runId) { var path = Path.Combine(Path.GetTempPath(), "codeact", runId); if (OperatingSystem.IsWindows()) return Directory.CreateDirectory(path).FullName; - // World-readable/traversable so container uid 65532 can read the bind mount. const UnixFileMode mode = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | UnixFileMode.GroupRead | UnixFileMode.GroupExecute | UnixFileMode.OtherRead | UnixFileMode.OtherExecute; - Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), "codeact"), mode); - return Directory.CreateDirectory(path, mode).FullName; + var parent = Path.Combine(Path.GetTempPath(), "codeact"); + Directory.CreateDirectory(parent); + File.SetUnixFileMode(parent, mode); + Directory.CreateDirectory(path); + File.SetUnixFileMode(path, mode); + return path; } /// Builds the repo-controlled sandbox image from Sandbox/Dockerfile on first use. From cfd8073c70069dee72bc318ddfbeaac24c40759f Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 15:38:00 +0200 Subject: [PATCH 10/16] fix(explorer): per-run isolation, bounded output and runtime RunSession.Current was process-global, so a second browser tab silently cancelled the first tab's run. Replace it with a registry keyed by (id, token), bound the output channel and add wall-clock/output ceilings, and stop child processes from inheriting Explorer's whole environment - only PATH/HOME/DOTNET_* plus a per-project allowlist (Azure OpenAI config by default, CodeAct's host-execution opt-ins only for CodeAct) survive. Endpoints move under /api/runs/{id}/{input,cancel}, auth'd by an X-Run-Token header; /api/run now hands back {id, token} as the first SSE event. This intentionally breaks app.js's calls to the old /api/run/input and /api/run/cancel routes - task 2.5b repairs the frontend. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- AgenticPatterns.Tests/RunSessionTests.cs | 81 ++++++++++++++++++ PatternExplorer/Catalog.cs | 14 ++- PatternExplorer/PatternExplorer.csproj | 4 + PatternExplorer/Program.cs | 33 +++++-- PatternExplorer/RunSession.cs | 104 ++++++++++++++++++++--- 5 files changed, 220 insertions(+), 16 deletions(-) create mode 100644 AgenticPatterns.Tests/RunSessionTests.cs diff --git a/AgenticPatterns.Tests/RunSessionTests.cs b/AgenticPatterns.Tests/RunSessionTests.cs new file mode 100644 index 0000000..8430975 --- /dev/null +++ b/AgenticPatterns.Tests/RunSessionTests.cs @@ -0,0 +1,81 @@ +using PatternExplorer; +using Xunit; + +namespace AgenticPatterns.Tests; + +/// Exercises RunSession's registry, lookup and channel behavior directly - never through +/// Start/RunAsync, which would spawn a real `dotnet run` child process. +public class RunSessionTests +{ + [Fact] + public void Two_sessions_are_independently_retrievable_and_isolated() + { + var a = new RunSession(); + var b = new RunSession(); + RunSession.Register(a); + RunSession.Register(b); + try + { + Assert.Same(a, RunSession.TryGet(a.Id, a.Token)); + Assert.Same(b, RunSession.TryGet(b.Id, b.Token)); + + a.Cancel(); + + Assert.True(a.CancellationToken.IsCancellationRequested); + Assert.False(b.CancellationToken.IsCancellationRequested); + } + finally + { + RunSession.Unregister(a); + RunSession.Unregister(b); + } + } + + [Fact] + public void TryGet_returns_null_for_wrong_token_or_wrong_id() + { + var session = new RunSession(); + RunSession.Register(session); + try + { + Assert.Null(RunSession.TryGet(session.Id, "wrong-token")); + Assert.Null(RunSession.TryGet("wrong-id", session.Token)); + } + finally + { + RunSession.Unregister(session); + } + } + + [Fact] + public void Register_throws_once_the_live_run_cap_is_reached() + { + var sessions = Enumerable.Range(0, 8).Select(_ => new RunSession()).ToList(); + foreach (var s in sessions) RunSession.Register(s); + try + { + var extra = new RunSession(); + Assert.Throws(() => RunSession.Register(extra)); + } + finally + { + foreach (var s in sessions) RunSession.Unregister(s); + } + } + + [Fact] + public void Bounded_channel_drops_oldest_instead_of_growing_or_blocking() + { + var session = new RunSession(); + + for (var i = 0; i < 5000; i++) + Assert.True(session.Writer.TryWrite(new Chunk("out", i.ToString()))); + + var received = new List(); + while (session.Reader.TryRead(out var chunk)) received.Add(chunk); + + Assert.True(received.Count <= 4096); + Assert.DoesNotContain(received, c => c.T == "0"); + Assert.Contains(received, c => c.T == "4999"); + } +} diff --git a/PatternExplorer/Catalog.cs b/PatternExplorer/Catalog.cs index b5f0539..1c9864d 100644 --- a/PatternExplorer/Catalog.cs +++ b/PatternExplorer/Catalog.cs @@ -14,7 +14,19 @@ public record PatternProject( bool Interactive = false, string? Server = null, int ServerPort = 0, - string? Note = null); + string? Note = null) +{ + /// Environment variable names copied from Explorer's own environment into the child process, + /// in addition to PATH/HOME/DOTNET_* which `dotnet run` always needs. Defaults to the four + /// Azure OpenAI settings (see Shared/AzureOpenAISettings.cs) most samples need to run at all. + public IReadOnlyList EnvironmentAllowlist { get; init; } = + [ + "AzureOpenAi__ChatModelDeployment", + "AzureOpenAi__EmbeddingModelDeployment", + "AzureOpenAi__Endpoint", + "AzureOpenAi__ApiKey" + ]; +} /// Optional warning shown above the Run button (e.g. "Runs model-written code on this machine."). public record PatternMeta(string Title, string Summary, string Category, PatternProject[] Projects, string? Risk = null); diff --git a/PatternExplorer/PatternExplorer.csproj b/PatternExplorer/PatternExplorer.csproj index 5563a73..e904fe6 100644 --- a/PatternExplorer/PatternExplorer.csproj +++ b/PatternExplorer/PatternExplorer.csproj @@ -4,4 +4,8 @@ PatternExplorer + + + + diff --git a/PatternExplorer/Program.cs b/PatternExplorer/Program.cs index f15d9b3..85686b3 100644 --- a/PatternExplorer/Program.cs +++ b/PatternExplorer/Program.cs @@ -59,13 +59,30 @@ return; } + RunSession session; + try + { + session = RunSession.Start(repoRoot, project); + } + catch (InvalidOperationException ex) + { + context.Response.StatusCode = 429; + await context.Response.WriteAsync(ex.Message); + return; + } + context.Response.Headers.ContentType = "text/event-stream"; context.Response.Headers.CacheControl = "no-cache"; context.Response.Headers["X-Accel-Buffering"] = "no"; - var session = RunSession.Start(repoRoot, project); try { + // The id/token pair the client needs to reach /api/runs/{id}/input and /cancel - sent as + // the first event so a single GET both starts the run and hands out its credentials. + await context.Response.WriteAsync( + $"event: session\ndata: {JsonSerializer.Serialize(new { session.Id, session.Token }, JsonSerializerOptions.Web)}\n\n"); + await context.Response.Body.FlushAsync(); + await foreach (var chunk in session.Reader.ReadAllAsync(context.RequestAborted)) { await context.Response.WriteAsync($"data: {JsonSerializer.Serialize(chunk, JsonSerializerOptions.Web)}\n\n"); @@ -82,16 +99,22 @@ } }); -app.MapPost("/api/run/input", async (HttpContext context) => +app.MapPost("/api/runs/{id}/input", async (HttpContext context, string id) => { + var session = RunSession.TryGet(id, context.Request.Headers["X-Run-Token"].ToString()); + if (session is null) return Results.NotFound(); + using var reader = new StreamReader(context.Request.Body); - RunSession.Current?.SendInput((await reader.ReadToEndAsync()).TrimEnd('\n')); + session.SendInput((await reader.ReadToEndAsync()).TrimEnd('\n')); return Results.Ok(); }); -app.MapPost("/api/run/cancel", () => +app.MapPost("/api/runs/{id}/cancel", (string id, HttpContext context) => { - RunSession.Current?.Cancel(); + var session = RunSession.TryGet(id, context.Request.Headers["X-Run-Token"].ToString()); + if (session is null) return Results.NotFound(); + + session.Cancel(); return Results.Ok(); }); diff --git a/PatternExplorer/RunSession.cs b/PatternExplorer/RunSession.cs index 7901c40..8ab7629 100644 --- a/PatternExplorer/RunSession.cs +++ b/PatternExplorer/RunSession.cs @@ -1,5 +1,8 @@ +using System.Collections.Concurrent; using System.Diagnostics; using System.Net.Sockets; +using System.Security.Cryptography; +using System.Text; using System.Threading.Channels; namespace PatternExplorer; @@ -8,42 +11,88 @@ namespace PatternExplorer; /// Raw text chunk (not line-buffered, so `Console.Write` prompts show up). public record Chunk(string S, string T); -/// Runs one sample at a time as a child `dotnet run` and streams its console output. +/// Runs one sample as a child `dotnet run` and streams its console output. One instance per run, +/// looked up by (id, token) so concurrent tabs don't fight over a single global. public sealed class RunSession { - // ponytail: one run at a time, so a static current session is all the bookkeeping needed. - public static RunSession? Current { get; private set; } - - readonly Channel _channel = Channel.CreateUnbounded(); + // ponytail: a dictionary keyed by run id, capped at MaxLiveRuns. A single-user local tool does + // not need a scheduler; it does need two tabs not to fight. Upgrade to a real queue/scheduler + // if Explorer ever grows multi-user or needs to run more than a handful of samples at once. + const int MaxLiveRuns = 8; + static readonly ConcurrentDictionary Runs = new(StringComparer.Ordinal); + + static readonly TimeSpan MaxRuntime = TimeSpan.FromMinutes(15); + const long MaxOutputBytes = 4 * 1024 * 1024; + + // CodeAct's opt-in host-execution variables (see CodeAct.AgentFramework/Execution/CodeRunnerFactory.cs) + // are forwarded only when that's the project being started - no other sample reads them. + const string CodeActProjectPath = "CodeAct.AgentFramework"; + static readonly string[] CodeActUnsafeExecutionVariables = + [ + "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION", + "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION" + ]; + + public string Id { get; } = Guid.NewGuid().ToString("N"); + public string Token { get; } = Convert.ToHexString(RandomNumberGenerator.GetBytes(16)); + + readonly Channel _channel = Channel.CreateBounded( + new BoundedChannelOptions(4096) { FullMode = BoundedChannelFullMode.DropOldest }); readonly CancellationTokenSource _cts = new(); readonly List _processes = []; StreamWriter? _input; + long _outputBytes; + int _outputLimitHit; public ChannelReader Reader => _channel.Reader; + // Test seam: lets tests write chunks and observe drop-oldest behavior without spawning a process. + internal ChannelWriter Writer => _channel.Writer; + + // Test seam: cancellation is otherwise unobservable from outside the session. + internal CancellationToken CancellationToken => _cts.Token; + + public static RunSession? TryGet(string id, string token) => + Runs.TryGetValue(id, out var session) && + CryptographicOperations.FixedTimeEquals( + Encoding.UTF8.GetBytes(session.Token), Encoding.UTF8.GetBytes(token)) + ? session : null; + public static RunSession Start(string repoRoot, PatternProject project) { - Current?.Cancel(); var session = new RunSession(); - Current = session; + Register(session); _ = Task.Run(() => session.RunAsync(repoRoot, project)); return session; } + // Registration and removal are split out from Start/RunAsync so tests can exercise session + // lifetime (lookup, isolation, the live-run cap) without ever spawning `dotnet run`. + internal static void Register(RunSession session) + { + if (Runs.Count >= MaxLiveRuns) + throw new InvalidOperationException($"Too many live runs (max {MaxLiveRuns}). Cancel one and retry."); + Runs[session.Id] = session; + } + + internal static void Unregister(RunSession session) => Runs.TryRemove(session.Id, out _); + async Task RunAsync(string repoRoot, PatternProject project) { try { + _cts.CancelAfter(MaxRuntime); + if (project.Server is not null) { Emit("sys", $"dotnet run --project {project.Server}"); - StartProcess(repoRoot, project.Server, "out"); + StartProcess(repoRoot, project, project.Server, "out"); await WaitForPortAsync(project.ServerPort, _cts.Token); Emit("sys", $"server is listening on port {project.ServerPort}"); } Emit("sys", $"dotnet run --project {project.Path}"); - var main = StartProcess(repoRoot, project.Path, "out"); + var main = StartProcess(repoRoot, project, project.Path, "out"); _input = main.StandardInput; await main.WaitForExitAsync(_cts.Token); @@ -62,10 +111,11 @@ async Task RunAsync(string repoRoot, PatternProject project) { KillAll(); _channel.Writer.TryComplete(); + Unregister(this); } } - Process StartProcess(string repoRoot, string projectPath, string tag) + Process StartProcess(string repoRoot, PatternProject project, string projectPath, string tag) { var directory = Path.Combine(repoRoot, projectPath); var info = new ProcessStartInfo("dotnet") @@ -78,6 +128,17 @@ Process StartProcess(string repoRoot, string projectPath, string tag) UseShellExecute = false }; + // The sample gets only what it needs to run, not Explorer's whole environment (which may + // hold credentials for other tools). `dotnet run` itself needs PATH/HOME/DOTNET_*. + info.Environment.Clear(); + foreach (var name in HostEnvironmentNamesForDotnetRun()) + CopyIfSet(info.Environment, name); + foreach (var name in project.EnvironmentAllowlist) + CopyIfSet(info.Environment, name); + if (projectPath == CodeActProjectPath) + foreach (var name in CodeActUnsafeExecutionVariables) + CopyIfSet(info.Environment, name); + var process = Process.Start(info) ?? throw new InvalidOperationException("dotnet run did not start."); lock (_processes) _processes.Add(process); @@ -86,6 +147,16 @@ Process StartProcess(string repoRoot, string projectPath, string tag) return process; } + static IEnumerable HostEnvironmentNamesForDotnetRun() => + Environment.GetEnvironmentVariables().Keys.Cast() + .Where(name => name is "PATH" or "HOME" || name.StartsWith("DOTNET_", StringComparison.Ordinal)); + + static void CopyIfSet(IDictionary environment, string name) + { + var value = Environment.GetEnvironmentVariable(name); + if (value is not null) environment[name] = value; + } + // Character-level, not line-level: `Console.Write("Approve? (y/n): ")` must reach the browser // before the sample blocks on Console.ReadLine. async Task PumpAsync(StreamReader reader, string tag) @@ -93,8 +164,21 @@ async Task PumpAsync(StreamReader reader, string tag) var buffer = new char[1024]; while (true) { + if (Volatile.Read(ref _outputLimitHit) != 0) return; + var count = await reader.ReadAsync(buffer); if (count == 0) return; + + if (Interlocked.Add(ref _outputBytes, Encoding.UTF8.GetByteCount(buffer, 0, count)) > MaxOutputBytes) + { + if (Interlocked.Exchange(ref _outputLimitHit, 1) == 0) + { + Emit("sys", "output limit reached, run cancelled"); + _cts.Cancel(); + } + return; + } + Emit(tag, new string(buffer, 0, count)); } } From 7585f64de4c5c5190855706853690bbaf2076959 Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 15:57:05 +0200 Subject: [PATCH 11/16] fix(explorer): polish per-run isolation per review round 1 - Delete the CodeAct-only hardcoding of the two AGENTIC_PATTERNS_* opt-in variables in RunSession.StartProcess. StigmergicCoordination.AgentFramework (task 2.3) reads the same two variables for its own host-build fallback, so hardcoding one project silently changed its opt-out-of-fail-closed behavior when launched from Explorer. Both patterns now declare an explicit environmentAllowlist in their frontmatter instead. - Make the MaxLiveRuns cap actually atomic (Register now locks around the count-check-then-insert). - Stop projecting EnvironmentAllowlist into the /api/patterns wire shape. - Add the TryGet(a.Id, b.Token) cross-run isolation assertion. - Rename the internal CancellationToken test seam to IsCancelled (bool) to remove the Color-Color trap against the BCL type. - Name the Windows-forwarding ceiling (PATH/HOME/DOTNET_* only) as a ponytail: comment instead of building support this repo doesn't claim. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- AgenticPatterns.Tests/RunSessionTests.cs | 7 +++- PatternExplorer/Program.cs | 8 +++- PatternExplorer/RunSession.cs | 37 +++++++++---------- PatternExplorer/patterns/CodeAct.md | 8 +++- .../patterns/StigmergicCoordination.md | 8 +++- 5 files changed, 42 insertions(+), 26 deletions(-) diff --git a/AgenticPatterns.Tests/RunSessionTests.cs b/AgenticPatterns.Tests/RunSessionTests.cs index 8430975..72ce685 100644 --- a/AgenticPatterns.Tests/RunSessionTests.cs +++ b/AgenticPatterns.Tests/RunSessionTests.cs @@ -19,10 +19,13 @@ public void Two_sessions_are_independently_retrievable_and_isolated() Assert.Same(a, RunSession.TryGet(a.Id, a.Token)); Assert.Same(b, RunSession.TryGet(b.Id, b.Token)); + // A valid token, just from the wrong live run, must not unlock this one. + Assert.Null(RunSession.TryGet(a.Id, b.Token)); + a.Cancel(); - Assert.True(a.CancellationToken.IsCancellationRequested); - Assert.False(b.CancellationToken.IsCancellationRequested); + Assert.True(a.IsCancelled); + Assert.False(b.IsCancelled); } finally { diff --git a/PatternExplorer/Program.cs b/PatternExplorer/Program.cs index 85686b3..d4a4b51 100644 --- a/PatternExplorer/Program.cs +++ b/PatternExplorer/Program.cs @@ -15,8 +15,12 @@ OnPrepareResponse = context => context.Context.Response.Headers.CacheControl = "no-store" }); +// EnvironmentAllowlist is server-side plumbing (what the child process is allowed to inherit) - +// project it out of the wire shape rather than exposing "what does the server forward" to the page. +object ProjectForWire(PatternProject p) => new { p.Flavor, p.Path, p.Interactive, p.Server, p.ServerPort, p.Note }; + app.MapGet("/api/patterns", () => Catalog.Load(patternsDir) - .Select(p => new { p.Id, p.Meta.Title, p.Meta.Summary, p.Meta.Category, p.Meta.Projects, p.Meta.Risk })); + .Select(p => new { p.Id, p.Meta.Title, p.Meta.Summary, p.Meta.Category, Projects = p.Meta.Projects.Select(ProjectForWire), p.Meta.Risk })); app.MapGet("/api/patterns/{id}", (string id) => { @@ -29,7 +33,7 @@ pattern.Meta.Title, pattern.Meta.Summary, pattern.Meta.Category, - pattern.Meta.Projects, + Projects = pattern.Meta.Projects.Select(ProjectForWire), pattern.Meta.Risk, pattern.Body, Sources = pattern.Meta.Projects.ToDictionary( diff --git a/PatternExplorer/RunSession.cs b/PatternExplorer/RunSession.cs index 8ab7629..390c9b6 100644 --- a/PatternExplorer/RunSession.cs +++ b/PatternExplorer/RunSession.cs @@ -15,24 +15,17 @@ public record Chunk(string S, string T); /// looked up by (id, token) so concurrent tabs don't fight over a single global. public sealed class RunSession { - // ponytail: a dictionary keyed by run id, capped at MaxLiveRuns. A single-user local tool does - // not need a scheduler; it does need two tabs not to fight. Upgrade to a real queue/scheduler - // if Explorer ever grows multi-user or needs to run more than a handful of samples at once. + // ponytail: a dictionary keyed by run id, capped at MaxLiveRuns and guarded by a single lock + // so the count-then-add is actually atomic. A single-user local tool does not need a + // scheduler; it does need two tabs not to fight. Upgrade to a real queue/scheduler if + // Explorer ever grows multi-user or needs to run more than a handful of samples at once. const int MaxLiveRuns = 8; static readonly ConcurrentDictionary Runs = new(StringComparer.Ordinal); + static readonly object RunsLock = new(); static readonly TimeSpan MaxRuntime = TimeSpan.FromMinutes(15); const long MaxOutputBytes = 4 * 1024 * 1024; - // CodeAct's opt-in host-execution variables (see CodeAct.AgentFramework/Execution/CodeRunnerFactory.cs) - // are forwarded only when that's the project being started - no other sample reads them. - const string CodeActProjectPath = "CodeAct.AgentFramework"; - static readonly string[] CodeActUnsafeExecutionVariables = - [ - "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION", - "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION" - ]; - public string Id { get; } = Guid.NewGuid().ToString("N"); public string Token { get; } = Convert.ToHexString(RandomNumberGenerator.GetBytes(16)); @@ -49,8 +42,9 @@ public sealed class RunSession // Test seam: lets tests write chunks and observe drop-oldest behavior without spawning a process. internal ChannelWriter Writer => _channel.Writer; - // Test seam: cancellation is otherwise unobservable from outside the session. - internal CancellationToken CancellationToken => _cts.Token; + // Test seam: cancellation is otherwise unobservable from outside the session. Named IsCancelled + // (not CancellationToken) so `CancellationToken.None` below keeps meaning the BCL type. + internal bool IsCancelled => _cts.IsCancellationRequested; public static RunSession? TryGet(string id, string token) => Runs.TryGetValue(id, out var session) && @@ -70,9 +64,12 @@ public static RunSession Start(string repoRoot, PatternProject project) // lifetime (lookup, isolation, the live-run cap) without ever spawning `dotnet run`. internal static void Register(RunSession session) { - if (Runs.Count >= MaxLiveRuns) - throw new InvalidOperationException($"Too many live runs (max {MaxLiveRuns}). Cancel one and retry."); - Runs[session.Id] = session; + lock (RunsLock) + { + if (Runs.Count >= MaxLiveRuns) + throw new InvalidOperationException($"Too many live runs (max {MaxLiveRuns}). Cancel one and retry."); + Runs[session.Id] = session; + } } internal static void Unregister(RunSession session) => Runs.TryRemove(session.Id, out _); @@ -135,9 +132,6 @@ Process StartProcess(string repoRoot, PatternProject project, string projectPath CopyIfSet(info.Environment, name); foreach (var name in project.EnvironmentAllowlist) CopyIfSet(info.Environment, name); - if (projectPath == CodeActProjectPath) - foreach (var name in CodeActUnsafeExecutionVariables) - CopyIfSet(info.Environment, name); var process = Process.Start(info) ?? throw new InvalidOperationException("dotnet run did not start."); lock (_processes) _processes.Add(process); @@ -147,6 +141,9 @@ Process StartProcess(string repoRoot, PatternProject project, string projectPath return process; } + // ponytail: PATH/HOME/DOTNET_* is what `dotnet run` needs on Linux/macOS, which is all this + // repo targets (see README). Windows would also need USERPROFILE/APPDATA/SystemRoot/TEMP - + // add them here if Explorer ever needs to run there. static IEnumerable HostEnvironmentNamesForDotnetRun() => Environment.GetEnvironmentVariables().Keys.Cast() .Where(name => name is "PATH" or "HOME" || name.StartsWith("DOTNET_", StringComparison.Ordinal)); diff --git a/PatternExplorer/patterns/CodeAct.md b/PatternExplorer/patterns/CodeAct.md index d57170a..fef1d08 100644 --- a/PatternExplorer/patterns/CodeAct.md +++ b/PatternExplorer/patterns/CodeAct.md @@ -4,7 +4,13 @@ "summary": "One code-execution tool instead of many bound tools — intermediate results stay inside the script.", "category": "Orchestration", "risk": "Executes model-written C# in a locked-down local container (no network, read-only, non-root, resource limits); fails closed without Docker/Podman. Host execution requires an explicit double opt-in.", - "projects": [ { "flavor": "AgentFramework", "path": "CodeAct.AgentFramework" } ] + "projects": [ + { "flavor": "AgentFramework", "path": "CodeAct.AgentFramework", "environmentAllowlist": [ + "AzureOpenAi__ChatModelDeployment", "AzureOpenAi__EmbeddingModelDeployment", + "AzureOpenAi__Endpoint", "AzureOpenAi__ApiKey", + "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION", "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION" + ] } + ] } --- diff --git a/PatternExplorer/patterns/StigmergicCoordination.md b/PatternExplorer/patterns/StigmergicCoordination.md index 619a237..e038a2a 100644 --- a/PatternExplorer/patterns/StigmergicCoordination.md +++ b/PatternExplorer/patterns/StigmergicCoordination.md @@ -3,7 +3,13 @@ "title": "Stigmergic Coordination", "summary": "Workers coordinate through a shared workspace and compiler-enforced contracts instead of exchanging messages.", "category": "Orchestration", - "projects": [ { "flavor": "AgentFramework", "path": "StigmergicCoordination.AgentFramework" } ] + "projects": [ + { "flavor": "AgentFramework", "path": "StigmergicCoordination.AgentFramework", "environmentAllowlist": [ + "AzureOpenAi__ChatModelDeployment", "AzureOpenAi__EmbeddingModelDeployment", + "AzureOpenAi__Endpoint", "AzureOpenAi__ApiKey", + "AGENTIC_PATTERNS_ALLOW_UNSAFE_HOST_EXECUTION", "AGENTIC_PATTERNS_ACKNOWLEDGE_UNSAFE_CODE_EXECUTION" + ] } + ] } --- From 41faf652543d40424409752678f50d0b06ed1acc Mon Sep 17 00:00:00 2001 From: arst Date: Tue, 25 Aug 2026 16:19:03 +0200 Subject: [PATCH 12/16] fix(explorer): repair frontend for per-run tokens, sanitised rendering, CSP Repairs the app.js calls task 2.5a intentionally left broken (/api/run/cancel and /api/run/input, both deleted): cancel and stdin now read the run id/token from the SSE `session` event and call /api/runs/{id}/cancel and /api/runs/{id}/input with an X-Run-Token header. Renders pattern-doc Markdown with raw HTML escaped (marked's renderer.html override - v15 dropped the old `sanitize` option), runs Mermaid in `strict` mode instead of `loose`, and caps terminal history at 5000 lines dropping from the front. Adds a same-origin Content-Security-Policy in Program.cs; verified live via Playwright that the vendored marked/mermaid bundles still work under it with zero console violations and a diagram actually renders to SVG. Nothing needed loosening from the brief's starting policy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc --- AgenticPatterns.Tests/SecurityHeadersTests.cs | 30 ++++++++++++++++ PatternExplorer/Program.cs | 11 ++++++ PatternExplorer/SecurityHeaders.cs | 10 ++++++ PatternExplorer/wwwroot/app.js | 35 ++++++++++++++++--- README.md | 5 ++- 5 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 AgenticPatterns.Tests/SecurityHeadersTests.cs create mode 100644 PatternExplorer/SecurityHeaders.cs diff --git a/AgenticPatterns.Tests/SecurityHeadersTests.cs b/AgenticPatterns.Tests/SecurityHeadersTests.cs new file mode 100644 index 0000000..e924471 --- /dev/null +++ b/AgenticPatterns.Tests/SecurityHeadersTests.cs @@ -0,0 +1,30 @@ +using PatternExplorer; +using Xunit; + +namespace AgenticPatterns.Tests; + +/// +/// A WebApplicationFactory-based end-to-end header assertion isn't available in this environment +/// (Program.cs's top-level-statement type isn't exposed for one, and the project brings no +/// Mvc.Testing/TestHost package), so this asserts the constant the middleware in Program.cs +/// actually writes into the response, rather than an HTTP round trip. Manual verification of the +/// live header (curl, and a Playwright-driven page load with zero CSP console violations) is in +/// the task report. +/// +public class SecurityHeadersTests +{ + [Fact] + public void ContentSecurityPolicy_restricts_to_self_and_permits_inline_style() + { + Assert.Contains("default-src 'self'", SecurityHeaders.ContentSecurityPolicy); + Assert.Contains("script-src 'self'", SecurityHeaders.ContentSecurityPolicy); + Assert.Contains("style-src 'self' 'unsafe-inline'", SecurityHeaders.ContentSecurityPolicy); + Assert.Contains("img-src 'self' data:", SecurityHeaders.ContentSecurityPolicy); + Assert.Contains("connect-src 'self'", SecurityHeaders.ContentSecurityPolicy); + + // No loosening beyond the brief's starting policy was needed - verified live in the + // report - so nothing here should grant 'unsafe-eval' or a wildcard source. + Assert.DoesNotContain("unsafe-eval", SecurityHeaders.ContentSecurityPolicy); + Assert.DoesNotContain("*", SecurityHeaders.ContentSecurityPolicy); + } +} diff --git a/PatternExplorer/Program.cs b/PatternExplorer/Program.cs index d4a4b51..f7ef18d 100644 --- a/PatternExplorer/Program.cs +++ b/PatternExplorer/Program.cs @@ -8,6 +8,17 @@ builder.WebHost.UseUrls(Environment.GetEnvironmentVariable("ASPNETCORE_URLS") ?? "http://localhost:5080"); var app = builder.Build(); + +// Everything the page needs (marked/mermaid vendored under wwwroot, the doc's own