Follow-up to #41: foreground hard-ceiling regression + dead delta branches (protocol research attached) #43

Deferred from PR #41's review cycle (stopped by author decision after cycle 1; triage: #41 (comment)). Two confirmed FIX findings plus the protocol research that resolved the two UNVERIFIED ones.

Finding 1 (critical): idle-reset lets a foreground turn outlive the Bash-host SIGKILL, orphaning the interrupt

At base, FOREGROUND_TURN_TIMEOUT_MS = 110000 (codex-companion.mjs:76) was a non-resettable race against the whole turn, so a foreground turn was always cut at 110s — before Claude Code's ~120s Bash-tool SIGKILL — and the catch block in captureTurn (lib/codex.mjs:765-775) reached interruptAppServerTurn in time. With #41's idle reset, a foreground turn that keeps producing events sails past 110s to the host SIGKILL; the catch never runs, no turn/interrupt is sent, and the broker keeps executing a possibly write-capable turn (the broker does not interrupt on socket close — see #42 for the pre-existing links of that chain). Three of the four links predate #41; the reachability of this state is new.

Fix plan: split the budgets. Keep the resettable idle budget (--turn-timeout-ms / CODEX_TURN_TIMEOUT_MS, 110s FG / 600s BG defaults, unchanged) and make the hard wall-clock ceiling an option instead of the HARD_WALL_CLOCK_CEILING_MS constant: companion passes foreground → FOREGROUND_TURN_TIMEOUT_MS (110s), background → default 45 min; add CODEX_TURN_HARD_CEILING_MS env override (tests + non-Claude hosts). The existing catch already interrupts on any deadline rejection — no changes needed there. Foreground at defaults becomes byte-compatible with base; a lowered idle budget still catches stalls faster.

Test plan: fixture behavior streaming item/commandExecution/outputDelta every 500ms for ~20s then completing; run task --turn-timeout-ms 2000 with CODEX_TURN_HARD_CEILING_MS=3500 → must fail at ~3.5s with a ceiling error, job failed, and fixture state.lastInterrupt recorded (the interrupt reached the fake). Red pre-fix (task completes after ~20s), green post-fix. Plumbing: hardCeilingMs alongside turnTimeoutMs through buildTaskRequest / executeTaskRun / executeReviewRun → runAppServerTurn / runAppServerReview → captureTurn (lib/codex.mjs:1236,1378).

Finding 2: of the nine delta reset branches, only three can ever fire

Verified against the installed binary (codex-cli 0.146.0, serde method strings) and openai/codex sources at tag rust-v0.146.0 (codex-rs/app-server-protocol/src/protocol/v2/):

The opt-out lives in DEFAULT_CAPABILITIES.optOutNotificationMethods (lib/app-server.mjs:44-52), sent unconditionally (no caller overrides capabilities). The long-reasoning stretch — the exact sol/xhigh case #40 was filed for — is therefore NOT protected: its only liveness signals are the opted-out methods.

Fix plan: remove item/agentMessage/delta, item/reasoning/summaryTextDelta, item/reasoning/textDelta from the opt-out list (keep item/reasoning/summaryPartAdded opted out — boundary marker, not a reset source); drop the two unreachable command/exec/process cases; keep the deprecated fileChange case with a comment (older servers may still emit it). Test: fixture streams reasoning summary deltas across the idle budget → completes; plus assert the fixture-recorded handshake capabilities.optOutNotificationMethods no longer contains the three methods (red pre-fix).

Also deferred

• Live smoke against real codex after implementing: one turn via spawned transport, prompt triggering a slow shell command, log every notification method — verifies emission → broker relay → belongsToTurn → reset end-to-end. Caveat: an already-running broker initialized its app-server with the old opt-out list and keeps it until restarted — kill the broker before verifying.
• Two SKIPs from cycle 1: stale captureTurn comment (lists only the three lifecycle events), idle error message still says "turn budget".
• Idea (separate issue if pursued): auto-detach foreground→background at the ceiling instead of interrupting, riding the existing job system.

Relation

• Turn timeout should measure agent-idle time, not total turn wall-clock #40 — the idle-timeout design this follows up on; fix: measure turn timeout as agent idle time, not total wall-clock #41 — the implementation PR.
• SIGKILLed foreground companion orphans a live app-server turn; /cancel can't reach it #42 — pre-existing orphan chain (broker socket-close, /cancel reconciliation); Finding 1 here is the new reachability link on top of it.
• adversarial-review ignores --turn-timeout-ms — turnTimeoutMs not forwarded to runAppServerTurn #38 / fix: thread turn-timeout-ms into adversarial-review's app-server turn #39 — adversarial-review drops turnTimeoutMs entirely (600s lib default + 45min ceiling apply); interacts with Finding 1's plumbing but tracked separately.

Added after #41 was merged (merge commit c33cf49)

#41 was merged on the strength of npm run build + npm run check-version only; the full npm test gate was not re-run at merge time. The two items below were open at merge and are tracked here.

Finding 3 (RESOLVED — merged as #45, ed9b4e3): the three "pre-existing flakes" were a deterministic session-filter leak

Superseded diagnosis. An earlier revision of this issue attributed these failures to CLAUDE_PLUGIN_DATA leaking through run() into spawned subprocesses. That was wrong: the test still fails with CLAUDE_PLUGIN_DATA unset, and resolveStateDir was verified to return an identical path in-process and in the child. The real variable is CODEX_COMPANION_SESSION_ID.

Actual root cause. status and result without an explicit job id filter the job list to the current Claude session via filterJobsForCurrentSession (job-control.mjs:22-28), which is a no-op only when CODEX_COMPANION_SESSION_ID is unset. Three tests wrote fixture jobs carrying no sessionId and called run() with no env, so the child inherited whatever the host exported. Unset in CI and in a bare shell → filter is a no-op → green. Exported inside a Claude Code session → every fixture job is filtered away → red:

✖ status shows phases, hints, and the latest finished job
✖ status preserves adversarial review kind labels
✖ result returns the stored output for the latest finished job by default
  AssertionError: No finished Codex jobs found for this repository yet.

Not flaky — deterministic given the ambient environment. Confirmed by running the suite with env -u CODEX_COMPANION_SESSION_ID (all three green) versus with it exported (all three red). This also settles the audit item from the earlier revision: all three failures share one cause, and #41's PR body describing them as "pre-existing flakes reproducing on unmodified main" was accurate about where but wrong about why.

Fix as landed (ed63724, tests only, production code untouched). Follow the pattern already used by "status without a job id only shows jobs from the current Claude session" (runtime.test.mjs:1703): pin sessionId: "sess-current" on the fixture jobs and pass a matching CODEX_COMPANION_SESSION_ID to the child.

Rejected alternative, recorded so it is not retried. Scrubbing the variable centrally in run() (helpers.mjs) looks like the tidier fix and is wrong: it makes filterJobsForCurrentSession a no-op, so listings then include unrelated jobs, breaking four tests that depend on the filter being active — status --wait times out cleanly, both cancel tests, and the stop-hook review-gate test. Measured 3 failures before, 7 after.

Result: full runtime.test.mjs suite is 95/95 with CODEX_COMPANION_SESSION_ID exported, up from 92/95. npm run build and npm run check-version both pass.

Finding 4 (latent, from #45's review): buildEnv does not pin CODEX_COMPANION_SESSION_ID

Surfaced by the local review of #45 and deliberately left out of that PR rather than widening its scope.

buildEnv (tests/fake-codex-fixture.mjs:901-914) builds the child environment for fixture-based tests. It pins PATH, CLAUDE_PLUGIN_DATA and CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS, but spreads process.env without overriding CODEX_COMPANION_SESSION_ID — so every buildEnv-based test still inherits the host's session id.

Not currently broken. Verified by running the full suite with a synthetic host value (CODEX_COMPANION_SESSION_ID=probe-host-session) → 96/96. The buildEnv tests survive because they either pass an explicit job reference (which bypasses the filter at job-control.mjs:303) or assert on output that is unaffected by session filtering.

Why it still matters. It is the same class #45 just closed, one layer down: a future test that adds a bare status/result call (no job id) on top of buildEnv reintroduces the exact ambient-dependent failure — green in CI, red inside a Claude Code session. The invariant "a test's session scope is pinned by the test, never inherited from the host" is currently enforced per-call-site rather than by the shared helper.

Fix plan: pin an explicit CODEX_COMPANION_SESSION_ID inside buildEnv (a fixture-scoped constant, e.g. "sess-fixture"), then drop the now-redundant per-test overrides where they exist. Note this is not the rejected "scrub the variable" approach — scrubbing makes filterJobsForCurrentSession a no-op and deletes coverage of its active branch; pinning keeps both branches under test.

Test plan: add a bare status (no job id) assertion over a buildEnv fixture whose job carries the pinned id — red pre-fix when the host exports a different session id, green after. Re-run the full suite with a synthetic host value to confirm no regression.

Also outstanding: installed plugin not updated to the merged code

The marketplace entry still pins the pre-#41 release. ~/Projects/plugin-marketplace/.claude-plugin/marketplace.json, plugin codex-fork:

{ "source": "git-subdir",
  "url": "https://github.com/axisrow/codex-plugin-cc.git",
  "path": "plugins/codex",
  "ref": "main",
  "sha": "a0e1c118125850e21b3f6e73e532feda6abc22f7" }

pinned at version 1.0.6-fork.5, while the repo manifests are at 1.0.6-fork.6 and main now carries the merged #41 (c33cf49), #44 (3aad7f0) and #45 (ed9b4e3). Consistent with the known url-sourced version-drift trap: the installed version resolves from marketplace.json, so bumping the upstream repo alone leaves the install untouched. Whether to ship this as fork.7 is a release decision, not part of this issue — noted here only so it isn't lost. Note also the broker caveat from the live-smoke item above: an already-running broker keeps its old opt-out list until restarted, so any post-update verification must kill the broker first.