fix(acquisition): authenticate terminal recovery omissions - #513
Conversation
PR SummaryHigh Risk Overview Verifier-issued capabilities extend recovered-public clearance to carry an authenticated terminal partition and add a terminal-disposition capability minted only after that replay succeeds. Resolved recovered-public logic subtracts terminal keys from required unknown-origin coverage and rejects terminal keys in resolved rows, operation bindings, and parse requests. Cycle manifest templates, schema docs, the official runbook, and tests are updated for the new variables, contracts, and rebinding checks. Reviewed by Cursor Bugbot for commit ccf5635. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
Next review available in: 30 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 (15)
WalkthroughThe recovery disclosure workflow now authenticates terminal-unavailable partitions and terminal-disposition artifacts. Capabilities and source snapshots flow through resolution, materialization, publishing, and replacement-source generation with fail-closed validation. ChangesTerminal disposition recovery
Estimated code review effort: 4 (Complex) | ~60 minutes 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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ccf5635. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccf5635e4d
ℹ️ 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.
Pull request overview
This PR tightens the acquisition pipeline’s handling of “terminal-unavailable” recovery outcomes by requiring an independently replayed terminal-disposition capability before allowing terminal documents to be omitted from resolved post-recovery outputs, and by binding the relevant terminal partition and verifier-captured bytes through run cards and replacement-source production.
Changes:
- Introduces a terminal-disposition capability boundary and integrates it into resolved post-recovery build/require paths (including parser-request and operation-binding checks).
- Extends CLI contracts, manifests, and run-card commitments to include a four-file terminal-disposition source bundle and the recovery terminal partition commitment.
- Adds focused tests and updates schema/runbook documentation to enforce the new fail-closed behavior and immutability guarantees.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_resolved_post_recovery.py | Adds coverage for omitting terminal partitions only with independently replayed terminal-disposition authority and for drift-precedence behavior. |
| tests/test_replacement_recovery_source_producer.py | Extends producer contract tests to bind and reject rebinding/omission of terminal partition inputs and terminal disposition bundle requirements. |
| tests/test_recovery_disclosure_continuation.py | Ensures disclosure continuation wiring includes the terminal-disposition flags/assignments. |
| tests/test_recovered_public_capability.py | Adds tests validating immutable terminal partition behavior and equality requirements for terminal-disposition capabilities. |
| tests/test_cycle_manifest_template_cli.py | Updates template-variable coverage to include terminal-disposition inputs. |
| tests/recovered_public_capability_helpers.py | Extends test helpers to issue recovered-public and terminal-disposition capabilities for downstream tests. |
| manifests/cycle-1-target-100.initial-recovery-disclosure.template.json | Adds terminal-disposition variables/flags to the resolve stage. |
| manifests/cycle-1-target-100.initial-recovery-disclosure-no-review.template.json | Adds terminal-disposition variables/flags to the resolve stage (no-review variant). |
| legalforecast/ingestion/resolved_post_recovery.py | Implements terminal-disposition capability boundary and integrates terminal-key subtraction / overlap checks into core build/require flows. |
| legalforecast/ingestion/replacement_recovery_source.py | Adds terminal partition commitments and disposition-source path validation to resolved-source coordinate derivation. |
| legalforecast/ingestion/provenance_clearance.py | Extends recovered-public capability to include a verifier-owned terminal-unavailable partition (path/bytes/hash/keys). |
| legalforecast/cli.py | Adds terminal-disposition CLI flags, replays/verifies terminal disposition sources, commits terminal partition + disposition sources, and enforces drift checks before mutation/publication. |
| docs/schemas/replacement-recovery-source-producer-v1.md | Documents the terminal partition evidence vs. omission authority separation and the additional terminal-disposition bundle requirements. |
| docs/schemas/provenance-clearance-v1.md | Documents that terminal-unavailable ledgers do not authorize omission absent a separate terminal-disposition capability. |
| docs/official-run-runbook.md | Updates operational commands/templates to supply the terminal-disposition bundle and explains the all-or-none/mandatory semantics. |
Suppressed comments (1)
legalforecast/cli.py:49201
- This only enforces that terminal-disposition inputs are present when the recovery’s terminal-unavailable partition is non-empty, but it still allows supplying the terminal-disposition bundle when (a) there is no recovered-public capability (so issuing the terminal capability will fail later), or (b) the partition exists but has
record_count == 0. Case (b) also causes downstream inconsistency: the resolver run card will committerminal_disposition_sources, butbuild-replacement-recovery-sourcerejects disposition sources when the terminal ledger is empty.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ccf5635 to
310584c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
legalforecast/cli.py:49295
resolve-post-recovery-documentscurrently accepts the--terminal-*disposition bundle even when there is no recovered-public recovery capability (or when the recovered terminal ledger is empty). In that caserecovery_capabilityisNone, but the code still tries to mint_issue_terminal_disposition_capability(... verified_recovery_capability=recovery_capability), which will raise later with a confusing error and/or emit a run card that downstream replacement-source verification rejects. Add an explicit fail-closed validation here: terminal-disposition inputs must only be allowed when a recovered-public capability is present and the recovered terminal ledger is non-empty.
310584c to
537bd85
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
legalforecast/ingestion/provenance_clearance.py:188
terminal_unavailable_pathis treated as required here, but the current failure message is misleading when the field is missing/None (it reports a path mismatch rather than a missing commitment). Splitting the check makes failures easier to diagnose (especially when processing legacy/older recovery evidence that may not emit the terminal ledger field).
terminal_unavailable_path = recovery.get("terminal_unavailable_path")
if not isinstance(terminal_unavailable_path, Path) or (
terminal_unavailable_path.resolve()
!= (recovery_root / "terminal-unavailable-operations.jsonl").resolve()
):
raise ProvenanceClearanceError(
"recovered-public recovery committed different terminal unavailable path"
)
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
legalforecast/cli.py (1)
40266-40302: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not return filesystem fallback reads as authenticated snapshots.
snapshot_byteswrites its fallback result intoverified_bytesat line 40274. Line 40302 then returns the same dict as_verified_clearance_source_snapshots. Fallback entries come from_read_singly_linked_regular_input, not from a verifier capability.Trace the effect through the resolver command:
- Line 49363 converts the mapping into
clearance_source_snapshots.- Line 49506 merges it into
authenticated_source_snapshots.- Lines 49240 and 49568 pass it to
_require_snapshot_unchangedunder the label"resolved post-recovery authenticated source".- Line 49586 records
_bytes_sha256of those bytes as a source commitment.For fallback entries, the immutability check only proves the file did not change after the CLI read it. It does not prove verifier provenance. The label and the key name both claim more than is established.
Keep the verifier-owned bytes separate from the read-through cache.
🔒️ Proposed separation
recovery_capability = lineage["authenticated_recovery_capability"] for path, payload in _consume_recovered_public_source_snapshots( recovery_capability ).items(): key = os.path.abspath(path) existing = verified_bytes.get(key) if existing is not None and existing != payload: raise CommandError( "recovered-public clearance and recovery snapshots conflict" ) verified_bytes[key] = payload + authenticated_keys = frozenset(verified_bytes) def snapshot_bytes(path: Path) -> bytes: key = os.path.abspath(path) try: return verified_bytes[key] except KeyError: payload = _read_singly_linked_regular_input( path, label="verified recovered-public downstream input" ) verified_bytes[key] = payload return payload @@ - "_verified_clearance_source_snapshots": verified_bytes, + "_verified_clearance_source_snapshots": { + key: payload + for key, payload in verified_bytes.items() + if key in authenticated_keys + },🤖 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/cli.py` around lines 40266 - 40302, Keep filesystem fallback bytes from snapshot_bytes separate from verified_bytes: use a distinct read-through cache for _read_singly_linked_regular_input results, while leaving verified_bytes exclusively for verifier-owned snapshots. Ensure _verified_clearance_source_snapshots and the downstream authenticated_source_snapshots/_require_snapshot_unchanged flow receive only genuinely verifier-authenticated bytes, without changing the existing artifact projection behavior.
🧹 Nitpick comments (8)
tests/test_recovered_public_capability.py (1)
280-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a subset case for the partition equality rule.
The test rejects a fully disjoint disposition partition. Line 155 of
legalforecast/ingestion/resolved_post_recovery.pyrequires exact set equality. The likely production drift is a subset or superset, not a disjoint set. Add a case where the disposition omits one pair that the recovery partition contains. That case pins the equality rule and prevents a later relaxation to a subset test, which would omit a document without independent replay.♻️ Proposed additional case
+def test_terminal_disposition_capability_rejects_partial_partition( + monkeypatch: pytest.MonkeyPatch, +) -> None: + schema = "legalforecast.recap_fetch_terminal_unavailable.v1" + first = { + "schema_version": schema, + "candidate_id": "case-a", + "source_document_id": "123", + } + second = { + "schema_version": schema, + "candidate_id": "case-b", + "source_document_id": "456", + } + capability = issue_recovered_public_capability( + monkeypatch, + [], + terminal_records=[first, second], + ) + + with pytest.raises( + resolved_module.ResolvedPostRecoveryError, + match="differs from recovery terminal partition", + ): + issue_terminal_disposition_capability(monkeypatch, capability, [first])🤖 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_recovered_public_capability.py` around lines 280 - 296, Add a second assertion in the existing ResolvedPostRecoveryError test that passes a disposition partition containing all but one pair from the recovery terminal partition, and verify it still raises with the same “differs from recovery terminal partition” message. Keep the existing disjoint-partition case unchanged so both non-equal partition shapes enforce exact set equality.legalforecast/ingestion/resolved_post_recovery.py (1)
775-778: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject the terminal-disposition capability where the branch cannot use it. Both functions consume
verified_terminal_disposition_capabilityonly inside theverified_recovery_capability is not Nonebranch. When a caller passes a terminal-disposition capability together withverified_lineage_capability, or on the legacydisclosure_authoritypath, the argument is silently ignored. No unauthorized omission occurs, becauseterminal_keysstays empty and every unknown-selected document remains required. The gap is a contract gap: the caller believes it granted omission authority, and the function proceeds without it. Line 769 already rejects the analogous conflicting pair with "conflicting authenticated clearance capabilities", so make the ignored combination explicit the same way.
legalforecast/ingestion/resolved_post_recovery.py#L775-L778: in_build_resolved_post_recovery_documents_core, raiseResolvedPostRecoveryErrorwhenverified_terminal_disposition_capability is not Noneandverified_recovery_capability is None.legalforecast/ingestion/resolved_post_recovery.py#L1114-L1117: apply the same guard in_require_resolved_post_recovery_documents_core, so the builder and the requirement check agree on the accepted capability combinations.♻️ Proposed guard, shown for the builder
recovered_lineages: Mapping[tuple[str, str], Mapping[str, object]] | None = None terminal_keys: frozenset[tuple[str, str]] = frozenset() + if ( + verified_terminal_disposition_capability is not None + and verified_recovery_capability is None + ): + raise ResolvedPostRecoveryError( + "terminal disposition authority requires recovered-public authority" + ) if verified_recovery_capability is not None:🤖 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/resolved_post_recovery.py` around lines 775 - 778, Reject terminal-disposition capabilities when no verified recovery capability can consume them. In legalforecast/ingestion/resolved_post_recovery.py#L775-L778, update _build_resolved_post_recovery_documents_core to raise ResolvedPostRecoveryError when verified_terminal_disposition_capability is set and verified_recovery_capability is None, matching the existing conflicting-capability guard. Apply the same guard in _require_resolved_post_recovery_documents_core at legalforecast/ingestion/resolved_post_recovery.py#L1114-L1117 so both paths enforce identical capability combinations.legalforecast/cli.py (5)
49567-49571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a distinct label for the post-publication drift check.
Line 49240 and line 49568 both call
_require_snapshot_unchangedwith the label"resolved post-recovery authenticated source". The first call guards publication. This call guards completion metadata and runs after the journal is already mutated. When this second check fails, the operator cannot tell from the message which phase detected the drift.Give this call a label such as
"resolved post-recovery completion source".🤖 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/cli.py` around lines 49567 - 49571, Update the _require_snapshot_unchanged call in the post-recovery completion-metadata path to use a distinct label, such as “resolved post-recovery completion source,” while leaving the earlier publication-phase label unchanged.
49549-49571: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the redundant path re-resolution and the identity dict copy.
_merge_authenticated_source_snapshotsalready resolves every key at line 49260. Three follow-on operations repeat that work:
- Line 49550 resolves keys that are already resolved.
- Line 49561 resolves a key taken from
authenticated_snapshot_by_path, which is already resolved.- Line 49569 rebuilds
authenticated_snapshot_by_pathinto an identical dict.Lines 49584-49588 add three more
.resolve()calls per input. Each.resolve()performs filesystem syscalls, so the completion path repeats stat work for every input.Precompute one resolved path per input and reuse it.
♻️ Proposed simplification
- authenticated_snapshot_by_path = { - path.resolve(): payload - for path, payload in authenticated_source_snapshots.items() - } + authenticated_snapshot_by_path = dict(authenticated_source_snapshots) base_input_resolved = {path.resolve() for path in base_input_paths} input_paths = ( *base_input_paths, *tuple( sorted( ( path for path in authenticated_snapshot_by_path - if path.resolve() not in base_input_resolved + if path not in base_input_resolved ), key=str, ) ), ) if authenticated_snapshot_by_path: _require_snapshot_unchanged( - {path: payload for path, payload in authenticated_snapshot_by_path.items()}, + authenticated_snapshot_by_path, label="resolved post-recovery authenticated source", )🤖 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/cli.py` around lines 49549 - 49571, Update the input-path assembly around _merge_authenticated_source_snapshots to reuse resolved paths: keep authenticated_snapshot_by_path keys as already-resolved paths, avoid resolving them again when filtering, and pass that same dictionary directly to _require_snapshot_unchanged instead of rebuilding it. Precompute each base input’s resolved path once and reuse those values through the remaining completion-path checks, including the logic around lines 49584-49588.
49307-49326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one set of terminal-disposition input helpers between both commands. Each command re-implements the collection of the four terminal-disposition paths and the rules that govern them. The duplication has already diverged in three ways: the error type (
CommandErroragainstReplacementRecoverySourceError), the error text, and the path normalization (.absolute()applied at lines 25772-25775 but not at line 49323). A future change to the bundle contract must be applied twice.
legalforecast/cli.py#L49307-L49326: extract this block into a helper, for example_collect_terminal_disposition_paths(args), that reads the four attributes, rejects a partial bundle, and returns normalized absolute paths. Call the helper here.legalforecast/cli.py#L25760-L25766: replace this inline collection and guard with the same helper, and keep the.absolute()normalization inside the helper so both commands behave identically.legalforecast/cli.py#L49270-L49281: call_require_terminal_disposition_bundlefrom the replacement-recovery-source command in place of the inline XOR check at lines 26063-26067, and let the helper accept the error type the caller needs.🤖 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/cli.py` around lines 49307 - 49326, In legalforecast/cli.py, update lines 49307-49326 to use a shared terminal-disposition helper that collects all four attributes, rejects partial bundles, and returns absolute paths; replace the duplicate collection and guard at lines 25760-25766 with the same helper, keeping normalization there. At lines 49270-49281, call _require_terminal_disposition_bundle instead of the inline XOR check at lines 26063-26067, allowing the helper to receive the caller-specific error type while preserving the required validation behavior.
36790-36801: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the authenticated source snapshot private to the verifier.
VerifiedTerminalPurchaseDispositionAuthorityis the verifier type and still owns those bytes through_terminal_failure_authority; expose only a publicsource_snapshotsaccessor here if callers need the mapping.🤖 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/cli.py` around lines 36790 - 36801, Update the source snapshot construction near VerifiedTerminalPurchaseDispositionAuthority so authenticated source bytes remain private within the verifier’s _terminal_failure_authority. Expose only the public source_snapshots accessor for callers that need the mapping, and remove direct access to the private purchase result, run card, and budget plan fields.
49444-49468: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the same input reader and journal mode as the terminal disposition replay path.
The
_read_singly_linked_regular_inputhelper rejects existing parent symlinks before opening the file;read_unique_regular_filedoes not do this for each parent fd. This selection file determinesselected_document_count, so use the same helper or a reader with the same parent-symlink policy. The verification helpers also only need journal access through the same read-only journal used at the other disposition replay site.🤖 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/cli.py` around lines 49444 - 49468, The disposition selection flow should use the same secure input reader and read-only journal mode as the terminal disposition replay path. Replace read_unique_regular_file in the code around _verify_materializer_docket_decision_authority with _read_singly_linked_regular_input, or an equivalent reader that rejects symlinked parent components, and pass the corresponding read-only journal to the verification helpers while preserving selected_document_count calculation.legalforecast/ingestion/replacement_recovery_source.py (1)
330-351: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConstrain the expected disposition-source key set, not only the card key set.
Line 330 requires the card's
terminal_disposition_sourcesto hold exactly the four names. The loop at line 343 then iteratesexpected_terminal_disposition_paths.items(). The code never requiresexpected_terminal_disposition_pathsto hold those same four names. If a caller supplies a partial mapping, the missing committed source paths are accepted without any comparison. Bind the expectation set to the same literal names so the authenticator cannot be weakened by an incomplete caller argument.♻️ Proposed fix to bind both key sets
+_TERMINAL_DISPOSITION_SOURCE_NAMES = frozenset( + {"selection", "snapshot_manifest", "purchase_result", "purchase_run_card"} +)if ( - set(disposition_sources) - != { - "selection", - "snapshot_manifest", - "purchase_result", - "purchase_run_card", - } - or expected_terminal_disposition_paths is None + set(disposition_sources) != _TERMINAL_DISPOSITION_SOURCE_NAMES + or expected_terminal_disposition_paths is None + or set(expected_terminal_disposition_paths) + != _TERMINAL_DISPOSITION_SOURCE_NAMES ): raise ReplacementRecoverySourceError( "resolved terminal disposition sources differ" )🤖 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/replacement_recovery_source.py` around lines 330 - 351, Require expected_terminal_disposition_paths to contain exactly the same four keys as disposition_sources before validating paths in the loop. Update the condition around the existing set comparison to reject partial or extra expectation mappings while preserving the current path comparisons.
🤖 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/cli.py`:
- Around line 36785-36789: Normalize both operands of the budget-plan comparison
in the terminal purchase validation, using the same path normalization that
resolves relative paths, symlinks, and `..` segments before comparing the
run-card value with `budget_plan_path`. Update the comparison near
`_purchase_budget_plan_path` while preserving the existing CommandError for
mismatches.
- Around line 26159-26162: Align the input paths passed into
derive_resolved_source_coordinates with the expected input order before
resolving commitments. Update the construction around terminal_unavailable_path,
clearance_coordinates.clearance_path, clearance_card_path, and
terminal_disposition_paths so card["input_paths"] follows
expected_resolved_inputs, ensuring each resolved input_sha256 matches the
corresponding final zip position.
- Around line 26125-26132: Update the terminal disposition flow around
_issue_terminal_disposition_capability to first verify that the clearance
lineage is provider_free_recovered_public and that
authenticated_recovery_capability is present before accessing
recovery_capability or issuing the capability. Preserve the existing behavior
for other clearance lineages by preventing capability issuance without raising
KeyError.
In `@legalforecast/ingestion/provenance_clearance.py`:
- Around line 194-222: The recovered-public artifact lookup in
_consume_recovered_public_clearance_capability must use canonical resolve-based
path keys consistently. Replace the os.path.abspath key used to extract
terminal_bytes with Path(...).resolve() and ensure verified_artifact_bytes
parsing/building uses the same resolved keys, while preserving the existing
resolved-path comparison for run-card entries.
In
`@manifests/cycle-1-target-100.initial-recovery-disclosure-no-review.template.json`:
- Line 1: Remove the terminal-disposition bundle arguments and variable
declarations from both
manifests/cycle-1-target-100.initial-recovery-disclosure-no-review.template.json:1-1
and manifests/cycle-1-target-100.initial-recovery-disclosure.template.json:1-1.
In each resolve-post-recovery-documents stage, remove
TERMINAL_DISPOSITION_SELECTION, TERMINAL_DISPOSITION_SNAPSHOT_MANIFEST,
TERMINAL_PURCHASE_RESULT, and TERMINAL_PURCHASE_RUN_CARD arguments, and remove
the same four entries from the template variables list; leave all other stages
and arguments unchanged.
---
Outside diff comments:
In `@legalforecast/cli.py`:
- Around line 40266-40302: Keep filesystem fallback bytes from snapshot_bytes
separate from verified_bytes: use a distinct read-through cache for
_read_singly_linked_regular_input results, while leaving verified_bytes
exclusively for verifier-owned snapshots. Ensure
_verified_clearance_source_snapshots and the downstream
authenticated_source_snapshots/_require_snapshot_unchanged flow receive only
genuinely verifier-authenticated bytes, without changing the existing artifact
projection behavior.
---
Nitpick comments:
In `@legalforecast/cli.py`:
- Around line 49567-49571: Update the _require_snapshot_unchanged call in the
post-recovery completion-metadata path to use a distinct label, such as
“resolved post-recovery completion source,” while leaving the earlier
publication-phase label unchanged.
- Around line 49549-49571: Update the input-path assembly around
_merge_authenticated_source_snapshots to reuse resolved paths: keep
authenticated_snapshot_by_path keys as already-resolved paths, avoid resolving
them again when filtering, and pass that same dictionary directly to
_require_snapshot_unchanged instead of rebuilding it. Precompute each base
input’s resolved path once and reuse those values through the remaining
completion-path checks, including the logic around lines 49584-49588.
- Around line 49307-49326: In legalforecast/cli.py, update lines 49307-49326 to
use a shared terminal-disposition helper that collects all four attributes,
rejects partial bundles, and returns absolute paths; replace the duplicate
collection and guard at lines 25760-25766 with the same helper, keeping
normalization there. At lines 49270-49281, call
_require_terminal_disposition_bundle instead of the inline XOR check at lines
26063-26067, allowing the helper to receive the caller-specific error type while
preserving the required validation behavior.
- Around line 36790-36801: Update the source snapshot construction near
VerifiedTerminalPurchaseDispositionAuthority so authenticated source bytes
remain private within the verifier’s _terminal_failure_authority. Expose only
the public source_snapshots accessor for callers that need the mapping, and
remove direct access to the private purchase result, run card, and budget plan
fields.
- Around line 49444-49468: The disposition selection flow should use the same
secure input reader and read-only journal mode as the terminal disposition
replay path. Replace read_unique_regular_file in the code around
_verify_materializer_docket_decision_authority with
_read_singly_linked_regular_input, or an equivalent reader that rejects
symlinked parent components, and pass the corresponding read-only journal to the
verification helpers while preserving selected_document_count calculation.
In `@legalforecast/ingestion/replacement_recovery_source.py`:
- Around line 330-351: Require expected_terminal_disposition_paths to contain
exactly the same four keys as disposition_sources before validating paths in the
loop. Update the condition around the existing set comparison to reject partial
or extra expectation mappings while preserving the current path comparisons.
In `@legalforecast/ingestion/resolved_post_recovery.py`:
- Around line 775-778: Reject terminal-disposition capabilities when no verified
recovery capability can consume them. In
legalforecast/ingestion/resolved_post_recovery.py#L775-L778, update
_build_resolved_post_recovery_documents_core to raise ResolvedPostRecoveryError
when verified_terminal_disposition_capability is set and
verified_recovery_capability is None, matching the existing
conflicting-capability guard. Apply the same guard in
_require_resolved_post_recovery_documents_core at
legalforecast/ingestion/resolved_post_recovery.py#L1114-L1117 so both paths
enforce identical capability combinations.
In `@tests/test_recovered_public_capability.py`:
- Around line 280-296: Add a second assertion in the existing
ResolvedPostRecoveryError test that passes a disposition partition containing
all but one pair from the recovery terminal partition, and verify it still
raises with the same “differs from recovery terminal partition” message. Keep
the existing disjoint-partition case unchanged so both non-equal partition
shapes enforce exact set equality.
🪄 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: defc71d0-3e3a-407d-a7d0-b7e0ddb1558e
📒 Files selected for processing (15)
docs/official-run-runbook.mddocs/schemas/provenance-clearance-v1.mddocs/schemas/replacement-recovery-source-producer-v1.mdlegalforecast/cli.pylegalforecast/ingestion/provenance_clearance.pylegalforecast/ingestion/replacement_recovery_source.pylegalforecast/ingestion/resolved_post_recovery.pymanifests/cycle-1-target-100.initial-recovery-disclosure-no-review.template.jsonmanifests/cycle-1-target-100.initial-recovery-disclosure.template.jsontests/recovered_public_capability_helpers.pytests/test_cycle_manifest_template_cli.pytests/test_recovered_public_capability.pytests/test_recovery_disclosure_continuation.pytests/test_replacement_recovery_source_producer.pytests/test_resolved_post_recovery.py
|
[awt-judge] Opus decision Decision: block_for_fixes Codex action itemsAction Items for CodexCritical Blockers (must fix before merge)
Recommended before merge (small, related to the blockers)
Optional / deferrable (fold in if convenient; otherwise fine as follow-up)
Expected Outcome
Codex follow-up guidancePrioritize getting the required Python quality gates check green first (item 1), then fix blockers 2-5, which are the correctness/security defects. Verify each CodeRabbit finding against current head before editing (some may already be partially addressed); skip any that no longer apply with a one-line reason. Items 6-7 are cheap hardening tied to the same invariants and should ideally land together. Items 8-15 may be deferred to a follow-up issue if they would expand scope, but 8 and 9 are low-effort and worth including. |
537bd85 to
33d75c4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
legalforecast/cli.py:49373
- Building
clearance_source_snapshotsvia a dict comprehension can silently drop entries when two distinct snapshot keys (e.g., symlink vs real path) normalize to the samePath.resolve()result. That would defeat the “fail closed” intent by potentially ignoring conflicting authenticated bytes.
Convert the mapping with an explicit collision check on resolved_path and raise if two inputs resolve to the same path with different payloads.
legalforecast/cli.py:49379
_consume_recovered_public_terminal_partition(...)raisesProvenanceClearanceErrorwhen the recovery capability is invalid, but this call happens outside the maintry/exceptthat wraps errors intoCommandError. That can leak an uncaught exception/traceback instead of a CLI-friendly failure.
Wrap the consume call (or move it inside the existing try and include ProvenanceClearanceError in the handled exceptions).
tests/recovered_public_capability_helpers.py:45
- When
legacy_without_terminal_ledger=True, this test helper setsterminal_path=Nonebut still populatesterminal_sha256with the digest of empty bytes. In production,_authenticate_recovered_public_lineage_from_raw_evidencesets bothterminal_pathandterminal_sha256toNonewhen there is no terminal ledger, so this helper diverges from real capability state and could mask bugs in callers that assume these fields are paired.
terminal_path=terminal_path,
terminal_sha256=hashlib.sha256(terminal_bytes).hexdigest(),
source_snapshots=(
legalforecast/cli.py:36780
purchase_run_card_bytes/purchase_result_bytesare now taken from private fields onterminal_authority, but the function never asserts those embedded paths still match the caller-providedpurchase_result_path/purchase_run_card_path. Adding an explicit resolved-path equality check would make “fail closed on path rebound” more direct and avoids committing bytes under unexpected paths if the authority normalizes or rewrites paths internally.
This issue also appears on line 49369 of the same file.
|
[awt-judge] Codex pushed after 28 minutes. Replying to: #513 (comment) |
|
[awt-judge] Opus decision Decision: approve_with_issue Follow-up artifact (if needed)Title: Consider normalizing both sides of the terminal purchase budget-plan path comparison ContextNon-blocking follow-up from PR #513 review (CodeRabbit finding r3732476031). In Why this is deferrable (not a blocker)The author intentionally uses lexical ( Suggested actionEvaluate whether an authority could legitimately store a relative path or a symlinked path for the budget plan. If so, normalize both operands consistently (either both Acceptance
|
Summary
Validation
uv run ruff format --check ...uv run ruff check ...uv run pyright(0 errors, 0 warnings)git diff --checkFixes LegalForecastBench-5qd6.118.16.31
Summary by CodeRabbit
New Features
Bug Fixes
Tests