feat: add open-world evidence foundation - #407
Merged
guangyu-reflexio merged 28 commits intoAug 3, 2026
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds canonical session-outcome identities, retry conflict handling, fenced purge execution claims, synchronous search-exposure recording, durable finalization receipts, and revised learning billing. It also adds migrations, retention targets, API validation, and regression coverage. ChangesSession outcome identity and finalization
Governed erasure
Search exposure recording
Durable finalization and learning billing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (21)
tests/server/services/test_search_exposure.py (1)
46-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-positive
interaction_idrule.
SearchExposureBatch.__post_init__converts a non-positiveinteraction_idtoNone(reflexio/server/services/search_exposure.py:37-38). That conversion changes the identity tuple and can activate theinvocation_idfallback. No test asserts it, while the analogous whitespace rule forrequest_idandsession_idis covered at lines 109-156.Add a case that passes
interaction_id=0and asserts both the normalized value and the resultingexposure_event_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/test_search_exposure.py` around lines 46 - 62, Add a test case alongside the existing normalization tests for the _batch helper using interaction_id=0; assert SearchExposureBatch normalizes interaction_id to None and that exposure_event_id reflects the resulting invocation_id fallback identity.reflexio/server/routes/search.py (1)
426-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the synchronous recorder call in a
profile_stepspan.Every other significant step in this endpoint reports timing:
search.reflexio_cacheat line 405 andsearch.response_viewat line 410. The exposure recording call performs a durable write on the request path but reports no timing. If the recorder becomes slow, the endpoint profile will not show the source.The batch uses
response.user_playbooksrather than the view models. That is correct for thetuple[UserPlaybook, ...]contract.♻️ Proposed span
- record_search_exposures( - SearchExposureBatch( - org_id=org_id, - request_id=payload.request_id, - session_id=payload.session_id, - interaction_id=payload.interaction_id, - user_id=payload.user_id, - user_playbooks=tuple(response.user_playbooks), - ) - ) + with profile_step("search.exposure_recording"): + record_search_exposures( + SearchExposureBatch( + org_id=org_id, + request_id=payload.request_id, + session_id=payload.session_id, + interaction_id=payload.interaction_id, + user_id=payload.user_id, + user_playbooks=tuple(response.user_playbooks), + ) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/routes/search.py` around lines 426 - 435, Wrap the synchronous record_search_exposures call in the endpoint’s existing profile_step mechanism using the span name search.record_search_exposures, while preserving the current SearchExposureBatch construction and response.user_playbooks tuple.reflexio/server/services/search_exposure.py (1)
82-86: 🩺 Stability & Availability | 🔵 TrivialBound the recorder call latency at this boundary.
recordruns synchronously on the search request path. The recorder is a durable ledger write. If the ledger becomes slow, search latency degrades and threadpool workers stay occupied.Failing closed is intentional; the boundary test confirms it. The gap is the absence of a latency bound. Define a timeout budget and a fast-fail rule for the recorder contract, and document that requirement in the
SearchExposureRecorderprotocol docstring so enterprise implementations honor it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/search_exposure.py` around lines 82 - 86, Update record_search_exposures to enforce a defined timeout budget and fast-fail behavior around the synchronous recorder.record call, preserving the existing no-recorder path and failing-closed semantics. Add the corresponding latency-bound requirement to the SearchExposureRecorder protocol docstring, using the existing timeout/error-handling conventions where available.reflexio/client/client.py (1)
1115-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
ValidationErrorraised on a partial server identity tuple.
SetSessionOutcomeResponserejects a response where only some of the four identity fields are populated.tests/client/test_session_outcomes_client.py:test_mark_session_outcome_rejects_partial_server_identityconfirmsmark_session_outcomepropagates thatValidationError. SDK callers pinned to a newer client against an older server hit this. Add it to the docstring so the failure mode is discoverable.📝 Proposed docstring addition
``recorded=False``. Sessions may report ``success``, ``failure``, or ``unknown`` and are not required to report an outcome. + + Raises: + pydantic.ValidationError: If the server returns only some of the + four outcome identity fields. They must be all populated or + all null. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/client/client.py` around lines 1115 - 1129, Update the docstring for the session-outcome recording method to document that mark_session_outcome propagates ValidationError when SetSessionOutcomeResponse contains only a partial subset of its four identity fields, including the compatibility failure with older servers.tests/server/services/storage/test_storage_contract_session_outcomes.py (2)
124-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the legacy-schema downgrade block into a helper.
The same five statements repeat verbatim in
test_legacy_all_null_identity_changed_payload_still_conflicts(Lines 178-194) andtest_legacy_all_null_identity_changed_governance_context_conflicts(Lines 230-245). Three copies of the table-copy and null-out sequence make the tests hard to read and hard to update when the identity column set changes.♻️ Proposed helper
def _downgrade_to_legacy_identity(sqlite_storage: SQLiteStorage, session_id: str) -> None: """Recreate session_outcomes without constraints and null the identity tuple.""" sqlite_storage.conn.execute( "CREATE TABLE legacy_session_outcomes AS SELECT * FROM session_outcomes" ) sqlite_storage.conn.execute("DROP TABLE session_outcomes") sqlite_storage.conn.execute( "ALTER TABLE legacy_session_outcomes RENAME TO session_outcomes" ) sqlite_storage.conn.execute( """UPDATE session_outcomes SET outcome_id = NULL, outcome_revision = NULL, outcome_contract_digest = NULL, finalized_trajectory_digest = NULL WHERE session_id = ?""", (session_id,), ) sqlite_storage.conn.commit()Each test then calls
_downgrade_to_legacy_identity(sqlite_storage, "legacy-retry").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/test_storage_contract_session_outcomes.py` around lines 124 - 140, Extract the repeated legacy-schema downgrade sequence into a helper such as _downgrade_to_legacy_identity, accepting SQLiteStorage and session_id, and move the table recreation, identity-column nulling, and commit statements into it. Replace the three duplicated blocks in the affected tests with calls to this helper, passing each test’s session identifier.
3-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBackend-specific tests are mixed into a backend-agnostic contract suite.
This file is a
test_storage_contract_*suite whosestoragefixture is typedBaseStorage. The new tests importSQLiteStorageand the private_canonical_session_snapshothelper, then usecast(SQLiteStorage, storage)to run raw SQL and to install asqlite3trace callback. If thestoragefixture is ever parameterized over Supabase or Postgres, these tests fail rather than skip, becausecastperforms no runtime check.Move the SQLite-only tests to
tests/server/services/storage/sqlite_storage/, or guard them withpytest.skipwhenstorageis not aSQLiteStorage. Keep the genuinely portable cases (test_first_write_preserves_outcome_fields,test_exact_finalization_retry_is_idempotent) here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/test_storage_contract_session_outcomes.py` around lines 3 - 15, Separate the SQLite-specific tests from the backend-agnostic contract suite: move tests that use SQLiteStorage, _canonical_session_snapshot, raw SQL, or sqlite3 tracing into tests/server/services/storage/sqlite_storage/, or skip them when storage is not a SQLiteStorage. Retain only the portable test_first_write_preserves_outcome_fields and test_exact_finalization_retry_is_idempotent cases in the contract file, removing backend-specific imports and casts there.reflexio/models/api_schema/domain/entities.py (1)
840-863: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe outcome-identity validation rule is implemented three times. Two Pydantic models and one dataclass each carry their own copy of the all-or-none identity check, and the two models also duplicate the SHA-256 digest format check. The shared root cause is that the rule has no single owning definition, so any change to the identity tuple must be applied in three files.
reflexio/models/api_schema/domain/entities.py#L840-L863: define a shared_OutcomeIdentityMixin(BaseModel)here that holds the four identity fields,validate_sha256_digest, andvalidate_identity_shape, then makeSessionOutcomeRecordinherit from it.reflexio/models/api_schema/domain/entities.py#L903-L938: remove the duplicated fields and both validators, and makeSetSessionOutcomeResponseinherit from the same mixin.reflexio/server/services/storage/storage_base/_session_outcomes.py#L27-L38: replace the inline tuple check in__post_init__with a call to one shared predicate exported alongside the mixin, so the dataclass and the models cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/models/api_schema/domain/entities.py` around lines 840 - 863, Centralize outcome-identity validation in a shared _OutcomeIdentityMixin and predicate. In reflexio/models/api_schema/domain/entities.py lines 840-863, place the four fields and both validators on the mixin, then have SessionOutcomeRecord inherit it; at lines 903-938, remove the duplicated fields and validators and make SetSessionOutcomeResponse inherit the mixin. In reflexio/server/services/storage/storage_base/_session_outcomes.py lines 27-38, replace __post_init__’s inline tuple check with the shared predicate.reflexio/server/services/storage/session_outcome_identity.py (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__.Ruff reports RUF022. Apply isort-style ordering.
♻️ Sorted `__all__`
__all__ = [ "CanonicalSessionTrajectory", - "canonical_session_trajectory", "canonical_json_bytes", + "canonical_session_trajectory", "outcome_contract_digest", "trajectory_digest", ]As per static analysis hints,
__all__is not sorted (RUF022).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/session_outcome_identity.py` around lines 11 - 17, Sort the exported names in __all__ using isort-style ordering to satisfy Ruff RUF022, while preserving the same symbols and their existing exports.Source: Linters/SAST tools
tests/models/test_session_outcome_identity.py (1)
77-135: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a case for a
Z-suffixed timestamp string.The parity test only covers the
+00:00text form and an awaredatetime. SQLite writes several timestamp columns withstrftime('%Y-%m-%dT%H:%M:%fZ','now'), which produces aZsuffix and milliseconds. That form is not covered here, and_canonical_timestamppasses text through unchanged. Add a third variant to pin the intended behavior. This relates to the comment onreflexio/server/services/storage/session_outcome_identity.pyLines 61-66.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/models/test_session_outcome_identity.py` around lines 77 - 135, Add a third timestamp variant in test_canonical_session_trajectory_normalizes_sqlite_and_postgres_rows using a Z-suffixed ISO string with milliseconds, and build its projection alongside the existing SQLite and Postgres cases. Assert it matches the canonical projection and preserves the existing digest expectation, covering normalization of SQLite’s strftime timestamp format.tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py (1)
108-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for a second migration pass.
_migrate_session_outcomes_schemarenames the table and then drops the legacy copy. The re-entry guard depends on a substring match for'unknown'in the storedCREATE TABLEtext. If that guard ever fails to match, the second startup rebuilds the table again. Open the database a third time and assert the rows andoutcome_idvalues are unchanged. This pins idempotency for a destructive migration.💚 Suggested addition
migrated = SQLiteStorage(org_id="legacy-session-outcomes", db_path=db_path) + migrated.conn.close() + migrated = SQLiteStorage(org_id="legacy-session-outcomes", db_path=db_path) records = migrated.get_session_outcomes( GetSessionOutcomesRequest(session_ids=["c", "b:c"]) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py` around lines 108 - 123, Add a third SQLiteStorage initialization in the migration test after the existing assertions to trigger a second migration pass, then re-read both session outcomes and assert the same session IDs, row contents, and outcome_id values remain unchanged, covering idempotency of _migrate_session_outcomes_schema.tests/server/services/storage/test_storage_contract_clear_user_data.py (1)
171-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThese two tests are backend-specific inside a contract suite.
Both tests assert
isinstance(sqlite_storage, SQLiteStorage)and then issue rawINSERT INTO session_outcomesSQL. The surrounding class tests the portableclear_user_datacontract with aBaseStorage-typed fixture. If the fixture is parameterized over another backend, these two tests fail on theisinstanceassertion instead of being skipped.Move them to a SQLite-specific module, or seed the rows through the portable
record_session_outcomeAPI so the tests run against every backend.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/test_storage_contract_clear_user_data.py` around lines 171 - 232, Make these contract tests backend-portable by removing the SQLiteStorage-specific assertions and raw session_outcomes INSERT statements, then seed equivalent records through the portable record_session_outcome API while preserving the authoritative-user deletion and zero-count expectations in test_session_outcomes_use_authoritative_user_and_report_stable_zero and test_default_clear_user_data_preserves_session_outcome_count.tests/server/services/storage/sqlite_storage/test_governance_storage.py (3)
1354-1354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the assertion message for consistency.
The two preceding assertions include
method_nameas the failure message. This one does not. A failure reports only the annotation set without naming the method.♻️ Proposed change
- assert parameter.annotation in {"PurgeExecutionClaim", PurgeExecutionClaim} + assert parameter.annotation in { + "PurgeExecutionClaim", + PurgeExecutionClaim, + }, method_name🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/sqlite_storage/test_governance_storage.py` at line 1354, Update the annotation assertion in the governance storage test to pass method_name as its failure message, matching the two preceding assertions and identifying the failing method.
1356-1359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the encoding when reading production sources.
path.read_text()uses the platform default encoding. CI and developer machines can differ. A source file with non-ASCII characters then raisesUnicodeDecodeErrorinstead of reporting a claim violation.Pass
encoding="utf-8".♻️ Proposed change
- tree = ast.parse(path.read_text(), filename=str(path)) + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/sqlite_storage/test_governance_storage.py` around lines 1356 - 1359, Update the production-source read in the AST scan loop around production_root and ast.parse to explicitly use UTF-8 encoding when calling path.read_text(), ensuring consistent parsing across platforms.
76-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe authoritative-user inference only covers two hard-coded user IDs.
_begin_test_purge_operationinfersauthoritative_user_idby testingsubject_refagainst"alice"and"bob"only. A future test that passes asubject_reffor any other user and omitsauthoritative_user_idgetsValueError: authoritative user identity is requiredfrombegin_purge_operation. The cause is the helper, not the test.Raise a clear error from the helper when inference fails.
♻️ Proposed change
subject_ref = kwargs.get("subject_ref") for user_id in ("alice", "bob"): if storage._subject_ref_for_user_id(user_id) == subject_ref: kwargs["authoritative_user_id"] = user_id break + else: + raise ValueError( + "pass authoritative_user_id explicitly; " + f"cannot infer it from subject_ref {subject_ref!r}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/sqlite_storage/test_governance_storage.py` around lines 76 - 87, Update _begin_test_purge_operation so that, after attempting inference for the user-erasure/user-scope case without authoritative_user_id, it raises a clear error when no matching user is found instead of delegating with a missing identity. Preserve the existing inference for alice and bob and the normal delegation path when authoritative_user_id is already provided.reflexio/server/services/governance/service.py (3)
122-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog heartbeat renewal loss in the background thread.
_runswallows_PurgeExecutionHeartbeatLostErrorand returns without any log record. The renewal error is stored and only surfaces if the main thread callsclaim()again. If the main thread is blocked in a long lifecycle call, the lease loss is invisible in logs.Add a warning log before returning.
♻️ Proposed change
def _run(self) -> None: while not self._stop.wait(_PURGE_EXECUTION_HEARTBEAT_SECONDS): try: self.renew_now() except _PurgeExecutionHeartbeatLostError: + logger.warning( + "purge execution heartbeat lost for purge_id=%s", self._purge_id + ) return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/governance/service.py` around lines 122 - 127, Update the exception handler in the background thread’s _run method to emit a warning log describing the _PurgeExecutionHeartbeatLostError before returning. Preserve the existing return behavior and use the service’s established logger.
311-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the two failure calls explicit about which one is expected to apply.
fail_subject_erasure_barrieralready sets the purge row tofailedand clearsexecution_claim_ownerandexecution_claim_expires_at. The followingfail_purge_operationthen re-validates the claim, finds statusfailedand no owner, and raises.suppress(Exception)hides that. The second call only does useful work when the first call failed, for example when no barrier row exists.The behavior is correct, but both outcomes are silent. Add a short comment that states the second call is the fallback for the no-barrier path, and log at debug level when a suppressed call fails. This prevents a future reader from removing either call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/governance/service.py` around lines 311 - 331, Clarify the two failure updates in the exception handler around fail_subject_erasure_barrier and fail_purge_operation: add a short comment identifying fail_purge_operation as the fallback when no barrier row exists, and emit debug-level logging whenever either suppressed call raises while preserving the existing suppression and call order.
390-421: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate
authoritative_user_digestin the retry fallback.
begin_purge_operationrejects mismatched digests, butPurgeOperationand_row_to_purge_operationdo not expose this column. Add the field or a storage accessor, then compare it here and remove the redundantgovernance_subject_ref(...)check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/governance/service.py` around lines 390 - 421, The retry fallback in _matching_user_erasure_purge_for_retry must validate authoritative_user_digest directly. Expose the stored digest through PurgeOperation or a storage accessor, compare it with the requested authoritative user’s digest, and remove the redundant governance_subject_ref check while preserving the existing identity-field validation.tests/server/services/governance/test_governance_local_e2e.py (1)
913-922: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPatching
governance_service_module.time.sleepmutates the shared stdlibtimemodule.
governance_service_module.timeis the same module object as the globaltimemodule.monkeypatch.setattr(governance_service_module.time, "sleep", ...)replacestime.sleepprocess-wide for the duration of the test, not just forGovernanceService.This test runs a winner thread concurrently. Any
time.sleepcall from that thread, fromsqlite3, or from any other library during the window runsrelease_winner_on_duplicate_poll. The same pattern appears at lines 1224-1231, 1275-1280, and 1334-1339, where the replacement raisesAssertionError. Those variants turn any unrelatedtime.sleepcall into a spurious test failure.Make the poll interval injectable, or wrap it in a service-owned helper that the test can patch on the service module itself.
♻️ Proposed approach
In
reflexio/server/services/governance/service.py:+def _duplicate_erase_sleep(seconds: float) -> None: + time.sleep(seconds)Call
_duplicate_erase_sleep(_DUPLICATE_ERASE_POLL_SECONDS)in the claim loop. Tests then patchgovernance_service_module._duplicate_erase_sleep, which affects only this code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/governance/test_governance_local_e2e.py` around lines 913 - 922, Stop patching governance_service_module.time.sleep in the duplicate-erase tests because it mutates the shared stdlib module. In the governance service claim loop, add a service-owned _duplicate_erase_sleep helper and call it with _DUPLICATE_ERASE_POLL_SECONDS; update the affected tests to patch governance_service_module._duplicate_erase_sleep instead, preserving each test’s release or assertion behavior without affecting unrelated sleeps.reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py (1)
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWidened
PurgeExecutionClaim | NoneMRO hints in two mixins. Both mixins declare_assert_purge_operation_execution_claim_lockedwith aPurgeExecutionClaim | Noneparameter, but the implementation inreflexio/server/services/storage/sqlite_storage/governance/_purge.pyline 370 declaresexecution_claim: PurgeExecutionClaim. The shared root cause is one copied structural hint that is looser than the method it describes, so a type checker acceptsNoneat call sites andvalidate_purge_execution_claimrejects it only at runtime.
reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py#L92-L96: change the declared type toCallable[[str, PurgeExecutionClaim], None].reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py#L80-L82: change the declared type toCallable[[str, PurgeExecutionClaim], None].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py` around lines 92 - 96, Align the structural type hints for _assert_purge_operation_execution_claim_locked with its implementation by replacing PurgeExecutionClaim | None with PurgeExecutionClaim in reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py#L92-L96 and reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py#L80-L82; no other behavior changes are needed.reflexio/server/services/storage/sqlite_storage/governance/_purge.py (2)
237-283: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap the read-then-insert in
begin_purge_operationinBEGIN IMMEDIATE.
begin_purge_operationreads the existing row byidempotency_keyat line 238, then inserts at line 263. Onlyself._lockguards the sequence. AnRLockprotects a single process against itself. It does not protect two processes, or twoSQLiteStorageinstances, that open the same database file.Two concurrent callers can both observe
existing is Noneand both attempt the insert.idx_purge_operations_org_idemthen makes the second insert raiseIntegrityErrorinstead of returning the existing operation idempotently.
GovernanceService.erase_userrecovers through_matching_user_erasure_purge_for_retry, so the service path degrades gracefully. Direct storage callers do not. Every other mutating method in this file now usesBEGIN IMMEDIATE; this one is the remaining exception.♻️ Proposed fix
now = _epoch_now() with self._lock: - existing = self.conn.execute( - """SELECT * FROM purge_operations - WHERE org_id = ? AND idempotency_key = ?""", - (self.org_id, validated_idempotency_key), - ).fetchone() - if existing is not None: + try: + self.conn.execute("BEGIN IMMEDIATE") + existing = self.conn.execute( + """SELECT * FROM purge_operations + WHERE org_id = ? AND idempotency_key = ?""", + (self.org_id, validated_idempotency_key), + ).fetchone() + if existing is not None: ... - return _row_to_purge_operation(existing) - self.conn.execute(...) - self.conn.commit() + self.conn.rollback() + return _row_to_purge_operation(existing) + self.conn.execute(...) + self.conn.commit() + except Exception: + self.conn.rollback() + raise return self.get_purge_operation(validated_purge_id)Keep the existing identity-mismatch
raisestatements inside thetry; theexcepthandler rolls back before re-raising.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py` around lines 237 - 283, Update begin_purge_operation to wrap the existing idempotency read-and-insert sequence inside a BEGIN IMMEDIATE transaction, matching the transaction pattern used by other mutating methods. Keep the identity validation and mismatch raises inside the transaction’s try block, and ensure the exception handler rolls back before re-raising; preserve returning the existing operation for matching retries.
346-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated claim-assertion body.
assert_purge_operation_execution_claim(lines 346-368) and_assert_purge_operation_execution_claim_locked(lines 370-394) contain identical validation logic. The only difference is the read helper:self._deps()._fetchone(...)versusself.conn.execute(...).fetchone().Two copies of a fencing predicate can diverge. A future change to the staleness condition applied to only one copy would let a stale worker pass one entry point and fail the other.
♻️ Proposed consolidation
def assert_purge_operation_execution_claim( self, purge_id: str, execution_claim: PurgeExecutionClaim ) -> None: purge_id = _validate_governance_purge_id("purge_id", purge_id) - claim = validate_purge_execution_claim(purge_id, execution_claim) - now = _epoch_now() row = self._deps()._fetchone( """SELECT status, execution_claim_owner, execution_claim_fence, execution_claim_expires_at FROM purge_operations WHERE purge_id = ? AND org_id = ?""", (purge_id, self.org_id), ) - if row is None: - raise ValueError(f"Purge operation {purge_id!r} not found") - if ( - row["status"] != "running" - or row["execution_claim_owner"] != claim.owner - or int(row["execution_claim_fence"]) != claim.fence - or row["execution_claim_expires_at"] is None - or int(row["execution_claim_expires_at"]) <= now - ): - raise ValueError("purge execution claim is no longer active") + self._assert_claim_row_active(purge_id, execution_claim, row) def _assert_purge_operation_execution_claim_locked( self, purge_id: str, execution_claim: PurgeExecutionClaim, ) -> None: purge_id = _validate_governance_purge_id("purge_id", purge_id) - claim = validate_purge_execution_claim(purge_id, execution_claim) - now = _epoch_now() row = self.conn.execute( """SELECT status, execution_claim_owner, execution_claim_fence, execution_claim_expires_at FROM purge_operations WHERE purge_id = ? AND org_id = ?""", (purge_id, self.org_id), ).fetchone() + self._assert_claim_row_active(purge_id, execution_claim, row) + + `@staticmethod` + def _assert_claim_row_active( + purge_id: str, + execution_claim: PurgeExecutionClaim, + row: sqlite3.Row | None, + ) -> None: + claim = validate_purge_execution_claim(purge_id, execution_claim) + now = _epoch_now() if row is None: raise ValueError(f"Purge operation {purge_id!r} not found") if ( row["status"] != "running" or row["execution_claim_owner"] != claim.owner or int(row["execution_claim_fence"]) != claim.fence or row["execution_claim_expires_at"] is None or int(row["execution_claim_expires_at"]) <= now ): raise ValueError("purge execution claim is no longer active")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py` around lines 346 - 394, Consolidate the duplicated validation in assert_purge_operation_execution_claim and _assert_purge_operation_execution_claim_locked by routing both methods through one shared claim-assertion implementation, while preserving each method’s appropriate row-fetch mechanism. Keep the purge ID and execution-claim validation, lookup errors, and active-claim predicate identical across both entry points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/billing_meter.py`:
- Around line 233-246: The docstring for the count-based wrapper incorrectly
uses "resumable_extraction" as the source example; update that example to an
online-extraction source while preserving the prohibition on using this wrapper
for resumable finalization and all other documentation.
In `@reflexio/server/services/base_generation/_usage_billing.py`:
- Around line 191-194: The billing path in
BaseGenerationService.compute_generation must use the count of learnings
retained by the resolved durable write plan, not the pre-deduplication
generated_count. Update the EMITS_LEARNING_BILLING emission to derive its value
after _resolve_write_plan() removes duplicates or invalid outputs, and add a
regression test covering extractor output eliminated during deduplication.
In `@reflexio/server/services/governance/service.py`:
- Around line 231-249: Update the duplicate-claim retry loop in the purge
execution flow around claim_purge_operation_execution to use a wall-clock
deadline and exponential backoff, defining the required deadline and backoff
constants alongside the existing timing constants. Stop retrying when the
deadline expires and raise a distinct timeout/error for callers to retry later;
retain the existing completed-status return and unsupported-status handling.
In `@reflexio/server/services/search_exposure.py`:
- Around line 89-95: The docstring for user_playbook_full_version_fingerprint
must document that adding or changing persisted UserPlaybook fields requires
bumping the "user-playbook-full-version-v1" schema_version, and that fingerprint
comparisons across schema versions are undefined.
In `@reflexio/server/services/storage/session_outcome_identity.py`:
- Around line 104-108: Update _canonical_json_column and the
canonical_json_bytes hashing path to handle floating-point values, including
nested floats, without raising TypeError. Normalize floats consistently before
canonicalization or extend the encoder, and ensure trajectory_digest
finalization remains successful for interactions containing float values.
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 968-1006: Update the legacy-row migration in the session outcomes
rebuild to read governance_subject_ref only when the column exists and its value
is non-null; otherwise derive it with
_subject_ref_for_user_id(str(row["user_id"])). Add a regression test covering a
legacy schema without governance_subject_ref and verify startup migration
completes with the derived reference.
In `@reflexio/server/services/storage/sqlite_storage/_session_outcomes.py`:
- Around line 26-28: The outcome allowed-value set is defined inconsistently
across the contract and digest paths. In
reflexio/server/services/storage/sqlite_storage/_session_outcomes.py:26-28,
derive and export _OUTCOME_ALLOWED_VALUES from SessionOutcomeKind; in
reflexio/server/services/storage/sqlite_storage/_base.py:990-997, import and
pass that shared constant to outcome_contract_digest instead of rebuilding the
values inline.
- Around line 117-133: Update the metadata comparison in the exact_retry
calculation to decode existing["metadata"] and compare the resulting structure
with the request metadata semantically, rather than comparing serialized text.
Preserve the existing retry checks and ensure migrated rows with different
separators or key ordering are recognized as identical.
- Around line 83-101: Update _migrate_request_session_id_required so the
requests table rebuild preserves governance_subject_ref: define the column on
requests_new and include it in the INSERT ... SELECT mapping. Ensure
init_governance_tables leaves the column available for subsequent add_request
and finalization queries.
In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py`:
- Around line 75-77: Update _authoritative_user_digest to derive the digest with
the existing governance HMAC mechanism and secret, matching
subject_refs._hmac_ref and get_governance_ref_secret() rather than unkeyed
SHA-256. Confirm how existing rows containing the old digest are handled before
merging, using a backfill or versioned digest strategy if required.
- Around line 258-262: Update begin_purge_operation’s existing-row handling to
support legacy rows with a NULL authoritative_user_digest: validate the current
authoritative identity, then safely backfill or adopt the digest before
returning the existing operation. Preserve idempotent reuse when validation
succeeds and retain the current ValueError fail-closed behavior when identity
validation is unavailable or mismatched; add tests covering both recovery and
rejection paths.
In
`@reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py`:
- Around line 166-175: Replace the full-table scan in
_authoritative_user_rows_remain_locked with a user_id-filtered existence query,
passing the resolved authoritative user_id from
_assert_bound_authoritative_user_identity_locked through
_same_subject_rows_remain_locked; confirm session_outcomes.user_id has an index
and add one if needed. In the related check around the legacy_request_ids
handling, move the expensive authoritative lookup after the early return so
legacy requests short-circuit first.
In `@tests/server/services/governance/test_governance_local_e2e.py`:
- Around line 965-970: Remove raising=False from the monkeypatch.setattr calls
targeting _PURGE_EXECUTION_HEARTBEAT_SECONDS in the heartbeat-interval patches,
including both occurrences in this test, so renamed or missing attributes fail
immediately.
In `@tests/server/services/governance/test_subject_write_barrier_sqlite.py`:
- Around line 390-397: Update the three affected
tests—test_begin_subject_erasure_barrier_preserves_terminal_erased_state,
test_fail_subject_erasure_barrier_rejects_terminal_erased_state, and
test_fail_purge_operation_rejects_terminal_complete_state—to either reach and
verify their intended terminal-state branches through a valid setup or rename
them and assert only unvalidated-claim rejection. Keep the production
terminal-state guards unchanged unless you confirm they are intentionally
unreachable and remove them accordingly.
---
Nitpick comments:
In `@reflexio/client/client.py`:
- Around line 1115-1129: Update the docstring for the session-outcome recording
method to document that mark_session_outcome propagates ValidationError when
SetSessionOutcomeResponse contains only a partial subset of its four identity
fields, including the compatibility failure with older servers.
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 840-863: Centralize outcome-identity validation in a shared
_OutcomeIdentityMixin and predicate. In
reflexio/models/api_schema/domain/entities.py lines 840-863, place the four
fields and both validators on the mixin, then have SessionOutcomeRecord inherit
it; at lines 903-938, remove the duplicated fields and validators and make
SetSessionOutcomeResponse inherit the mixin. In
reflexio/server/services/storage/storage_base/_session_outcomes.py lines 27-38,
replace __post_init__’s inline tuple check with the shared predicate.
In `@reflexio/server/routes/search.py`:
- Around line 426-435: Wrap the synchronous record_search_exposures call in the
endpoint’s existing profile_step mechanism using the span name
search.record_search_exposures, while preserving the current SearchExposureBatch
construction and response.user_playbooks tuple.
In `@reflexio/server/services/governance/service.py`:
- Around line 122-127: Update the exception handler in the background thread’s
_run method to emit a warning log describing the
_PurgeExecutionHeartbeatLostError before returning. Preserve the existing return
behavior and use the service’s established logger.
- Around line 311-331: Clarify the two failure updates in the exception handler
around fail_subject_erasure_barrier and fail_purge_operation: add a short
comment identifying fail_purge_operation as the fallback when no barrier row
exists, and emit debug-level logging whenever either suppressed call raises
while preserving the existing suppression and call order.
- Around line 390-421: The retry fallback in
_matching_user_erasure_purge_for_retry must validate authoritative_user_digest
directly. Expose the stored digest through PurgeOperation or a storage accessor,
compare it with the requested authoritative user’s digest, and remove the
redundant governance_subject_ref check while preserving the existing
identity-field validation.
In `@reflexio/server/services/search_exposure.py`:
- Around line 82-86: Update record_search_exposures to enforce a defined timeout
budget and fast-fail behavior around the synchronous recorder.record call,
preserving the existing no-recorder path and failing-closed semantics. Add the
corresponding latency-bound requirement to the SearchExposureRecorder protocol
docstring, using the existing timeout/error-handling conventions where
available.
In `@reflexio/server/services/storage/session_outcome_identity.py`:
- Around line 11-17: Sort the exported names in __all__ using isort-style
ordering to satisfy Ruff RUF022, while preserving the same symbols and their
existing exports.
In
`@reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py`:
- Around line 92-96: Align the structural type hints for
_assert_purge_operation_execution_claim_locked with its implementation by
replacing PurgeExecutionClaim | None with PurgeExecutionClaim in
reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py#L92-L96
and
reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py#L80-L82;
no other behavior changes are needed.
In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py`:
- Around line 237-283: Update begin_purge_operation to wrap the existing
idempotency read-and-insert sequence inside a BEGIN IMMEDIATE transaction,
matching the transaction pattern used by other mutating methods. Keep the
identity validation and mismatch raises inside the transaction’s try block, and
ensure the exception handler rolls back before re-raising; preserve returning
the existing operation for matching retries.
- Around line 346-394: Consolidate the duplicated validation in
assert_purge_operation_execution_claim and
_assert_purge_operation_execution_claim_locked by routing both methods through
one shared claim-assertion implementation, while preserving each method’s
appropriate row-fetch mechanism. Keep the purge ID and execution-claim
validation, lookup errors, and active-claim predicate identical across both
entry points.
In `@tests/models/test_session_outcome_identity.py`:
- Around line 77-135: Add a third timestamp variant in
test_canonical_session_trajectory_normalizes_sqlite_and_postgres_rows using a
Z-suffixed ISO string with milliseconds, and build its projection alongside the
existing SQLite and Postgres cases. Assert it matches the canonical projection
and preserves the existing digest expectation, covering normalization of
SQLite’s strftime timestamp format.
In `@tests/server/services/governance/test_governance_local_e2e.py`:
- Around line 913-922: Stop patching governance_service_module.time.sleep in the
duplicate-erase tests because it mutates the shared stdlib module. In the
governance service claim loop, add a service-owned _duplicate_erase_sleep helper
and call it with _DUPLICATE_ERASE_POLL_SECONDS; update the affected tests to
patch governance_service_module._duplicate_erase_sleep instead, preserving each
test’s release or assertion behavior without affecting unrelated sleeps.
In `@tests/server/services/storage/sqlite_storage/test_governance_storage.py`:
- Line 1354: Update the annotation assertion in the governance storage test to
pass method_name as its failure message, matching the two preceding assertions
and identifying the failing method.
- Around line 1356-1359: Update the production-source read in the AST scan loop
around production_root and ast.parse to explicitly use UTF-8 encoding when
calling path.read_text(), ensuring consistent parsing across platforms.
- Around line 76-87: Update _begin_test_purge_operation so that, after
attempting inference for the user-erasure/user-scope case without
authoritative_user_id, it raises a clear error when no matching user is found
instead of delegating with a missing identity. Preserve the existing inference
for alice and bob and the normal delegation path when authoritative_user_id is
already provided.
In
`@tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py`:
- Around line 108-123: Add a third SQLiteStorage initialization in the migration
test after the existing assertions to trigger a second migration pass, then
re-read both session outcomes and assert the same session IDs, row contents, and
outcome_id values remain unchanged, covering idempotency of
_migrate_session_outcomes_schema.
In `@tests/server/services/storage/test_storage_contract_clear_user_data.py`:
- Around line 171-232: Make these contract tests backend-portable by removing
the SQLiteStorage-specific assertions and raw session_outcomes INSERT
statements, then seed equivalent records through the portable
record_session_outcome API while preserving the authoritative-user deletion and
zero-count expectations in
test_session_outcomes_use_authoritative_user_and_report_stable_zero and
test_default_clear_user_data_preserves_session_outcome_count.
In `@tests/server/services/storage/test_storage_contract_session_outcomes.py`:
- Around line 124-140: Extract the repeated legacy-schema downgrade sequence
into a helper such as _downgrade_to_legacy_identity, accepting SQLiteStorage and
session_id, and move the table recreation, identity-column nulling, and commit
statements into it. Replace the three duplicated blocks in the affected tests
with calls to this helper, passing each test’s session identifier.
- Around line 3-15: Separate the SQLite-specific tests from the backend-agnostic
contract suite: move tests that use SQLiteStorage, _canonical_session_snapshot,
raw SQL, or sqlite3 tracing into tests/server/services/storage/sqlite_storage/,
or skip them when storage is not a SQLiteStorage. Retain only the portable
test_first_write_preserves_outcome_fields and
test_exact_finalization_retry_is_idempotent cases in the contract file, removing
backend-specific imports and casts there.
In `@tests/server/services/test_search_exposure.py`:
- Around line 46-62: Add a test case alongside the existing normalization tests
for the _batch helper using interaction_id=0; assert SearchExposureBatch
normalizes interaction_id to None and that exposure_event_id reflects the
resulting invocation_id fallback identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3bb1590-5619-43d8-8d8a-e7164005d2ce
📒 Files selected for processing (42)
reflexio/client/client.pyreflexio/lib/_session_outcome.pyreflexio/models/api_schema/domain/entities.pyreflexio/models/api_schema/domain/enums.pyreflexio/server/billing_meter.pyreflexio/server/routes/search.pyreflexio/server/services/base_generation/_usage_billing.pyreflexio/server/services/base_generation_service.pyreflexio/server/services/extraction/resume_worker.pyreflexio/server/services/governance/service.pyreflexio/server/services/playbook/components/aggregator.pyreflexio/server/services/search_exposure.pyreflexio/server/services/storage/governance_claims.pyreflexio/server/services/storage/governance_validation.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/session_outcome_identity.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_governance.pyreflexio/server/services/storage/sqlite_storage/_session_outcomes.pyreflexio/server/services/storage/sqlite_storage/governance/_erase_execution.pyreflexio/server/services/storage/sqlite_storage/governance/_purge.pyreflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_session_outcomes.pyreflexio/server/services/storage/storage_base/governance/_erase_execution.pyreflexio/server/services/storage/storage_base/governance/_purge.pyreflexio/server/services/storage/storage_base/governance/_subject_barrier.pytests/client/test_session_outcomes_client.pytests/models/test_session_outcome_identity.pytests/server/api_endpoints/test_session_outcomes_integration.pytests/server/routes/test_search_exposure_boundary.pytests/server/services/extraction/test_resume_worker.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/governance/test_subject_write_barrier_sqlite.pytests/server/services/storage/sqlite_storage/test_governance_retrieved_learning.pytests/server/services/storage/sqlite_storage/test_governance_storage.pytests/server/services/storage/sqlite_storage/test_session_outcome_migration.pytests/server/services/storage/test_storage_contract_clear_user_data.pytests/server/services/storage/test_storage_contract_session_outcomes.pytests/server/services/test_non_extraction_learning_metering.pytests/server/services/test_search_exposure.pytests/server/test_billing_meter_events.py
Address review findings across extraction billing, resumable finalization, governance erasure, and session-outcome migration. Persist finalization receipts atomically so retries reuse durable learning IDs.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/storage/sqlite_storage/governance/_purge.py (1)
405-434: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDeclare or validate the minimum SQLite version.
The project declares Python
>=3.12but no SQLite minimum or version check. If the linked SQLite library is older than3.35.0, bothRETURNINGstatements raisesqlite3.OperationalError. Add a supported SQLite requirement or fail duringSQLiteStorageBaseinitialization with a clear error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py` around lines 405 - 434, Ensure SQLiteStorageBase initialization validates that the linked SQLite library is at least version 3.35.0, or declare this minimum as a supported project requirement. When the version is too old, fail immediately with a clear error identifying the required SQLite version, before execution reaches the RETURNING statements such as the purge-operation claim query.
🧹 Nitpick comments (7)
reflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.py (1)
250-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReceipt write ordering and fencing look correct.
Validation runs before the insert, ownership and entity-type are checked against
_agent_runs, and the commit is skipped inside an enclosingcommit_scope. The read-back comparison makes a conflicting rewrite fail loudly.One note: the equality check
json.loads(stored["learning_ids"]) != learning_idsis order-sensitive. A concurrent loser that produced the same IDs in a different order raises "immutable" instead of returning. The receipt read short-circuit makes this unlikely in the current callers, so no change is required now.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.py` around lines 250 - 302, No code change is required; preserve the current order-sensitive comparison in save_agent_run_finalization_receipt and its existing immutable-conflict behavior.reflexio/server/services/base_generation_service.py (2)
345-354: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe base implementation silently ignores
finalization_run_id.The base accepts
finalization_run_idand discards it withnoqa: ARG002. A subclass that does not override this method therefore gets non-idempotent finalization with no signal, even when the caller supplies a run ID. OnlyProfileGenerationServiceandPlaybookGenerationServicehonour the receipt.If a run ID is passed to a service without receipt support, that is a programming error. Consider raising
NotImplementedErrorwhenfinalization_run_id is not None, so the gap fails loudly instead of double-persisting on a retry.♻️ Proposed change
def _finalize_extracted_items( self, items: list, *, - finalization_run_id: str | None = None, # noqa: ARG002 + finalization_run_id: str | None = None, ) -> list[str] | None: """Persist already-flattened extracted items through the service path.""" + if finalization_run_id is not None: + raise NotImplementedError( + f"{type(self).__name__} does not support run-scoped finalization receipts" + ) if items: self._process_results([items]) return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/base_generation_service.py` around lines 345 - 354, Update _finalize_extracted_items to raise NotImplementedError whenever finalization_run_id is not None, before processing items; retain the existing _process_results behavior when no run ID is supplied, and remove the now-unnecessary ARG002 suppression.
650-655: 📐 Maintainability & Code Quality | 🔵 Trivial
generated_countnow carries two different meanings.For services with
EMITS_LEARNING_BILLING,generated_countis the retained write-plan count. For all other services it stays the raw extractor result count. This value is correct for billing, but it also feeds thegeneration_succeededtelemetrycount_valueat Line 709.The metric therefore means "retained learnings" for profile and playbook and "generated results" elsewhere. Dashboards that compare the two will read a drop for profile and playbook that is not a real regression. Consider carrying a separate
billable_countonGenerationComputePlanand leaving the telemetry count unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/base_generation_service.py` around lines 650 - 655, The generated_count assignment in the generation computation flow conflates billing and telemetry semantics. Preserve _count_generated_results(result) for generation_succeeded telemetry, add a separate billable_count to GenerationComputePlan, and use _count_retained_online_learnings(write_plan) only for billing when EMITS_LEARNING_BILLING is enabled.reflexio/server/services/profile/service.py (2)
350-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared finalization-receipt protocol. Both services implement the same four steps by hand: read the receipt before the scope, re-read it inside
commit_scope, persist the write plan, then save the receipt. The duplication is the root cause: a future change to the protocol, such as adding scheduler-dispatch state to the receipt, must be applied twice and can silently diverge.
reflexio/server/services/profile/service.py#L350-L389: move the read-check-persist-save sequence into a shared helper onBaseGenerationServicethat takesentity_type, the plan, and a callback producing the learning IDs.reflexio/server/services/playbook/service.py#L638-L685: call the same helper, then dispatch the schedulers after it returns.The helper also gives one place to encode the rule that the ID list is computed before persist for profiles and after persist for playbooks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/profile/service.py` around lines 350 - 389, The duplicated finalization-receipt protocol must be centralized. In reflexio/server/services/profile/service.py lines 350-389, add and use a BaseGenerationService helper accepting entity_type, the write plan, and a learning-ID callback; preserve profile IDs being computed before persistence. In reflexio/server/services/playbook/service.py lines 638-685, replace the inline protocol with the same helper, preserve post-persistence ID computation, and dispatch schedulers only after the helper returns.
360-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the profile IDs are computed before persist.
ProfileGenerationServicebuildslearning_idsfrom the plan before_persist_write_planruns, butPlaybookGenerationServicebuilds its list after persist. The reason is thatprofile_idis assigned during extraction, whileuser_playbook_idis assigned bysave_user_playbooks. Add a one-line comment here so a later refactor does not move this computation and break the playbook symmetry assumption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/profile/service.py` around lines 360 - 373, In ProfileGenerationService, add a concise one-line comment immediately before the learning_ids computation explaining that profile_id is assigned during extraction, so IDs must be collected before _persist_write_plan; preserve the existing ordering and logic.tests/server/services/test_non_extraction_learning_metering.py (1)
55-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the global usage-event recorder after each test.
configure_usage_event_recordersets a process-global sink. These tests install it and never restoreNone. The recorder stays active for every test that runs afterwards in the same session, so unrelated tests append into a list they do not own.tests/server/services/test_generation_billing_emission.pyguards this withtry/finally.Add an autouse fixture in this module so the reset cannot be forgotten.
♻️ Proposed fixture
`@pytest.fixture`(autouse=True) def _reset_usage_event_recorder(): yield configure_usage_event_recorder(None)Also applies to: 135-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/test_non_extraction_learning_metering.py` around lines 55 - 72, Add an autouse pytest fixture in the test module that yields for each test and then calls configure_usage_event_recorder(None) to clear the process-global sink. Ensure this cleanup covers all tests in the module, including the test around test_resumable_profile_bills_only_ids_returned_by_finalization and the additional referenced test range.tests/server/services/storage/sqlite_storage/test_agent_run_storage.py (1)
199-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two entity-type guards.
The suite does not exercise
get_agent_run_finalization_receiptraising "entity type changed" (Line 243 ofreflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.py), norsave_agent_run_finalization_receiptraising "entity type is invalid" for an extractor-kind mismatch (Line 274). Both guards protect the receipt from being read or written under the wrong entity type. Add one test for each.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/sqlite_storage/test_agent_run_storage.py` around lines 199 - 216, Add two tests alongside test_finalization_receipt_rejects_conflicting_immutable_value: one must verify get_agent_run_finalization_receipt raises StorageError matching “entity type changed” when the stored receipt is requested with a different entity type, and the other must verify save_agent_run_finalization_receipt raises StorageError matching “entity type is invalid” for an extractor-kind mismatch. Use the existing agent-run and receipt setup/helpers, and assert the guarded operation raises without changing the valid receipt behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/services/playbook/service.py`:
- Around line 638-645: Update the receipt short-circuit in the playbook
finalization flow so an existing receipt cannot skip required scheduler
dispatch. Ensure the receipt records or otherwise preserves dispatch state, and
on the receipt path invoke _dispatch_playbook_schedulers when dispatch has not
completed, relying on the documented idempotency of both schedulers; keep
already-dispatched retries from enqueuing duplicate work.
In `@tests/server/services/extraction/test_resume_worker.py`:
- Around line 726-727: Add an inline Ruff noqa suppression for BLE001 to the
intentional BaseException handler in the worker thread, preserving the existing
errors.append(exc) behavior and documenting that the broad catch is deliberate.
---
Outside diff comments:
In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py`:
- Around line 405-434: Ensure SQLiteStorageBase initialization validates that
the linked SQLite library is at least version 3.35.0, or declare this minimum as
a supported project requirement. When the version is too old, fail immediately
with a clear error identifying the required SQLite version, before execution
reaches the RETURNING statements such as the purge-operation claim query.
---
Nitpick comments:
In `@reflexio/server/services/base_generation_service.py`:
- Around line 345-354: Update _finalize_extracted_items to raise
NotImplementedError whenever finalization_run_id is not None, before processing
items; retain the existing _process_results behavior when no run ID is supplied,
and remove the now-unnecessary ARG002 suppression.
- Around line 650-655: The generated_count assignment in the generation
computation flow conflates billing and telemetry semantics. Preserve
_count_generated_results(result) for generation_succeeded telemetry, add a
separate billable_count to GenerationComputePlan, and use
_count_retained_online_learnings(write_plan) only for billing when
EMITS_LEARNING_BILLING is enabled.
In `@reflexio/server/services/profile/service.py`:
- Around line 350-389: The duplicated finalization-receipt protocol must be
centralized. In reflexio/server/services/profile/service.py lines 350-389, add
and use a BaseGenerationService helper accepting entity_type, the write plan,
and a learning-ID callback; preserve profile IDs being computed before
persistence. In reflexio/server/services/playbook/service.py lines 638-685,
replace the inline protocol with the same helper, preserve post-persistence ID
computation, and dispatch schedulers only after the helper returns.
- Around line 360-373: In ProfileGenerationService, add a concise one-line
comment immediately before the learning_ids computation explaining that
profile_id is assigned during extraction, so IDs must be collected before
_persist_write_plan; preserve the existing ordering and logic.
In
`@reflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.py`:
- Around line 250-302: No code change is required; preserve the current
order-sensitive comparison in save_agent_run_finalization_receipt and its
existing immutable-conflict behavior.
In `@tests/server/services/storage/sqlite_storage/test_agent_run_storage.py`:
- Around line 199-216: Add two tests alongside
test_finalization_receipt_rejects_conflicting_immutable_value: one must verify
get_agent_run_finalization_receipt raises StorageError matching “entity type
changed” when the stored receipt is requested with a different entity type, and
the other must verify save_agent_run_finalization_receipt raises StorageError
matching “entity type is invalid” for an extractor-kind mismatch. Use the
existing agent-run and receipt setup/helpers, and assert the guarded operation
raises without changing the valid receipt behavior.
In `@tests/server/services/test_non_extraction_learning_metering.py`:
- Around line 55-72: Add an autouse pytest fixture in the test module that
yields for each test and then calls configure_usage_event_recorder(None) to
clear the process-global sink. Ensure this cleanup covers all tests in the
module, including the test around
test_resumable_profile_bills_only_ids_returned_by_finalization and the
additional referenced test range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf8cacd8-cddc-4211-91e2-771495ebca50
📒 Files selected for processing (30)
reflexio/server/billing_meter.pyreflexio/server/services/base_generation/_usage_billing.pyreflexio/server/services/base_generation_service.pyreflexio/server/services/extraction/README.mdreflexio/server/services/extraction/resume_worker.pyreflexio/server/services/governance/service.pyreflexio/server/services/playbook/service.pyreflexio/server/services/profile/service.pyreflexio/server/services/search_exposure.pyreflexio/server/services/storage/session_outcome_identity.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_session_outcomes.pyreflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.pyreflexio/server/services/storage/sqlite_storage/governance/_erase_execution.pyreflexio/server/services/storage/sqlite_storage/governance/_purge.pyreflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.pyreflexio/server/services/storage/storage_base/agent_run/_agent_run_store.pyreflexio/server/services/storage/storage_base/governance/_erase_execution.pyreflexio/server/services/storage/storage_base/governance/_subject_barrier.pytests/models/test_session_outcome_identity.pytests/server/api_endpoints/test_session_outcomes_integration.pytests/server/services/extraction/test_resume_worker.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/governance/test_subject_write_barrier_sqlite.pytests/server/services/storage/sqlite_storage/test_agent_run_storage.pytests/server/services/storage/sqlite_storage/test_governance_storage.pytests/server/services/storage/sqlite_storage/test_session_id_migration.pytests/server/services/storage/sqlite_storage/test_session_outcome_migration.pytests/server/services/test_generation_billing_emission.pytests/server/services/test_non_extraction_learning_metering.py
🚧 Files skipped from review as they are similar to previous changes (7)
- reflexio/server/billing_meter.py
- reflexio/server/services/base_generation/_usage_billing.py
- reflexio/server/services/storage/storage_base/governance/_erase_execution.py
- reflexio/server/services/storage/storage_base/governance/_subject_barrier.py
- tests/models/test_session_outcome_identity.py
- reflexio/server/services/search_exposure.py
- reflexio/server/services/governance/service.py
Keep derived scheduler delivery best-effort and at-most-once, separate generation telemetry from billable survivors, and fail fast on unsupported SQLite runtimes.
Contributor
Author
|
CodeRabbit review-body disposition after a3bec76:\n\nImplemented and verified:\n- SQLite >= 3.35.0 fail-fast guard, boundary tests, and README prerequisite.\n- Base finalization now fails loudly when a run-scoped receipt ID reaches a service without receipt support.\n- Raw generation telemetry is separate from retained billable survivor count (regression proves telemetry=2, billing=1).\n- Profile pre-persist ID timing is documented.\n- Receipt entity-type read/write guards have direct immutable-state regressions.\n- The module-global usage-event recorder teardown was already present and was reverified.\n- Session outcome explorer now includes unknown and accurately documents canonical retry/conflict context.\n- Intentional BaseException capture has a narrow BLE001 suppression.\n\nExplicitly not adopted:\n- Receipt retries do not redispatch optimization/aggregation. Owner selected best-effort, at-most-once derived scheduling because current schedulers lack durable cross-process idempotency; replay can create duplicate UUID jobs/derived work. The policy is documented and tested.\n- The two receipt flows are not centralized into a new helper. Profile IDs exist before persistence, playbook IDs after persistence, and playbook has post-commit scheduler behavior; a shared abstraction would hide these domain differences without reducing correctness risk.\n- Receipt ID order remains significant, as the review itself recommends.\n\nThe first-pass review-body items labeled trivial/low-value/poor-tradeoff were also triaged. Correctness-overlapping items were fixed in d3d919f (migration re-entry, service-owned sleep helper, annotations, identity validation, and governance transaction behavior). Remaining optional instrumentation, test-file movement/helper extraction, logging polish, and model-mixin refactors are deferred as P5 scope expansion with no open correctness finding.\n\nLocal gate for this follow-up: 656 passed, 3 skipped; changed-file Ruff/format/Pyright and diff check are clean. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
reflexio/server/services/deferred_learning_plan.py (1)
137-142: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the snapshot contract with the implementation.
BaseGenerationService.emit_generation_side_effectsstill readsself._last_token_totals,self._last_precheck_sessions, andself._last_extractor_run_stats(reflexio/server/services/base_generation_service.py, Lines 690-729). This paragraph says the emitter reads the plan instead of mutable state. Either pass these values throughGenerationComputePlan, or state that the snapshot is partial and depends on the single-use-instance invariant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/deferred_learning_plan.py` around lines 137 - 142, Update the snapshot-contract documentation around emit_generation_side_effects to acknowledge that token totals, precheck sessions, and extractor run stats are still read from the service’s mutable _last_* fields, or extend GenerationComputePlan and the emitter to pass those values explicitly. Ensure the documented purity and single-use-instance guarantees match the chosen implementation.tests/server/services/storage/test_sqlite_storage.py (1)
64-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert connection cleanup on rejection.
The test verifies
RuntimeError, but it does not verify that the opened connection is closed. The constructor opensself.connbefore the version check. Patchsqlite_storage_base.sqlite3.connectand assertclose()after thepytest.raisesblock. This catches a connection leak if the cleanup regresses.Proposed test assertion
with ( + patch.object(sqlite_storage_base.sqlite3, "connect") as connect, patch.object(SQLiteStorage, "_get_embedding", return_value=[0.0] * 512), pytest.raises(...), ): SQLiteStorage(...) + connect.return_value.close.assert_called_once_with()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/services/storage/test_sqlite_storage.py` around lines 64 - 71, Update the version-rejection test around SQLiteStorage initialization to patch sqlite_storage_base.sqlite3.connect, retain the returned connection mock, and assert its close() method is called after the pytest.raises block. Keep the existing RuntimeError and version-message assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 107: Update the SQLite runtime prerequisite near the existing README
table entry to explicitly require Python’s sqlite3 library version 3.35.0 or
newer, not merely the standalone SQLite executable. Add the provided Python
command as the verification method, preserving the existing documentation
structure.
In `@tests/server/services/test_generation_billing_emission.py`:
- Line 516: Update the pytest.raises call’s match argument in the relevant
billing emission test to use a raw regex string for the case-insensitive
“receipt-aware” pattern, resolving Ruff RUF043 without changing the expected
exception or message.
---
Nitpick comments:
In `@reflexio/server/services/deferred_learning_plan.py`:
- Around line 137-142: Update the snapshot-contract documentation around
emit_generation_side_effects to acknowledge that token totals, precheck
sessions, and extractor run stats are still read from the service’s mutable
_last_* fields, or extend GenerationComputePlan and the emitter to pass those
values explicitly. Ensure the documented purity and single-use-instance
guarantees match the chosen implementation.
In `@tests/server/services/storage/test_sqlite_storage.py`:
- Around line 64-71: Update the version-rejection test around SQLiteStorage
initialization to patch sqlite_storage_base.sqlite3.connect, retain the returned
connection mock, and assert its close() method is called after the pytest.raises
block. Keep the existing RuntimeError and version-message assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70e71306-5159-4524-b137-4ea8d42c6f91
📒 Files selected for processing (11)
README.mdreflexio/server/services/base_generation/_usage_billing.pyreflexio/server/services/base_generation_service.pyreflexio/server/services/deferred_learning_plan.pyreflexio/server/services/playbook/service.pyreflexio/server/services/profile/service.pyreflexio/server/services/storage/sqlite_storage/_base.pytests/server/services/extraction/test_resume_worker.pytests/server/services/storage/sqlite_storage/test_agent_run_storage.pytests/server/services/storage/test_sqlite_storage.pytests/server/services/test_generation_billing_emission.py
🚧 Files skipped from review as they are similar to previous changes (7)
- reflexio/server/services/profile/service.py
- tests/server/services/extraction/test_resume_worker.py
- reflexio/server/services/playbook/service.py
- reflexio/server/services/base_generation/_usage_billing.py
- tests/server/services/storage/sqlite_storage/test_agent_run_storage.py
- reflexio/server/services/base_generation_service.py
- reflexio/server/services/storage/sqlite_storage/_base.py
yyiilluu
added a commit
that referenced
this pull request
Aug 3, 2026
## Summary - Replace per-generation, full-corpus playbook aggregation with a durable signal and bounded scheduled work. - Make newly armed work eligible immediately; apply the configured one-hour minimum only after a version drains successfully. - Keep automatic runs efficient beyond `REFLEXIO_MAX_CLUSTERING_PLAYBOOKS` by processing a bounded row budget rather than treating it as a corpus ceiling. - Match new playbooks only to compatible centroids for the same agent version, then cluster unmatched residuals without rereading or reclustering the full corpus. ## Changes ### Scheduling and execution - Convert the post-generation trigger into an idempotent durable scheduling write. - Add per-organization claims, database-time leases, fencing, retry handling, and backlog-aware continuation. - Continue promptly while backlog remains; after a drained success, wait at least `REFLEXIO_AGGREGATION_MIN_INTERVAL_SECONDS` (default: one hour). - Keep the administrative full-rerun path capped and make it honor the same configured minimum interval. ### Incremental aggregation - Add bounded intake, same-version nearest-centroid attachment, residual clustering, stable cluster identity, and centroid maintenance. - Use agglomerative clustering for residual batches below 50 rows and HDBSCAN for larger batches. - Treat `REFLEXIO_MAX_CLUSTERING_PLAYBOOKS` (default: 20,000) as the maximum rows admitted to one scheduled unit of work, including intake and invalidation repair; it is also the fail-before-mutation cap for an administrative full rerun. - Distinguish generated, semantic-null, retryable-failure, and missing-embedding outcomes so healthy effects commit while only unfinished members remain pending. - Reuse shared prompt context per batch and avoid repeated full-corpus reads or quadratic prompt grouping. ### Durable storage - Add backend-neutral contracts for claims, backlog discovery, clusters, item dispositions, lifecycle invalidations, and atomic effects. - Implement the contracts for SQLite, including vector-index dirty repair and pre-delete invalidation capture. - Capture archive, revise, merge, status, purge, and delete changes without allowing stale cluster membership to survive. - Group SQLite merge invalidations by source agent version so mixed-version merges arm every affected version. - Filter SQLite ANN candidates for model, dimension, version, and active state before applying the nearest-neighbor limit. - Preserve the pending-invalidation partial index across repeated SQLite storage initialization instead of dropping and recreating it.\n- Skip empty legacy fingerprints so they cannot become active clusters without a centroid or vector-index row. ### Documentation and compatibility - Rewrite the playbook and server READMEs as code maps for the scheduler, storage contracts, cadence, budget semantics, version isolation, failure dispositions, and extension points. - Isolate the durable-learning transaction regression test from the local scheduler thread so the test measures the transaction boundary deterministically. - Rebase on current OSS main through receipt-finalization PR #408 while preserving the invariant from #407 that aggregation does not create a second learning charge. - Calculate finite profile TTLs in UTC so crossing a local daylight-saving transition does not add or subtract an hour. ## Flow ```mermaid flowchart LR A["User playbook committed"] --> B["Durable state due now"] B --> C["Scheduler claim"] C --> D["Bounded intake for one agent version"] D --> E{"Compatible centroid match?"} E -->|Yes| F["Attach and update centroid"] E -->|No| G["Durable residual"] G --> H["Bounded residual clustering"] H --> I["Generate agent playbook"] F --> J["Atomic fenced commit"] I --> J J --> K{"Backlog remains?"} K -->|Yes| C K -->|No| L["One-hour minimum before next drained run"] ``` ## Test Plan - `uv run ruff check` and `uv run ruff format --check` passed across the complete OSS source and test tree (879 files). - Pyright on all changed Python paths: 0 errors. - CodeRabbit regression coverage: 40 focused storage/profile tests passed, including mixed-version merge invalidation, repeated initialization, ANN limit/version isolation, empty legacy-cluster adoption, and guaranteed timezone restoration. - Exact OSS CI unit command: 4,401 passed, 9 skipped, 6 subtests passed. - OSS E2E suite: 47 passed, 51 skipped. - A production-like companion self-host/native-Postgres run completed all 17 launch phases against an isolated database and real MiniMax LLM calls: 16 phases passed. Aggregation scheduling, profile/playbook generation, aggregation, search, cleanup, and evaluation all passed. The resumable-extraction phase completed its suspend/resume lifecycle but its evidence reviewer rejected the generated candidate, so no durable playbook was saved; this was reproduced twice on the companion checkout's currently pinned pre-evidence revision and is not counted as a pass. - The live run did not exercise the four SQLite-only CodeRabbit fixes; those are covered by the focused SQLite regression suite above. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added durable, incremental playbook aggregation with scheduling, retries, progress tracking, and safe coordination. * Playbook updates now trigger bounded, resumable, version-scoped aggregation with invalidation handling and full-rerun support. * Added configurable minimum aggregation intervals and automatic local scheduling where supported. * Improved clustering with bounded processing, embedding-based matching, stable results, and safer large-cluster handling. * **Documentation** * Updated storage and aggregation documentation for durable scheduling and incremental processing. * **Bug Fixes** * Improved timestamp handling for profile expiration across timezone and daylight-saving changes. * **Tests** * Expanded coverage for scheduling, retries, invalidations, clustering, reruns, and failure handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Aug 3, 2026
yyiilluu
added a commit
that referenced
this pull request
Aug 4, 2026
Preserve session-outcome compatibility across the #407 downgrade, make governance erasure and transactions robust, and keep learning metering retry-safe and aligned with persisted survivors.
yyiilluu
added a commit
that referenced
this pull request
Aug 4, 2026
## Summary - Revert #407 and restore session outcomes, governance erasure, billing, and search behavior to the pre-open-world-evidence contracts. - Also revert #408 and #409 because their finalization-receipt and exposure-retention changes depend entirely on APIs introduced by #407. - Preserve the independent incremental aggregation work from #405 and #410. - Address every valid CodeRabbit finding, including SQLite downgrade compatibility and retry-safe metering. - Fix callback drop-rate anomaly emission on hosts with less than one hour of monotonic uptime, discovered by the full validation run. ## Changes ### Evidence foundation rollback - Remove search-exposure recording and session-outcome identity helpers. - Restore the prior session outcome schemas, client surface, and SQLite/storage contracts. - Restore the prior governance erase/claim flow and retention behavior. - Restore the prior resumable extraction and learning-billing behavior. ### Dependent follow-ups - Remove receipt-winner finalization behavior from #408. - Remove the exposure ownership and protected-retention behavior from #409. ### Review follow-ups - Rebuild #407-era SQLite `session_outcomes` tables into the restored schema, preserving `success`/`failure` rows, backfilling governance subject references, and explicitly dropping unrepresentable `unknown` outcomes with a warning. - Restore the SQLite 3.35 minimum required by existing `RETURNING` and `DROP COLUMN` usage. - Make outcome erasure resilient to governance-secret rotation and return a stable `session_outcomes` deletion count. - Acquire SQLite governance write locks before state checks, serialize idempotent purge begin/prepare flows across connections, and roll back failed target writes so SQLite cannot retain a stale writer transaction.\n- Reject legacy session-outcome schemas with empty governance-subject defaults and rebuild them with derived subject references.\n- Make synchronous playbook/profile persistence atomic while keeping scheduler dispatch strictly post-commit. - Meter resumable extraction from persisted survivors only, use retry-stable fallback keys, and emit learning billing from incremental aggregation. - Treat post-persist optimization and aggregation scheduling failures as best-effort side effects. ### Validation follow-up - Represent the callback executor's last anomaly time with an explicit unset sentinel so the first threshold crossing is never suppressed by low system uptime. ## Test Plan - `uv run ruff check reflexio tests` - `uv run ruff format --check reflexio tests` - Pyright on all 23 staged Python files: 0 errors, 0 warnings - Latest affected review files: 301 passed - OSS non-E2E suite: 5,535 passed, 73 skipped, 6 subtests passed - OSS E2E suite: 47 passed, 51 skipped - `npm --prefix docs run lint`: 0 errors (3 existing warnings) - `cd docs && npx tsc --noEmit` - `python -c "import reflexio"` Reverts `85a4b2255a96ef2a5b50f4cbe7c10758439e76b3`, plus dependent follow-ups `785a9e053ff771f40704bb7b0b5bbbe36048806a` and `eb88f44fd3b53457b76e8500ac1a30ba7d4ab16e`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Changes** - Session outcomes now support only success or failure, with simpler responses and retry behavior. - Governance data erasure workflows have streamlined retry and completion handling, including session-outcome removal. - Search exposure event recording has been removed; search results and metering remain available. - Learning-generation billing supports durable per-record tracking, retry-stable keys, and count-based fallback. - Scheduler failures during playbook processing are logged without preventing other scheduled actions. - **Documentation** - Quick Start prerequisites now list Node.js without the previous SQLite verification step. - Billing and extraction guidance has been updated. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Aug 6, 2026
- Add the shared evidence foundation for a future offline tuner without
enabling a production tuner or scheduler.
- Make session outcomes immutable, conflict-aware, reconstructable
records with server-owned contract and trajectory identities.
- Record synchronous search-exposure evidence and retain it under
governance-safe lifecycle rules.
- Keep billing limited to durable learnings created by online/resumable
extraction; derived aggregation and tuning remain unmetered.
- Add durable session-outcome identity fields, canonical digest helpers,
exact-retry behavior, and conflicting-finalization rejection.
- Add synchronous search-exposure recording with deterministic
identities and incomplete-evidence classification.
- Extend retention contracts for session outcomes and exposure evidence.
- Bind erasure to authoritative tenant-scoped users.
- Add purge execution claims, lease renewal, fencing, stale-claim
recovery, and deterministic retry behavior.
- Preserve missing-schema tolerance and stable deletion receipts.
- Emit learning usage only for durable extraction-created rows.
- Remove derived aggregation billing and make resumable retry keys
deterministic.
- Document immutable outcomes, extraction-only billing, and the
intentionally unavailable tuner capability.
- Add focused SQLite, API, governance, concurrency, migration, and
compatibility coverage.
```mermaid
flowchart LR
A[Online extraction] --> B[Durable profiles and playbooks]
A --> C[Session outcomes]
D[Search] --> E[Exposure evidence]
C --> F[Governance and retention]
E --> F
B --> G[Future offline analysis]
C --> G
E --> G
G -. disabled in Phase 1 .-> H[Candidate publication]
```
- Phase matrix: `146 passed, 9 skipped` across SQLite,
Supabase/PostgREST, native PostgreSQL, governance, exposure, and
migration cases.
- Migration/schema/placement gate: `64 passed`.
- Shared governance and storage focused suite: `58 passed`.
- Tuner availability/config gate: `7 passed`.
- Changed Python scope: Ruff clean, format clean, Pyright `0 errors, 0
warnings`.
- Changed SQL migration lint: `7 files, 0 findings`.
- Import and range diff checks passed.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
* **New Features**
* Session outcomes support `unknown` results with stable identity and
integrity details.
* Exact retries remain idempotent; conflicting finalizations are
rejected.
* Search results can be recorded as user-playbook exposures.
* User-data erasure supports safer retries and recovery during
concurrent processing.
* Finalized learning records support reliable retry handling and
accurate billing.
* **Bug Fixes**
* Session outcomes are removed using the authoritative user identity.
* Billing records avoid duplicate or unattributable finalized-learning
entries.
* **Migration**
* Existing session-outcome data is preserved and upgraded automatically.
* **Documentation**
* Local SQLite storage now requires version 3.35.0 or newer.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Aug 7, 2026
- Add the shared evidence foundation for a future offline tuner without
enabling a production tuner or scheduler.
- Make session outcomes immutable, conflict-aware, reconstructable
records with server-owned contract and trajectory identities.
- Record synchronous search-exposure evidence and retain it under
governance-safe lifecycle rules.
- Keep billing limited to durable learnings created by online/resumable
extraction; derived aggregation and tuning remain unmetered.
- Add durable session-outcome identity fields, canonical digest helpers,
exact-retry behavior, and conflicting-finalization rejection.
- Add synchronous search-exposure recording with deterministic
identities and incomplete-evidence classification.
- Extend retention contracts for session outcomes and exposure evidence.
- Bind erasure to authoritative tenant-scoped users.
- Add purge execution claims, lease renewal, fencing, stale-claim
recovery, and deterministic retry behavior.
- Preserve missing-schema tolerance and stable deletion receipts.
- Emit learning usage only for durable extraction-created rows.
- Remove derived aggregation billing and make resumable retry keys
deterministic.
- Document immutable outcomes, extraction-only billing, and the
intentionally unavailable tuner capability.
- Add focused SQLite, API, governance, concurrency, migration, and
compatibility coverage.
```mermaid
flowchart LR
A[Online extraction] --> B[Durable profiles and playbooks]
A --> C[Session outcomes]
D[Search] --> E[Exposure evidence]
C --> F[Governance and retention]
E --> F
B --> G[Future offline analysis]
C --> G
E --> G
G -. disabled in Phase 1 .-> H[Candidate publication]
```
- Phase matrix: `146 passed, 9 skipped` across SQLite,
Supabase/PostgREST, native PostgreSQL, governance, exposure, and
migration cases.
- Migration/schema/placement gate: `64 passed`.
- Shared governance and storage focused suite: `58 passed`.
- Tuner availability/config gate: `7 passed`.
- Changed Python scope: Ruff clean, format clean, Pyright `0 errors, 0
warnings`.
- Changed SQL migration lint: `7 files, 0 findings`.
- Import and range diff checks passed.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
* **New Features**
* Session outcomes support `unknown` results with stable identity and
integrity details.
* Exact retries remain idempotent; conflicting finalizations are
rejected.
* Search results can be recorded as user-playbook exposures.
* User-data erasure supports safer retries and recovery during
concurrent processing.
* Finalized learning records support reliable retry handling and
accurate billing.
* **Bug Fixes**
* Session outcomes are removed using the authoritative user identity.
* Billing records avoid duplicate or unattributable finalized-learning
entries.
* **Migration**
* Existing session-outcome data is preserved and upgraded automatically.
* **Documentation**
* Local SQLite storage now requires version 3.35.0 or newer.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Aug 10, 2026
- Add the shared evidence foundation for a future offline tuner without
enabling a production tuner or scheduler.
- Make session outcomes immutable, conflict-aware, reconstructable
records with server-owned contract and trajectory identities.
- Record synchronous search-exposure evidence and retain it under
governance-safe lifecycle rules.
- Keep billing limited to durable learnings created by online/resumable
extraction; derived aggregation and tuning remain unmetered.
- Add durable session-outcome identity fields, canonical digest helpers,
exact-retry behavior, and conflicting-finalization rejection.
- Add synchronous search-exposure recording with deterministic
identities and incomplete-evidence classification.
- Extend retention contracts for session outcomes and exposure evidence.
- Bind erasure to authoritative tenant-scoped users.
- Add purge execution claims, lease renewal, fencing, stale-claim
recovery, and deterministic retry behavior.
- Preserve missing-schema tolerance and stable deletion receipts.
- Emit learning usage only for durable extraction-created rows.
- Remove derived aggregation billing and make resumable retry keys
deterministic.
- Document immutable outcomes, extraction-only billing, and the
intentionally unavailable tuner capability.
- Add focused SQLite, API, governance, concurrency, migration, and
compatibility coverage.
```mermaid
flowchart LR
A[Online extraction] --> B[Durable profiles and playbooks]
A --> C[Session outcomes]
D[Search] --> E[Exposure evidence]
C --> F[Governance and retention]
E --> F
B --> G[Future offline analysis]
C --> G
E --> G
G -. disabled in Phase 1 .-> H[Candidate publication]
```
- Phase matrix: `146 passed, 9 skipped` across SQLite,
Supabase/PostgREST, native PostgreSQL, governance, exposure, and
migration cases.
- Migration/schema/placement gate: `64 passed`.
- Shared governance and storage focused suite: `58 passed`.
- Tuner availability/config gate: `7 passed`.
- Changed Python scope: Ruff clean, format clean, Pyright `0 errors, 0
warnings`.
- Changed SQL migration lint: `7 files, 0 findings`.
- Import and range diff checks passed.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
* **New Features**
* Session outcomes support `unknown` results with stable identity and
integrity details.
* Exact retries remain idempotent; conflicting finalizations are
rejected.
* Search results can be recorded as user-playbook exposures.
* User-data erasure supports safer retries and recovery during
concurrent processing.
* Finalized learning records support reliable retry handling and
accurate billing.
* **Bug Fixes**
* Session outcomes are removed using the authoritative user identity.
* Billing records avoid duplicate or unattributable finalized-learning
entries.
* **Migration**
* Existing session-outcome data is preserved and upgraded automatically.
* **Documentation**
* Local SQLite storage now requires version 3.35.0 or newer.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Aug 11, 2026
- Add the shared evidence foundation for a future offline tuner without
enabling a production tuner or scheduler.
- Make session outcomes immutable, conflict-aware, reconstructable
records with server-owned contract and trajectory identities.
- Record synchronous search-exposure evidence and retain it under
governance-safe lifecycle rules.
- Keep billing limited to durable learnings created by online/resumable
extraction; derived aggregation and tuning remain unmetered.
- Add durable session-outcome identity fields, canonical digest helpers,
exact-retry behavior, and conflicting-finalization rejection.
- Add synchronous search-exposure recording with deterministic
identities and incomplete-evidence classification.
- Extend retention contracts for session outcomes and exposure evidence.
- Bind erasure to authoritative tenant-scoped users.
- Add purge execution claims, lease renewal, fencing, stale-claim
recovery, and deterministic retry behavior.
- Preserve missing-schema tolerance and stable deletion receipts.
- Emit learning usage only for durable extraction-created rows.
- Remove derived aggregation billing and make resumable retry keys
deterministic.
- Document immutable outcomes, extraction-only billing, and the
intentionally unavailable tuner capability.
- Add focused SQLite, API, governance, concurrency, migration, and
compatibility coverage.
```mermaid
flowchart LR
A[Online extraction] --> B[Durable profiles and playbooks]
A --> C[Session outcomes]
D[Search] --> E[Exposure evidence]
C --> F[Governance and retention]
E --> F
B --> G[Future offline analysis]
C --> G
E --> G
G -. disabled in Phase 1 .-> H[Candidate publication]
```
- Phase matrix: `146 passed, 9 skipped` across SQLite,
Supabase/PostgREST, native PostgreSQL, governance, exposure, and
migration cases.
- Migration/schema/placement gate: `64 passed`.
- Shared governance and storage focused suite: `58 passed`.
- Tuner availability/config gate: `7 passed`.
- Changed Python scope: Ruff clean, format clean, Pyright `0 errors, 0
warnings`.
- Changed SQL migration lint: `7 files, 0 findings`.
- Import and range diff checks passed.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
* **New Features**
* Session outcomes support `unknown` results with stable identity and
integrity details.
* Exact retries remain idempotent; conflicting finalizations are
rejected.
* Search results can be recorded as user-playbook exposures.
* User-data erasure supports safer retries and recovery during
concurrent processing.
* Finalized learning records support reliable retry handling and
accurate billing.
* **Bug Fixes**
* Session outcomes are removed using the authoritative user identity.
* Billing records avoid duplicate or unattributable finalized-learning
entries.
* **Migration**
* Existing session-outcome data is preserved and upgraded automatically.
* **Documentation**
* Local SQLite storage now requires version 3.35.0 or newer.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Aug 13, 2026
- Add the shared evidence foundation for a future offline tuner without
enabling a production tuner or scheduler.
- Make session outcomes immutable, conflict-aware, reconstructable
records with server-owned contract and trajectory identities.
- Record synchronous search-exposure evidence and retain it under
governance-safe lifecycle rules.
- Keep billing limited to durable learnings created by online/resumable
extraction; derived aggregation and tuning remain unmetered.
- Add durable session-outcome identity fields, canonical digest helpers,
exact-retry behavior, and conflicting-finalization rejection.
- Add synchronous search-exposure recording with deterministic
identities and incomplete-evidence classification.
- Extend retention contracts for session outcomes and exposure evidence.
- Bind erasure to authoritative tenant-scoped users.
- Add purge execution claims, lease renewal, fencing, stale-claim
recovery, and deterministic retry behavior.
- Preserve missing-schema tolerance and stable deletion receipts.
- Emit learning usage only for durable extraction-created rows.
- Remove derived aggregation billing and make resumable retry keys
deterministic.
- Document immutable outcomes, extraction-only billing, and the
intentionally unavailable tuner capability.
- Add focused SQLite, API, governance, concurrency, migration, and
compatibility coverage.
```mermaid
flowchart LR
A[Online extraction] --> B[Durable profiles and playbooks]
A --> C[Session outcomes]
D[Search] --> E[Exposure evidence]
C --> F[Governance and retention]
E --> F
B --> G[Future offline analysis]
C --> G
E --> G
G -. disabled in Phase 1 .-> H[Candidate publication]
```
- Phase matrix: `146 passed, 9 skipped` across SQLite,
Supabase/PostgREST, native PostgreSQL, governance, exposure, and
migration cases.
- Migration/schema/placement gate: `64 passed`.
- Shared governance and storage focused suite: `58 passed`.
- Tuner availability/config gate: `7 passed`.
- Changed Python scope: Ruff clean, format clean, Pyright `0 errors, 0
warnings`.
- Changed SQL migration lint: `7 files, 0 findings`.
- Import and range diff checks passed.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
* **New Features**
* Session outcomes support `unknown` results with stable identity and
integrity details.
* Exact retries remain idempotent; conflicting finalizations are
rejected.
* Search results can be recorded as user-playbook exposures.
* User-data erasure supports safer retries and recovery during
concurrent processing.
* Finalized learning records support reliable retry handling and
accurate billing.
* **Bug Fixes**
* Session outcomes are removed using the authoritative user identity.
* Billing records avoid duplicate or unattributable finalized-learning
entries.
* **Migration**
* Existing session-outcome data is preserved and upgraded automatically.
* **Documentation**
* Local SQLite storage now requires version 3.35.0 or newer.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Aug 14, 2026
- Add the shared evidence foundation for a future offline tuner without
enabling a production tuner or scheduler.
- Make session outcomes immutable, conflict-aware, reconstructable
records with server-owned contract and trajectory identities.
- Record synchronous search-exposure evidence and retain it under
governance-safe lifecycle rules.
- Keep billing limited to durable learnings created by online/resumable
extraction; derived aggregation and tuning remain unmetered.
- Add durable session-outcome identity fields, canonical digest helpers,
exact-retry behavior, and conflicting-finalization rejection.
- Add synchronous search-exposure recording with deterministic
identities and incomplete-evidence classification.
- Extend retention contracts for session outcomes and exposure evidence.
- Bind erasure to authoritative tenant-scoped users.
- Add purge execution claims, lease renewal, fencing, stale-claim
recovery, and deterministic retry behavior.
- Preserve missing-schema tolerance and stable deletion receipts.
- Emit learning usage only for durable extraction-created rows.
- Remove derived aggregation billing and make resumable retry keys
deterministic.
- Document immutable outcomes, extraction-only billing, and the
intentionally unavailable tuner capability.
- Add focused SQLite, API, governance, concurrency, migration, and
compatibility coverage.
```mermaid
flowchart LR
A[Online extraction] --> B[Durable profiles and playbooks]
A --> C[Session outcomes]
D[Search] --> E[Exposure evidence]
C --> F[Governance and retention]
E --> F
B --> G[Future offline analysis]
C --> G
E --> G
G -. disabled in Phase 1 .-> H[Candidate publication]
```
- Phase matrix: `146 passed, 9 skipped` across SQLite,
Supabase/PostgREST, native PostgreSQL, governance, exposure, and
migration cases.
- Migration/schema/placement gate: `64 passed`.
- Shared governance and storage focused suite: `58 passed`.
- Tuner availability/config gate: `7 passed`.
- Changed Python scope: Ruff clean, format clean, Pyright `0 errors, 0
warnings`.
- Changed SQL migration lint: `7 files, 0 findings`.
- Import and range diff checks passed.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
* **New Features**
* Session outcomes support `unknown` results with stable identity and
integrity details.
* Exact retries remain idempotent; conflicting finalizations are
rejected.
* Search results can be recorded as user-playbook exposures.
* User-data erasure supports safer retries and recovery during
concurrent processing.
* Finalized learning records support reliable retry handling and
accurate billing.
* **Bug Fixes**
* Session outcomes are removed using the authoritative user identity.
* Billing records avoid duplicate or unattributable finalized-learning
entries.
* **Migration**
* Existing session-outcome data is preserved and upgraded automatically.
* **Documentation**
* Local SQLite storage now requires version 3.35.0 or newer.
<!-- 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.
Summary
Changes
Evidence contracts
Governance
Billing
Documentation and tests
Diagrams
flowchart LR A[Online extraction] --> B[Durable profiles and playbooks] A --> C[Session outcomes] D[Search] --> E[Exposure evidence] C --> F[Governance and retention] E --> F B --> G[Future offline analysis] C --> G E --> G G -. disabled in Phase 1 .-> H[Candidate publication]Test Plan
146 passed, 9 skippedacross SQLite, Supabase/PostgREST, native PostgreSQL, governance, exposure, and migration cases.64 passed.58 passed.7 passed.0 errors, 0 warnings.7 files, 0 findings.Summary by CodeRabbit
unknownresults with stable identity and integrity details.