feat: add durable session outcome tracking - #388
Conversation
📝 WalkthroughWalkthroughSession outcome recording and querying are added across schemas, client APIs, service facades, FastAPI routes, SQLite storage, and governance erasure. Validation covers timing, metadata, pagination, duplicate context, acceptance hooks, and non-finite values. ChangesSession outcomes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant publisher_api
participant Reflexio
participant SQLiteStorage
Client->>FastAPI: POST outcome or outcome query
FastAPI->>publisher_api: Scope payload to organization
publisher_api->>Reflexio: Invoke session outcome operation
Reflexio->>SQLiteStorage: Validate and persist or query
SQLiteStorage-->>Reflexio: Return result
Reflexio-->>Client: Return response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 1
🧹 Nitpick comments (6)
reflexio/server/api.py (1)
139-160: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNon-finite detection only inspects
input, notctx.
_contains_non_finite_numberis only applied toerror.get("input"). If a validation error's non-finite value only surfaces viactx(Pydantic's context dict for some error types) rather thaninput, detection would miss it and the request would fall through to the default handler, which still serializes the rawctx. In practicectxvalues are typically schema-defined constants rather than user input, so risk is low, but worth a quick sanity check given this handler is global (applies to every route, not just session outcomes).🤖 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/api.py` around lines 139 - 160, The validation error scan in _safe_request_validation_exception_handler only checks each error’s input and can miss non-finite values nested in ctx. Extend the detection to inspect both error.get("input") and error.get("ctx"), while preserving the existing default-handler path when neither contains a non-finite number.reflexio/lib/_session_outcome.py (1)
53-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBroad
except Exceptioncollapses all failures — including acceptance-provider bugs — intoSTORAGE_ERROR.If
provider(...)(an externally-registeredSESSION_OUTCOME_ACCEPTANCEhook) raises due to a bug rather than a real storage failure, the caller and logs both reportSTORAGE_ERROR, obscuring the true failure source during incident response. Consider catching storage-specific exceptions around the storage calls only, or otherwise annotating the log/response with which stage failed.♻️ Possible refinement
- try: - for _attempt in range(3): - context = storage.get_session_outcome_context(request.session_id) + try: + for _attempt in range(3): + try: + context = storage.get_session_outcome_context(request.session_id) + except Exception: + logger.exception("Storage error while fetching session outcome context") + raise ... provider = get_service(SESSION_OUTCOME_ACCEPTANCE) if provider is not None: - reason = provider( - self.org_id, request, received_at, context.user_id - ) + try: + reason = provider( + self.org_id, request, received_at, context.user_id + ) + except Exception: + logger.exception("Session outcome acceptance provider raised") + raise🤖 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/lib/_session_outcome.py` around lines 53 - 133, Limit the broad exception handling in the session outcome flow around the storage operations in the method containing the shown retry loop, so exceptions from the externally registered SESSION_OUTCOME_ACCEPTANCE provider are not converted to STORAGE_ERROR. Catch and log storage-specific failures only, while allowing provider exceptions to propagate or be reported distinctly; preserve the existing STORAGE_ERROR response for actual storage failures.reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py (1)
271-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented asymmetry:
session_outcomesis the only count conditionally omitted.Every other key in this dict (
interactions,requests, etc.) is always included, even when zero.session_outcomesis only included whenrowcountis truthy. It's harmless today becauseapply_governance_user_data_deletereads it viacounts.get(key, 0), but the asymmetry is easy to "fix" during a future refactor in a way that would break the exact-dict-equality assertion intest_apply_governance_user_data_delete_retains_lineage_skeleton. A short comment explaining why this key is special would prevent that regression.📝 Suggested comment
return { + # Omitted when zero (unlike other keys) to preserve the pre-existing + # exact-dict-equality assertions in governance storage tests that + # predate session outcomes; `counts.get(key, 0)` in + # apply_governance_user_data_delete tolerates the omission. **( {"session_outcomes": session_outcomes_cur.rowcount} if session_outcomes_cur.rowcount else {} ),🤖 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 271 - 276, Add a concise comment immediately above the conditional session_outcomes entry in the return dict, documenting that its omission when rowcount is zero is intentional and must remain compatible with apply_governance_user_data_delete and its exact-dict equality expectations. Do not change the existing counting behavior.tests/server/services/storage/test_storage_contract_session_outcomes.py (1)
12-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
get_session_outcome_context's ownexistingflag.The duplicate-rejection assertions here (
test_first_write_preserves_outcome_fields) reuse acontextcaptured before the first write, so they only exerciserecord_session_outcome's internal existing-row check — not whetherget_session_outcome_context()itself reportsexisting=Trueafter a successful write. That flag is whatmark_session_outcome(lib layer) uses to short-circuit before callingrecord_session_outcomeat all, so it's a distinct, currently-untested code path in this contract suite.Consider adding a case that calls
get_session_outcome_context(session_id)again after a successfulrecord_session_outcomeand assertsexisting is True.🤖 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 12 - 63, Extend test_first_write_preserves_outcome_fields by calling get_session_outcome_context("s1") after the successful first record_session_outcome call and assert that the returned context has existing set to True, covering the post-write context behavior separately from duplicate rejection.reflexio/server/services/storage/sqlite_storage/_base.py (1)
1996-2014: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider enforcing the one-outcome-per-session invariant at the DB layer.
The PK is
(user_id, session_id), but every application check (get_session_outcome_context/record_session_outcomein_session_outcomes.py) looks up and enforces "first write wins" purely bysession_id, never by the(user_id, session_id)pair. The composite PK doesn't itself block two rows for the samesession_idwith differentuser_id— that's currently prevented only by theBEGIN IMMEDIATEcheck-then-insert transaction. Since this table is new, makingsession_idthe PK (or addingUNIQUE(session_id)) would make the intended invariant DB-enforced too, at no migration cost.♻️ Proposed schema tweak
CREATE TABLE IF NOT EXISTS session_outcomes ( user_id TEXT NOT NULL, - session_id TEXT NOT NULL, + session_id TEXT NOT NULL PRIMARY KEY, outcome TEXT NOT NULL CHECK (outcome IN ('success', 'failure')), occurred_at INTEGER NOT NULL, source TEXT NOT NULL, label TEXT, value REAL, metadata TEXT, governance_subject_ref TEXT NOT NULL, - created_at INTEGER NOT NULL, - PRIMARY KEY (user_id, session_id) + created_at INTEGER NOT 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/server/services/storage/sqlite_storage/_base.py` around lines 1996 - 2014, Update the session_outcomes schema definition so session_id is uniquely constrained independently of user_id, replacing the composite PRIMARY KEY or adding a UNIQUE constraint on session_id. Preserve the existing columns and indexes while ensuring the database enforces one outcome per session across all users.reflexio/server/services/storage/sqlite_storage/_session_outcomes.py (1)
66-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
record_session_outcome()before starting a transaction
record_session_outcome()unconditionally issuesBEGIN IMMEDIATE, so it will raisecannot start a transaction within a transactionif it’s ever called from an existing transaction. Mirror the_own_transaction()guard used in_requests.pyso this write path composes safely withcommit_scope().🤖 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/_session_outcomes.py` around lines 66 - 153, Update record_session_outcome() to use the same _own_transaction() guard pattern as _requests.py before issuing BEGIN IMMEDIATE, so it joins existing commit_scope() transactions instead of starting a nested transaction. Preserve the current commit/rollback behavior for transactions it owns, while avoiding rollback or commit of caller-owned transactions.
🤖 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/routes/interactions.py`:
- Around line 76-86: Add the established `@limiter.limit`(...) decorator to the
get_session_outcomes route, matching the rate-limit pattern and limit used by
the other interactions POST endpoints. Keep the existing response model,
dependency, and publisher_api.get_session_outcomes call unchanged.
---
Nitpick comments:
In `@reflexio/lib/_session_outcome.py`:
- Around line 53-133: Limit the broad exception handling in the session outcome
flow around the storage operations in the method containing the shown retry
loop, so exceptions from the externally registered SESSION_OUTCOME_ACCEPTANCE
provider are not converted to STORAGE_ERROR. Catch and log storage-specific
failures only, while allowing provider exceptions to propagate or be reported
distinctly; preserve the existing STORAGE_ERROR response for actual storage
failures.
In `@reflexio/server/api.py`:
- Around line 139-160: The validation error scan in
_safe_request_validation_exception_handler only checks each error’s input and
can miss non-finite values nested in ctx. Extend the detection to inspect both
error.get("input") and error.get("ctx"), while preserving the existing
default-handler path when neither contains a non-finite number.
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1996-2014: Update the session_outcomes schema definition so
session_id is uniquely constrained independently of user_id, replacing the
composite PRIMARY KEY or adding a UNIQUE constraint on session_id. Preserve the
existing columns and indexes while ensuring the database enforces one outcome
per session across all users.
In `@reflexio/server/services/storage/sqlite_storage/_session_outcomes.py`:
- Around line 66-153: Update record_session_outcome() to use the same
_own_transaction() guard pattern as _requests.py before issuing BEGIN IMMEDIATE,
so it joins existing commit_scope() transactions instead of starting a nested
transaction. Preserve the current commit/rollback behavior for transactions it
owns, while avoiding rollback or commit of caller-owned transactions.
In
`@reflexio/server/services/storage/sqlite_storage/governance/_erase_execution.py`:
- Around line 271-276: Add a concise comment immediately above the conditional
session_outcomes entry in the return dict, documenting that its omission when
rowcount is zero is intentional and must remain compatible with
apply_governance_user_data_delete and its exact-dict equality expectations. Do
not change the existing counting behavior.
In `@tests/server/services/storage/test_storage_contract_session_outcomes.py`:
- Around line 12-63: Extend test_first_write_preserves_outcome_fields by calling
get_session_outcome_context("s1") after the successful first
record_session_outcome call and assert that the returned context has existing
set to True, covering the post-write context behavior separately from duplicate
rejection.
🪄 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: f34e6836-4e86-4521-94a0-e324c60bf07d
📒 Files selected for processing (26)
docs/lib/methods/requests-sessions.tsreflexio/client/client.pyreflexio/lib/_session_outcome.pyreflexio/lib/reflexio_lib.pyreflexio/models/api_schema/domain/entities.pyreflexio/models/api_schema/domain/enums.pyreflexio/server/README.mdreflexio/server/api.pyreflexio/server/api_endpoints/README.mdreflexio/server/api_endpoints/publisher_api.pyreflexio/server/routes/interactions.pyreflexio/server/services/storage/governance_validation.pyreflexio/server/services/storage/sqlite_storage/__init__.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_governance.pyreflexio/server/services/storage/sqlite_storage/_requests.pyreflexio/server/services/storage/sqlite_storage/_session_outcomes.pyreflexio/server/services/storage/sqlite_storage/governance/_erase_execution.pyreflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_session_outcomes.pytests/client/test_session_outcomes_client.pytests/server/api_endpoints/test_session_outcomes_integration.pytests/server/services/storage/sqlite_storage/test_governance_storage.pytests/server/services/storage/test_storage_contract_session_outcomes.pytests/server/test_app_route_inventory.py
1 participant
Summary
Changes
Test Plan
uv run pytest tests/client/test_session_outcomes_client.py tests/server/api_endpoints/test_session_outcomes_integration.py tests/server/services/storage/test_storage_contract_session_outcomes.py tests/server/test_app_route_inventory.py -q -o 'addopts='uv run pytest -q --no-cov tests/server/api_endpoints/test_session_outcomes_integration.py(9 passed)reflexio.server.routes.interactions.get_session_outcomesas60 per 1 minuteuv run ruff checkanduv run ruff format --checkover all changed Python filesuv run pyrightover all changed Python filesnpx biome check lib/methods/requests-sessions.tsnpx tsc --noEmitindocs/Summary by CodeRabbit
422responses.