fix(maintenance): harden cursor reconciliation proof gates - #3873
Conversation
|
@codex review |
📝 WalkthroughWalkthroughThe planner now handles disappeared selected cursor paths with preserved incomparable authority data. Backup validation verifies content-addressed blobs against inventory, receipts, manifest counts, sizes, hashes, paths, and protection metadata. ChangesReconciliation validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BackupValidation
participant BlobInventory
participant VerificationReceipt
participant Manifest
BackupValidation->>BlobInventory: Read and hash content-addressed blobs
BlobInventory->>VerificationReceipt: Compare path, digest, size, and protection metadata
BlobInventory->>Manifest: Compare blob count and inventory data
BlobInventory-->>BackupValidation: Return validated inventory evidence
BackupValidation-->>BackupValidation: Add manifest digest and inventory summary
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py`:
- Around line 463-495: Update the inventory and blob-path validation around the
reconciliation flow: require manifest["blob_inventory_file"] to be exactly
"blob-inventory.json" before reading it, reject a symlinked blob_root, and
require every blob file to have the canonical path
blob/<blob_hash[:2]>/<blob_hash[2:]> rather than merely matching directory and
filename lengths. Preserve the existing inventory membership and hash validation
checks.
In `@tests/unit/maintenance/test_cursor_authority_reconcile.py`:
- Around line 441-456: Update the successful _validate_backup assertion in the
relevant test to also verify blob_inventory.count, blob_inventory.size_bytes,
and blob_inventory.inventory_digest against the expected evidence values,
placing these assertions immediately after validation and before any blob
mutation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 706fb175-044a-43b2-bbe0-93f4686fe0aa
📒 Files selected for processing (2)
polylogue/maintenance/cursor_authority_reconcile.pytests/unit/maintenance/test_cursor_authority_reconcile.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55dac48c63
ℹ️ 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".
| projection.available | ||
| and projection.cursor_ahead_status == "unknown" |
There was a problem hiding this comment.
When the selected cursor row disappears while incomparable gaps remain, the canonical raw_frontier_integrity_projection sets cursor_ahead_status to "unknown" and consequently sets available to false. _require_healthy_projection_siblings already rejects that projection at line 357, and this added projection.available condition is therefore also impossible to satisfy. The new test preserves available=True via replace, creating a state production cannot return, so the intended not_applicable result still always raises instead.
Useful? React with 👍 / 👎.
| blob_root = root / "blob" | ||
| for path in sorted(blob_root.rglob("*")): | ||
| if path.is_symlink(): |
There was a problem hiding this comment.
If blob/ is replaced with a directory symlink after backup verification, Path.rglob() traverses its target and the yielded child files are not themselves symlinks, so this check accepts the altered backup when the target contains the attested bytes. The apply-time gate therefore fails to enforce that rollback evidence remains inside the verified full-evidence backup; validate blob_root itself with lstat before traversing it.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dd415d3f7
ℹ️ 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".
| if ( | ||
| require_candidate | ||
| and selected_path_disappeared |
There was a problem hiding this comment.
When the caller supplies any existing file that is absent from ingest_cursor while unrelated authority gaps remain, this check treats that absence as proof that the file's cursor row “disappeared.” The path-file input is caller-controlled and the plan contains no earlier cursor census or candidate binding, so the command can emit a not_applicable plan claiming a previously selected violation vanished even when that path was never a cursor row; require evidence tying the path to the prior candidate before making this classification.
Useful? React with 👍 / 👎.
| inventory_path = root / "blob-inventory.json" | ||
| try: | ||
| declared = json.loads(inventory_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
If blob-inventory.json is replaced after verification with a symlink to a file containing the attested bytes, read_text() follows it and every subsequent receipt/blob comparison succeeds, so apply accepts rollback evidence whose inventory is outside the verified backup directory. Check this artifact with lstat/O_NOFOLLOW before parsing it, as the backup producer and migration validator already do for verification artifacts.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
| return { | ||
| "root": _path_identity(root), | ||
| "manifest_sha256": _sha256_file(root / "manifest.json"), | ||
| "blob_inventory": blob_inventory, |
There was a problem hiding this comment.
When manifest.json is modified after verification without changing the fields consumed above—for example, by altering created_at, warnings, or backed_up_files—all validation still succeeds because the current hash is merely copied into the reconciliation evidence and is never compared with the authenticated receipt["manifest_sha256"]. This allows apply to accept a manifest whose bytes were never verified; require equality with the signed receipt before treating the backup as valid.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
Problem: the scoped reconciliation route rejected a disappeared selected cursor row when the known incomparable population remained, and backup validation trusted receipt-level blob claims without re-reading the current backup files. What changed: emit a typed not_applicable plan for that zero-ahead case, re-hash every content-addressed backup blob, compare the live inventory with blob-inventory.json and the authenticated verification receipt, and include the verified inventory summary in reconciliation evidence. Compatibility/migration: the live cursor-authority receipt remains a separate production obligation. No production archive was mutated. Ref polylogue-s8gcr Co-Authored-By: Claude <noreply@anthropic.com>
Problem: The reconciliation backup gate could validate noncanonical inventory/blob locations, and a canonical unavailable projection could not produce the documented no-op plan. What changed: Bind inventory validation to blob-inventory.json, reject symlinked and noncanonical blob paths, and accept only the exact disappeared-cursor unknown projection. Extend real-route tests. Compatibility/migration: No archive data or cursor row is changed. Ref polylogue-s8gcr Co-Authored-By: Claude <noreply@anthropic.com>
6dd415d to
27f8fe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py`:
- Line 595: Move the _validated_blob_inventory call below the
verify_verification_receipt attestation block, ensuring receipt authenticity is
checked before any blob re-hashing occurs. Preserve the existing inventory
result and return-value behavior after the attestation succeeds.
- Around line 537-552: Update the expected-row construction in the
reconciliation function to reject any non-dict entry in expected instead of
filtering it out. Preserve the existing normalization for valid dict rows, but
make malformed receipt rows fail validation before comparing against on-disk
blobs.
In `@tests/unit/maintenance/test_cursor_authority_reconcile.py`:
- Around line 508-510: Update the `pytest.raises` assertion for
`_validate_backup` after `blob_path.unlink()` to match the specific “current
backup blob inventory does not match its verification receipt” error, rather
than the broad `"blob inventory"` substring. Keep the test focused on the
receipt-comparison guard that is actually reached when the blob tree is empty.
- Around line 434-439: Add "blob_count": 1 to the manifest in the relevant test
setup so the production count-validation path executes with a matching count,
then modify the manifest later in the test to use a mismatched count and assert
that the expected exception is raised.
- Around line 236-250: Update
test_planner_refuses_unavailable_projection_with_unknown_sibling so the mutated
projection uses available=True, allowing _require_healthy_projection_siblings to
reach the missing_source_raw_status check. Change the expected exception match
from “unavailable” to the sibling guard’s “not healthy” message, while
preserving the unknown sibling status mutation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b10f99f5-d6a3-42ff-b6e8-cdeba5371266
📒 Files selected for processing (2)
polylogue/maintenance/cursor_authority_reconcile.pytests/unit/maintenance/test_cursor_authority_reconcile.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27f8fe45c6
ℹ️ 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".
| raise CursorAuthorityReconciliationError(f"backup blob path is not canonical: {relative}") | ||
| if blob_hash not in declared_by_hash: | ||
| raise CursorAuthorityReconciliationError("backup contains a blob absent from blob-inventory.json") | ||
| size_bytes, sha256 = _file_fingerprint(path) |
There was a problem hiding this comment.
When a blob is replaced after verification with a hard link to an external file containing the attested bytes, the symlink checks and _file_fingerprint() both succeed, even though the blob no longer provides independent rollback evidence inside the backup. The backup producer's regular-artifact validation rejects files with st_nlink != 1; enforce the same check here before accepting the blob inventory.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
| if expected_rows != actual_rows: | ||
| raise CursorAuthorityReconciliationError( | ||
| "current backup blob inventory does not match its verification receipt" | ||
| ) |
There was a problem hiding this comment.
When a regular blob-inventory.json is modified after verification without changing the projected hash/size/protection fields—for example, by adding an ignored field or merely reformatting it—expected_rows == actual_rows still passes. The signed receipt already records the inventory file's exact SHA-256 under blob_inventory_file, so compare that evidence with the current file before accepting this as the verified rollback inventory.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
| if projection.cursor_ahead_count == 0: | ||
| if require_candidate and projection.cursor_authority_gap_count == 0 and projection.overall_status == "healthy": | ||
| not_applicable_plan: dict[str, object] = { | ||
| "format": PLAN_FORMAT, | ||
| "archive_identity": _path_identity(root), | ||
| "active_index": _active_index_binding(root), | ||
| "code_sha": _code_sha(), | ||
| "deployed_package_sha": _deployed_package_sha(), | ||
| "tier_fingerprints": tiers, | ||
| "source_schema_versions": {tier: tiers[tier]["user_version"] for tier in _REQUIRED_TIERS}, | ||
| "selected_path_digest": path_digest, | ||
| "observed_at_ms": int(time.time() * 1000), | ||
| "status": "not_applicable", | ||
| "cursor_byte_offset": None, | ||
| "accepted_frontier": None, | ||
| "accepted_raw_id_digest": None, | ||
| "source_prefix_digest": None, | ||
| "before_projection": _private_projection(projection), | ||
| } | ||
| not_applicable_plan["plan_digest"] = _canonical_digest(not_applicable_plan) | ||
| return not_applicable_plan | ||
| raise CursorAuthorityReconciliationError("cursor authority is incomparable or has no selected violation") | ||
| current_cursor_paths = {Path(path).resolve() for path, _offset in _cursor_rows(root)} | ||
| selected_path_disappeared = source_path.resolve() not in current_cursor_paths |
There was a problem hiding this comment.
When ops.db exists but its cursor schema is unreadable or missing, the canonical projection degrades to available=False, cursor_ahead_status="unknown", and a zero ahead count; this new branch then calls _cursor_rows() before _require_healthy_projection_siblings(), and the raw sqlite3.Error from its SELECT escapes. The CLI catches only CursorAuthorityReconciliationError, so this maintenance failure produces a traceback instead of the intended typed unavailable-projection error; query the cursor census only for the recognized incomparable case or translate its SQLite errors.
Useful? React with 👍 / 👎.
Reissue the exact carrier head after rebasing onto current master.
Problem: the cursor planner could classify an arbitrary path as a no-op when the projection was unavailable and backup validation trusted malformed or weakly bound receipt evidence.\n\nWhat changed: require healthy projection siblings before cursor-row access, require a current selected cursor row for healthy no-op classification, authenticate manifest and inventory bytes before blob rehashing, reject malformed receipt rows, symlinks, and hard-linked backup artifacts, and extend mutation tests.\n\nCompatibility/migration: no archive schema or production data path changed. The reconciliation route now rejects ambiguous unavailable-projection inputs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f456e2a1a
ℹ️ 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".
| inventory_evidence.get("path") != "blob-inventory.json" | ||
| or inventory_evidence.get("present") is not True | ||
| or inventory_evidence.get("size_bytes") != inventory_metadata.st_size | ||
| or inventory_evidence.get("sha256") != _sha256_file(inventory_path) |
There was a problem hiding this comment.
When blob-inventory.json is a regular single-linked file but cannot be opened—for example, because its read permission was removed—_sha256_file() raises PermissionError here before execution reaches the existing guarded read_text() call. The CLI catches only CursorAuthorityReconciliationError, so applying with this damaged backup produces a traceback rather than the typed “backup blob inventory is unreadable” failure; catch and translate the hash read's OSError as well.
Useful? React with 👍 / 👎.
Problem: the cursor proof retained a dead unavailable-projection helper, its sibling guard test did not reach the unknown-sibling branch, and inventory hash read failures could escape as raw filesystem errors. What changed: remove the unreachable no-op helper, make the sibling test cover the intended guard, and translate inventory hashing I/O failures into the reconciliation error type with a focused regression. Compatibility/migration: fail-closed validation behavior only; no successful backup or reconciliation path changes. Ref polylogue-s8gcr Co-Authored-By: Claude <noreply@anthropic.com>
Problem: A caller-supplied current cursor path could be classified as the disappeared selected violation without evidence that the original plan selected that path.\n\nWhat changed: Require the plan's redacted cursor-ahead sample to contain the selected path digest before apply can enter recovery or no-op handling. Add a focused regression for a mismatched sample.\n\nCompatibility/migration: Existing plans remain valid when their before-projection sample contains the selected path; unrelated paths now fail closed.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af79c380f
ℹ️ 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".
| if not isinstance(samples, list) or not any( | ||
| isinstance(sample, dict) and sample.get("source_path") == selected_path_digest for sample in samples | ||
| ): |
There was a problem hiding this comment.
When a cursor was recorded through a symlink or another non-canonical path spelling, planning still succeeds because _head_details() consistently compares resolved paths, but _private_projection() hashes the sample's original string while this check hashes the now-resolved current_path. Those hashes differ for the same file, so every apply attempt is rejected as lacking a prior violation; normalize source_path before redacting it or preserve a canonical path digest in the plan.
Useful? React with 👍 / 👎.
Problem: A symlinked cursor path was resolved for apply but hashed in its original spelling in the frozen projection, making a valid plan fail closed during apply. What changed: Canonicalize source paths before redacting cursor-authority projection evidence and cover the symlink spelling case in the focused proof suite. Compatibility/migration: No archive or cursor mutation is performed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfab730ae5
ℹ️ 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".
| if ( | ||
| require_candidate | ||
| and projection.overall_status == "healthy" | ||
| and source_path.resolve() in current_cursor_paths | ||
| ): | ||
| not_applicable_reason = "selected cursor-ahead violation is no longer present" |
There was a problem hiding this comment.
When the archive is already healthy, supplying any path that currently has an ingest_cursor row satisfies this condition and produces a not_applicable plan claiming that a selected cursor-ahead violation disappeared, even if that path was never ahead. The fresh evidence after the prior comment is that the final branch now checks only the current healthy projection and row membership; _require_selected_path_in_before_projection cannot help because it runs only while applying a previously planned result. Require prior candidate evidence here, or reject healthy inputs instead of emitting the unsupported audit claim.
Useful? React with 👍 / 👎.
Problem: A healthy archive accepted any current cursor path as a not-applicable proof, even when no earlier cursor-ahead candidate had selected that path. What changed: Healthy zero-gap inputs now fail closed unless they are handled through the existing apply-time prior-plan recovery path. The focused test records the rejection. Compatibility/migration: A previously planned reconciliation still uses its bound recovery checks; no cursor mutation is added.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd265be86e
ℹ️ 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".
| if not path.is_file(): | ||
| continue |
There was a problem hiding this comment.
When a FIFO, socket, or device node is added under blob/ after verification, is_file() returns false and this branch silently skips it, so the authenticated blob-row comparison still passes. Apply can then mutate the archive while relying on a backup whose later copytree restore may fail or hang; reject every non-directory entry unless it is a single-linked regular file, as the backup producer does.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
1 participant
Summary
Harden the scoped cursor-authority reconciliation route with explicit no-op classification and current backup blob verification.
Problem
The route rejected a selected cursor row that disappeared while the known incomparable population remained, so it could not produce a typed no-op plan. Its backup gate also trusted receipt-level blob claims without re-reading the current backup files, allowing post-verification additions, deletions, or content changes to go unnoticed.
Solution
The planner now emits a self-describing
not_applicableplan only when the selected row is gone and the incomparable population is preserved. Backup validation re-hashes every content-addressed blob, checks its path, size, digest, protection metadata, and exact correspondence withblob-inventory.jsonand the authenticated verification receipt. The resulting inventory count, byte total, and digest are carried into the reconciliation evidence. No direct cursor, accepted-head, or source-row repair path was added.Verification
direnv exec . devtools test tests/unit/maintenance/test_cursor_authority_reconcile.py: 24 passed.direnv exec . devtools test tests/unit/sources/test_live_watcher.py -k 'cursor_authority or authoritative_frontier': 2 passed, 109 deselected.direnv exec . devtools test tests/unit/storage/test_raw_retention.py -k cursor: 6 passed, 59 deselected.direnv exec . devtools test tests/unit/cli/test_archive_maintenance_cli.py -k 'cursor_authority or cursor or maintenance_help': 9 passed, 70 deselected.direnv exec . devtools verify --quick: all 24 steps passed.Follow-ups
The live cursor-authority receipt remains open and separate. This PR contains no production archive mutation or live receipt.
Ref polylogue-s8gcr
Summary by CodeRabbit