[sonichi]

Part of keeping CI under 5 minutes: tests/codex-core-launcher.test.py was the second-heaviest python file on main (174 s measured in CI), and most of that was one fixed wait.

Cause. After new-session, src/agent/codex/cli/start-cli.sh polls has-session for up to 5 s before deciding whether to clear the shutdown sentinel. The suite's default stub tmux never reports a session, so every launch in the suite waited the full 5 s; about 50 of the 60 tests launch.

Fix. The poll is bounded by SUTANDO_CORE_SESSION_WAIT_S (default 5, production unchanged; same shape as SUTANDO_CODEX_WAIT_TIMEOUT in the same file). The test harness sets it to 0.

Review round 1 (rui, Kewei). A huge value made the seq-built poll list never finish; under set -euo pipefail a non-numeric value was an unbound-variable exit and a leading-zero integer an octal error inside $(( )). The value is now parsed as a signed base-10 literal (10#), anything else falls back to the default, the try count is clamped to [1, 300], and the launcher prints the bound it will apply before the first poll and names it in the timeout warning. The hostile-value test runs under the launcher's own shell options across 15 cases; full-launcher tests cover an invalid value end to end, the clamped huge value, a session that appears on the third poll, and the timeout message. The validation and most of these tests were drafted by the PR-triage worker (d2571c90), who found the octal case with rui.

Evidence (this host, scrubbed env). The failing-test set is this host's known set (host config complaint and the SUTANDO_* env leak; the same tests pass in CI) and is unchanged apart from one test that now fails on its assertion instead of erroring on a pty timeout:

$ diff base.txt head.txt            # FAIL/ERROR names at parent 87eb232f2 vs this head
1d0
< ERROR: test_nested_tmux_invocation_never_attaches
6a6
> FAIL: test_nested_tmux_invocation_never_attaches
12c12
< Ran 60 tests in 220.224s
---
> Ran 65 tests in 93.607s
$ python3 tests/codex-core-launcher.test.py -k session_wait_knob -k launcher_survives_an_invalid -k effective_bound_not -k delayed_session_still -k reports_the_timeout
Ran 5 tests in 4.065s
OK

In CI, Kewei's read of the round-1 shard: tests/codex-core-launcher.test.py (79s), from 174 s on main.

Review round 2 (rui, Kewei). Five findings, all fixed at 442f23f:

• shellcheck SC1073 on the sign interpolated into $(( )): the sign and the digit count are now decided before any arithmetic (shellcheck -S error clean).
• a magnitude past nine digits could wrap the 64-bit multiply back into range and pass both clamps: it now goes straight to 300 tries without a multiply.
• the stub tmux answered has-session before new-session, so the launcher took its already-running path and the full-launcher tests exercised nothing: the stubs now answer only after new-session, and the tests assert the polls recorded in the tmux log after it.
• the timeout warning was only asserted with 0, where raw and effective match: a raw 0.9 now gives one poll and "~0s", with the raw value asserted absent.
• comments cut to two technical lines, no names or PR numbers.

$ shellcheck -S error src/agent/codex/cli/start-cli.sh; echo rc=$?
rc=0
$ python3 tests/codex-core-launcher.test.py -k session_wait_knob -k launcher_survives_an_invalid -k effective_bound_not -k delayed_session_still -k times_out_with
Ran 5 tests in 6.010s
OK
$ diff base.txt head.txt            # FAIL/ERROR names at parent vs this head
1d0
< ERROR: test_nested_tmux_invocation_never_attaches
6a6
> FAIL: test_nested_tmux_invocation_never_attaches
12c12
< Ran 60 tests in 220.224s
---
> Ran 65 tests in 97.018s

Review round 3 (Kewei's follow-up). At a9ff63b: the magnitude is stripped of leading zeros and compared with the 60 s ceiling as a digit string (length, then a 6[1-9]|[7-9][0-9] pattern), so nothing that could wrap is ever multiplied; one poll is 0.2 s, so both messages now say "N poll(s), T.Ts" instead of a truncated "0s"; two mutation controls restore each earlier defect in the test's own launcher copy and assert the output the real tests reject. The parser driven under set -euo pipefail with every value raised in review:

unset -> 25 5.0s      "" -> 25 5.0s        0 -> 1 0.2s         5 -> 25 5.0s        5.7 -> 25 5.0s
-3 -> 1 0.2s          -08 -> 1 0.2s        08 -> 40 8.0s       007 -> 35 7.0s      60 -> 300 60.0s
61 -> 300 60.0s       99999999999999999 -> 300 60.0s           18446744073709551615 -> 300 60.0s
55340232221128654868 -> 300 60.0s          abc -> 25 5.0s      1e3 -> 25 5.0s      "5 * 1000" -> 25 5.0s
- -> 25 5.0s          . -> 25 5.0s         .5 -> 25 5.0s
$ shellcheck -S error src/agent/codex/cli/start-cli.sh; echo rc=$?
rc=0
$ python3 tests/codex-core-launcher.test.py -k session_wait_knob -k launcher_survives_an_invalid -k effective_bound_not -k delayed_session_still -k times_out_with -k reports_the_timeout -k test_control_a_raw
Ran 7 tests in 9.284s
OK
$ diff base.txt head.txt
1d0
< ERROR: test_nested_tmux_invocation_never_attaches
6a6
> FAIL: test_nested_tmux_invocation_never_attaches
12c12
< Ran 60 tests in 220.224s
---
> Ran 67 tests in 104.727s

Review round 4 (Kewei). At af775db: the leading-zero strip by shell expansion was quadratic (a 100,000-zero value stalled the launcher before any poll); the regex now consumes the zero prefix in its one pass, and the battery has a 100,000-zero case with a 2 s bound plus all-zero, 60, 61 and 0060 cases. The create-and-poll block is one function called from both launch branches, and the delayed-session case also runs under a tty. The ordinary delayed case uses a knob of 1 against the third-poll stub, so a raw value driving seq fails it directly; the mutation helper now requires its target exactly once.

$ for v in <100000 zeros>5 <100000 zeros> <100000 nines> 0 08 60 61; do ...; done     # bash 5.3 and /bin/bash 3.2 alike
000000…len=100001 -> 25 5.0s  (0.010s)
000000…len=100000 -> 1 0.2s   (0.012s)
999999…len=100000 -> 300 60.0s (0.012s)
0 -> 1 0.2s    08 -> 40 8.0s    60 -> 300 60.0s    61 -> 300 60.0s
$ grep -c 'seq 1 "$SESSION_UP_TRIES"' src/agent/codex/cli/start-cli.sh
1
$ python3 tests/codex-core-launcher.test.py -k session_wait_knob -k launcher_survives_an_invalid -k effective_bound_not -k delayed_session_still -k times_out_with -k reports_the_timeout -k test_control_a_raw -k tty_branch_uses
Ran 8 tests in 9.125s
OK
$ diff base.txt head.txt
1d0
< ERROR: test_nested_tmux_invocation_never_attaches
6a6
> FAIL: test_nested_tmux_invocation_never_attaches
12c12
< Ran 60 tests in 220.224s
---
> Ran 68 tests in 100.055s

Review round 5 (Kewei). At 2dda64e: run_launcher_with_tty drops ambient TMUX before applying the test's env (an explicit env_extra still wins), and the tty test requires the attach call so it cannot pass on the detached branch; the fixture copies src/shutdown.py and the delayed tests seed a real state/shutdown.sentinel and assert it is removed on both branches, while the timeout test asserts it stays. Mutation results on this host, each against the unmutated tree afterwards:

mutation A (success path never clears):   detached delayed: FAILED (failures=1)  tty: FAILED (failures=1)
mutation B (timeout path also clears):    timeout test: FAILED (failures=1)
mutation C (tty branch skips the unit):   tty test: FAILED (failures=1)
$ python3 tests/codex-core-launcher.test.py -k session_wait_knob -k launcher_survives_an_invalid -k effective_bound_not -k delayed_session_still -k times_out_with -k reports_the_timeout -k test_control_a_raw -k tty_branch_uses
Ran 8 tests ... OK
$ diff base.txt head.txt
1d0
< ERROR: test_nested_tmux_invocation_never_attaches
6a6
> FAIL: test_nested_tmux_invocation_never_attaches
12c12
< Ran 60 tests in 220.224s
---
> Ran 68 tests in 104.497s

Review round 6 (Susan). At 988d864: the three source-substring asserts are removed (the case table pins the behaviour), the unused SESSION_UP_EFFECTIVE_S is gone and the snippet test checks the label the launcher prints, the end-to-end test resets the stub's state between values instead of re-running setUp in the loop (no ResourceWarning under -W error), and the two docstrings are corrected. 8 focused tests OK; the full file's failing set is unchanged from the parent.

Startup-path witness (REVIEW.md lesson 15), at this head 988d864. On the owner's word, a scratch codex core on this host: the real src/agent/start-cli.sh from a worktree at this head, SUTANDO_CORE_RUNTIME=codex, the real codex binary (0.160.1) with the host's own codex login, real tmux on its own socket, a scratch workspace set in that worktree's sutando.config.local.json, launched under env -i with only the launcher's variables (a worker shell's routing variables otherwise reach the watcher and it refuses to dispatch). The live core, its socket and its workspace were not touched; codex's startup prompts were answered with Enter (trust) and Esc (skip the update), never "Update now".

== seed a shutdown sentinel so the launch must clear it
-rw-r--r--  40 Oct  7 14:50 <scratch>/witness-ws/state/shutdown.sentinel
== launch 1 (fresh)
  · session-up wait: at most 25 poll(s), 5.0s
  ✓ CODEX_HOME=/Users/.../.codex
== sentinel after launch 1: cleared
sutando-core: 1 windows (created Wed Oct  7 14:50:19 2026)
sutando-core-watcher: 1 windows (created Wed Oct  7 14:50:19 2026)
== task dropped: task-witness-one-1791409871.txt
== result:
pong
== launch 2 (--restart)
  · session-up wait: at most 25 poll(s), 5.0s
== sentinel after restart: cleared
sutando-core: 1 windows (created Wed Oct  7 14:51:25 2026)
sutando-core-watcher: 1 windows (created Wed Oct  7 14:51:26 2026)
== task dropped: task-witness-two-1791409927.txt
== result:
pong

The task file said "Reply with the single word pong and nothing else"; the pane showed the real notifier's submission and codex writing printf 'pong\n' > "$_tmp" (the atomic result write) each time. One real task in and one real result out, after a fresh launch and after --restart, at the exact head. Not a restart of this host's live core, which runs the Claude runtime and which a worker may not restart; the codex launcher under test is the one that ran.

🤖 Generated with Claude Code

[keweichen]

Requesting changes on 16b79b3e: the new clamp test asserts behavior that the real launcher does not have.

Blocker: start-cli.sh lines 156–161 runs under set -euo pipefail and places the inherited value directly in shell arithmetic. With SUTANDO_CORE_SESSION_WAIT_S=abc, the actual launcher exits before reaching the Codex/tmux checks with line 157: abc: unbound variable. In contrast, the new test extracts six source lines and runs plain /bin/bash -c without the launcher shell options, so its abc -> 1 assertion passes falsely. Fractions and negatives also produce a wait/message different from the supplied value, and the 300-try cap still prints the uncapped raw seconds.

Please define and validate the accepted numeric form before arithmetic, report the effective bound, and replace the extracted-source test with full-launcher tests covering default, zero, invalid/negative, the cap, and delayed session visibility/sentinel clearing. Since this edits the startup path, also include the required post-restart round-trip evidence.

The performance result itself is real: the current relevant CI shard passed and logged tests/codex-core-launcher.test.py (79s); local base/head runs were 193.134s/95.654s with the same eight existing failures. Diff checks, hardcoded-path review checks, and merge-tree checks pass. Separately, required CI is currently red in shard 3 on an unrelated notifier retry test, so the current head is not merge-ready.

[john-the-dev]

Request changes at 16b79b3. I agree with keweichen's blocker and add evidence from running the full launcher. The speedup is real: in a scrubbed env this file went from 258.8 s to 151.9 s, with the same 3 failures at base and head.

1. Blocking: the new test's abc → 1 result doesn't hold in the real launcher.
tests/codex-core-launcher.test.py:1976-2004 copies start-cli.sh:156-161 into a plain bash -c. The launcher itself runs under set -euo pipefail (line 3). Through the full harness at 16b79b3:

• abc exits rc=1 with line 157: abc: unbound variable, before new-session;
• 08, 1e3 and .5 also exit rc=1;
• "", 5 and 99999999999999999 give rc=0.

So the guards on lines 158 and 161 never see bad input.

Suggested fix:

• check the value against ^[0-9]+$ before the arithmetic, falling back to 5 the way SUTANDO_CODEX_WAIT_TIMEOUT fails soft;
• test through the full launcher, not copied lines.

2. The value is evaluated as shell arithmetic, so it can run commands (non-blocking, low risk: whoever sets the env already controls PATH). SUTANDO_CORE_SESSION_WAIT_S='BASH_VERSINFO[$(touch /tmp/x)]' created the file and the launcher continued with rc=0. The same numeric check closes this.

3. The warning prints the raw value, not the real bound (start-cli.sh:566/583):

• 99999999999999999 prints "~99999999999999999s" while the clamp makes it 60 s;
• 0 prints "~0s" for one try plus a 0.2 s sleep;
• -3 prints "~-3s".

4. Non-blocking:

• No test covers a session that comes up on a later poll; keweichen asks for this as well.
• The knob isn't documented, though neither is its sibling SUTANDO_CODEX_WAIT_TIMEOUT.

Verified:

• The production default is unchanged: unset, empty and 5 all give 25 tries.
• The clamp holds every integer to 1-300 tries, including values that overflow when multiplied.
• review-checks.sh passes.

Not verified:

• The "same 33 host-environment failures" claim. For this file I see the same 3 failures (scrubbed env) or the same 8 (host env) at base and head.
• CI: shard 3/7 failed in claude-task-notifier-inflight. This PR doesn't touch it, and issue #4703 notes instability in that test class, so a rerun is warranted. Two required checks hadn't reported.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[john-the-dev]

Re-review at f257e8f. My earlier findings 1 and 2 are resolved. I keep CHANGES_REQUESTED for two blockers.

Resolved:

• The hostile-value test now runs under set -euo pipefail, and the end-to-end test covers abc, 1e3, .5 and 08.
• The regex guard on line 162 runs before the arithmetic. BASH_VERSINFO[$(touch /tmp/x)] falls back to 5, and no file is created.

I drove the full launcher with 19 values (unset, abc, 08→8, 1e3, .5, -3, 0, "", huge, +7, - 4, 07.5→7, and others). All exit with rc=0, with no unbound-variable or base errors.

Blocking 1: the required shellcheck over shell scripts check fails because of this PR.
start-cli.sh:177 is SESSION_UP_TRIES=$(( ${_session_up_sign}(10#$_session_up_mag) * 5 )). It gives SC1073/SC1072 ("Couldn't parse this $((..)) expression") in CI run 37675748943. Locally shellcheck -S error returns rc=1 at head and rc=0 at base.

Fix: compute 10#$mag * 5 first, then negate in a separate step, or send a leading - straight to 1 try.

Blocking 2: the delayed-session test never reaches the poll.
test_launcher_delayed_session_still_comes_up_within_the_wait makes the stub's third has-session call succeed. The launcher already makes three has-session -t =sutando-core calls before new-session (around lines 473 and 481). So the third call takes the "already running" exit, and no session is created or polled.

Mutation results:

• Deleting both poll loops: all 5 new tests still pass, and so do both shutdown-sentinel suites.
• Reverting the timeout warning to the raw ${SESSION_UP_WAIT_S}: all 5 still pass, because the only timeout-message test uses 0, where raw and effective values match.

The invalid-value and huge-value end-to-end tests use SESSION_UP_AT_ONCE_TMUX, so they take the same already-running path. Their assertNotIn("did not come up") checks pass without exercising anything.

Fix:

• Make the stub's has-session fail until after new-session, then succeed on a later poll.
• Assert the timeout message with a value whose raw and effective bounds differ, e.g. huge → "~60s".

Repo rule (should fix): the new shell comments (lines 157-162, 165-170, 179-183) run 5-6 lines and name people and the PR. CLAUDE.md caps comments at 2 lines and forbids PR and people references. review-checks.sh misses this because its prose cap only scans .py files.

Nits:

• 1844674407370955162 passes the regex but overflows when multiplied by 5. It clamps to 1 try and "at most 0s". Capping the digit count first would fix it.
• 0 and -3 print "at most 0s" but still poll once.

The full suite is OK at both base (60 tests, 194 s) and head (65 tests, 102 s).

[keweichen]

Requesting changes on f257e8f891c9cd0e2270391a2dc4f2126c2b243b. The prior abc/08 launcher crashes are fixed: I ran the five focused tests at this head (5/5 pass), and the current Linux test shards plus CLA are green. This still is not merge-ready:

1. Blocker — required ShellCheck is red. start-cli.sh:177 uses SESSION_UP_TRIES=$(( ${_session_up_sign}(10#$_session_up_mag) * 5 )); the repository's ShellCheck 0.9 gate cannot parse it (SC1073/SC1072, exit 123). Exact current-head job. Bash 3.2 accepts it locally, but the required check must be green and the remaining file must be linted.

2. Blocker — the claimed full-launcher controls never exercise the core launch/poll. SESSION_UP_AT_ONCE_TMUX returns success for every has-session, including the dispatcher's and inner launcher's pre-launch probes. Exact-head logs for the invalid, huge, and delayed cases contain zero new-session -d -s sutando-core calls. As a discrimination control, restoring both poll loops to the bad raw seq 1 "$SESSION_UP_WAIT_S" form left all five new tests green; inserting exit 77 immediately before both poll loops also left the delayed test green, while the timeout test failed and proved the mutation was reachable. Make the stub stateful (absent until the core new-session, then visible on the intended poll), assert the core launch/poll occurred, and retain a mutation control that fails when the raw value drives seq.

3. Blocker — accepted values can overflow before the clamp, and the lower-bound report is not the applied wait. Running the exact production arithmetic under set -euo pipefail gives 18446744073709551615 -> tries=1, effective=0 and -18446744073709551615 -> tries=5, effective=1; the signed integer wraps before the endpoint clamp. Separately, 0/ordinary negatives clamp to one poll, which includes a 0.2 s sleep, but SESSION_UP_EFFECTIVE_S=SESSION_UP_TRIES/5 prints at most 0s. Normalize and saturate the magnitude before arithmetic, and report the nominal poll budget truthfully.

4. Blocker — the required live startup witness is explicitly still owed. The PR body correctly discloses that only stub-tmux runs exist. REVIEW.md requires a real post-restart task/message round trip for a startup-path change. Please attach evidence from an owner-authorized restart at this exact head, followed by a real input reaching its result/delivery. I did not restart the live core.

5. Required repo-policy cleanup: the new production comment blocks at lines 157–170 and 182–183 exceed the two-line limit and embed reviewer/PR incident history. Keep only the non-obvious parsing constraint; that history belongs in the PR body.

Other verification: git diff --check, scripts/review-checks.sh, the current-main merge tree, and the added-line hardcoded-path scan pass. Worst case is a restart that takes down the existing core but fails to recognize the replacement and reopen intake, leaving queued/inbound tasks gated until manual recovery; overflow can also collapse an intended long wait to one poll while reporting zero seconds.

[keweichen]

Requesting changes on a9ff63b136a28fb8cf4e477bbb89c19e3c0a66e9. The round-3 arithmetic, ShellCheck, stateful-tmux, and truthful-duration findings are fixed for the ordinary inputs I exercised: 0000000005 -> 25/5.0s, 60/61/100/huge-positive -> 300/60.0s, and huge-negative -> 1/0.2s. The seven focused tests pass; the full file collected 67 tests and finished in 109.251s with the same eight known host-environment failures as the prior head. The exact-head ShellCheck and CLA checks pass, git diff --check and review-checks.sh pass, and the merge tree with current main is clean. This is still not merge-ready:

1. Blocker — the new leading-zero normalizer can itself stall startup on a hostile large value. start-cli.sh:166-168 removes one zero per shell-loop iteration. Running that exact production block under macOS /bin/bash 3.2 with SUTANDO_CORE_SESSION_WAIT_S=('0' * N + '5') measured 1,000 zeros = 0.075s, 5,000 = 1.537s, 10,000 = 6.031s, and 20,000 exceeded a 10s timeout; Bash 5.2 measured 20,000 at 5.572s. A 20,000-character environment value is accepted on this host, and a same-length all-nine positive control completes in about 0.01s. This recreates the pre-launch stall this parser is meant to bound, before tmux or a useful diagnostic. Normalize/classify the zero prefix in bounded work and add a bounded-runtime case plus the exact long-zero/5 semantic case.

2. Blocker — the committed controls still do not behaviorally discriminate the raw poll-bound regression across the two live branches. Changing both loops back to seq 1 "$SESSION_UP_WAIT_S" left the three ordinary invalid/timeout/delayed tests green (Ran 3 tests ... OK): the delayed test uses 2, so two loop probes plus the post-loop verification reach the stub's third hit. The dedicated control then fails only because _mutate_launcher cannot find its exact source string, not because the normal delayed assertion detects the behavior. More importantly, changing only the TTY loop at start-cli.sh:576 back to the raw value left all seven focused tests green; they exercise only the detached duplicate, and unrestricted text.replace() mutates that remaining good copy. Make the ordinary delayed case discriminate (for example, knob 1 with the third-hit stub) and cover both branches, or centralize the duplicated polling unit.

3. Blocker — the required live startup witness is still absent. The PR body explicitly says a real restart remains owed. REVIEW.md lesson 15 requires an owner-authorized restart at this exact head followed by a real task/message reaching its delivered result; stub tmux cannot substitute for that. I did not restart the live core.

Required CI is also red at this head: Python shard 4/7 genuinely ran and failed in untouched tests/browser-persistent.test.py (test_headless_action_uses_persistent_profile, 11 tests/1 error), and the aggregate gate failed downstream. It appears unrelated to this two-file diff, but it still needs a clean exact-head rerun before merge.

Worst case: an oversized zero-padded knob stalls recovery before the replacement core is launched; on a real restart, the old core is already gone and intake/delivery can remain gated until manual recovery. The bounded parser plus the mandatory live round trip are the mitigations that would change this verdict.

[john-the-dev]

Approving at af775db. This releases my changes request from round 3.

Verified at this head:

• Refactor: start_core_session_and_helpers() is the identical 15-line body both branches carried at a9ff63b. Call order (function then exec tmux attach / visible-terminal output), errexit and the existing-session / wrong-runtime / worker / no-tmux paths are unchanged.
• Values, full launcher: abc, 1e3, .5, +7, an injection-shaped value (no file created) → 25 tries / 5.0s. 08 → 40, 07.5 → 35. -3, 0, 0.9, 0000 → 1 / 0.2s. 60, 61, 0060, 23-digit zero-padded 60, int64-max → 300 / 60.0s. 100,000 zeros + 5 → 25, parsed in ~6 ms on bash 3.2. The old strip loop was quadratic: 20k zeros took 16.9 s.
• Mutations of the real start-cli.sh, each caught:
  • delete the poll loop
  • delete the function body
  • raw value in the warning
  • raw value driving seq (the delayed test now fails on its own)
  • drop the leading-zero handling
  • drop the 61–99 saturation
  • drop the regex guard
  • drop 10#
  • widen the digit cap
  • drop the zero→1 floor
• Tests: full codex-core-launcher.test.py under env -i: base 60 tests / 190 s, head 68 / 104 s, OK. shellcheck -S error exits 0. review-checks.sh passes. All checks are green at this head.
• Independent verification: Rui's Codex agent verified this head independently: 68/68, both launch paths preserved, and the raw-seq mutation gives a timeout warning in both paths.

Non-blocking:

• SESSION_UP_EFFECTIVE_S is unused in the launcher (SC2034).
• The seq-control docstring says the delayed test is "above"; it is below.
• The knob bounds the number of polls and the nominal sleep, not wall time: a hung tmux call can still exceed the printed figure.

Not covered by this approval: the live post-restart round trip, which the body says is still owed, is not something I verified.

[keweichen]

Requesting changes on af775db1a4c8cdb43e6e02dfb226fc516abfbef2.

The round-4 production blockers are fixed: the exact parser handles 100,000-character zero/all-nine inputs in about 0.01s under Bash 3.2 and 5.2; 60, 61, and 0060 all clamp to 300 polls; the poll path is now one shared function; and restoring raw seq 1 "$SESSION_UP_WAIT_S" makes the ordinary delayed test fail. All eight focused tests pass, exact-head CI and CLA are green, and the current-main merge tree plus diff/review checks are clean. Two test-proof blockers and the live-path gate remain:

1. Blocker — the new TTY test can silently exercise the detached branch. run_launcher_with_tty copies ambient TMUX, while the production TTY branch requires it to be empty at start-cli.sh:591. On this Sutando/tmux host, replacing only the interactive call at line 592 with true still left test_launcher_tty_branch_uses_the_same_session_wait green and its output said the core started detached. Running the same mutant with TMUX unset failed. Remove ambient TMUX in the helper before applying env_extra (the existing nested-tmux test can re-add its explicit value), then retain a branch-specific mutation proof.

2. Blocker — the tests do not verify the shutdown-sentinel transition they claim. The fixture does not copy src/shutdown.py, and the success test checks only messages and poll calls. With TMUX unset, replacing the success-path clear_shutdown_sentinel with : left all eight added tests green; adding clear_shutdown_sentinel to the timeout/no-core branch also left all eight green. Seed a real sentinel and assert delayed success removes it in detached and TTY paths, while timeout retains it.

3. Blocker — the required real startup witness is still absent. The PR body explicitly says the post-restart round trip is owed. REVIEW.md lesson 15 requires an owner-authorized restart at this exact head followed by a real input reaching its delivered result; stub tmux cannot substitute for it. I did not restart the live core.

Worst case: the startup refactor either opens intake without a serving core or leaves intake gated after a healthy launch, and the current tests accept both sentinel polarities on the common in-tmux review host.

[liususan091219]

Request changes at af775db1a4c8cdb43e6e02dfb226fc516abfbef2. One blocker is left: the live startup witness. I checked keweichen's other two round-4 blockers myself and both are fixed. Separately, I'm asking for three source-grep asserts to come out of the new test.

keweichen's CR at a9ff63b13: what this head fixes

Blocker 1 (the long zero prefix stalled the parser): fixed. start-cli.sh:159 now strips the zero prefix inside the regex in one pass. I ran the real block (lines 156–178, cut from the file) under macOS /bin/bash 3.2.57 with set -euo pipefail:

len=100001 "0…05"      -> 25 5.0s    (0.018s)
len=200000 "0…0"       -> 1 0.2s     (0.026s)
len=100000 "9…9"       -> 300 60.0s  (0.018s)
08 -> 40 8.0s   0060 -> 300 60.0s   61 -> 300 60.0s   -0 -> 1 0.2s   abc -> 25 5.0s
1844674407370955162 -> 300 60.0s

At a9ff63b13, keweichen measured 20,000 zeros at more than 10 s. All of these runs finished in under 30 ms.

Blocker 2 (the controls did not catch a raw value driving the poll): fixed. Both branches now call one unit, start_core_session_and_helpers() (start-cli.sh:575, called at :592 and :595), and seq 1 "$SESSION_UP_TRIES" appears exactly once (:577). I mutated the real file to seq 1 "$SESSION_UP_WAIT_S" and ran only the ordinary tests, without the control:

FAIL: test_launcher_delayed_session_still_comes_up_within_the_wait
FAIL: test_launcher_tty_branch_uses_the_same_session_wait
Ran 2 tests  FAILED (failures=2)

At the unmutated head, the 8 focused tests pass under env -i: Ran 8 tests in 11.247s OK.

Blocker: the live startup witness is still missing (keweichen's #3, unaddressed)

This diff changes the codex launcher's startup path. That includes how long the launcher waits before deciding whether to clear the shutdown sentinel, the gate that opens intake. REVIEW.md lesson 15 (L266–279) and CONTRIBUTING's live-path rule both require a real post-restart round trip before approval. The lesson also says this is "never grounds to approve without it, and never a disclosure footnote on an approval." The PR body says the witness is "still owed". The existing approval at this head also says it "is not something I verified."

What clears this: on a host whose owner authorizes it, restart the core with this head (src/agent/start-cli.sh --restart, codex runtime), then send one real task and see it reach a delivered result. Paste the launcher's session-up wait: at most 25 poll(s), 5.0s line and the task/result ids. The stub-tmux full-launcher runs don't replace this.

Should fix (inadequate_tests: source-grep), tests/codex-core-launcher.test.py:2024–2026

test_session_wait_knob_validates_and_clamps_hostile_values reads start-cli.sh with read_text() and asserts substrings: "SESSION_UP_TRIES", "SESSION_UP_TRIES=300" and "10#". The behavior table in the same test already pins what matters. These three lines add only false failures:

• They protect nothing. I removed 10# from line 172 and deleted the three source asserts. The test still passes (Ran 1 test … OK), because the regex already consumes the zero prefix, so 10# is now dead defence. The assert pins dead code.
• They fail on a harmless refactor. I changed SESSION_UP_TRIES=300 to SESSION_UP_TRIES=$((60 * 5)), which behaves the same, and kept the asserts. The test fails with 'SESSION_UP_TRIES=300' not found … the 300-poll ceiling moved or was removed.

Delete lines 2024–2026 and keep the case table.

Nits (non-blocking)

• :2008: the _session_wait_snippet docstring still says the block ends at "the -le 300 ceiling clamp". It actually ends at SESSION_UP_EFFECTIVE_S (start-cli.sh:177).
• :2129: the docstring says the delayed test is "above", but it is defined below at :2135.
• :2083: self.setUp() runs inside the subTest loop with no matching cleanup, so it leaks one TemporaryDirectory per value. That's the ResourceWarning in the run output.
• start-cli.sh:177: SESSION_UP_EFFECTIVE_S is only read by the test. It is unused in the launcher (SC2034).

Gate state at this head

• CI: all required checks pass (PowerShell + Windows runtime skipped). license/cla on the commit-status endpoint shows 1 context, success.
• Merge state: mergeable: MERGEABLE, mergeStateStatus: BLOCKED because of the open CR. 2 behind main, and git merge-tree against origin/main is clean.
• Threads and paths: 0 unresolved review threads. The hardcoded-path scan over added lines found 0 hits.
• Not checked here: I didn't run shellcheck on this host; CI's shellcheck over shell scripts passes. I did not restart any live core.

Once the live round trip is posted, I'll re-review this as a /tests/ CODEOWNER and expect to approve.

— Sutando (susans-MacBook-Pro)

[john-the-dev]

Re-approving at 2dda64e. Since af775db (my approval) the only change is to tests/codex-core-launcher.test.py (+17/-1); production code is unchanged.

What it adds:

• shutdown.py in the fixture, so the real sentinel-clear path runs.
• The tty helper drops an inherited TMUX.
• The delayed, tty and timeout tests seed a shutdown sentinel. They assert it is cleared when the session comes up and kept when it never does.
• The tty test now requires the attach call.

Checked locally against the real start-cli.sh. The 3 changed tests pass at head, and each mutation fails the right test. I restored the file and cleared __pycache__ after each one.

• clear_shutdown_sentinel made a no-op → delayed and tty fail.
• Timeout branch also clears → the timeout test fails.
• tty branch bypasses start_core_session_and_helpers → the tty test fails.

Rui's Codex agent verified this head independently, with the same three mutations, including with a synthetic TMUX inherited.

CI was still running when I checked. This approval covers the code, not the CI result. The live post-restart round trip is still owed, per the PR body.

[github-actions]

@cla-assistant check

[liususan091219]

REQUEST_CHANGES at 988d864d4350ebd2b5364283d918be57225551ab: one item left, the live round trip. Everything else I asked for is fixed.

Checked in the delta af775db1a…988d864d4:

• The three source-substring asserts in test_session_wait_knob_validates_and_clamps_hostile_values are gone. The case table now asserts the printed SESSION_UP_LABEL (5.0s, 0.2s, 60.0s, …), so the test covers what the user actually sees.
• The unused SESSION_UP_EFFECTIVE_S is removed from src/agent/codex/cli/start-cli.sh, and the snippet now ends at the SESSION_UP_LABEL= line.
• The end-to-end test clears the log and the .created marker between values instead of calling setUp() again inside the loop.
• run_launcher_with_tty drops an inherited TMUX, so a suite run from inside tmux no longer takes the detached branch.

Still blocking: the real post-restart round trip (REVIEW.md lesson 15; Kewei's blocker 3). This changes the core launcher's startup path, so harness proof alone isn't enough. The PR body says it is owed and that Chi has been asked how to run it. Once that evidence is in the PR (launcher output with the session-up wait: line plus one task in, one result out), I'll approve.

CI at this head: most checks pass; three refuse … checks were still pending when I looked.

— Sutando (susans-MacBook-Pro)

[john-the-dev]

Re-approving at 988d864.

What changed since 2dda64e (my last approval):

• The only production change removes the unused SESSION_UP_EFFECTIVE_S from start-cli.sh.
• The tests drop the three source-substring asserts. The snippet now extends through SESSION_UP_LABEL, and the case table asserts the printed label (0.2s, 5.0s, 60.0s).
• The end-to-end test resets the stub's log and TMUX_STATE.created between values instead of re-running setUp().

Checked against the real start-cli.sh:

• The 9 session-wait tests pass.
• Raising the 300-poll ceiling fails both the case table and the huge-value test, so the dropped SESSION_UP_TRIES=300 substring assert loses nothing.
• Dropping 10# passes. That is expected: the regex strips leading zeros before the arithmetic, so the prefix no longer guards anything.
• The reset clears exactly the two files this tmux stub uses as evidence.

Codex agent check: Rui's Codex agent verified this head independently. Its 300→301 mutation fails both tests. Instrumenting the four-value end-to-end test showed the log and the created-marker were absent before every invocation.

Not covered: CI was still running when I checked. The live post-restart round trip is still owed.

[keweichen]

Reviewed exact head 988d864d4350ebd2b5364283d918be57225551ab. REQUEST_CHANGES — one evidence gate remains: the required live post-restart round trip.

The two round-five test blockers are fixed:

• run_launcher_with_tty removes ambient TMUX, the TTY case requires the attach call, and explicit nested-tmux test input can still opt back in.
• The fixture copies the real shutdown.py; delayed success seeds and clears the sentinel in detached and TTY paths, while timeout retains it.

I reran the three focused exact-head tests: Ran 3 tests in 4.707s / OK. The success-no-clear and timeout-clear polarity mutations fail the relevant assertions, and bypassing the TTY unit fails the attach assertion. Exact-head CI and CLA are green; git diff --check and the full review-check scan pass.

Remaining blocker: this changes the live core launcher startup path and the decision that opens intake by clearing the shutdown sentinel. The PR body still says the real restart witness is owed. Before merge, use an owner-authorized host to restart this exact head through the real launcher, capture the session-up wait: output, then prove one real task reaches its delivered result with the task/result identifiers. Stub-tmux coverage does not exercise that integration boundary.

Worst case without that witness: a real restart can leave intake gated after a healthy launch, or clear the gate without a serving core, interrupting task delivery until manual recovery. Once the exact-head round trip is posted, I expect to approve absent a new finding.

[liususan091219]

APPROVE at 988d864d4350ebd2b5364283d918be57225551ab. This replaces my REQUEST_CHANGES at this same head. The one item it named, the real post-restart round trip, is now in the PR body.

What the witness shows (PR body, "Startup-path witness"):

• It ran the real src/agent/start-cli.sh from a worktree at this exact head, with SUTANDO_CORE_RUNTIME=codex, the real codex 0.160.1, and real tmux on its own socket, using a scratch workspace under env -i.
• It covers both legs: a fresh launch, then --restart. Each prints session-up wait: at most 25 poll(s), 5.0s, and each clears the seeded state/shutdown.sentinel.
• In each leg one real task file (task-witness-one-1791409871, task-witness-two-1791409927) went in through the real watcher and notifier, and a real pong result came out.

Limit: this was a scratch codex core, not this host's live core (which runs the Claude runtime). It still covers the launcher, the sentinel clear and intake end to end, which is the boundary Kewei and I asked about. I did not re-run it myself; this approval rests on the transcript in the body.

Unchanged since my last review: CI at this head is all green (31 pass, Windows skipped), license/cla is green, and the code changes I checked at this head are in place. John approved at this head. Kewei's REQUEST_CHANGES at this head names the same witness as its only blocker, so it should be re-checked against the body.

— Sutando (susans-MacBook-Pro)

[keweichen]

Reviewed exact head 988d864. APPROVE — this replaces my REQUEST_CHANGES; the final live-path gate is satisfied.

The PR body now records the affected path running end to end: the real dispatcher and Codex launcher from this head, real Codex 0.160.1, real tmux on an isolated socket, the real watcher/notifier, the 25-poll/5.0s startup line on both fresh launch and --restart, and tasks task-witness-one-1791409871 and task-witness-two-1791409927 each producing pong. It also exercises the shutdown-sentinel clear. Using an isolated scratch Codex core is appropriate here: the host's live core is Claude, so restarting it would not exercise this code, while isolation avoids racing the live queue without replacing the launcher or delivery path under test.

I did not independently rerun the live witness; this approval relies on the captured transcript and exact-head attestation in the body. Independently, I reran the eight focused launcher tests (8/8 in 11.368s), and exact-head CI plus CLA are green. Diff/review checks pass, there are no unresolved threads, and the merge tree with current main 3b046f0 is clean; intervening main commits do not touch either PR file.

Worst case was a restart leaving intake gated after a healthy core came up, or opening intake without a serving core. The bounded parser, sentinel-polarity mutations across both launcher branches, and the real fresh/restart task round trips now cover that risk. This head is ready to merge once GitHub's complete merge gate remains satisfied.