fix(reindex): close residual rebuild correctness gaps (#3893) - #3893
Conversation
📝 WalkthroughWalkthroughThe pull request adds selected SQLite index observation, effective topology validation, pinned read-only archive access, descriptor-backed durable-tier publication, stronger archive ownership checks, and ordered empty-source rebuild handling. Tests and maintenance documentation cover race detection, cleanup outcomes, and report metadata. ChangesArchive evidence and topology validation
Archive access and durable initialization
Rebuild maintenance
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant AffordanceUsage
participant SQLiteIndex
participant SnapshotObserver
CLI->>AffordanceUsage: provide selected index
AffordanceUsage->>SQLiteIndex: open pinned database and sidecars
AffordanceUsage->>SnapshotObserver: capture before and after observations
SnapshotObserver-->>AffordanceUsage: return identity and stability
AffordanceUsage-->>CLI: emit report metadata
sequenceDiagram
participant RebuildCLI
participant RebuildIndex
participant ArchiveOwnership
participant RebuildTransaction
RebuildCLI->>RebuildIndex: request rebuild
RebuildIndex->>ArchiveOwnership: acquire ownership
RebuildIndex->>RebuildTransaction: validate or retire resumable operation
RebuildTransaction-->>RebuildIndex: return stale status or empty receipt
RebuildIndex-->>RebuildCLI: emit rebuild result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Sinity
changed the title
![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: 45303a6c0d
ℹ️ 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".
| ) -> dict[str, Any] | None: | ||
| if not effective_detail_patterns or args.family: | ||
| return None | ||
| if config.db_path.parent.resolve() != config.archive_root.resolve(): |
There was a problem hiding this comment.
When --index-db selects a differently named database inside the configured archive root and detail-pattern mode is used without --family, this check permits the product fast path even though ArchiveStore.open_existing(config.archive_root) always reads <archive-root>/index.db. The report then labels and hashes the selected database while its action counts come from another database, and snapshot_identity.stable can still be true. Only use this fast path when the selected path is exactly the database that ArchiveStore will open; otherwise retain the direct SQLite fallback.
Useful? React with 👍 / 👎.
| LEFT JOIN messages m ON m.session_id = l.src_session_id | ||
| WHERE l.resolved_dst_session_id IS NULL | ||
| AND COALESCE(NULLIF(TRIM(l.status), ''), 'unresolved') = 'unresolved' | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM session_links resolved | ||
| WHERE resolved.src_session_id = l.src_session_id |
There was a problem hiding this comment.
When every unresolved link belongs to a child that also has a usable resolved parent, this predicate correctly removes all rows from the sample, but unresolved_count is still the unfiltered count. The status logic therefore reports not_observed and the validation report adds a blocking “were not exercised” reason even though there are no effective unresolved-parent reads to exercise. Keep the total census separately, but base not_applicable versus not_observed on a count using this same effective-edge predicate. This distinction is required because session_links deliberately retains every asserted parent reference, including unresolved alternates.
AGENTS.md reference: AGENTS.md:L100-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 353bfa1eba
ℹ️ 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".
| ) -> dict[str, Any] | None: | ||
| if not effective_detail_patterns or args.family: | ||
| return None | ||
| if config.db_path.resolve() != (config.archive_root / "index.db").resolve(): |
There was a problem hiding this comment.
When an index generation is promoted after this comparison but before ArchiveStore.open_existing() runs, the atomic index.db symlink replacement in IndexGenerationStore.promote() makes the store read the new generation while conn, the observer, and both snapshot hashes remain pinned to the old resolved file. The report can therefore combine action counts from the new index with other counts and a stable: true identity for the old index. Fresh evidence beyond the earlier path-selection finding is this time-of-check/time-of-open race introduced by the new pre-open equality check; query through the selected connection or bind the opened store to that same physical database.
Useful? React with 👍 / 👎.
| existing_siblings.append(sibling) | ||
| train_marker_root = path.parent / ".maintenance-state" / "durable-change-trains" | ||
| train_marker = train_marker_root.is_dir() and any(train_marker_root.iterdir()) | ||
| if existing_siblings or location.active_pointer is not None or train_marker: |
There was a problem hiding this comment.
When all durable siblings are missing but an established root retains a dangling-symlink .index-active-pointer, ArchiveLocation.resolve() ignores that marker because it tests Path.exists(), leaving active_pointer as None; similarly, a non-directory durable-change-train marker path makes train_marker false. This condition therefore treats a damaged established archive as completely unadopted and can publish an empty durable database over a recovery situation. Inspect the marker paths themselves with lstat() and fail closed for malformed or dangling markers, since these tiers contain durable or irreplaceable evidence.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88091c42f3
ℹ️ 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 named_publication_path is not None: | ||
| os.close(descriptor) | ||
| publication_descriptor = None |
There was a problem hiding this comment.
When O_TMPFILE is unavailable, the fallback exposes a named staging file and closes its descriptor before linking it. A concurrent same-UID process can therefore replace or modify that path between the close and os.link; replacement is detected only after the unexpected inode has already been published at the durable-tier path, while in-place modification preserves the inode identity and is accepted as canonical. Refuse this fallback or publish from a protected descriptor/primitive, and remove any published target when identity validation fails, so source.db or irreplaceable user.db cannot be initialized from tampered bytes.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f3781f0b
ℹ️ 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".
| The flag builds the canonical database in memory, writes it into an anonymous | ||
| inode when the filesystem supports `O_TMPFILE`, and otherwise uses a same- | ||
| directory 0600 exclusive temporary file. Both paths fsync the image and | ||
| publish it with a no-replace hard link, then fsync the directory. It refuses |
There was a problem hiding this comment.
When the archive filesystem lacks or rejects O_TMPFILE, this runbook promises a same-directory temporary-file fallback, but initialize_missing_durable_tier() instead raises MigrationError both when the flag is unavailable and when the anonymous open fails. Operators following this recovery procedure will therefore encounter an undocumented hard refusal; describe the fail-closed requirement rather than a fallback that no longer exists.
Useful? React with 👍 / 👎.
| initialize=initialize, | ||
| read_only=read_only, | ||
| read_timeout=read_timeout, | ||
| frozen_index_path=index_path, |
There was a problem hiding this comment.
When the active pointer is promoted after the caller resolves index_path, this new path correctly pins the old physical generation, but _initialize_store() still invokes _ensure_read_runtime_indexes() for every non-frozen read-only open. That helper opens the pinned database read-write and executes index DDL, so an affordance evidence run can mutate a now-inactive generation without generation ownership whenever a runtime index is missing; pinned evidence opens should bypass this repair and remain genuinely read-only.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified at published head 788a7993ecd4f9c62b3c05d30df8d0dc362a3493: read-only ArchiveStore initialization does not invoke write-time runtime-index repair, and the pinned descriptor path stays read-only. The existing pinned-read production coverage and the final quick gate pass. No additional change is required for this thread.
| ) from exc | ||
| if location.active_pointer is not None and active_pointer_marker not in adoption_markers: | ||
| adoption_markers.append(active_pointer_marker) | ||
| if existing_siblings or adoption_markers: |
There was a problem hiding this comment.
When all three durable database files are missing but an established pre-pointer archive still retains its conventional index.db, this condition sees neither a durable sibling nor either selected marker and permits publication of an empty source, user, or audit tier. A normal fresh bootstrap records .bootstrap.pending before creating tiers, so an unmarked surviving index is a damaged or legacy archive rather than a completely unadopted directory; fail closed on existing archive tiers (and retained blob evidence) instead of manufacturing new durable authority alongside them.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polylogue/operations/durable_change_train.py (1)
175-183: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap anonymous publication link failures in
MigrationError.
initialize_missing_durable_tier()preserves the tier-specific recovery contract for all setup failures, but anyOSErrorfromos.link()during anonymous publication leaves the maintenance command with a raw system error. Add a catch-allexcept OSErrorafter the collision handler and wrap it ascannot initialize missing {tier.value} tier: anonymous durable publication failed: {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/operations/durable_change_train.py` around lines 175 - 183, Update initialize_missing_durable_tier() to catch any OSError from the anonymous os.link() publication after the existing FileExistsError handler, and raise MigrationError with the message “cannot initialize missing {tier.value} tier: anonymous durable publication failed: {path}”. Preserve the existing collision-specific MigrationError and exception chaining.
🤖 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 `@devtools/affordance_usage.py`:
- Around line 194-290: Consolidate the duplicated snapshot helpers by reusing
the shared _file_sha256 and _data_version implementations from the existing
devtools module used by lineage_validation.py. Move or centralize
_snapshot_observation and _snapshot_identity there as needed so
affordance_usage.py consumes the same snapshot identity contract, then remove
its local duplicate definitions and update callers/imports accordingly.
In `@devtools/lineage_validation.py`:
- Around line 331-346: Extract the shared effective-link `NOT EXISTS` predicate
used by the count and sample queries into one query helper or SQL fragment, then
reuse it in both `effective_unresolved_count` and the sample-query construction.
Preserve the existing filtering semantics so both queries always select the same
effective unresolved links.
In `@polylogue/maintenance/rebuild_index.py`:
- Around line 1342-1355: Reduce redundant count_source_raw_sessions scans in the
rebuild flow: cache the pre-ownership result used by
_rebuild_index_from_source_owned instead of recomputing it at the later
pre-ownership check, while keeping the ownership-held check authoritative. Pass
the count from that ownership-held check into _rebuild_index_from_source_owned
and use it in place of the downstream probe, including the check near the
function’s existing raw-session handling.
- Around line 1213-1229: Update _empty_source_receipt to accept a
consumed_evidence parameter and use it in the returned RebuildIndexReceipt.
Replace the inline empty-source receipt construction in
_rebuild_index_from_source_owned with a call to _empty_source_receipt(root,
consumed_evidence=consumed_evidence), preserving the existing empty-source
receipt fields.
In `@tests/unit/cli/test_archive_maintenance_cli.py`:
- Around line 2604-2632: Remove the duplicate test function
test_migrate_tier_cli_missing_initialization_refuses_malformed_train_marker,
retaining the equivalent malformed-marker-parent coverage. If the differing file
contents are intentional, consolidate both cases by parameterizing the existing
test while preserving their shared assertions.
- Around line 523-531: Update _refresh_fresh_bootstrap_marker to assert that the
.bootstrap marker exists instead of returning silently when marker.is_file() is
false; retain the existing unlink and _record_fresh_durable_bootstrap flow once
the precondition is satisfied.
- Around line 2478-2485: Make the fail_blob_inspection monkeypatch preserve
Path.iterdir’s lazy failure timing by returning a generator that raises OSError
when consumed for blob_root, not when called. Ensure the production code’s
surrounding exception handling also covers both iterator creation and iteration
so the blob inspection failure is caught regardless of when it surfaces.
In `@tests/unit/devtools/test_lineage_validation.py`:
- Line 397: The report-generation logic in devtools/lineage_validation.py must
use singular wording when the unresolved-parent count is 1 and plural wording
otherwise; update the corresponding assertion in test_lineage_validation.py to
expect “1 effective unresolved-parent link was not exercised through the
reader.”
In `@tests/unit/maintenance/test_rebuild_index_provenance_gate.py`:
- Around line 133-143: Strengthen the transaction cleanup assertion in
test_nonempty_source_still_requires_schema_inference_receipt_after_ownership by
asserting that .index-rebuild-transactions does not exist, rather than globbing
its contents. Also distinguish this test from
test_missing_receipt_fails_before_lease_and_candidate_mutation by documenting
the ownership-ordering behavior it uniquely verifies, or fold the assertion into
that existing test if no distinct coverage remains.
---
Outside diff comments:
In `@polylogue/operations/durable_change_train.py`:
- Around line 175-183: Update initialize_missing_durable_tier() to catch any
OSError from the anonymous os.link() publication after the existing
FileExistsError handler, and raise MigrationError with the message “cannot
initialize missing {tier.value} tier: anonymous durable publication failed:
{path}”. Preserve the existing collision-specific MigrationError and exception
chaining.
🪄 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: 9a14ce43-4383-4fa7-867f-d5cc93510730
📒 Files selected for processing (13)
.beads/issues.jsonldevtools/affordance_usage.pydevtools/lineage_validation.pydocs/maintenance.mdpolylogue/maintenance/rebuild_index.pypolylogue/operations/durable_change_train.pypolylogue/storage/sqlite/archive_tiers/archive.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/devtools/test_affordance_usage.pytests/unit/devtools/test_lineage_validation.pytests/unit/maintenance/test_rebuild_index_ownership.pytests/unit/maintenance/test_rebuild_index_provenance_gate.pytests/unit/storage/test_schema_policy_contracts.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fdd8dd86b
ℹ️ 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".
| durable_siblings = tuple(path.parent / f"{sibling.value}.db" for sibling in ArchiveTier if sibling is not tier) | ||
| existing_siblings: list[Path] = [] | ||
| for sibling in durable_siblings: | ||
| if adoption_lstat(sibling, "archive tier") is not None: | ||
| existing_siblings.append(sibling) |
There was a problem hiding this comment.
When the conventional tier files, active pointer, and blob directory are missing but .index-generations/gen-*/generation.json or .index-rebuild-transactions/*.json remains, this census ignores those canonical roots even though IndexGenerationStore defines them as archive lifecycle state (polylogue/storage/index_generation.py:504-505). The damaged established archive is therefore treated as unadopted, allowing an empty source, user, or audit database to be published alongside retained rebuild evidence. Fresh evidence beyond the earlier conventional-tier finding is that nested generation and transaction state still bypasses the new sibling scan; fail closed when either root contains entries.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0ac9274b5
ℹ️ 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".
| try: | ||
| metadata_before = path.stat() | ||
| except FileNotFoundError: | ||
| files.append({"path": str(path), "present": False}) |
There was a problem hiding this comment.
When a selected physical generation is unlinked after the SQLite connections open but before the first observation—for example, by concurrent generation retention or candidate discard—this branch records the main file as absent without clearing observation_complete. Both observations can then match while data_version remains unchanged because the connections still read the deleted inode, allowing affordance evidence to report stable: true and lineage evidence to remain citable even though the digest contains no database bytes. Fresh evidence beyond the earlier pointer-promotion race is this missing-main-file path in the new shared snapshot helper; mark main-file absence incomplete and require completeness in both report callers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified at published head 788a7993ecd4f9c62b3c05d30df8d0dc362a3493: the shared snapshot marks a missing selected main file as incomplete even when the opened inode remains readable, and both report callers require completeness for a stable result. The unlink-after-open and selected-disappearance route tests pass.
| raw_count = count_source_raw_sessions(root) | ||
| if raw_count == 0: | ||
| return _empty_source_receipt(root, consumed_evidence) |
There was a problem hiding this comment.
When an empty-source request includes operation_id and its schema-inference receipt is missing, expired, or invalid, the earlier validation stores a RebuildProvenanceError in initial_provenance_error, but this return reports successful empty-source completion before that error is raised or the resumable transaction is marked stale. This bypasses the resume provenance gate and can leave an existing transaction indefinitely resumable; process initial_provenance_error before the empty-source return.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified at published head 788a7993ecd4f9c62b3c05d30df8d0dc362a3493: a resumable request records the initial provenance failure, acquires ownership, marks the transaction stale, and raises before the empty-source receipt shortcut. Existing provenance and ownership tests cover that ordering.
| read_only=read_only, | ||
| read_timeout=read_timeout, | ||
| frozen_index_path=index_path, |
There was a problem hiding this comment.
When open_existing(..., index_path=...) creates the new pinned evidence store, passing the path here does not set _inactive_candidate_durable_read_only, because that flag still covers only owned inactive generations and frozen-source validation. Consequently a public method such as delete_hook_event() calls _ensure_source_conn(), which opens source.db read-write and commits deletion even though this store was required to use read_only=True. Fresh evidence beyond the earlier runtime-index repair finding is this durable-source write path; pinned evidence stores must make source/user connections read-only or reject every mutation method.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified at published head 788a7993ecd4f9c62b3c05d30df8d0dc362a3493: pinned read-only stores use mode-ro source connections and the public mutation guard rejects writes before durable-tier mutation. The read-only archive contract and final quick gate pass.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba4bde8eba
ℹ️ 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".
| maintenance_state_metadata = adoption_lstat(maintenance_state_root, "maintenance state parent") | ||
| if maintenance_state_metadata is not None and ( | ||
| stat.S_ISLNK(maintenance_state_metadata.st_mode) or not stat.S_ISDIR(maintenance_state_metadata.st_mode) | ||
| ): | ||
| adoption_markers.append(maintenance_state_root) |
There was a problem hiding this comment.
When the database tiers and other scanned markers are missing but .maintenance-state/source-continuity-pending/*.json survives, this condition ignores the normal maintenance-state directory and the later census inspects only durable-change-trains, so missing-tier initialization can publish an empty durable database. Those pending intents are persisted before a source mutation specifically so startup can recover it (polylogue/storage/sqlite/durable_change_train.py:548-560), making them definitive evidence of an established archive. Fresh evidence beyond the earlier retained-generation finding is this separate recovery-state root; census it (and other source-continuity state) and fail closed rather than manufacturing empty durable authority alongside it.
AGENTS.md reference: AGENTS.md:L114-L121
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devtools/affordance_usage.py (1)
918-924: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not let a missing selected index abort the whole report.
resolve(strict=True)raisesFileNotFoundErrorif the selected index is unlinked or replaced during the run._try_product_detail_reportis called frombuild_reportinside atry/finallywith noexcept, so the exception propagates and the report fails.Every other precondition in this function degrades with
return Noneand lets the direct read-only SQLite path serve the request from the already-open connection. This PR also treats a disappeared selected index as reportable evidence (observation_completeis false insnapshot_index_file_set), not as a fatal error. Keep the same behavior here.🛡️ Proposed fix to degrade to the fallback path
- selected_index_db = config.db_path.resolve(strict=True) + try: + selected_index_db = config.db_path.resolve(strict=True) + except OSError: + # The selected database is gone or unreadable. The direct read-only + # connection can still serve counts from the open inode, and the + # snapshot identity records the incomplete observation. + return None if selected_index_db != (config.archive_root / "index.db").resolve():🤖 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 `@devtools/affordance_usage.py` around lines 918 - 924, Update the selected-index validation in _try_product_detail_report to handle FileNotFoundError from config.db_path.resolve(strict=True) by returning None, preserving the direct read-only SQLite fallback. Keep the existing archive-root identity check and ensure a disappeared or replaced index does not propagate an exception or abort build_report.
🤖 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/rebuild_index.py`:
- Around line 1413-1414: Update the empty-source branch in
polylogue/maintenance/rebuild_index.py at lines 1413-1414 to detect
request.operation_id, load the corresponding transaction, and retire it as stale
before returning _empty_source_receipt; preserve the current behavior when no
operation is being resumed. In
tests/unit/maintenance/test_rebuild_index_ownership.py at lines 375-390, create
the transaction with IndexGenerationStore.create_transaction before invoking the
rebuild and assert that the transaction status becomes stale instead of
asserting the directory is absent.
In `@polylogue/operations/durable_change_train.py`:
- Around line 184-187: Update the anonymous durable publication sequence around
os.link, fsync, lstat, directory os.open, and directory fsync so every OSError
is translated to MigrationError, while preserving FileExistsError as the
no-replacement result. Add cleanup or recovery for failures occurring after the
target becomes visible, using the existing publication and tier-path symbols.
In `@tests/unit/maintenance/test_rebuild_index_ownership.py`:
- Around line 375-390: Create a transaction for operation_id
"empty-source-resume" using IndexGenerationStore.create_transaction before
invoking rebuild_index_from_source_sync, then assert the resulting transaction
status reflects the empty-source rebuild. Update the test to verify
resumed-request behavior rather than merely confirming the transaction directory
is absent.
---
Outside diff comments:
In `@devtools/affordance_usage.py`:
- Around line 918-924: Update the selected-index validation in
_try_product_detail_report to handle FileNotFoundError from
config.db_path.resolve(strict=True) by returning None, preserving the direct
read-only SQLite fallback. Keep the existing archive-root identity check and
ensure a disappeared or replaced index does not propagate an exception or abort
build_report.
🪄 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: f4c66c0a-bb26-49c1-837b-682359a20054
📒 Files selected for processing (15)
devtools/affordance_usage.pydevtools/index_snapshot.pydevtools/lineage_validation.pydocs/plans/degrade-loudly-allowlist.yamlpolylogue/maintenance/rebuild_index.pypolylogue/operations/durable_change_train.pypolylogue/storage/sqlite/archive_tiers/archive.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/devtools/test_affordance_usage.pytests/unit/devtools/test_lineage_validation.pytests/unit/maintenance/test_rebuild_index_ownership.pytests/unit/maintenance/test_rebuild_index_provenance_gate.pytests/unit/storage/test_archive_tiers_archive.pytests/unit/storage/test_prefix_dependent_delete_indexes.pytests/unit/storage/test_schema_policy_contracts.py
💤 Files with no reviewable changes (1)
- docs/plans/degrade-loudly-allowlist.yaml
|
@codex review |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4b3fd8c1c
ℹ️ 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".
| try: | ||
| metadata_before = path.stat() |
There was a problem hiding this comment.
When the selected database is atomically replaced after both SQLite connections open but before the first file observation, these metadata reads see and hash only the replacement while both readers and PRAGMA data_version remain attached to the old inode. Both observations can therefore be complete and identical, producing stable: true even though the reported counts came from a different database than the recorded digest. Fresh evidence beyond the earlier missing-main-file finding is that replacement keeps the path present and bypasses that completeness check; capture and verify the main-file identity against the inode opened for the census.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bca5ed4. Both report routes keep the selected main index inode open before SQLite readers start, hash that opened handle, and reject a pathname inode mismatch. Lineage and affordance replacement-after-reader-open mutations now exercise the production routes.
| # Link the still-open anonymous inode directly. A named fallback | ||
| # would let another same-UID process replace or modify the staged | ||
| # bytes before publication, so unsupported filesystems fail closed. | ||
| os.link(f"/proc/self/fd/{descriptor}", path, follow_symlinks=True) |
There was a problem hiding this comment.
When another same-UID process renames the checked archive directory and installs a different private directory at the same pathname between the adoption census and this link, the destination path is resolved afresh and the anonymous inode can be published into the unchecked replacement. The subsequent lstat, cleanup, and directory fsync also resolve that replacement, so the inode identity check succeeds despite bypassing all adoption-marker checks. Hold and verify a directory descriptor and perform destination operations relative to it so source.db or irreplaceable user.db cannot be initialized in a swapped root.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8e89b3f. Durable admission opens and validates one archive-directory descriptor, then performs census, target checks, anonymous link, identity validation, cleanup, and fsync relative to that descriptor. The CLI test swaps the archive directory before anchoring and confirms publication is refused.
| try: | ||
| path.unlink() |
There was a problem hiding this comment.
When any post-link check or fsync fails, this cleanup unlinks the newly published tier but never fsyncs the parent directory afterward. A crash before that unlink reaches stable storage can therefore resurrect the link even though the command reported failure and observed the target as removed, causing the next recovery attempt to treat an unsuccessfully published source.db or user.db as established durable authority. Fsync the anchored parent after unlinking, and surface an explicit recovery failure if that cleanup cannot be made durable.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8e89b3f. Post-link cleanup unlinks the owned target relative to the anchored descriptor and fsyncs that same parent. The publication fault-injection test forces a post-link target-observation failure and asserts the cleanup directory fsync.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Addressed CodeRabbit review 4890050820. The selected-index strict resolution now stays inside the direct-reader fallback, and the affordance unlink-after-open route serves from the opened inode while marking the snapshot incomplete. The exact-head implementation is in bca5ed4, with production-route coverage in tests/unit/devtools/test_affordance_usage.py. |
|
@codex review |
Problem: post-publication cleanup could rename a foreign inode or overwrite a newer target, and evidence readers assumed Linux procfs. What changed: preserve uncertain durable targets without pathname mutation, validate descriptor aliases with a safe path fallback, and include selected lineage evidence identity in every emitted artifact. Verification: focused managed tests pass with 157 passed and 2 skipped; devtools verify --quick exits 0.
6c14cc7 to
81d02e4
Compare
|
@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: 81d02e4e22
ℹ️ 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".
| # Ownership pins an existing directory descriptor. Fresh test and demo | ||
| # archives legitimately arrive as a not-yet-created path, so create the | ||
| # root before resolving and acquiring its identity. This is part of | ||
| # bootstrap, not an authority bypass: the descriptor is still acquired | ||
| # and checked before any tier is initialized. | ||
| root.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
On first production daemon startup with a new POLYLOGUE_ARCHIVE_ROOT, this mkdir is never reached: polylogue/daemon/cli.py acquires durable archive ownership first, and OwnedArchiveLocation.acquire() now requires the root directory to exist. The daemon therefore still raises ArchiveOwnershipError instead of bootstrapping. Fresh evidence at this exact head is that the regression fix only covers direct callers of initialize_active_archive_root() and does not modify the earlier daemon acquisition path.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
| WHERE resolved.src_session_id = l.src_session_id | ||
| AND resolved.resolved_dst_session_id IS NOT NULL | ||
| AND COALESCE(NULLIF(TRIM(resolved.status), ''), 'unresolved') != 'quarantined' |
There was a problem hiding this comment.
When a child has an unresolved prefix-sharing reference plus any resolved non-quarantined edge that the reader cannot compose—such as spawned-fresh or one without a branch point—this predicate excludes the unresolved row and can report not_applicable. _prefix_sharing_edge_sync() only treats a resolved row as usable when inheritance = 'prefix-sharing' and branch_point_message_id IS NOT NULL, so the exclusion should mirror those conditions rather than accepting every resolved edge.
AGENTS.md reference: AGENTS.md:L100-L104
Useful? React with 👍 / 👎.
| retained_evidence_roots = ( | ||
| (".index-generations", "retained index-generation evidence"), | ||
| (".index-rebuild-transactions", "retained index-rebuild transaction evidence"), | ||
| (".maintenance-state/source-continuity-pending", "source-continuity recovery evidence"), | ||
| (".maintenance-state/source-continuity-refreshes", "source-continuity refresh evidence"), | ||
| ) |
There was a problem hiding this comment.
When durable tiers are missing but .maintenance-state/<operation_id>.json or .maintenance-state/failures.jsonl survives, the parent directory is valid and this tuple ignores its direct children, so the archive is classified as unadopted and an empty durable tier may be published. Those files are canonical persisted replay/resume and failure evidence (polylogue/maintenance/registry.py and failure_routing.py); scan direct maintenance-state entries as well as the enumerated subdirectories before initializing source.db or user.db.
AGENTS.md reference: AGENTS.md:L118-L121
Useful? React with 👍 / 👎.
| if not stat.S_ISREG(metadata.st_mode): | ||
| raise ArchiveOwnershipError(f"archive ownership lock is not a regular file: {path}") | ||
| if metadata.st_nlink != 1: | ||
| raise ArchiveOwnershipError(f"archive ownership lock has unexpected link count: {path}") |
There was a problem hiding this comment.
When another same-UID process renames .archive-ownership.lock and installs a replacement after this process opens it, the opened inode can retain st_nlink == 1 under its new name, so both validations pass even though the canonical lock pathname now names a different inode. This process then holds flock on the moved inode while another daemon or maintenance process can lock the replacement, defeating the sole-writer boundary; compare the anchored directory entry's identity with fstat(fd) after acquiring the lock rather than checking link count alone.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devtools/affordance_usage.py (1)
908-946: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe divergent-index
RuntimeErroris swallowed by the surrounding handler.Lines 916-918 raise a
RuntimeErrorwhenArchiveStoreopens a different physical index than the selected one. Theexcept Exception: return Noneat line 945 catches that same error and silently degrades to the direct-SQLite fallback.The counts stay correct, because the fallback reads the descriptor-pinned
conn. However, the condition the check exists to detect never reaches the operator or the report. Let that specific error propagate, or record it in the report notes.🐛 Proposed fix to preserve the alarm
+class _DivergentSelectedIndexError(RuntimeError): + """ArchiveStore opened a physical index other than the selected one.""" + + def _try_product_detail_report(opened_index_db = Path(archive.index_db_path).resolve(strict=True) if opened_index_db != selected_index_db: - raise RuntimeError( + raise _DivergentSelectedIndexError( "ArchiveStore opened a different physical index than the selected affordance evidence database" )- except Exception: + except _DivergentSelectedIndexError: + raise + except Exception: return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devtools/affordance_usage.py` around lines 908 - 946, Update the exception handling around ArchiveStore.open_existing in the merged_rows loading path so the divergent-index RuntimeError is not swallowed by the broad fallback handler. Let that specific error propagate, or preserve it in the report notes, while retaining the direct-SQLite fallback behavior for other exceptions.
🤖 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 `@devtools/affordance_usage.py`:
- Around line 1225-1237: Replace the manual context-manager lifecycle in the
affected flow of affordance_usage.py and the corresponding lineage_validation.py
locations with contextlib.ExitStack, acquiring the index-file context and
connections inside the guarded stack so cleanup is automatic. Remove the
conn/observer None sentinels and the redundant assert conn is not None, while
preserving acquisition order and existing cleanup behavior.
- Around line 41-44: Align the private aliases in affordance_usage with
lineage_validation: bind _snapshot_identity to snapshot_index_file_set and
_snapshot_report_identity to snapshot_identity, replacing the current
_snapshot_observation alias. Update the relevant call site around the affordance
usage snapshot handling to invoke _snapshot_report_identity(...), while
preserving the existing behavior.
In `@devtools/index_snapshot.py`:
- Around line 200-247: Extract the repeated path validation in the snapshot
observation flow into a helper that accepts the path, expected handle metadata,
error class, and replacement-message context. Have it perform the non-following
stat, regular-file validation, and device/inode comparison while preserving the
existing race and unsafe-sidecar error types and messages; update both checks
here and the variant near the later observation block to use the helper.
In `@devtools/lineage_validation.py`:
- Around line 915-932: Update the snapshot validation flow around
_snapshot_report_identity to build snapshot_identity before deriving verdict
reasons. Remove the local recomputation of observations_complete,
file_set_stable, and no_concurrent_commits, then read observation_complete,
file_set_stable, and no_concurrent_commits from snapshot_identity when appending
reasons.
In `@docs/maintenance.md`:
- Around line 76-79: Split the merged paragraph in the migration runbook so the
sentence ending with “never replaces durable data.” is followed by a blank line
before the numbered migration instructions. Add documentation for the
migrate-tier JSON failure payload’s durable_recovery field, including that state
“uncertain” means a durable tier file remains on disk and requires manual
operator intervention.
In `@polylogue/cli/commands/maintenance/_migrate_tier.py`:
- Around line 119-121: Update the plain-output branch of the migration command,
alongside the JSON `durable_recovery` handling, to inspect `exc.cleanup` for a
`DurableCleanupOutcome` whose state is `"uncertain"`. Include a clear
manual-action warning and the remaining durable tier file information in the
`str(exc)` output, while preserving the existing plain output for other
failures.
In `@polylogue/maintenance/rebuild_index.py`:
- Around line 1420-1434: Revalidate archive ownership after each RebuildLease
acquisition and before either transaction mutation helper. In the
initial_provenance_error branch, and separately in the raw_count == 0 resume
branch, call assert_owns_archive_location(owned, ArchiveLocation.resolve(root))
before _mark_rebuild_transaction_stale_after_provenance_failure or
_retire_empty_source_resume_transaction. Add regression coverage for
archive-root replacement in both lifecycle paths.
In `@polylogue/operations/durable_change_train.py`:
- Around line 374-388: Update the finally block around publication_descriptor
and directory_descriptor so both os.close calls are attempted unconditionally,
even when the first close fails. Preserve any in-flight primary exception, and
attach close failures to it rather than raising a replacement MigrationError;
when no primary exception exists, retain the existing MigrationError behavior
and cleanup uncertainty details.
In `@polylogue/storage/archive_identity.py`:
- Around line 121-128: In polylogue/storage/archive_identity.py lines 121-128,
update the pointer-file read handler around the active index logic to catch both
OSError and ValueError, preserving the ArchiveLocationError conversion. Also
update lines 474-479 in _lock_holder_pid to catch both exception types around
the pread/decode operation, preserving its best-effort None result.
- Around line 613-616: Update the owner-metadata write tail in the relevant
archive identity function to wrap os.ftruncate, os.write, and os.fsync in the
same failure handling used elsewhere: on OSError, close fd and raise
ArchiveOwnershipError while preserving the original error context. Keep the
successful path returning fd unchanged so acquire does not receive a raw OSError
or leak the descriptor.
In `@polylogue/storage/sqlite/archive_tiers/bootstrap.py`:
- Around line 321-326: Ensure both archive-root creation sites use a private
directory mode by passing mode=0o700 to root.mkdir in the bootstrap flow at
polylogue/storage/sqlite/archive_tiers/bootstrap.py:321-326 and
archive_root_path.mkdir in polylogue/daemon/cli.py:2121-2126; preserve the
existing parents=True and exist_ok=True behavior.
In `@tests/unit/cli/test_archive_maintenance_cli.py`:
- Around line 2438-2464: Remove the fail_directory_open helper and its module
os.open monkeypatch from the test setup, since this CLI path always supplies
directory_fd and exercises only the os.dup failure. Keep fail_directory_dup and
the existing directory_open parametrization behavior intact.
- Around line 2406-2422: Update fail_fsync in the archive maintenance test to
inject the image_fsync failure based on the specific image descriptor identity,
not fsync_calls ordinal; capture or identify the descriptor used by the
publication route’s image fsync. Remove the unused directory_fsyncs counter, or
add an assertion that validates it if it remains necessary.
- Line 3015: Update the return annotation of fail_blob_inspection to list[str],
matching the list returned by real_listdir, and remove the now-unused Iterator
import.
In `@tests/unit/devtools/test_affordance_usage.py`:
- Around line 755-763: Replace the hardcoded capture_calls == 4 trigger in
tests/unit/devtools/test_affordance_usage.py lines 755-763 by wrapping
_snapshot_observation and creating the sidecars immediately before its second
invocation, so setup no longer depends on production capture_sidecars call
counts. Apply the same change in tests/unit/devtools/test_lineage_validation.py
lines 652-658; both tests should create sidecars before the after observation.
- Around line 220-222: Update the assertion in the report validation around
selected_db and configured_root to compare report["index_db"] against the
configured archive’s resolved index file path, not the configured directory
path. Keep the existing selected database equality and evidence_root assertions
unchanged.
In `@tests/unit/maintenance/test_rebuild_index_ownership.py`:
- Around line 45-71: Update the assertions in _init_nonempty_source so the
census verifies the fixture produced classified replayable evidence: retain
census.scanned == 1 and also assert census.classified_full == 1.
In `@tests/unit/storage/test_connection_profile.py`:
- Around line 28-41: Add a unit test alongside
test_open_readonly_connection_refuses_without_descriptor_bound_path that creates
a database, opens a descriptor, and verifies open_readonly_connection raises
ValueError matching “immutable mode” when immutable=True and opened_main_fd are
both provided; close the descriptor in a finally block.
In `@tests/unit/storage/test_schema_policy_contracts.py`:
- Around line 245-249: Strengthen the test around ArchiveStore.open_existing by
inserting a row present only in old_index before opening the archive, then query
that row through archive._conn while the context is active. Assert the expected
old-index data is returned, while retaining the existing read-only and index
assertions, so the test verifies index_path selects the pinned physical index.
---
Outside diff comments:
In `@devtools/affordance_usage.py`:
- Around line 908-946: Update the exception handling around
ArchiveStore.open_existing in the merged_rows loading path so the
divergent-index RuntimeError is not swallowed by the broad fallback handler. Let
that specific error propagate, or preserve it in the report notes, while
retaining the direct-SQLite fallback behavior for other exceptions.
🪄 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: bb728287-68cd-4607-9ec3-f45d2373d5d4
📒 Files selected for processing (21)
devtools/affordance_usage.pydevtools/index_snapshot.pydevtools/lineage_validation.pydocs/maintenance.mdpolylogue/cli/commands/maintenance/_migrate_tier.pypolylogue/daemon/cli.pypolylogue/maintenance/rebuild_index.pypolylogue/operations/durable_change_train.pypolylogue/storage/archive_identity.pypolylogue/storage/sqlite/archive_tiers/archive.pypolylogue/storage/sqlite/archive_tiers/bootstrap.pypolylogue/storage/sqlite/connection_profile.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/daemon/test_daemon_cli.pytests/unit/devtools/test_affordance_usage.pytests/unit/devtools/test_lineage_validation.pytests/unit/maintenance/test_rebuild_index_ownership.pytests/unit/storage/test_archive_identity.pytests/unit/storage/test_archive_tiers_archive.pytests/unit/storage/test_connection_profile.pytests/unit/storage/test_schema_policy_contracts.py
| opened_index_files = open_index_file_set(index_db) | ||
| opened_file_set = opened_index_files.__enter__() | ||
| opened_main_fd = opened_file_set.main_fd | ||
| conn: Connection | None = None | ||
| observer: Connection | None = None | ||
| try: | ||
| conn = open_readonly_connection(index_db, opened_main_fd=opened_main_fd) | ||
| opened_file_set.capture_sidecars(index_db) | ||
| observer = open_readonly_connection(index_db, opened_main_fd=opened_main_fd) | ||
| assert conn is not None | ||
| observer_data_version_before = _data_version(observer) | ||
| opened_file_set.capture_sidecars(index_db) | ||
| conn.execute("BEGIN") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use contextlib.ExitStack instead of manual __enter__/__exit__.
Lines 1226 and 1353 drive the context manager by hand. ExitStack expresses the same lifetime, keeps the acquisition inside the guarded region, and removes the need for the conn/observer None sentinels.
Line 1234 assert conn is not None follows the assignment at line 1231 directly, so it can never fail. Remove it.
The same manual pattern exists in devtools/lineage_validation.py lines 796-798 and 966.
🤖 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 `@devtools/affordance_usage.py` around lines 1225 - 1237, Replace the manual
context-manager lifecycle in the affected flow of affordance_usage.py and the
corresponding lineage_validation.py locations with contextlib.ExitStack,
acquiring the index-file context and connections inside the guarded stack so
cleanup is automatic. Remove the conn/observer None sentinels and the redundant
assert conn is not None, while preserving acquisition order and existing cleanup
behavior.
| continue | ||
| opened_fd = opened_main_fd if path == index_db else (opened_sidecar_fds or {}).get(suffix) | ||
| if opened_fd is not None: | ||
| handle_metadata = os.fstat(opened_fd) | ||
| try: | ||
| path_metadata_before = path.stat(follow_symlinks=False) | ||
| except FileNotFoundError: | ||
| complete = False | ||
| else: | ||
| if not stat.S_ISREG(path_metadata_before.st_mode): | ||
| error = IndexSnapshotRaceError if path == index_db else IndexSnapshotUnsafeSidecarError | ||
| raise error(f"selected index file is not regular: {path}") | ||
| if (path_metadata_before.st_dev, path_metadata_before.st_ino) != ( | ||
| handle_metadata.st_dev, | ||
| handle_metadata.st_ino, | ||
| ): | ||
| label = "selected index path" if path == index_db else "selected index sidecar" | ||
| error = IndexSnapshotRaceError if path == index_db else IndexSnapshotUnsafeSidecarError | ||
| raise error(f"{label} was replaced while its reader was open: {path}") | ||
| digest = _file_sha256_descriptor(opened_fd) | ||
| try: | ||
| path_metadata_after = path.stat(follow_symlinks=False) | ||
| except FileNotFoundError: | ||
| complete = False | ||
| path_present = False | ||
| else: | ||
| if not stat.S_ISREG(path_metadata_after.st_mode): | ||
| error = IndexSnapshotRaceError if path == index_db else IndexSnapshotUnsafeSidecarError | ||
| raise error(f"selected index file is not regular: {path}") | ||
| if (path_metadata_after.st_dev, path_metadata_after.st_ino) != ( | ||
| handle_metadata.st_dev, | ||
| handle_metadata.st_ino, | ||
| ): | ||
| label = "selected index path" if path == index_db else "selected index sidecar" | ||
| error = IndexSnapshotRaceError if path == index_db else IndexSnapshotUnsafeSidecarError | ||
| raise error(f"{label} was replaced during snapshot observation: {path}") | ||
| path_present = True | ||
| files.append( | ||
| { | ||
| "path": str(path), | ||
| "present": path_present, | ||
| "size": handle_metadata.st_size, | ||
| "mtime_ns": handle_metadata.st_mtime_ns, | ||
| "inode": handle_metadata.st_ino, | ||
| "sha256": digest, | ||
| "changed_during_observation": False, | ||
| } | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract the repeated inode-identity comparison.
Lines 205-218 and 221-235 repeat the same three steps: stat(follow_symlinks=False), regular-file check, and (st_dev, st_ino) comparison against handle_metadata. Lines 258-264 repeat a third variant. A small helper that takes the path, the expected metadata, and the error class would remove the duplication and keep the two error messages as the only difference.
🤖 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 `@devtools/index_snapshot.py` around lines 200 - 247, Extract the repeated path
validation in the snapshot observation flow into a helper that accepts the path,
expected handle metadata, error class, and replacement-message context. Have it
perform the non-following stat, regular-file validation, and device/inode
comparison while preserving the existing race and unsafe-sidecar error types and
messages; update both checks here and the variant near the later observation
block to use the helper.
Track the current source schema and preserve explicit schema-inference preflight seams in daemon bulk-route tests. Ref #3893
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 069b36953c
ℹ️ 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".
| # Ownership pins an existing directory descriptor. Fresh test and demo | ||
| # archives legitimately arrive as a not-yet-created path, so create the | ||
| # root before resolving and acquiring its identity. This is part of | ||
| # bootstrap, not an authority bypass: the descriptor is still acquired | ||
| # and checked before any tier is initialized. | ||
| root.mkdir(mode=0o700, parents=True, exist_ok=True) |
There was a problem hiding this comment.
When another same-UID process renames the archive root and installs a replacement after OwnedArchiveLocation.acquire() succeeds, the bootstrap census and every marker/database operation below still resolve root by pathname, so they write into the replacement while the held directory descriptor and ownership lock cover the old directory. This allows direct bootstrap or daemon first-run initialization to create durable tiers without sole-writer ownership; retain the ownership token and perform these operations relative to its descriptor, or otherwise fail closed if the pathname no longer names that directory.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
1 participant
Summary
Rebase PR #3893 onto current
origin/masterat9223190211a0200387c185d5d39702df3482a914and close the still-valid canonical snapshot and durable publication authority gaps. The branch preserves the existing pinned-reader, consumed-evidence, provenance-ordering, and adoption repairs while adding one shared snapshot identity contract and fail-closed durable initialization on filesystems withoutO_TMPFILE.Problem
The two devtools reports carried duplicate snapshot identity logic, which allowed their stability fields to drift. Durable missing-tier initialization still exposed a named staging fallback that could be modified before publication, and an
os.linkfailure surfaced as a generic publication error instead of the anonymous-publication authority failure. Active-pointer marker inspection also followed pathname existence and missed dangling marker evidence.Solution
devtools/index_snapshot.py; both affordance and lineage reports consume the same implementation while retaining their test seams.O_TMPFILEpublication, wrap anonymous open and link failures with the tier/path context, and keep no-replace cleanup receipts for post-publication failures.lstat, including dangling symlinks, so retained marker evidence reaches the established-archive refusal route.Verification
direnv exec . devtools test tests/unit/devtools/test_affordance_usage.py tests/unit/devtools/test_lineage_validation.py tests/unit/storage/test_archive_identity.py tests/unit/cli/test_archive_maintenance_cli.py:174 passed, 2 skipped.direnv exec . devtools test tests/unit/devtools/test_affordance_usage.py tests/unit/devtools/test_lineage_validation.py:43 passed, 2 skipped.direnv exec . devtools verify --quick: exit 0; all 24 steps passed, including ruff, mypy, generated-surface rendering, layering, policy, and schema promotion audit.git diff HEAD^..HEAD --check: clean.git -c core.hooksPath=/dev/null rebase origin/master; no merge commit was introduced.Acceptance and review matrix
dfcd3c052;devtools/index_snapshot.py; the two devtools test filesdevtools/lineage_validation.py; alternate-edge and zero-sample tests_empty_source_receiptand resume tests intests/unit/maintenance/test_rebuild_index_ownership.py_mark_rebuild_transaction_stale_after_provenance_failureordering andtests/unit/maintenance/test_rebuild_index_provenance_gate.pysnapshot_index_file_setincomplete-observation path and unlink mutation tests in both devtools suitesdfcd3c052;tests/unit/cli/test_archive_maintenance_cli.py; docs updatedfcd3c052;test_migrate_tier_cli_missing_initialization_refuses_dangling_active_pointer; retainedindex.dbsibling census testArchiveStore._initialize_storecallsensure_runtime_indexes_synconly in the writable branch;3cecbc5acand archive mutation testspolylogue-reindex-candidate-acceptanceremains the named successor for live inactive-candidate acceptanceScope carrier
Summary by CodeRabbit
New Features
Bug Fixes