feat(evaluation): judge retrieved learnings against their original rows - #338
Merged
Conversation
Retrieved-learning evaluation (RLE) judged only learnings that were still *retrieval-eligible* at judge time — status NULL, unexpired, and (for agent playbooks) approved. That is the wrong question. A learning that was injected into a response and then archived, expired, or un-approved was still the exact learning the agent saw, so dropping it silently discards a verdict about work that actually happened. Worse, a lifecycle change landing mid-run (aggregation or consolidation completing while the LLM judges are in flight) could make an in-flight verdict vanish at commit. Reframe the invariant from "still retrievable" to "the original row still exists": - Add `include_inactive` to the three bulk by-id getters. It returns every matching owned row regardless of lifecycle status, expiry, or approval. It is a strict superset of the existing `include_tombstones` flag on the singular by-id getters (which only unhides MERGED/SUPERSEDED for lineage walks); `user_id` scoping still applies, and the default preserves retrieval behavior. - Rename the RLE commit-path check `_rle_eligible_refs` -> `_rle_resolvable_refs` and drop its lifecycle predicates: it now locks and rechecks row *existence* only. The `attached AND resolvable` commit contract is unchanged. - Resolution never follows a lineage pointer to a successor — the judge sees the original content, never the row that replaced it. Combining `include_inactive` with an explicit `status_filter` is contradictory: the filter would be silently dropped and the caller would get back rows it asked to exclude. `validate_include_inactive` rejects that combination, raising StorageError specifically because both `handle_exceptions` decorators re-raise StorageError untouched while wrapping any other exception type (and, on the Supabase path, reporting it to Sentry) — a caller bug should fail loud and identically on every backend, not become error-report noise. Resolution is best-effort by design: row retention evicts tombstones first, so a genuinely-served learning can age out and then resolve to nothing. An old session simply judges fewer learnings. Tests: contract coverage for `include_inactive` on all three getters, including that `user_id` remains the only surviving tenant wall under the flag and that the contradictory filter combination is rejected; evaluator tests that an archived, expired, or never-approved attached learning is still judged, and that a superseded learning is judged on its original content rather than its successor's.
📝 WalkthroughWalkthroughThe PR adds historical-resolution modes to bulk storage getters, changes retrieved-learning evaluation from lifecycle eligibility to underlying-row existence, and updates candidate resolution and contract tests for archived, expired, unapproved, superseded, and missing learnings. ChangesHistorical retrieved-learning resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Evaluator as RetrievedLearningEvaluator
participant Profiles as ProfileStore
participant UserPlaybooks as UserPlaybookStore
participant AgentPlaybooks as AgentPlaybookStore
participant EvalStore as RetrievedLearningEvaluationStore
Evaluator->>Profiles: Resolve attached profiles including inactive rows
Evaluator->>UserPlaybooks: Resolve attached user playbooks including inactive rows
Evaluator->>AgentPlaybooks: Resolve attached agent playbooks including inactive rows
Evaluator->>EvalStore: Replace retrieved-learning evaluation results
EvalStore->>EvalStore: Keep attached references with existing underlying rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
reflexio/server/services/storage/sqlite_storage/playbook/_user.py (1)
544-574: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
# noqa: S608to theinclude_inactivequery for consistency.Ruff flags this as a possible SQL injection vector, but it's a false positive —
phis a string of?placeholders and all values are bound as parameters. The existingget_user_playbooks_by_ids_any_useron line 761 of this same file uses# noqa: S608for the identical pattern. Adding it here keeps the file consistent and silences the linter.♻️ Proposed fix
if include_inactive: rows = self._fetchall( "SELECT * FROM user_playbooks " - f"WHERE user_id = ? AND user_playbook_id IN ({ph})", + f"WHERE user_id = ? AND user_playbook_id IN ({ph})", # noqa: S608 (user_id, *user_playbook_ids), )🤖 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/playbook/_user.py` around lines 544 - 574, Add an inline # noqa: S608 suppression to the include_inactive SQL query in get_user_playbooks_by_ids, matching the existing suppression in get_user_playbooks_by_ids_any_user; leave the parameterized query behavior unchanged.Source: Linters/SAST tools
reflexio/server/services/storage/sqlite_storage/profiles/_profile_store.py (1)
674-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
# noqa: S608breaks the "Ruff clean" claim.The new f-string query is safe (only placeholder count is interpolated, values are parameterized), but this exact pattern is suppressed with
# noqa: S608elsewhere in this file (e.g.delete_profiles_by_ids). This line lacks that suppression and will be flagged by Ruff.🔧 Proposed fix
if include_inactive: rows = self._fetchall( - f"SELECT * FROM profiles WHERE user_id = ? AND profile_id IN ({ph})", + f"SELECT * FROM profiles WHERE user_id = ? AND profile_id IN ({ph})", # noqa: S608 (user_id, *profile_ids), ) return [_row_to_profile(r) for r in rows]🤖 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/profiles/_profile_store.py` around lines 674 - 680, Add the file’s established `# noqa: S608` suppression to the interpolated SQL statement in the `include_inactive` branch of the profile-fetching method, matching the existing pattern used by `delete_profiles_by_ids`.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.
Nitpick comments:
In `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py`:
- Around line 544-574: Add an inline # noqa: S608 suppression to the
include_inactive SQL query in get_user_playbooks_by_ids, matching the existing
suppression in get_user_playbooks_by_ids_any_user; leave the parameterized query
behavior unchanged.
In `@reflexio/server/services/storage/sqlite_storage/profiles/_profile_store.py`:
- Around line 674-680: Add the file’s established `# noqa: S608` suppression to
the interpolated SQL statement in the `include_inactive` branch of the
profile-fetching method, matching the existing pattern used by
`delete_profiles_by_ids`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08ff1e32-036e-4029-b6b0-d0bf434c716e
📒 Files selected for processing (13)
reflexio/server/services/agent_success_evaluation/components/retrieved_learning_evaluator.pyreflexio/server/services/storage/lifecycle_filters.pyreflexio/server/services/storage/sqlite_storage/playbook/_agent.pyreflexio/server/services/storage/sqlite_storage/playbook/_eval_results.pyreflexio/server/services/storage/sqlite_storage/playbook/_user.pyreflexio/server/services/storage/sqlite_storage/profiles/_profile_store.pyreflexio/server/services/storage/storage_base/playbook/_agent.pyreflexio/server/services/storage/storage_base/playbook/_user.pyreflexio/server/services/storage/storage_base/profiles/_profile_store.pytests/server/services/agent_success_evaluation/test_retrieved_learning_evaluator.pytests/server/services/storage/test_storage_contract_playbook.pytests/server/services/storage/test_storage_contract_profiles.pytests/server/services/storage/test_storage_contract_retrieved_learning_evals.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Retrieved-learning evaluation (RLE) judged only learnings that were still retrieval-eligible at judge time —
status IS NULL, unexpired, and (for agent playbooks)approved. That asks the wrong question.A learning that was injected into a response and then archived, expired, or un-approved is still the exact learning the agent saw. Filtering it out silently discards a verdict about work that actually happened. It also created a race: a lifecycle change landing mid-run — aggregation or consolidation completing while the LLM judges are in flight — could make an in-flight verdict vanish at commit time.
This reframes the invariant from "is it still retrievable?" to "does the original row still exist?".
Changes
include_inactiveon the three bulk by-id getters (get_profiles_by_ids,get_user_playbooks_by_ids,get_agent_playbooks_by_ids). Returns every matching owned row regardless of lifecycle status, expiry, or approval.It is a strict superset of the existing
include_tombstonesflag on the singular by-id getters, which only unhides MERGED/SUPERSEDED for lineage walks. Both flags now cross-reference each other in thestorage_basedocstrings so the next caller can tell them apart.user_idscoping still applies, and the default preserves existing retrieval behavior._rle_eligible_refs→_rle_resolvable_refson the commit path. It still locks (FOR SHARE) and rechecks the source rows in-transaction, but the lifecycle predicates are gone — it now verifies row existence only. Theattached AND resolvablecommit contract is otherwise unchanged.Resolution never follows a lineage pointer to a successor. The judge sees the original content, never the row that replaced it.
A contradictory-argument guard. Passing
include_inactive=Trueand an explicitstatus_filteris a caller bug: the filter would be silently dropped and the caller would get back rows it explicitly asked to exclude.validate_include_inactiverejects the combination.It raises
StorageErrorrather thanValueErrorfor a specific reason: bothhandle_exceptionsdecorators re-raiseStorageErroruntouched, but wrap any other exception type — and on the Supabase path also report it to Sentry. RaisingValueErrormade a pure caller bug surface as an opaqueStorageErroron one backend and aValueErroron another, with Sentry noise attached. It now fails loud and identically on every backend.Notes
Resolution is best-effort by design, not a durability guarantee. Row retention evicts tombstoned rows first, so a genuinely-served learning can age out and then resolve to nothing. That is a silent skip — an old session simply judges fewer learnings. Documented on
_resolve_candidates.Test Plan
include_inactiveon all three getters, covering that an archived row is returned, thatuser_idremains the only surviving tenant wall under the flag (it strips every other predicate, so this is now the one thing standing between tenants on that path), and that the contradictory filter combination is rejected.109 passedacross the evaluator and storage-contract suites (up from 103). Ruff and Pyright clean.Summary by CodeRabbit
New Features
Bug Fixes
Validation