fix(maintenance): stop counting unowned attachments as attachment debt - #4701
Conversation
#4697 typed the writer's owner-ambiguous attachment as `attachment_unowned` in `source-conservation`, but two other declarations count the same row. `attachment-coverage` reports it as `unreachable` debt and `blob-reference-closure` as an acquired attachment lacking canonical refs, so seeded archives still verify non-green. All three now share one predicate. An acquired attachment is debt only when its `ref_count` is non-zero: the sweep set it while refs existed and the refs then disappeared without the sweep running, which is the polylogue-w06b state those checks exist to catch. A row inserted with ref_count 0 and deliberately kept out of the sweep never had a ref to lose, and `attachment-coverage` reports it as its own `unowned_count` dimension instead. `test_acquired_unreachable_attachment_debt_is_blocking` seeded ref_count 0, which is now the writer's deliberate shape; it seeds the stale non-zero count that actually witnesses lost refs. 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: 3a911e49df
ℹ️ 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".
| """SQL predicate for an acquired attachment whose refs went away.""" | ||
| return ( | ||
| f"{alias}.acquisition_status = 'acquired'\n" | ||
| f" AND {alias}.ref_count != 0\n" |
There was a problem hiding this comment.
For an archive affected by the documented pre-#3514 full-replace bug, the lost attachment_refs edge can leave an acquired attachment with ref_count = 0; test_orphaned_attachment_is_relinked_from_raw_reparse and test_scan_attachment_coverage_flags_acquired_row_with_no_attachment_ref preserve this exact live-archive shape. This predicate now excludes it, so both attachment-coverage and blob-reference-closure report OK even though normal reads cannot surface the attachment and the relink planner can recover it. ref_count = 0 therefore cannot by itself distinguish intentional owner ambiguity from historical attachment debt.
AGENTS.md reference: AGENTS.md:L192-L193
Useful? React with 👍 / 👎.
| AND NOT EXISTS ( | ||
| SELECT 1 FROM attachment_refs r WHERE r.attachment_id = a.attachment_id | ||
| ) | ||
| WHERE {acquired_attachment_missing_ref_predicate()} |
There was a problem hiding this comment.
For a single acquired owner-ambiguous attachment with no attachment_refs row, this filtered query produces acquired_unreachable_count = 0, while the unchanged acquired_reachable_count calculation returns 1. Consequently scan_attachment_coverage().to_dict() claims the attachment is reachable even though every documented read path inner-joins attachment_refs; the new unowned category needs to be excluded from the reachable total or physical unreachability must remain distinct from blocking debt.
Useful? React with 👍 / 👎.
An acquired attachment retained with an ambiguous owner has no ref, so it is not reachable through any read path either. Reporting it as neither unreachable-debt nor unowned left it counted as reachable and made the check's summary claim a live reference it does not have. `acquired_unowned_count` is scoped to acquired rows, which is this report's domain, and `acquired_reachable_count` subtracts it. Two further tests seeded `ref_count` 0 for the debt case, which is now the writer's deliberate shape; they seed the stale non-zero count that witnesses lost refs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
|
Second commit ( Excluding the unowned row from The same caller audit found two more tests seeding
|
|
Arming auto-merge. I had said I would hold for the local affected-file run, but it is queued behind a full The differential evidence method used here already proved accurate on #4697: it predicted that PR would clear |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636641bd48
ℹ️ 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".
| SELECT COUNT(*) FROM attachments a | ||
| WHERE a.acquisition_status = 'acquired' | ||
| AND NOT EXISTS (SELECT 1 FROM attachment_refs r WHERE r.attachment_id = a.attachment_id) | ||
| WHERE {acquired_attachment_missing_ref_predicate()} |
There was a problem hiding this comment.
For a writer-generated acquired attachment with an ambiguous owner (ref_count = 0 and no attachment_refs row), this predicate now reports closure as healthy, but _acquired_attachment_ids() still selects the same row for plan_blob_reference_closure(). A dry-run reconciliation therefore reports deliberate retained state as an orphan/blocker, or raises MessageOwnerAmbiguityError when the authoritative raw reproduces the ambiguous ownership because the relink path does not catch that exception. Apply the same exclusion to the repair planner's attachment population so audit and repair agree.
Useful? React with 👍 / 👎.
| SELECT a.attachment_id FROM attachments a | ||
| WHERE a.acquisition_status = 'acquired' | ||
| AND NOT EXISTS (SELECT 1 FROM attachment_refs r WHERE r.attachment_id = a.attachment_id) | ||
| WHERE {acquired_attachment_missing_ref_predicate()} |
There was a problem hiding this comment.
When an acquired owner-ambiguous attachment has ref_count = 0 and no reference, this new filter makes the check succeed, but the success branch still says that every acquired attachment has canonical reference closure. That observable is false for the newly exempted row and contradicts the attachment-coverage summary, which explicitly reports retained unowned attachments; include the unowned count or limit the claim to debt-bearing attachments.
Useful? React with 👍 / 👎.
1 participant
Summary
Follow-up to #4697. That PR typed the writer's owner-ambiguous attachment as
attachment_unownedinsource-conservation, but two further declarationscount 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:
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()inpolylogue/storage/blob_liveness.py, used byclosure_counts, theblob-reference-closuresample query, andscan_attachment_coverage.An acquired ref-less attachment is debt only when its
ref_countis 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_count0 and deliberately kept out of the sweep never had a ref to lose.scan_attachment_coveragereports those as their ownunowned_countdimension, and the OK summary names them rather than claiming every acquired
attachment has a live reference.
Verification
All three declarations, same fixture, only
ref_countvarying:The
ref_count=2column is the anti-vacuity control: the exemption does notdisable 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, noout of sync.ruffandmypyclean on the changed modules.Residuals
test_acquired_unreachable_attachment_debt_is_blockingpreviously seededref_count0. That is now precisely the writer's deliberate shape, so thetest 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.
verify_allon the host'ssingle pytest slot; I will post it here when it lands and will not enable
auto-merge before then.
test_empty_covered_table_without_stats_is_not_missing_coverage(already redin the 2026-09-05 corpus;
sqlite_stat1holds 3 rows for an emptyaction_pairs, contradicting the premise fix: profile, delegation and storage reds from the 2026-09-05 corpus #4692 documented) and adispatch_reasonkey added tosession_links.evidence_jsonby feat(lineage): resolve child session links from parent-side dispatch evidence #4690.🤖 Generated with Claude Code
https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid