feat(maintenance): add source-conservation and population-coverage gates - #4685
Conversation
|
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 |
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. |
![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: c3724283ca
ℹ️ 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".
| executable_wires: dict[str, str] = { | ||
| provider.value: spec.origin.value | ||
| for spec in specs | ||
| if spec.lifecycle == "executable" |
There was a problem hiding this comment.
When source.db contains the valid compatibility row (origin='unknown-export', detected_provider='unknown'), the origin is correctly classified as unsupported_declared, but this executable-only filter drops the declared Provider.UNKNOWN wire, causing the detector-route construct to become uncovered and the gate to fail. Include non-executable provider wires whose capability-matrix entry has a typed unsupported receipt, and classify those routes as unsupported rather than undeclared.
AGENTS.md reference: AGENTS.md:L96-L101
Useful? React with 👍 / 👎.
| present = [witness.fixture_path for witness in entry.witnesses if (REPO_ROOT / witness.fixture_path).is_file()] | ||
| if not present: | ||
| return None | ||
| return COVERED, ";".join(present) |
There was a problem hiding this comment.
If one of aistudio-drive's two declared witness paths is missing while the other still exists, this filtering leaves a nonempty present list and returns covered, so the gate exits successfully despite one provider route having no real fixture. Check every witness path and report the entry uncovered when any declared witness is absent.
AGENTS.md reference: AGENTS.md:L175-L176
Useful? React with 👍 / 👎.
| def _source_exists(archive_root: Path, source_path: str) -> bool: | ||
| path = Path(source_path) | ||
| if not path.is_absolute(): | ||
| path = archive_root / path | ||
| return path.exists() |
There was a problem hiding this comment.
For normally ingested ZIP exports, raw_sessions.source_path is stored as <archive.zip>:<member> by ZipEntryReadContext.source_path, so treating that entire coordinate as a filesystem path always returns false even while the source ZIP still exists. Consequently every ZIP-backed raw is retyped as source_missing, hiding its actual materialization/exclusion term and inflating the missing-source evidence; probe the container path using the stored ZIP coordinate metadata instead.
Useful? React with 👍 / 👎.
| known_kind = kind in {member.value for member in ArtifactKind} | ||
| if spec is None or not known_kind or kind == ArtifactKind.UNKNOWN.value: | ||
| return CoverageConstruct("artifact-kind", key, UNCOVERED, "no artifact declaration", "none", count) |
There was a problem hiding this comment.
When inventory contains a production failure carrier such as terminal_corrupt_input/decode_failed or deferred_cas_frontier/partial_decode, this ArtifactKind-only check classifies it as uncovered, although these are valid RawFailureEvidenceKind rows written by the ingest pipeline. As a result, ordinary typed parse failures make population-coverage --archive-root fail; validate the closed failure-kind/support-status pairs and report them as their declared deferred or terminal exclusion.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
| if parse_as_session == 0 and artifact_kind is not None and artifact_kind != "unknown": | ||
| lineage_class = f"artifact:{artifact_kind}" |
There was a problem hiding this comment.
After a deferred CAS raw succeeds, supersede_deferred_cas_evidence intentionally retains its artifact row as terminal_superseded_deferred_cas_frontier with parse_as_session=0; if the resulting session is indexed, this condition labels that legitimate session a blocking phantom. Distinguish raw-failure lifecycle and resolution carriers from actual non-session artifact classifications before applying the phantom-lineage rule.
AGENTS.md reference: AGENTS.md:L112-L115
Useful? React with 👍 / 👎.
| #: index-tier tables the planner-stats check expects ``ANALYZE`` coverage for | ||
| #: (polylogue-l3tk: fresh generations without stats pick pathological plans). | ||
| _PLANNER_STATS_COVERED_TABLES: tuple[str, ...] = ("blocks", "messages", "action_pairs") | ||
| _PLANNER_STATS_COVERED_TABLES: tuple[str, ...] = ("blocks", "messages", "session_links", "action_pairs") |
There was a problem hiding this comment.
A valid archive with no lineage edges has an empty session_links table, and SQLite's ANALYZE session_links creates no sqlite_stat1 row for an empty table. Adding it unconditionally to this tuple therefore makes planner-stats warn even after the production rebuild's declared ANALYZE session_links step completed; treat an empty covered table as satisfied or seed an intentional representative statistic.
Useful? React with 👍 / 👎.
| declared_origin = executable_wires.get(str(provider)) | ||
| wire = Provider.from_string(str(provider)) | ||
| mapped = origin_from_provider(wire).value if wire is not Provider.UNKNOWN else None | ||
| if declared_origin is None or mapped != str(origin): |
There was a problem hiding this comment.
For a valid raw acquired under claude-code-session whose parser records detected_provider='codex', this equality check marks claude-code-session/codex uncovered even though the storage contract intentionally keeps immutable acquisition origin separate from the provider that replay should route through. Judge whether the detected provider has a declared parser wire independently of the stored acquisition origin rather than requiring its derived origin to match.
AGENTS.md reference: AGENTS.md:L96-L101
Useful? React with 👍 / 👎.
| _declared_owner( | ||
| name="source-conservation", | ||
| semantic_owner="source-materialization", | ||
| applicable_routes=frozenset({_ROUTE_LIVE}), |
There was a problem hiding this comment.
The new check is declared only for live-archive, so the production cross-tier candidate route never runs it, and verify_archive(..., index_path_override=..., checks=('source-conservation',)) rejects the check as unsupported despite the newly added _check_source_conservation_at_index_path implementation. A candidate containing the phantoms, orphans, or unexplained rows this check is meant to gate can therefore be activated before the problem is observed; add the cross-tier candidate route and bind the override path.
AGENTS.md reference: AGENTS.md:L85-L90
Useful? React with 👍 / 👎.
Add the `source-conservation` verify-archive declaration: every acquired source item types into exactly one term citing its rule, and every index session, message, block, and attachment ref traces back to a raw row. Phantom sessions (polylogue-b508) are reported by lineage and never deleted. A source file that is gone while the archive still retains its raw payload bytes is typed accounting (`source_missing`); only genuine byte loss (`source_lost`) blocks, alongside unexplained, unclassified, orphan, and phantom terms. Add `devtools gate population-coverage`: every origin, detector route, and artifact kind is declared and witnessed. Coverage comes from the declarations, so a stored `recognized_unparsed` support status is not itself a declaration, and the gate evaluates a source inventory only when `--archive-root` names one - its result never depends on ambient machine state. `planner-stats` now covers the documented set including `session_links`, and the coherent fixture ANALYZEs the whole index tier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
c372428 to
7900e50
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7900e502c6
ℹ️ 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".
| retained_expr = ( | ||
| "(r.blob_hash IS NOT NULL AND EXISTS(SELECT 1 FROM blob_refs b WHERE b.blob_hash = r.blob_hash))" | ||
| if table_exists(conn, "blob_refs") | ||
| else "(r.blob_hash IS NOT NULL)" |
There was a problem hiding this comment.
When the original source file and its CAS blob are both missing but the normal blob_refs row remains, this predicate still sets bytes_retained=1; the later filesystem override therefore reports nonblocking source_missing, and verify-archive --check source-conservation returns OK despite losing the durable raw payload. A reference row proves intended liveness, not physical byte presence, so this check must verify the blob itself before deciding between source_missing and source_lost.
AGENTS.md reference: AGENTS.md:L65-L70
Useful? React with 👍 / 👎.
| evaluated = source_db is not None and source_db.is_file() | ||
| if evaluated and source_db is not None: | ||
| constructs.extend(inventory_constructs(source_db, specs=specs, manifest=manifest)) |
There was a problem hiding this comment.
When --archive-root contains a typo, points to an uninitialized archive, or otherwise lacks source.db, this silently skips inventory evaluation and main() still exits 0 as long as the static declarations pass. For example, python -m devtools.verify_population_coverage --archive-root /tmp/nonexistent prints Population coverage: PASS, so an operator or CI job can believe the requested archive was checked when none of its population was read; supplying --archive-root should make a missing source tier an uncovered/error result.
Useful? React with 👍 / 👎.
| SELECT COUNT(*) FROM idx_tier.attachment_refs ar | ||
| WHERE NOT EXISTS (SELECT 1 FROM idx_tier.messages m WHERE m.message_id = ar.message_id) | ||
| """ |
There was a problem hiding this comment.
For a schema-valid attachment_refs row whose session_id names session A while its message_id names a message belonging to session B, this predicate reports no orphan because the message exists. The attachment is then returned under A by attachment queries while carrying B's message ID, yet source conservation can remain green; compare the referenced message's session_id with ar.session_id so cross-session ownership mismatches become blocking evidence.
AGENTS.md reference: AGENTS.md:L36-L42
Useful? React with 👍 / 👎.
| for session_id, native_id, origin, source_path, parse_as_session, artifact_kind in conn.execute( | ||
| f""" | ||
| SELECT s.session_id, s.native_id, s.origin, r.source_path, {parse_as_session_expr}, {kind_expr} | ||
| FROM idx_tier.sessions s | ||
| JOIN raw_sessions r ON r.raw_id = s.raw_id |
There was a problem hiding this comment.
When a raw is acquired under one origin but its detected parser materializes a session under another—such as a claude-code-session raw routed through the Codex parser—this query retains only s.origin. The fallback phantom check consequently tests the raw source_path against Codex artifact rules instead of the acquisition origin's rules, so a Claude sidecar path without a raw_artifacts row can evade phantom_declared_non_session_lineage; select r.origin separately and use it for _declared_non_session_rule.
AGENTS.md reference: AGENTS.md:L94-L101
Useful? React with 👍 / 👎.
| elif witness is None: | ||
| out.append(CoverageConstruct("origin-declaration", origin, UNCOVERED, route, "no unsupported receipt")) | ||
| else: | ||
| out.append(CoverageConstruct("origin-declaration", origin, UNSUPPORTED_DECLARED, route, witness[1])) |
There was a problem hiding this comment.
If a reserved or compatibility-only origin such as beads-issue is accidentally changed in the capability matrix from an unsupported receipt to an ordinary fixture witness, _matrix_witness() returns COVERED, but this branch relabels that result as unsupported_declared and the gate still passes. That defeats the declaration check precisely when the matrix starts claiming executable evidence for a non-executable lifecycle; require witness[0] == UNSUPPORTED_DECLARED here and report every other status as uncovered.
Useful? React with 👍 / 👎.
…4697) ## 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`), so both surfaced as the same failure. ## Problem Corpus run on master 754553b (`/realm/tmp/work/corpus-2026-09-05c.log`): ``` AssertionError: archive verification registry is not green: [('planner-stats', 'warning', 'sqlite_stat1 missing coverage for: session_links (polylogue-l3tk class)', ...), ('source-conservation', 'error', '5 raw item(s), 0 hook event(s), 4 index session(s); revision_superseded=1; attachment_unreferenced=1!', ...)] ``` **source-conservation.** #4692 (917e793) stopped dropping an attachment whose owning message is ambiguous and began retaining it as a typed unowned row (`ref_count` 0, deliberately excluded from the ref-count sweep). It has no `attachment_refs` row, so `source-conservation` counted it under `attachment_unreferenced`, which blocks. A deliberate, explained retention was reported as archive corruption. **planner-stats.** #4685 (0fa2a9a) added `session_links` to the verified covered set. The empty-table exemption added by #4692 is working correctly — the warning fires because `session_links` is *populated* and un-ANALYZEd. The production rebuild route (`rebuild_index.py`) did ANALYZE it; the test archive builders ANALYZEd a separately hand-copied list (`tests/infra/convergence_harness.py`, `tests/infra/schema_inference.py`) that still read `blocks, messages, action_pairs`. One route drifted from the verified 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 it never had a ref: the new non-blocking `attachment_unowned` term. - `ref_count != 0` — refreshed while refs existed, then lost them without the sweep running. This is exactly the state `refresh_and_sweep_attachment_rows` documents as unreachable from every read path; it stays `attachment_unreferenced` and keeps blocking. No schema change: adding a marker column to the index tier would have forced an `INDEX_SCHEMA_VERSION` bump, regenerated schema-disposition artifacts, and a rebuild plan, to record a fact two existing columns already determine. **planner-stats** moves the covered set to one constant, `PLANNER_STATS_COVERED_TABLES` in `polylogue/storage/sqlite/maintenance.py`, with an `analyze_planner_stats_tables()` helper. The verification check, the rebuild replay, and both test builders now read it, so a table added to the verified set cannot go uncovered on a single route. `rebuild_index` still ANALYZEs `sessions` in addition, for its session-scoped replay lookups. ## Verification Red-first, measured against the same fixtures with the pre-fix modules loaded side by side: ``` === owner-ambiguous (ref_count=0) === OLD: blocking_count=1 ... attachment_unreferenced=1! NEW: blocking_count=0 ... attachment_unowned=1 === stale ref_count (ref_count=2) === OLD: blocking_count=1 ... attachment_unreferenced=1! NEW: blocking_count=1 ... attachment_unreferenced=1! ``` ``` OLD builder ANALYZE set : warning missing=['session_links'] NEW builder ANALYZE seam: ok missing=[] ``` The `ref_count=2` row is the control: the split does not disable the blocking term, 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, no `out of sync`. `ruff check` / `ruff format --check` clean across `polylogue/` and `tests/`; `mypy` clean on the changed production modules. ## Residuals - The 13-file affected-set run (the previously-red convergence/property/reindex files) is still queued behind an unrelated full `verify` on the host's single pytest 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_table` imports the builder's private `_analyze_registry_tables` deliberately: asserting against a restatement of the table list would not catch a builder reverting to a hand-copied one. - Unrelated, found while researching: CLAUDE.md and `docs/internals.md` still point at `polylogue/storage/sqlite/lifecycle.py` and the delta-class lifecycle mechanism, deleted in #4666 (879a995). Wants its own sweep. 🤖 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
Adds two verification instruments: the
source-conservationverify-archivedeclaration (polylogue-bx26w, with the phantom-session term for polylogue-b508)
and
devtools gate population-coverage(polylogue-rrxe4.1).Problem
Nothing accounted for the whole acquired population.
source-index-coveragereasons over logical heads, so a raw row that is neither a head nor an error
could vanish from every ledger, and no check asked the reverse question: does
every index session, message, block, and attachment ref trace back to a raw
row it is entitled to come from? Against the live archive that gap hides real
material — sessions materialized from declared non-session artifacts
(polylogue-b508 phantoms) are indistinguishable from real ones by row shape.
Separately, no gate asserted that every origin, detector route, and artifact
kind observed in a source inventory is actually declared and witnessed.
Solution
source-conservationtypes every acquired source item — eachraw_sessionsrow, hook event, and history sidecar — into exactly one term that cites the
rule explaining it, and traces every index row back to its owner. Only
unexplained terms block:
unexplained,unclassified_shape,source_lost,orphans, and phantoms. Phantoms are reported by lineage and never deleted.
A source file that no longer exists on disk is not by itself a loss. When the
archive still retains the raw payload bytes the content is conserved, so that
is typed accounting (
source_missing); only a vanished file whose bytes arealso gone (
source_lost) blocks. This is what makes the check usable as theacceptance instrument for a rebuilt archive, where 12,717 rows are
source-missing by ordinary export rotation.
population-coveragederives coverage from the declarations. A storedrecognized_unparsedsupport status is a row value, not a declaration, so itno longer confers coverage on a kind that no
OriginSpecrule claims. Thegate evaluates a source inventory only when
--archive-rootnames one; itpreviously defaulted to the operator's configured archive, which made a repo
gate's verdict depend on ambient machine state.
Two fixes fall out of the fixture work:
planner-statsnow covers the setdocs/maintenance.mdand CLAUDE.md already documented (session_linkswasmissing from the code's tuple while
action_pairswas never ANALYZEd by thecoherent fixture, so the check warned on every seeding), and the coherent
fixture ANALYZEs the whole index tier.
Verification
.venv/bin/python -m devtools test tests/unit/maintenance/test_source_conservation.py tests/unit/devtools/test_verify_population_coverage.py tests/unit/maintenance/test_archive_verification.py.venv/bin/python -m devtools verify --quick— all 13 gates ok, noout of sync..venv/bin/python -m devtools gate population-coverage—Population coverage: PASS.Against the live archive, the forward direction accounts for all 43,124 raw
rows: 20,206 materialized, 8,153 revision-superseded, 12,717 source-missing,
1,140 non-session, 483 pending, 323 sidecar artifacts, 68 parse failures, and
34 unexplained. Reverse: 23,496 sessions, of which 5,072 are phantoms by
lineage, plus 1,858 unreferenced attachments.
Each changed test names its anti-vacuity condition:
test_deleted_source_file_retypes_the_raw_as_source_missingis vacuouswithout the on-disk probe (the raw stays
materialized);test_deleted_source_file_without_retained_bytes_trips_source_conservationis vacuous without the retained-bytes join (byte loss reads as the
non-blocking term);
test_removed_artifact_rule_turns_its_kind_uncoveredisvacuous if coverage is read off the stored support status rather than the
declarations.
Residuals
The 34 unexplained raws and 5,072 phantoms are reported, not resolved — that
is the point of the instrument, and closing them is separate work.
Run against the live archive,
population-coverage --archive-rootreports twouncovered constructs,
aistudio-drive/unknown/decode_failed(15) andaistudio-drive/unknown/unknown(2). Both predate this change and want theirown bead.
source-conservationstats every distinct source path once per run. That isbounded by distinct paths rather than raw rows, but it is real I/O on a cold
cache; callers that must not touch the filesystem can pass
probe_filesystem=False.Beads: polylogue-bx26w, polylogue-rrxe4.1, polylogue-b508
🤖 Generated with Claude Code
https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid