feat: select open-world tuning evidence - #444
Conversation
|
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 changes separate strict source validation from persisted source preservation, add streaming session trajectory digests, bound SQLite migration reads, and update session outcome finalization. They also harden worker startup, timeout routing, sweep registration, and retention configuration. ChangesSession outcome contracts and persistence
Runtime safeguards
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StorageClient
participant SQLiteStorage
participant CanonicalTrajectoryDigestAccumulator
StorageClient->>SQLiteStorage: finalize session outcome
SQLiteStorage->>SQLiteStorage: read ordered request and interaction rows
SQLiteStorage->>CanonicalTrajectoryDigestAccumulator: stream rows in bounded batches
CanonicalTrajectoryDigestAccumulator-->>SQLiteStorage: return canonical digest
SQLiteStorage-->>StorageClient: return finalized outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
36a642c to
4a9b4ef
Compare
|
@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: 1
🧹 Nitpick comments (3)
reflexio/server/services/storage/sqlite_storage/_session_outcomes.py (1)
93-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign
sourcenormalization with the writer.Line 95 computes
str(first["source"]). The writer computesstr(first["source"] or "")at Line 244. Ifrequests.sourceis ever NULL, the reader produces the string"None"and the writer produces"". The equality check at Line 248 then fails andrecord_session_outcomereturnscontext_changed=Truefor every attempt on that session.The current
requestsschema declaressource TEXT NOT NULL DEFAULT '', so NULL is not reachable today. Applying the same normalization in both places removes the asymmetry.♻️ Proposed change
return SessionOutcomeContext( user_id=str(first["user_id"]), - source=str(first["source"]), + source=str(first["source"] or ""), first_request_at=_iso_to_epoch(first["created_at"]),🤖 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 93 - 99, Update the source assignment in SessionOutcomeContext construction to normalize NULL values the same way as record_session_outcome: convert first["source"] or an empty string to str. Preserve the existing source value behavior for non-NULL inputs and align it with the writer’s normalization.tests/server/services/storage/test_storage_contract_session_outcomes.py (1)
419-420: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe fetch-size assertion covers only the conflict call.
Line 419 rebinds
guarded_connectionto a new_NoFetchAllConnection. That instance owns a freshfetch_sizeslist. The assertions at Lines 437-438 therefore inspect only the thirdrecord_session_outcomecall. The sizes recorded during the first and retry calls are discarded.The no-
fetchallguard still applies to all three calls, so the main invariant holds. If you want the batch-size assertion to cover every call, keep both wrappers and assert over the combined list.♻️ Proposed change
- guarded_connection = _NoFetchAllConnection(raw_connection) - cast(Any, sqlite_storage).conn = guarded_connection + conflict_connection = _NoFetchAllConnection(raw_connection) + cast(Any, sqlite_storage).conn = conflict_connection try: conflict = storage.record_session_outcome( outcome, created_at=503, expected_context=storage.get_session_outcome_context(session_id), ) finally: cast(Any, sqlite_storage).conn = raw_connection- assert guarded_connection.fetch_sizes - assert len(set(guarded_connection.fetch_sizes)) == 1 + all_fetch_sizes = guarded_connection.fetch_sizes + conflict_connection.fetch_sizes + assert all_fetch_sizes + assert len(set(all_fetch_sizes)) == 1Also applies to: 437-438
🤖 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 419 - 420, Update the test setup around _NoFetchAllConnection and the fetch_sizes assertions so size records from the initial, retry, and conflict record_session_outcome calls are preserved and checked together. Keep the no-fetchall guard active for every call, but avoid replacing the earlier wrapper’s fetch_sizes collection with a fresh list; aggregate both wrappers’ recorded sizes before the assertions.tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py (1)
395-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected trajectory-query count from both chunk sizes
_prefetch_canonical_session_trajectory_digestsusesRETENTION_DELETE_CHUNK(500), while migration batches use size 256. The 501 test rows therefore produce two queries today. IfRETENTION_DELETE_CHUNKis lowered below 256, the assertion fails even though the migration remains correct. Derive the expected count from both constants, or assert the required lower bound.🤖 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 395 - 402, Update the trajectory_input_queries assertion in the migration test to derive the expected query count from both RETENTION_DELETE_CHUNK and the migration batch size, or assert only the required lower bound. Keep validating that every matching query includes LEFT JOIN interactions while avoiding a hard-coded count tied to current chunk sizes.Source: Linters/SAST tools
🤖 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 `@tests/models/test_session_outcome_identity.py`:
- Around line 881-884: Update the match pattern in the pytest.raises call to use
a raw string literal, preserving the existing “canonical trajectory digest
accumulator is invalid$” regex unchanged.
---
Nitpick comments:
In `@reflexio/server/services/storage/sqlite_storage/_session_outcomes.py`:
- Around line 93-99: Update the source assignment in SessionOutcomeContext
construction to normalize NULL values the same way as record_session_outcome:
convert first["source"] or an empty string to str. Preserve the existing source
value behavior for non-NULL inputs and align it with the writer’s normalization.
In
`@tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py`:
- Around line 395-402: Update the trajectory_input_queries assertion in the
migration test to derive the expected query count from both
RETENTION_DELETE_CHUNK and the migration batch size, or assert only the required
lower bound. Keep validating that every matching query includes LEFT JOIN
interactions while avoiding a hard-coded count tied to current chunk sizes.
In `@tests/server/services/storage/test_storage_contract_session_outcomes.py`:
- Around line 419-420: Update the test setup around _NoFetchAllConnection and
the fetch_sizes assertions so size records from the initial, retry, and conflict
record_session_outcome calls are preserved and checked together. Keep the
no-fetchall guard active for every call, but avoid replacing the earlier
wrapper’s fetch_sizes collection with a fresh list; aggregate both wrappers’
recorded sizes before the assertions.
🪄 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: ad85775a-5934-4a8c-b1d0-e2cd46714909
📒 Files selected for processing (19)
AI_AGENT_INTEGRATION.mdreflexio/models/api_schema/domain/entities.pyreflexio/models/api_schema/validators.pyreflexio/server/middleware.pyreflexio/server/services/lineage/gc_scheduler.pyreflexio/server/services/search_metering_worker.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/retention_mixin.pyreflexio/server/services/storage/session_outcome_identity.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_requests.pyreflexio/server/services/storage/sqlite_storage/_session_outcomes.pytests/models/test_session_outcome_identity.pytests/server/services/storage/sqlite_storage/test_session_outcome_migration.pytests/server/services/storage/test_storage_contract_requests.pytests/server/services/storage/test_storage_contract_session_outcomes.pytests/server/services/test_search_metering_worker.pytests/server/test_api_security_middleware.pytests/server/test_create_app_capabilities.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py (1)
113-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
__getattr__does not cover implicit dunder lookups.Python looks up special methods on the type, not the instance, so
__getattr__will not forward__iter__or__next__to the wrapped cursor. If the migration code ever iterates the cursor directly (for row in cursor), the wrapper raisesTypeErrorinstead of streaming, and the failure will look unrelated to the guard. Adding an explicit__iter__that raises the sameAssertionErrorasfetchallkeeps the intent of the double clear.♻️ Optional hardening of the cursor double
def fetchall(self) -> Any: raise AssertionError("trajectory migration must not call fetchall") + def __iter__(self) -> Any: + raise AssertionError("trajectory migration must not iterate the cursor") + def fetchmany(self, size: int) -> Any:🤖 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 113 - 127, Update _NoTrajectoryFetchAllCursor with an explicit __iter__ method that raises the same AssertionError as fetchall, ensuring direct cursor iteration fails with the intended guard instead of relying on __getattr__.tests/server/services/storage/test_storage_contract_session_outcomes.py (1)
680-702: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe mock relies on a fixed
executecall order.
connection.execute.side_effectsupplies exactly three cursors in a fixed order. Any added or reordered query insideget_session_outcome_contextraisesStopIterationinstead of a clear failure. Consider keying the cursors by statement text so the test fails with a readable message and survives benign query additions.🤖 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 680 - 702, Update test_sqlite_context_normalizes_nullable_request_source to replace the positional connection.execute.side_effect list with a statement-aware dispatcher that returns each mocked cursor based on the SQL statement, raising a clear assertion for unexpected queries. Preserve the existing cursor results and assertions while making the test resilient to query additions or reordering in get_session_outcome_context.
🤖 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.
Nitpick comments:
In
`@tests/server/services/storage/sqlite_storage/test_session_outcome_migration.py`:
- Around line 113-127: Update _NoTrajectoryFetchAllCursor with an explicit
__iter__ method that raises the same AssertionError as fetchall, ensuring direct
cursor iteration fails with the intended guard instead of relying on
__getattr__.
In `@tests/server/services/storage/test_storage_contract_session_outcomes.py`:
- Around line 680-702: Update
test_sqlite_context_normalizes_nullable_request_source to replace the positional
connection.execute.side_effect list with a statement-aware dispatcher that
returns each mocked cursor based on the SQL statement, raising a clear assertion
for unexpected queries. Preserve the existing cursor results and assertions
while making the test resilient to query additions or reordering in
get_session_outcome_context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1980a3fb-3235-4955-8bb8-1a37222bfcd4
📒 Files selected for processing (4)
reflexio/server/services/storage/sqlite_storage/_session_outcomes.pytests/models/test_session_outcome_identity.pytests/server/services/storage/sqlite_storage/test_session_outcome_migration.pytests/server/services/storage/test_storage_contract_session_outcomes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- reflexio/server/services/storage/sqlite_storage/_session_outcomes.py
|
Addressed both current-head CodeRabbit nitpicks in bb9f6ae: the cursor double now rejects implicit iteration explicitly, and the nullable-source test dispatches mocked cursors by SQL statement. Focused verification: 27 passed; Ruff, format, and Pyright clean. @coderabbitai review |
|
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The two review-body nitpicks are already addressed in current head |
1 participant
Summary
Changes
Test Plan
nice -n 10 uv run python -m reflexio_ext.scripts.phase2_evidence_gatefrom the enterprise checkout: 693 passed, 14 expected storage-specific skips.Summary by CodeRabbit
New Features
Bug Fixes
Documentation