fix(acquisition): preserve purchase journal bytes on replay

[johnjhughes]

Summary

• add an explicit read_only=True purchase-journal mode that replays committed WAL or hot rollback-journal state only in an expendable private copy
• preserve and verify canonical db/lock/wal/shm/journal bytes, inode identity, size, mode, owner, group, atime, mtime, ctime, and existence across verifier entry/exit
• route snapshot, terminal authority, materialization/replay, extension, and replacement-approval verification through the shared reader while retaining writer mode for planner, purchase, reconciliation, recovery, and resolution mutations
• fail closed on lock/path replacement races and preserve primary validation errors while unconditionally releasing descriptors

Validation

• uv run ruff check legalforecast/ingestion/case_dev_purchase.py legalforecast/ingestion/replacement_purchase_approval.py legalforecast/cli.py tests/test_case_dev_purchase.py tests/test_replacement_purchase_approval.py
• uv run pyright
• uv run pytest -q tests/test_case_dev_purchase.py (15 passed)
• uv run pytest -q tests/test_replacement_purchase_approval.py (26 passed)
• dependent terminal/materializer/consolidation/docket suite (128 passed)
• ranked planner writer-mode regressions (2 passed)

Bead: LegalForecastBench-5qd6.118.16.14

[coderabbitai]

Warning

Review limit reached

@secure-gate-dev-token-broker[bot], you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7e471196-c52f-4e93-a9f1-1cec2891090b

📥 Commits

Reviewing files that changed from the base of the PR and between 3342f07 and 6eba351.

📒 Files selected for processing (5)

• legalforecast/cli.py
• legalforecast/ingestion/case_dev_purchase.py
• legalforecast/ingestion/replacement_purchase_approval.py
• tests/test_case_dev_purchase.py
• tests/test_replacement_purchase_approval.py

Walkthrough

CaseDevPurchaseJournal now supports read-only, WAL-aware snapshots. It copies and validates SQLite artifacts in temporary storage, authenticates purchase state, enforces query-only access, and verifies canonical files remain unchanged.

Changes

Purchase snapshot isolation

Layer / File(s)	Summary
Read-only snapshot isolation legalforecast/ingestion/case_dev_purchase.py	The journal adds read_only mode, shared-lock acquisition, secure copying of ledger and SQLite sidecars, temporary snapshot validation, and failed-initialization cleanup.
Authenticated snapshot replay legalforecast/ingestion/case_dev_purchase.py, tests/test_case_dev_purchase.py	authenticated_snapshot() validates caps and computes the purchase-state digest. Snapshot reads use the read-only journal, verify canonical files on close, and test WAL replay, failed-operation reconstruction, write rejection, and file immutability.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant CaseDevPurchaseJournal
  participant TemporarySQLite
  participant CanonicalFiles
  Caller->>CaseDevPurchaseJournal: request read-only purchase snapshot
  CaseDevPurchaseJournal->>CanonicalFiles: acquire shared lock
  CaseDevPurchaseJournal->>TemporarySQLite: copy ledger, WAL, and journal files
  CaseDevPurchaseJournal->>TemporarySQLite: validate and query in read-only mode
  CaseDevPurchaseJournal-->>Caller: return authenticated snapshot
  CaseDevPurchaseJournal->>CanonicalFiles: verify unchanged namespace on close

Loading

Possibly related PRs

• johnhughes3/LegalForecastBench#102: Extends CaseDevPurchaseJournal with read-only snapshots, WAL handling, and authenticated state access.
• johnhughes3/LegalForecastBench#190: Modifies CaseDevPurchaseJournal snapshot and lineage handling.
• johnhughes3/LegalForecastBench#315: Relates to read-only snapshot handling in CaseDevPurchaseJournal and read_case_dev_purchase_snapshot().

Suggested reviewers: copilot

Poem

I copy the ledger, then safely inspect,
With WAL pages aligned and each file checked.
No writes in the journal, no records rearranged,
The digest confirms that the state has not changed.
Hop, hop—snapshots are sound!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: preserving purchase journal bytes during replay.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/purchase-journal-byte-preserving

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

tests/test_case_dev_purchase.py (1)

156-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the mutation detector in close().

This test proves the happy path: the canonical files do not change, so close() passes. No test drives the failure branch at case_dev_purchase.py lines 1687-1691. That branch is the fail-closed control of this PR. It raises CaseDevPurchaseLedgerError when the canonical namespace changes during a read-only audit.

Add a test that mutates a canonical file inside the with CaseDevPurchaseJournal(..., read_only=True) block, then asserts that leaving the block raises CaseDevPurchaseLedgerError with the message about filesystem state.

The rejection of allow_create=True combined with read_only=True at line 1558 is also uncovered.

Do you want me to generate both tests?

🤖 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_case_dev_purchase.py` around lines 156 - 168, Add tests in
tests/test_case_dev_purchase.py covering both read-only safeguards: mutate a
canonical file inside the with CaseDevPurchaseJournal(..., read_only=True) block
and assert exiting raises CaseDevPurchaseLedgerError with the filesystem-state
message, then add coverage asserting read_only=True with allow_create=True is
rejected at journal construction. Reuse the existing journal, canonical-path,
and assertion helpers from the surrounding tests.

legalforecast/ingestion/case_dev_purchase.py (1)

2873-2906: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Align purchase_state_sha256() with authenticated_snapshot() and avoid duplicate digest inputs.

Both methods hash the current JSON-serializable purchase rows plus committed_amount_usd, while _purchase_operation_record() exposes raw fields from the unknown public material recovery join. Make purchase_state_sha256() reuse committed_amount_usd and the canonical operation-row shape instead of defining the digest payload separately.

🤖 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 `@legalforecast/ingestion/case_dev_purchase.py` around lines 2873 - 2906,
Update purchase_state_sha256() to derive its digest payload from the same
committed_amount_usd value and canonical operation-row representation used by
authenticated_snapshot(). Reuse the existing authenticated_snapshot() or shared
canonicalization path rather than independently hashing raw
_purchase_operation_record() fields, while preserving the existing cycle and
policy identifiers in the digest.

🤖 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 `@legalforecast/ingestion/case_dev_purchase.py`:
- Around line 2882-2891: Sort the candidate IDs before iterating in the per-case
cap validation loop around _read_candidate_committed_amount, replacing the
unordered set iteration with deterministic ordering. Preserve the existing cap
comparison and CaseDevPurchaseLedgerError behavior while ensuring the same
exceeding candidate is reported consistently.

In `@tests/test_case_dev_purchase.py`:
- Around line 136-142: Update the read-only probe around main_file_only to
explicitly close the SQLite connection after the query, using contextlib.closing
or an explicit main_file_only.close() while preserving the existing
pytest.raises assertion.

---

Nitpick comments:
In `@legalforecast/ingestion/case_dev_purchase.py`:
- Around line 2873-2906: Update purchase_state_sha256() to derive its digest
payload from the same committed_amount_usd value and canonical operation-row
representation used by authenticated_snapshot(). Reuse the existing
authenticated_snapshot() or shared canonicalization path rather than
independently hashing raw _purchase_operation_record() fields, while preserving
the existing cycle and policy identifiers in the digest.

In `@tests/test_case_dev_purchase.py`:
- Around line 156-168: Add tests in tests/test_case_dev_purchase.py covering
both read-only safeguards: mutate a canonical file inside the with
CaseDevPurchaseJournal(..., read_only=True) block and assert exiting raises
CaseDevPurchaseLedgerError with the filesystem-state message, then add coverage
asserting read_only=True with allow_create=True is rejected at journal
construction. Reuse the existing journal, canonical-path, and assertion helpers
from the surrounding tests.

🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 846326fb-4dc1-49bd-9c34-96598188722e

📥 Commits

Reviewing files that changed from the base of the PR and between 2ba5e59 and 3342f07.

📒 Files selected for processing (2)

• legalforecast/ingestion/case_dev_purchase.py
• tests/test_case_dev_purchase.py

[Copilot]

Pull request overview

This PR hardens purchase-ledger “read-only audit” by introducing a byte-preserving replay path: instead of opening the canonical SQLite files directly (and potentially triggering WAL/checkpoint side effects), it copies the committed SQLite namespace into a private temp directory, replays/validates on the copy, and verifies the canonical on-disk bytes/mtimes/identities did not change during the audit.

Changes:

• Add read_only=True mode to CaseDevPurchaseJournal that acquires a shared lock, snapshots reserved SQLite paths into a private temp namespace, runs integrity/policy/schema checks, enforces PRAGMA query_only=ON, and compares filesystem identity on close.
• Refactor read_case_dev_purchase_snapshot() to route through the journal’s read-only mode and a new authenticated_snapshot() helper (caps checks + purchase_state_sha256).
• Add a regression test ensuring WAL-backed terminal state is replayed correctly without mutating canonical db/wal/shm/journal bytes, and that attempted writes are rejected.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
legalforecast/ingestion/case_dev_purchase.py	Introduces the byte-preserving read-only journal path and consolidates snapshot authentication logic.
tests/test_case_dev_purchase.py	Adds coverage for WAL replay + immutability verification and write rejection in read-only mode.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

legalforecast/ingestion/case_dev_purchase.py:3723

• _purchase_snapshot_filesystem_identity also opens files with O_NOATIME, which can fail with EPERM/EACCES depending on file ownership/capabilities. Currently that propagates as a raw OSError rather than a CaseDevPurchaseLedgerError, making read-only audit failures harder to diagnose.

        descriptor = os.open(path, _read_only_snapshot_open_flags())

legalforecast/ingestion/case_dev_purchase.py:3674

• _copy_regular_single_link_file opens the source file using O_NOATIME, which can raise PermissionError/EPERM unless the current user owns the file (or has CAP_FOWNER). Right now that bubbles up as a raw OSError, which breaks the otherwise consistent CaseDevPurchaseLedgerError surface for read-only audits and makes failures harder to interpret.

This issue also appears on line 3723 of the same file.

    source_flags = _read_only_snapshot_open_flags()
    source_fd = os.open(source, source_flags)
    destination_fd: int | None = None

legalforecast/ingestion/case_dev_purchase.py:3707

• In _copy_regular_single_link_file's finally, if os.close(destination_fd) raises, source_fd will never be closed, leaking a file descriptor (and contradicting the PR goal of unconditionally releasing descriptors). Use a nested try/finally so source_fd is always closed.

    finally:
        if destination_fd is not None:
            os.close(destination_fd)
        os.close(source_fd)

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[ai-agent-on-behalf-of-john-hughes]

[awt-judge] Opus decision

Decision: approve_as_is
Reasoning: All required checks are green (Python quality gates: success; 5/5 CI tasks passed, 0 abnormal). The single reported blocker (Copilot 3725288518) claims read_only=True breaks _verify_replacement_projection_replay because plan_clearance_replacements can call append_replacement_event against a query_only=ON journal. I refuted it at head 6eba351 by code inspection plus empirical tests: (1) I confirmed BEGIN IMMEDIATE under PRAGMA query_only=ON does raise 'attempt to write a readonly database', so the mechanism is real; but (2) append_replacement_event (clearance_replacement.py:755) is only reached when pending_quarantines is non-empty, and on a faithful replay every replacement event is already durable and drained from active at lines 585-610, so pending_quarantines is empty and the write branch is unreachable — a property the code additionally enforces by raising if replay yields any new replacement plan (cli.py:12132-12135). (3) The exact flagged path is covered by tests/test_cycle_manifest_template_cli.py (26 passed), which drives the CLI project-target-cohort replay through a read_only=True journal with a quarantined clearance. Running the changed-file suites and the quarantine end-to-end test produced 91 passing tests with zero failures. The 5 informational comments were addressed in commits 049ce2b-6eba351 and bot-confirmed. No must-escalate categories are touched (no token-scope/secret/credential changes; diff matches stated intent). No material findings remain.
Head SHA: 6eba351
Model: opus