Repository navigation
feat(health): a Codex observer feeds the core's runtime observation [HealthStatus] - #5140
Conversation
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review of head ba126dd7 (COMMENT from the shared qingyun-wu login; the decisive verdict on this login is the owner's).
Verdict: no code blockers. I would approve once the branch is rebased onto main and CI has run green at the new head. Not merge-ready as it stands.
Blockers (state, not code)
- The PR is
CONFLICTINGand no CI has run at the head.gh api .../commits/ba126dd7/check-runsreturnstotal_count: 0; onlylicense/clais reported. The parent #5086 merged asaa232ec3, which is not the6e8bdf32this branch carries (git merge-base --is-ancestor aa232ec3 ba126dd7fails), and #5139 then landed on top.git merge-tree origin/main ba126dd7conflicts insrc/health_snapshot.py,docs/health-snapshot.mdandtests/health-snapshot.test.py, all parent-layer files. The child commit itself applies cleanly:git cherry-pick ba126dd7ontoorigin/main(b638ca38) had no conflicts. Rebasing only that commit should be enough.
What I verified (on the child commit cherry-picked onto origin/main b638ca38)
node --test tests/codex-observer.test.mjs: 14 pass.tests/codex-core-launcher.test.py -k observer: 3 OK. With main'sstart-cli.shand the PR's tests:FAILED (failures=1, errors=2), the same three names as the body's "before". I confirmed that before/after evidence.- Full
codex-core-launcher.test.py: 61 run, 2 fail.test_unassigned_notifier_task_bypasses_slow_history_scanis the wall-clock flake the body names.test_launches_codex_and_managed_task_notifier("managed core-input monitor did not start") also fails with main's ownstart-cli.shon this host, so it is environmental and not caused by this PR. Note: these tests must run in a clean env (env -i PATH HOME TMPDIR). An inherited poolSUTANDO_INSTANCE_ID/SUTANDO_TMUX_SESSIONmakes them fail spuriously. health-snapshot.test.py92 OK,runtime-observation.test.py13 OK,src-map.test.py27 passed.- Mutants I ran myself, each caught:
task_completealways healthy (1 fail), usage-lesstoken_countas success (1 fail), dropping theparent_thread_idfilter (2 fail), taking the first of several top-level rollouts (2 fail). - Record contract: a
buildRecordrecord piped to main'sruntime_observation.py write, thenload(ws, "core"), returns{phase: idle, motion: idle, condition: healthy, observer: codex-observer}with no rejection reason. The observer's output is accepted by the writer that is now on main. - Hardcoded paths:
scripts/review-checks.sh --diffon the child diff passes. The positive control (a/Users/someone/xline) fails as expected. The only/tmp/sutando-tmux.sockliteral is pre-existing. - Pool workers can't reach
ensure_codex_observer.codex/cli/start-cli.sh:19refuses anySUTANDO_INSTANCE_ID, and workers launch throughlaunch-codex-worker-session.sh. So "core seat only" holds. - Tmux session enumerators (
core_heartbeat.py:220,health-check.py:12514,session-launch.sh:41) gate on a core process or only clear an env pin. A<session>-observersession runningnodeis treated the same way as the existing-watchersession.
Interaction with main since the branch point (lesson 2)
#5139 (now on main) drops a cli_wedge retry-loop claim whenever the observation phase is in RETRY_DISPROVING_PHASES = {idle, waiting, failed} (health_snapshot.py:55,292). With this PR, a Codex core's idle now feeds that rule. I traced the cases. A mid-request retry stays in requesting because task_started was seen. A tail replay that starts mid-turn stays unknown, since tool lines are ignored unless motion is moving. So I found no path where the observer says idle during an in-flight request. Please confirm this in the rebased PR body, because it's a new consumer that the original design didn't have.
Live path
This touches startup/restart/stop. Body section 3 is a real launcher round trip (start-cli.sh --runtime codex on a private socket: launch, --restart with a new observer_id writing after the next prompt, stop-core.sh leaving no observer). That is the right kind of witness. I did not re-run it, since it needs a live Codex TUI. It was taken before the rebase, but the child layer doesn't change in a rebase, so I don't think it needs re-running.
Nits (non-blocking)
discover()caches a failedreadMetaas{path}forever (codex-observer.mjs,this.metas.set(p, {...(await readMeta(p)), path: p})), andpickRolloutcounts that entry as top-level. If a subagent rollout is caught while its multi-hundred-KBsession_metaline is still partly written, the core reports no record for as long as that file stays open. I reproduced it with stub deps: pass 1target null, then the meta becomes readable withparent_thread_id, and pass 2 is stilltarget nullwithreadMetacalled only twice. This fails safe ("no opinion"), but the cure is cheap: don't cache anullmeta. Themetasmap also never evicts closed paths, which is minor for a long-lived core.- The header comment of
codex-observer.mjsis 4 lines including the usage line. CLAUDE.md caps comments at 2.prose-caponly scans.py, so CI won't flag it. - The 15 s heartbeat keeps a
requesting · movingrecord fresh indefinitely during a stalled request. That's harmless, because condition resets tounknownattask_started, abnormal still wins, and supersede only acts on a newer success. It could be worth one sentence in the blind-spots paragraph.
Not verified
- I didn't run Codex live, so the rollout event names (
task_started,token_count.info,task_complete,turn_aborted) are taken from the body and the fixtures, not observed. - I didn't measure
lsof/pscost (the body also lists this as unmeasured). - I didn't check Linux
lsof -Fnoutput parity.
Merge gate
The PR needs two approvals from accounts with write access, plus code-owner review: /src/agent/codex/ → @sonichi / @john-the-dev / @keweichen, and /src/ (for src/agent/start-cli.sh, src/agent/stop-core.sh) → @sonichi / @qingyun-wu. @sonichi covers both paths. There are no reviews on record yet.
The Codex launcher starts codex-observer.mjs in its own <session>-observer tmux session. It finds the rollout file the core's Codex process holds open (lsof over the pane's process tree; exactly one top-level file, else no record), follows it, and writes the core's record through runtime_observation.py with the interpreter scripts/python-binary.sh resolves. task_started reads requesting/moving, a tool call tool, each token_count stamps last_success_at and healthy, task_complete idle. The rollout records no failures, so the record is never abnormal. The app-server daemon is not used: a core launched with --search (as the launcher does) does not register on it. The observer exits once the core session is gone, is replaced when its script changes, and is killed with the notifier on restart, runtime switch and stop. Co-Authored-By: Claude <noreply@anthropic.com>
ba126dd to
4c57010
Compare
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Reviewed as a src/agent/codex/ code owner. Read the full diff, not just the description.
Design checks out: codex-observer.mjs is genuinely read-only (lsof/ps/tmux list-panes for discovery, tails+follows the rollout file by byte offset) and writes only to runtime_observation.py's separate HealthStatus record -- never touches the core session or Codex process itself. pickRollout correctly refuses an opinion when more than one top-level rollout is open (no parent_thread_id/agent_path), rather than guessing. State machine (lineChange/apply) is honest about its own limits: a turn that ends without a completed response reads 'unknown', never fabricated as healthy or abnormal, and the rollout's lack of failure records is explicitly called out in both the code comment and docs rather than glossed over.
Lifecycle wiring mirrors ensure_task_notifier's existing pattern closely: version-cksum check via a tmux env var, kill-and-restart on a stale version, and the new observer session is correctly added to every kill path (--restart, foreign-session replacement, stop-core.sh) so it can't be orphaned.
Test coverage is solid: pure-function unit tests cover the real edge cases (half-written lines held back, large-rollout tail-read skipping a cut first line, a failing write never throwing, a gone core session stopping the loop), plus launcher integration tests for all three lifecycle paths (start beside core, leave current version running, replace a stale one, kill on restart).
CI green (30/30). No concerns.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Review of head 60353b54 (authored commit 4c570103, merged with main f7b69fc8; 0 commits behind).
Verdict: REQUEST_CHANGES. There are two defects in the observer's discovery path. I reproduced both against the real writer (src/runtime_observation.py), and I confirmed them independently of the Codex-host report. Under ordinary core activity, either one can leave HealthStatus holding a stale record, or no record, for as long as the observer has been running. The design itself (rollout file over the app-server daemon, never claiming abnormal, the lifecycle wiring) looks right to me.
Blocker 1: reselecting a rollout resets seq but keeps observer_id, so the writer rejects every later record
src/agent/codex/cli/codex-observer.mjs:128:select()doesthis.state = newState(), which setsseqback to 0.this.ident.observerId(:106) is created once per process and doesn't change.src/runtime_observation.py:104:_supersededrejectsnew.seq < old.seqfrom the sameobserver_id.main()turns that into exit 0 with "stale record ignored", andflush()swallows it.
Reselection happens any time discovery returns a different target: the core opens a new thread, or a single discovery returns nothing (see blocker 2). For the same file, the tail replay (TAIL_BYTES = 256 KB) recomputes a seq bounded by the tail, but the stored seq keeps growing with everything the observer has followed since it started. So on a long-lived core, every record after a reselect loses.
Repro 1, writer only:
write(old):idle seq=15, returns True.write(new): sameobserver_id,requesting seq=1, returns False.load()still returnsidle 15.- Passing that record to
health_snapshot._supersedewith a livecli_wedgeretry-loopopinion givesopinion: None, superseded_by: observation, becauseidleis inRETRY_DISPROVING_PHASES(src/health_snapshot.py:55,:293). For up to the 45 s lease, a stale idle record hides a genuine retry loop. After that the record expires and the observer stays silent until itsseqcatches up.
Repro 2, the Observer class with fake deps and the real writer, switching /A to /B:
sent seq=30 ... | stored seq=30 hb=1791000030
--- core starts a fresh thread: /A closed, /B opened
sent seq=3 hb=1791000038 | stored seq=30 hb=1791000030
sent seq=3 hb=1791000053 | stored seq=30 hb=1791000030
sent seq=3 hb=1791000083 | stored seq=30 hb=1791000030
Fix: keep seq monotonic across select() (carry it over instead of resetting it), or mint a new observer_id/observer_started_at per selection. Add a regression test that goes through runtime_observation.write (and ideally health_snapshot), not only the fake write.
Blocker 2: discovery throws away valid lsof output whenever a listed pid has exited
codex-observer.mjs:205:run()resolvesnullon any non-zero exit.:249-251:openRolloutspassesps's pid list tolsof -a -p. On macOS,lsofexits 1 when any pid in that list is gone, and it still prints the live pids' files:rc_with_dead_pid=1 (still printed p<pid>/fcwd…) rc_live_only=0- The core's tool calls spawn short-lived children, so a ps→lsof race is routine during a turn. When it happens,
openRolloutsreturns[],select(null)drops the target, and the next discovery reselects, which triggers blocker 1.
Real repro: real fileDeps and real lsof. A shell holds rollout-…jsonl open while spawning a 50 ms child in a loop:
openRollouts returned [] in 21/40 calls while the held rollout stayed open.- With the real writer and a 600 KB rollout, 16 of 30 discoveries dropped the target. Each reselect replayed
seq=332against a stored341/344, and those writes were rejected.
Fix: treat lsof's partial-result exit (status 1 with stdout) as usable. Keep timeout, signal and maxBuffer errors as failures; don't trust truncated output. Add a subprocess regression test with a child that exits between ps and lsof.
Non-blocking
:117: a single failed or timed-outtmux list-panes(panePid0) makestick()return false, andmain()exits for good. Only a launcher run (ensure_codex_observer) brings the observer back. Consider requiring the session to be gone on two consecutive discoveries, or checkinghas-sessionexplicitly.- Scope note: the observer can't detect stalls. A turn with no rollout events for an hour still reads
requesting · moving/healthy. That's by design here (screen-derived sources own stalls). The docs paragraph should say it in one line so nobody readsmovingas liveness.
What I verified
Scrubbed env (env -i HOME=<scratch> PATH=$PATH), scratch workspace, fresh clone:
head 60353b54 |
main f7b69fc8 |
|
|---|---|---|
node --test tests/codex-observer.test.mjs |
14/14 | n/a |
tests/codex-core-launcher.test.py |
61 OK | 58 OK |
tests/health-snapshot.test.py |
92 OK | 92 OK |
tests/runtime-observation.test.py |
13 OK | 13 OK |
bash scripts/review-checks.sh --diff <(gh pr diff 5140), run from the PR head: PASS.- CI at this head: 31 pass, 1 skipping (Windows).
- The Codex-host verification by Rui's Codex agent matches: node 14/14, codex-core-launcher 61/61, health-snapshot 92, runtime-observation 13. That agent found blockers 1 and 2 first; I reproduced both independently.
- Neither defect is covered by the new tests: the
fake()deps never reselect after a write, and they never return a partiallsof.
I didn't run a live Codex core. The live checks are listed for the Codex-host verifier.
…d probe is not a loss - select() keeps seq across a reselect. runtime_observation refuses a lower seq from the same observer_id, so a reset made every record after a new thread (or a dropped target) lose until seq caught up. - lsof exits 1 when a listed pid has exited but still prints the live pids' files; that output now counts. A timeout, signal or overflow still does not. - A rollout missing from one discovery keeps the target, and the core session counts as gone (observer exits) only on the second miss in a row. - docs: the record does not see a stall; a silent turn still reads moving. Co-Authored-By: Claude <noreply@anthropic.com>
|
@john-the-dev thanks. Both blockers were real, and they're fixed in Blocker 1,
Blocker 2,
Non-blocking items, also done.
Live check. I ran a real Codex core with the core's flags and the observer at Local results. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at 9eaa92b. Both blockers from my earlier review are fixed, which supersedes my CHANGES_REQUESTED at 60353b5.
- Seq after reselect:
select()now carriesseqforward. Switching from rollout A to B writes seq 33 against a stored 30 and is accepted (it was 3 against 30 and rejected before). Switching to a mid-turn rollout now replaces the staleidle seq 15withrequesting seq 16, and the retry-loop opinion is no longer suppressed. - Partial lsof: with real lsof against a holder that spawns children,
openRolloutsdropped the target in 0 of 40 calls (it was 21 of 40). End to end with the real writer, 0 of 30 discoveries were lost and seq never lagged. A fake lsof confirmed the guard: exit 1 with output is kept; exit 1 with no output, exit 2, a timeout, SIGKILL and more than 4 MB of output are all rejected. - Regressions: the new tests fail 4 of 17 against the old behaviour and pass 17 of 17 here. Reverting each fix alone fails exactly its matching test.
- Suites, run with a scrubbed env: codex-observer 17, codex-core-launcher 61, health-snapshot 92 and runtime-observation 13. review-checks PASS.
- Codex-core verification (Rui's Codex agent, at this head): it reproduced both fixes independently with the real Observer, writer and HealthStatus reducer. On reselection, 60353b5 gave seq 15 -> 1 (stored idle, retry-loop lost) and this head gives 15 -> 16 (stored requesting, retry-loop kept). With real lsof and an exited child,
openRolloutsreturns count 0 at 60353b5 and 1 here. It ran the same four suites green on a Codex host.
Not merge-ready on CI yet. Python shard 2 fails at external-core-helpers.test.py:238, where the global heartbeat/input-watch logs changed. That is the flake #5181 fixes, and this diff doesn't touch it. watch-tasks-stream-interrupted-task-retry also failed in that run. Both pass locally at this head. CI needs a re-run, or #5181 to land, before merge.
Non-blocking: no test covers the timeout/signal guard on the lsof partial path. Removing !err.killed && !err.signal still passes 17 of 17, so it's checked only by the fake-lsof run above.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested on 9eaa92bc52aca0d13a17194aceb7b42f3d8c9b7a. I verified three merge-blocking behaviors.
1. A quota-failed turn is recorded as a fresh success
codex-observer.mjs:33-40 treats every token_count with truthy info as success, then ignores task_complete.error. info is cumulative session usage, not per-turn success evidence. In Codex's official source, a usage-limit failure updates rate limits and emits TokenCount before returning the error (handler, emission from existing session state). TokenCount and TurnComplete are durable while the separate Error event is not (rollout policy); the durable failure signal is TurnComplete.error (construction).
I replayed prior successful turn -> task_started -> quota TokenCount carrying prior info -> task_complete{error} through lineChange/apply. HEAD ended with:
{"phase":"idle","motion":"idle","condition":"healthy","lastSuccessAt":5,"turnSucceeded":true}That false lastSuccessAt then satisfies health_snapshot._supersede and can erase the newer quota-limit diagnosis. Please derive success from durable completion evidence, honor an error-bearing completion, and add the prior-success-then-429 regression through the health reduction.
2. A refused in-core runtime switch kills the live core's helpers first
The generic dispatcher converts Codex -> Claude into --restart, then kills the watcher and new observer at src/agent/start-cli.sh:110-120. The Claude launcher does not apply the inherited-core restart guard until claude/cli/start-cli.sh:250-255.
In an isolated fake-tmux harness with a live Codex runtime, requested Claude runtime, and inherited SUTANDO_CORE_SESSION=1, the target correctly refused with exit 42 and did not kill the core, but the dispatcher log already contained:
kill-session -t =sutando-core-watcher
kill-session -t =sutando-core-observer
The caller gets a refusal while the still-running core loses task intake and health observation. Apply the shared restart guard in the generic policy owner after deciding the cross-runtime restart but before either helper mutation. Please pin the refusal with zero watcher/observer/core kills, plus unmarked and explicit-override controls.
3. The metadata cache is permanent, unbounded, and caches failed reads as fact
discover() stores every full session_meta payload forever, although only the parent/agent classification is needed. A deterministic 100-rollout harness with the same 600 KB metadata size already used by this PR's test retained:
{"cachedEntries":100,"openRollouts":1,"retainedBytes":60007570}It also stores a transient parse miss as {path} and never retries it. With one main rollout plus a subagent whose first metadata read returned null, repairing the subagent metadata on the next discovery still produced targetAfterRepair:null, readMetaCalls:2, and cached {path:"/sub"}. For an always-on observer, prune closed paths, retain only the classification fields, and retry absent/invalid metadata; add both regressions.
Required test and repository-rule fixes
tests/codex-observer.test.mjs:170invokes barepython3despite receiving a resolvedpy; that can select the macOS developer-tools stub. Resolve it through the repository helper and execute that path (REVIEW criterion 7).- The new launcher test inherits
SUTANDO_TMUX_SOCKETthroughdict(os.environ)but asserts the default socket. Under the actual core environment,python3 tests/codex-core-launcher.test.py -k observer -vran 3 tests and failed the new test at line 660 because it observed the configured socket; the scrubbed run passed 3/3. Pin or remove that variable in the fixture. - Non-blocking cleanup: the new module's opening comment spans four physical lines; the repository rule caps code comments at two.
Evidence and readiness
I reviewed the cumulative merge-base diff and the 60353b54..9eaa92bc fix layer. The earlier sequence-reset and partial-lsof findings are fixed. At this exact head, 210 focused tests passed in a scrubbed environment: observer 17, launcher 61, health snapshot 92, runtime observation 13, and src-map 27. git diff --check and the review checks also passed. The current required Python aggregate is red from the separately tracked external-helper /tmp race, so I am not attributing that failure to this patch.
Worst case without these fixes: a refused runtime switch silently strands a live core without intake, while a real quota failure is simultaneously published as healthy and suppresses the correct quota diagnosis; long-lived observers also retain rollout metadata without bound. This head is not ready to merge. A re-review can approve once the three behaviors and the two required test issues above are fixed and the current required checks are green.
… error; the dispatcher refuses before reaping - A model output (assistant message, reasoning, tool call) is the success evidence. token_count is not: Codex writes one from running totals before returning a usage-limit failure, which read a failed turn as a fresh success. - task_complete.error makes the record abnormal (failed · idle) with codex_error_info mapped to needs-login, quota-limit or api-error; it stands until a later model output, and a new turn alone does not clear it. - Discovery keeps only the open rollouts' parent/agent classification, prunes closed ones, and retries a session_meta it could not read. - src/agent/start-cli.sh applies the shared in-session restart guard before it reaps the watcher and observer, so a refused Codex -> Claude switch no longer strips a live core of task intake and observation. - Tests: the real-writer test resolves python through scripts/python-binary.sh; the launcher fixture drops an inherited SUTANDO_TMUX_SOCKET/SESSION. - The module's opening comment is two lines. Co-Authored-By: Claude <noreply@anthropic.com>
|
@keweichen thanks. All three behaviours, both test issues and the comment cleanup are fixed in 1. A quota-failed turn is no longer a success.
2. The dispatcher refuses before it reaps anything.
3. Metadata cache.
Test and repo-rule fixes.
Mutants, each caught by a named test:
Local results.
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 9a88224091dff970b03cd70d37953bfcd9733fbe. I verified that the earlier sequence, partial-lsof, quota-classification, cache-bounding, restart-safety, interpreter-resolution, and inherited-socket findings are fixed on their ordinary paths. The current head still has three runtime/state blockers and one required test-path fix.
1. Blocker: unreadable rollout metadata is treated as proof that the rollout is irrelevant
discover() retries an unreadable session_meta, but omits that open path from kept and passes null to pickRollout. With one known top-level rollout and one persistently unreadable sibling, the unknown sibling is therefore treated as if it were known to be a subagent. The known rollout remains selected, misses resets to zero, and its old state keeps receiving fresh heartbeats forever.
My deterministic current-head control held /old at idle/healthy and returned null for /new on every metadata read. After three discoveries and heartbeats:
{
"target": "/old",
"cached": ["/old"],
"writes": [
{"phase":"idle","condition":"healthy","heartbeat_at":1791342000,"seq":3},
{"phase":"idle","condition":"healthy","heartbeat_at":1791342015,"seq":3},
{"phase":"idle","condition":"healthy","heartbeat_at":1791342030,"seq":3},
{"phase":"idle","condition":"healthy","heartbeat_at":1791342045,"seq":3}
]
}The new test currently pins the unsafe assumption directly: “an unreadable sibling is not a second top-level rollout”. That contradicts the documented contract, “one top-level file, else no record”: unreadable means unknown, not subagent. The stale idle phase can also suppress a real cli_wedge retry loop through _supersede.
Please count any unreadable open rollout as ambiguity while still retrying its metadata. Retaining the current target for one transient miss is reasonable; a second ambiguous discovery must stop refreshing/drop it. Add persistent-unreadable, repaired-as-subagent, and repaired-as-second-top-level regressions.
2. Blocker: the initial tail can consume bytes past the offset it records, then lose a completion
readTail stats the file for the returned offset, then readRange stats it again and reads through the newer size. If Codex appends between those snapshots, the observer consumes the append but records the older offset. follow() rereads it; when the append was a partial JSON line, rest + reread duplicates the prefix, parsing fails, and that event is skipped permanently.
I forced exactly that interleaving with a split quota completion:
{
"after_select": {"initial_bytes":198,"actual_bytes":245,"offset":198,"rest_bytes":47,"phase":"requesting","condition":"healthy"},
"after_follow": {"actual_bytes":341,"offset":341,"rest_bytes":0,"phase":"requesting","motion":"moving","condition":"healthy","reason":null}
}The completed task_complete{error: usage_limit_exceeded} should have produced failed/idle/abnormal/quota-limit; it was lost. Please read from one fd/snapshot or return the actual consumed end offset, and add an append-between-snapshot-and-read regression proving a split completion is applied exactly once.
3. Blocker: the safe dispatcher refusal bypasses Claude's durable restart audit
The new guard correctly runs before helper reaping, but src/agent/start-cli.sh:116-135 exits before the Claude launcher can execute its established log_restart_attempt contract. In the same isolated canonical-dispatcher harness:
merge-base: rc=1 log_exists=yes refusal_count=1
HEAD: rc=1 log_exists=no refusal_count=0
This affects same-runtime Claude --restart/--force-restart and the Codex-to-Claude switch that synthesizes --restart. Preserve the pre-mutation refusal without dropping its durable audit, and pin the canonical dispatcher for explicit restart, force-restart, and synthesized cross-runtime restart—not only stderr and zero kills.
4. Required test fix: the observer suite cannot run from a path containing spaces
tests/codex-observer.test.mjs:16-19 uses URL .pathname, leaving a space encoded as %20. From an exact-head worktree named sutando pr5140..., the suite failed before collecting its 21 tests:
rc=1
.../sutando%20pr5140.../scripts/python-binary.sh: No such file or directory
tests 1, pass 0, fail 1
This host's canonical checkout itself lives below Application Support, so the failure is not hypothetical. Use fileURLToPath(new URL('..', import.meta.url)) and rerun the suite from a spaced checkout.
Verification and readiness
At this head, the ordinary-path suites passed: observer 21/21, full Codex launcher 61/61, health snapshot 92/92, runtime observation 13/13, dispatcher guard 3/3, direct runtime guard, and src-map 27/27. The cumulative merge-base diff passes git diff --check and review-checks.sh; a hardcoded /Users/... positive control fails as expected. Required CI and CLA are green. The original gap is real: the merge base has no Codex observer and explicitly documents the Codex runtime as having no observation record.
Those good paths do not cover the failures above. The worst case is a stale old rollout renewing idle/healthy indefinitely and suppressing a real retry loop, while the initial-tail race can permanently drop the very failure completion meant to correct that record. Separately, a safely refused restart is now absent from the audit used to diagnose failed restart attempts.
This head is not ready to merge. After the fixes move the head, please provide a fresh real post-restart task/health round trip at that exact head; the existing live witnesses predate 9a882240, which changes both observer semantics and startup dispatch. Please also update the PR body, which still says token_count is success evidence and that the observer never reports abnormal.
Non-blocking cleanup: tests/start-cli-dispatcher-restart-guard.test.sh:2-5 opens with a four-line comment block, above the repository's two-line cap.
…d is one snapshot; refusals stay audited - pickRollout treats an open rollout whose session_meta cannot be read as possibly top-level: ambiguous, so the target is kept for one discovery and dropped on the next instead of renewing an old record forever. - readTail/readFrom read through one fd with one fstat and return the end of the bytes actually read, so a write landing during the initial tail is followed exactly once instead of duplicated into an unparseable line. - The restart-attempts audit writer moves into restart-guard.sh; the dispatcher's pre-reap refusal logs through it, as the Claude launcher does. - tests: fileURLToPath for the repo path (checkouts with spaces); dispatcher test covers synthesized, explicit and force restarts and their audit line. Co-Authored-By: Claude <noreply@anthropic.com>
|
@keweichen thanks. All four are fixed in 1. An unreadable rollout is treated as ambiguous.
2. The tail read uses one snapshot.
3. Refusals stay audited.
4. Paths with spaces.
Cleanup. The dispatcher test's header is two lines. Live round trip at
Full output is in the PR body. The scratch run's overall PR body rewritten for the new rules: model-output success, Local results.
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at c6b678c.
Every finding from keweichen's two CRs (at 9eaa92b and 9a88224) is fixed, and each fix has a regression test. Run against the head's tests, 8/24 observer tests fail on 9eaa92b and 4/24 fail on 9a88224. The dispatcher refusal cases fail on both older heads.
Mutants at head are all caught: token_count counted as success, the completion error ignored, an unreadable rollout taken for a subagent, a failed metadata read cached, closed paths not pruned, the old double-snapshot readTail, and helpers reaped before the guard.
Suites passed in a scrubbed env:
- codex-observer 24/24
- codex-core-launcher 61/61
- health-snapshot 92/92
- runtime-observation 13/13
- dispatcher guard 5/5
review-checks passes and CI is green.
Rui's Codex-core agent verified this head independently on a macOS Codex host:
- A persistently unreadable sibling drops the target on the second discovery, and heartbeats stop renewing the record.
- A split usage_limit_exceeded completion during the initial tail read applies once, as failed/abnormal/quota-limit.
- Prior success followed by quota: the full health_snapshot keeps both the observer failure and the pane quota claim, with no superseded_by. In the positive control, a later model output does supersede the claim.
- Dispatcher refusal with an inherited SUTANDO_CORE_SESSION=1, on a private tmux fixture: exits 1, sessions are unchanged, and exactly one audit line is written.
Not independently verified: the dispatcher refusal and the post-restart task/health round trip on an authenticated live core. Both rely on Mark's exact-head witness (#5140 (comment)).
Non-blocking: the /health regression has no competing pane claim, so the token_count-as-success mutant passes it. Only the unit test on last_success_at pins that case.
…g pane claim A cli_wedge quota-limit claim that began after the real success must stand; counting the failed turn's token_count as a success would supersede it, and this test alone now catches that. Co-Authored-By: Claude <noreply@anthropic.com>
|
@john-the-dev thanks for the approval. I've done the non-blocking item in
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Requesting changes at 10339fc. This replaces my approval at c6b678c because of one silent defect.
Blocker: the observer never starts when run through a symlinked path
codex-observer.mjs:343 calls main() only when import.meta.url === pathToFileURL(process.argv[1]).href. Node resolves import.meta.url to the real path but leaves argv[1] as typed. Both launchers build REPO with plain pwd, which keeps symlinks. So for any checkout reached through a symlink, the observer exits 0 at once and makes no probes, and /health has no runtime observation, exactly as before this PR. That includes anything under /tmp, which is macOS's own symlink to /private/tmp. Nothing errors, and the core itself is unaffected.
I reproduced it with a stub tmux that logs every call:
| How the observer was invoked | Exit | tmux probes |
|---|---|---|
| Real path | still running (alarm) | 1 |
| Real path containing a space | still running (alarm) | 1 |
| Through a directory symlink | 0, immediately | 0 |
Through /tmp/... |
0, immediately | 0 |
Rui's Codex agent reproduced it independently: through a symlink it exited 0 with zero probes, and canonicalizing argv[1] restored the probes.
No test starts the observer as a process, so the entrypoint and main() never run in the suite.
Fix: compare against pathToFileURL(fs.realpathSync(process.argv[1])), or have the launchers use pwd -P. Then add one process-level test that starts the script through a symlink and asserts it probes.
Kewei's points: all resolved in code at this head
- Unreadable rollout is ambiguous. Lines 100-104; discovery keeps the target for one miss, then drops it at lines 163-166. The three regressions exist.
- Single-fd tail read.
readRangeat lines 266-277 uses one fd and returns the real end offset. - Dispatcher refuses before killing helpers. It writes one shared audit entry (
restart-guard.sh:15-23,start-cli.sh:116-127). The guard test fails 5/5 at the merge base and passes 5/5 at head. token_countis not success. An error completion records failed/abnormal.- Bounded metadata cache.
- Spaces in paths. The suite passes 24/24 from a directory with a space in its name.
Non-blocking test gaps
Each of these mutations survived the suite:
stop-core.shno longer kills the observer.- The main-loop try/catch is removed.
fileDeps.writeis passed a different--workspace.- Replacing a non-Codex session leaves the observer running.
Worth closing in the same push if it's convenient.
Also
The live post-restart round trip in the body was run at c6b678c. Runtime code is unchanged since then, because 10339fc is test-only, but the symlink fix will change runtime code, so please run it again at the new head.
…op, writer and kill paths - The entrypoint check resolves argv[1] with realpath before comparing it to import.meta.url (which Node resolves), so a checkout reached through a symlink such as /tmp no longer exits 0 without probing. - The main loop is runLoop(observer, sleep): a throwing tick is survived, a gone core ends it. - Tests: the observer started as a process through a symlinked directory probes the core session; isEntrypoint; runLoop; fileDeps.write's argv and stdin; stop-core kills the observer; the direct Codex launcher's foreign-session replacement kills the observer. Co-Authored-By: Claude <noreply@anthropic.com>
|
@john-the-dev thanks. That was a real one. Fixed in Blocker: symlinked entrypoint.
Non-blocking gaps, each mutant now killed:
Live round trip at
Full output is in the PR body. Local results.
|
…dex-observer # Conflicts: # src/agent/codex/cli/start-cli.sh # tests/codex-core-launcher.test.py
|
@cla-assistant check |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
APPROVE at exact head 75fdcf97ac34bfe83eac5102559c7970c4edc3ba. This supersedes my CHANGES_REQUESTED review at 9a882240.
I re-read the full activated observer, writer, health-merge, launcher/dispatcher, restart, and stop paths against base c7da6002, including the merge conflict that produced this head. The conflict resolution preserves ensure_codex_observer inside start_core_session_and_helpers() for both launcher branches.
The earlier blockers are fixed and discriminated:
- Unreadable open-rollout metadata is ambiguity, with one-discovery grace and then target drop. The three targeted tests pass; restoring the old “ignore unreadable” rule makes all three fail.
- Tail/follow reads now use one fd/size snapshot and advance to the bytes actually read. Restoring the old two-snapshot behavior makes the mid-read split-quota test fail (
requesting/unknowninstead offailed/abnormal/quota-limit). - The dispatcher audits synthesized, explicit, and force-restart refusals before reaping helpers. Removing the audit call makes those three cases fail.
- The entrypoint resolves the invoked path. The process-level symlink test passes; restoring the old URL comparison makes it time out without ever probing tmux.
Current-head local evidence:
node --test tests/codex-observer.test.mjs: 28/28 pass, with the new tests collected by name.python3 tests/codex-core-launcher.test.py: 72/72 pass.- Dispatcher guard: 5/5; stop-core: all cases pass.
- Health snapshot: 92/92; runtime observation: 13/13; src-map: 27/27.
git diff --checkandscripts/review-checks.sh: pass. GitHub CI and CLA are green at this head.
For the live-path gate, the author’s real launcher round trip at e447969a covers launch through the /tmp symlink, task → observation → /health, restart with a new observer and second task, in-session refusal/audit, and stop cleanup. I did not repeat that quota-consuming live run at the merge commit; I verified the only launcher conflict here was the main-branch session-wait helper extraction above, then ran the expanded 72-test launcher suite on the merge result.
Worst-case disruption is a wrong or costly health observation from the always-on ps/lsof poller. It is bounded to reporting: the observer is read-only, performs no recovery/restart, treats ambiguity as no opinion, swallows probe/write failures, and its record expires after 45 seconds without renewal. The unmeasured busy-host polling cost remains disclosed and is non-blocking.
Verdict: APPROVE — the code is ready at this head. This does not override the merge gate: john-the-dev still has an outstanding CHANGES_REQUESTED review on an older head, which must be replaced or dismissed before merge readiness is evaluated.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-review at 75fdcf97 by Chi's Sutando agent. Every standing change request is fixed and I found no new blocker. This is a COMMENT, not an approval, because one item is missing: a live core round trip at this head (REVIEW.md lesson 15).
john-the-dev's CR at 10339fc: fixed. I checked this by running the observer.
isEntrypoint now compares pathToFileURL(realpathSync(argv1)) against import.meta.url (codex-observer.mjs:350-358). I started the observer with a stub tmux:
| How it was started | 75fdcf9 | 10339fc |
|---|---|---|
| real path | runs | runs |
| through a directory symlink | runs | exits 0 without running |
via /tmp |
runs | exits 0 without running |
The new test at tests/codex-observer.test.mjs:344 covers this case.
keweichen's CRs at 9eaa92b and 9a88224: fixed. I checked these by reading the code at head.
token_countno longer counts as success, and an error completion is applied.- An unreadable rollout counts as ambiguous and is dropped on the second miss.
- The metadata cache is bounded.
- The tail is read from a single fd.
- The restart guard runs before helpers are killed.
- The tests use
fileURLToPathand resolve the repo's Python.
The merge of main is resolved correctly. Against main's parent it only adds lines, so nothing from main was lost. In start-cli.sh, the two ensure_codex_observer calls became one call inside start_core_session_and_helpers(), which covers both branches.
Tests run at head:
| Suite | Result |
|---|---|
| codex-observer | 28/28 |
| codex-core-launcher | 72 OK |
| restart-guard | 5 PASS |
| stop-core | all pass |
| health-snapshot | 92 OK |
| runtime-observation | 13 OK |
review-checks.sh passes. CI shows 33 green and 1 skipped, and CLA is green.
Still missing: a live round trip at this head. Mark's live run (launch via /tmp, --restart, stop-core) was at e447969, before the merge restructured the Codex start path. One post-merge live run would close this, and I'd approve after that.
Not run by me: a live Codex core.
|
@sonichi here is the live round trip at exactly Setup:
I updated the PR body's live section to this run. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at 75fdcf97. This replaces my CHANGES_REQUESTED at 10339fcf. The owner (Rui) has accepted the existing evidence for the live gate.
My CR is fixed.
isEntrypoint()resolvesargv[1]withrealpathSyncbefore comparing it withimport.meta.url(codex-observer.mjs:350-358).- The process-level test at
tests/codex-observer.test.mjs:344launches through a symlinked dir. Putting the old comparison back makes it fail. - A real launch via
/tmpkeeps running and probes tmux.
Evidence relied on:
- Live run (Mark's): launch via
/tmp,--restartwith a newobserver_id, a refused in-session switch with its audit line, andstop-core, all ate447969a. That's the commit carrying the fix.75fdcf97only adds a hand-resolved merge of main. Three independent diffs (mine, Chi's agent's, Rui's Codex core's) found that merge equals main plus the observer hunks only, with the twoensure_codex_observercalls folded intostart_core_session_and_helpers(). - Rui's Codex core at this head, without a live core: codex-core-launcher 72/72 at head against 68/68 on main on the same host, observer 28/28, restart-guard 5/5, and stop-core passing. The real observer, launched through a directory symlink against an isolated tmux session, exited 0 with no stderr after the session was removed. The old entrypoint check reproduces the silent exit.
- Others at this head: keweichen re-reviewed and approved. Chi's agent re-reviewed (COMMENT) and found every standing CR fixed and no new blocker.
- CI: all 15 required checks are green on their latest runs.
Not verified: an authenticated live launch/restart/attach/stop at 75fdcf97 itself. The owner accepted Mark's run at e447969a in its place.
Follow-up to #5086 (merged), which added the runtime observation record; this gives the Codex core an observer. Single commit,
4c570103, on current main.What changed, and why
#5086 lets a seat's observer publish a runtime observation record, and HealthStatus reads it. Only Claude seats had an observer.
docs/health-snapshot.mdsays a Codex core has no record, so its health comes only from the screen.This adds
src/agent/codex/cli/codex-observer.mjs, a small zero-dependency Node process that writes the Codex core's record. It changes nothing in Codex.ps), and lists the rollout files those processes hold open (lsof -Fn). The core's file is the one whosesession_metanames no parent thread. It needs exactly one such file, and every open rollout's metadata must be readable (an unreadable one could be a second top-level thread). Otherwise it keeps its target for one discovery, then writes no record, which/healthreads as "no opinion". Only the open files' parent/agent classification is cached, and an unreadablesession_metais retried.task_startedreadsrequesting · moving;response_itemtool call readstool, and its output readsrequestingagain;message,reasoningor a tool call) stampslast_success_atandhealthy.token_countis not success evidence, because Codex writes one from the session's running totals before a usage-limit failure;task_completereadsidle · idle;task_completewith anerrorreadsfailed · idle · abnormal, withcodex_error_infomapped toneeds-login(unauthorized),quota-limit(usage_limit_exceeded,session_budget_exceeded) orapi-error(anything else). The failure stands until a later model output; a new turn alone does not clear it;turn_abortedreads idle.moving, so stalls stay with the screen-derived sources. Approval waits aren't in the rollout either.python3 src/runtime_observation.py write, using the interpreterscripts/python-binary.shresolves, the same as claude-observer. No interpreter means no write. The record is seatcore, session = the core's tmux session, observercodex-observer. Writes are debounced, there's a 15 s heartbeat inside the 45 s lease, and write errors are swallowed.src/agent/codex/cli/start-cli.shaddsensure_codex_observernext to eachensure_task_notifier. It starts a<session>-observertmux session and stamps it withSUTANDO_OBSERVER_VERSION, a checksum of the script, so a changed script replaces it, the same pattern as the notifier.--restart, replacing a non-Codex session,src/agent/start-cli.sh's non-Codex runtime switch, andsrc/agent/stop-core.sh.src/agent/start-cli.shapplies the shared in-session restart guard before it reaps the watcher and observer. A refused Codex → Claude switch (or--restart/--force-restartfrom inside the core) no longer leaves the live core without task intake, and the refusal is still written tologs/restart-attempts.log. The audit writer now lives inrestart-guard.sh, shared with the Claude launcher.Why not the app-server daemon (the first design). Two things showed up live (codex-cli 0.160.0, daemon 0.160.1).
The daemon never sees the core. Launched with
--search, whichCODEX_ARGSalways passes, the TUI does not register on the shared daemon. I tested one flag at a time and counted new threads inthread/loaded/list:--sandbox danger-full-access--ask-for-approval never--add-dir $HOME-m <model>--search--searchA
-c key=valueoverride also skips the daemon.The daemon refuses to subscribe to a thread before its first turn (
-32600 no rollout found).The rollout file is written in both modes, so it is the one source that sees the core.
Where to look first
src/agent/codex/cli/codex-observer.mjs:lineChange,pickRollout, andObserver.discover/select/feed.src/agent/codex/cli/start-cli.sh:ensure_codex_observerand the added kills.docs/health-snapshot.md, "Runtime observation": the new Codex core paragraph.Before / after
1. The launcher starts, keeps, replaces and kills the observer. The new launcher tests at the parent (
6e8bdf32) and at HEAD:2. Observer against a live Codex TUI with the core's exact flags (
-C <dir> --add-dir $HOME --sandbox danger-full-access --ask-for-approval never --search --no-alt-screen). Prompt:Run the shell command: sleep 6; cat hello.txt. The record, sampled every 1.5 s (heartbeat age, phase, motion, condition, seq, age of last success):/health?view=full(src/health_snapshot.py) for that core:observationsource{'phase': 'idle', 'observer': 'codex-observer', ..., 'last_success_age_s': 23.3}, opinionidle / healthy. After the core's tmux session was killed, the observer exited within 10 s.3. Live path, real launcher. I ran
bash src/agent/start-cli.sh --runtime codex --no-schedule-reconcilewith a private socket, sessionobs-e2eand a scratch workspace, so the live core wasn't touched.obs-e2e,obs-e2e-watcher,obs-e2e-observer. After one prompt, the record is{"phase": "idle", "motion": "idle", "condition": "healthy", "session": "obs-e2e", "seq": 3, ...}./healthshowssession: obs-e2eand the observation opinionidle / healthy.--restart: a new observer (a differentobserver_id, started after the restart) writesidle healthywithsucc_age 11.8after the next prompt.src/agent/stop-core.sh:no server running, and nocodex-observer.mjsprocess is left.hung. That came from the supervisor (core alive but stalled (status stale, no recognized prompt)), because the scratch workspace has nocore-status.json. It's unrelated to this change, andhungis never superseded.Tests
At head
e447969a:node --test tests/codex-observer.test.mjspython3 tests/codex-core-launcher.test.py-k observeralso passes with an inheritedSUTANDO_TMUX_SOCKET/SESSIONbash tests/start-cli-dispatcher-restart-guard.test.shbash tests/start-cli-restart-inside-core-guard.test.sh,tests/start-cli-restart-verify.test.py,tests/graceful-restart.test.shpython3 tests/health-snapshot.test.py/runtime-observation.test.py/src-map.test.pyshellcheck -S warning,review-checks.shMutants that the tests catch:
token_countcounted as success; a user message counted as success;seqreset on reselect;lsofoutput dropped;Live round trip at
75fdcf97(the post-merge head). This is the real launcher, run through/tmp/..., the/private/tmpsymlink. Private socket, scratch workspace.Not verified / not in this PR
task_complete.error) is seen. A retry that later succeeds, or an error that never completes a turn, still reaches/healthonly through the screen (cli_wedge, the supervisor).--ask-for-approval never, so none are expected.lsofandpsrun every 10 s; I didn't measure their cost on a busy machine. Reading the file runs every 1 s and is astatplus the new bytes.