test(dashboard): make chat-pins transient-I/O test portable on Windows - #2454
Conversation
chmod(0o000) does not block reads on Windows (POSIX perm bits are ignored for read access), so load_chat_pins reads successfully and the expected OSError never fires, failing test_load_transient_io_error_preserves_existing_state on the Windows backend shard. Inject the transient read error by monkeypatching read_text to raise PermissionError instead, matching the sibling test_load_transient_io_error_no_destructive_followon in the same file.
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Test-only portability fix at the true root cause (non-portable failure injection), mirroring the sibling test's established pattern; production code untouched. [DESIGN-REVIEWED] fbde279 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
kirodotdev#2454) chmod(0o000) does not block reads on Windows (POSIX perm bits are ignored for read access), so load_chat_pins reads successfully and the expected OSError never fires, failing test_load_transient_io_error_preserves_existing_state on the Windows backend shard. Inject the transient read error by monkeypatching read_text to raise PermissionError instead, matching the sibling test_load_transient_io_error_no_destructive_followon in the same file. Co-authored-by: Joe Guo <zejiangg@amazon.com>
Problem
The Windows backend CI shard fails deterministically:
Why it matters
This is a base breakage on
main(the test landed via #1676), so it turns the Windows shard red on every open PR, not just the branch that introduced it — masking real regressions behind a known-failing check.Fix (symptom → root cause → change)
load_chat_pins()to re-raiseOSError, but on Windows nothing is raised.(tmp_path / "chat_pins.json").chmod(0o000). Windows ignores POSIX permission bits for read access — a0o000file still opens there — sopath.read_text(...)succeeds,load_chat_pins()returns normally, andpytest.raises(OSError)sees no exception. The production code under test is correct; only the failure-injection was non-portable.monkeypatch.setattr(type(path), "read_text", _failing_read)where_failing_readraisesPermissionError(anOSErrorsubclass). This deterministically hits theexcept OSError: raisebranch on every OS. The assertion that in-memory_chat_pinsis preserved is unchanged. This mirrors the sibling testtest_load_transient_io_error_no_destructive_followonin the same file, which already uses exactly this pattern. As a side benefit it removes thechmodtry/finallycleanup dance, which on POSIX could leave an unreadable temp file if the body failed before thefinally.Tests
test/test_dashboard_chat_pins.py::test_load_transient_io_error_preserves_existing_state— reworked to inject the transient read error portably; still asserts theOSErrorre-raise and that valid in-memory pins are not clobbered.isort/flake8clean. No production code changed.Manual verification
N/A — unit coverage is sufficient; this is a test-only portability fix and the change is exercised by the reworked test itself on the local (non-Windows) run.