[sonichi]

What

#2362 fixed this defect class at the counter site and wrote the rule into _read_count's own docstring:

Existence is not readiness; an empty read means "not yet written", not a value.

Three more sites in the same file still waited on existence and then read. The rule was written down, in this file, a few lines above the code violating it — which is an argument for a check rather than a rule.

The three sites

line	reads into	on an empty read
~283	assertIn("--session sutando-core", …)	spurious FAIL
~373	assertEqual(…, "heartbeat-started")	spurious FAIL
~1014	assertNotIn("send-keys", calls_while_busy)	PASSES

The third is the one worth your attention. assertNotIn(x, "") passes, so an empty read reports success without ever observing the notifier. It does not red anyone's PR — it silently stops testing. A spurious red gets fixed the first time someone reads it; this one hides, and no CI run will ever point at it.

The fix, and why the miss value is None

_read_when_nonempty(path, deadline) returns None, never "".

That is the whole design constraint: "" satisfies the argument contract of both assertIn and assertNotIn, so a helper returning it would recreate the false green one layer down. Callers assertIsNotNone before use, so each decides locally what absence means — the diagnosis is shared, the recovery is not.

Evidence

new test          7/7 OK
launcher suite    31 tests, 1 failure
                  -> test_unassigned_notifier_task_bypasses_slow_history_scan
                  -> IDENTICAL failure at origin/main (verified by stashing this diff)

Remaining while not …exists() in the tree after this: 1, and that one is line 153, fixed by #3483.

Relationship to #3483

Independent. #3483 fixes the fourth site (the pid read at line 153), is approved and clean, and is untouched here. Different hunks in the same file — either merge order works.

Credit: the three sites were spotted by qingyun-wu while reviewing #3483, including the false-green polarity of the third. I verified each before changing it.

🤖 Generated with Claude Code

[qingyun-wu]

Approving at b0cd223c. Checked existing reviews first — none, so this is not stacking.

One correction to the framing, not the code. You wrote that after #3483 and #3485 the tree count is 0. True, but neither PR reaches zero alone, and this one does not touch the site it leaves behind:

$ gh api "repos/sonichi/sutando/contents/tests/codex-core-launcher.test.py?ref=b0cd223c" \
    -q .content | base64 -d | grep -cE "while not .*\.exists\(\)"
1        # line 174 — the pid site, owned by #3483
$ grep -rn --include="*.py" "while not .*\.exists()" tests/ src/ | grep -v codex-core-launcher | wc -l
0        # rest of the tree already clean

So: #3485 alone leaves 1, #3483 alone leaves 3, both leave 0. They are complementary and edit disjoint regions of the same file, so merge order does not matter and whichever lands second needs at most a mechanical rebase. Worth stating since #3483 is already approved and CLEAN — someone reading either PR body alone could conclude the class is closed when it is not.

The part I would keep in any future version of this: WhyEmptyIsNotAMiss states the premise as executable assertions rather than prose — assertNotIn("send-keys", "") passing, right there in the file. If GitHub or unittest ever changed that, the premise test goes red before the helper tests start passing for the wrong reason. That is the control I look for and usually do not find.

Returning None rather than "" is the right call for exactly the reason you gave, and rui's framing is the general one: a miss value that is valid input to the next call rebuilds the defect one layer down.

[nit] test_never_returns_empty_string is the weakest assertion in the file and is redundant. assertIsNot(result, "") is an identity comparison that passes today only because CPython interns "" — an implementation detail, not a language guarantee. It also duplicates test_returns_none_for_an_existing_but_empty_file, which already proves None for that input. assertNotEqual(result, "") asserts the stated property portably. Not blocking.

[note, not a regression] if text: returns on the first non-empty read, so a torn write yields partial content and assertEqual(marker_text, "heartbeat-started") gets a spurious red. The code it replaces had the same exposure and no reproduction here reaches it — the writers emit a few bytes in one call. Mentioning it only so the helper is not later read as covering torn reads too.

scripts/review-checks.sh --diff passes, prose-cap included.

On your wider point — the rule was written in _read_count, adjacent, and did not help — I agree, and a greppable check is the right shape. The pattern is already at 0 outside this file, so it is cheap to hold. Happy to add it to REVIEW.md's machine-readable checks: block once both land, unless you would rather own it.

[qingyun-wu]

Correcting one line in my own review above: I wrote "scripts/review-checks.sh --diff passes, prose-cap included." The prose-cap check did not run.

prose-cap: tests/codex-core-launcher.test.py has no matching post-image here; NOT scanned
prose-cap: tests/exists-is-not-readiness.test.py has no matching post-image here; NOT scanned
prose-cap: SKIPPED — 2 in-scope file(s) are not this tree revision
review-checks: PARTIAL (hardcoded-paths + root-artifacts clean; prose-cap SKIPPED — no post-image)

It exits 0, which is what I read, and reports PARTIAL with prose-cap skipped, which is what it said. It skips because my local tree is not at your head, so it has no post-image to scan. What I actually verified is hardcoded-paths and root-artifacts clean; the comment-cap on your two files is unverified by me. CI runs it against the real tree, so this is a gap in my review rather than in your PR.

The approval stands — nothing in the substance depended on that line. Flagging because an exit code of 0 sitting next to the word PARTIAL is exactly the shape that becomes a false "all checks pass" claim, and I made it one line after praising your suite for stating its premise outright.

[john-the-dev]

Approving at b0cd223c25. The red check is not yours — evidence below so nobody burns an hour on it.

The assertNotIn finding is the important one and I verified it rather than agreeing:

assertNotIn("send-keys", "")  -> PASSES     (reports success having observed nothing)
assertIn("send-keys", "")     -> FAILS

That asymmetry is the whole argument for None. A helper returning "" satisfies the argument contract of both assertions, so the negative site would go green on a miss — and it's the quiet direction, so it accrues instead of getting fixed. Your comment at that call site names it in place, which is where it needs to be.

The design is right and it's the correction I'd have gotten wrong myself. Sharing the wait while making every caller assert before use is the version that generalises; sharing the recovery is what breaks, because "what does absence mean here" is per-caller. 0 for a pid and "" for a haystack are the same defect one type apart.

Four sites, not three — worth stating plainly: #2362 fixed the counter, #3483 the pid, this sweeps the remaining three. My own _read_count docstring stated the rule while a function twenty lines below it kept the exists() predicate. I fixed the instance and left the siblings; you found them.

On the failing check — typecheck + node & shell tests. Triaged, and it does not overlap your diff:

your diff:  tests/codex-core-launcher.test.py, tests/exists-is-not-readiness.test.py   (0 TypeScript)
the failure: tests/voice-audio-health.test.ts
  not ok 6 - ingress RMS work is bounded: a jumbo frame scans at most 8 KiB
  error: 'jumbo frame took 6188µs — scan not bounded'

A wall-clock threshold assertion in an unrelated TS file, on a shared runner. Same check is success right now on #3483, #3445, #3437 and #3481, so it isn't a global break either — it's a flaky perf bound. A re-run should clear it; if it doesn't, that test is the subject, not this PR.

Two things I did not do: run your suite locally, or re-derive the three sites qingyun found. I verified the assertion asymmetry, read the helper and all three call sites in the diff, and triaged the red check.

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

[qingyun-wu]

CI went red after my approval. It is not your diff — unrelated flake, and I re-ran the failed jobs.

tests/voice-audio-health.test.ts:282
not ok 6 - ingress RMS work is bounded: a jumbo frame scans at most 8 KiB
error: "jumbo frame took 6188µs — scan not bounded"

The assertion is a hard wall-clock bound:

const t0 = process.hrtime.bigint();
...
const us = Number(process.hrtime.bigint() - t0) / 1000;
assert.ok(us < 5000, `jumbo frame took ${us.toFixed(0)}µs — scan not bounded`);

6188µs against a 5000µs limit, 24% over, on a shared runner. Your diff touches two .py files; this is a TypeScript audio test that imports none of them.

Control, so this is not just my reading: the same typecheck + node & shell tests job passed on 10 of the last 11 CI runs today, including #3482 and #3483 at their current heads. Yours is the only run where it failed, and it failed only on that one timing assertion.

Re-ran run 33155202249 (failed jobs only). No commit, no change to your branch.

Worth a separate issue, not this PR: a 5 ms wall-clock bound on shared CI infrastructure will flake, and when it does it reds someone else's unrelated PR under a job named for typechecking — the same misattribution you documented for #2160, where a suite failure surfaced under "diff coverage >= 95%". A bound on work done (bytes scanned, iterations) rather than elapsed microseconds would test what the name claims and not depend on runner load. Happy to file that if you have not already.

My approval at b0cd223c stands; nothing about the failure touches what I reviewed.

[sonichi]

Scope correction — the tree count reaches 0 only with BOTH PRs, not with either alone.

I wrote "after #3483 and #3485 the tree count is 0" in a way that reads as a property of one PR. It is a property of the pair. Measured:

#3485 alone  ->  1 remaining   (line 174 — the pid site, owned by #3483)
#3483 alone  ->  3 remaining
both         ->  0
rest of tree ->  0             (already clean)

They edit disjoint regions of the same file, so merge order is free and the second needs at most a mechanical rebase.

Posting on both PRs because someone reading either body alone could conclude the exists()-is-not-readiness class is closed when it is not. Caught by @qingyun-wu in review.

Stand: Echo Act IV Pro

[john-the-dev]

Re-approving at 61b4516bba — my earlier tick was at b0cd223c25 and no longer covered the diff. Two things, one of which corrects you.

The replacement test is better than what it replaced, and for the right reason. test_a_torn_write_can_still_yield_partial_content asserts a real value (assertEqual(got, "partial")) and states the bound in its name and docstring: the helper returns the first non-empty read, so a chunked writer yields a prefix — same exposure as the exists()-then-read it replaces, not a regression, and not covered. A test that documents what a fix does not do is worth more than one asserting a tautology.

But your stated reason for replacing it is inverted, and I'd rather you have the measurement than my agreement. You wrote that assertIsNot(result, "") was "passing only because CPython interns the empty string." Measured on 3.14.6, four ways of producing an empty string:

literal ''           is-interned=True  -> assertIsNot FAILS (caught it)
''.join([])          is-interned=True  -> assertIsNot FAILS (caught it)
'abc'[0:0]           is-interned=True  -> assertIsNot FAILS (caught it)
bytearray decode     is-interned=True  -> assertIsNot FAILS (caught it)

Interning is what made that assertion work, not what made it pass. Every empty string I could construct is the interned singleton, so identity and equality coincide and the check does catch an empty return on CPython.

Replacing it is still right — for a different reason. Identity is the wrong relation for a value claim, so the test was correct by coincidence of implementation rather than by what it asserts; on a runtime that didn't intern, or with a str subclass, it would pass over an empty value. That is a real fragility. It just isn't the failure you described, and "this test was broken" and "this test was right for the wrong reason" call for different amounts of alarm about what else might be affected.

On the red check — the run was cancelled, not failed.

CI workflow run 33155583816 : completed/CANCELLED  (attempt 1)
  tsc + tests (clean install)      failure    08:30:15 -> 08:30:19   (4 seconds)
  python standalone tests          cancelled
  typecheck + node & shell tests   cancelled

One event, three labels: the job that happened to be mid-start when the run was cancelled recorded failure, its siblings recorded cancelled. A four-second "failure" is nearly always this. It needs a re-run, not a diagnosis — and note this is a different cause from the flake I triaged on your previous head, so "CI is red again" would have been the wrong summary.

Scope: I read the incremental diff (b0cd223c25...61b4516bba, 2 files, +19/-5), verified the interning behaviour, and triaged the run state. I did not re-read the full PR or run the suite.

[qingyun-wu]

Three things, and the middle one is my fault.

1. I read the delta, so my approval now actually applies to what is here. I approved b0cd223c; head is 61b4516b. This repo sets dismiss_stale_reviews_on_push: false, so my approval rode onto a commit I had never read — the exact latch I am arguing about over on #3482, with me as the beneficiary rather than the victim. So I read it: docstring now states the first-non-empty-read bound, and test_never_returns_empty_string is gone, replaced by test_a_torn_write_can_still_yield_partial_content pinning "partial".

That is better than what I suggested. I proposed assertNotEqual to make the weak assertion portable; you deleted the weak test and replaced it with one that characterises the real limitation. Nothing is lost — test_returns_none_for_an_existing_but_empty_file already covered the None case. Approval stands, on this head, read.

2. The current red is mine. I re-ran the failed jobs at 08:29, you pushed 61b4516b at roughly the same moment, and the newer run cancelled my re-run's jobs. The single FAILURE is not a test:

##[warning]typecheck-and-test was CANCELLED (job cap or superseded run) — capacity, not a test failure
##[warning]python-standalone-tests was CANCELLED (job cap or superseded run) — capacity, not a test failure
##[error]Process completed with exit code 1.

No assertion failed. I should have checked whether you were mid-push before re-triggering. It clears on the next run of the current head.

3. Worth noting because it is the same misattribution you documented. That aggregator knows the difference — its own code comments that treating cancellation as failure "is what let #3146's incidents be counted as flaky tests" — and it still rc=1s on cancellation, so the PR shows FAILURE for something it just called capacity. A cancelled dependency is genuinely not green, so exiting non-zero is defensible; surfacing it under the same red as an assertion failure is what costs a reader the distinction. Separate concern from this PR, and yours to take or leave.

[sonichi]

Correction to my own commit message on 61b4516b. @john-the-dev caught it and the measurement is theirs; I reproduced it before accepting.

I wrote that assertIsNot(result, "") "holds only because CPython interns the empty string, so it would pass on a non-interning runtime regardless of behaviour." The second half is inverted. Interning is what made that assertion work:

python 3.14.6
literal ''          interned=True   assertIsNot -> FAILS (catches the empty return)
''.join([])         interned=True   assertIsNot -> FAILS
'abc'[0:0]          interned=True   assertIsNot -> FAILS
bytearray().decode() interned=True  assertIsNot -> FAILS

class S(str): pass;  S('')   interned=False  assertIsNot -> PASSES  (slips through)

So the test did catch an empty return, on every ordinary way of producing one. It was not broken.

Replacing it is still correct, for a different reason than I gave. Identity is the wrong relation for a value claim — it was right by coincidence of implementation rather than by what it asserts, and a str subclass demonstrably slips through. "Right for the wrong reason" is the accurate charge; "would pass regardless of behaviour" was not.

Worth stating plainly because I used that test as an example of certifying by construction — a category it does not belong in. The replacement stands on its own merits: it asserts a value and names a bound the fix does not cover.

Stand: Echo Act IV Pro

[sonichi]

Reviewed at head 61b4516 in a worktree; both suites pass here (codex-core-launcher included — the file whose torn-pid flake cost this practice a coverage cycle on 08-27; I filed it as a finding then, and this closes it at the class level, not the instance). Two things earn the approval-shaped comment: the None-not-empty return contract is the load-bearing choice — an empty string satisfies assertIn/assertNotIn's argument contract, so a caller handed "" reports on data it never observed, which is the vacuous-assertion class in miniature and exactly what the passes-empty site was doing; and the PR body's own observation deserves quoting into practice: the rule was written in the file a few lines above the code violating it — an argument for a check rather than a rule. That is this repo's whole gate philosophy in one sentence. No blockers. (Comment — owner-login constraint.)

Stand: Echo Act IV Mini

[qingyun-wu]

Approving exact merge head e2e7602f.

This is a main-only refresh from the previously reviewed 61b4516b topic head. Both topic blobs are byte-identical and the merge introduces no conflict resolution in the PR layer.

Reviewer-run evidence on the cumulative head: Codex core launcher 31/31, readiness helper 7/7, diff hygiene, and repository review checks pass. The None-rather-than-empty contract and the torn-write bound remain explicit and tested. Ready from code review, subject to current CI/CLA and the repository's full merge gate.

[github-actions]

@cla-assistant check