fix(storage): close acquired blob references - #3816
Merged
Merged
Conversation
Problem: the async raw acquisition writer persisted raw_sessions without its canonical raw_payload ref, and historical acquired attachments can remain detached from attachment_refs. Reindex acceptance had no cross-tier closure check.\n\nWhat changed: write the canonical raw ref on the async acquisition route, add a read-only exact closure invariant, and add a dry-run-default reconciliation command. Raw repairs use persisted source fields. Attachment repairs require a complete authoritative raw reparse and an existing owning message. Apply is offline, backup-gated, additive, and receipt-backed; typed blockers remain untouched. Reindex candidates run the closure check before promotion.\n\nCompatibility/migration: no schema changes. The live archive was inspected read-only only.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds blob-reference closure auditing and guarded repair across source and index databases. It adds canonical reference writes, normalized identity mapping, archive verification, a maintenance CLI command, receipts, backup validation, and production-route tests. ChangesBlob reference closure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant MaintenanceCLI
participant BlobReferenceClosure
participant SourceDB
participant IndexDB
participant ArchiveVerification
Operator->>MaintenanceCLI: Run blob-reference-closure
MaintenanceCLI->>BlobReferenceClosure: Request dry-run or guarded apply
BlobReferenceClosure->>SourceDB: Inspect raw sessions and source metadata
BlobReferenceClosure->>IndexDB: Inspect attachments and references
BlobReferenceClosure-->>MaintenanceCLI: Return plan, blockers, and counts
BlobReferenceClosure->>SourceDB: Apply canonical raw references
BlobReferenceClosure->>IndexDB: Apply attachment references
MaintenanceCLI->>ArchiveVerification: Validate closure
ArchiveVerification->>SourceDB: Check exact raw references
ArchiveVerification->>IndexDB: Check attachment references
ArchiveVerification-->>MaintenanceCLI: Return verification result
Possibly related PRs
🚥 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 |
Problem: closure relinking used a native-id-only message map, so id-less attachments were not repairable and duplicate native ids could select the wrong message.\n\nWhat changed: share the production attachment owner maps with relinking, including position fallback and duplicate-id exclusion. Add closure plan/apply regressions for id-less positional ownership and duplicate native-id disambiguation.\n\nCompatibility/migration: dry-run default, offline guard, backup validation, receipts, and typed blockers are unchanged.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: closure relinking mapped relative attachment positions from append payloads against the session origin, and duplicate native ids were compared before storage normalization.\n\nWhat changed: share the production MAX(position)+1 helper with closure relinking and normalize stripped native ids before duplicate exclusion. Add plan/apply regressions that assert append attachments avoid older messages and whitespace duplicate ids fall back by position.\n\nCompatibility/migration: dry-run defaults, offline and backup gates, receipts, and additive reference repair are unchanged.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: duplicate native ids were normalized for storage but several write-side owner maps still compared raw provider strings, allowing whitespace variants to reuse an ambiguous native-id mapping. The append regression also used a native id, so it did not exercise position-derived identity. What changed: route every duplicate exclusion in write.py through _normalized_message_native_id. Replace the append regression with two id-less messages whose appended attachment must land at MAX(position)+1, and make the whitespace-variant closure case fail closed through both plan and apply. Compatibility/migration: no schema or archive data changes. Closure remains dry-run by default and apply remains offline, backup-gated, additive, and receipt-backed. Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/maintenance/archive_verification.py`:
- Around line 1009-1013: Update the connection-opening logic around _open_ro so
that if opening index_path fails after source_conn succeeds, source_conn is
closed before returning the _error_check result. Preserve normal cleanup for
successful opens and ensure the partial-failure path does not retain the source
connection.
In `@polylogue/maintenance/blob_reference_closure.py`:
- Around line 185-193: Update the unrecoverable attachment sample model and
plan_orphaned_attachment_relink flow in attachment_relink.py to expose a typed
reason field distinguishing missing owning messages from missing authoritative
raw data. Change _attachment_blockers to select BlobReferenceBlockerKind from
that enum instead of searching item.reason prose, while preserving the existing
reason text in the resulting blocker.
- Around line 489-515: Define a shared helper or SQL fragment in
blob_reference_closure.py for the exact-one exact raw_payload reference
predicate, then reuse it in closure_counts and _raw_candidates_and_blockers
instead of duplicating the condition. Export that shared predicate so
polylogue/maintenance/archive_verification.py can use the same definition,
preserving identical broken-row semantics across planning and verification.
- Around line 213-226: Make the attachment sample cap explicit by defining a
shared named limit used by plan_orphaned_attachment_relink, _plan_connections,
and _attachment_blockers instead of the unnamed 1,000,000 value. Add and
propagate a partial-plan indicator on BlobReferenceClosurePlan and the closure
receipt whenever unrecoverable_attachment samples are capped, so consumers can
distinguish complete blocker data from a sample-limited plan.
- Around line 294-323: Update _write_receipt and _append_receipt to fsync the
receipt’s parent directory after the file contents are flushed and synced. Open
the directory represented by path.parent, fsync that descriptor, and close it
reliably so each newly created or appended receipt record is durable with its
directory entry.
🪄 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: 817d1dc9-dc89-4e5b-ad6f-f7cf7f12646f
📒 Files selected for processing (13)
docs/maintenance.mdpolylogue/cli/commands/maintenance/__init__.pypolylogue/cli/commands/maintenance/_blob_reference_closure.pypolylogue/maintenance/archive_verification.pypolylogue/maintenance/blob_reference_closure.pypolylogue/storage/attachment_relink.pypolylogue/storage/sqlite/archive_tiers/write.pypolylogue/storage/sqlite/queries/raw_writes.pytests/unit/cli/test_maintenance_registration.pytests/unit/maintenance/test_archive_verification.pytests/unit/maintenance/test_blob_reference_closure.pytests/unit/storage/test_attachment_acquisition.pytests/unit/storage/test_raw.py
Problem: append closure planning applied a fresh positional offset to messages already materialized in the index, so id-less append attachments could point past the real tail. Batch append dedupe also compared raw incoming native ids with normalized stored ids, allowing INSERT OR REPLACE to overwrite an existing message. What changed: match append closure payloads against materialized native ids or exact writer content hashes before using the provisional append offset. Normalize native ids in both append exclusion and changed-existing-message checks. Add production-route regressions for id-less append relinking and whitespace-padded duplicate preservation. Compatibility/migration: full-replace closure mapping and non-append writes are unchanged. No schema or migration changes. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: closure relinking restored an acquired attachment reference without restoring its typed provider, file, or drive identities. Full replacement also used a four-byte positional hash with INSERT OR REPLACE, allowing two distinct attachments whose ids collided in that truncated hash to overwrite one ref. What changed: carry the complete typed attachment identity through relink planning and closure apply, and use the same collision-safe reference position allocator in write and closure routes. Preserve legacy positions for unique attachments, resolve collision groups by canonical attachment identity, and reject a distinct pre-existing ref instead of replacing it. Compatibility/migration: no schema changes. Existing unique attachment positions and archive write ordering remain unchanged; collision handling is deterministic and idempotent across input ordering. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: Native message IDs and positional coordinates shared an untagged generated namespace, while legacy attachment position collisions could leave source and index tiers only partly repaired. Repair receipts also duplicated closure predicates and did not durably publish their directory entry.\n\nWhat changed: Tag native and positional identities, declare the derived schema semantic-reparse boundary, reject silent message replacement, and make writer and relink attachment positions collision-safe. Plan typed bounded blockers before mutation, sanitize repaired native IDs for SQLite, share the raw closure predicate, fsync receipt directories, and apply source plus index repairs through one attached SQLite transaction.\n\nVerification: Ref #3816. Focused managed tests passed 139 selected tests; one unchanged validation node failed with its pre-existing empty-result assertion and reproduced separately. devtools verify --quick passed with exit code 0.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
polylogue/storage/attachment_relink.py (1)
456-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead handler and the always-empty
errorslist.Lines 495-496 catch
sqlite3.Errorand re-raise it unchanged, which is equivalent to no handler.errorsis declared at Line 456, never appended to, and still returned at Line 504. Delete thetry/exceptand either droperrorsor state in the docstring that the field is retained only for the result shape.♻️ Proposed fix
- relinked = 0 - errors: list[str] = [] + relinked = 0 if not dry_run: for item in plan.eligible: - try: - index_conn.execute( + index_conn.execute(Then remove the matching
except sqlite3.Error: raiseand dedent the body.Also applies to: 495-496
🤖 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/storage/attachment_relink.py` at line 456, In the attachment relinking function, remove the unused errors list and its empty return field, then delete the no-op sqlite3.Error handler and dedent the wrapped body. If the result shape must retain errors, document that it is intentionally always empty instead of maintaining dead error-handling code.polylogue/maintenance/blob_reference_closure.py (1)
505-520: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA receipt-write failure after the commit records the phase as
aborted.Line 505 commits both tiers. Lines 506-513 then append the
source_committed,index_committed, andcommittedphases. If any of those appends raises, control reaches Line 514.source_conn.in_transactionis already false, so the rollback is a no-op, and Line 519 appendsaborted. The receipt then states that the repair was aborted while both tiers hold the committed references. A later continuation reads that receipt and reaches the wrong conclusion.Record the commit fact before the phase appends, and label a post-commit failure distinctly.
🐛 Proposed fix
source_conn.commit() + committed = True _append_receipt(receipt_path, "source_committed", repaired_count=source_repaired) @@ except Exception as exc: if source_conn.in_transaction: source_conn.rollback() if prepared: with suppress(OSError): - _append_receipt(receipt_path, "aborted", error=str(exc)) + _append_receipt( + receipt_path, + "committed_receipt_incomplete" if committed else "aborted", + error=str(exc), + ) raiseInitialize
committed = Falsenext toprepared = Falseat Line 405.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@polylogue/maintenance/blob_reference_closure.py` around lines 505 - 520, In the repair flow surrounding the commit and exception handler, initialize a committed flag alongside prepared, set it immediately after source_conn.commit() succeeds and before any _append_receipt calls, and distinguish post-commit exceptions from pre-commit aborts. Preserve rollback and the existing aborted receipt only when committed is false; when committed is true, record a distinct post-commit failure phase so continuation does not interpret the repair as aborted.
🤖 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/core/identity_law.py`:
- Around line 51-53: Normalize native_id by stripping surrounding whitespace
before constructing the native identity in message_local_id() and the
corresponding message_id()/writer paths. Ensure archive_tiers_specs.py’s
regenerated message_id uses the same normalized value, so Python and generated
SQL produce identical IDs for whitespace-padded and trimmed native IDs.
In `@polylogue/maintenance/blob_reference_closure.py`:
- Around line 136-150: Update raw_reference_closure_predicate to wrap the entire
returned OR expression in one outer pair of parentheses, preserving both
subquery comparisons and their existing logic so callers can safely combine the
exported predicate with additional WHERE conditions.
In `@polylogue/material_protocol/v1/records.py`:
- Around line 111-115: Update the parent_message_id construction in records.py
so the parent link is only created when message.parent_native_id contains a
non-blank value. The current check in the parent_message_id expression only
excludes None, which lets empty or whitespace-only values flow into
message_local_id(..., position=0) and resolve to the position-0 message; change
the guard to treat blank input as missing and return None instead. Keep the
existing session_id/message_local_id formatting path unchanged for valid
parent_native_id values.
In `@polylogue/storage/attachment_relink.py`:
- Around line 163-192: Optimize content-hash matching in the message-resolution
flow by indexing stored rows once by their bytes(row[4]) content hash, then
resolving each id-less payload message through that index instead of hashing it
against every row. Compute _message_content_hash only for distinct
(message_index, position, variant_index) combinations present in candidate rows,
and cache results by that key so repeated coordinates are hashed once. Preserve
the existing unique-match behavior and native-ID path.
In `@polylogue/storage/sqlite/archive_tiers/write.py`:
- Around line 3390-3424: Remove the initial session-wide
_attachment_reference_positions(attachments) call and initialize
attachment_positions empty before processing attachments_by_message. Keep the
per-message recomputation inside the message_id loop, including
occupied-position handling, since only resolved attachments are written; the
message_group loop is non-empty, so simplify the unreachable current_ids
fallback if appropriate.
In `@tests/unit/maintenance/test_blob_reference_closure.py`:
- Around line 232-274: In the collision fixture setup around
_attachment_position, explicitly assert that the two attachment IDs produce the
same truncated position before querying attachment_refs. Keep the existing
closure assertions unchanged, and place the assertion before base_position is
used so the test fails if the fixture no longer exercises the intended collision
path.
---
Outside diff comments:
In `@polylogue/maintenance/blob_reference_closure.py`:
- Around line 505-520: In the repair flow surrounding the commit and exception
handler, initialize a committed flag alongside prepared, set it immediately
after source_conn.commit() succeeds and before any _append_receipt calls, and
distinguish post-commit exceptions from pre-commit aborts. Preserve rollback and
the existing aborted receipt only when committed is false; when committed is
true, record a distinct post-commit failure phase so continuation does not
interpret the repair as aborted.
In `@polylogue/storage/attachment_relink.py`:
- Line 456: In the attachment relinking function, remove the unused errors list
and its empty return field, then delete the no-op sqlite3.Error handler and
dedent the wrapped body. If the result shape must retain errors, document that
it is intentionally always empty instead of maintaining dead error-handling
code.
🪄 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: fc4e425a-7f02-4c38-b3b9-5e4b05ac0274
📒 Files selected for processing (24)
docs/data-model.mddocs/examples/demo-tour/command-output/01-claim-versus-receipt.txtdocs/examples/demo-tour/command-output/03-composed-lineage.txtdocs/examples/demo-tour/report.jsondocs/examples/demo-tour/report.mddocs/examples/demo-tour/transcript.txtpolylogue/core/identity_law.pypolylogue/maintenance/archive_verification.pypolylogue/maintenance/blob_reference_closure.pypolylogue/material_protocol/v1/records.pypolylogue/pipeline/services/ingest_batch/_core.pypolylogue/storage/attachment_relink.pypolylogue/storage/sqlite/archive_tiers/archive_tiers_specs.pypolylogue/storage/sqlite/archive_tiers/index.pypolylogue/storage/sqlite/archive_tiers/write.pypolylogue/storage/sqlite/lifecycle.pytests/unit/core/test_identity_law.pytests/unit/maintenance/test_blob_reference_closure.pytests/unit/material_protocol/v1/fixture.pytests/unit/material_protocol/v1/test_round_trip.pytests/unit/pipeline/test_archive_write.pytests/unit/pipeline/test_ingest_batch.pytests/unit/storage/test_archive_tiers_write.pytests/unit/storage/test_attachment_relink.py
Comment on lines
51
to
+53
| if native_id is not None and native_id.strip(): | ||
| return _required_text("message native_id", native_id) | ||
| return f"{_required_non_negative('position', position)}.{_required_non_negative('variant_index', variant_index)}" | ||
| return f"n:{_required_text('message native_id', native_id)}" | ||
| return f"p:{_required_non_negative('position', position)}.{_required_non_negative('variant_index', variant_index)}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the stored-native-id normalizer and every caller of message_local_id.
ast-grep run --pattern 'def _stored_message_native_id($$$) { $$$ }' --lang python polylogue/storage/sqlite/archive_tiers/write.py 2>/dev/null
rg -nP -C6 'def _stored_message_native_id' polylogue/storage/sqlite/archive_tiers/write.py
rg -nP -C3 '\bmessage_local_id\s*\(' --type=py
rg -nP -C3 '\bmessage_id\s*\(' polylogue/core/identity_law.pyRepository: Sinity/polylogue
Length of output: 838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)polylogue/(core/identity_law\.py|storage/sqlite/archive_tiers/(write\.py|archive_tiers_specs\.py)|core/\w*id|storage/sqlite/archive_tiers/\w*id)'
echo
echo "== identity_law outline and relevant lines =="
ast-grep outline polylogue/core/identity_law.py --view expanded || true
sed -n '1,120p' polylogue/core/identity_law.py
echo
echo "== writer native normalizer relevant lines =="
sed -n '6550,6630p' polylogue/storage/sqlite/archive_tiers/write.py
echo
echo "== message_id generation relevant lines =="
rg -n -C8 'message_id|required.*message_id|n:\||n:' polylogue/storage/sqlite/archive_tiers/archive_tiers_specs.py polylogue/storage/sqlite/archive_tiers/write.py polylogue/core/identity_law.py
echo
echo "== message_local_id call sites =="
rg -n -C4 '\bmessage_local_id\s*\(' --glob '*.py'
rg -n -C4 '\bmessage_id\s*\(' --glob '*.py'Repository: Sinity/polylogue
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast, pathlib, re
for path in [
pathlib.Path("polylogue/core/identity_law.py"),
pathlib.Path("polylogue/storage/sqlite/archive_tiers/write.py"),
]:
print(f"\n== {path} syntax check ==")
ast.parse(path.read_text())
print("OK")
files = [pathlib.Path(p) for p in pathlib.Path("polylogue").rglob("*.py")]
for sym in ["message_local_id", "message_id"]:
print(f"\n== {sym} call sites in polylogue Python files ==")
for p in files:
data = p.read_text()
if re.search(rf'\b{sym}\s*\(', data):
print(p)
PY
echo
echo "== focused identity specs and writer normalizer =="
sed -n '40,70p' polylogue/core/identity_law.py
rg -n -C4 'def _message_id|def message_id|message_id = ' polylogue/storage/sqlite/archive_tiers/write.py polylogue/core/identity_law.py
sed -n '6574,6614p' polylogue/storage/sqlite/archive_tiers/write.py
echo
echo "== focused generated column =="
rg -n -C5 "message_id|n:.*native_id|native_id.*n:" polylogue/storage/sqlite/archive_tiers/archive_tiers_specs.pyRepository: Sinity/polylogue
Length of output: 44447
Normalize native IDs before building message identity strings.
message_local_id() accepts native_id.strip() but embeds the unstripped native_id; this helper only has call sites in message_id(), so normalization can live in message_id()/writers, but archive_tiers_specs.py regenerates message_id from the stored native_id without stripping. Match the Python and generated SQL forms so "n:" + " msg-1 " and "n:" + "msg-1" cannot diverge.
🤖 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/core/identity_law.py` around lines 51 - 53, Normalize native_id by
stripping surrounding whitespace before constructing the native identity in
message_local_id() and the corresponding message_id()/writer paths. Ensure
archive_tiers_specs.py’s regenerated message_id uses the same normalized value,
so Python and generated SQL produce identical IDs for whitespace-padded and
trimmed native IDs.
Problem: blob-reference closure was based before the strict acceptance and raw-failure lifecycle gates landed, and its derived identity change shared index version 65 with an existing view-only change.\n\nWhat changed: preserve both archive verification checks, include both checks in candidate acceptance and red-twin coverage, and sequence the identity replay as derived index version 66 after the existing version 65 view change.\n\nCompatibility/migration: the identity change remains semantic-reparse-only and requires the established raw replay path. The focused closure suite passed 199 tests; two existing Antigravity pathology tests remain blocked by the generic synthetic-wire limitation.\n\nRef polylogue-3816\n\nCo-Authored-By: Codex <noreply@openai.com>
Problem: the merged blob-closure branch carried stale demo-tour evidence and could not pass the repository freshness gate.\n\nWhat changed: regenerate and publish the committed demo command output, report, and transcript from the current production routes.\n\nCompatibility/migration: documentation and generated evidence only; no archive data is retained in the repository worktree.\n\nRef polylogue-3816\n\nCo-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polylogue/storage/sqlite/archive_tiers/index.py (1)
1036-1043: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClassify an explicit error flag as an error.
When
ap.is_error = 1andap.exit_code = 0, this CASE returnsoutcome_success. The explicit error flag must not be discarded. Classify the action asoutcome_errorwhen either field reports an error.Proposed fix
CASE WHEN ap.tool_result_block_id IS NULL THEN 'no_result' + WHEN ap.is_error = 1 + OR (ap.exit_code IS NOT NULL AND ap.exit_code != 0) THEN 'outcome_error' WHEN ap.is_error IS NULL AND ap.exit_code IS NULL THEN 'outcome_unknown' - WHEN ap.exit_code IS NOT NULL AND ap.exit_code != 0 THEN 'outcome_error' - WHEN ap.exit_code IS NULL AND ap.is_error = 1 THEN 'outcome_error' ELSE 'outcome_success' END AS result_state🤖 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/storage/sqlite/archive_tiers/index.py` around lines 1036 - 1043, Update the result_state CASE expression in the archive action query so ap.is_error = 1 is classified as outcome_error regardless of ap.exit_code, including when the exit code is 0. Preserve the no_result and outcome_unknown cases, and retain outcome_success only when neither ap.is_error nor ap.exit_code reports an error.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@polylogue/storage/sqlite/archive_tiers/index.py`:
- Around line 1036-1043: Update the result_state CASE expression in the archive
action query so ap.is_error = 1 is classified as outcome_error regardless of
ap.exit_code, including when the exit code is 0. Preserve the no_result and
outcome_unknown cases, and retain outcome_success only when neither ap.is_error
nor ap.exit_code reports an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fdf36abb-a6dc-41a5-8eaf-f8b3f8a27f09
📒 Files selected for processing (10)
docs/examples/demo-tour/command-output/01-claim-versus-receipt.txtdocs/examples/demo-tour/report.jsondocs/examples/demo-tour/report.mddocs/examples/demo-tour/transcript.txtdocs/maintenance.mdpolylogue/cli/commands/maintenance/__init__.pypolylogue/maintenance/archive_verification.pypolylogue/storage/sqlite/archive_tiers/index.pypolylogue/storage/sqlite/lifecycle.pytests/unit/maintenance/test_archive_verification.py
Problem: the current master acceptance gates advanced after the blob-closure branch was published, leaving the branch dirty and its cross-tier check registry incomplete.\n\nWhat changed: retain the blob-reference closure check and red twin alongside the newer raw-failure, blob-integrity, and attachment-debt gates while synchronizing the branch with master.\n\nCompatibility/migration: no new archive mutation. The semantic identity replay remains the existing index v66 boundary.\n\nRef polylogue-3816\n\nCo-Authored-By: Codex <noreply@openai.com>
Problem: the closure repair path could report an aborted receipt after a successful two-tier commit, expose a predicate that was unsafe to compose, scan stored message hashes redundantly, and treat blank parent IDs as real links.\n\nWhat changed: distinguish post-commit receipt failure, wrap and reuse the exact raw-reference predicate safely, cache attachment content-hash lookups, remove dead relink error handling, ignore blank parent IDs, compute attachment positions only for resolved owners, and pin the collision fixture to its intended collision.\n\nVerification: the focused managed suite passed 131 tests. Ruff and mypy passed.\n\nRef polylogue-3816\n\nCo-Authored-By: Codex <noreply@openai.com>
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Align every production write-side duplicate exclusion with the storage-normalized message native ID, and replace the two regressions with behaviorally discriminating plan, apply, and append-write proofs.
Problem
_duplicate_message_native_ids()stores stripped native IDs, but four downstream write paths still compared raw provider strings. A whitespace variant could therefore reuse an ambiguous native-ID mapping. The append regression also used a named native ID, so it could pass without exercising position-derived identity.Solution
polylogue/storage/sqlite/archive_tiers/write.pynow compares_normalized_message_native_id(message)against the normalized duplicate sets in attachment ownership, parent-link, session-event, and lineage-tail bookkeeping. The append regression uses two id-less messages whose source positions are both0; the production append route must store the second message atMAX(position) + 1, position1, and attach the blob tosession_id:1.0. The whitespace regression usesmessage_provider_id=" duplicate "with no positional fallback while the messages containduplicateandduplicate; the closure plan produces no candidate and a typed blocker, and apply leavesattachment_refsempty.The route trace is:
write_parsed_session_to_archive()->_next_message_position()->_write_attachments()for append ownership; closure plan and apply both reachplan_orphaned_attachment_relink()->_match_session_payload()->write.py:_attachment_message_id_maps(), while apply inserts only the planned candidates.Verification
direnv exec . devtools test tests/unit/maintenance/test_blob_reference_closure.py-> 8 passed.direnv exec . devtools test tests/unit/storage/test_archive_tiers_write.py-> 84 passed.direnv exec . devtools test tests/unit/storage/test_attachment_relink.py-> 4 passed.direnv exec . devtools verify --quick-> exit 0, including format, lint, mypy, generated surfaces, layering, policy checks, and schema audit.git diff --check-> clean.a25704d50-> exit 0.No production archive or live data was accessed.
Compatibility/migration
No schema or archive data changes. Closure remains dry-run by default, and apply remains offline, backup-gated, additive, and receipt-backed.
Summary by CodeRabbit
New Features
blob-reference-closuremaintenance command to audit and safely repair missing blob references.Bug Fixes