refactor(ingestion): extract docket-gap path validation - #451
Conversation
WalkthroughThe planner now provides shared validation for snapshot and writable output paths. The CLI uses this validator and converts planning failures to ChangesOpinion docket gap path validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 refactors opinion docket-gap writable-path validation out of the CLI and into the provider-free planner domain module so the same validation rules can be imported and reused outside the command layer, while preserving the CLI error contract.
Changes:
- Added
validate_opinion_docket_gap_pathstolegalforecast.ingestion.opinion_docket_gap_plannerand raised domain-specificOpinionDocketGapPlanningErrorwith the same user-facing messages as before. - Simplified the CLI
_validate_opinion_docket_gap_pathsto delegate to the planner helper and adapt errors toCommandError. - Relocated/expanded tests so domain behavior is covered directly (including the “new outputs” fast path that avoids a snapshot walk) and the CLI test now asserts the adapter contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
legalforecast/ingestion/opinion_docket_gap_planner.py |
Introduces reusable domain-level path validation for snapshot containment, distinct outputs, and alias detection. |
legalforecast/cli.py |
Replaces duplicated CLI validation logic with a thin adapter around the planner helper. |
tests/test_opinion_docket_gap_planner.py |
Adds direct domain tests for the extracted validation helper, including the “no snapshot walk for new outputs” fast path. |
tests/test_opinion_docket_gap_cli.py |
Updates CLI test to assert the CommandError mapping contract is preserved. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 `@legalforecast/ingestion/opinion_docket_gap_planner.py`:
- Around line 41-44: Update validate_opinion_docket_gap_paths() to catch
exceptions from both snapshot_path.resolve() and each writable_paths
path.resolve(), translating symlink-loop RuntimeError and other filesystem
resolution failures into OpinionDocketGapPlanningError. Add regression tests
covering failures while resolving the snapshot path and output paths, while
preserving successful path validation behavior.
🪄 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: 30b6ee17-2aa1-4374-a0d6-98db4fb64c1d
📒 Files selected for processing (4)
legalforecast/cli.pylegalforecast/ingestion/opinion_docket_gap_planner.pytests/test_opinion_docket_gap_cli.pytests/test_opinion_docket_gap_planner.py
Summary
CommandErroradapter and unchanged messagesValidation
uv run ruff format --check .uv run ruff check .uv run pyrightuv run pytest -q tests/test_opinion_docket_gap_planner.py tests/test_opinion_docket_gap_cli.py tests/test_multiharness_command_adapter.py::test_systemd_scope_preserves_duplex_protocol_and_defers_provider_values(45 passed)uv run pytest -q(6099 passed, 13 skipped)Bead:
LegalForecastBench-cwdv.3Note
Low Risk
Pure refactor with behavior preserved; changes are localized path-validation and test relocation with no auth, data, or execution-path changes.
Overview
Writable-path checks for
plan-opinion-docket-gapsnow live invalidate_opinion_docket_gap_pathson the opinion docket-gap planner module, so the same rules can be imported outside the CLI.The CLI keeps a thin
_validate_opinion_docket_gap_pathsthat delegates to that function and mapsOpinionDocketGapPlanningErrortoCommandErrorwith unchanged user-facing messages. Behavior is unchanged: outputs must stay outside the immutable snapshot, plan/summary paths must be distinct, hard-linked duplicates are rejected, and existing outputs are checked against snapshot files viasamefile.Planner tests now cover the domain helper directly (including the fast path that skips snapshot
rglobwhen outputs do not exist yet); the CLI test asserts theCommandErroradapter contract.Reviewed by Cursor Bugbot for commit 335c8e2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit