feat: restore open-world evidence foundation - #416
Conversation
- 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 -->
- make durable finalization receipt insertion atomically identify the sole winning transaction - emit resumable extraction billing only for the receipt winner - keep optimizer, aggregation, and tagging dispatch best-effort and at-most-once - preserve ordered winner IDs, empty receipts, and existing list-returning service wrappers - add an internal `FinalizationResult` carrying learning IDs and receipt ownership - roll back stale loser writes and reuse the committed winner receipt - independently isolate optimizer and aggregation scheduling failures - gate tagging and billing on receipt ownership - add deterministic retry, empty-receipt, identical-ID race, and scheduler coverage - 60 focused shared receipt/resume tests passed - repeated profile/playbook concurrency regressions passed - Ruff format/check and focused Pyright passed - enterprise Supabase/native-Postgres adapter contract is covered in the related enterprise branch <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Improved resumable extraction finalization to prevent duplicate learning records, tags, and billing events. * Added reliable handling for concurrent finalization attempts, preserving the original result and avoiding repeated processing. * Scheduler failures are isolated so one failed follow-up task does not disrupt completed finalization. * Retry flows now maintain durable learning IDs and consistent outcomes. * **Reliability** * Finalization receipts are now immutable and safely reused across retries and competing workers. * Added safeguards for empty or incomplete finalization receipts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - preserve every user-playbook exposure ingested within the fixed 14-day evidence window, even when the generic row cap is exceeded - separate the optional retrieval subject from the served playbook owner so unscoped searches remain attributable without becoming join-eligible - normalize blank retrieval subjects to the unscoped representation while retaining fail-closed owner checks for scoped searches ## Why The Phase 1 offline-tuner evidence foundation requires complete, reviewable exposure evidence. Generic row-cap retention could delete current-window events, and the original exposure envelope overloaded playbook ownership as the retrieval subject for unscoped searches. Both behaviors could distort later evidence eligibility and exact-user governance. ## Behavior - exposure rows younger than 14 days are protected from row-cap deletion - an exposure may carry a retrieval subject and a playbook owner independently - blank or whitespace retrieval subjects become unscoped - scoped retrieval still rejects a user playbook owned by another user before persistence ## Verification - 46 shared exposure and retention contract tests passed after rebasing onto current `main` - final affected enterprise matrix: 164 passed, 9 adapter-applicability skips - Ruff formatting/lint, Pyright, import, diff, and gitlink checks passed - correctness, security-resilience, architecture, and verification-testing review lenses are clean ## Related PRs - Enterprise Phase 1 stack: ReflexioAI/reflexio-enterprise#936 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Exposure records now preserve the playbook owner separately from the person retrieving it. * Blank retrieval identifiers are normalized consistently. * Playbook exposure data now includes governance subject references where available. * **Bug Fixes** * Prevented exposure records from being created when a playbook belongs to a different scoped user. * Added safeguards for unscoped exposure scenarios. * **Data Retention** * Open-world evidence, including playbook exposure records, is retained for at least 14 days before cleanup. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Make resumable finalization replay stable per-learning usage keys until the configured recorder durably accepts or deduplicates them, while preserving fail-open metering for ordinary product calls.
Preserve current-main storage and billing contracts while restoring the Phase 1 replay semantics.
Prefetch request and interaction rows in SQLite-safe chunks while preserving the atomic legacy outcome rebuild and canonical trajectory identities.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds durable session-outcome identities, receipt-backed finalization, strict usage delivery, fenced governance erasure, deterministic search-exposure records, retention age limits, always-global sweeps, and SQLite compatibility checks. ChangesDurable outcomes and finalization
Governance and exposure
Scheduler and compatibility
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (25)
reflexio/server/services/governance/service.py (1)
418-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the tautological
purge_idcheck.
_matching_user_erasure_purge_for_retryfetches the purge bypurge_idthroughget_purge_operation(purge_id), then asserts thatpurge.purge_id == purge_id. That comparison cannot fail. The remaining checks onoperation_type,scope_type,subject_ref, andrequest_refcarry the actual value.♻️ Proposed simplification
purge = self.storage.get_purge_operation(purge_id) expected_identity = { - "purge_id": purge_id, "operation_type": operation_type, "scope_type": scope_type, "subject_ref": subject_ref, "request_ref": request_ref, }🤖 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 418 - 449, Remove the tautological purge_id entry from expected_identity in _matching_user_erasure_purge_for_retry, while preserving validation of operation_type, scope_type, subject_ref, and request_ref and the authoritative-user check.tests/server/services/governance/test_subject_write_barrier_sqlite.py (2)
622-629: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate barrier fetch.
barrierandstored_barrierread the same row throughget_subject_write_barrier(subject_ref)and then assert the samestatus == "erased". Keep one.♻️ Proposed change
barrier = storage.get_subject_write_barrier(subject_ref) assert barrier is not None - stored_barrier = storage.get_subject_write_barrier(subject_ref) stored_purge = storage.get_purge_operation(purge.purge_id) assert barrier.status == "erased" - assert stored_barrier is not None - assert stored_barrier.status == "erased" assert stored_purge is not None assert stored_purge.status == "complete"🤖 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_subject_write_barrier_sqlite.py` around lines 622 - 629, Remove the redundant get_subject_write_barrier(subject_ref) call in this test, keeping a single barrier fetch and its associated non-null and status assertions; preserve the stored_purge retrieval and its independent assertions.
42-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the deliberate lease steal in
_claim_purge.When a claim is already held, this helper forcibly expires
execution_claim_expires_atand re-claims. Each call also increments the fence, so any claim obtained by an earlier call becomes stale. The current call sites use each claim before the next_claim_purgecall, so the tests are correct. Add a comment stating that the helper always takes over the lease, and that callers must not hold a claim across two_claim_purgecalls.🤖 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_subject_write_barrier_sqlite.py` around lines 42 - 62, Add a concise comment in `_claim_purge` documenting that it deliberately expires and steals any existing execution lease, increments the fence, and makes claims from earlier calls stale; state that callers must use each returned claim before invoking `_claim_purge` again.tests/server/services/governance/test_governance_local_e2e.py (3)
963-966: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
time.sleepinstead of a throwawayEvent.
threading.Event().wait(0.001)creates an event that is never set, only to block for 1 ms.time.sleep(0.001)states the intent directly.♻️ Proposed change
def release_winner_on_duplicate_poll(_seconds: float) -> None: lifecycle.release_first_call.set() - threading.Event().wait(0.001) + time.sleep(0.001)Add
import timeat the top of the file.🤖 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 963 - 966, Update release_winner_on_duplicate_poll to use time.sleep(0.001) instead of creating and waiting on a throwaway threading.Event, and add the required time import at the file’s imports.
1052-1110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the fake-clock arithmetic.
This test depends on a precise relationship that is not stated: the claim starts at
_epoch_now() == 100with a 300-second lease, so it expires at 400. The lifecycle advances the clock to 350, waits for three renewals that push the expiry to 650, then advances to 401. The duplicate caller at 401 must therefore still see a live lease. A reader who changes_PURGE_EXECUTION_LEASE_SECONDSor one of these literals will get a confusing flake. Add a comment stating the intended 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 `@tests/server/services/governance/test_governance_local_e2e.py` around lines 1052 - 1110, Document the fake-clock invariant in test_healthy_slow_lifecycle_renews_lease_and_duplicate_converges: state that the claim begins at 100 with a 300-second lease expiring at 400, three renewals extend it to 650, and advancing to 401 keeps the duplicate caller within the live lease. Place the comment next to the relevant fake_now updates and renewal wait.
1044-1049: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the backoff expectations from the module constants.
The assertions hardcode
0.05,1.0, and5.0. Those values duplicate_DUPLICATE_ERASE_POLL_SECONDS,_DUPLICATE_ERASE_MAX_POLL_SECONDS, and_DUPLICATE_ERASE_WAIT_SECONDS. If a constant changes, this test fails on the literal rather than on the backoff shape. Read the constants fromgovernance_service_moduleso the test keeps checking the doubling behavior and the total bound.♻️ Proposed change
- assert service.sleep_delays[:5] == pytest.approx([0.05, 0.1, 0.2, 0.4, 0.8]) - assert max(service.sleep_delays) == 1.0 - assert sum(service.sleep_delays) == pytest.approx(5.0) + base = governance_service_module._DUPLICATE_ERASE_POLL_SECONDS + assert service.sleep_delays[:5] == pytest.approx( + [base * 2**step for step in range(5)] + ) + assert max(service.sleep_delays) == ( + governance_service_module._DUPLICATE_ERASE_MAX_POLL_SECONDS + ) + assert sum(service.sleep_delays) == pytest.approx( + governance_service_module._DUPLICATE_ERASE_WAIT_SECONDS + )🤖 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 1044 - 1049, Update the backoff assertions in the governance erase retry test to derive expected initial, maximum, and total delays from governance_service_module’s _DUPLICATE_ERASE_POLL_SECONDS, _DUPLICATE_ERASE_MAX_POLL_SECONDS, and _DUPLICATE_ERASE_WAIT_SECONDS constants. Preserve the existing checks for doubling behavior, maximum delay, and total wait bound without hardcoded duplicate values.reflexio/server/services/storage/sqlite_storage/governance/_purge.py (1)
449-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated claim assertion.
assert_purge_operation_execution_claimand_assert_purge_operation_execution_claim_lockedcontain the same validation and the same five-condition row check. Only the row-fetch mechanism differs. Extract the row check so a future change to the fencing rule cannot be applied to one copy only.♻️ Proposed refactor
+ def _assert_claim_row(self, row: Any, claim: PurgeExecutionClaim) -> None: + if row is None: + raise ValueError(f"Purge operation {claim.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"]) <= _epoch_now() + ): + raise ValueError("purge execution claim is no longer active") + 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( + self._assert_claim_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") + (purge_id, self.org_id), + ), + claim, + )Apply the same delegation in
_assert_purge_operation_execution_claim_locked.🤖 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 449 - 497, Consolidate the duplicated validation in assert_purge_operation_execution_claim and _assert_purge_operation_execution_claim_locked by having the locked method delegate to the shared assertion logic while preserving its locked row-fetch behavior. Extract or reuse a common row-check helper so both paths apply the same five claim conditions, with only the database fetch mechanism differing.tests/server/services/storage/sqlite_storage/test_governance_storage.py (2)
1677-1704: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the caller scan fail when the production root resolves incorrectly.
production_rootis derived fromparents[5], which couples the test to its exact directory depth. If this file moves one level,parents[5]points at a different directory,rglob("*.py")yields nothing, andassert violations == []passes without scanning anything. The test then reports success while enforcing nothing.Add an assertion that the root exists and that the scan visited a non-zero number of files.
🛡️ Proposed guard
production_root = Path(__file__).resolve().parents[5] / "reflexio" + assert production_root.is_dir(), production_root violations: list[str] = [] + scanned = 0 for path in production_root.rglob("*.py"): + scanned += 1 tree = ast.parse(path.read_text(), filename=str(path))+ assert scanned > 0 assert violations == []🤖 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 1677 - 1704, Strengthen the production scan in the test around production_root by asserting that the derived root exists and is a directory before scanning. Track the files yielded by production_root.rglob("*.py") and assert that the scan visited at least one file, while preserving the existing violation checks.
218-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
_claim_purgesteals a live lease.When a live unexpired lease already exists,
claim_purge_operation_executionreturnsNone._claim_purgethen force-expires the lease and re-claims, which increments the fence and invalidates every previously issued claim for thatpurge_id.Several tests call
_claim_purgemore than once for the samepurge_id, for example Lines 701, 708, and 722. Each call currently supersedes the prior one. That works today only because every call site uses its claim immediately. A future test that captures a claim into a variable and then calls_claim_purgeagain before using it will fail withpurge execution claim is no longer active, and the cause will not be obvious.Add a docstring stating the takeover behavior, or split the helper into
_claim_purge(first claim only) and_take_over_purge(force-expire and re-claim).🤖 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 218 - 238, Document the lease-takeover behavior in the test helper _claim_purge: add a docstring stating that when a live lease exists it force-expires that lease, reclaims the purge, and invalidates previously issued claims for the same purge_id. Keep the existing claiming logic unchanged.tests/server/services/lineage/test_gc_scheduler_global_sweep.py (2)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the always-sweep helpers directly instead of guarding with
getattr.
register_always_global_sweepandclear_always_global_sweepsexist ingc_scheduler. Two other tests in this file already callgc_scheduler.register_always_global_sweepdirectly at Line 135 and Line 153.The fixture guard is the larger risk. If
clear_always_global_sweepsis renamed or removed, the fixture silently stops clearing registrations, and always-sweep hooks leak into every later test in the session. A direct import fails loudly instead.♻️ Proposed change
def _isolate_hooks(): clear_global_sweeps() - clear_always = getattr(gc_scheduler, "clear_always_global_sweeps", None) - if clear_always is not None: - clear_always() + gc_scheduler.clear_always_global_sweeps() yield clear_global_sweeps() - if clear_always is not None: - clear_always() + gc_scheduler.clear_always_global_sweeps()def test_run_once_invokes_always_global_sweep_when_expiry_disabled(monkeypatch): calls: list[int] = [] - register = getattr(gc_scheduler, "register_always_global_sweep", None) - assert callable(register) - register(lambda now: calls.append(now) or 1) + gc_scheduler.register_always_global_sweep(lambda now: calls.append(now) or 1)Also applies to: 110-112
🤖 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/lineage/test_gc_scheduler_global_sweep.py` around lines 30 - 36, Update the fixture cleanup to import and call gc_scheduler.clear_always_global_sweeps directly, removing the getattr guard and clear_always variable; likewise import register_always_global_sweep directly where the fixture or tests use it, so missing helpers fail loudly while preserving cleanup before and after each test.
143-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert against the scheduler constants instead of literals.
5and86400duplicate_BOOTSTRAP_RETRY_INTERVAL_SECONDSand_DEFAULT_POLL_INTERVAL_SECONDSingc_scheduler.py. If either constant changes, these tests fail with an opaque numeric mismatch that does not name the constant.Import the constants and compare against them so the failure message points at the source of truth.
Also applies to: 170-170
🤖 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/lineage/test_gc_scheduler_global_sweep.py` at line 143, Update the assertions in the global sweep scheduler tests, including the assertion around scheduler._run_once() and the one at the referenced second location, to compare against _BOOTSTRAP_RETRY_INTERVAL_SECONDS and _DEFAULT_POLL_INTERVAL_SECONDS from gc_scheduler.py instead of numeric literals. Import both constants from the scheduler module and preserve the existing expected-value behavior.reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py (1)
188-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
scope_typecheck.Line 190 already rejects any purge whose
scope_typeis not"user". Line 207 repeats the same comparison inside the composite condition, so it can never be true at that point.♻️ Proposed simplification
if ( - purge_row["scope_type"] != "user" - or purge_row["subject_ref"] != subject_ref + purge_row["subject_ref"] != subject_ref or self._subject_ref_for_user_id(authoritative_user_id) != subject_ref🤖 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/_subject_barrier.py` around lines 188 - 215, Remove the redundant purge_row["scope_type"] != "user" comparison from the composite validation condition after the initial guard. Keep the existing scope validation at the start of the completion flow and preserve all other authoritative identity checks unchanged.reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py (1)
92-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the
_assert_purge_operation_execution_claim_lockedMRO hints to a non-optional claim.All three mixins declare the MRO-provided callable as
Callable[[str, PurgeExecutionClaim | None], None]. The implementation inreflexio/server/services/storage/sqlite_storage/governance/_purge.pydeclaresexecution_claim: PurgeExecutionClaim, andtest_claimed_erasure_mutation_signatures_and_callers_require_claimasserts the public annotation is non-optional. The| Nonewidens the type only for callers inside these three mixins, so Pyright would accept aNoneclaim there without an error, and the failure would surface at runtime insidevalidate_purge_execution_claim.
reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py#L92-L94: change the hint toCallable[[str, PurgeExecutionClaim], None].reflexio/server/services/storage/sqlite_storage/governance/_rebuild_hide.py#L85-L87: change the hint toCallable[[str, PurgeExecutionClaim], None].reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py#L80-L82: change the hint toCallable[[str, PurgeExecutionClaim], None].♻️ Proposed change (identical at each site)
- _assert_purge_operation_execution_claim_locked: Callable[ - [str, PurgeExecutionClaim | None], None - ] + _assert_purge_operation_execution_claim_locked: Callable[ + [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 - 94, Update the MRO-provided _assert_purge_operation_execution_claim_locked callable annotation to require PurgeExecutionClaim rather than allowing None in reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py:92-94, _rebuild_hide.py:85-87, and _subject_barrier.py:80-82. Keep the callable signature as Callable[[str, PurgeExecutionClaim], None] at all three sites, matching the implementation and public contract.tests/models/test_session_outcome_identity.py (1)
133-135: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAnnotate the golden digest, and consider versioning the trajectory shape.
This literal pins the output of
trajectory_digestoverCanonicalSessionTrajectory. Any field added toCanonicalRequestorCanonicalInteraction, and any change to_canonical_trajectory_json, fails this assertion with an opaque hex mismatch. Add a comment that states what the value covers and what to do when it changes intentionally.The deeper point:
canonical_session_trajectoryembeds no schema version, unlikeoutcome_contract_digest, which hashes an explicitschema_version, and unlikeuser_playbook_full_version_fingerprintinreflexio/server/services/search_exposure.pylines 103-118, which pins"user-playbook-full-version-v1"and documents the bump requirement. Persistedfinalized_trajectory_digestvalues are compared against freshly computed ones on retry. After a field is added to the canonical shape, every stored digest silently stops matching, and callers receiveconflicting_finalizationfor identical payloads. Aschema_versionkey inside the trajectory payload makes that transition explicit and diagnosable.♻️ Proposed comment for the golden value
assert sqlite_projection == postgres_projection assert sqlite_projection["requests"][0]["request"]["evaluation_only"] is False + # Golden digest over the canonical trajectory shape. Update this value only + # together with an intentional change to CanonicalRequest, + # CanonicalInteraction, or _canonical_trajectory_json. Stored + # finalized_trajectory_digest values do not survive such a change. assert trajectory_digest(sqlite_projection) == ( "73d0f738bb5a3c7668787c678230c4758b68923e12a16e3851b401db780e4272" )🤖 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 133 - 135, Annotate the golden digest assertion for trajectory_digest with a comment describing that it covers CanonicalSessionTrajectory and instructing maintainers to review and intentionally update the value when the canonical shape or _canonical_trajectory_json changes. Also add an explicit schema_version to the canonical_session_trajectory payload, following the existing versioning pattern, and ensure intentional version bumps make persisted digest changes diagnosable rather than causing opaque retry conflicts.tests/server/api_endpoints/test_session_outcomes_integration.py (1)
232-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
session_idfrom the parameter id, not from the string length.
session_iduseslen(stored_metadata). The three current values have lengths 0, 10, and 20000, so they do not collide today. A future parameter whose length equals an existing one reuses the samesession_id. The second run would then post against a session that the first run already finalized, and the failure would point at the outcome logic rather than at the duplicate id.Pass an explicit id alongside each metadata value.
♻️ Proposed fix
`@pytest.mark.parametrize`( - "stored_metadata", + ("case_id", "stored_metadata"), [ - pytest.param("", id="empty"), - pytest.param("{malformed", id="malformed"), - pytest.param("[" * 10_000 + "]" * 10_000, id="pathological-nesting"), + pytest.param("empty", "", id="empty"), + pytest.param("malformed", "{malformed", id="malformed"), + pytest.param( + "pathological-nesting", + "[" * 10_000 + "]" * 10_000, + id="pathological-nesting", + ), ], ) def test_retry_rejects_invalid_stored_metadata( - client_with_org: tuple[TestClient, str], stored_metadata: str + client_with_org: tuple[TestClient, str], case_id: str, stored_metadata: str ) -> None: client, org_id = client_with_org storage = get_reflexio(org_id=org_id).get_storage() - session_id = f"invalid-stored-metadata-{len(stored_metadata)}" + session_id = f"invalid-stored-metadata-{case_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/api_endpoints/test_session_outcomes_integration.py` around lines 232 - 245, Update test_retry_rejects_invalid_stored_metadata to parameterize an explicit identifier alongside each stored_metadata value, then derive session_id from that identifier instead of len(stored_metadata). Preserve the existing metadata cases and use unique parameter ids such as empty, malformed, and pathological-nesting to prevent collisions.reflexio/models/api_schema/domain/entities.py (1)
829-867: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the outcome-identity validation into one shared definition.
The digest validator and the all-or-none identity rule now exist three times:
reflexio/models/api_schema/domain/entities.pylines 843-866 (SessionOutcomeRecord)reflexio/models/api_schema/domain/entities.pylines 918-941 (SetSessionOutcomeResponse)reflexio/server/services/storage/storage_base/_session_outcomes.pylines 26-38 (SessionOutcomeWriteResult.__post_init__)If a fifth identity field is added later, all three copies must change together. A missed copy produces a model that accepts a partial identity. Define the identity tuple and its check once, then reuse it. The two Pydantic models can share a mixin base class; the dataclass can call the same helper function.
♻️ Proposed shared mixin for the two Pydantic models
+class OutcomeIdentityMixin(BaseModel): + outcome_id: NonEmptyStr | None = None + outcome_revision: int | None = Field(default=None, ge=1) + outcome_contract_digest: Sha256Digest | None = None + finalized_trajectory_digest: Sha256Digest | None = None + + `@field_validator`("outcome_contract_digest", "finalized_trajectory_digest") + `@classmethod` + def validate_sha256_digest(cls, value: str | None) -> str | None: + if value is not None and ( + len(value) != 64 or any(char not in "0123456789abcdef" for char in value) + ): + raise ValueError("outcome identity digests must be lowercase SHA-256 hex") + return value + + `@model_validator`(mode="after") + def validate_identity_shape(self) -> Self: + identity = ( + self.outcome_id, + self.outcome_revision, + self.outcome_contract_digest, + self.finalized_trajectory_digest, + ) + if any(value is None for value in identity) and not all( + value is None for value in identity + ): + raise ValueError( + "outcome identity fields must be all populated or all null" + ) + return self
SessionOutcomeRecordandSetSessionOutcomeResponsethen inherit fromOutcomeIdentityMixinand drop their local copies. Field order in serialized output changes, so confirm the expected dict intests/models/test_session_outcome_identity.pylines 160-176 still matches.🤖 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 829 - 867, Extract the shared outcome identity fields, lowercase SHA-256 digest validation, and all-or-none identity check from SessionOutcomeRecord and SetSessionOutcomeResponse into one reusable OutcomeIdentityMixin/helper. Update both Pydantic models to reuse it and change SessionOutcomeWriteResult.__post_init__ to call the same shared identity validator, preserving partial-identity rejection and digest validation. Verify serialized field ordering and expected dictionaries in the session outcome identity tests.tests/server/services/storage/test_storage_contract_clear_user_data.py (1)
176-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
monkeypatch.setenvcall has no effect on this test.The
storagefixture is constructed before the test body runs, so settingREFLEXIO_GOVERNANCE_REF_SECREThere does not change any secret the storage instance already resolved. The test also passes"test-governance-secret"explicitly togovernance_subject_ref, andclear_user_datadeletes byuser_idwithout deriving a subject ref. Remove themonkeypatchparameter and thesetenvcall, or move the environment setup into the fixture if the storage-derived ref is meant to match.🤖 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 176 - 186, Remove the unused monkeypatch parameter from the affected test and delete the REFLEXIO_GOVERNANCE_REF_SECRET setenv call; keep the explicit "test-governance-secret" arguments to governance_subject_ref and the existing SQLiteStorage setup unchanged.tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py (1)
199-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the expected query count from the chunk size.
row_count = 501andassert len(trajectory_input_queries) == 4both encode thechunked()default chunk size implicitly. If that default changes, the assertion fails with no indication of the cause. Import the chunk-size constant (or computeceil(row_count / chunk_size) * 2) so the bound the test protects is explicit.🤖 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 199 - 254, Update the migration test around _migrate_session_outcomes_schema to derive the expected trajectory_input_queries count from the shared chunk-size constant, accounting for two query types per chunk (for example, ceil(row_count / chunk_size) * 2). Replace the hard-coded 501/4 relationship while preserving the existing row-count assertion.tests/server/services/storage/test_storage_contract_session_outcomes.py (1)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the SQLite-only tests with
isinstanceinstead ofcast.This module is a cross-backend storage contract suite typed against
BaseStorage.cast(SQLiteStorage, storage)is a static-only assertion. If thestoragefixture is later parametrized with Supabase or PostgreSQL, these tests fail with an opaqueAttributeErroron.connrather than a clear skip or assertion.tests/server/services/storage/test_storage_contract_clear_user_data.pyalready usesassert isinstance(sqlite_storage, SQLiteStorage). Apply the same pattern here for the four SQLite-specific tests.♻️ Proposed change (repeat at each cast site)
- sqlite_storage = cast(SQLiteStorage, storage) + sqlite_storage = storage + assert isinstance(sqlite_storage, SQLiteStorage)Also applies to: 124-131
🤖 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 100 - 102, Update the four SQLite-specific tests in this contract module, including test_legacy_all_null_identity_exact_retry_uses_available_context, to assert isinstance(storage, SQLiteStorage) before accessing SQLite-only attributes such as conn, then use the narrowed storage directly instead of cast. Follow the guard pattern from test_storage_contract_clear_user_data.py so non-SQLite backends fail clearly or skip rather than raising an opaque AttributeError.reflexio/server/services/base_generation_service.py (1)
727-729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the billing parameter to match its new meaning.
emit_generation_side_effectsnow passesplan.billable_countinto a parameter still namedgenerated_count. The mixin docstring already describes it as "retained write-plan learnings". The name now contradicts the value at every call site and invites a future caller to pass raw generated counts again. Rename the keyword in_record_billing_learning_eventstobillable_countand update the two call sites.♻️ Proposed rename at this call site
self._record_billing_learning_events( - prepared=plan.prepared, generated_count=plan.billable_count + prepared=plan.prepared, billable_count=plan.billable_count )Apply the matching signature change in
reflexio/server/services/base_generation/_usage_billing.py:def _record_billing_learning_events( self, *, prepared: "PreparedGenerationRun[Any]", billable_count: int ) -> None: ... record_learnings_generated( org_id=ctx["org_id"], count=billable_count, ... )Also update
tests/server/services/test_generation_billing_emission.pyline 493.🤖 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 727 - 729, Rename the _record_billing_learning_events keyword parameter from generated_count to billable_count, use billable_count when calling record_learnings_generated, and update both call sites, including emit_generation_side_effects and the affected billing emission test.tests/server/test_usage_metrics.py (1)
31-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd fail-open coverage for
record_usage_eventon the same two inputs.This test proves that a legacy recorder returning
NoneyieldsUNKNOWNfromrecord_usage_event_strict. That behavior is correct for the strict API. The untested consequence is on the non-strict API:record_usage_eventnow funnels the sameUNKNOWNinto itsexcepthandler and emits a warning for every event. See the issue raised onreflexio/server/usage_metrics.pyLines 118-157.Add two cases that pin the intended fail-open contract:
record_usage_eventwith no recorder configured must not log a warning.record_usage_eventwith aNone-returning recorder must deliver the event and must not log a warning.Use
caplogatWARNINGlevel to assert the absence of records.💚 Proposed tests
def test_ordinary_recording_is_silent_without_a_recorder(caplog): usage_metrics.configure_usage_event_recorder(None) with caplog.at_level("WARNING", logger=usage_metrics.logger.name): usage_metrics.record_usage_event( org_id="7", event_name="search_request", event_category="application", ) assert caplog.records == [] def test_ordinary_recording_is_silent_for_legacy_none_recorder(caplog): captured = [] usage_metrics.configure_usage_event_recorder(captured.append) try: with caplog.at_level("WARNING", logger=usage_metrics.logger.name): usage_metrics.record_usage_event( org_id="7", event_name="search_request", event_category="application", event_key="search:1", ) finally: usage_metrics.configure_usage_event_recorder(None) assert [event.event_key for event in captured] == ["search:1"] assert caplog.records == []🤖 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/test_usage_metrics.py` around lines 31 - 46, Add fail-open coverage in tests/server/test_usage_metrics.py for record_usage_event: add one test with no recorder configured and another with a legacy recorder returning None, using caplog at WARNING level to assert no warnings are emitted; verify the latter still delivers the event and always reset the recorder configuration.reflexio/server/services/playbook/service.py (1)
661-735: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared receipt-finalization flow.
This method duplicates
ProfileGenerationService._finalize_extracted_items_with_outcomeinreflexio/server/services/profile/service.pyalmost line for line: the pre-transaction receipt read, the in-transaction re-read, the persist call, thesave_agent_run_finalization_receiptinsert, the_FinalizationReceiptAlreadyExistsErrorrollback signal, and the "receipt disappeared" guard. Only two things differ: theentity_typeconstant, the point where learning ids become available, and the playbook-only scheduler dispatch.The receipt protocol is the correctness-critical part of this PR. Two copies mean a future fix to one copy silently misses the other. Extract the protocol into one helper on the base service or a mixin, parameterized by
entity_type, an id-extraction callback, and an optional post-commit hook.🤖 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/playbook/service.py` around lines 661 - 735, Extract the duplicated receipt protocol from _finalize_extracted_items_with_outcome and ProfileGenerationService._finalize_extracted_items_with_outcome into one shared base-service or mixin helper. Parameterize it with entity_type, a callback that derives learning IDs after _persist_write_plan, and an optional post-commit hook for playbook scheduler dispatch; preserve the pre-transaction read, transactional re-read/persist/receipt insert, conflict rollback signal, and disappeared-receipt guard while making both services use the shared flow.tests/server/test_billing_meter.py (1)
123-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
UNKNOWNmapping.The parametrization covers
FAILEDandREJECTED. It does not cover theexcept Exceptionbranch at Line 489 ofbilling_meter.py, which maps any non-UsageEventDeliveryErrorfailure toReceiptBillingDeliveryError(UsageEventDeliveryStatus.UNKNOWN).That status decides the retry class in
_finalization_failure_status:REJECTEDterminates the run, everything else retries. Add a case that raises a plainRuntimeErrorfromrecord_usage_event_strictand asserts the resulting status isUNKNOWN.🤖 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/test_billing_meter.py` around lines 123 - 146, Extend test_strict_receipt_billing_preserves_delivery_status to cover a plain RuntimeError from record_usage_event_strict, alongside the existing UsageEventDeliveryError cases. Assert that emit_learnings_generated_records_strict raises ReceiptBillingDeliveryError with status UsageEventDeliveryStatus.UNKNOWN.reflexio/server/billing_meter.py (1)
268-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
record_eventinstead of usingAny.
record_event: Anydisables signature checking on the only call in this helper.record_usage_eventandrecord_usage_event_strictmust keep identical keyword parameters for both call paths to work. AProtocolorCallablealias makes drift a type error.♻️ Proposed typing
+class _UsageEventRecorder(Protocol): + def __call__( + self, + *, + org_id: str, + event_name: str, + event_category: str, + pipeline: str | None, + user_id: str | None, + request_id: str | None, + session_id: str | None, + source: str | None, + agent_version: str | None, + playbook_name: str | None, + entity_type: str | None, + entity_id: str | None, + event_key: str | None, + count_value: int, + platform_llm: bool | None, + platform_storage: bool | None, + caller_type: str | None, + metadata: Mapping[str, Any] | None, + created_at: float | None, + ) -> Any: ... + + def _record_learnings_generated_records( *, - record_event: Any, + record_event: _UsageEventRecorder,🤖 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/billing_meter.py` around lines 268 - 288, The _record_learnings_generated_records helper currently accepts record_event as Any, bypassing signature validation. Define a Protocol or Callable type describing the exact keyword arguments used by its record_event call, and apply it to record_event so both record_usage_event and record_usage_event_strict remain compatible and parameter drift is caught by type checking.reflexio/server/services/profile/service.py (1)
360-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared receipt-fencing flow.
This method duplicates
PlaybookGenerationService._finalize_extracted_items_with_outcomealmost line for line: the pre-scope receipt read, the in-scope receipt re-read, the persist call,save_agent_run_finalization_receipt, the_FinalizationReceiptAlreadyExistsErrorconflict rollback, and the post-conflict re-read that raisesRuntimeErrorwhen the receipt is missing.Only three parts differ: the
entity_typeconstant, whenlearning_idsare derived (before persist for profiles, after persist for playbooks), and the playbook scheduler dispatch. A shared helper that takesentity_type, a persist callback, and a learning-id callback would remove the copy. This is a money and durability path, so divergence between the two copies is expensive.🤖 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 - 422, Extract the duplicated receipt-fencing logic from profile `_finalize_extracted_items_with_outcome` and `PlaybookGenerationService._finalize_extracted_items_with_outcome` into a shared helper. Parameterize the helper with `entity_type`, a persist callback, and a learning-ID callback so profiles can derive IDs before persistence while playbooks derive them afterward; keep playbook scheduler dispatch outside the helper. Preserve the pre-scope and in-scope receipt checks, conflict rollback, post-conflict receipt reread, and missing-receipt `RuntimeError` behavior exactly.
🤖 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/api.py`:
- Around line 475-481: Move the cleanup scope in the startup flow to begin
immediately after configure_usage_event_recorder(exempt_usage_event_recorder),
before run_startup_config_guards, data-plane initialization, or scheduler
startup can raise. Ensure every failure after configuring the recorder clears or
restores the global recorder, including failures occurring before the existing
finally block.
In `@reflexio/server/billing_meter.py`:
- Around line 398-411: Update the docstrings for emit_learnings_generated,
record_learnings_generated, and the ordinary emitter described near the
learning-ID parameters so they no longer claim resumable finalization uses the
non-strict helpers. Document ExtractionResumeWorker._record_finalized_learnings
as using emit_learnings_generated_records_strict, while preserving the existing
ownership and fail-open semantics for the ordinary paths.
In `@reflexio/server/services/extraction/resume_worker.py`:
- Around line 943-956: Align _record_finalized_learnings with the
FinalizationResult contract by either removing billing from the winner-only
side-effects wording in FinalizationResult’s docstring, or gating its invocation
on result.won_receipt; preserve the intended idempotent billing replay behavior
indicated by the existing test. Move emit_learnings_generated_records_strict
from the method-local import into the module-level billing_meter import
alongside ReceiptBillingDeliveryError.
- Around line 93-106: Update _finalization_failure_status so
ReceiptBillingDeliveryError cases other than REJECTED also check
next_attempt_count against max_finalization_attempts before returning
FINALIZATION_FAILED. Preserve immediate FAILED status for REJECTED, and return
FAILED when the attempt ceiling is reached for UNKNOWN and other non-rejected
receipt failures.
In `@reflexio/server/services/search_exposure.py`:
- Around line 172-183: Sort the exported names in __all__ alphabetically using
the project formatter, updating only the ordering in this list while preserving
all existing exports.
In `@reflexio/server/services/storage/session_outcome_identity.py`:
- Around line 11-17: Sort the exported names in __all__ alphabetically to
satisfy Ruff RUF022, preserving all existing entries and their spellings.
- Around line 165-188: The _canonical_trajectory_json function needs a
maximum-depth guard while recursively encoding mappings, tuples, and lists.
Track recursion depth and raise the established validation error when persisted
interaction JSON exceeds the permitted depth, ensuring trajectory_digest and
session-outcome finalization cannot hit RecursionError.
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1183-1187: Update the outcome_revision value in the INSERT
construction around the existing column-presence check to fall back to revision
1 when the column is missing or row["outcome_revision"] is NULL. Preserve
non-NULL stored revisions while ensuring legacy rows always satisfy the database
constraints.
In `@reflexio/server/services/storage/storage_base/agent_run/_agent_run_store.py`:
- Around line 27-34: Update the docstring for get_agent_run_finalization_receipt
to explicitly define the return contract: None means no receipt exists and
finalization should proceed, while an empty list means a receipt was committed
despite producing no durable learnings and must be treated as finalized.
Preserve this distinction across all backend implementations.
In `@reflexio/server/usage_metrics.py`:
- Around line 118-157: The fail-open wrapper currently logs warnings for
expected UNKNOWN delivery outcomes. In reflexio/server/usage_metrics.py lines
118-157, update record_usage_event to catch UsageEventDeliveryError separately,
suppress warnings when exc.status is UNKNOWN, log other statuses, add
exc_info=True to the generic recorder-exception handler, and document that
missing or legacy recorders are non-errors; in
tests/server/test_usage_metrics.py lines 31-46, add caplog tests covering both
no configured recorder and a legacy recorder returning None, verifying no
warning is emitted and the event reaches the legacy recorder.
In `@tests/server/api_endpoints/test_session_outcomes_integration.py`:
- Line 188: Update the retry response assertion to validate that
retry.json()["reason"] is None rather than asserting the key is absent. Keep the
existing conflict assertion that accesses the reason key so both tests verify
the response value consistently.
In `@tests/server/services/test_non_extraction_learning_metering.py`:
- Around line 45-51: Reset the module-level usage-event recorder after every
test in this file. Add an autouse pytest fixture near _capture_events that
yields for the test and calls configure_usage_event_recorder(None) in a finally
block, ensuring cleanup also occurs when assertions fail; keep _capture_events
responsible only for installing its recorder.
---
Nitpick comments:
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 829-867: Extract the shared outcome identity fields, lowercase
SHA-256 digest validation, and all-or-none identity check from
SessionOutcomeRecord and SetSessionOutcomeResponse into one reusable
OutcomeIdentityMixin/helper. Update both Pydantic models to reuse it and change
SessionOutcomeWriteResult.__post_init__ to call the same shared identity
validator, preserving partial-identity rejection and digest validation. Verify
serialized field ordering and expected dictionaries in the session outcome
identity tests.
In `@reflexio/server/billing_meter.py`:
- Around line 268-288: The _record_learnings_generated_records helper currently
accepts record_event as Any, bypassing signature validation. Define a Protocol
or Callable type describing the exact keyword arguments used by its record_event
call, and apply it to record_event so both record_usage_event and
record_usage_event_strict remain compatible and parameter drift is caught by
type checking.
In `@reflexio/server/services/base_generation_service.py`:
- Around line 727-729: Rename the _record_billing_learning_events keyword
parameter from generated_count to billable_count, use billable_count when
calling record_learnings_generated, and update both call sites, including
emit_generation_side_effects and the affected billing emission test.
In `@reflexio/server/services/governance/service.py`:
- Around line 418-449: Remove the tautological purge_id entry from
expected_identity in _matching_user_erasure_purge_for_retry, while preserving
validation of operation_type, scope_type, subject_ref, and request_ref and the
authoritative-user check.
In `@reflexio/server/services/playbook/service.py`:
- Around line 661-735: Extract the duplicated receipt protocol from
_finalize_extracted_items_with_outcome and
ProfileGenerationService._finalize_extracted_items_with_outcome into one shared
base-service or mixin helper. Parameterize it with entity_type, a callback that
derives learning IDs after _persist_write_plan, and an optional post-commit hook
for playbook scheduler dispatch; preserve the pre-transaction read,
transactional re-read/persist/receipt insert, conflict rollback signal, and
disappeared-receipt guard while making both services use the shared flow.
In `@reflexio/server/services/profile/service.py`:
- Around line 360-422: Extract the duplicated receipt-fencing logic from profile
`_finalize_extracted_items_with_outcome` and
`PlaybookGenerationService._finalize_extracted_items_with_outcome` into a shared
helper. Parameterize the helper with `entity_type`, a persist callback, and a
learning-ID callback so profiles can derive IDs before persistence while
playbooks derive them afterward; keep playbook scheduler dispatch outside the
helper. Preserve the pre-scope and in-scope receipt checks, conflict rollback,
post-conflict receipt reread, and missing-receipt `RuntimeError` behavior
exactly.
In
`@reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py`:
- Around line 92-94: Update the MRO-provided
_assert_purge_operation_execution_claim_locked callable annotation to require
PurgeExecutionClaim rather than allowing None in
reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py:92-94,
_rebuild_hide.py:85-87, and _subject_barrier.py:80-82. Keep the callable
signature as Callable[[str, PurgeExecutionClaim], None] at all three sites,
matching the implementation and public contract.
In `@reflexio/server/services/storage/sqlite_storage/governance/_purge.py`:
- Around line 449-497: Consolidate the duplicated validation in
assert_purge_operation_execution_claim and
_assert_purge_operation_execution_claim_locked by having the locked method
delegate to the shared assertion logic while preserving its locked row-fetch
behavior. Extract or reuse a common row-check helper so both paths apply the
same five claim conditions, with only the database fetch mechanism differing.
In
`@reflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.py`:
- Around line 188-215: Remove the redundant purge_row["scope_type"] != "user"
comparison from the composite validation condition after the initial guard. Keep
the existing scope validation at the start of the completion flow and preserve
all other authoritative identity checks unchanged.
In `@tests/models/test_session_outcome_identity.py`:
- Around line 133-135: Annotate the golden digest assertion for
trajectory_digest with a comment describing that it covers
CanonicalSessionTrajectory and instructing maintainers to review and
intentionally update the value when the canonical shape or
_canonical_trajectory_json changes. Also add an explicit schema_version to the
canonical_session_trajectory payload, following the existing versioning pattern,
and ensure intentional version bumps make persisted digest changes diagnosable
rather than causing opaque retry conflicts.
In `@tests/server/api_endpoints/test_session_outcomes_integration.py`:
- Around line 232-245: Update test_retry_rejects_invalid_stored_metadata to
parameterize an explicit identifier alongside each stored_metadata value, then
derive session_id from that identifier instead of len(stored_metadata). Preserve
the existing metadata cases and use unique parameter ids such as empty,
malformed, and pathological-nesting to prevent collisions.
In `@tests/server/services/governance/test_governance_local_e2e.py`:
- Around line 963-966: Update release_winner_on_duplicate_poll to use
time.sleep(0.001) instead of creating and waiting on a throwaway
threading.Event, and add the required time import at the file’s imports.
- Around line 1052-1110: Document the fake-clock invariant in
test_healthy_slow_lifecycle_renews_lease_and_duplicate_converges: state that the
claim begins at 100 with a 300-second lease expiring at 400, three renewals
extend it to 650, and advancing to 401 keeps the duplicate caller within the
live lease. Place the comment next to the relevant fake_now updates and renewal
wait.
- Around line 1044-1049: Update the backoff assertions in the governance erase
retry test to derive expected initial, maximum, and total delays from
governance_service_module’s _DUPLICATE_ERASE_POLL_SECONDS,
_DUPLICATE_ERASE_MAX_POLL_SECONDS, and _DUPLICATE_ERASE_WAIT_SECONDS constants.
Preserve the existing checks for doubling behavior, maximum delay, and total
wait bound without hardcoded duplicate values.
In `@tests/server/services/governance/test_subject_write_barrier_sqlite.py`:
- Around line 622-629: Remove the redundant
get_subject_write_barrier(subject_ref) call in this test, keeping a single
barrier fetch and its associated non-null and status assertions; preserve the
stored_purge retrieval and its independent assertions.
- Around line 42-62: Add a concise comment in `_claim_purge` documenting that it
deliberately expires and steals any existing execution lease, increments the
fence, and makes claims from earlier calls stale; state that callers must use
each returned claim before invoking `_claim_purge` again.
In `@tests/server/services/lineage/test_gc_scheduler_global_sweep.py`:
- Around line 30-36: Update the fixture cleanup to import and call
gc_scheduler.clear_always_global_sweeps directly, removing the getattr guard and
clear_always variable; likewise import register_always_global_sweep directly
where the fixture or tests use it, so missing helpers fail loudly while
preserving cleanup before and after each test.
- Line 143: Update the assertions in the global sweep scheduler tests, including
the assertion around scheduler._run_once() and the one at the referenced second
location, to compare against _BOOTSTRAP_RETRY_INTERVAL_SECONDS and
_DEFAULT_POLL_INTERVAL_SECONDS from gc_scheduler.py instead of numeric literals.
Import both constants from the scheduler module and preserve the existing
expected-value behavior.
In `@tests/server/services/storage/sqlite_storage/test_governance_storage.py`:
- Around line 1677-1704: Strengthen the production scan in the test around
production_root by asserting that the derived root exists and is a directory
before scanning. Track the files yielded by production_root.rglob("*.py") and
assert that the scan visited at least one file, while preserving the existing
violation checks.
- Around line 218-238: Document the lease-takeover behavior in the test helper
_claim_purge: add a docstring stating that when a live lease exists it
force-expires that lease, reclaims the purge, and invalidates previously issued
claims for the same purge_id. Keep the existing claiming logic unchanged.
In
`@tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py`:
- Around line 199-254: Update the migration test around
_migrate_session_outcomes_schema to derive the expected trajectory_input_queries
count from the shared chunk-size constant, accounting for two query types per
chunk (for example, ceil(row_count / chunk_size) * 2). Replace the hard-coded
501/4 relationship while preserving the existing row-count assertion.
In `@tests/server/services/storage/test_storage_contract_clear_user_data.py`:
- Around line 176-186: Remove the unused monkeypatch parameter from the affected
test and delete the REFLEXIO_GOVERNANCE_REF_SECRET setenv call; keep the
explicit "test-governance-secret" arguments to governance_subject_ref and the
existing SQLiteStorage setup unchanged.
In `@tests/server/services/storage/test_storage_contract_session_outcomes.py`:
- Around line 100-102: Update the four SQLite-specific tests in this contract
module, including
test_legacy_all_null_identity_exact_retry_uses_available_context, to assert
isinstance(storage, SQLiteStorage) before accessing SQLite-only attributes such
as conn, then use the narrowed storage directly instead of cast. Follow the
guard pattern from test_storage_contract_clear_user_data.py so non-SQLite
backends fail clearly or skip rather than raising an opaque AttributeError.
In `@tests/server/test_billing_meter.py`:
- Around line 123-146: Extend
test_strict_receipt_billing_preserves_delivery_status to cover a plain
RuntimeError from record_usage_event_strict, alongside the existing
UsageEventDeliveryError cases. Assert that
emit_learnings_generated_records_strict raises ReceiptBillingDeliveryError with
status UsageEventDeliveryStatus.UNKNOWN.
In `@tests/server/test_usage_metrics.py`:
- Around line 31-46: Add fail-open coverage in
tests/server/test_usage_metrics.py for record_usage_event: add one test with no
recorder configured and another with a legacy recorder returning None, using
caplog at WARNING level to assert no warnings are emitted; verify the latter
still delivers the event and always reset the recorder configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/storage/session_outcome_identity.py`:
- Around line 167-183: Update _canonical_trajectory_json so list, tuple, and
mapping containers are rejected when depth >=
MAX_CANONICAL_TRAJECTORY_JSON_DEPTH, before traversing their contents; retain
scalar handling at the existing boundary. Add a regression test covering nested
empty containers and verify the same limit applies consistently to sequences and
mappings.
🪄 Autofix
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: 05dad9a2-62a0-4c36-be08-59c34adc074e
📒 Files selected for processing (15)
reflexio/server/api.pyreflexio/server/billing_meter.pyreflexio/server/services/deferred_learning_plan.pyreflexio/server/services/extraction/resume_worker.pyreflexio/server/services/search_exposure.pyreflexio/server/services/storage/session_outcome_identity.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/storage_base/agent_run/_agent_run_store.pyreflexio/server/usage_metrics.pytests/models/test_session_outcome_identity.pytests/server/api_endpoints/test_session_outcomes_integration.pytests/server/services/extraction/test_resume_worker.pytests/server/services/storage/sqlite_storage/test_session_outcome_migration.pytests/server/test_create_app_capabilities.pytests/server/test_usage_metrics.py
🚧 Files skipped from review as they are similar to previous changes (10)
- reflexio/server/api.py
- reflexio/server/usage_metrics.py
- reflexio/server/services/deferred_learning_plan.py
- tests/server/test_usage_metrics.py
- tests/models/test_session_outcome_identity.py
- reflexio/server/services/search_exposure.py
- reflexio/server/billing_meter.py
- reflexio/server/services/storage/sqlite_storage/_base.py
- tests/server/api_endpoints/test_session_outcomes_integration.py
- tests/server/services/extraction/test_resume_worker.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
Persist non-empty direct user-playbook search results through the canonical synchronous exposure boundary before metering or response release.
Align direct user-playbook search admission with the 100-event exposure contract and publish the bound through shared client metadata.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/README.md`:
- Around line 103-104: Update create_app to register a default
SEARCH_EXPOSURE_RECORDER for every authenticated deployment, ensuring
record_search_exposures persists exposure evidence before successful responses.
If universal registration is not intended, revise the README guarantee to
explicitly apply only to deployments that configure the recorder.
🪄 Autofix
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: 2cf8534c-559c-4d1e-89fd-35f1a162c365
📒 Files selected for processing (9)
docs/lib/methods/user-playbooks.tsreflexio/client/client.pyreflexio/models/api_schema/retriever_schema.pyreflexio/server/README.mdreflexio/server/routes/search.pyreflexio/server/services/README.mdreflexio/server/services/search_exposure.pytests/client/test_search.pytests/server/routes/test_search_exposure_boundary.py
🚧 Files skipped from review as they are similar to previous changes (4)
- reflexio/client/client.py
- tests/client/test_search.py
- reflexio/models/api_schema/retriever_schema.py
- reflexio/server/services/search_exposure.py
Clarify that authenticated enterprise capability wiring supplies the fail-closed recorder while shared create_app intentionally has no default implementation.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Why this replacement PR
The earlier foundation PRs were reverted after local end-to-end and evaluation failures. This branch was rebuilt from current main rather than reopening the reverted history. It keeps the independent mainline fixes and adds regression coverage for the failures found during the rebuild.
This PR must remain unmerged until the replacement enterprise PR restores PostgreSQL, Supabase, and PostgREST adapter parity and the complete stacked end-to-end matrix is green.
Verification
Prior PRs
Summary by CodeRabbit