Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Capacitor.Cli.Daemon/DaemonConfig.cs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,17 @@ public UserHome Home {
/// Never set from config directly.</summary>
public bool CodexAppServerActive { get; set; }

/// <summary>Per-daemon opt-in: also host INTERACTIVE Codex agents over app-server, not just
/// unattended reviewers. Requires <see cref="CodexAppServerActive"/> — this widens which launches
/// take that transport, it does not select it. Off unless an operator sets
/// <c>KCAP_CODEX_APPSERVER_INTERACTIVE</c> on this daemon, so turning it on moves one host.
///
/// <para>Scope: INTERACTIVE launches the SERVER dispatches (the web launch dialog). PR review and
/// review flows are untouched by this switch — review flows already take app-server wherever it is
/// selected, and PR review stays on PTY. `kcap agent start` spawns an attachable terminal through the
/// local control socket, which never reaches the runtime factory, so it stays PTY regardless.</para></summary>
public bool CodexAppServerInteractive { get; set; }

/// <summary>Seconds an interactive hosted Codex approval (<c>*/requestApproval</c>) waits for the
/// user before failing closed (deny). Overridable via <c>KCAP_CODEX_APPSERVER_APPROVAL_TIMEOUT_SECONDS</c>.
/// Consumed as <c>TimeSpan.FromSeconds(Math.Max(1, …))</c>.</summary>
Expand Down
5 changes: 2 additions & 3 deletions src/Capacitor.Cli.Daemon/DaemonRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,12 @@ public static async Task<int> RunAsync(string[] args) {
if (Environment.GetEnvironmentVariable("KCAP_CODEX_PATH") is { Length: > 0 } envCodexPath)
config.CodexPath = envCodexPath;

if (Environment.GetEnvironmentVariable("KCAP_CODEX_TRANSPORT") is { Length: > 0 } envCodexTransport)
config.CodexTransport = envCodexTransport;

if (Environment.GetEnvironmentVariable("KCAP_CODEX_APPSERVER_APPROVAL_TIMEOUT_SECONDS") is { Length: > 0 } envApprovalTimeout
&& int.TryParse(envApprovalTimeout, out var approvalTimeoutSeconds) && approvalTimeoutSeconds > 0)
config.CodexAppServerApprovalTimeoutSeconds = approvalTimeoutSeconds;

Harness.Codex.CodexTransportDecision.BindFromEnvironment(config, Environment.GetEnvironmentVariable);

// One resolved fact for both the launch router and the certification advertisement.
config.CodexAppServerActive = Harness.Codex.CodexTransportDecision.ResolveActive(
config.CodexTransport, () => ProbeCliVersion(config.CodexPath));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,16 @@ public CodexHostedAgentRuntimeFactory(
public IReviewerModelResolver? ReviewerModelResolver => _pty.ReviewerModelResolver;
public bool SupportsModelSelection => _pty.SupportsModelSelection;

/// <summary>App-server is used for a launch only when the daemon resolved it active AND the
/// launch is unattended (review-flow). An interactive launch always takes the PTY path — even
/// under an app-server selection — because interactive hosting is a later phase.</summary>
internal bool UsesAppServer(RuntimeStartContext ctx) => _config.CodexAppServerActive && ctx.IsReviewFlow;
/// <summary>App-server hosts unattended reviewers (review-flow) wherever the daemon resolved it
/// active. INTERACTIVE launches join only where the operator opted that daemon in, so one daemon can
/// run interactive on app-server while the rest of a fleet stays on PTY.
///
/// <para>Interactive is stated positively — neither a review flow nor a PR review — rather than as
/// "not a review flow": the launch classes here are review-flow, PR review and interactive, so the
/// negative form would sweep PR review along with it and widen the opt-in past what it names.</para></summary>
internal bool UsesAppServer(RuntimeStartContext ctx) =>
_config.CodexAppServerActive
&& (ctx.IsReviewFlow || (_config.CodexAppServerInteractive && !ctx.IsReview));

public Task<HostedRuntimeStart> StartAsync(RuntimeStartContext ctx, CancellationToken ct) {
// §2.7 B4: resume is app-server-only (thread/resume). A resume request routed to the PTY path can't
Expand Down
29 changes: 29 additions & 0 deletions src/Capacitor.Cli.Daemon/Harness/Codex/CodexTransportDecision.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,40 @@ internal static class CodexTransportDecision {
/// selection.</summary>
public const string VersionFloor = "0.146.0";

/// <summary>Binds the two environment variables that decide this daemon's Codex transport onto
/// <paramref name="config"/>. Both are read from the environment and nowhere else — no profile or
/// config-file binding — so the variable NAMES and the present-but-empty guard are part of the
/// contract an operator relies on, not incidental. Takes the lookup as a delegate so that contract is
/// testable without a process environment.
///
/// <para>An absent or empty value leaves the existing setting alone rather than resetting it, matching
/// every other env binding in the daemon's startup.</para></summary>
public static void BindFromEnvironment(Capacitor.Cli.Daemon.DaemonConfig config, Func<string, string?> getEnv) {
if (getEnv("KCAP_CODEX_TRANSPORT") is { Length: > 0 } transport)
config.CodexTransport = transport;

if (getEnv("KCAP_CODEX_APPSERVER_INTERACTIVE") is { Length: > 0 } interactive)
config.CodexAppServerInteractive = IsInteractiveOptIn(interactive);
}

/// <summary>Resolves the effective transport: app-server only when selected AND the installed
/// build meets <see cref="VersionFloor"/>. An unknown/unparseable version fails toward PTY.</summary>
public static bool UsesAppServer(string? transport, string? cliVersion) =>
IsAppServerSelected(transport) && MeetsFloor(cliVersion);

/// <summary>Reads the per-daemon interactive opt-in from its environment value. Trimmed and
/// case-insensitive, matching the daemon's other environment booleans
/// (<c>DaemonRunner.ParseDebugFramesFlag</c>), over a superset of their vocabulary. Anything else —
/// including "0", "false" and any typo — leaves it OFF. Off is the safe direction: a mistyped value
/// keeps the daemon on the transport it already had rather than moving interactive hosting onto an
/// operator who did not ask for it.</summary>
public static bool IsInteractiveOptIn(string? value) =>
value?.Trim() is { Length: > 0 } v
&& (v == "1"
|| string.Equals(v, "true", StringComparison.OrdinalIgnoreCase)
|| string.Equals(v, "yes", StringComparison.OrdinalIgnoreCase)
|| string.Equals(v, "on", StringComparison.OrdinalIgnoreCase));

/// <summary>Resolves the daemon-wide <c>CodexAppServerActive</c> at startup. The version probe is
/// deferred behind the selection check so a PTY daemon (the default) never pays for it — which is
/// why this owns the composition rather than the caller evaluating the probe eagerly.</summary>
Expand Down
5 changes: 4 additions & 1 deletion src/Capacitor.Cli/Services/ServiceEnvironment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ static class ServiceEnvironment {
/// the unit is a file on disk.</summary>
static readonly string[] Keys =
["PATH", "KCAP_PROFILE", "KCAP_URL", "KCAP_CLAUDE_PATH", "KCAP_CODEX_PATH",
"KCAP_CONSENT_SEED_DEFAULT", "KCAP_EXPECT_SERVER_URL"];
"KCAP_CONSENT_SEED_DEFAULT", "KCAP_EXPECT_SERVER_URL",
// Codex transport selection + interactive opt-in: the daemon reads both from its own
// environment and nowhere else, so the install has to carry them into the unit.
"KCAP_CODEX_TRANSPORT", "KCAP_CODEX_APPSERVER_INTERACTIVE"];

/// <summary>
/// The unattended reviewers' opt-OUT switches. Carried on every platform: these are booleans, not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,22 +72,83 @@ CodexLauncher NewLauncher() =>
Cols: 80, Rows: 24, ServerUrl: "https://t.example", DaemonBridgeUrl: null, CapacitorPath: "/opt/kcap");

CodexHostedAgentRuntimeFactory Factory(
RecordingPtyFactory pty, bool appServerActive, CodexAppServerSpawnFactory? spawn = null) =>
RecordingPtyFactory pty, bool appServerActive, CodexAppServerSpawnFactory? spawn = null,
bool appServerInteractive = false) =>
new(NewLauncher(), pty,
new DaemonConfig { CodexAppServerActive = appServerActive, Version = "0.146.0" },
new DaemonConfig {
CodexAppServerActive = appServerActive,
CodexAppServerInteractive = appServerInteractive,
Version = "0.146.0",
},
NullLoggerFactory.Instance, spawn);

// ── Routing decision ─────────────────────────────────────────────────────────────────────
[Test]
[Arguments(false, true, false)] // not active -> pty even for a review flow
[Arguments(true, false, false)] // active but interactive -> pty
[Arguments(true, false, false)] // active, interactive, not opted in -> pty
[Arguments(true, true, true)] // active + review flow -> app-server
public async Task UsesAppServer_gates_on_active_AND_review_flow(bool active, bool isReviewFlow, bool expected) {
using var wt = new TempDir();
var factory = Factory(new RecordingPtyFactory(), active);
await Assert.That(factory.UsesAppServer(Ctx(isReviewFlow, wt.Path))).IsEqualTo(expected);
}

/// <summary>The per-daemon opt-in widens app-server to interactive launches on THIS daemon only.
/// Reviewers are unaffected by it, and it cannot switch a daemon whose transport is still PTY —
/// which is what lets one daemon run interactive on app-server while a fleet stays on PTY.</summary>
[Test]
[Arguments(true, false, true, true)] // opted in + active, interactive launch -> app-server
[Arguments(true, true, true, true)] // opted in + active, review flow -> app-server
[Arguments(false, false, true, false)] // NOT opted in, interactive -> pty
[Arguments(true, false, false, false)] // opted in but transport still pty -> pty
public async Task UsesAppServer_admits_interactive_only_where_the_daemon_opted_in(
bool interactiveOptIn, bool isReviewFlow, bool active, bool expected) {
using var wt = new TempDir();
var factory = Factory(new RecordingPtyFactory(), active, appServerInteractive: interactiveOptIn);
await Assert.That(factory.UsesAppServer(Ctx(isReviewFlow, wt.Path))).IsEqualTo(expected);
}

/// <summary>The opt-in names INTERACTIVE, and a PR review is not that. Expressed as "not a review
/// flow" the switch would move PR review too — a launch class the operator never opted in — so this
/// pins the third class explicitly rather than leaving it to the negative form.</summary>
[Test]
public async Task UsesAppServer_leaves_a_pr_review_on_pty_even_when_the_daemon_opted_in() {
using var wt = new TempDir();
var factory = Factory(new RecordingPtyFactory(), appServerActive: true, appServerInteractive: true);
var prReview = Ctx(isReviewFlow: false, wt.Path) with { IsReview = true };

await Assert.That(factory.UsesAppServer(prReview)).IsFalse();
// Control: the same daemon DOES take an interactive launch to app-server.
await Assert.That(factory.UsesAppServer(Ctx(isReviewFlow: false, wt.Path))).IsTrue();
}

/// <summary>The env spelling is the operator's only lever, so it is pinned directly: affirmative
/// spellings turn it on — trimmed and case-insensitive, so an ordinary shell export is not silently
/// discarded — and everything else, including "0", "false" and a typo, leaves it off. Off is the safe
/// direction; a value the parser does not know must never move a daemon.</summary>
[Test]
[Arguments("1", true)]
[Arguments("true", true)]
[Arguments("TRUE", true)]
[Arguments("True", true)]
[Arguments("yes", true)]
[Arguments("on", true)]
[Arguments("TrUe", true)]
[Arguments("ON", true)]
[Arguments("Yes", true)]
[Arguments(" true ", true)]
[Arguments("\ttrue\n", true)]
[Arguments(" ", false)]
[Arguments("0", false)]
[Arguments("false", false)]
[Arguments("off", false)]
[Arguments("no", false)]
[Arguments("app-server", false)]
[Arguments("", false)]
[Arguments(null, false)]
public async Task Interactive_opt_in_accepts_only_affirmative_spellings(string? value, bool expected) =>
await Assert.That(CodexTransportDecision.IsInteractiveOptIn(value)).IsEqualTo(expected);

[Test]
public async Task Pty_transport_delegates_a_review_flow_to_the_pty_factory() {
using var wt = new TempDir();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,50 @@ public async Task MeetsFloor_is_exactly_the_pinned_floor() {
await Assert.That(CodexTransportDecision.MeetsFloor("0.146")).IsTrue(); // patch defaults to 0
await Assert.That(CodexTransportDecision.MeetsFloor("0.145.99")).IsFalse();
}

// ── Environment binding: the variable names and the empty guard are the operator's contract ──

static DaemonConfig BoundWith(params (string Key, string? Value)[] env) {
var config = new DaemonConfig();
var map = env.ToDictionary(e => e.Key, e => e.Value, StringComparer.Ordinal);
CodexTransportDecision.BindFromEnvironment(config, k => map.TryGetValue(k, out var v) ? v : null);

return config;
}

/// <summary>The exact variable names bind. These are read from the environment and nowhere else, so a
/// renamed key would silently leave a daemon on defaults while an operator believed they had selected
/// app-server — which the helper-only tests could not catch.</summary>
[Test]
public async Task BindFromEnvironment_binds_both_codex_transport_keys() {
var config = BoundWith(("KCAP_CODEX_TRANSPORT", "app-server"), ("KCAP_CODEX_APPSERVER_INTERACTIVE", "1"));

await Assert.That(config.CodexTransport).IsEqualTo("app-server");
await Assert.That(config.CodexAppServerInteractive).IsTrue();
}

/// <summary>An unset or present-but-empty variable leaves the existing setting alone rather than
/// resetting it — the same contract every other env binding in daemon startup follows.</summary>
[Test]
public async Task BindFromEnvironment_leaves_settings_alone_when_unset_or_empty() {
var untouched = BoundWith();
await Assert.That(untouched.CodexTransport).IsEqualTo("pty"); // the shipped default
await Assert.That(untouched.CodexAppServerInteractive).IsFalse();

var empty = BoundWith(("KCAP_CODEX_TRANSPORT", ""), ("KCAP_CODEX_APPSERVER_INTERACTIVE", ""));
await Assert.That(empty.CodexTransport).IsEqualTo("pty");
await Assert.That(empty.CodexAppServerInteractive).IsFalse();
}

/// <summary>A value the parser does not recognise leaves the opt-in OFF, and the two keys are
/// independent: selecting app-server does not by itself opt a daemon's interactive launches in.</summary>
[Test]
public async Task BindFromEnvironment_keeps_the_opt_in_off_for_unrecognised_values_and_is_independent() {
var typo = BoundWith(("KCAP_CODEX_APPSERVER_INTERACTIVE", "ture"));
await Assert.That(typo.CodexAppServerInteractive).IsFalse();

var transportOnly = BoundWith(("KCAP_CODEX_TRANSPORT", "app-server"));
await Assert.That(transportOnly.CodexTransport).IsEqualTo("app-server");
await Assert.That(transportOnly.CodexAppServerInteractive).IsFalse();
}
}
18 changes: 18 additions & 0 deletions test/Capacitor.Cli.Tests.Unit/Services/ServiceEnvironmentTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,24 @@ public async Task Build_never_invents_a_consent_flag() {
await Assert.That(env.ContainsKey(key)).IsFalse();
}

/// <summary>Both Codex transport settings reach a service unit on BOTH platforms. The daemon reads
/// them from its own environment and nowhere else, so a unit missing them silently runs PTY.</summary>
[Test]
[Arguments(false)]
[Arguments(true)]
public async Task Codex_transport_selection_survives_a_service_install(bool isWindows) {
var source = new Dictionary<string, string> {
["PATH"] = "/usr/bin",
["KCAP_CODEX_TRANSPORT"] = "app-server",
["KCAP_CODEX_APPSERVER_INTERACTIVE"] = "1",
};

var env = ServiceEnvironment.Build(profileName: null, source: source, config: Config.Root, isWindows: isWindows);

await Assert.That(env.TryGetValue("KCAP_CODEX_TRANSPORT", out var t) ? t : null).IsEqualTo("app-server");
await Assert.That(env.TryGetValue("KCAP_CODEX_APPSERVER_INTERACTIVE", out var i) ? i : null).IsEqualTo("1");
}

/// <summary>
/// EVERY reviewer's opt-out reaches a service unit, on BOTH platforms, with a DISABLING value.
///
Expand Down