Skip to content

Let one daemon host interactive Codex on app-server without moving a fleet - #736

Merged
realtonyyoung merged 5 commits into
mainfrom
tonyyoung/appserver-per-daemon-optin
Sep 1, 2026
Merged

realtonyyoung merged 5 commits into
mainfrom
tonyyoung/appserver-per-daemon-optin

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

Unblocks the AI-2110 certification strategy: opt one daemon into Codex app-server — for hosted agents and unattended reviewers — while everyone else keeps PTY.

Two gaps this closes

1. The transport selection never reached a supervised unit. KCAP_CODEX_TRANSPORT is read by the daemon from its own environment and nowhere else — no profile or config-file binding — so kcap daemon service install silently dropped it and the daemon ran PTY while the operator believed they had selected app-server, with nothing in the unit to show otherwise. It is now captured, alongside the reviewer consent switches that carry for exactly the same reason. (Found during the AI-2110 cert: the cert daemon had to run unsupervised because of this.)

2. Interactive launches were hard-gated to PTY. UsesAppServer required ctx.IsReviewFlow, so app-server could only ever host unattended reviewers, and AI-2110's interactive ACs (AC1–AC3) were unreachable. That gate becomes a per-daemon opt-in:

_config.CodexAppServerActive && (ctx.IsReviewFlow || _config.CodexAppServerInteractive)

Where the opt-in is off, nothing changes. Where an operator turns it on (KCAP_CODEX_APPSERVER_INTERACTIVE=1), that daemon hosts interactive Codex over app-server while every other daemon — and every customer — stays on PTY. It widens which launches take the transport; it cannot select the transport, so a PTY daemon is unaffected by it.

Why an opt-in rather than the flip

The alternative was dropping && ctx.IsReviewFlow outright, which moves an entire fleet at once and can't be certified against production without exposing everyone. This makes the interactive path certifiable on a single daemon, against a real deployment, with a blast radius of one.

Scope worth knowing

This covers launches the server dispatches — the web launch dialog, PR review, review flows. kcap agent start spawns an attachable terminal straight through the local control socket and never reaches the runtime factory, so it stays PTY regardless of the opt-in. I verified that empirically: with the gate flipped locally, kcap agent start codex still came up as codex --cd … --no-alt-screen, the PTY launcher.

Tests

  • Codex_transport_selection_survives_a_service_install (both platforms).
  • UsesAppServer_admits_interactive_only_where_the_daemon_opted_in — 4 cases including opted in but transport still PTY → PTY, so the opt-in provably cannot select the transport.

Both mutation-checked: dropping the keys from the capture list fails the first; restoring the hard review-flow gate fails the second.

🤖 Generated with Claude Code

…fleet

Two gaps stopped an operator opting a single daemon into app-server:

The transport selection never reached a supervised unit. KCAP_CODEX_TRANSPORT is
read by the daemon from its own environment and nowhere else — no profile or
config-file binding — so `daemon service install` silently dropped it and the
daemon ran PTY while the operator believed they had selected app-server. Captured
now, alongside the reviewer consent switches that carry for the same reason.

Interactive launches were hard-gated to PTY, so app-server could only ever host
unattended reviewers. That gate is now a per-daemon opt-in
(KCAP_CODEX_APPSERVER_INTERACTIVE): where it is off, nothing changes; where an
operator turns it on, that daemon hosts interactive Codex over app-server while
every other daemon — and every customer — stays on PTY. It widens which launches
take the transport, it cannot select the transport, so a PTY daemon is unaffected.

Scope worth knowing: this covers launches the SERVER dispatches (the web launch
dialog, PR review, review flows). `kcap agent start` spawns an attachable terminal
straight through the local control socket, never reaching the runtime factory, so
it stays PTY regardless of the opt-in.

Both tests are mutation-checked: dropping the keys from the capture list fails the
service-install test, and restoring the hard review-flow gate fails the opt-in test.
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Enable per-daemon interactive Codex app-server routing

🐞 Bug fix ✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Preserve Codex transport settings when installing supervised daemon services.
• Route server-dispatched interactive Codex sessions through app-server on opted-in daemons.
• Verify routing gates and environment capture across launch types and platforms.
Diagram

graph TD
  Install["Service Install"] --> Env["Captured Env"] --> Runner["Daemon Runner"] --> Config["Transport Config"] --> Router{"Eligible Launch"}
  Router -->|"Active review or opt-in"| App["App Server"]
  Router -->|"Otherwise"| Pty["PTY Runtime"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Route all interactive sessions to app-server
  • ➕ Eliminates the additional rollout flag and simplifies routing policy.
  • ➕ Moves every eligible daemon to one Codex hosting transport immediately.
  • ➖ Creates a fleet-wide behavior change with no production canary.
  • ➖ Expands certification risk and rollback impact beyond one daemon.
2. Add profile-backed transport settings
  • ➕ Provides durable declarative configuration without reinstalling service units.
  • ➕ Could centralize transport and interactive rollout settings with other daemon options.
  • ➖ Requires broader profile schema, binding, migration, and documentation changes.
  • ➖ Does not match the existing environment-only Codex transport control without additional work.

Recommendation: Keep the per-daemon environment opt-in: it preserves current PTY defaults, supports a one-daemon certification canary, and cannot activate app-server unless transport selection and version checks already succeeded. Profile-backed configuration can be considered separately if operators need dynamic or centrally managed rollout controls.

Files changed (6) +69 / -5

Enhancement (2) +16 / -1
DaemonConfig.csAdd per-daemon interactive app-server setting +11/-0

Add per-daemon interactive app-server setting

• Adds a default-off configuration flag that widens an already-active Codex app-server transport to server-dispatched interactive launches. Documentation clarifies fleet isolation and excludes direct local 'kcap agent start' launches.

src/Capacitor.Cli.Daemon/DaemonConfig.cs

CodexHostedAgentRuntimeFactory.csRoute opted-in interactive launches to app-server +5/-1

Route opted-in interactive launches to app-server

• Expands app-server eligibility from review flows to interactive launches when the daemon opt-in is enabled. Effective app-server activation remains mandatory, preserving PTY fallback for inactive transports.

src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs

Bug fix (1) +6 / -1
ServiceEnvironment.csPersist Codex transport controls in service units +6/-1

Persist Codex transport controls in service units

• Captures the Codex transport selection and interactive opt-in from the installer environment. This prevents supervised daemon installations from silently losing app-server rollout settings.

src/Capacitor.Cli/Services/ServiceEnvironment.cs

Tests (2) +43 / -3
CodexHostedAgentRuntimeFactoryTests.csCover per-daemon Codex routing decisions +23/-3

Cover per-daemon Codex routing decisions

• Extends the runtime factory fixture with the interactive opt-in and tests review, interactive, active-transport, and PTY fallback combinations. Existing default-off behavior remains covered.

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Codex/CodexHostedAgentRuntimeFactoryTests.cs

ServiceEnvironmentTests.csVerify Codex settings survive service installation +20/-0

Verify Codex settings survive service installation

• Adds Windows and non-Windows coverage proving both Codex transport environment variables are retained in generated service environments.

test/Capacitor.Cli.Tests.Unit/Services/ServiceEnvironmentTests.cs

Other (1) +4 / -0
DaemonRunner.csRead interactive app-server opt-in at startup +4/-0

Read interactive app-server opt-in at startup

• Parses 'KCAP_CODEX_APPSERVER_INTERACTIVE' into daemon configuration using accepted truthy values. The flag remains independent from transport activation and version-floor resolution.

src/Capacitor.Cli.Daemon/DaemonRunner.cs

@qodo-code-review

qodo-code-review Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Transport test comment overexplains rationale ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new test documentation goes beyond stating the pinned cross-platform behavior and narrates a
hypothetical failed install plus the operator's belief. This unnecessary rationale makes the test
comment dense and stale-prone.
Code

test/Capacitor.Cli.Tests.Unit/Services/ServiceEnvironmentTests.cs[R255-258]

+    /// <summary>The Codex transport selection and its interactive opt-in reach a service unit on BOTH
+    /// platforms. The daemon reads these from its own environment and nowhere else — there is no profile
+    /// or config-file binding — so a supervised install that drops them leaves the daemon on PTY while the
+    /// operator believes they selected app-server, with nothing in the unit to show otherwise.</summary>
Evidence
PR Compliance ID 24 says test doc comments should state what the test pins and longer comments must
contain essential information unavailable elsewhere. The first sentence already states the
guarantee, while lines 256-258 add redundant counterfactual and operator-belief narration.

CLAUDE.md: Write Only Current, Necessary, Constraint-Focused Comments
test/Capacitor.Cli.Tests.Unit/Services/ServiceEnvironmentTests.cs[255-258]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test summary contains a multi-line operational narrative beyond the behavior the test pins.

## Issue Context
Keep a concise statement that both Codex environment settings survive service installation on both platforms; the assertions already show the details.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/Services/ServiceEnvironmentTests.cs[255-258]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Opt-in comment narrates history ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new CodexAppServerInteractive documentation includes fleet-upgrade and prior-PTY narration
(has always used) rather than limiting itself to the current configuration contract. This creates
stale-prone source commentary that violates the prohibition on change-history narration.
Code

src/Capacitor.Cli.Daemon/DaemonConfig.cs[R171-173]

+    /// take that transport, it does not select it. Off by default so a fleet upgrading to app-server
+    /// reviewers keeps interactive on the PTY it has always used; an operator turns it on for one
+    /// daemon (KCAP_CODEX_APPSERVER_INTERACTIVE) without touching anyone else.
Evidence
PR Compliance ID 25 excludes phrases describing how code used to differ. The added comment says
interactive launches remain on the PTY they have always used and frames the setting around a fleet
upgrade, which is historical rollout context rather than a live API constraint.

CLAUDE.md: Exclude Ephemeral Project and Review Metadata from Comments
src/Capacitor.Cli.Daemon/DaemonConfig.cs[171-173]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `CodexAppServerInteractive` comment narrates rollout history and prior PTY behavior instead of documenting only the current contract.

## Issue Context
Retain the durable facts that the option requires `CodexAppServerActive`, widens routing, and does not select the transport; remove fleet-upgrade and `has always used` narration.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/DaemonConfig.cs[169-177]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Service key comment overexplains behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The four-line comment above the new environment keys narrates the omitted-capture failure and
operator action instead of concisely documenting the live requirement. The essential constraint can
be stated directly in one or two lines, reducing duplicated and stale-prone explanation.
Code

src/Capacitor.Cli/Services/ServiceEnvironment.cs[R17-20]

+         // The Codex transport selection and its interactive opt-in, for the same reason as the
+         // reviewer consent switches below: the daemon reads them from its own environment and
+         // nowhere else, so without capture a supervised install silently drops them and the daemon
+         // runs PTY no matter what the operator exported.
Evidence
PR Compliance ID 24 requires comments to remain sparse, generally one or two lines, and to avoid
implementation narration. Lines 17-20 repeat the capture rationale in four lines and describe the
counterfactual without capture execution path rather than only the current constraint.

CLAUDE.md: Write Only Current, Necessary, Constraint-Focused Comments
src/Capacitor.Cli/Services/ServiceEnvironment.cs[17-20]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added service-environment comment is longer than necessary and narrates the behavior that would occur without this implementation.

## Issue Context
Preserve only the non-obvious live constraint: the daemon reads these values from its own environment, so service installation must carry them into the unit.

## Fix Focus Areas
- src/Capacitor.Cli/Services/ServiceEnvironment.cs[17-21]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Interactive opt-in misparsed ✓ Resolved 🐞 Bug ≡ Correctness
Description
KCAP_CODEX_APPSERVER_INTERACTIVE uses exact, untrimmed string matching, so values such as TrUe,
ON, or  true  are silently interpreted as false despite the daemon's existing opt-in boolean
convention being trimmed and case-insensitive. Because this value directly gates interactive
app-server routing, an enabled daemon can unexpectedly continue launching interactive Codex through
PTY.
Code

src/Capacitor.Cli.Daemon/DaemonRunner.cs[R178-180]

+        if (Environment.GetEnvironmentVariable("KCAP_CODEX_APPSERVER_INTERACTIVE") is { Length: > 0 } envCodexInteractive)
+            config.CodexAppServerInteractive =
+                envCodexInteractive is "1" or "true" or "TRUE" or "True" or "yes" or "on";
Evidence
The changed code recognizes only a handful of exact spellings without normalization. The existing
daemon opt-in parser trims and compares true case-insensitively, while UsesAppServer proves that
a false parse routes non-review launches to PTY.

src/Capacitor.Cli.Daemon/DaemonRunner.cs[178-180]
src/Capacitor.Cli.Daemon/DaemonRunner.cs[1051-1057]
src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[75-87]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Parse `KCAP_CODEX_APPSERVER_INTERACTIVE` with the daemon's established trimmed, case-insensitive opt-in semantics so ordinary case and whitespace variants do not silently disable interactive app-server routing.

## Issue Context
`ParseDebugFramesFlag` already defines the daemon's opt-in environment-boolean behavior. Add focused parsing tests, including mixed-case and surrounding-whitespace inputs.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[178-180]
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[1051-1057]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli.Daemon/DaemonConfig.cs Outdated
Comment thread src/Capacitor.Cli/Services/ServiceEnvironment.cs Outdated
Comment thread test/Capacitor.Cli.Tests.Unit/Services/ServiceEnvironmentTests.cs Outdated
Comment thread src/Capacitor.Cli.Daemon/DaemonRunner.cs Outdated
Review found the predicate said 'not a review flow', which also swept PR review
onto app-server on an opted-in daemon — a launch class the operator never opted in
and the switch does not name. It now states interactive positively: neither a
review flow nor a PR review.

Also drops a stale summary that still claimed interactive always takes PTY, and
extracts the env spelling into CodexTransportDecision.IsInteractiveOptIn so the
operator's only lever is pinned directly rather than exercised through a hand-set
bool: affirmative spellings turn it on, everything else — including '0', 'false'
and a typo — leaves it off.

The PR-review case is mutation-checked: restoring the negative form fails it.
The variable names and the present-but-empty guard are the operator's contract —
these are read from the environment and nowhere else — but nothing covered the
wiring between the parsing helper and the config field, so a renamed key would
have passed every test while leaving a daemon on defaults.

BindFromEnvironment takes the lookup as a delegate and binds both codex transport
keys; DaemonRunner passes Environment.GetEnvironmentVariable. Tests pin the exact
names, that unset and present-but-empty leave the existing setting alone, that an
unrecognised value keeps the opt-in off, and that the two keys are independent —
selecting app-server does not by itself opt interactive in.

Mutation-checked: renaming the interactive key fails the binding test.
…booleans

Exact untrimmed matching read TrUe, ON and a shell-quoted " true " as OFF, so an
operator could set the variable and keep launching interactive Codex on PTY with
nothing to show why. Trim and compare case-insensitively, as ParseDebugFramesFlag
already does, over a superset of its vocabulary.

Mutation-checked: restoring exact matching fails five spelling cases.
@realtonyyoung
realtonyyoung merged commit da3fd4e into main Sep 1, 2026
6 checks passed
@realtonyyoung
realtonyyoung deleted the tonyyoung/appserver-per-daemon-optin branch September 1, 2026 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant