chore: Delete delta-class lifecycle and baseline-commit gate; manifest… - #4666
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
polylogue/tests/unit/storage/test_attachment_cascade_sweep.py
Lines 67 to 69 in f89f450
Preserve the attachment-deletion regression tests
Deleting this whole file also removes two tests unrelated to lifecycle fast-forwarding: one proves that deleting a session sweeps the attachment whose only reference cascaded away, and the other proves that a shared attachment survives with its count decremented. The production delete_sessions sweep remains in place, but its named anti-vacuity coverage is now gone merely because later tests in the same file depended on the deleted lifecycle module; move these two tests to a surviving storage test module instead.
AGENTS.md reference: AGENTS.md:L162-L167
ℹ️ 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".
| label="gate schema-versioning", | ||
| in_quick=True, | ||
| ), | ||
| Gate( |
There was a problem hiding this comment.
For a checkout that changes SOURCE_DDL without bumping SOURCE_SCHEMA_VERSION or adding a numbered migration, devtools verify --quick can now pass because the schema-versioning gate was removed and schema-manifest was not added to quick_gates(); even the standalone command only renders the checkout's current DDL unless an archive root is supplied, so it cannot detect an undisclosed evolution change. This removes the PR-time guard against shipping code that expects a new shape while existing durable archives retain the old one, contrary to the required numbered-migration and lifecycle classification regimes.
AGENTS.md reference: AGENTS.md:L85-L92
Useful? React with 👍 / 👎.
| if path is None or not path.exists(): | ||
| return result |
There was a problem hiding this comment.
When --archive-root points to a nonexistent or partial archive, every missing database takes this return path with ok=True, so the command reports all six tiers as passing and exits 0 without comparing those tiers at all. In explicit archive-comparison mode, a missing source.db, user.db, or audit.db must be a failure rather than being treated like the intentional render-only mode used when no root is provided.
AGENTS.md reference: AGENTS.md:L65-L74
Useful? React with 👍 / 👎.
| result: dict[str, Any] = {"tier": tier.value, "version": expected.version, "ok": True} | ||
| if path is None or not path.exists(): | ||
| return result | ||
| with sqlite3.connect(f"file:{path.resolve()}?mode=ro", uri=True) as conn: |
There was a problem hiding this comment.
When the supplied archive-root path contains a legal filename character such as ? or #, interpolating the resolved path directly into a SQLite URI changes its meaning. For example, archive?copy/source.db is parsed as database archive plus query text, so this supposedly read-only verification can inspect or even create the wrong file instead of checking the requested tier. Construct the URI with path.resolve().as_uri() before appending ?mode=ro.
Useful? React with 👍 / 👎.
| args = parser.parse_args(argv) | ||
| results = [] | ||
| for tier in ArchiveTier: | ||
| path = args.archive_root / f"{tier.value}.db" if args.archive_root is not None else None |
There was a problem hiding this comment.
For an archive where .index-active-pointer selects a promoted generation while the conventional root index.db is a stale regular file—an explicitly handled interrupted-promotion state in archive_identity.py—this path construction checks the stale shadow instead of the index actually served by the application. The command can therefore report PASS when the active generation has schema drift, or fail because only the stale shadow differs; resolve ArchiveLocation and use its active_index_path for the index tier.
Useful? React with 👍 / 👎.
The rebase conflict resolution re-added the archive index-fast-forward CommandSpec that #4666 deleted along with devtools/index_fast_forward.py. Its module target does not exist.
The rebase conflict resolution re-added the archive index-fast-forward CommandSpec that #4666 deleted along with devtools/index_fast_forward.py. Its module target does not exist.
…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
Remove the retired derived-tier lifecycle and git-baseline schema gate. Add canonical schema-manifest verification and keep startup validation on the manifest route.
Problem
Derived-tier identity hashes and canonical manifests make delta-class declarations, fast-forward execution, and git-history DDL archaeology redundant. The old command and its tests also depended on repository history being available.
Solution
The lifecycle module, fast-forward actuator and executor, schema-versioning gate, command entry, and obsolete tests are deleted. The manifest compatibility variant is owned by
schema_manifest.py.devtools verify schema-manifestrenders all six canonical tiers and optionally compares archive files.Verification
uv run devtools verify schema-manifest:schema-manifest: PASSuv run devtools verify schema-manifest --json:"ok": true, all six tiers"ok": trueuv run devtools verify --quick: success, exit 0uv run devtools render all --check: all selected surfacessync OKgit diff --check: successResidual risk
The focused managed pytest selection was blocked by the shared pytest pueue slot being occupied by unrelated jobs. Static verification and direct manifest checks passed.