feat: admit confirmed_online_support and declare the confirmation seam - #474
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe lifecycle terminal-reason contract now accepts ChangesProvisional publication confirmation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR adds a lifecycle reason and a declared storage seam without changing current runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
confirm_user_playbook_provisional_publication joins restore and displace on UserPlaybookLifecycleTerminationStore and on the base storage playbook mixin. Declared on the base rather than left as an enterprise-only extra: the three are one termination triple over the same lifecycle row, and a backend satisfying two thirds of the Protocol would be a surface every reader has to special-case. There is no reason parameter -- confirmed_online_support is the only reason this transition can record -- and the six evidence counts are re-checked by the RPC inside its own transaction rather than validated twice.
guangyu-reflexio
force-pushed
the
codex/offline-tuner-open-world-phase6
branch
from
August 31, 2026 23:40
a461288 to
110db92
Compare
guangyu-reflexio
changed the base branch from
codex/offline-tuner-open-world-phase5
to
main
August 31, 2026 23:40
guangyu-reflexio
added a commit
that referenced
this pull request
Sep 1, 2026
…th (#475) The open-source half of Phase 7 (the §21 clean break): contracts the replay literals out of the public type surface and fixes the SQLite allowlist rebuild so a *removal* actually takes effect. Stacked on #474 (Phase 6). Review only the commits above that base. ## The defect this fixes, which is the interesting part Both SQLite allowlist rebuilds trigger on a **positive** substring predicate: ```python if all(check in table_sql for check in required_checks) and ( "'offline_tuner_open_world'" in table_sql ): return ``` That is correct for *adding* a literal and silently wrong for *removing* one. Drop a literal from the target tuple and an existing database — whose `table_sql` still contains every remaining required string — satisfies `all(...)` and returns early. **The rebuild never runs and the old permissive CHECK survives forever.** A migration that reports success while changing nothing. The removal direction needs a negative predicate: rebuild if any *retired* literal is still present. Proven by contrast rather than asserted: | `_base.py` triggers | result | | --- | --- | | negative predicate (shipped) | 4 passed | | reverted to positive-only | 2 failed — `assert 'offline_tuner_replay' not in 'CREATE TABL…'` | ## Remediation, because the rebuild is a table copy A row carrying a retired value fails the copy, so the copy remediates: `offline_tuner_replay` → `offline_tuner_legacy` (accurate — those jobs *were* offline-tuner jobs, and that literal stays admissible), retired `stage`/`terminal_outcome` values nulled (both columns nullable), and `replay_manifest` artifact rows deleted (the column is NOT NULL and part of a UNIQUE key, so there is nothing to relabel it to). This is local-development SQLite, not tenant Postgres. No production row is touched. Verified end to end: a legacy database carrying a replay job and a `replay_manifest` artifact migrates rather than failing, the job returns relabelled with `stage`/`terminal_outcome` NULL, the `candidate` artifact survives, and the rebuild is idempotent. ## Type surface `optimizer_kind`, `stage`, `terminal_outcome` and `artifact_kind` unions lose their replay members. Four kinds survive. **Backward compatibility, stated precisely:** this is a **read** break, not a write break. An older image reads a retired-literal row fine — its own `Literal` still admits it. The *new* image raises `ValidationError` at model construction. Tenant Postgres still stores such rows until the enterprise contract migration lands, so during that bake a new task reading an old replay row raises. That is the clean break by design; the enterprise side removed the path that would act on such a row. ## Scope The OSS package remains importable and self-consistent standing alone — full non-e2e tier: **6,379 passed, 13 skipped, 0 failed**, run from the package's own venv. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Retired replay-specific optimizer types, job stages, terminal outcomes, and artifacts are no longer supported. * Publication claims and legacy publication requests now support GEPA only; open-world optimization remains supported for decision proofs. * **Bug Fixes** * Database constraint validation now rebuilds incomplete schemas to preserve support for all valid optimizer values and stages. * **Tests** * Expanded coverage for outcome reachability, publication validation, migrations, storage compatibility, and rejected replay transitions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Adds the open-source half of Phase 6 (passive confirmation): a ninth lifecycle terminal reason and the storage seam the enterprise confirm path implements.
Stacked on #473 (Phase 5). Review only the two commits above that base.
What this adds
LIFECYCLE_TERMINAL_REASONSgainsconfirmed_online_support— nine values, mirroring the tenant CHECK. Phase 5's note that Phase 6 would need no contract migration is true of the restore half and false of the confirm half: every one of the previous eight reasons describes why a successor was pulled, sostate = 'confirmed'was unrepresentable.confirm_user_playbook_provisional_publicationdeclared on theUserPlaybookStoreMixin, besiderestore_anddisplace_, raisingNotImplementedErroron backends that do not support provisional confirmation.Why the seam lives here rather than in the enterprise allowlist
_ENTERPRISE_ONLY_METHODSis for methods absent from the OSS ABC. This one is present on it, exactly like its two siblings — registering it in the allowlist would have failedtest_storage_public_method_surface_matches.Scope
Contract only. Nothing in this repository calls the new method, and the OSS backends inherit the
NotImplementedError. The enterprise counterpart carries the RPC, the day-14 boundary, and the memo intake.Rollout note for whoever composes this later
The reason-set widening must be fully deployed — all tasks on the new image — before the confirm path is wired, not merely merged. During a rollout the first new task widens the SQL CHECK while old-image tasks still hold the eight-value set, and an old task reading back a confirmed lifecycle would raise
lifecycle terminal result reason is not enumerated. Unreachable today, since nothing can commit a confirmation.Summary by CodeRabbit