fix(raw-authority): make frontier recovery executable - #3908
Conversation
📝 WalkthroughWalkthroughThe PR makes raw-authority frontier application daemon-owned and inspection-only through the CLI. It adds guarded offline recovery for census ledgers and orphaned index seeds, with typed plans, executor routing, durable intents, receipts, validation, and restart handling. ChangesRaw-authority convergence and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant RecoveryCLI
participant RawAuthorityRecovery
participant MutationExecutor
participant Archive
Operator->>RecoveryCLI: request inspect, apply, or resume
RecoveryCLI->>RawAuthorityRecovery: submit typed recovery operation
RawAuthorityRecovery->>MutationExecutor: authorize guarded actuator
MutationExecutor->>Archive: persist intent and receipt
Archive-->>RecoveryCLI: return recovery report
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
@codex review Please review exact pushed head |
![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: 152e7c19bb
ℹ️ 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 apply_changes: | ||
| if plan_file is None: | ||
| raise click.ClickException("--apply requires --plan-file from a prior inspect") | ||
| report = apply_raw_authority_recovery(plan_file, backup_manifest=backup_manifest) |
There was a problem hiding this comment.
When --apply receives a valid plan from another archive, or a plan whose operation differs from --operation, this call executes the archive root and operation embedded in the plan while ignoring both selected and env.config.archive_root. An operator can therefore request an index-seed prune against archive A but accidentally reset the census ledger in archive B; reject the apply unless the plan's operation and archive identity match the current CLI request.
Useful? React with 👍 / 👎.
| _same(_protected_digest(conn, excluded=excluded), plan.protected_digest, "protected source rows") | ||
| _backup_from_plan(plan, connection=conn) | ||
| for table in _RESET_TABLES: | ||
| conn.execute(f"DELETE FROM {_quote_identifier(table)}") |
There was a problem hiding this comment.
For every successful recovery apply, _require_apply_preconditions first requires the daemon to be stopped and this code then deletes durable rows directly from the CLI process; archive ownership and RebuildLease do not establish the daemon writer coordinator lease. This creates a second production SQLite writer instead of routing the authorized request to polylogued, contrary to the repository's explicit single-writer invariant.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
| if any(after_counts.values()): | ||
| raise RawAuthorityRecoveryError("census reset postflight left ledger rows behind") | ||
| postflight = _postflight(conn, protected_digest=plan.protected_digest, excluded=excluded) | ||
| conn.commit() |
There was a problem hiding this comment.
If the process crashes, the receipt destination is unwritable, or the disk fills after this commit and before _write_immutable at line 737, the destructive mutation is durable but no authoritative receipt exists. Retrying the same plan cannot recover it because the changed fingerprints/counts make the freshly prepared plan stale, so the command reports failure and leaves an unreceipted effect; persist a recoverable intent/outcome around the commit or make restart reconciliation reconstruct and validate the receipt from terminal postconditions.
Useful? React with 👍 / 👎.
| records an inspection census only; it has no manual plan selector or apply | ||
| option. | ||
|
|
||
| ### `polylogue ops maintenance raw-authority-recovery` - break-glass ledger recovery |
There was a problem hiding this comment.
Operators following this documentation are explicitly taught that the new manual apply command is a "break-glass" tier, despite the structured scope requiring no break-glass semantics and the underlying issue requiring recovery to be framed as an explicit authorized consent flow. Reframe the command and documentation around its typed recovery authority rather than establishing a named escape-hatch category.
Useful? React with 👍 / 👎.
| args.operation.value, | ||
| ), | ||
| ), | ||
| affected_count=sum(report.plan.before_counts.values()), |
There was a problem hiding this comment.
For PRUNE_INDEX_SEEDS, before_counts contains the total population of both index tables, so this receipt reports every valid row as affected rather than counting only the candidate keys deleted. With one orphan among 100 valid heads and applications it claims 202 affected rows after deleting two, and with no orphans it still returns applied with a nonzero affected count; derive the count from candidate_keys and return a typed already-satisfied/not-applicable result for a zero-candidate plan.
Useful? React with 👍 / 👎.
| postflight = receipt.get("postflight") | ||
| if not isinstance(postflight, dict) or receipt.get("protected_digest_after") != postflight.get("protected_digest"): | ||
| raise RawAuthorityRecoveryError("existing recovery receipt has inconsistent postflight evidence") |
There was a problem hiding this comment.
When a receipt file is replaced or fabricated with a recomputed self-hash and the matching plan digest/operation ID, this validation compares the live files only with the receipt's caller-controlled *_after fingerprints and checks that two fields inside the receipt agree with each other. It never verifies the receipt format, operation, backup binding, expected protected digest or after-counts, nor that the operation-specific target rows are actually absent, so a receipt describing the unchanged pre-apply state can make apply_raw_authority_recovery return already_satisfied without performing the recovery; recompute and enforce the exact terminal postconditions before accepting this shortcut.
Useful? React with 👍 / 👎.
| report = _apply_plan(live) | ||
| return MutationReceipt( | ||
| operation=self.operation, | ||
| plan_hash=args.expected_plan_digest, |
There was a problem hiding this comment.
Every successful executor-routed recovery returns args.expected_plan_digest as MutationReceipt.plan_hash, but authorization and stale-plan checking were performed against the distinct MutationPlan.plan_hash produced by prepare. The resulting receipt therefore cannot be correlated with the authorization that permitted the mutation and appears to attest a plan hash the executor never authorized; retain the supplied plan and stamp its plan_hash in the mutation receipt while keeping the domain recovery digest in domain evidence.
Useful? React with 👍 / 👎.
| "blackboard-post-loop", | ||
| "assertion-candidate-capture-loop", | ||
| "raw-authority-blocker-resolution-loop", | ||
| "raw-authority-recovery-loop", |
There was a problem hiding this comment.
The mutation-routes lane now claims raw-authority-recovery-loop and both recovery operations as covered by test_raw_authority_reset.py, but that test module never invokes the Click adapter or daemon dispatch; it calls inspect_raw_authority_recovery and apply_raw_authority_recovery directly. Consequently the generated coverage remains complete even if the production command ignores its operation/archive arguments or stops dispatching entirely, so add a real CLI/daemon route test with a mutation twin before assigning these path and operation targets to this lane.
Useful? React with 👍 / 👎.
|
@codex review Please review exact pushed head |
|
@codex review Please review exact pushed head |
|
@circleci run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68527d4d73
ℹ️ 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".
| after_counts = _count_tables(conn, _INDEX_TARGETS) | ||
| expected_after = {key: plan.before_counts[key] - len(plan.candidate_keys[key]) for key in _INDEX_TARGETS} | ||
| if after_counts != expected_after: | ||
| return None |
There was a problem hiding this comment.
On the index-prune restart path, a committed deletion is recognized only when the current total row counts still equal the immediate post-transaction totals. If the process crashes after commit but before receipt publication and the daemon subsequently appends a legitimate raw_revision_applications row, this comparison returns None; _apply_plan then retries the stale plan and fails fingerprint revalidation, so the already-committed destructive operation can never receive its authoritative receipt. Fresh evidence is the new _committed_postflight recovery path's dependence on aggregate counts rather than the planned candidate keys; verify the exact candidates are absent while allowing subsequent valid rows when reconstructing the receipt.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
| if not create: | ||
| raise | ||
| with suppress(FileExistsError): | ||
| os.mkdir(component, mode=0o700, dir_fd=current_fd) |
There was a problem hiding this comment.
When the recovery directory—or a custom nested receipt directory—does not already exist, this creates each component but never fsyncs the parent that received the new directory entry. Fsyncing only the final directory later does not make its ancestors durable, so a power loss after the SQLite mutation commits can discard the supposedly fsynced intent directory and recreate the unreceipted destructive state this recovery protocol is intended to prevent. Fresh evidence is the new descriptor-relative publication path creating these ancestors without persisting their directory entries; fsync each parent immediately after mkdir before proceeding.
Useful? React with 👍 / 👎.
|
|
||
| def _read_json_at(directory_fd: int, name: str, *, display_path: Path) -> dict[str, object]: | ||
| try: | ||
| artifact_fd = os.open(name, os.O_RDONLY | os.O_NOFOLLOW, dir_fd=directory_fd) |
There was a problem hiding this comment.
If a corrupted or adversarial archive contains a FIFO at the expected receipt or intent name, this blocking os.open waits indefinitely for a writer instead of producing the promised fail-closed “not a regular archive-owned file” error. O_NOFOLLOW rejects symlinks but does not establish that the opened inode is a regular file; open without blocking and verify stat.S_ISREG(os.fstat(fd).st_mode) before passing the descriptor to json.load.
Useful? React with 👍 / 👎.
| current_location = ArchiveLocation.resolve(root) | ||
| assert_owns_archive_location(owned, current_location) | ||
| with RebuildLease(root): | ||
| return _apply_plan(selected) |
There was a problem hiding this comment.
If the first process crashes after _write_recovery_intent but before the SQLite transaction commits, the intent exists while _committed_postflight returns None; this branch nevertheless calls _apply_plan directly, which proceeds to perform the destructive deletes without OperationExecutor.execute and therefore emits no executor-bound mutation receipt. Fresh evidence in the new resume branch is that it handles both committed and uncommitted intents even though the declared exception is only receipt finalization; directly finalize only a proven committed outcome, and route an uncommitted intent back through executor authorization or refuse it.
Useful? React with 👍 / 👎.
|
@circleci run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa1764408
ℹ️ 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".
| from polylogue.storage.archive_identity import ArchiveIdentity | ||
|
|
||
| identity = ArchiveIdentity.resolve_location(location) | ||
| return identity.as_dict(unit="raw-authority-recovery") |
There was a problem hiding this comment.
ArchiveIdentity.as_dict() includes the current process_id, executable, and invocation ID, but _revalidate_common later requires this entire payload to equal the identity stored in the plan. Consequently, the documented two-command flow—write a plan during one CLI invocation, then run --apply --plan-file in another—always rejects the plan as stale because the PID changes; a successfully applied plan likewise cannot return already_satisfied from a later process. Persist and compare a stable archive identity rather than process-scoped diagnostic fields.
Useful? React with 👍 / 👎.
| if any(after_counts.values()): | ||
| raise RawAuthorityRecoveryError("census reset postflight left ledger rows behind") | ||
| postflight = _postflight(conn, protected_digest=plan.protected_digest, excluded=excluded) | ||
| conn.commit() |
There was a problem hiding this comment.
When the archive has a released source durable-change train, this commit changes source.db content but never writes a source-continuity pending intent or calls refresh_released_source_train_continuity. The next daemon startup runs reconcile_durable_change_trains_on_startup, whose released-train check compares the live content hash with the pre-reset evidence and therefore rejects the legitimately reset archive; the same stale evidence also blocks the next durable migration. Route this source mutation through the existing continuity-refresh protocol, including its pre-commit recovery intent, before reporting the reset complete.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@docs/maintenance.md`:
- Around line 797-799: Update the maintenance heading for `polylogue ops
maintenance raw-authority-recovery` to remove “break-glass” and describe the
route using terminology consistent with its guarded, plan-bound, receipt-bound
offline operation guarantees.
In `@polylogue/artifacts/runtime.py`:
- Around line 671-676: Update the raw_authority_index_seed_recovery_receipt
artifact definition to use the durable artifact layer instead of
ArtifactLayer.PROJECTION. Preserve its immutable receipt metadata, dependencies,
code references, and readiness surfaces.
In `@polylogue/cli/commands/maintenance/_raw_authority_recovery.py`:
- Around line 65-77: Update the --apply plan-file branch in the command handler
to load the artifact with RawAuthorityRecoveryPlan.from_dict, validate that its
operation matches the requested --operation, and refuse mismatches before
applying changes. Pass the validated plan object to apply_raw_authority_recovery
instead of allowing that function to independently derive the operation from the
file.
In `@polylogue/maintenance/raw_authority_recovery.py`:
- Around line 1086-1098: The _RecoveryActuator plan and receipt derive
destructive scope from constants instead of args.operation. In prepare
(1086-1098) and apply (1116-1121) of
polylogue/maintenance/raw_authority_recovery.py, derive the tier from
args.operation and pass it to make_target_ref; in the receipt at 1122, set
affected_count to the deleted candidate_keys count for PRUNE_INDEX_SEEDS or
before_counts count for RESET_CENSUS, and report 0 when report.applied is false.
- Around line 209-236: Update _validate_ledger so each table’s PRAGMA table_info
lookup runs once before iterating its rows, then reuse the resulting column-name
list for every row while preserving the existing JSON validation and error
behavior.
- Around line 288-294: Update the exclusion handling in the recovery query flow
around _index_seed_digest so excluded_keys is not expanded into an unbounded
number of SQL placeholders. Filter rows against the excluded set in Python or
stage the keys in a temporary table and join, while preserving the current
behavior of excluding matching key_column values in both plan-build and
postflight verification paths.
In `@tests/unit/cli/test_archive_maintenance_cli.py`:
- Around line 1873-1876: Split the parameterized test around its shared
assertions: keep the archive setup, JSON payload, group help, and command help
checks in one unparameterized test, and move only the option-rejection
assertions into a separate test parameterized by option and value. Preserve the
existing rejection cases and expected results while ensuring the shared CLI
inspections run only once.
In `@tests/unit/maintenance/test_raw_authority_reset.py`:
- Around line 310-328: Strengthen
test_uncommitted_recovery_intent_reauthorizes_through_executor by asserting
after the expected authorization failure that the seeded census row remains
unchanged. Re-read the ledger or census data from source.db using the existing
test helpers and verify the r-keep entry survives, proving
apply_raw_authority_recovery performs no mutation before
OperationExecutor.authorize.
🪄 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: c9a4c30a-5c86-4d3b-a5a4-cd6dd0d147f0
📒 Files selected for processing (26)
.beads/issues.jsonldevtools/validation_lane_catalog_contracts.pydevtools/verify_raw_authority_frontier_executability.pydocs/daemon.mddocs/evidence/polylogue-xeck9-cursor-authority-census-2026-08-04.mddocs/maintenance.mddocs/plans/mutation-census.yamldocs/test-quality-workflows.mdpolylogue/artifacts/runtime.pypolylogue/cli/commands/maintenance/__init__.pypolylogue/cli/commands/maintenance/_raw_authority_recovery.pypolylogue/cli/commands/maintenance/_raw_identity.pypolylogue/daemon/cli.pypolylogue/maintenance/raw_authority_recovery.pypolylogue/maintenance/raw_authority_reset.pypolylogue/operations/specs.pypolylogue/product/raw_authority.pypolylogue/storage/raw_authority.pypolylogue/storage/raw_reconciler.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/cli/test_maintenance_registration.pytests/unit/daemon/test_daemon_cli.pytests/unit/maintenance/test_raw_authority_reset.pytests/unit/operations/test_mutation_census.pytests/unit/operations/test_specs.pytests/unit/product/test_raw_authority.py
|
@codex review Please review exact pushed head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa1764408
ℹ️ 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 _index_seed_digest(conn) != plan.post_target_digest: | ||
| raise RawAuthorityRecoveryError("recovery intent does not match the exact committed index seed state") |
There was a problem hiding this comment.
When the prune commits but final receipt publication fails, a later daemon ingest can legitimately append a new raw_revision_applications row before the operator resumes. The aggregate count check returns None, and this new whole-table digest would likewise reject any successor row; the code then attempts a fresh PREPARE against the already-mutated archive and rejects the original plan as stale, leaving the committed prune permanently unreceipted. Fresh evidence in 1fa1764 is that _index_seed_digest(conn) binds recovery to the entire live tables rather than proving that the planned retained rows are unchanged and the exact candidate keys are absent while permitting append-only successors.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
| placeholders = ", ".join("?" for _ in excluded) | ||
| rows_query = f"SELECT * FROM {quoted} WHERE {_quote_identifier(key_column)} NOT IN ({placeholders})" | ||
| rows = conn.execute(rows_query, excluded) |
There was a problem hiding this comment.
When either orphan candidate set exceeds SQLite's host-parameter limit, constructing one NOT IN placeholder per key makes plan inspection fail with OperationalError: too many SQL variables before recovery can run. The checked build limits a statement to 250,000 variables, and other SQLite builds can impose lower limits; an append-only raw_revision_applications history can therefore make the recovery path unusable precisely on a large archive with many orphaned decisions. Compute the retained-row digest without binding the complete candidate set in one statement, such as by streaming rows and excluding keys in Python or batching safely.
Useful? React with 👍 / 👎.
| if created: | ||
| os.fsync(current_fd) | ||
| os.fsync(next_fd) |
There was a problem hiding this comment.
If the process dies after os.mkdir succeeds but before these conditional fsyncs, the next resume opens that component through the normal existing-directory branch, leaves created false, and never fsyncs the parent that contains the potentially non-durable directory entry. Fsyncing deeper directories and the final receipt directory does not persist that ancestor entry, so the retry can durably write an intent, commit the SQLite mutation, and still lose the entire intent subtree on a later power failure. Fresh evidence in 1fa1764 is that parent persistence is conditional on the directory having been created during the current invocation rather than repairing a directory left by an interrupted invocation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 207a01bf51
ℹ️ 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 operation is RecoveryOperation.RESET_CENSUS: | ||
| _write_source_continuity_pending_intent(plan) |
There was a problem hiding this comment.
When source.db changes after inspection, or _backup_from_plan rejects changed backup evidence, this call has already persisted a source-continuity pending intent, but the subsequent checks at lines 1080–1085 roll back without removing it or creating the referenced receipt. The original plan can no longer be resumed because it is stale, while daemon startup fails in _recover_pending_source_continuity_intents when that receipt is missing and later source mutations are blocked by assert_source_continuity_apply_allowed; ensure every precommit refusal durably clears the pending intent, while preserving it only once the reset may have committed.
Useful? React with 👍 / 👎.
| rows = [[_value_for_digest(value) for value in row] for row in conn.execute(f"SELECT * FROM {quoted}")] | ||
| rows.sort(key=_canonical_bytes) |
There was a problem hiding this comment.
During an index-seed recovery inspection, _protected_digest invokes this function for every non-target table in the active index, and this comprehension retains every row of each table in Python before sorting it; byte columns are also expanded to hex by _value_for_digest. On a multi-GiB archive, a large blocks or FTS shadow table can therefore exhaust memory before the dry-run plan is produced. Compute the deterministic digest incrementally with bounded-memory ordering rather than materializing a complete table.
Useful? React with 👍 / 👎.
|
@circleci run |
1 similar comment
|
@circleci run |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a5c744ccb
ℹ️ 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 operation is RecoveryOperation.PRUNE_INDEX_SEEDS: | ||
| _verify_index_seed_post_target(conn, plan) |
There was a problem hiding this comment.
If the process crashes after publishing the prune intent but before deleting any SQLite rows, this call invokes _verify_index_seed_post_target, which raises because the planned candidates are still present instead of returning None for an uncommitted outcome. Both plan-based apply and --operation-id resume therefore abort before reaching the executor reauthorization at line 1389, leaving the valid recovery intent permanently unusable. Fresh evidence after the earlier authorization issue is that the new reauthorization branch cannot actually be reached for interrupted index prunes; distinguish candidate-presence-before-commit from a malformed committed postflight and continue through OperationExecutor in that case.
Useful? React with 👍 / 👎.
| row_digests: dict[str, str] = {} | ||
| for row in conn.execute(f"SELECT * FROM {quoted}"): | ||
| key = str(row[key_index]) | ||
| if key in excluded: | ||
| continue | ||
| if key in row_digests: | ||
| raise RawAuthorityRecoveryError(f"index seed table has a duplicate primary key: {table}.{key_column}") | ||
| row_digests[key] = _digest({"columns": columns, "row": [_value_for_digest(value) for value in row]}) | ||
| payload[table] = row_digests |
There was a problem hiding this comment.
When raw_revision_applications contains a large append-only history, this dictionary retains one key and SHA-256 string for every non-candidate row, and the resulting mapping is then embedded in both the plan and durable intent. Inspection and apply therefore require memory and artifact space proportional to the complete target tables—potentially hundreds of megabytes or more—even when only one orphan is being pruned. Preserve the exact retained-row proof in a streaming or otherwise bounded representation rather than constructing the whole keyed snapshot in memory.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/operations/mutation_transaction.py`:
- Line 835: Extend the target-authority policy used by
PruneOrphanedIndexRevisionSeedsActuator so raw-authority-recovery-loop
authorizes both source and index target kinds, matching the refs generated by
make_target_ref.
In `@polylogue/storage/sqlite/durable_change_train.py`:
- Around line 803-814: The source receipt classifier in
_source_mutation_receipt_phase must treat a missing receipt as a distinct “not
yet finalized” phase instead of raising DurableChangeTrainError; update
_recover_pending_source_continuity_intents to skip that intent while continuing
reconciliation for other archive intents. In
polylogue/storage/sqlite/durable_change_train.py lines 803-814, implement the
missing-file classification and skip behavior. In
polylogue/maintenance/raw_authority_recovery.py lines 1186-1198, document or
narrow the window where write_source_continuity_pending_intent references
plan.receipt_path before the receipt exists, while preserving
resume_raw_authority_recovery resolution after interruption.
- Around line 951-953: Update the after_counts validation in the receipt
verification flow to require a non-empty dict whose values are integers,
rejecting empty mappings and non-integer entries before checking that every
count is zero. Preserve the existing DurableChangeTrainError behavior for
invalid or non-zero counts.
🪄 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: 2a1594ea-deeb-495c-ba49-c70364075d72
📒 Files selected for processing (8)
docs/maintenance.mdpolylogue/artifacts/runtime.pypolylogue/cli/commands/maintenance/_raw_authority_recovery.pypolylogue/maintenance/raw_authority_recovery.pypolylogue/operations/mutation_transaction.pypolylogue/storage/sqlite/durable_change_train.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/maintenance/test_raw_authority_reset.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16ad7a565a
ℹ️ 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".
| """Classify a committed source-maintenance receipt without guessing its format.""" | ||
|
|
||
| try: | ||
| raw = json.loads(receipt_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When a pending raw-authority source-continuity intent points to a receipt path that has been replaced by a FIFO, this startup classifier blocks indefinitely in Path.read_text() before the later receipt validation can reject it, preventing polylogued from starting. Although the recovery artifact reader now opens nonblocking and verifies regular files, this newly added continuity path bypasses that protection; open the receipt with O_NONBLOCK | O_NOFOLLOW and verify stat.S_ISREG before reading.
Useful? React with 👍 / 👎.
| if not daemon_write_lease_active(): | ||
| raise RuntimeError("raw authority frontier apply requires the daemon writer lease") |
There was a problem hiding this comment.
When a coordinator-held operation spawns a child task, the child inherits _ACTIVE_LEASE; if it outlives the owning task, daemon_write_lease_active() remains true in that copied context even after the coordinator has released and potentially reassigned the gate. This new guard therefore permits the detached task to run apply_raw_authority_frontier concurrently with the real lease owner, despite DaemonWriteCoordinator.run explicitly rejecting inherited leases whose recorded task is not asyncio.current_task(). Apply the same owner/liveness check here while preserving the intended worker-thread propagation.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
Problem: count conservation was independently implemented in the storage report constructor and product wrapper, allowing the two correctness authorities to drift. What changed: extract one storage-side validator used by both boundaries, then cover incoherent report construction and product rejection of a coherent untyped response separately. Adversarial review: iteration 1 identified and this commit removes the duplicated validation logic.
Problem: a typed coherent actuator receipt could still report a different selected-plan count or preview census than the request passed through the product boundary. What changed: bind validation to the exact selected plan tuple length and preview census id, with regressions for both mismatches. Adversarial review: iteration 2 identified the missing request binding and this commit closes it.
Bind actuator outcome references to the selected plan ids and reject product applies without the daemon writer lease. Exercise the real coordinator path with coherent receipts and retain inspection-only CLI behavior.
Problem: the integrated frontier and recovery routes needed one explicit operator-facing ownership statement and a real test that legacy storage helpers cannot mutate either target set. What changed: document daemon-owned frontier application beside the guarded recovery family, record that boundary in the mutation census plan, and test both direct compatibility helpers fail closed. Compatibility/migration: routine frontier application remains daemon-only; reset_raw_authority_census and prune_orphaned_index_revision_seeds retain the existing inspect/apply recovery command family.
Add real recovery tests for a running daemon and a changed source-tier fingerprint. Both cases refuse before deleting ledger rows and preserve the poisoned state for later operator recovery.
Problem: recovery committed its target transaction before writing the only receipt, so a receipt I/O failure left no restartable evidence of the mutation. Receipt destinations also accepted arbitrary paths, while the route was described too broadly as daemon-owned.\n\nWhat changed: persist and fsync an immutable archive-owned intent before mutation, finalize it after commit, and resume that intent under offline ownership after a final receipt failure. Reject receipt paths outside the archive maintenance state and state the stopped-daemon operator-maintenance boundary in the operation registry and docs.\n\nCompatibility: routine raw-authority frontier convergence remains daemon-owned. Break-glass recovery remains an explicit offline operator action.
Problem: a completed mutation with a failed final receipt could only be resumed from a retained external plan, and an archive receipt route could traverse a symlink.\n\nWhat changed: persist the complete plan in the fsynced intent, add operation-id resume, reject archive symlink traversal, and record the offline durable-intent continuation boundary.\n\nCompatibility: fresh recovery applies remain executor-routed; only a previously authorized durable intent can finalize its receipt offline.
Problem: an operation-id resume could not find a permitted custom receipt path, and pathname-based publication still had a symlink replacement window.\n\nWhat changed: persist and read recovery artifacts through descriptor-relative no-follow file operations, accept the original archive-owned receipt path during resume, and prove a custom-path recovery survives a failed final receipt write.\n\nCompatibility: external plan artifacts keep their existing path behavior; durable receipt and intent artifacts remain restricted to archive-owned maintenance state.
Problem: count-only recovery finalization could accept altered retained index rows; receipt paths also accepted non-regular artifacts, and an uncommitted intent skipped the executor authorization boundary. What changed: persist and verify the exact retained index-seed digest, fsync each new receipt directory, reject non-regular receipt artifacts before reading, and reauthorize uncommitted intents through the maintenance executor. Compatibility: persisted index-prune intents without a post-target digest now fail closed rather than resuming against unverifiable state.
Persist recovery artifacts with stable archive identity, bind the CLI operation to its plan, and extend source reset through the durable source-train continuity protocol.\n\nRecord exact retained index candidate state for restart-safe pruning while allowing append-only successors, avoid unbounded SQLite bindings, fsync all receipt parents, and report correct actuator targets. Older index intents without retained state fail closed.
Clear a source-continuity pending intent only after its source transaction is demonstrably rolled back, leaving it durable after any committed or ambiguous outcome. Stream protected-table fingerprints by primary key or rowid so inspection never collects an entire table in Python memory.
Treat an intact index-prune intent as uncommitted so resume reaches the executor authorization boundary. Replace retained-row maps with streaming watermark proofs that retain only the candidate rowids already required for deletion, while allowing append-only application successors.
Declare the source and index recovery targets in the operation catalog. Keep an unfinalized source receipt pending for raw-authority resume, continue other startup recovery, and require non-empty integer zero-count reset proof.
Keep the authorized mutation plan through recovery execution so the receipt proves the same plan hash that was authorized. Exercise the full executor authorize/execute path.
Reject dirty source builds during recovery planning and revalidation. Preserve the declared durable target class when the executor materializes a legacy actuator target.
Preserve bounded recovery proofs across source-backed successor heads, distinguish reset receipt finalization from missing liveness evidence, and block overlapping source mutations.
Keep the pre-commit continuity-intent interception aligned with the typed mutation kind and assert that census reset emits raw_authority_recovery.
Bind census reset plans to WAL-visible ledger rows, admit source-backed head supersession during receipt recovery, and defer stale source-train validation until an interrupted reset receipt is finalized.
Assert that a WAL-visible census change alters the authorized recovery plan and is rejected before lease acquisition.
Keep recovery-plan deserialization aligned with the validated optional ledger digest type.
403d938 to
8408b5b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8408b5bb1c
ℹ️ 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 allowed_pending_operation_id is not None: | ||
| pending_intents = tuple( | ||
| path | ||
| for path in pending_intents | ||
| if _load_source_continuity_pending_intent(path).get("operation_id") != allowed_pending_operation_id |
There was a problem hiding this comment.
When a census reset reuses the operation_id of a pending blob-liveness mutation, this filter removes that unrelated intent solely because its ID matches; the reset can then publish a second continuity intent and mutate source.db while the first evidence chain is unresolved. Fresh evidence beyond the earlier pending-intent finding is that the new exemption does not check mutation_kind, receipt identity, or the recovery plan digest. Exempt only the exact matching raw-authority recovery intent so unrelated source mutations remain serialized.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
| return write_source_continuity_pending_intent( | ||
| root, | ||
| mutation_receipt=Path(plan.receipt_path), | ||
| backup_manifest=Path(str(authority["manifest_path"])), | ||
| pre_mutation_evidence=before, | ||
| operation_id=plan.operation_id, | ||
| evidence_ref=f"proof:raw-authority-recovery:{plan.plan_digest}", | ||
| mutation_kind="raw_authority_recovery", |
There was a problem hiding this comment.
If the first reset process dies after write_source_continuity_pending_intent replaces the pending file but before that helper fsyncs its directory, an uncommitted-intent retry reaches this call with the same pre-mutation evidence. The helper sees the existing file and returns without fsyncing source-continuity-pending, so the retry can commit the destructive reset while its continuity intent remains only in the page cache; a later power loss can discard that intent and leave the released source train bound to pre-reset evidence. Ensure the existing-file path fsyncs the containing directory before treating the intent as durable.
Useful? React with 👍 / 👎.
| return _build_plan( | ||
| archive_root, | ||
| operation=selected, | ||
| operation_id=operation_id or f"raw-authority-recovery:{uuid.uuid4().hex}", | ||
| backup_manifest=backup_manifest, |
There was a problem hiding this comment.
When the CLI or Python API supplies a whitespace-only operation_id, this truthiness fallback preserves it instead of generating an ID. A source census reset can then commit and publish its recovery receipt before continuity refresh calls _require_nonempty(operation_id) and fails; the command reports failure with a pending continuity intent, and daemon startup repeats the same refusal. Strip and validate the ID while building the plan so this input is rejected before any durable mutation.
Useful? React with 👍 / 👎.
1 participant
Summary
Make raw-authority frontier recovery executable through daemon-owned, receipt-bound maintenance routes and make interrupted durable recovery restartable.
Problem
Raw-authority recovery had actuator and receipt fragments, but a final-receipt failure could leave a committed mutation requiring an external plan artifact. Receipt paths also needed stronger archive ownership boundaries. Without restart-safe evidence, implementation success could be mistaken for live convergence.
Solution
This is intentionally a partial disposition for
polylogue-fbkr. It satisfies the implementation and recovery-closure slice but does not claim live raw-authority convergence or a production apply. Those residual obligations are carried bypolylogue-c0z2a.Verification
devtools test tests/unit/maintenance/test_raw_authority_reset.py tests/unit/operations/test_mutation_census.py(19 passed)devtools verify --quickon the exact pushed head (all 24 steps passed)Remaining scope
polylogue-c0z2aremains the partial-disposition successor for operator-side closure, including explicit daemon lease ownership (review finding 3746699042) and descriptor-safe, nonblocking continuity receipt handling (review finding 3746699036).Current exact-head verification
devtools test tests/unit/maintenance/test_raw_authority_reset.py tests/unit/storage/test_durable_change_train.py:95 passeddevtools verify --quick:exit 0, 24 stepsSummary by CodeRabbit
raw-authority-recoverymaintenance command for inspecting, planning, applying, and resuming guarded recovery operations.