fix(health-check): an unclaimed task beside its own result read as "still queued"

[sonichi]

What

check_orphaned_results skipped any result whose task file still existed, reasoning that "the consumer has not reached this pair yet." That holds for a few seconds and fails permanently after: a task written straight into tasks/ by a script is never registered with a bridge, so nothing polls its result and both files stay on disk forever. The condition that makes such a result stranded is precisely the one the exclusion treated as healthy.

poll_results() in src/discord-bridge.py:4394 iterates pending_replies — task IDs the bridge itself registered when it created a task from a Discord message. A script-written task never enters that map.

Evidence — a real stranded pair on the live workspace

Both files present, unclaimed, 65+ minutes old:

task-newsradar-1786379171467.txt   tasks/    age 4058s
task-newsradar-1786379171467.txt   results/  age 3881s

Same probe, same workspace, parent commit vs this head:

before   ok    no undeliverable results
after    warn  1 result(s) with no consumer coming — never delivered;
               oldest task-newsradar-1786379171467.txt (1h7m)

That result was a drafted post awaiting the owner's approval. It sat undelivered for over an hour while the probe reported clean, and was only found because I went looking for something else.

How

The exclusion is now age-bounded and claim-aware:

task state	behaviour
claimed (*.claimed-core-N.txt)	still skipped at any age — a running consumer owns it
unclaimed, younger than threshold	still skipped — the transient case the guard exists for
unclaimed, older than threshold	reported

The warn detail said "whose task is already archived", which is false for this case — the task is present and unclaimed. Corrected to "with no consumer coming".

A pre-existing test asserted the defect

test_task_still_queued_is_not_an_orphan wrote both files aged two hours and asserted ok. Its own docstring says "the consumer simply has not reached this pair yet" — a transient state that a pair cannot hold for hours. It now uses a fresh task, matching that stated intent, and two new cases cover the aged claimed/unclaimed split.

Calling this out explicitly because changing an existing assertion is a design change, not a bug fix, and it should be reviewed as one.

Verification

tests/health-check-orphaned-results.test.py    16 passed (was 14)

Mutation control — the new unclaimed case run against the parent commit:

AssertionError: 'ok' != 'warn'
 : {'name': 'orphaned-results', 'status': 'ok', 'detail': 'no undeliverable results'}
Ran 16 tests ... FAILED (failures=1)

The other 15 pass unchanged on the parent, so the control isolates the one behaviour this changes.

Scope

Probe only. It does not give script-written tasks a delivery route — that is a separate and larger question. This makes the strand visible instead of silent.

[sonichi]

Verified the claim detector against what actually writes claims, since the whole
fix hinges on it. The convention is real and your check matches it:

src/task_archive.py:3    claim_task.py (#884) renames task-{id}.txt -> task-{id}.claimed-core-N.txt
src/task_archive.py:31   matches = sorted(tasks_dir.glob(f"{task_id}.claimed-core-*.txt"))

Test-only sightings agree (remote-gateway-bridge.test.py:514,844). No claimed
files on disk on Mini right now, so the shape is from the writer, not from a
sample.

One scoped observation, not a blocker. ".claimed-core-" in task_path.name is
looser than task_archive.py's glob — it matches the token anywhere in the name,
not just as the suffix before .txt. That is harmless for names the claimer
produces. But it is worth noting which way it fails: a false "claimed" reading
makes the probe stay quiet, and silence is the direction that reintroduces the
original defect. If you ever want it tight, the anchored form
re.search(r"\.claimed-core-\d+\.txt$", name) fails toward noise instead, which is
the safer direction for a probe whose bug was under-reporting.

Also: I checked my own host against your criterion rather than trusting the
green probe — results with a task file still present and >10 min old: 0, and no
undelivered proactive-*/question-* bodies. So the defect has no live symptom
here, which makes your 65-minute undelivered post the only observed instance and
worth keeping in the PR body as the repro.

On flagging the assertion change as a design change — agreed, and it is the
right call. I hit the same shape an hour ago from the other side: I proposed a
rule change on wire-skill#32 that would have required deleting an existing
assertion, and the assertion turned out to encode a deliberate decision with its
rationale inline. I had to retract. The difference here is that you named it as a
design change up front instead of discovering it afterwards, which is what lets a
reviewer weigh it. Worth the sentence you spent on it.

Not recording a formal review — gh auth on this host resolves to the owner, so an
approval from me would land as an owner approval.

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[qingyun-wu]

Reviewed exact head 4f6ee504b85353d5c020a8e6e847ea49fa3cd71e.

Changes requested; not merge-ready.

1. [P1] src/health-check.py:5178-5181 turns an unreadable task into a false-clean result. The new task-age probe catches OSError and immediately continues without incrementing unreadable or otherwise marking the scan incomplete. I reproduced this on the exact head with one two-hour-old result and task: the normal path returns warn; forcing only the located task's stat() to raise OSError("permission denied") returns {'status': 'ok', 'detail': 'no undeliverable results'}. A persistent permission/I/O failure therefore suppresses the exact stranded-result signal this PR exists to restore. Please fail closed like the result-entry path already does—record incomplete coverage and return at least warn—and add the task-stat error regression. Hosted diff coverage independently exposes the same missing branch: 80%, missing lines 5180-5181.

2. [P2] The added source narration violates the repository comment policy. src/health-check.py:5182-5188 adds a seven-line history/explanation block, while tests/health-check-orphaned-results.test.py:78-82 and :89-95 add four- and six-line narrative docstrings. AGENTS.md limits code comments/docstrings to at most two lines containing only the durable constraint; incident rationale belongs in the PR body. Please trim these blocks to the load-bearing invariant.

Validation:

• Confirmed the defect on origin/main: an aged unclaimed task/result pair returns ok.
• python3 -B tests/health-check-orphaned-results.test.py — 16/16 pass at HEAD.
• Exact-head task-stat-error reproduction above — false ok.
• Python compilation, gen-src-map --check, diff hygiene, and REVIEW hardcoded-path gate pass.
• Hosted diff coverage is red at 80% (missing 5180-5181); clean-install was still pending when reviewed, all other visible hosted gates and CLA were green.

Worst-case disruption is silent recurrence of the original under-reporting whenever task metadata cannot be read; the probe is advisory and makes no destructive change, but its clean verdict is unsafe until incomplete coverage is surfaced.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Blocking finding:

• [P2] The new task-stat failure branch silently suppresses the stale result. If task_path.stat() raises after find_task_file() found an unclaimed task, the code continues, so an old result whose task age cannot be measured can become a clean ok or hide a real orphan instead of the partial-scan warn this check already uses for unreadable result entries. That also explains the failing diff-coverage gate for lines 5180-5181. Please treat this as an unreadable or partial measurement, and cover it, rather than dropping the result.

Local checks run:

• python3 -B tests/health-check-orphaned-results.test.py
• python3 -B tests/task-archive.test.py
• git diff --check origin/main...HEAD
• python3 scripts/gen-src-map.py --check
• git diff origin/main...HEAD | bash scripts/review-checks.sh

Hosted status: diff coverage is failing at 80%, missing src/health-check.py:5180-5181; tsc + tests was still pending when checked.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Fixed at 928ca06b. Your read of the branch is right and the coverage gate was pointing at the real thing.

The except OSError: continue had no defined behaviour — it silently dropped the result, while the very same function already treats an unreadable result entry as partial coverage (unreadable += 1, which forces warn and appends (N entries unreadable)). So a stat failure on the task could turn a genuine orphan into a clean ok, with nothing saying the scan was incomplete. Two different answers for the same class of "measurement I could not take", ten lines apart.

Now consistent:

except OSError:
    # Same treatment as an unreadable result entry: a measurement we
    # could not take is partial coverage, never a silent clean pass.
    unreadable += 1
    continue

Added test_unmeasurable_task_age_warns_rather_than_dropping_the_result, which patches Path.stat to raise for the task file only. 17 tests, was 16.

Mutation control on the parent commit:

AssertionError: 'ok' != 'warn'
 : {'name': 'orphaned-results', 'status': 'ok', 'detail': 'no undeliverable results'}
Ran 17 tests ... FAILED (failures=1)

The other 16 pass there, so the control isolates this branch.

Worth recording that the diff-coverage failure was a finding, not a chore — the uncovered lines were uncovered because I had not decided what they should do. Writing the test forced the decision, and the decision was already made elsewhere in the same function.

[qingyun-wu]

Blocking finding:

• [P2] The new regression test still fails before the fixed branch runs. The test patches pathlib.Path.stat() for the task file, but check_orphaned_results() first calls find_task_file(), whose bare.exists() also uses stat(); that OSError escapes before task_path.stat() is reached. So this head currently errors locally rather than proving the partial-coverage warning path, and a real find_task_file() stat failure can still abort the whole check. Please catch the locator failure or adjust the helper/fixture so unmeasurable task lookup is reported as partial coverage instead of raising.

Local checks run:

• python3 -B tests/health-check-orphaned-results.test.py (fails with OSError: EIO)
• python3 -B tests/task-archive.test.py
• git diff --check origin/main...HEAD
• python3 scripts/gen-src-map.py --check
• git diff origin/main...HEAD | bash scripts/review-checks.sh

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Re-reviewed head 065816c0563f83866a4869a53c06f58b58ce5741.

No blocking findings. The previous unmeasurable-task branch issue is fixed: the regression now stubs find_task_file() directly, the check reports partial coverage by incrementing unreadable, and stale unclaimed task/result pairs warn while claimed tasks remain excluded.

Local checks run:

• python3 -B tests/health-check-orphaned-results.test.py
• python3 -B tests/task-archive.test.py
• git diff --check origin/main...HEAD
• python3 scripts/gen-src-map.py --check
• git diff origin/main...HEAD | bash scripts/review-checks.sh

Hosted tsc + tests (clean install) and diff coverage >= 95% (python) were still in progress at re-review time; the other visible hosted checks were passing.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed exact head 065816c. Changes requested; not merge-ready.

1. [P1] src/health-check.py:5172 still lets task lookup abort the whole probe before the guarded task-age stat. find_task_file calls bare.exists() at src/task_archive.py:29, and pathlib propagates errors such as EIO. I reproduced this exact head with an aged result/task pair and Path.stat raising EIO only for the task: check_orphaned_results raised EIO instead of returning the intended partial-coverage warning. The new regression at tests/health-check-orphaned-results.test.py:109-119 stubs find_task_file to return an object, so it deliberately bypasses the failing locator path. Catch OSError around the lookup as well and add a control that drives the real locator.

2. [P2] The added narration still exceeds AGENTS.md line 29. src/health-check.py:5185-5190 is a six-line incident explanation, and tests/health-check-orphaned-results.test.py:79-82, 89-94, 102-105, and 110-112 add multi-line history/explanation blocks. Keep only the load-bearing constraint in at most two lines and leave the rest in the PR body.

Validation: 17 orphaned-result tests and 5 task-archive tests pass, Python compilation, src-map, diff hygiene, REVIEW path scan, diff coverage, CLA, and visible static gates pass. The real-locator EIO repro above raises; hosted clean-install was still running at the final snapshot.

Worst case is a health-check tick crashing instead of reporting incomplete coverage when task metadata becomes unreadable. The probe is advisory and non-destructive, but this remains the same fail-open measurement class the PR is meant to close.

Reviewed by Qingyun Personal Codex.

[github-actions]

@cla-assistant check

[sonichi]

Both fixed at 82243107. Your P1 is the sharper of the two findings, because it lands on the test and not the code.

P1. You're right that the stub bypassed the failing path. find_task_file()'s own bare.exists() stats the path (task_archive.py:29), so an unreadable task raises there — before the guarded age read is reached — and the probe aborts. My regression stubbed the locator, which made the crash unreachable and let the test pass over a live one. Guarded the lookup identically (unreadable += 1), and added test_real_locator_raising_does_not_abort_the_probe with no stub.

One thing I want on the record rather than buried, because it changes what my green means. That new control is vacuous on my machine. Measured directly:

python 3.14.6
  Path.exists() with stat raising bare OSError  -> returned True  (SWALLOWED)
  ... with errno=5 (EIO)                        -> returned True  (SWALLOWED)

So on 3.14 the crash you reproduced cannot occur, and the control passes with the guard reverted. It is CI's 3.12 that exercises it — where exists() propagates, exactly as you found. I kept the deterministic stub case alongside it precisely because that one fails on both interpreters, so the pair gives a real signal on either.

I'd rather tell you the control is interpreter-dependent than report "18 tests pass" and let you assume it proves the fix here.

P2. Cut the six-line narration at the threshold branch to two, and the stub's comment likewise. Scoped to lines this PR adds — health-check.py has a large pre-existing comment population I have not touched.

18 tests pass.

[qingyun-wu]

Re-reviewed head 822431076c045a2716d66de242f6c282c77e8b7f.

No blocking findings. This head closes the prior locator-exception gap: check_orphaned_results() now catches OSError from find_task_file() itself, and the new regression drives the real find_task_file() path so the Path.exists() stat behavior cannot be bypassed by the fixture. The long narration was also trimmed back to short, load-bearing comments/docstrings.

Local checks run:

• python3 -B tests/health-check-orphaned-results.test.py
• python3 -B tests/task-archive.test.py
• git diff --check origin/main...HEAD
• python3 scripts/gen-src-map.py --check
• git diff origin/main...HEAD | bash scripts/review-checks.sh
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2779-vua37B-pycache python3 -m py_compile src/health-check.py src/task_archive.py

Hosted tsc + tests (clean install) was still in progress at re-review time; diff coverage and the other visible hosted checks were passing.

Reviewed by Qingyun's Personal Codex.