fix(acquisition): account terminal recovery operations - #460
Conversation
|
Warning Review limit reached
Next review available in: 6 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 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThe recovery flow now records canonical terminal-unavailable operations in a URL-free JSONL artifact. The CLI writes and verifies this artifact, binds records to authenticated purchase state, updates run-card commitments and counts, and avoids provider I/O for terminal failures. ChangesTerminal-unavailable recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RecoveryCLI
participant QuarantineRecovery
participant PurchaseLedger
participant CourtListener
RecoveryCLI->>PurchaseLedger: read authenticated purchase snapshot
RecoveryCLI->>QuarantineRecovery: verify attempt policy and operations
QuarantineRecovery->>PurchaseLedger: validate terminal ledger bindings
QuarantineRecovery->>CourtListener: request recoverable documents only
QuarantineRecovery-->>RecoveryCLI: return recovered, restricted, and terminal-unavailable records
RecoveryCLI-->>RecoveryCLI: commit artifact bytes and run-card counts
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR tightens RECAP Fetch quarantine recovery accounting by splitting authorized recovery authority into (a) recoverable unknown-status documents that require fresh CourtListener detail + PDF I/O and (b) canonical “terminal-unavailable” failed queue outcomes (statuses 3/6/7) that are recorded for spend/cap accounting but intentionally skip any provider/PDF activity. It also extends materializer verification and run-card commitments to bind this new partition back to authenticated purchase state (including optional broker receipt history).
Changes:
- Extend quarantine recovery to emit a new
terminal-unavailable-operations.jsonlpartition and return it fromrecover_recap_fetch_quarantine_documents. - Add strict validation + purchase-snapshot replay verification for terminal-unavailable rows during materializer verification.
- Update CLI, runbook/docs, and tests to cover the new output artifact, commitments, and partition invariants.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_unknown_public_recovery.py | Adds coverage for terminal-unavailable partitioning and broker-receipt tamper rejection. |
| tests/test_resolved_post_recovery.py | Extends CLI/materializer verification tests to include the terminal-unavailable artifact and commitments. |
| tests/test_purchase_approval.py | Updates CLI argument wiring tests for the new --terminal-unavailable-output parameter. |
| legalforecast/ingestion/recap_fetch_quarantine_recovery.py | Implements terminal-unavailable record generation plus validation and ledger-binding replay. |
| legalforecast/cli.py | Adds new CLI output flag, commits the terminal artifact in run cards, and verifies it during materializer replay. |
| docs/schemas/recap-fetch-quarantine-recovery-v1.md | Documents the v1 partition contract and terminal artifact semantics. |
| docs/README.md | Links the new quarantine recovery schema doc from the schema index. |
| docs/official-run-runbook.md | Updates official-run recovery instructions to include terminal-unavailable output and the partition rules. |
Suppressed comments (1)
legalforecast/cli.py:45305
- In recovered-public provenance clearance, the verifier checks that the recovery committed the expected manifest/restriction/case relevance/review-request/document-root paths, but it does not check the newly committed
terminal_unavailable_path. That omission makes recovered-public authority verification less strict than the underlying quarantine recovery contract for terminal-unavailable artifacts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 064742dcf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
legalforecast/ingestion/recap_fetch_quarantine_recovery.py (1)
923-931: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the two canonical hashers.
_canonical_mapping_sha256and_canonical_operation_sha256serialize with identical options. They differ only by thedict()copy and the"sha256:"prefix. Two copies of a canonicalization rule can drift, and a drift here changes every committed digest. Define one helper and derive the prefixed form from it.♻️ Proposed consolidation
def _canonical_operation_sha256(value: Mapping[str, Any]) -> str: - payload = json.dumps( - value, - ensure_ascii=False, - separators=(",", ":"), - sort_keys=True, - allow_nan=False, - ).encode("utf-8") - return "sha256:" + hashlib.sha256(payload).hexdigest() + return "sha256:" + _canonical_mapping_sha256(value)The two
(use-jsonify)findings from ast-grep on these functions are false positives. That rule targets HTTP response bodies. These functions compute digests.Also applies to: 1172-1180
🤖 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/recap_fetch_quarantine_recovery.py` around lines 923 - 931, Consolidate _canonical_mapping_sha256 and _canonical_operation_sha256 around one shared canonical JSON/SHA-256 helper, preserving the existing serialization options and digest values. Keep the mapping helper’s unprefixed hash and have the operation helper derive its "sha256:"-prefixed result from the shared helper; remove duplicated canonicalization logic without changing callers.Source: Linters/SAST tools
tests/test_unknown_public_recovery.py (1)
347-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing canonical encoder instead of duplicating it.
_canonical(receipt)is the production encoder used when appending a broker receipt, but this test rebuilds the receipt digest with its own matchingjson.dumps(..., allow_nan=False)call. Import the helper fromlegalforecast.ingestion.case_dev_purchasefor the test digest and persistence updates, or define one locally, so canonicalization cannot drift and the malformed receipt cases stop depending on a duplicated encoder format.🤖 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_unknown_public_recovery.py` around lines 347 - 360, Replace the duplicated json.dumps canonicalization in the test setup around the response update with the existing production _canonical helper imported from legalforecast.ingestion.case_dev_purchase. Use that helper when computing receipt_item["sha256"] and when serializing response for the purchase_operations persistence update, preserving the test’s existing behavior while ensuring both paths share the canonical encoder.tests/test_resolved_post_recovery.py (1)
2104-2116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd recovery coverage for terminal queue statuses 3 and 7.
This test covers terminal accounting only for
queue_status == 6. Recovery parses status fromCourtListenerRecapFetchError: RECAP Fetch terminal queue status <status>and treats statuses 3, 6, and 7 as terminal, so statuses 3 and 7 also need assertions forterminal_record["queue_status"],terminal_record["recovery_provider_request_executed"] is False, andterminal_record["paid_redispatch_executed"] is False.🤖 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_resolved_post_recovery.py` around lines 2104 - 2116, Add recovery test coverage for terminal queue statuses 3 and 7 alongside the existing status-6 assertions in the resolved post recovery test. Verify each terminal record reports the expected queue_status and that recovery_provider_request_executed and paid_redispatch_executed are both False, preserving the existing terminal accounting checks.
🤖 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/recap_fetch_quarantine_recovery.py`:
- Around line 416-426: Update the recovery flow around
_terminal_unavailable_record so selection_document_sha256 is taken from the
authenticated purchase snapshot’s operation["attempt_document_sha256"], rather
than record["attempt_document_sha256"]. Preserve the existing comparison at the
caller so it rejects artifacts whose top-level attempt_document_sha256 differs
from the snapshot value.
- Around line 336-337: Update validate_terminal_unavailable_records to require
queue_status to be an exact int before checking membership in
_TERMINAL_QUEUE_STATUSES or constructing/comparing terminal_reason; reject
booleans and floats such as 6.0 while preserving acceptance of valid integer
statuses.
- Around line 786-788: Replace the queue_id.isdigit() validation in the affected
queue-id validation flow with the module-level _CANONICAL_QUEUE_ID pattern,
defined alongside the other patterns as the canonical positive ASCII decimal
check. Preserve the existing string and leading-zero rejection behavior while
ensuring only values matching that pattern can enter terminal records.
- Around line 883-900: The terminal receipt validation in
_validate_terminal_broker_receipts must reject retained history containing
billing evidence. Extend the existing conflict condition to fail when
authoritative_fee_usd is not None or "0.00", or when billing_evidence is
non-empty, while preserving the current identity and ordering checks; do not
alter validate_broker_receipt or introduce a separate failed-state path.
- Line 888: Update the timestamp comparison in the quarantine recovery logic to
use exact string inequality rather than lexical ordering. Preserve the existing
previous-value guard, but replace the ordering check so valid non-equal
canonical timestamps are not rejected based on string sort order.
In `@tests/test_resolved_post_recovery.py`:
- Around line 2067-2085: Close both SQLite ledger connections deterministically
by wrapping sqlite3.connect in contextlib.closing: update
tests/test_resolved_post_recovery.py lines 2067-2085 and keep
connection.commit() plus PRAGMA wal_checkpoint(TRUNCATE) inside the block before
the handle is used by cli.main or read_case_dev_purchase_snapshot; update
tests/test_unknown_public_recovery.py lines 328-360 similarly and add an
explicit connection.commit() after the purchase_operations UPDATE so the
mutation is preserved before closing.
---
Nitpick comments:
In `@legalforecast/ingestion/recap_fetch_quarantine_recovery.py`:
- Around line 923-931: Consolidate _canonical_mapping_sha256 and
_canonical_operation_sha256 around one shared canonical JSON/SHA-256 helper,
preserving the existing serialization options and digest values. Keep the
mapping helper’s unprefixed hash and have the operation helper derive its
"sha256:"-prefixed result from the shared helper; remove duplicated
canonicalization logic without changing callers.
In `@tests/test_resolved_post_recovery.py`:
- Around line 2104-2116: Add recovery test coverage for terminal queue statuses
3 and 7 alongside the existing status-6 assertions in the resolved post recovery
test. Verify each terminal record reports the expected queue_status and that
recovery_provider_request_executed and paid_redispatch_executed are both False,
preserving the existing terminal accounting checks.
In `@tests/test_unknown_public_recovery.py`:
- Around line 347-360: Replace the duplicated json.dumps canonicalization in the
test setup around the response update with the existing production _canonical
helper imported from legalforecast.ingestion.case_dev_purchase. Use that helper
when computing receipt_item["sha256"] and when serializing response for the
purchase_operations persistence update, preserving the test’s existing behavior
while ensuring both paths share the canonical encoder.
🪄 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: 1b1efa85-4bfa-4daa-98d2-d3fce2168d17
📒 Files selected for processing (8)
docs/README.mddocs/official-run-runbook.mddocs/schemas/recap-fetch-quarantine-recovery-v1.mdlegalforecast/cli.pylegalforecast/ingestion/recap_fetch_quarantine_recovery.pytests/test_purchase_approval.pytests/test_resolved_post_recovery.pytests/test_unknown_public_recovery.py
|
[awt-judge] Opus decision Decision: block_for_fixes Codex action itemsAction Items for CodexRequired CI is green, but the findings below are correctness/security/data-integrity defects raised at the current head by Codex, CodeRabbit, and Copilot. Fix every Critical and Security/Integrity item and re-run quality gates before re-requesting merge. Grouped by file, most-blocking first. legalforecast/ingestion/recap_fetch_quarantine_recovery.pyCritical — functional regressions (fix first)
Security / data integrity (must fix)
tests/test_resolved_post_recovery.py and tests/test_unknown_public_recovery.py
Optional — defer to follow-up (not merge-blocking)
Expected Outcome
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
legalforecast/ingestion/recap_fetch_quarantine_recovery.py:258
- The docstring says this publishes an immutable canonical JSONL manifest, but the implementation relies on
json.dumps(...)defaults (ensure_ascii=True,allow_nan=True). To make the emitted bytes unambiguously canonical (and consistent withlegalforecast.ingestion.canonical_json), it’s safer to pinensure_ascii=Falseandallow_nan=Falsefor each record serialization.
) -> None:
"""Atomically publish an immutable canonical JSONL manifest."""
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
legalforecast/ingestion/recap_fetch_quarantine_recovery.py:280
write_recap_fetch_quarantine_manifestnow supports alabelfor clearer conflict errors, butwrite_recap_fetch_restriction_evidencestill calls it without a label. If the restriction-evidence file already exists with different bytes, the error will misleadingly say it’s an “existing quarantine manifest” conflict. Pass an explicit label here (and similarly for other non-manifest uses) so troubleshooting points at the correct artifact.
def write_recap_fetch_restriction_evidence(
path: Path, records: Sequence[Mapping[str, Any]]
) -> None:
"""Publish immutable URL-free fresh-detail public restriction evidence."""
write_recap_fetch_quarantine_manifest(path, records)
|
[awt-judge] Opus decision Decision: block_for_fixes Codex action itemsAction Items for CodexRequired check Critical Blockers (must fix)
Fix approach
Expected outcome
Optional / non-blocking
|
|
[awt-judge] Opus decision Decision: approve_as_is |
## Summary - allow the exact-100 successor's authenticated empty pre-recovery purchased manifest to derive paid scope from final-selection gap identities - subtract only verifier-owned terminal decision omissions and bind their source artifacts and partition into the consolidation run card - filter historical closed-tranche material to the final active scope while rejecting missing, duplicate/rebound, uncleared, unledgered, or inconsistent records - route terminal-authority journal replay through the byte-preserving read-only API landed in #459 and integrate the terminal-recovery contract landed in #460 ## Validation - `uv run pytest -q tests/test_replacement_recovery_consolidation.py tests/test_cohort_document_materializer.py tests/test_unknown_public_recovery.py` — 95 passed - `uv run ruff format --check legalforecast/cli.py tests/test_replacement_recovery_consolidation.py` - `uv run ruff check legalforecast/cli.py tests/test_replacement_recovery_consolidation.py` - `uv run pyright` — 0 errors - `uv run pytest -q` — 6,157 passed, 13 skipped No live purchase ledger, provider, paid acquisition, evaluation, freeze, or dispatch operation was performed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for authenticated terminal omission inputs during replacement recovery. * Recovery now records verified source snapshots and input locations. * Added replay handling for terminal omission metadata. * **Bug Fixes** * Strengthened validation of paid-document coverage, ledger consistency, clearance status, and recovery gaps. * Duplicate recovery records are now rejected consistently. * Recovery now detects missing or inconsistent authority inputs and coverage data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Validation
No live purchase ledger, provider, paid operation, evaluation, freeze, or dispatch was used.
Note
Medium Risk
Touches purchase-ledger interpretation, materializer verification, and official-run recovery boundaries; incorrect terminal classification could mis-account spend or skip recovery incorrectly, though changes are heavily validated and fail-closed.
Overview
RECAP Fetch quarantine recovery now exactly partitions every attempt-policy document into either a recoverable quarantine manifest row or a new
terminal-unavailable-operations.jsonlartifact (legalforecast.recap_fetch_terminal_unavailable.v1). Documents whose purchase ledger operation is a canonical failed RECAP Fetch queue outcome (statuses 3, 6, or 7) are recorded as terminal-unavailable without CourtListener detail/PDF I/O or paid redispatch; all other authorized operations still go through fresh public recovery. Non-canonical failures remain fail-closed.The
recover-recap-fetch-quarantineCLI adds--terminal-unavailable-output, extends the run card with authorized / recovered / terminal counts and a committed hash for the terminal file, and materializer verification replays terminal rows against the authenticated purchase snapshot (including optional broker receipt history). Docs and schema reference describe the partition contract.Reviewed by Cursor Bugbot for commit 064742d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation