fix(maintenance): type unowned attachments and share the ANALYZE set - #4697
Conversation
Two merged changes left the archive verification registry non-green on every seeded fixture, tripping 16 corpus tests at one seam. source-conservation: an attachment whose owning message is ambiguous is now retained as a typed unowned row. It has no ref, so it counted as `attachment_unreferenced`, which blocks. The two states split on `ref_count`: the writer inserts an owner-ambiguous row with ref_count 0 and keeps it out of the ref-count sweep, so ref_count 0 means it never had a ref -- the explained, non-blocking `attachment_unowned` term. A ref-less row with a non-zero ref_count lost its refs without the sweep running and is unreachable from every read path, so it keeps blocking. planner-stats: `session_links` joined the verified set, but the archive builders ANALYZEd a hand-copied table list that did not include it, so every seeded archive with a populated `session_links` warned. The covered set now lives in one constant that the check, the rebuild replay, and the builders all read, so a table added to it cannot go uncovered on a single route. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 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 |
![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: 6af709e3f7
ℹ️ 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".
| conn.execute( | ||
| f""" | ||
| SELECT COUNT(*) FROM idx_tier.attachments a | ||
| WHERE {unreferenced_predicate} AND a.ref_count = 0 |
There was a problem hiding this comment.
When a parsed attachment names a message that does not exist—such as the message_provider_id="missing-message" input already exercised by test_writer_skips_orphan_attachment_before_direction_validation—the production writer persists it without an attachment_refs row and with ref_count = 0. This predicate now classifies that malformed, unreachable attachment as the nonblocking attachment_unowned term even though the term's rule applies only to genuinely ambiguous owners; for an unfetched attachment, the other closure checks do not catch it, so archive verification can accept a rebuild that lost the attachment's owner. Persist or check an explicit ambiguity reason, or reject unmatched owner IDs, rather than treating every zero-count row as explained.
Useful? React with 👍 / 👎.
| "attachment was written unreferenced because its owning message is ambiguous " | ||
| "(ref_count 0, never swept); identity and bytes are retained as evidence" |
There was a problem hiding this comment.
For a genuinely ambiguous attachment carrying inline_bytes or a precomputed blob, the ingest route preacquires the bytes as acquired, and the writer intentionally stores the attachment with no ref and ref_count = 0. Although this new rule declares that state explained and says its bytes are retained as evidence, the same live/candidate verification registry still runs attachment-coverage and blob-reference-closure, both of which unconditionally return errors for every acquired attachment without a ref. Such valid input therefore still blocks archive verification or candidate promotion; those checks need to recognize explicit typed-unowned provenance, or this state needs a distinct acquisition status.
Useful? React with 👍 / 👎.
#4701) ## Summary Follow-up to #4697. That PR typed the writer's owner-ambiguous attachment as `attachment_unowned` in `source-conservation`, but two further declarations count the same row, so seeded archives still verify non-green and the convergence tests stay red. ## Problem Running the previously-red files at #4697's head, the registry assertion is still tripped, now by a different pair: ``` archive verification registry is not green: [ ('attachment-coverage', 'error', 'acquired attachment debt: missing_blob=0, unreachable=1', ...), ('blob-reference-closure', 'error', '0 raw session(s) and 1 acquired attachment(s) lack canonical refs', ...)] ``` 14 of the 19 failures in that run reduce to this one attachment. Both checks ask "is this acquired attachment reachable through a ref?", and both answer no for a row the writer deliberately created without one. Three separate call sites carried their own copy of the "acquired and ref-less" SQL, so fixing one would have left the others reporting the same row. ## Solution One shared predicate, `acquired_attachment_missing_ref_predicate()` in `polylogue/storage/blob_liveness.py`, used by `closure_counts`, the `blob-reference-closure` sample query, and `scan_attachment_coverage`. An acquired ref-less attachment is debt only when its `ref_count` is non-zero. That is the polylogue-w06b state these checks exist to catch: the sweep set the count while refs existed, then the refs disappeared without the sweep running, leaving a row unreachable from every read path. A row inserted with `ref_count` 0 and deliberately kept out of the sweep never had a ref to lose. `scan_attachment_coverage` reports those as their own `unowned_count` dimension, and the OK summary names them rather than claiming every acquired attachment has a live reference. ## Verification All three declarations, same fixture, only `ref_count` varying: ``` === owner-ambiguous (ref_count=0) === attachment-coverage ok all 1 acquired attachment(s) have bytes and a live attachment reference; 1 retained unowned blob-reference-closure ok every raw session and acquired attachment has canonical reference closure source-conservation ok ... non_session_artifact=1; attachment_unowned=1 === stale ref_count (ref_count=2) === attachment-coverage error acquired attachment debt: missing_blob=0, unreachable=1 blob-reference-closure error 0 raw session(s) and 1 acquired attachment(s) lack canonical refs source-conservation error ... attachment_unreferenced=1! ``` The `ref_count=2` column is the anti-vacuity control: the exemption does not disable any of the three checks, it only stops them claiming the explained case. `.venv/bin/python -m devtools verify --quick` — exit 0, all 13 gates ok, no `out of sync`. `ruff` and `mypy` clean on the changed modules. ## Residuals - `test_acquired_unreachable_attachment_debt_is_blocking` previously seeded `ref_count` 0. That is now precisely the writer's deliberate shape, so the test seeded a case that is no longer debt; it seeds the stale non-zero count that actually witnesses lost refs. The scenario it describes is unchanged. - The affected-file run is queued behind a full `verify_all` on the host's single pytest slot; I will post it here when it lands and will not enable auto-merge before then. - Two failures in that run are inherited from master, not addressed here: `test_empty_covered_table_without_stats_is_not_missing_coverage` (already red in the 2026-09-05 corpus; `sqlite_stat1` holds 3 rows for an empty `action_pairs`, contradicting the premise #4692 documented) and a `dispatch_reason` key added to `session_links.evidence_json` by #4690. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Summary
Restores the archive verification registry to green on seeded fixtures. Two
merged PRs each left one declaration non-green, and 16 corpus tests assert the
whole registry at a single seam (
tests/infra/convergence_harness.py:371), soboth surfaced as the same failure.
Problem
Corpus run on master 754553b (
/realm/tmp/work/corpus-2026-09-05c.log):source-conservation. #4692 (917e793) stopped dropping an attachment whose
owning message is ambiguous and began retaining it as a typed unowned row
(
ref_count0, deliberately excluded from the ref-count sweep). It has noattachment_refsrow, sosource-conservationcounted it underattachment_unreferenced, which blocks. A deliberate, explained retention wasreported as archive corruption.
planner-stats. #4685 (0fa2a9a) added
session_linksto the verifiedcovered set. The empty-table exemption added by #4692 is working correctly —
the warning fires because
session_linksis populated and un-ANALYZEd. Theproduction rebuild route (
rebuild_index.py) did ANALYZE it; the test archivebuilders ANALYZEd a separately hand-copied list
(
tests/infra/convergence_harness.py,tests/infra/schema_inference.py) thatstill read
blocks, messages, action_pairs. One route drifted from theverified set with nothing to catch it.
Solution
source-conservation splits the ref-less attachment population on
ref_count, which is the discriminator the writer already establishes:ref_count = 0— inserted unreferenced and kept out of the sweep, so itnever had a ref: the new non-blocking
attachment_unownedterm.ref_count != 0— refreshed while refs existed, then lost them without thesweep running. This is exactly the state
refresh_and_sweep_attachment_rowsdocuments as unreachable from every readpath; it stays
attachment_unreferencedand keeps blocking.No schema change: adding a marker column to the index tier would have forced an
INDEX_SCHEMA_VERSIONbump, regenerated schema-disposition artifacts, and arebuild plan, to record a fact two existing columns already determine.
planner-stats moves the covered set to one constant,
PLANNER_STATS_COVERED_TABLESinpolylogue/storage/sqlite/maintenance.py,with an
analyze_planner_stats_tables()helper. The verification check, therebuild replay, and both test builders now read it, so a table added to the
verified set cannot go uncovered on a single route.
rebuild_indexstillANALYZEs
sessionsin addition, for its session-scoped replay lookups.Verification
Red-first, measured against the same fixtures with the pre-fix modules loaded
side by side:
The
ref_count=2row is the control: the split does not disable the blockingterm, it only stops it claiming the explained case.
Four new tests, each naming its anti-vacuity condition:
4 passed, 3 warnings in 13.81s..venv/bin/python -m devtools verify --quick— exit 0, all 13 gates ok, noout of sync.ruff check/ruff format --checkclean acrosspolylogue/andtests/;mypyclean on the changed production modules.Residuals
files) is still queued behind an unrelated full
verifyon the host's singlepytest slot. Both non-green terms that produced the assertion are proven
resolved above, but those 16 tests have not yet been observed green in one
run; I will post the result on this PR when it lands.
test_convergence_builder_analyze_covers_every_verified_tableimports thebuilder's private
_analyze_registry_tablesdeliberately: asserting againsta restatement of the table list would not catch a builder reverting to a
hand-copied one.
docs/internals.mdstillpoint at
polylogue/storage/sqlite/lifecycle.pyand the delta-classlifecycle mechanism, deleted in chore: Delete delta-class lifecycle and baseline-commit gate; manifest… #4666 (879a995). Wants its own sweep.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid