Fix skill_phases undeclared-row check swallowing unreadable state

[fdaviddpt]

scripts/skill_phases.py's _undeclared_rows() caught (RuntimeError, OSError) around its call to manager_docs.documents() and returned [] on either -- folding an unreadable phases directory into the same shape as "no undeclared files found", the state the module's own docstring six lines above promises to preserve. Guard and bypass in one function.

Fixes #589.

What changed

OSError now returns a single-item list with a fully-populated unreadable row (matching the sibling row shape emitted a few lines below for documents()'s own unreadable messages). RuntimeError (no spine at root) still returns [], since that case is already surfaced elsewhere as the spine document's own missing row.

Honesty about reachability

The release-gate audit that filed this issue could not demonstrate a live route to the bypass at HEAD, and neither could I: documents() already catches OSError internally around its own iterdir() and returns it via the unreadable list rather than raising, and a genuinely unreadable spine would already raise inside check()'s own unguarded read_bytes() call before _undeclared_rows() is ever reached. So this is a latent bypass with no reachable caller today, not a live defect -- the fix closes it before documents() grows an arm that reaches it. Tests reproduce all three of the auditor's own control arms via monkeypatch: a healthy-tree positive control, documents() raising PermissionError, and documents() returning a real unreadable message.

Two below-bar items from the brief, and a third one self-review found

The brief named two adjacent observations below the filing bar and asked me to decide whether either belongs in this diff. Both stay out: skill_phases.py:118-120's check() catching only FileNotFoundError around the spine's own read_bytes() call, and manager_docs.py:68's FileNotFoundError handling for the phases directory without the deepest-lookable-ancestor disambiguation doctor._dir_state built for exactly this (#380). Both are real, both are one sentence to state, and neither is reachable through this diff's own footprint without widening scope into a design decision (what state does an unreadable-but-not-missing spine get?) that #589 was not scoped to make. below-bar.

Self-review (both the reviewer and the auditor spawn, independently) found the same class in the comment I wrote for the RuntimeError arm: it claimed that case is unconditionally "already reported" by check()'s own missing row, which only holds for a genuinely absent spine -- Path.is_file() also folds a stat failure into the same False, and check()'s narrow except FileNotFoundError (the same pre-existing gap named above) means that narrower case reaches the RuntimeError arm only after check() has already raised uncaught, not after a missing row was emitted. Fixed in a follow-up commit by rewording the comment rather than widening the except clause -- same design-decision reasoning as the two items above, so the comment now says so instead of overclaiming. below-bar.

Docs

changelog.d/589.fixed.md added. README.md (the repo's only docs_targets entry) opened and read against this diff -- it does not describe skill_phases.py's internal row states, so no-change-needed. scripts/doctor.py does not reference skill_phases.py at all, so the diagnostic is unaffected by construction -- already covered, confirmed by grep.

Tests

python3 -m pytest tests/test_skill_phases_undeclared_bypass_589.py -q --no-cov: red before the fix (documents() raised PermissionError but _undeclared_rows() returned [] -- the unreadable state was folded into 'nothing found'), green after (16 passed alongside test_skill_phase_split.py). Guard sweep via lane_setup.py --lane named test_claude_md_currency.py, test_gate_state_consumers_328.py, test_unwired_scripts_253.py; all green.

Verified by the maintainer

Re-ran the new test file against origin/main with the fix absent, in a detached worktree:

E  AssertionError: documents() raised PermissionError but _undeclared_rows() returned []
   -- the unreadable state was folded into 'nothing found'
E  assert []
1 failed, 2 passed

One failed, two passed — the three-arm control behaving as designed. Arms 1 and 3 pass before the fix, which is exactly right: arm 1 is the positive control that stops the assertion being vacuous, and arm 3 proves the unreadable state is reachable at all. Only arm 2 discriminates, and it is the one that went red.

The honesty here is the part worth keeping

This closes a latent bypass, not a live defect, and the pull request says so. The round-2 auditor could not demonstrate a route to it at HEAD, both self-review spawns confirmed that independently, and the lane did not manufacture reachability to make the fix look more urgent than it is. That was the outcome I asked for and the easiest one to quietly not deliver — a lane wanting a stronger-looking diff would have written a reachable-sounding scenario and nobody reading the merge would have checked.

The value is that it stops being latent the moment documents() grows an arm that raises, which is a cheap thing to buy now and an expensive one to discover later.

The self-review round found a defect in the diff's own new prose

Both spawns independently flagged that the comment added on the except RuntimeError: return [] arm overclaimed — it said that case is unconditionally already reported by check()'s own missing handling, which is not true for an unreadable-but-not-missing spine. Fixed in 46e5176.

That is the second lane in two days to ship a wrong sentence alongside a right fix and have its own reviewers catch it. Worth noting as a pattern rather than an incident: the prose explaining a subtle fix is written last, under the least scrutiny, and is the part that outlives the diff.

The two adjacent gaps stay out, correctly

skill_phases.py:118-120 (except FileNotFoundError only, so a PermissionError on a phase file escapes check()) and manager_docs.py:68 (FileNotFoundError read as a genuine absence without the deepest-lookable-ancestor disambiguation #380 built for exactly that) were both judged below-bar and written up rather than fixed. I agree with the routing: each needs a design decision, and manager_docs.py:68 in particular is the Windows MAX_PATH case where an over-long name arrives as errno 2 with winerror None — indistinguishable from a real miss, and not something to settle inside a lane scoped to one except clause.

Provenance: filed by the v0.14.0 release gate, round 2, dispatch token rel-0140-r2-6d21ef58c4, and carried past the tag deliberately as non-blocking.