Review: fix: skip unreachable reparse points (WinError 448)

This is a well-scoped, correctly-targeted Windows crash fix. The root cause is well-understood — iterdir() happily yields entries for reparse points and junctions, but is_dir() internally calls stat() which raises OSError when Windows deems the mount point untrusted. The fix is minimal and correct.

What's done well

Coverage is complete. Both scan passes (detect_rooms_from_folders has a top-level pass and a one-level-deep nested pass) now have the guard. It would be easy to fix only the first pass and miss the second — the author didn't.

Exception type is precise. Using except OSError (not a bare except Exception) is the right call. OSError is exactly what Windows raises for reparse-point stat errors, and catching it narrowly avoids accidentally silencing unrelated failures like permission errors on normal directories.

Variable naming is consistent. The second pass uses item_is_dir and subitem_is_dir to avoid clobbering the outer variable. Clean.

Tests cover both scan depths. Two tests: one for the top-level pass, one for the nested pass. Both use patch.object on Path.is_dir to inject the exact WinError 448 string and assert the function (a) doesn't raise and (b) still discovers rooms from accessible siblings. That's the right testing strategy for OS-specific error injection on non-Windows CI.

Issues found

No warning/logging on skip. The PR silently skips inaccessible entries. For a user running mempalace init who has important directories under junction points, there's currently no feedback that anything was skipped. A logger.debug("Skipping %s: %s", item, exc) or even a logger.warning would help diagnostics without being noisy in the normal case. The module doesn't currently import logging — a one-liner addition.

item.iterdir() in the nested pass is unguarded. After the outer is_dir guard succeeds, the nested pass calls item.iterdir() directly (line ~148). On some Windows configurations, a reparse point that passes is_dir() can still fail on iterdir() itself. Worth wrapping that call too:

try:
    subitems = list(item.iterdir())
except OSError:
    continue
for subitem in subitems:
    ...

continue in the nested subitem guard is structurally different. In the outer loop a continue skips the whole item. In the inner loop the continue only skips the current subitem — which is correct, but the asymmetry is worth a comment for future readers.

Test monkeypatching is class-level. patch.object(bad.__class__, "is_dir", patched_is_dir) patches Path.is_dir for all Path instances in the test (the closure filters by identity, but another thread calling is_dir during the with block could theoretically see patched behavior). In a single-threaded pytest run this is fine; just flagging for awareness.

Suggestions

1. Add import logging + logger = logging.getLogger("mempalace") to room_detector_local.py and emit a debug log on skip.
2. Wrap item.iterdir() in the nested pass with try/except OSError: continue.
3. Add a brief # WinError 448 comment above each guard so the next person searching for that error code finds it immediately (the PR description mentions it, but code comments are more discoverable).

Overall verdict

Approve with minor suggestions. The core fix is correct and complete for the stated bug. The unguarded iterdir() in the nested pass is a real edge case worth addressing. The logging gap is a UX issue, not a correctness issue. Neither blocks merge — this is a crash fix for Windows users and the code is right.

Reviewed by MemPalace-AGI — autonomous research system with perfect memory

All three concerns addressed:

• Logging on skip ✓ (debug-level, right verbosity)
• Unguarded wrapped ✓
• Third test for iterdir mock ✓

Clean fix. LGTM.