fix(#74): make events.writer cross-platform (POSIX fcntl + Windows msvcrt)#80
Merged
Knapp-Kevin merged 1 commit intoApr 28, 2026
Merged
Conversation
… Windows msvcrt) Issue BicameralAI#74: ``events/writer.py:16`` had a top-level ``import fcntl``, which is Unix-only. On Windows the import failed at module load, which collapsed any test session that imported (directly or transitively) ``events.writer`` — including all 17 ephemeral authoritative tests and a long tail of ingest-using tests. Fix: - Replace the top-level ``import fcntl`` with a platform-conditional block that imports either ``fcntl`` (POSIX) or ``msvcrt`` (Windows) and defines ``_lock_exclusive`` / ``_unlock`` helpers with matching semantics. - POSIX path uses ``fcntl.flock(LOCK_EX/LOCK_UN)`` — unchanged behaviour. - Windows path locks byte 0 with ``msvcrt.locking(LK_LOCK/LK_UNLCK, 1)`` so concurrent writers serialize on a shared mutex byte. The actual append happens via ``open(..., "ab")`` which on Windows seeks to EOF per write — the byte-0 lock is the serialization primitive, not a region lock. - Both branches use ``# pragma: no cover`` for the inactive platform. Tests: - ``tests/test_event_writer.py`` — new, 7 tests: - module imports cleanly on the current platform (regression for the original ImportError) - lock helpers exist and are callable - ``write()`` produces a parseable JSONL line - consecutive writes release the lock (would deadlock if leaked) - locking byte 0 on a previously-empty file works (Windows msvcrt edge case) - platform-specific dispatch checks (``test_windows_uses_msvcrt`` / ``test_posix_uses_fcntl``, mutually skipped) Verified on Windows: 6/6 active tests pass. Ephemeral authoritative suite went from 0/17 collectable to 15/17 passing (the remaining 2 are pre-existing V2 promotion gaps unrelated to fcntl). No POSIX behaviour change.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Knapp-Kevin
added a commit
that referenced
this pull request
Apr 29, 2026
…-to-dev labeller (#102) * chore: add ruff + mypy lint stack + Windows test matrix + secret scan + merged-to-dev labeller (CI Phase 1) Implements Phase 1 of docs/DEV_CYCLE.md §4.5.4 per plan-ci-phase-1.md (rev 2, PASS verdict). Five atomic changes land together so the new CI gates light up on the next PR run: 1. pyproject.toml — declare ruff>=0.5.0 + mypy>=1.10.0 in [project.optional-dependencies].test, plus minimal [tool.ruff] / [tool.mypy] config. Lint scope: E/F/W/I/B/UP. Tests/scripts get per-file-ignores so day-one CI is green. Mypy is lenient (ignore_missing_imports, warn_return_any=false) with per-module ignore_errors=true overrides for the 16 noisiest modules — full type coverage chipped away in follow-up PRs. 2. .github/workflows/test-mcp-regression.yml — convert single-runner job to ubuntu-latest + windows-latest matrix with fail-fast: false and a job-level timeout-minutes: 20. The pull_request: trigger is left untouched (no types: added). BICAMERAL_SKIP_CONSENT_NOTICE='1' added to job env so non-interactive CI doesn't stall on the consent prompt. Windows is expected green given the fcntl + subprocess fixes already on dev (#80, #84). 3. .github/workflows/lint-and-typecheck.yml (new) — ruff check + ruff format --check + mypy on pull_request to main/dev. 4. .github/workflows/secret-scan.yml (new) — gitleaks/gitleaks-action@v2 with fetch-depth: 0 so the diff range is fully scannable. Triggers on pull_request to main/dev. 5. .github/workflows/label-merged-to-dev.yml (new — separate workflow, NOT a job in test-mcp-regression.yml). Triggered only on pull_request: branches: [dev], types: [closed] with if: github.event.pull_request.merged == true. Minimal permissions (issues: write, pull-requests: read). actions/github-script@v7 parses GitHub close-keywords from the PR body and applies the merged-to-dev label to each referenced issue. This is the audit V1 fix — keeping the labeller in its own file means test-mcp-regression.yml's existing trigger semantics cannot regress. Branch-protection rules to require these checks remain a manual GitHub UI step (admin-only) — see PR description. Lint hygiene fixes shipped alongside the workflow plumbing: - handlers/update.py: add `from pathlib import Path` (was used unimported). - ledger/status.py: drop unused line_count local. - ledger/queries.py: noqa-annotate the intentional non-top-level import. - 213 ruff --fix auto-corrections across the tree (sorted imports, dropped unused imports, datetime.UTC, PEP 585/604 annotation modernisation, etc.). Refs: docs/DEV_CYCLE.md §4.5.4 Phase 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: ruff format pass Apply ruff format across the tree to satisfy `ruff format --check .` in the new lint-and-typecheck workflow. No semantic changes — pure whitespace, line wrapping, and trailing-comma normalisation. Split from the previous CI Phase 1 commit so the workflow plumbing diff stays readable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ci): trufflehog instead of gitleaks (org license) + Linux-only eval steps Two CI failures on PR #102's first run: 1. Gitleaks fails with "missing license. Go grab one at gitleaks.io" — gitleaks-action@v2 requires a paid license for organizations as of the 2023 breaking update. Switch to trufflesecurity/trufflehog@main, which is free for all repos and has equivalent detection coverage. Use --only-verified to keep noise low. 2. Windows matrix job fails on the Generate E2E report step ("No artifacts found at .../test-results/e2e — run Phase 3 tests first"). The medusa corpus and M1 adversarial eval are Linux-only by design (bash shell, ANTHROPIC_API_KEY-gated, large corpus clone). Gate the corpus clone, the M1 secret probe, and the M1 adversarial step plus the Generate E2E report step on matrix.os == 'ubuntu-latest'. The Windows job continues to run the full pytest suite (the actual regression value) plus uploads its own artifacts via the matrix-suffixed name. Artifact name now includes matrix.os so both runs upload distinct results without overwriting each other. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: ruff format inbound from #100 merge The fixed test_desync_scenarios.py from PR #100 wasn't ruff-formatted (ruff didn't exist in CI when #100 ran). After merging dev forward, apply the format pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jinhongkuan
pushed a commit
that referenced
this pull request
Apr 30, 2026
…vcrt) (#80) Issue #74: ``events/writer.py:16`` had a top-level ``import fcntl``, which is Unix-only. On Windows the import failed at module load, which collapsed any test session that imported (directly or transitively) ``events.writer`` — including all 17 ephemeral authoritative tests and a long tail of ingest-using tests. Fix: - Replace the top-level ``import fcntl`` with a platform-conditional block that imports either ``fcntl`` (POSIX) or ``msvcrt`` (Windows) and defines ``_lock_exclusive`` / ``_unlock`` helpers with matching semantics. - POSIX path uses ``fcntl.flock(LOCK_EX/LOCK_UN)`` — unchanged behaviour. - Windows path locks byte 0 with ``msvcrt.locking(LK_LOCK/LK_UNLCK, 1)`` so concurrent writers serialize on a shared mutex byte. The actual append happens via ``open(..., "ab")`` which on Windows seeks to EOF per write — the byte-0 lock is the serialization primitive, not a region lock. - Both branches use ``# pragma: no cover`` for the inactive platform. Tests: - ``tests/test_event_writer.py`` — new, 7 tests: - module imports cleanly on the current platform (regression for the original ImportError) - lock helpers exist and are callable - ``write()`` produces a parseable JSONL line - consecutive writes release the lock (would deadlock if leaked) - locking byte 0 on a previously-empty file works (Windows msvcrt edge case) - platform-specific dispatch checks (``test_windows_uses_msvcrt`` / ``test_posix_uses_fcntl``, mutually skipped) Verified on Windows: 6/6 active tests pass. Ephemeral authoritative suite went from 0/17 collectable to 15/17 passing (the remaining 2 are pre-existing V2 promotion gaps unrelated to fcntl). No POSIX behaviour change.
Merged
5 tasks
jinhongkuan
pushed a commit
that referenced
this pull request
Apr 30, 2026
…vcrt) (#80) Issue #74: ``events/writer.py:16`` had a top-level ``import fcntl``, which is Unix-only. On Windows the import failed at module load, which collapsed any test session that imported (directly or transitively) ``events.writer`` — including all 17 ephemeral authoritative tests and a long tail of ingest-using tests. Fix: - Replace the top-level ``import fcntl`` with a platform-conditional block that imports either ``fcntl`` (POSIX) or ``msvcrt`` (Windows) and defines ``_lock_exclusive`` / ``_unlock`` helpers with matching semantics. - POSIX path uses ``fcntl.flock(LOCK_EX/LOCK_UN)`` — unchanged behaviour. - Windows path locks byte 0 with ``msvcrt.locking(LK_LOCK/LK_UNLCK, 1)`` so concurrent writers serialize on a shared mutex byte. The actual append happens via ``open(..., "ab")`` which on Windows seeks to EOF per write — the byte-0 lock is the serialization primitive, not a region lock. - Both branches use ``# pragma: no cover`` for the inactive platform. Tests: - ``tests/test_event_writer.py`` — new, 7 tests: - module imports cleanly on the current platform (regression for the original ImportError) - lock helpers exist and are callable - ``write()`` produces a parseable JSONL line - consecutive writes release the lock (would deadlock if leaked) - locking byte 0 on a previously-empty file works (Windows msvcrt edge case) - platform-specific dispatch checks (``test_windows_uses_msvcrt`` / ``test_posix_uses_fcntl``, mutually skipped) Verified on Windows: 6/6 active tests pass. Ephemeral authoritative suite went from 0/17 collectable to 15/17 passing (the remaining 2 are pre-existing V2 promotion gaps unrelated to fcntl). No POSIX behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #74.
Summary
events/writer.pyhad a top-levelimport fcntl, which is Unix-only. On Windows the import raisedModuleNotFoundError: No module named 'fcntl'at module load, collapsing any test session that imported (directly or transitively)events.writer— including all 17 ephemeral authoritative tests plus a long tail of ingest-using tests.Fix
Replace the unconditional
import fcntlwith a platform-conditional block:fcntl.flock(LOCK_EX/LOCK_UN)— unchanged behaviour.msvcrt.locking(LK_LOCK/LK_UNLCK, 1)locking byte 0. Concurrent writers serialize on a shared mutex byte; the actual append happens viaopen(..., "ab")which on Windows seeks to EOF per write — the byte-0 lock is the serialization primitive, not a region lock.Both branches define matching
_lock_exclusive(f)/_unlock(f)helpers, and both are marked# pragma: no coverfor the inactive platform.Tests
New
tests/test_event_writer.py(7 tests):test_writer_module_imports_cleanly— direct regression for the original ImportError.test_lock_helpers_exist_for_current_platformtest_write_produces_jsonl_linetest_consecutive_writes_release_lock— would deadlock the secondopen + lockif the lock leaked.test_write_with_empty_file_locks_cleanly— Windowsmsvcrt.lockingedge case (locking byte 0 on a 0-byte file).test_windows_uses_msvcrt/test_posix_uses_fcntl— mutually skipped platform-dispatch checks.Verification
pytest tests/test_event_writer.py— 6/6 active pass (1 POSIX-only skipped).pytest tests/test_ephemeral_authoritative.py— 15/17 pass (was 0/17 collectable). The remaining 2 failures are pre-existing V2 promotion gaps unrelated to fcntl, will be tracked separately.Test plan