Skip to content

[AI-1762] guard-1: suppress the codex hook watcher for envelope-sourced sessions - #617

Merged
realtonyyoung merged 2 commits into
mainfrom
tonyyoung/ai-1762-guard1-hook-suppression
Aug 19, 2026
Merged

realtonyyoung merged 2 commits into
mainfrom
tonyyoung/ai-1762-guard1-hook-suppression

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

What

Guard-1 of §2.5's ingestion dedup (the daemon half; guard-2 is the server per-write source refusal). A hosted app-server codex session will — once the activation slice flips it on — push its transcript through the daemon's envelope path. This guard stops the rollout watcher double-ingesting that same session.

  • Marker (daemon) — CodexHostedAgentRuntimeFactory.BuildEnv stamps KCAP_HOSTED_APPSERVER=1 on the spawned codex process only when the runtime is emitting envelopes. The marker and the runtime's own emission now both read one emitEnvelopeTranscript decision in StartAppServerAsync, so they can never disagree — the activation slice flips both by flipping that single local.
  • Suppression (hook) — CodexHookCommand skips WatcherManager.EnsureWatcherRunning at the SessionStart and Stop spawn sites when the inherited marker is present. Only the watcher (the transcript-forward vector) is suppressed; the idempotent lifecycle POSTs and the cross-session spool drain are untouched, so the idle-marker stop POST still clears the "working" indicator.

Dormant

emitEnvelopeTranscript is hardcoded false here, so the marker is never stamped and the shipped reviewers (still hook-ingested) keep their watcher exactly as today. The activation slice (PR6) flips the one decision, and guard-1 + guard-2 + the emission all come alive together.

Tests

  • Daemon: BuildEnv stamps the marker iff emitting; a real app-server launch stamps no marker while dormant.
  • Hook: watcher suppressed at both SessionStart and Stop when the marker is set (lifecycle POSTs still fire), with a positive control (no marker → watcher spawns) so the suppression assertion can actually fail on a regression.

Comments are Linear-id-free per scripts/check-linear-ids.sh (verified locally).

Part of AI-1762.

…ed sessions

A hosted app-server codex session will (once the activation slice flips it) push
its transcript through the daemon's envelope path. To stop the rollout watcher
double-ingesting the same session, the daemon stamps KCAP_HOSTED_APPSERVER=1 on
the codex process and the codex hook stands its watcher down.

Marker (daemon): BuildEnv sets KCAP_HOSTED_APPSERVER only when the runtime is
emitting envelopes. The marker and the runtime's own emission now read ONE
emitEnvelopeTranscript decision in StartAppServerAsync, so they can never
disagree — the activation slice flips both by flipping that single local. It is
false today, so the marker is never stamped and the shipped reviewers (still
hook-ingested) keep their watcher.

Suppression (hook): the SessionStart and Stop watcher-spawn sites skip
EnsureWatcherRunning when the inherited marker is present. Only the watcher (the
transcript-forward vector) is suppressed — the idempotent lifecycle POSTs and the
cross-session spool drain are untouched, so the "working" indicator still clears.

Tests: BuildEnv gating (marker present iff emitting) + a dormant-path proof that a
real app-server launch stamps no marker today; hook suppression at both sites with
a positive control (no marker -> watcher spawns). Linear-id-free per the guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 19, 2026

Copy link
Copy Markdown

AI-1762

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Suppress Codex watchers for envelope-sourced hosted sessions

🐞 Bug fix 🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Couple hosted-session marker stamping to the runtime's envelope-emission decision.
• Suppress SessionStart and Stop rollout watchers for envelope-sourced Codex sessions.
• Preserve lifecycle posts and dormant watcher behavior with focused positive and negative tests.
Diagram

graph TD
    A{"Emit envelopes?"} --> B["Build environment"] --> C["App-server runtime"] --> D["Codex hook"] --> E{"Marker present?"}
    A -->|Controls emission| C
    E -->|No| F["Rollout watcher"]
    E -->|Yes| G["Watcher suppressed"]
    D --> H["Lifecycle API"]
Loading
High-Level Assessment

The environment marker is the appropriate cross-process signal because Codex hooks naturally inherit the spawned process environment. Coupling marker creation and envelope emission to one decision prevents split-brain ingestion behavior; disabling hooks entirely would incorrectly suppress lifecycle posts, while inferring hosted status from existing identifiers would be ambiguous.

Files changed (4) +167 / -9

Bug fix (2) +32 / -9
CodexHostedAgentRuntimeFactory.csGate hosted-session marker with envelope emission +14/-3

Gate hosted-session marker with envelope emission

• Introduces one dormant 'emitEnvelopeTranscript' decision shared by runtime construction and environment generation. 'BuildEnv' now stamps 'KCAP_HOSTED_APPSERVER=1' only when envelope transcript emission is enabled.

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

CodexHookCommand.csSuppress watchers for marked hosted sessions +18/-6

Suppress watchers for marked hosted sessions

• Checks the inherited hosted app-server marker before starting or restarting rollout watchers during SessionStart and Stop. Lifecycle posts and spool draining remain active.

src/Capacitor.Cli/Commands/Harness/CodexHookCommand.cs

Tests (2) +135 / -0
CodexHostedAgentRuntimeFactoryTests.csVerify marker gating and dormant launches +47/-0

Verify marker gating and dormant launches

• Tests that environment construction stamps the marker exactly when envelope emission is enabled. A real app-server launch test confirms the dormant path leaves the marker unset.

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

CodexHookCommandTests.csCover watcher suppression and lifecycle preservation +88/-0

Cover watcher suppression and lifecycle preservation

• Verifies marked SessionStart and Stop hooks skip watcher creation while retaining lifecycle posts. Adds an unmarked Stop positive control proving the watcher still spawns normally.

test/Capacitor.Cli.Tests.Unit/Commands/Harness/CodexHookCommandTests.cs

@qodo-code-review

qodo-code-review Bot commented Aug 19, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Inherited marker drops transcripts ✓ Resolved 🐞 Bug ☼ Reliability
Description
BuildEnv only adds KCAP_HOSTED_APPSERVER when emission is enabled but never clears an inherited
value, so a non-emitting app-server launched by a daemon with this variable already set remains
marked. Its hooks then suppress the only active watcher while _emitEnvelopeTranscript is false,
causing transcript ingestion loss.
Code

src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[148]

+        if (emitEnvelopeTranscript) env["KCAP_HOSTED_APPSERVER"] = "1";
Evidence
The factory omits the marker from its overlay when emission is false, but its production launcher
constructs a ProcessStartInfo and only overlays supplied keys without removing inherited ones.
Repository launch code explicitly calls psi.Environment.Remove(...) when inherited variables must
not reach a child, confirming that omission from an overlay is insufficient; the hook suppresses
watchers solely from this variable, while the runtime documents that envelope emission remains
disabled when the flag is false.

src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[137-149]
src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[164-171]
src/Capacitor.Cli/Commands/Harness/CodexHookCommand.cs[458-464]
src/Capacitor.Cli.Daemon/Harness/Codex/CodexAppServerHostedAgentRuntime.cs[119-125]
src/Capacitor.Cli.Core/Harness/Claude/ClaudeCliRunner.cs[197-217]

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

## Issue description
A stale `KCAP_HOSTED_APPSERVER=1` inherited by the daemon reaches a Codex child even when `emitEnvelopeTranscript` is false. The hook consequently suppresses its watcher despite the runtime not emitting envelopes.

## Issue Context
`ProcessStartInfo.Environment` contains inherited variables, while `BuildEnv` only supplies overlays. Ensure non-envelope launches explicitly remove the marker from the effective child environment, while envelope-enabled launches still set it to `1`.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[137-149]
- src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[164-171]
- test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Codex/CodexHostedAgentRuntimeFactoryTests.cs[149-191]

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


2. Envelope decision comment is verbose ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The five-line comment around emitEnvelopeTranscript explains implementation mechanics and rollout
history in excessive detail. This violates the requirement that comments remain concise and focused
on the non-obvious constraint.
Code

src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[R80-83]

+        // §2.5: an envelope-sourced session is the transcript's exclusive source, so the
+        // KCAP_HOSTED_APPSERVER marker (which makes the codex hook/watcher stand down — guard-1) and the
+        // runtime's own envelope emission must never disagree — both read this ONE decision. It stays
+        // false until the activation slice flips it together with the runtime's Transcript wiring, so
Evidence
PR Compliance ID 4 requires comments to be short and focused on non-obvious reasons. The added
five-line comment describes marker behavior, activation sequencing, transcript wiring, and current
rollout state rather than stating the invariant concisely.

CLAUDE.md: Keep Code Comments Minimal and Focused on Non-Obvious “Why”
src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[80-85]

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 comment describing `emitEnvelopeTranscript` is verbose and paraphrases implementation mechanics.

## Issue Context
Retain only the non-obvious invariant that marker stamping and envelope emission must use the same decision.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs[80-85]

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


3. Hook suppression comment is verbose ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The added five-line hook comment restates how the marker is propagated and which sessions contain
it. It should be reduced to the non-obvious reason the watcher must not start.
Code

src/Capacitor.Cli/Commands/Harness/CodexHookCommand.cs[R458-461]

+    // §2.5 guard-1: a hosted app-server codex session is envelope-sourced — the daemon pushes its
+    // transcript directly, so this hook must NOT spawn a rollout watcher (that would double-ingest the
+    // same session). The daemon stamps the marker on the codex process; the hook inherits it through the
+    // same channel as KCAP_AGENT_ID / KCAP_DAEMON_URL. Absent on every non-hosted and pre-activation
Evidence
PR Compliance ID 4 prohibits verbose comments and commentary that paraphrases straightforward code.
The added block spends five lines explaining watcher suppression, environment inheritance, and
marker absence immediately above a self-describing marker check.

CLAUDE.md: Keep Code Comments Minimal and Focused on Non-Obvious “Why”
src/Capacitor.Cli/Commands/Harness/CodexHookCommand.cs[458-464]

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 watcher-suppression comment is unnecessarily long and explains straightforward marker propagation details.

## Issue Context
Preserve only the reason for suppression: envelope ingestion would otherwise duplicate transcript ingestion.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/Harness/CodexHookCommand.cs[458-464]

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


Grey Divider

Context sources
Review mode: ⚖️ Balanced: This is a behavioral daemon/hook ingestion change spanning multiple runtime and lifecycle paths, with deduplication and environment-marker coordination that merits a careful single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs Outdated
Comment thread src/Capacitor.Cli/Commands/Harness/CodexHookCommand.cs Outdated
Comment thread src/Capacitor.Cli.Daemon/Harness/Codex/CodexHostedAgentRuntimeFactory.cs Outdated
…qodo/CI)

qodo: BuildEnv only overlaid KCAP_HOSTED_APPSERVER, so a child could inherit a
stale marker from the daemon's own environment and — while NOT emitting envelopes
— suppress its hook watcher, losing transcript ingestion. Materialize the child
env through ApplyChildEnv, which clears any inherited marker before applying the
overlay, so the child's marker reflects only this launch's emit decision. Added
two ApplyChildEnv tests (inherited-clear when dormant; set when emitting).

CI: the three hook tests reused session_id "abc", which collides with the
real-watcher test's registration — EnsureWatcherRunning fast-paths on
IsWatcherAlive(key), so the positive control observed zero spawns under CI
ordering. Give each test a unique session id.

Also trimmed the two comments qodo flagged as verbose.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@realtonyyoung
realtonyyoung merged commit 25e7433 into main Aug 19, 2026
6 checks passed
@realtonyyoung
realtonyyoung deleted the tonyyoung/ai-1762-guard1-hook-suppression branch August 19, 2026 18:42
realtonyyoung added a commit that referenced this pull request Aug 19, 2026
…erver (#624)

* [AI-1762] §2.6: activate envelope-sourced transcript for hosted app-server

Flip the app-server path from the dormant hook-sourced transcript to the live
envelope source. One envelopeSourced decision in StartAppServerAsync now drives all
four coupled facts so they can never desync: the guard-1 marker (BuildEnv →
KCAP_HOSTED_APPSERVER, stands the rollout watcher down), the runtime's envelope
emission (emitEnvelopeTranscript), the deferred first turn behind the source claim
(deferFirstTurn), and the forwarder wiring (Transcript: runtime on HostedRuntimeStart).

Everything downstream was already built and merged and keys off these: the
orchestrator drains the runtime's envelopes through AcpTranscriptForwarder and, seeing
RequiresSourceClaimBeforeFirstTurn, runs source-claim → BeginFirstTurn → confirm
(#612); guard-2 refuses any stray hook write server-side (#1547); death-close is the
existing finalize → EndAgentSession, and the 60-min rollout-idle fallback disengages
automatically because the stamped marker suppresses the hook's watcher (#617). So this
is a four-value flip in one method, not new machinery.

Scope: this activates the app-server path, which today is reviewers only (UsesAppServer
stays `active && IsReviewFlow`) — the interactive routing flip is deliberately the LAST
step, after §2.6 lifecycle + §2.7. Gated behind codex.transport=app-server (default
pty), so a default daemon is byte-unchanged; requires a server carrying #1530/#1547
(server-before-CLI); and AI-2110's live cert validates it before app-server becomes the
default. Only the dormant-marker factory test inverts (now asserts the marker is
stamped, the Transcript is wired, and the first turn is deferred).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* [AI-1762] §2.6: a cancelled source-claim must not dispatch the first turn (qodo)

qodo reliability bug (latent in #612, armed by this activation): a launch that
finalized during the source-claim / forwarder setup could still release the held
first turn. StartForwarderAfterBind aborts silently when finalize cancelled acpCts
and/or removed the agent, but StartEnvelopeSourcedSessionAsync then called
BeginFirstTurnAsync unconditionally, and BeginFirstTurnAsync unsealed the dispatcher
(sending turn/start) BEFORE observing its token — so turn/start could race the
teardown.

Two-part fix: BeginFirstTurnAsync now ThrowIfCancellationRequested BEFORE Unseal (the
dispatch runs on CancellationToken.None and can't be recalled, so this is the only
safe gate); and StartEnvelopeSourcedSessionAsync re-checks the SAME liveness
StartForwarderAfterBind guards on (cancelled OR agent-removed) before dispatching, so
the agent-removed-but-token-live arm is covered too. Both land in the OCE/return arms
that already tear down or log-and-abort. Added Cancelled_BeginFirstTurn_does_not_
dispatch_the_held_turn. Also trimmed the activation test comment qodo flagged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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