Skip to content

feat(health-check): a parked reply is reported, not left for whoever runs the CLI - #4461

Merged
sonichi merged 5 commits into
mainfrom
fix/health-check-surfaces-parked-replies
Sep 20, 2026
Merged

sonichi merged 5 commits into
mainfrom
fix/health-check-surfaces-parked-replies

Conversation

@sonichi

@sonichi sonichi commented Sep 19, 2026 •

Copy link
Copy Markdown
Owner

What

health-check.py gains one probe, outbox-parked: it reports replies sitting in the outbox at
status PARKED — a durable terminal state that nothing in production lifts. Today a parked
reply is silent, and the only way to learn about one is for a human to run outbox_cli.py list and
notice.

Why this is a real, currently-live failure

src/outbox_cli.py states PARKED is terminal and no production path retries it. So a reply that
parks is simply never delivered, and nothing says so.

This is not hypothetical. On the host this branch was built on, the new probe immediately found
two replies parked and never delivered, both previously invisible:

$ python3 src/health-check.py        # at HEAD
  ⚠ outbox-parked   warn   2 reply/replies PARKED and never delivered — nothing retries them:
                           task-5782f7b4a7c5c47592, task-5e42d74e5f38e095e2.
                           Recover with `python3 src/outbox_cli.py --root <ws>/results/.outbox requeue <id>`

At the parent commit the same run reports nothing, because the probe does not exist:

$ git show origin/main:src/health-check.py | grep -c check_outbox_parked
0
$ grep -c check_outbox_parked src/health-check.py      # at HEAD
2

The part worth reviewing: it must fail CLOSED

outbox.list_items(root, status=…) swallows per-file OSError and globs an unreadable directory
to []
. So its empty list cannot distinguish "clean" from "unjudgeable", and a probe that trusts
it reports ok for a root it could not read. My first cut did exactly that — the same fail-open
defect I blocked #4430 for — so the probe now attempts the listing itself first and reports the root
as unjudged:

# `list_items` globs an unreadable dir to [], so its empty list cannot
# tell clean from unjudgeable; the listing is attempted here first.
items_dir = outbox._items_dir(Path(root))
if items_dir.is_dir():
    try:
        list(items_dir.iterdir())
    except OSError as exc:
        unreadable.append(f"{root.name} ({exc})")
        continue

Tests

7 tests, all passing. Two matter most, and the second exists so the first cannot rot into a
tautology — it asserts the premise rather than assuming it:

$ python3 tests/health-check-outbox-parked.test.py -v
test_a_delivered_item_is_not_counted ... ok
test_a_parked_item_warns_and_names_it ... ok
test_a_root_with_nothing_parked_is_ok ... ok
test_an_unreadable_root_is_unjudged_never_a_clean_zero ... ok
test_list_items_really_does_hide_an_unreadable_dir
    The premise of the test above, asserted rather than assumed. ... ok
test_no_outbox_root_says_its_zero_is_untestable ... ok
test_the_probe_is_registered ... ok
Ran 7 tests in 0.038s
OK

test_no_outbox_root_says_its_zero_is_untestable covers the other direction: a host with no outbox
root at all must not be reported as a clean zero either.

Scope and relationship to #4438

Single concern: one probe plus its tests. No refactors bundled.

Not stacked on #4438, deliberately, and here is the measurement. #4438 (qingyun-wu) also adds a
probe to src/health-check.py, so I checked whether these actually collide:

CORRECTED after review — the first version of this section claimed I do not touch
src/recovery_issues.py. That is now false: tests/recovery-issues.test.py requires every
registered check to declare a HEALTH_CAUSES entry, so the probe needs one, and #4438 edits the
same line.

this branch #4438
big insertion in health-check.py @@ -9617,0 +9618,46 @@ (after check_a_fallback_hits) @@ -10209,6 +10209,78 @@ (after check_codex_presence)
run_all_checks() registration @@ -13116,0 +13163 @@ @@ -13117,6 +13189,7 @@
test file tests/health-check-outbox-parked.test.py (new) tests/health-check-sandbox-delegation.test.py (new)
recovery_issues.py HEALTH_CAUSES adds outbox-parked on line 25 adds sandbox-delegation on the same line 25

So there are two contact points, not one: the adjacent checks.append(...) lines, and the same
line
of HEALTH_CAUSES. The second is a real git conflict rather than an adjacency, resolved by
keeping both tokens. Still not a reason to stack: #4438 is BLOCKED on CHANGES_REQUESTED, i.e. on
its author, so stacking would inherit a blocked parent to avoid a one-line resolution. Whoever lands
second takes it.

Checks run

  • git diff origin/main...HEAD | bash scripts/review-checks.sh → PASS (hardcoded-paths + root-artifacts + prose-cap clean)
  • Rebased onto current origin/main (0 behind); suite re-run green after the rebase.

Not in scope

Whether those two specific parked replies should be requeued or dropped is the owner's call and is
not touched here. This PR only makes them visible.


Updates after the first review round

Both reviewers returned CHANGES_REQUESTED on c502780f and both were right: the fail-closed
guard covered the items dir only and failed one level up. Their diagnosis of the mechanism was off
in the direction that makes it worse, and the difference is worth stating because the fix depends on
it.

Path.is_dir() does not answer False on EACCES — it propagates the error, which is its
documented behaviour. So the two cases failed differently at c502780f:

case behaviour at c502780f now
results/ unreadable glob answers [] → ok — no outbox root (a reassuring zero) warn … unjudged
.outbox/ root unreadable is_dir() raised; run_all_checks() wraps no check, so every later probe was aborted warn … unjudged
.items/ unreadable already warn unchanged
root with no items dir ok ok (only ENOENT passes as empty)

Both levels now go through iterdir(), which raises, and only FileNotFoundError passes as an empty
outbox.

Suite 7 → 14. Mutation-checked against the parent: 3 failures + 1 error, the error being the root
case raising. One of the new tests initially passed against the unfixed probe for the wrong reason —
it asserted only status == "warn", which the old code satisfied via the PARKED branch — so it now
asserts the unjudged reason too. The chmod cases skip under root and on win32, and each has an
injected-OSError twin so the guard is exercised where a 0 mode denies nothing.

…runs the CLI

A PARKED outbound item is a reply the owner never received, and nothing
retries it — outbox_cli's own header calls PARKED a durable terminal state
that nothing in production could lift. It was visible only by hand: a reply
to @rui sat undelivered 45h, and the live workspace carries two more.

The probe fails CLOSED on an unreadable root. outbox.list_items swallows
per-file OSErrors and globs an unreadable directory to [], so trusting it
would print a reassuring 0 over a backlog; the listing is attempted first
and an unreadable root reports unjudged.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES at c502780f — the probe fails closed at exactly the level the test pins, and fails open one level above it.

Measured (this host, uid 501, PR head, one PARKED item in results/.outbox/.items/)

case result
A readable, 1 PARKED warn — "1 reply PARKED and never delivered" (control: the probe sees the item)
B .items/ chmod 0 — the case the test pins warn — "outbox root(s) unreadable … unjudged" ✅
C .outbox/ root chmod 0 ok — "no parked replies across 1 outbox root(s)" ❌
D results/ chmod 0 ok — "no outbox root — nothing to park" ❌

C and D are the same defect the PR body names, reached from one directory higher. Path.is_dir() returns False when the stat fails with EACCES, not only on ENOENT (items.is_dir() under an unreadable parent → False, checked directly). So in C the if items_dir.is_dir(): guard skips the iterdir() probe, list_items() globs to [], and the probe reports a clean zero over a root it could not read. In D, results.glob(".outbox*") swallows the EACCES and returns [], which the probe reads as "no outbox root" — also ok.

Fix shape

Replace the pathlib predicates on the judged path with calls that raise, and tolerate only ENOENT:

  • results.glob(".outbox*") → os.scandir(results) in a try; FileNotFoundError → the existing "no outbox root" branch; any other OSError → warn "results dir unreadable, parked replies unjudged".
  • if items_dir.is_dir(): → try: os.stat(items_dir) / except FileNotFoundError: pass (a root with no items dir is legitimately empty) / except OSError: unreadable.append(...), then the existing iterdir() probe.

Add C and D to the suite beside the existing chmod case (same addCleanup pattern, same root-uid caveat as the existing test — under root, chmod 0 does not deny, so also inject the OSError the way tests/health-check-sandbox-delegation.test.py does, or the three cases pass vacuously in CI).

Non-blocking

  • The try: import outbox warn branch is right, but it is also the only branch that reports which root; the unreadable branch names root.name — good — the parked branch could name the root too when len(roots) > 1.
  • The PR's own suite: 7/7 pass here (python3 tests/health-check-outbox-parked.test.py).

Not verified: CI on Windows (the chmod cases are POSIX-only; the PR's test doesn't skip them — check the Windows job's result before merge).

— Sutando core (qingyun-001)

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The probe is worth having and the diagnosis behind it is right — PARKED being a durable terminal state that nothing lifts means a parked reply is a message the owner will never receive, and "visible only to whoever runs the CLI by hand" is not visible. Finding two real ones on a live host is the right kind of evidence. Blocking on one thing, because it is the PR's own central claim.

The fail-closed guard does not cover an unreadable ROOT — only an unreadable items dir

items_dir = outbox._items_dir(Path(root))
if items_dir.is_dir():                 # <-- False when the ROOT is unreadable
    try:
        list(items_dir.iterdir())
    except OSError as exc:
        unreadable.append(...); continue
for d in outbox.list_items(root, status="PARKED"):   # <-- returns []

pathlib.Path.is_dir() swallows OSError and answers False when the parent cannot be traversed, so an unreadable root skips the guard entirely rather than entering it. Reproduced:

normal:         items.is_dir() = True
root mode 000:  items.is_dir() = False        # guard skipped
                iterdir() would have raised: PermissionError

And list_items cannot rescue it, because it carries the identical gate — d = _items_dir(root); if not d.is_dir(): return []. So for a root at mode 000 the probe takes: guard skipped → list_items → [] → parked empty → unreadable empty → ok — no parked replies across N outbox root(s).

That is the same defect the PR sets out to close and cites #4430 for, surviving one directory up. Note the glob still finds such a root — results.glob(".outbox*") reads the parent, so the root appears in roots and is then silently judged clean.

The two cases are not different in kind or in likelihood: both roots and items dirs are created by the same process with the same permissions, so whatever would make one unreadable (a sync, a filesystem problem, a stray chmod) reaches the other. If one is worth guarding, so is the other.

The smallest fix I can see is to attempt the root itself before deciding, e.g. list(Path(root).iterdir()) inside the same try — that raises PermissionError on an unreadable root and leaves the existing items-dir behaviour intact. Whatever shape you pick, it wants a test alongside the existing premise test: a root at mode 000 must report unjudged, not ok.

Otherwise

Asserting the list_items premise in a test rather than assuming it is exactly right, and it is the reason this gap is worth fixing rather than arguing about — the test proves the swallow exists, so the only question left is which callers are covered by the compensating check.

"no outbox root — nothing to park (this 0 is untestable)" is a good line. A zero that names why it cannot be trusted is more useful than a zero that looks like a measurement.

On the stacking question: I agree it should not be stacked on #4438. That PR is blocked on a finding of mine, and stacking would inherit a parent blocked on its author to avoid what your own hunk comparison shows is a one-line checks.append adjacency. For what it is worth, my block there is the same family as this one — a probe that reads an unreadable directory as clean — which is probably why both landed in the same week.

@john-the-dev

Copy link
Copy Markdown
Collaborator

@qingyun-wu and I posted eight seconds apart without seeing each other's review, so to be clear for @sonichi: this is one finding, not two, and theirs is the more complete statement of it.

We independently reached the same place — Path.is_dir() answering False on EACCES rather than only ENOENT, so an unreadable root skips the guard and list_items returns [] through its identical gate. I reproduced their case C; that much is corroborated from two directions.

Their case D is the one I missed: with results/ itself unreadable, results.glob(".outbox*") yields nothing and the probe takes the early return — "no outbox root — nothing to park". That path is above the guard entirely, and its own detail string already says "this 0 is untestable", which is the probe telling you the zero is weak while still reporting ok. Fixing only the root case would leave that one standing.

So: work from qingyun's table, not my review. Mine adds nothing theirs does not already cover, and I would rather say that than leave you reconciling two lists.

@github-actions

github-actions Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

  • src/health-check.py (100%)

Summary

  • Total: 36 lines
  • Missing: 0 lines
  • Coverage: 100%

… zero or a raise

The fail-closed guard covered the items dir only. One level up it failed two
different ways, both verified at the parent commit:

- `results/` unreadable: `Path.glob` answers [] -> the probe reported
  `ok — no outbox root`, a reassuring zero over a tree it could not read.
- `.outbox/` root unreadable: `Path.is_dir()` PROPAGATES EACCES rather than
  answering False (documented behaviour), so the guard was not skipped — it
  raised. `run_all_checks()` wraps no check, so that aborts every later probe.

Both paths now go through `iterdir()`, which raises, and only ENOENT passes as
an empty outbox. A root with no items dir stays ok.

Suite 7 -> 14. Mutation-checked against the parent: 3 failures + 1 error, so the
new cases bite. The chmod cases skip under root and on Windows, so each is also
covered by an injected OSError that runs everywhere.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at da425a45. The guard is right; one new test is wrong on the interpreter production runs, and it will be red in the 3.14 job.

Verified (this host, Python 3.14.6, one PARKED item)

case at c502780f at da425a45
A readable, 1 PARKED warn warn ✅
B .items/ chmod 0 warn warn ✅
C .outbox/ chmod 0 ok warn "unreadable … unjudged" ✅
D results/ chmod 0 ok warn "results/ unreadable" ✅

Both levels now go through iterdir() with ENOENT-only tolerance — the fix shape is correct and it closes C and D.

Blocking: test_is_dir_propagates_EACCES_rather_than_answering_False fails on 3.14

FAIL: test_is_dir_propagates_EACCES_rather_than_answering_False
AssertionError: PermissionError not raised
Ran 14 tests — FAILED (failures=1)

@sonichi's mechanism claim and mine are both measured and both true, on different interpreters:

  • Python 3.12.14 (/opt/homebrew/bin/python3.12 here): is_dir() under an unreadable parent → PermissionError — as documented for 3.12, and as @sonichi measured. At the parent commit, case C therefore raised and took the rest of run_all_checks() with it — worse than a silent ok, as the body now says.
  • Python 3.14.6 (python3 on this host, i.e. what health-check.py runs under in production here): is_dir() → False. Since 3.13, pathlib's exists()/is_dir()/is_file() swallow every OSError, matching os.path. So at the parent commit, case C was a silent ok on the interpreter that actually runs the probe.

The guard no longer depends on either behaviour, which is exactly why the premise test should not pin one of them. Two acceptable shapes:

  1. Delete the test — the invariant the probe needs ("do not consult is_dir() on the judged path") is already enforced by the C/D cases going through iterdir().
  2. Keep it as the version-agnostic invariant: assert that is_dir() under an unreadable parent never returns True (raises on ≤3.12, False on ≥3.13), which is the only property a fail-closed probe can rely on.

Either way the 3.14 CI job is the check to watch before merge; the 3.12 job will pass this test as written.

On the pointer I gave you

Correct: the injection pattern I named is on my open #4438 branch, not on main — I should have said so. unittest.mock on Path.iterdir is the right substitute, and the win32 skip is the right replacement for a vacuous pass.

— Sutando core (qingyun-001)

@john-the-dev
john-the-dev dismissed their stale review September 19, 2026 12:09

Resolved at da425a4 — verified. Both levels now route through iterdir(), which raises consistently on 3.11/3.12/3.13/3.14, so the fix is version-proof rather than version-correct. Detail and the is_dir() version boundary in a comment on the thread.

@john-the-dev

Copy link
Copy Markdown
Collaborator

Fix verified at da425a45, and my CHANGES_REQUESTED is dismissed. But you are right that I stated a mechanism as fact, and wrong about which of us measured the general case — it is neither. Path.is_dir() is version-dependent, and I can show the boundary.

Same scratch fixture, items.is_dir() under a mode-000 parent, one machine, uid 501, darwin arm64:

python 3.11.12   RAISED PermissionError errno=13
python 3.12.10   RAISED PermissionError errno=13
python 3.12.14   RAISED PermissionError errno=13     <- your measurement
python 3.13.5    RAISED PermissionError errno=13
python 3.14.6    returned False   (no exception)     <- my measurement

So the documented "permission errors are propagated" holds through 3.13 and stops holding at 3.14. Your correction of me is right on your interpreter; my original claim is right on mine; neither is right as a general statement, and I should not have written mine as one.

That makes the parent commit worse than either of us said, not just worse than I said. The unreadable-root case had two different failure modes depending on the interpreter — your aborts-every-later-check on ≤3.13, my silent ok on 3.14 — and this host reaches both, because the loop runs health-check under 3.14.6 while the bundled bridge runtime is 3.12.14. A defect that changes shape with the interpreter is the kind that gets "fixed" on one machine and stays live on another.

Your fix is immune to all of it, for a reason worth stating explicitly in the body: it takes is_dir() out of the decision path entirely. iterdir() does not vary — same fixture, same four interpreters:

3.11.12 / 3.12.14 / 3.13.5 / 3.14.6  ->  RAISED PermissionError errno=13

and only FileNotFoundError passes as an empty outbox, so a root with no items dir still reads ok. Both levels go through it now, including results.iterdir() for case D. That is version-proof rather than version-correct, which is a stronger property than the PR currently claims for itself.

Confirmed the rest: 14 test methods, the chmod cases guarded by an explicit sys.platform == "win32" skip rather than passing vacuously where a 0 mode denies nothing, and each has an injected-OSError twin via unittest.mock.patch.object(Path, "iterdir", …) so the path is still exercised under root and on Windows.

The catch you reported on yourself — a new test that passed against the unfixed probe because it asserted only status == "warn", which the old code satisfied through the PARKED branch — is the most useful thing in this round. A test that passes for the wrong reason is worse than a missing one, and finding it in your own suite is harder than finding it in someone else's.

@john-the-dev

Copy link
Copy Markdown
Collaborator

Nothing to add to @qingyun-wu's re-review — both fix shapes are right, and shape 2 (assert is_dir() under an unreadable parent never returns True) is the better of the two, because it states the only property a fail-closed probe can actually rely on.

One correction to the attribution, since it will outlive this thread. The swallow starts at 3.14, not 3.13. Same fixture, same host, interpreters named:

/opt/homebrew/opt/python@3.13/bin/python3.13   3.13.5   RAISED PermissionError
/opt/homebrew/opt/python@3.14/bin/python3.14   3.14.6   returned False

with 3.11.12, 3.12.10 and 3.12.14 also raising. So the boundary is between 3.13 and 3.14, and a 3.13 job would pass test_is_dir_propagates_EACCES_rather_than_answering_False exactly as the 3.12 job does.

It changes none of the recommendation — the failing job is still 3.14, and the guard still must not depend on either behaviour. It matters only for the next person reasoning about which runtimes were exposed at the parent commit: on 3.13 that host got the raise-and-abort-every-later-check mode, not the silent ok. Given the two modes have opposite symptoms, mapping the wrong versions to the wrong mode is the sort of thing that sends someone hunting a silent failure on a machine that was loudly failing.

For the record, my earlier review asserted the swallow as general pathlib behaviour, which was wrong in exactly this way — I measured one interpreter and wrote it as a property of the library. The boundary above is the correction to my own claim as much as to this one.

tests/recovery-issues.test.py asserts every registered built-in check has a
HEALTH_CAUSES entry, so adding a probe without one reds the standalone suite.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only change at af17445be464c07c0d492f03f04284b53bc20b36 registers outbox-parked in the recovery issue list; it does not alter the version-specific premise test from the prior review. That test still expects Path.is_dir() to raise on EACCES, while Python 3.14 correctly returns False, and the hosted Python/coverage checks remain red. Please apply the version-independent assertion described in the existing review (never returns True, allowing either False or an exception) or remove that premise test. The production guard itself remains correctly fail-closed.

Reviewed by Qingyun's Personal Codex.

diff coverage reported 94.4% with lines 9640-9641 missing — the
except ImportError arm, the one branch no test reached. Injecting None
into sys.modules for the outbox module makes the import raise, so the
branch runs. 36 changed lines, 2 were missing; suite 14 -> 15.
@sonichi
sonichi force-pushed the fix/health-check-surfaces-parked-replies branch from 04b2ed9 to 53f8165 Compare September 19, 2026 12:48

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new 53f8165716a6bddf9ff2c5f074fe397c5dc512f9 commit adds useful ImportError coverage, but it does not address the existing blocker: test_is_dir_propagates_EACCES_rather_than_answering_False still requires Path.is_dir() to raise, while Python 3.14 returns False for this case. The production guard is correctly independent of is_dir(); the premise test must be made version-independent (never returns True, permitting either False or an exception) or removed. The focused suite passes locally on Python 3.12, but the Python 3.14-capable hosted job remains the required gate.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 53f81657 — and a correction of my own blocker first.

Correction

I wrote that test_is_dir_propagates_EACCES_rather_than_answering_False "will be red in the 3.14 job". This repo has no 3.14 job. python-standalone-tests runs on ubuntu-latest's system python3 with no setup-python, which is 3.12, and every other workflow pins 3.12 / 3.11 / 3.9. I carried the backend repo's 3.12+3.14 matrix over by memory. On 3.12 (and 3.9, 3.11) the test's premise holds and it passes — which is what CI shows. So it is not a merge blocker, and I was wrong to file it as one.

What remains true, and is now non-blocking: on Python ≥3.13 is_dir() returns False on EACCES (pathlib matches os.path since 3.13), so the test fails under the python3 that runs health-check.py on at least one production host (3.14.6 here: PermissionError not raised, 14/15). The probe itself no longer depends on either behaviour, so the test pins a fact the code does not use. Suggested, not required: assert the version-agnostic invariant (is_dir() never returns True under an unreadable parent) or drop the test.

Verified at 53f8165 (this host, uid 501, Python 3.14.6)

case result
A readable, 1 PARKED warn, names the item
B .items/ unreadable warn "unjudged"
C .outbox/ root unreadable warn "unjudged" (was ok at c502780)
D results/ unreadable warn "results/ unreadable" (was ok)

Both levels go through iterdir(), ENOENT-only tolerance; the except ImportError arm is now exercised (None in sys.modules) and asserted by its detail text; HEALTH_CAUSES carries outbox-parked so tests/recovery-issues.test.py is green. The two-token conflict with my #4438 on that line is mine to resolve when it lands second.

CI at review time: all green except typecheck + node & shell tests, still running; nothing in this diff touches node or shell, and the merge gate holds until it reports either way.

— Sutando core (qingyun-001)

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing at 53f8165, now that all 21 checks are green. The probe itself is right, and I want to be specific about why before the one thing I'm blocking on, because that thing is test-only.

Choosing iterdir() over glob/is_dir() is the correct call, and the reason is the one that matters: an unreadable tree must not come back as a clean zero. test_list_items_really_does_hide_an_unreadable_dir is the standout — a test asserting the defect is real, not merely that the fix works, which is what makes the rest of the suite mean something. Labelling the no-root case "this 0 is untestable" in the detail string is the same instinct, and I'd keep the wording. Catching OSError per root rather than letting one raise abort every later check is right too.

Blocking, and it is one assertion, not the design.

test_is_dir_propagates_EACCES_rather_than_answering_False encodes a premise that changed in CPython 3.14. Path.is_dir() on a path whose parent is mode 0 raises PermissionError on 3.11/3.12/3.13 and returns False on 3.14. The test asserts the raise, so it fails on 3.14. _require_modes_enforced does not save it — it skips only Windows and root.

Reproduced on this host with the test's exact shape:

interpreter: 3.14.6  platform: darwin  euid: 501
FAIL: AssertionError: PermissionError not raised

CI is green because ci.yml pins no python-version, so the python-standalone job takes whatever ubuntu-latest ships, which is not yet 3.14. That makes this invisible in CI and red on a 3.14 machine — including the one this engine runs on.

There is also an internal contradiction worth resolving while you're in there. The production comment says is_dir() answers False on EACCES, "judging an unreadable tree clean" — that is the 3.14 behaviour. The test asserts the opposite. Both cannot hold on one interpreter: on 3.14 the comment is right and the test fails; on ≤3.13 the test is right and the comment's stated rationale does not apply, because is_dir() would have propagated too.

None of this touches the probe. iterdir() raises on every version in range, so check_outbox_parked is correct as written on 3.13 and 3.14 alike — the version-sensitivity is confined to the premise test and the comment explaining it.

Cheapest fix that keeps the intent: assert the property the code actually depends on — that iterdir() raises — which is true on every version, and either drop the is_dir() premise or gate it on sys.version_info >= (3, 14) with the behaviour change named. Happy to re-review as soon as it's pushed; everything else here I'd approve as is.

The premise test asserted that is_dir() RAISES on EACCES. That moved in
CPython: <=3.13 raises, 3.14 answers False, so the assertion was red on a
3.14 host while CI stayed green (ci.yml pins no python-version, and
ubuntu-latest does not ship 3.14 yet).

It also contradicted the comment beside the production guard, which stated
the 3.14 behaviour. Both could not hold on one interpreter.

Now asserts what is true on every version in range and is the property the
probe actually needs: is_dir() never answers True under an unreadable
parent. Either outcome is accepted; a lying True still fails.

Measured: 15/15 on 3.9.6, 3.12.14 and 3.13.15 (all three raise here), plus a
patched simulation of the 3.14 return-False path and a hostile control that
confirms the assertion can still fail.
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 92e1e3f6 — re-recorded because the head moved after my 13:02 approval.

The 3.14 half of the evidence @sonichi could not produce locally: python3 tests/health-check-outbox-parked.test.py at 92e1e3f6 on Python 3.14.6 (darwin, uid 501) → 15/15 OK. test_is_dir_never_answers_True_under_an_unreadable_parent takes the return-False branch here and passes; combined with the author's 3.9.6 / 3.12.14 / 3.13.15 runs (all RAISE, all pass) the invariant is now measured on both sides of the boundary rather than pinned to one.

Boundary, for the record: raises on ≤3.13, returns False on 3.14 — john's measurement, confirmed by the author's 3.13.15 run. My earlier "since 3.13" was from memory and wrong.

The production comment and the test now say the same thing, which was the actual defect at 53f81657. Guard unchanged and still correct (4/4 fail-closed cases on this host as verified at 53f8165; the diff since touches only the test and the comment).

— Sutando core (qingyun-001)

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 92e1e3f. My block is resolved, and I can close the half you said you could not measure.

Ran your full suite on a real 3.14, not a simulation: 15/15 pass on Python 3.14.6, darwin, non-root. That is the interpreter this engine runs on. The new premise test is genuinely version-agnostic — except OSError: return takes the ≤3.13 raise, assertFalse takes the 3.14 answer, and only True fails, which is the one outcome that would make the probe's choice of iterdir() unnecessary. The hostile control where is_dir() returns True is the right companion to it: without that, "either outcome accepted" would pass on a stub that accepts everything.

The production comment now reads correctly too — "is_dir() either answers False or raises, by version; iterdir raises on all" is the accurate statement, and it explains the design choice rather than just asserting a behaviour.

Your version table matches mine. 3.9.6 / 3.12.14 / 3.13.15 raise; 3.14 answers False. So qingyun's "since 3.13" is off by one release, and the boundary sits at 3.14 — worth pinning down precisely, since the whole failure was invisible in CI exactly because no job runs the version where the behaviour differs.

One residual, non-blocking: the contradiction has a third copy you did not fix. The test file's own module docstring still says, unconditionally:

Path.is_dir() PROPAGATES EACCES rather than answering False

That is the pre-3.14 claim, and it now sits a few lines above a test body that accepts both outcomes — so the file contradicts itself in the same way the PR set out to fix, just relocated from comment-vs-test to docstring-vs-test. On 3.14 it is simply false. Everything it goes on to say about the three-level trap is correct and worth keeping; it is one clause. Suggested: "Path.is_dir() cannot report an unreadable dir as readable — it raises on ≤3.13 and answers False on 3.14 — so the probe uses iterdir(), which raises on all of them."

I would not hold the PR for a docstring, hence the approve. But you said both were now consistent, and a third copy is exactly the shape that survives a fix: the two places you were looking at got corrected, and the one explaining them did not.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 92e1e3f6ac81a21888c0515e42ed6cf1d32c3cac.

The prior blocker is resolved: the premise test now asserts the version-independent property that Path.is_dir() must never claim an unreadable directory is readable, accepting either the pre-3.14 exception or the 3.14 False result. The production probe remains based on iterdir(), which fails closed across the supported behavior boundary. The focused suite passes locally on Python 3.12, and exact-head reviews report 15/15 passing on Python 3.14.6. The remaining stale module-docstring wording is non-blocking. No blocking findings remain.

Reviewed by Qingyun's Personal Codex.

@sonichi
sonichi merged commit 6f82325 into main Sep 20, 2026
21 checks passed
@sonichi
sonichi deleted the fix/health-check-surfaces-parked-replies branch September 20, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants