fix(maintenance): add scoped cursor authority reconciliation - #3860
Conversation
Problem: live ingestion fails closed when a cursor is ahead of accepted byte authority, leaving no scoped recovery route. What changed: add digest-bound dry-run planning and full-evidence backup validation; route apply through LiveBatchProcessor's normal full-ingest path under a context-local single-use path and frontier authorization; emit atomic plan and receipt evidence without direct cursor or accepted-head writes. Compatibility/migration: dry-run is default and resolves only /realm/db/polylogue; apply requires the daemon stopped, an immutable plan, a verified full_evidence backup, and a new receipt. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: the scoped reconciliation route rejected the real incomparable cursor population and did not model the typed deferred outcome. What changed: preserve incomparable rows while binding the one true ahead row, make plans immutable, validate the unchanged gap census, and expand behavior tests for the scoped authorization and backup boundary. Compatibility/migration: the command remains dry-run by default and production apply remains backup-gated. Ref polylogue-cursor-authority-reconcile-implementation Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a backup-gated ChangesCursor authority reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant MaintenanceCLI
participant Reconciliation
participant ArchiveSQLite
participant LiveBatchProcessor
Operator->>MaintenanceCLI: run reconciliation command
MaintenanceCLI->>Reconciliation: build plan or apply plan
Reconciliation->>ArchiveSQLite: validate archive, cursor, and backup evidence
Reconciliation->>LiveBatchProcessor: ingest selected source with scoped authorization
LiveBatchProcessor->>ArchiveSQLite: validate gate and persist normal ingestion
Reconciliation->>Operator: return plan or receipt
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 511-525: Update the reconciliation receipt construction in the
recovery and typed-deferred branches to avoid asserting a fixed changed_rows
count: derive the cursor change from the before/after projections or record the
count as null when it cannot be verified. Validate and retrieve
plan["before_projection"] once before either branch, reusing the existing
defensive validation used near the earlier access, so missing or invalid data
raises CursorAuthorityReconciliationError instead of KeyError.
- Around line 432-453: Update _find_recovery_attempt and its caller
apply_reconciliation to require a completed ingest_attempts record whose
relevant timestamp is later than the reconciliation plan’s observation time,
passing that plan timestamp into the lookup. When such a recovery is found, mark
the receipt outcome as observed rather than performed and do not report it as
this command’s reconciliation or claim a locally changed cursor row.
- Around line 117-124: Update _sqlite_snapshot to open the tier file once and
derive both the SHA-256 digest and byte size from that same file descriptor,
using os.fstat for the size and hashing the opened stream. Remove the separate
path.stat() and _sha256_file(path) observations while preserving the returned
size_bytes and sha256 fields.
- Around line 420-425: Extract the repeated daemon precondition from both entry
points around the plan and apply flows into one shared helper, reusing
_archive_root, the minimal Config construction, running_daemon_pid, and
CursorAuthorityReconciliationError. Replace both inline guards with calls to
that helper so both operations consistently require the daemon to be stopped.
- Around line 75-96: Update _read_private_source_path to open path_file once and
retain its descriptor, use os.fstat on that descriptor for the regular-file,
single-link, ownership, and mode checks, then read the path contents from the
same descriptor instead of calling lstat followed by read_text. Preserve the
existing validation errors and path-resolution behavior.
- Around line 356-381: Require verification to be a dictionary containing
source_blobs_resolved, index_attachment_blobs_resolved, and
blob_inventory_exact, with all three values true; reject missing or incomplete
verification before the blob path checks in the reconciliation validation flow.
Also guard the backup_tier fingerprint file access in the tier loop so a missing
tier file is converted into CursorAuthorityReconciliationError with the existing
mismatch/error handling rather than allowing FileNotFoundError to escape.
- Around line 395-416: Update _write_atomic_json so refuse_existing uses an
atomic non-overwriting publication, such as linking temporary_path to path and
handling FileExistsError as CursorAuthorityReconciliationError, instead of
os.replace. Preserve os.replace for the overwrite-allowed case and retain
temporary-file cleanup and directory fsync behavior.
- Around line 245-249: Update the reconciliation logic around the frontier and
cursor-offset extraction to convert invalid or missing data into
CursorAuthorityReconciliationError. Validate head[5] and raw[3] before integer
conversion, and replace the unguarded next(...) lookup in _cursor_rows with
explicit missing-row handling that raises the typed error, preserving
apply_reconciliation’s recovery path.
- Around line 536-559: Update the post-ingest validation flow in
apply_reconciliation after _normal_ingest returns so every unexpected
postcondition failure writes the standard audit receipt before raising. Reuse
the existing receipt construction and persistence path, recording a
typed_deferred/failure verdict with the observed metrics and projection state,
then raise CursorAuthorityReconciliationError. Cover failures for raw-frontier
worsening, invalid cursor-ahead reconciliation, and changed pre-existing cursor
population.
- Around line 169-195: Update _private_projection to redact logical_source_key
with the same established digest used for source_path, and make both sample
branches retain dictionary samples even when source_path is missing, setting it
to None consistently. Also apply the logical_source_key redaction in
_head_details so plans and receipts never persist the live provider session
identifier.
In `@tests/unit/cli/test_archive_maintenance_cli.py`:
- Around line 1729-1742: Expand
test_cursor_authority_reconcile_cli_exposes_only_scoped_inputs to assert --plan
and --receipt, then add focused CLI invocations covering missing required
options and invalid mixed dry-run/apply combinations. Verify each invocation
returns the expected validation failure while preserving the existing
help-output scope assertions.
In `@tests/unit/maintenance/test_cursor_authority_reconcile.py`:
- Around line 232-247: Extend
test_backup_validation_requires_blob_rollback_evidence or add focused tests that
create the required blob directory and backup tier files, then cover
_validate_backup fingerprint mismatches and byte-level re-hash failures with the
expected reconciliation errors. Add tests for apply_reconciliation covering both
the recovered-attempt branch and the typed_deferred verdict, including their
expected state and outcomes.
- Around line 204-208: Update the SQLite mutation block in the cursor
reconciliation test to import and use contextlib.closing around sqlite3.connect,
while retaining the existing transaction context so changes are committed and
the connection is deterministically closed before the code under test reopens
the database.
🪄 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: 92fa2c72-dab9-44c5-80dd-bb6adc3b3172
📒 Files selected for processing (8)
docs/devtools.mdpolylogue/cli/commands/maintenance/__init__.pypolylogue/cli/commands/maintenance/_cursor_authority.pypolylogue/maintenance/cursor_authority_reconcile.pypolylogue/sources/live/batch.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/maintenance/test_cursor_authority_reconcile.pytests/unit/sources/test_live_watcher.py
| recovered_receipt_payload: dict[str, object] = { | ||
| "format": RECEIPT_FORMAT, | ||
| "verdict": "reconciled" if after_projection.overall_status == "healthy" else "typed_deferred", | ||
| "archive_identity": {"root": str(root.resolve())}, | ||
| "plan_digest": plan["plan_digest"], | ||
| "backup": backup_evidence, | ||
| "before_projection": plan["before_projection"], | ||
| "after_projection": _private_projection(after_projection), | ||
| "changed_rows": {"cursor": 1, "accepted_head_direct_writes": 0}, | ||
| "ingest_attempt_id": recovery_attempt, | ||
| "code_sha": plan["code_sha"], | ||
| "deployed_package_sha": plan["deployed_package_sha"], | ||
| "tier_fingerprints": _tier_snapshots(root), | ||
| "quick_check": _quick_checks(root), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not assert changed_rows as a constant, and guard the before_projection subscript.
Line 519 writes "changed_rows": {"cursor": 1, "accepted_head_direct_writes": 0} as a literal in both receipt payloads (also line 572). The value is never measured. In the recovery branch no ingest ran in this command, and in the typed_deferred branch the ingest failed, yet both receipts still claim one cursor row changed. A receipt that states an unverified count is worse than one that omits it. Derive the count from the before and after projections, or record it as null.
Line 517 subscripts plan["before_projection"] directly. _load_plan verifies only the format and the digest, so the key can be absent and the apply then raises KeyError instead of CursorAuthorityReconciliationError. Lines 503-506 in the same branch already read the same field defensively, so the two accesses disagree. The second branch validates it at lines 552-554 before use; apply the same validation once, before both branches.
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py` around lines 511 - 525,
Update the reconciliation receipt construction in the recovery and
typed-deferred branches to avoid asserting a fixed changed_rows count: derive
the cursor change from the before/after projections or record the count as null
when it cannot be verified. Validate and retrieve plan["before_projection"] once
before either branch, reusing the existing defensive validation used near the
earlier access, so missing or invalid data raises
CursorAuthorityReconciliationError instead of KeyError.
| with sqlite3.connect(cursor._db_path) as conn: | ||
| conn.execute( | ||
| "UPDATE ingest_cursor SET byte_offset = byte_offset + 1 WHERE source_path = ?", | ||
| (str(source_path),), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Close the SQLite connection after the cursor mutation.
with sqlite3.connect(...) commits the transaction but does not close the connection. The connection to ops.db stays open for the rest of the test, and the code under test re-opens the same file to rebuild the frontier projection. Use contextlib.closing so the write is released deterministically.
♻️ Proposed fix
- with sqlite3.connect(cursor._db_path) as conn:
- conn.execute(
- "UPDATE ingest_cursor SET byte_offset = byte_offset + 1 WHERE source_path = ?",
- (str(source_path),),
- )
+ with closing(sqlite3.connect(cursor._db_path)) as conn, conn:
+ conn.execute(
+ "UPDATE ingest_cursor SET byte_offset = byte_offset + 1 WHERE source_path = ?",
+ (str(source_path),),
+ )Add the import at the top of the file:
from contextlib import closing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with sqlite3.connect(cursor._db_path) as conn: | |
| conn.execute( | |
| "UPDATE ingest_cursor SET byte_offset = byte_offset + 1 WHERE source_path = ?", | |
| (str(source_path),), | |
| ) | |
| with closing(sqlite3.connect(cursor._db_path)) as conn, conn: | |
| conn.execute( | |
| "UPDATE ingest_cursor SET byte_offset = byte_offset + 1 WHERE source_path = ?", | |
| (str(source_path),), | |
| ) |
🤖 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/unit/maintenance/test_cursor_authority_reconcile.py` around lines 204 -
208, Update the SQLite mutation block in the cursor reconciliation test to
import and use contextlib.closing around sqlite3.connect, while retaining the
existing transaction context so changes are committed and the connection is
deterministically closed before the code under test reopens the database.
![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: 95fe9893d7
ℹ️ 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("backup lacks a readable verification receipt") from exc | ||
| if not isinstance(manifest, dict) or manifest.get("profile") != "full_evidence": | ||
| raise CursorAuthorityReconciliationError("apply requires a full_evidence backup") | ||
| if not isinstance(receipt, dict) or receipt.get("verdict") != "success": |
There was a problem hiding this comment.
When an incomplete or fabricated backup directory is supplied, this accepts any JSON receipt whose verdict is success; the verification object is optional, and the blob checks only require a directory and inventory file to exist. It never calls verify_verification_receipt or binds the signed receipt evidence to the manifest, tier artifacts, and blobs, so an empty blob backup with hand-written metadata can authorize the source-tier mutation without usable rollback evidence.
Useful? React with 👍 / 👎.
| recovery_attempt = _find_recovery_attempt(root, current_path) | ||
| if recovery_attempt is None or after_projection.cursor_ahead_count != 0: | ||
| raise CursorAuthorityReconciliationError("plan bindings changed before archive ownership") |
There was a problem hiding this comment.
When the planned violation disappears before apply, _find_recovery_attempt accepts any of the last 50 completed attempts for the same path, without requiring that it occurred after the plan or carried this plan digest and scoped authorization. A prior ordinary attempt will commonly satisfy this, so an unrelated cursor edit or ingest can produce a receipt claiming this plan reconciled the archive and citing an unrelated attempt ID.
Useful? React with 👍 / 👎.
| metrics.succeeded_file_count != 0 | ||
| or str(current_path) not in metrics.failed_paths | ||
| or metrics.time_budget_exceeded |
There was a problem hiding this comment.
When the authorized ingest fails while the cursor remains ahead, these checks classify every zero-success failed-path result as typed_deferred without inspecting the ingest attempt's disposition. For example, SchemaVersionMismatchError or an unclassified parser failure meets these metric conditions, so apply emits an accepted deferred receipt even though the normal ingest route recorded a structural or unknown failure rather than an explicitly typed deferral.
Useful? React with 👍 / 👎.
| accepted_frontier=_plan_int(plan, "accepted_frontier"), | ||
| plan_digest=str(plan["plan_digest"]), | ||
| ): | ||
| metrics = await processor.ingest_files([source_path], emit_event=False) |
There was a problem hiding this comment.
When the stored parser fingerprint is current and the source has bytes beyond the ahead cursor, this generic call allows _append_plan to start reading at the untrusted cursor.byte_offset rather than the proven accepted_frontier. If the ahead gap contains one or more complete records, those records are skipped permanently while the later append can succeed, advance the cursor, make the projection healthy, and produce a reconciled receipt despite missing source data.
Useful? React with 👍 / 👎.
| sample = projection.cursor_ahead_samples[0] | ||
| if Path(sample.source_path).resolve() != source_path.resolve(): | ||
| raise CursorAuthorityReconciliationError("selected source path is not the sole cursor-ahead path") | ||
| with closing(sqlite3.connect(f"file:{(root / 'index.db').resolve()}?mode=ro", uri=True)) as conn: |
There was a problem hiding this comment.
When .index-active-pointer targets a generation different from the conventional root/index.db, this query reads the stale shadow index, and _normal_ingest later explicitly opens that same stale path. The command can therefore plan and write source/index state against a non-serving generation while leaving the actual active index unreconciled, creating cross-tier divergence instead of repairing the live archive.
Useful? React with 👍 / 👎.
| @click.option("--source-path-file", type=click.Path(path_type=Path, dir_okay=False), default=None) | ||
| @click.option("--output-plan", type=click.Path(path_type=Path, dir_okay=False), default=None) | ||
| @click.option("--plan", "plan_path", type=click.Path(path_type=Path, dir_okay=False), default=None) | ||
| @click.option("--backup-manifest", type=click.Path(path_type=Path, dir_okay=False), default=None) |
There was a problem hiding this comment.
When the operator follows the apply example in docs/devtools.md:335 and passes the full-evidence backup directory, Click rejects it because this option sets dir_okay=False, even though _backup_root explicitly supports a directory and the documented value is /private/full-evidence-backup. As written, the published apply command cannot reach reconciliation unless the operator instead guesses that they must pass the nested manifest.json file.
Useful? React with 👍 / 👎.
| polylogue, | ||
| (WatchSource(name=source_path.parent.name, root=source_path.parent),), | ||
| cursor=cursor, | ||
| parser_fingerprint="live-batched-v2", |
There was a problem hiding this comment.
When the watcher parser fingerprint is bumped for a parser-semantics change, this maintenance processor remains pinned to live-batched-v2. Even if reconciliation correctly falls back to a full capture, it writes the repaired cursor with the stale literal, so the subsequently started daemon sees a parser-fingerprint mismatch and immediately revives and full-reingests the same potentially large source instead of treating the reconciliation result as current.
Useful? React with 👍 / 👎.
Problem: cursor-authority reconciliation trusted racy file observations, fixed receipt counts, and incomplete recovery and backup evidence.\n\nWhat changed: make source and tier observations descriptor-safe, scope recovery to later completed attempts, validate typed frontier and backup data, publish refuse-existing receipts atomically, and record observed, deferred, and failed outcomes with redacted projections. Extend the focused maintenance, CLI, and retention tests.\n\nCompatibility/migration: the route remains daemon-stopped, backup-gated, and limited to the normal scoped live-ingest path.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: post-ingest receipt construction could fail while reporting the original reconciliation failure, and recovery variables shadowed the normal post-state.\n\nWhat changed: make failure receipts tolerant of unavailable post-state evidence, label recovery observations explicitly, and tighten typed test assertions.\n\nCompatibility/migration: no archive schema or mutation route changes.\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: a89f2b2b11
ℹ️ 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".
| attempt_id=attempt_id, | ||
| attempt_observation="performed", | ||
| evidence=evidence, | ||
| tolerate_state_errors=True, |
There was a problem hiding this comment.
When post-ingest _tier_snapshots or PRAGMA quick_check fails—for example because reconciliation leaves a tier corrupt or unreadable—this flag suppresses the error and still writes a reconciled or typed_deferred receipt with both integrity fields set to null. That allows downstream operators to treat the repair as accepted precisely when its final archive integrity could not be established; successful receipt construction should remain fail-closed.
Useful? React with 👍 / 👎.
| failure_payload = _receipt_payload( | ||
| plan=plan, | ||
| backup=backup_evidence, | ||
| root=root, | ||
| verdict="failed", |
There was a problem hiding this comment.
When ingest or a postcondition fails because an archive tier has become missing, unreadable, or otherwise uninspectable, this failure-path call retains the default tolerate_state_errors=False; _receipt_payload then repeats the failing tier snapshot/quick-check and exits before writing the promised failed receipt, often masking the original reconciliation error as well. The tolerant state-evidence behavior belongs on this failure receipt path rather than on accepted receipts.
Useful? React with 👍 / 👎.
| if projection.cursor_ahead_count != 1: | ||
| raise CursorAuthorityReconciliationError("refusing to guess among multiple cursor-ahead rows") | ||
| if projection.broken_head_count or projection.missing_source_raw_count: | ||
| raise CursorAuthorityReconciliationError( | ||
| "global raw-frontier violation set is not exactly one cursor-ahead row" |
There was a problem hiding this comment.
When raw_materialization_readiness_snapshot is unavailable because one of its classification queries fails, the projection reports missing_source_raw_status="unknown" but leaves missing_source_raw_count at zero; because a proven cursor-ahead violation dominates unknown status, overall_status is still violated. These count-only checks therefore create a plan, and the scoped gate likewise accepts it, even though the command has not proven that missing source evidence is absent. Require the projection to be fully available, or explicitly require the broken-head and missing-source components to be healthy, before permitting the one-violation repair.
Useful? React with 👍 / 👎.
| def _build_plan(root: Path, source_path: Path, *, require_candidate: bool = True) -> dict[str, object]: | ||
| tiers = _tier_snapshots(root) | ||
| projection = _projection_for(root) |
There was a problem hiding this comment.
When the stopped daemon leaves any required tier with nonempty WAL frames, _tier_snapshots hashes only the pre-checkpoint main database file. A subsequent verified backup calls daemon.backup._backup_sqlite, which checkpoints and truncates the WAL before recording and copying the tier, so the backup's source fingerprint and bytes no longer match this plan even though the logical archive did not change. The documented plan-plus-fresh-backup workflow is therefore unusable for this common SQLite state unless planning first obtains the same checkpointed snapshot or otherwise fingerprints the complete WAL-aware database image.
Useful? React with 👍 / 👎.
| if current_plan is None or not _same_plan_bindings(current_plan, plan): | ||
| recovery_projection = _projection_for(root) | ||
| recovery_attempt = _find_recovery_attempt(root, current_path, _plan_int(plan, "observed_at_ms")) | ||
| if recovery_attempt is None or recovery_projection.cursor_ahead_count != 0: |
There was a problem hiding this comment.
When the planned violation has disappeared, this branch validates the projection and writes an accepted recovery receipt without ever acquiring the shared archive lease used by the normal branch below. A daemon can start after _require_daemon_stopped returns and mutate the tiers while the recovery projection, quick checks, and fingerprints are collected, producing a receipt that combines inconsistent observations or no longer describes the live archive. Acquire ownership before deciding between observed recovery and performed reconciliation.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
| "backup": dict(backup), | ||
| "before_projection": dict(before_projection), | ||
| "after_projection": _private_projection(after_projection) if after_projection is not None else None, | ||
| "metrics": metrics.to_payload() if metrics is not None else None, |
There was a problem hiding this comment.
When reconciliation is deferred or fails, LiveBatchMetrics.to_payload() includes the selected absolute path in failed_paths; successful append-style metrics can also include raw session identities in new_sessions or updated_sessions. This payload is placed directly into the durable receipt and, for a returned typed-deferred result, echoed by the CLI, bypassing the command's mode-0600 path-file boundary and the digest redaction applied to the plan and projections. Serialize a reconciliation-specific redacted metrics payload instead.
Useful? React with 👍 / 👎.
|
run-ci |
|
Reopening immediately to rerun the required CI gate after refreshing the structured PR scope carrier. |
Problem: cursor-authority reconciliation could observe recovery outside archive ownership, fingerprint only SQLite main files, accept untyped deferrals, and persist private path and identity material.\n\nWhat changed: bind the scoped authorization to the exact plan and path, route its one-use maintenance flow through full ingest, resolve the active index generation, attest durable backups, hash effective SQLite images, require healthy sibling projections, bind recovery attempts to planning events, and redact receipts and metrics.\n\nCompatibility/migration: ordinary live ingestion remains unchanged and no direct cursor or accepted-head writes are introduced.
Problem: the scoped authorization return path used an implicit empty return and token truthiness, which strict mypy rejected.\n\nWhat changed: return the optional authorization explicitly and pass the consumed typed token directly; tighten the backup validation test's result narrowing.\n\nCompatibility/migration: runtime routing and ordinary ingestion semantics are unchanged.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 590-601: Update the planning_bindings accumulation in the
event-row loop so repeated rows for the same attempt preserve a prior match:
combine the existing binding with the current digest comparison using logical OR
instead of overwriting it. Keep the existing JSON validation and keying by
str(attempt_id) unchanged.
- Line 336: Replace cursor_authority_path_digest with _identity_digest when
deriving logical_source_key in the plan bindings, so _build_plan and
apply_reconciliation remain stable across working directories and match
_private_projection’s identity digest.
- Around line 843-845: Remove the second _build_plan invocation and the
redundant _same_plan_bindings check near the ownership reconciliation step;
reuse the current_plan value created earlier and already validated before
applying the archive ownership changes.
- Around line 533-540: Update the refuse_existing branch in _write_atomic_json
to handle non-FileExistsError OSError from os.link by using a link-free
exclusive publication fallback, preserving exclusive-create semantics and
cleanup of temporary_path. Keep FileExistsError mapped to
CursorAuthorityReconciliationError, and ensure failures in the fallback do not
silently overwrite an existing output.
In `@tests/unit/maintenance/test_cursor_authority_reconcile.py`:
- Around line 355-403: Extend
test_backup_validation_rehashes_and_rejects_mismatched_tier to construct plan
with active_index from reconcile._active_index_binding(tmp_path) and set the
manifest’s index tier path to a different value. Assert _validate_backup raises
CursorAuthorityReconciliationError matching “does not bind the active index
generation”, thereby covering the active-index path-binding branch.
In `@tests/unit/sources/test_live_watcher.py`:
- Around line 352-370: Update the shadow-index verification around shadow_before
and the final assertions to account for SQLite index.db-wal and index.db-shm
sidecars, not just shadow_index.read_bytes(). Capture and compare the complete
shadow snapshot state before and after ingestion, including sidecar presence and
contents, while preserving the existing full_file_count assertion.
🪄 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: 8e6e732f-092b-456d-8a91-635ec19697cc
📒 Files selected for processing (8)
docs/devtools.mdpolylogue/cli/commands/maintenance/_cursor_authority.pypolylogue/maintenance/cursor_authority_reconcile.pypolylogue/sources/live/batch.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/maintenance/test_cursor_authority_reconcile.pytests/unit/sources/test_live_watcher.pytests/unit/storage/test_raw_retention.py
| if prefix_digest != blob_hash: | ||
| raise CursorAuthorityReconciliationError("source prefix does not match the accepted raw blob hash") | ||
| return { | ||
| "logical_source_key": cursor_authority_path_digest(Path(logical_source_key)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not digest logical_source_key with the path digest helper.
logical_source_key is a provider identity such as codex:session-1. It is not a filesystem path. cursor_authority_path_digest calls path.resolve(), which resolves a relative name against the current working directory. The resulting digest therefore depends on the directory the operator ran the command from.
Two consequences follow:
_build_planstores this value in the plan.apply_reconciliationrebuilds the plan and compares it with_same_plan_bindings. If the plan and the apply run from different working directories, the comparison fails and the apply refuses a valid plan with "plan bindings changed before archive ownership"._private_projectionalready digests the same field with_identity_digestat line 241. The module produces two different digests for one identity.
Use _identity_digest here so both surfaces agree and the digest is independent of the working directory.
🐛 Proposed fix
return {
- "logical_source_key": cursor_authority_path_digest(Path(logical_source_key)),
+ "logical_source_key": _identity_digest(logical_source_key),
"cursor_byte_offset": cursor_offset,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "logical_source_key": cursor_authority_path_digest(Path(logical_source_key)), | |
| "logical_source_key": _identity_digest(logical_source_key), |
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py` at line 336, Replace
cursor_authority_path_digest with _identity_digest when deriving
logical_source_key in the plan bindings, so _build_plan and apply_reconciliation
remain stable across working directories and match _private_projection’s
identity digest.
| if refuse_existing: | ||
| try: | ||
| os.link(temporary_path, path) | ||
| except FileExistsError as exc: | ||
| raise CursorAuthorityReconciliationError(f"output path already exists: {path}") from exc | ||
| temporary_path.unlink() | ||
| else: | ||
| os.replace(temporary_path, path) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle os.link failures other than FileExistsError.
os.link fails with OSError on filesystems that do not support hard links, for example some FUSE and overlay mounts. Only FileExistsError is handled here. Any other OSError propagates.
This matters most in the apply failure path. apply_reconciliation calls _write_atomic_json(receipt, failure_payload, refuse_existing=True) after the ingest already ran. If os.link raises ENOSYS or EPERM, the durable failure receipt is lost and the original reconciliation error is replaced by an unrelated OSError.
Add a link-free exclusive publication fallback for that case.
🛡️ Proposed fix
if refuse_existing:
try:
os.link(temporary_path, path)
except FileExistsError as exc:
raise CursorAuthorityReconciliationError(f"output path already exists: {path}") from exc
+ except OSError:
+ # This filesystem does not support hard links. Publish through an
+ # exclusive create instead so the receipt still lands.
+ try:
+ exclusive = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
+ except FileExistsError as exc:
+ raise CursorAuthorityReconciliationError(f"output path already exists: {path}") from exc
+ with os.fdopen(exclusive, "w", encoding="utf-8") as handle:
+ handle.write(encoded)
+ handle.flush()
+ os.fsync(handle.fileno())
temporary_path.unlink()
else:
os.replace(temporary_path, path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if refuse_existing: | |
| try: | |
| os.link(temporary_path, path) | |
| except FileExistsError as exc: | |
| raise CursorAuthorityReconciliationError(f"output path already exists: {path}") from exc | |
| temporary_path.unlink() | |
| else: | |
| os.replace(temporary_path, path) | |
| if refuse_existing: | |
| try: | |
| os.link(temporary_path, path) | |
| except FileExistsError as exc: | |
| raise CursorAuthorityReconciliationError(f"output path already exists: {path}") from exc | |
| except OSError: | |
| # This filesystem does not support hard links. Publish through an | |
| # exclusive create instead so the receipt still lands. | |
| try: | |
| exclusive = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) | |
| except FileExistsError as exc: | |
| raise CursorAuthorityReconciliationError(f"output path already exists: {path}") from exc | |
| with os.fdopen(exclusive, "w", encoding="utf-8") as handle: | |
| handle.write(encoded) | |
| handle.flush() | |
| os.fsync(handle.fileno()) | |
| temporary_path.unlink() | |
| else: | |
| os.replace(temporary_path, path) |
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py` around lines 533 - 540,
Update the refuse_existing branch in _write_atomic_json to handle
non-FileExistsError OSError from os.link by using a link-free exclusive
publication fallback, preserving exclusive-create semantics and cleanup of
temporary_path. Keep FileExistsError mapped to
CursorAuthorityReconciliationError, and ensure failures in the fallback do not
silently overwrite an existing output.
| planning_bindings: dict[str, bool] = {} | ||
| for attempt_id, payload_json in event_rows: | ||
| try: | ||
| payload = json.loads(str(payload_json)) | ||
| except (TypeError, ValueError): | ||
| continue | ||
| if not isinstance(payload, dict): | ||
| continue | ||
| planning_bindings[str(attempt_id)] = ( | ||
| payload.get("cursor_authority_plan_digest") == plan_digest | ||
| and payload.get("cursor_authority_path_digest") == path_digest | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Accumulate the planning binding instead of overwriting it.
The loop assigns planning_bindings[str(attempt_id)] on every event row. If one attempt has more than one planning stage event, the last row iterated wins. The query orders by observed_at_ms DESC, so an older non-matching event overwrites a newer matching one and the attempt is then skipped. Combine the results with or so any matching planning event binds the attempt.
♻️ Proposed refactor
- planning_bindings[str(attempt_id)] = (
- payload.get("cursor_authority_plan_digest") == plan_digest
- and payload.get("cursor_authority_path_digest") == path_digest
- )
+ matches = (
+ payload.get("cursor_authority_plan_digest") == plan_digest
+ and payload.get("cursor_authority_path_digest") == path_digest
+ )
+ planning_bindings[str(attempt_id)] = planning_bindings.get(str(attempt_id), False) or matches📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| planning_bindings: dict[str, bool] = {} | |
| for attempt_id, payload_json in event_rows: | |
| try: | |
| payload = json.loads(str(payload_json)) | |
| except (TypeError, ValueError): | |
| continue | |
| if not isinstance(payload, dict): | |
| continue | |
| planning_bindings[str(attempt_id)] = ( | |
| payload.get("cursor_authority_plan_digest") == plan_digest | |
| and payload.get("cursor_authority_path_digest") == path_digest | |
| ) | |
| planning_bindings: dict[str, bool] = {} | |
| for attempt_id, payload_json in event_rows: | |
| try: | |
| payload = json.loads(str(payload_json)) | |
| except (TypeError, ValueError): | |
| continue | |
| if not isinstance(payload, dict): | |
| continue | |
| matches = ( | |
| payload.get("cursor_authority_plan_digest") == plan_digest | |
| and payload.get("cursor_authority_path_digest") == path_digest | |
| ) | |
| planning_bindings[str(attempt_id)] = planning_bindings.get(str(attempt_id), False) or matches |
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py` around lines 590 - 601,
Update the planning_bindings accumulation in the event-row loop so repeated rows
for the same attempt preserve a prior match: combine the existing binding with
the current digest comparison using logical OR instead of overwriting it. Keep
the existing JSON validation and keying by str(attempt_id) unchanged.
| current_plan = _build_plan(root, current_path) | ||
| if not _same_plan_bindings(current_plan, plan): | ||
| raise CursorAuthorityReconciliationError("plan bindings changed after archive ownership") |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Remove the duplicate _build_plan call.
Line 797 already builds current_plan, and line 800 proves it matches the stored plan bindings. Line 843 rebuilds the same plan inside the same ownership hold, and line 844 repeats the same comparison. Nothing mutates the archive between the two calls.
_build_plan is expensive. It runs _tier_snapshots, which performs a full sqlite3.backup copy plus a SHA-256 pass over four tiers, and _head_details, which hashes the source prefix up to the accepted frontier. This doubles that cost on every apply.
Reuse the value from line 797.
♻️ Proposed refactor
- current_plan = _build_plan(root, current_path)
- if not _same_plan_bindings(current_plan, plan):
- raise CursorAuthorityReconciliationError("plan bindings changed after archive ownership")
metrics: LiveBatchMetrics | None = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| current_plan = _build_plan(root, current_path) | |
| if not _same_plan_bindings(current_plan, plan): | |
| raise CursorAuthorityReconciliationError("plan bindings changed after archive ownership") |
🤖 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 `@polylogue/maintenance/cursor_authority_reconcile.py` around lines 843 - 845,
Remove the second _build_plan invocation and the redundant _same_plan_bindings
check near the ownership reconciliation step; reuse the current_plan value
created earlier and already validated before applying the archive ownership
changes.
| def test_backup_validation_rehashes_and_rejects_mismatched_tier( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| backup = tmp_path / "backup" | ||
| backup.mkdir() | ||
| (backup / "blob").mkdir() | ||
| (backup / "blob-inventory.json").write_text("{}", encoding="utf-8") | ||
| tiers: dict[str, dict[str, object]] = {} | ||
| for tier in ("source", "index", "ops", "audit"): | ||
| path = backup / f"{tier}.db" | ||
| with sqlite3.connect(path) as conn: | ||
| conn.execute("CREATE TABLE marker (value TEXT)") | ||
| tiers[tier] = reconcile._sqlite_snapshot(path) | ||
| plan = {"tier_fingerprints": tiers} | ||
| manifest = { | ||
| "profile": "full_evidence", | ||
| "included_tiers": [f"{tier}.db" for tier in tiers], | ||
| "tier_source_fingerprints": {f"{tier}.db": value for tier, value in tiers.items()}, | ||
| } | ||
| (backup / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") | ||
| (backup / "verification-receipt.json").write_text( | ||
| json.dumps( | ||
| { | ||
| "verdict": "success", | ||
| "verification": { | ||
| "source_blobs_resolved": True, | ||
| "index_attachment_blobs_resolved": True, | ||
| "blob_inventory_exact": True, | ||
| }, | ||
| } | ||
| ), | ||
| encoding="utf-8", | ||
| ) | ||
| monkeypatch.setattr(reconcile, "ARCHIVE_ROOT", tmp_path) | ||
| with pytest.raises(reconcile.CursorAuthorityReconciliationError, match="attestation"): | ||
| reconcile._validate_backup(backup, plan) | ||
| monkeypatch.setattr(reconcile, "verify_verification_receipt", lambda *args, **kwargs: None) | ||
| validated = reconcile._validate_backup(backup, plan) | ||
| assert isinstance(validated["root"], dict) | ||
| assert validated["root"]["basename"] == backup.name | ||
| (backup / "audit.db").unlink() | ||
| with pytest.raises(reconcile.CursorAuthorityReconciliationError, match="tier is missing"): | ||
| reconcile._validate_backup(backup, plan) | ||
| with sqlite3.connect(backup / "audit.db") as conn: | ||
| conn.execute("CREATE TABLE marker (value TEXT)") | ||
| with sqlite3.connect(backup / "source.db") as conn: | ||
| conn.execute("INSERT INTO marker VALUES ('tampered')") | ||
| with pytest.raises(reconcile.CursorAuthorityReconciliationError, match="image does not match"): | ||
| reconcile._validate_backup(backup, plan) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extend the backup test to cover the active-index binding branch.
plan here is {"tier_fingerprints": tiers} with no active_index key. _validate_backup reads plan.get("active_index"), gets None, and therefore skips both the "active index generation changed since planning" check and the tier == "index" path-binding check at polylogue/maintenance/cursor_authority_reconcile.py lines 497-505. Those two checks bind the apply to one index generation, so they remain untested.
Add a plan that carries active_index from reconcile._active_index_binding(tmp_path) and a manifest whose index path entry does not match the active index, then assert the "does not bind the active index generation" error.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 373-373: use jsonify instead of json.dumps for JSON output
Context: json.dumps(manifest)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 375-384: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"verdict": "success",
"verification": {
"source_blobs_resolved": True,
"index_attachment_blobs_resolved": True,
"blob_inventory_exact": True,
},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/unit/maintenance/test_cursor_authority_reconcile.py` around lines 355 -
403, Extend test_backup_validation_rehashes_and_rejects_mismatched_tier to
construct plan with active_index from reconcile._active_index_binding(tmp_path)
and set the manifest’s index tier path to a different value. Assert
_validate_backup raises CursorAuthorityReconciliationError matching “does not
bind the active index generation”, thereby covering the active-index
path-binding branch.
| shadow_index = tmp_path / "index.db" | ||
| active_index = tmp_path / "generations" / "active" / "index.db" | ||
| active_index.parent.mkdir(parents=True) | ||
| shutil.copy2(shadow_index, active_index) | ||
| (tmp_path / ".index-active-pointer").write_text(f"{active_index}\n", encoding="utf-8") | ||
| projection = reconcile._projection_for(tmp_path) | ||
| sample = projection.cursor_ahead_samples[0] | ||
| shadow_before = shadow_index.read_bytes() | ||
| with scoped_cursor_authority_authorization( | ||
| source_path_digest=cursor_authority_path_digest(source_path), | ||
| cursor_byte_offset=sample.cursor_byte_offset, | ||
| accepted_frontier=sample.accepted_frontier, | ||
| plan_digest="active-index-test", | ||
| force_full_ingest=True, | ||
| ): | ||
| metrics = await processor.ingest_files([source_path], emit_event=False) | ||
|
|
||
| assert metrics.full_file_count == 1 | ||
| assert shadow_index.read_bytes() == shadow_before |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how the live write path resolves the index and whether WAL is enabled for archive tiers.
set -euo pipefail
echo "== _open_archive_for_live_write"
ast-grep run --lang python --pattern 'def _open_archive_for_live_write($$$):
$$$'
echo "== journal_mode / WAL configuration for archive connections"
rg -nP --type=py -C3 'journal_mode' polylogue
echo "== initialize_active_archive_root"
rg -nP --type=py -C5 'def initialize_active_archive_root' polylogueRepository: Sinity/polylogue
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files near target =="
git ls-files | rg '(^|/)test_live_watcher\.py$|pol|arch|reconcile|sources' | head -200
echo "== target file stats and nearby content =="
if [ -f tests/unit/sources/test_live_watcher.py ]; then
wc -l tests/unit/sources/test_live_watcher.py
sed -n '300,390p' tests/unit/sources/test_live_watcher.py
fi
echo "== search reconciliation helpers =="
rg -n --type=py -C3 '_projection_for|_open_archive_for_live_write|initialize_active_archive_root|journal_mode|WAL|sqlite' .Repository: Sinity/polylogue
Length of output: 8385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files near source and test =="
git ls-files 'tests/unit/sources/*.py' 'polylogue/sources/**/*.py' 'polylogue/*sources*.py' | head -200
echo "== search live watcher symbols and sqlite config =="
rg -n --type=py -C3 'live_watcher|_projection_for|scoped_cursor_authority_authorization|process_files|ingest_files|index-active-pointer|journal_mode|WAL|_sqlite|connect' tests/unit/sources test* . 2>/dev/null | head -500Repository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target test section =="
sed -n '320,375p' tests/unit/sources/test_live_watcher.py
echo "== live archive helpers =="
sed -n '1,260p' polylogue/sources/live/archive_open.py
echo "== sqlite connection helpers =="
fd 'connection\.py|wal|sqlite' polylogue/storage | sed -n '1,80p'
rg -n --type=py -C3 'journal_mode|WAL|initialize_archive_database|ArchiveTier\.INDEX|open_connection|BEGIN IMMEDIATE|PRAGMA' polylogue/storage polylogue/sources polylogue/archive tests/unit/sources/test_live_watcher.py | sed -n '1,260p'Repository: Sinity/polylogue
Length of output: 24897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cursor authority helper =="
rg -n --type=py -C5 'def _seed_live_cursor_authority_case|cursor_authority_case|live_cursor' tests/unit/sources/test_live_watcher.py
echo "== reconcile projection open logic =="
rg -n --type=py -C5 'def _projection_for|cursor_ahead_samples|active-index-pointer|Open existing|archive_root|index.db' polylogue/maintenance polylogue/archive | sed -n '1,260p'
echo "== sqlite connection WAL defaults =="
sed -n '1,260p' polylogue/storage/sqlite/connection.py
rg -n --type=py -C3 'def open_connection|journal_mode|WAL|sqlite3\.connect' polylogue/storage/sqlite/connection.py polylogue/storage/sqlite/connection_profile.py | sed -n '1,220p'
echo "== sqlite snapshot write path =="
sed -n '1,240p' polylogue/sources/sqlite_snapshot.pyRepository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== seed helper body =="
sed -n '124,210p' tests/unit/sources/test_live_watcher.py
echo "== sqlite snapshot initialization and write selection =="
sed -n '1,320p' polylogue/sources/sqlite_snapshot.py
rg -n --type=py -C4 'sqlite_snapshot|SqliteSnapshot|looks_like_sqlite_bytes|raw_id|raw_sessions|INSERT INTO raw_sessions|BEGIN IMMEDIATE|read_only=False|open_existing' polylogue/sources polylogue/storage sqlite | sed -n '1,260p'
echo "== reconcile projection exact implementation =="
rg -n --type=py -C6 'def _projection_for' polylogue/maintenance/cursor_authority_reconcile.py
sed -n '1,240p' polylogue/maintenance/cursor_authority_reconcile.py
sed -n '240,420p' polylogue/maintenance/cursor_authority_reconcile.pyRepository: Sinity/polylogue
Length of output: 33215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sqlite snapshot raw write flow =="
rg -n --type=py -C5 'raw_id|blob_hash|stage|sqlite_snapshot|SqliteSnapshot|write_raw_payload|bind_raw_revision' polylogue/storage polylogue/sources/live polylogue/archive | sed -n '1,360p'
echo "== raw session materialization write selection =="
rg -n --type=py -C5 'write_raw_sessions|raw_sessions|INSERT INTO raw_sessions|sqlite_snapshot|SqliteSnapshot|blob_path_for_hash|read_only=False|open_existing' polylogue/storage | sed -n '1,400p'
echo "== initialize active archive root implementation =="
rg -n --type=py -C8 'def initialize_active_archive_root' polylogue/storage polylogue
sed -n '1,220p' polylogue/storage/sqlite/async_sqlite_archive.py
rg -n --type=py -C6 'initialize_active_archive_root' polylogue/storage/sqlite/async_sqlite_archive.pyRepository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sqlite snapshot imports/search by symbol =="
rg -n "write_raw_payload|write_raw_sessions|write_from_path|BlobStore|SQLiteBlobSnapshot|stage_sqlite_snapshot|sqlite_snapshot" polylogue/sources polylogue/storage | sed -n '1,320p'
echo "== archive backend write helpers =="
fd -a 'archive.*\.py$|sqlite.*\.py$' polylogue/storage | sed -n '1,30p'
rg -n --type=py -C5 'class ArchiveStore|write_raw_payload|open_existing|read_only=False|raw_sessions|sqlite_snapshot|stage_sqlite_snapshot|snapshot_sqlite_to_blob' polylogue/storage polylogue/archive | sed -n '1,400p'
echo "== live batch processor open flow =="
rg -n --type=py -C6 'scoped_cursor_authority_authorization|cursor_authority_open_authorization|_open_archive_for_live_write|apply_raw_revision_replay|bind_raw_revision|write_raw_payload|force_full_ingest|ingest_files' polylogue/sources/live polylogue/maintenance | sed -n '1,400p'Repository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== batch sqlite acquisition functions =="
rg -n --type=py -C6 'acquire_sqlite_source|sqlite_snapshot|SQLiteBlobSnapshot|raw_source_revisions|snapshot_sqlite_to_blob|snapshot_sqlite_database|sqlite_source_revision|stage_sqlite_snapshot|write_from_path' polylogue/sources/live/batch.py | sed -n '1,280p'
echo "== write raw payload implementation =="
rg -n --type=py -C8 'def write_raw_payload|-- insert raw_sessions|INSERT INTO raw_sessions|source_revision|blob_hash|raw_id|sqlite_snapshot|snapshot_sqlite_to_blob' polylogue/storage/sqlite/archive_tiers/revision_governance.py polylogue/storage/sqlite/archive_tiers/archive.py polylogue/storage/sqlite/archive_tiers/archive_plan.py | sed -n '1,420p'
echo "== active pointer handling =="
rg -n --type=py -C8 'index.active|index-active-pointer|active_index|\.index-active|index_tier' polylogue/maintenance/cursor_authority_reconcile.py polylogue/sources/live/batch.py polylogue/storage | sed -n '1,320p'Repository: Sinity/polylogue
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sqlite snapshot snapshot to blob implementation =="
sed -n '32,212p' polylogue/sources/sqlite_snapshot.py
echo "== sqlite snapshot source revision implementation =="
sed -n '211,227p' polylogue/sources/sqlite_snapshot.py
echo "== sqlite snapshot database for sidecar implementation =="
sed -n '200,227p' polylogue/sources/sqlite_snapshot.py
echo "== reconcile projection and assertion helpers =="
sed -n '184,215p' polylogue/storage/sqlite/durable_change_train.py
sed -n '1,120p' polylogue/maintenance/cursor_authority_reconcile.py
sed -n '120,230p' polylogue/maintenance/cursor_authority_reconcile.py
sed -n '1240,1280p' polylogue/maintenance/cursor_authority_reconcile.pyRepository: Sinity/polylogue
Length of output: 19795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== raw frontier integrity projection =="
rg -n --type=py -C8 'def raw_frontier_integrity_projection|class RawFrontierIntegrityProjection|cursor_ahead_samples|source_revision|sqlite_source_revision|active_index_path' polylogue/storage/raw_retention.py | sed -n '1,260p'
echo "== snapshot_sqlite_to_blob call path and SQLite read-sidecar handling =="
sed -n '226,260p' polylogue/storage/sqlite/durable_change_train.py
sed -n '39,80p' polylogue/storage/sqlite/durable_change_train.pyRepository: Sinity/polylogue
Length of output: 16883
Make the shadow-index assertion include SQLite WAL sidecars.
initialize_active_archive_root configures index.db in WAL mode, so shutil.copy2(shadow_index, active_index) only copies the main file when index.db-wal/index.db-shm can exist. Later, the active generation may apply the replay to a partial shadow backup; comparing only shadow_index.read_bytes() does not prove that ingestion did not reach the shadow tier because writes can land in WAL while shadow_index stays unchanged. Compare the active snapshot, including -wal/-shm state, or assert that the active generation acquired the new session.
🤖 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/unit/sources/test_live_watcher.py` around lines 352 - 370, Update the
shadow-index verification around shadow_before and the final assertions to
account for SQLite index.db-wal and index.db-shm sidecars, not just
shadow_index.read_bytes(). Capture and compare the complete shadow snapshot
state before and after ingestion, including sidecar presence and contents, while
preserving the existing full_file_count assertion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6588c6c56f
ℹ️ 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".
| verify_verification_receipt( | ||
| receipt, | ||
| tier="source", | ||
| live_tier_path=location.configured_tier("source").configured_path, | ||
| ) |
There was a problem hiding this comment.
When a valid successful receipt from an older backup is copied into a newly assembled backup directory, these calls only authenticate the receipt's HMAC; _validate_backup never compares the signed manifest_sha256, artifact_inventory, blob_inventory_root_sha256, or blob evidence to the supplied files. The fresh code now calls verify_verification_receipt, but a transplanted receipt plus plan-matching tier copies and an empty blob/ and blob-inventory.json still passes, allowing the durable reconciliation without usable blob rollback evidence.
Useful? React with 👍 / 👎.
| if metrics is None: | ||
| return None | ||
| payload = metrics.to_payload() | ||
| payload["failed_paths"] = [_path_identity(Path(str(path))) for path in metrics.failed_paths] |
There was a problem hiding this comment.
When a source filename itself contains a private project, user, or session identity, this redaction still writes Path.name into the durable receipt and echoes it in the CLI result. The fresh redaction hashes the absolute path but explicitly preserves its basename, so it only partially addresses the prior identity-leak issue; failed paths should be represented solely by an opaque digest.
Useful? React with 👍 / 👎.
| if blob_size != frontier or len(blob_hash) != 64: | ||
| raise CursorAuthorityReconciliationError("accepted raw does not bind a complete byte frontier") |
There was a problem hiding this comment.
When the selected cursor's accepted head is a normal byte-proven append revision, blob_size is only the appended slice length while accepted_frontier is the absolute append_end_offset, and the blob hash likewise covers only that slice rather than the complete source prefix. This check therefore rejects every such head at blob_size != frontier (and the following prefix-hash check would also fail), so the reconciliation command cannot plan repairs for incrementally ingested sources after their first accepted append.
Useful? React with 👍 / 👎.
| else: | ||
| if after_projection.cursor_ahead_status != "healthy": | ||
| raise CursorAuthorityReconciliationError("reconciliation did not prove a healthy cursor frontier") | ||
| verdict = "reconciled" |
There was a problem hiding this comment.
When the forced full-ingest route reclassifies the selected file as a non-session artifact, _mark_excluded_cursor marks it excluded and returns neither a succeeded nor failed path. Because the raw-frontier projection omits excluded cursor rows, cursor_ahead_count then becomes zero and this branch emits a reconciled receipt even though metrics.succeeded_file_count is zero and no accepted head was advanced; the source is also left permanently excluded from future watcher retries.
Useful? React with 👍 / 👎.
## Summary Record the post-review state of two reindex Beads and preserve their remaining implementation obligations as named successors. ## Problem Late Codex review found that the cursor reconciliation implementation still lacks a typed no-op path for the preserved incomparable population and current blob-inventory revalidation at apply time. It also found that the proof-edge correction has no executable negative guard for removal of a required edge. Closing either Bead would release downstream readiness prematurely. ## Solution Reopen `polylogue-cursor-authority-reconcile-implementation` and link hardening successor `polylogue-s8gcr`. Reopen `polylogue-reindex-proof-edge-correction` and link guard successor `polylogue-eqq02`; the guard also blocks preflight and terminal proof readiness. Preserve the merged implementation and phase-edge evidence as historical evidence. No production mutation, candidate generation, acceptance, promotion, restart, or live receipt is claimed. ## Verification `bd dep cycles --json` reports no dependency cycles. The pre-push Beads validation scanned 1,749 issues with zero unhandled findings. The PR carrier validates against the exact branch head and Beads snapshot. ## Bead disposition | Bead | Disposition | Successor | Evidence | | --- | --- | --- | --- | | `polylogue-cursor-authority-reconcile-implementation` | Partial | `polylogue-s8gcr` | PR #3860, Codex findings 3734483480 and 3734483485 | | `polylogue-reindex-proof-edge-correction` | Partial | `polylogue-eqq02` | PR #3869, Codex finding 3734483475 | <!-- polylogue-pr-scope:v1 { "assigned_beads": [ "polylogue-cursor-authority-reconcile-implementation", "polylogue-reindex-proof-edge-correction" ], "beads_digest": "52dc2676c0ae91412073d39b799dcd51bd3cd082a224f225e9857826578426dc", "dispositions": [ { "bead_id": "polylogue-cursor-authority-reconcile-implementation", "disposition": "partial", "evidence": [ { "kind": "commit", "ref": "dec1eab35cfb41a40d3bf3014d51b0624004ea94" }, { "kind": "review", "ref": "PR #3860" } ], "successors": [ "polylogue-s8gcr" ] }, { "bead_id": "polylogue-reindex-proof-edge-correction", "disposition": "partial", "evidence": [ { "kind": "commit", "ref": "dc88ecee89d8e5958e38d9058df4ab5375a2ee55" }, { "kind": "command", "ref": "bd dep cycles --json" } ], "successors": [ "polylogue-eqq02" ] } ], "head_sha": "b6438e090dcadd36c859a727a2806708e2245c35", "scope_digest": "c6bc811ecd2f02c86fbba1ab3960972a3274002e297a5f423b1685f54084fc76", "version": 1 } -->
1 participant
Summary
Add the scoped maintenance surface required to reconcile the one known cursor-ahead violation without disabling the global fail-closed authority gate.
Problem
PR #3823 made cursor authority load-bearing across watcher, catch-up, flush, convergence, recovery, and reindex selection. That correctly parks ordinary ingestion while the known cursor-ahead relation remains, but leaves no safe route to reconcile the exact affected path. The live archive also contains 725/2 typed incomparable rows that must remain visible and must not be mistaken for the true ahead violation.
Solution
Add
polylogue ops maintenance cursor-authority-reconcile, dry-run by default. The plan path is supplied through a mode-0600 file, the archive root is fixed to/realm/db/polylogue, daemon ownership is required to be absent, source/index/ops/audit fingerprints and schema versions are captured, the selected byte-authoritative accepted head is checked against a two-stat source-prefix hash, and the plan is immutable and self-digested. Apply requires a verifiedfull_evidencebackup, reacquires writer ownership, revalidates every binding, installs a context-local single-use authorization bound to path digest, cursor offset, accepted frontier, and plan digest, then invokes the existing full-ingest route. No direct cursor, accepted-head, or source-row edits are present. The postcondition is either reconciled or an explicitly typed deferred attempt; the pre-existing incomparable population must remain unchanged. Behavior tests cover the real watcher seam, immutable plans, permissions, tampering, multiple-ahead refusal, source mutation, single-use and changed-frontier authorization, healthy-archive refusal, backup evidence, CLI surface, raw-frontier behavior, and exact-frontier ingestion.Verification
direnv exec . devtools test tests/unit/maintenance/test_cursor_authority_reconcile.pyproduced23 passed.direnv exec . devtools test tests/unit/sources/test_live_watcher.py -k 'cursor_authority or authoritative_frontier'produced2 passed, 107 deselected.direnv exec . devtools test tests/unit/storage/test_raw_retention.py -k cursorproduced6 passed, 59 deselected.direnv exec . devtools test tests/unit/cli/test_archive_maintenance_cli.py -k 'cursor_authority or cursor or maintenance_help'produced9 passed, 69 deselected.direnv exec . devtools verify --quickproduced exit code0, including format, lint, mypy, generated surfaces, layering, graph, schema, CI, documentation, and policy checks.Follow-ups
The implementation is intentionally partial.
polylogue-cursor-authority-live-proofremains open for the operator-controlled production dry-run, fresh backup, apply, immutable receipt, and consumption bypolylogue-live-operation-receipts. No production access or live receipt is included here.Ref polylogue-cursor-authority-reconcile-implementation
Summary by CodeRabbit
New Features
cursor-authority-reconcilemaintenance command with dry-run planning and controlled apply workflows.Documentation
Bug Fixes