feat(evaluation): retrieved-learning relevance/impact evaluation - #329
Merged
Conversation
Publishers can now attach the learnings they retrieved and injected per
turn via InteractionData.retrieved_learnings — a new minimal
RetrievedLearning{kind, learning_id} model (deliberately not Citation:
no tag/title exposure). The field round-trips through every storage
backend and the InteractionView.
When the sampled group evaluation fires, a new RetrievedLearningEvaluator
runs two judge families (relevance: does the learning apply to the
session; impact: positive/negative/neutral) over every attached,
still-retrieval-eligible learning, with exact per-ref verdict coverage
validation and one bounded repair per chunk. Verdicts land in a new
retrieved_learning_evaluation table as a latest-snapshot per session,
replaced atomically under generation + session-fingerprint fencing
(fingerprint covers every interaction id + attachment refs, recomputed
under the replacement lock — no mutation-path instrumentation).
Runner now returns GroupEvaluationOutcome (independent per-family
completion); regen jobs count both families; grade_on_demand carries
retrieved_learning_status with a fingerprint-revalidated cache;
/healthz/eval gains retrieved-learning counters; new read endpoint
POST /api/get_retrieved_learning_evaluation_results. Governance erase
scrubs the new table plus all three evaluation _operation_state
namespaces (agent-success marker + grade cache were a pre-existing RTBF
gap), via shared key builders in evaluation_state_keys.py. Retention
registers the table keyed on (user_id, session_id) so whole session
snapshots are always removed together.
Also fixes pre-existing reds found along the way: stale hardcoded
copies of _CANONICAL_DELETE_TARGET_NAMES in the governance suites,
sqlite planned/deleted counts missing the offline-tuner targets,
shadow-service tests hardcoding judge prompt v1.0.0, a CitationKind
pyright error in storage_base/_extras.py, and unannotated
RetentionMixin accesses in the retention contract tests.
- Add get_retrieved_learning_evaluation_results client method + route. - Add bulk get_agent_playbooks_by_ids with lifecycle filters (storage_base abstract + sqlite impl); evaluator uses it instead of per-id fetches and takes agent_version explicitly. - Incremental SessionFingerprintBuilder + transcript char-bounding (append_bounded_snapshot_interaction, DEFAULT_TRANSCRIPT_CHAR_LIMIT); snapshot carries a precomputed fingerprint. - get_matching_retrieved_learning_terminal_state recomputes live state under a writer transaction (BEGIN IMMEDIATE) so a stale cache is rejected after a concurrent publish; grade_on_demand caches a terminal outcome only after that confirmation and drops the no-attachment short-circuit.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds retrieved-learning attachment models, relevance and impact evaluation, SQLite persistence with fingerprint fencing, a results API and client method, grade-cache integration, and governance/retention support with extensive unit and integration coverage. ChangesRetrieved-learning evaluation
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 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/agent_success_evaluation/runner.py (1)
268-285: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute these failure exits through
_finish_with_retrieved_evaluationtoo. These direct returns bypass the retrieved-learning phase, even though the runner treats it as independent elsewhere; a session that hits agent-success failures or saves zero rows never gets that second evaluation family to progress.🤖 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/agent_success_evaluation/runner.py` around lines 268 - 285, The failure exits in the group evaluation runner currently return before running the retrieved-learning phase. Update both branches checking `evaluation_service.has_run_failures()` and `last_run_saved_result_count == 0` to call `_finish_with_retrieved_evaluation` with the appropriate failure outcome instead of returning directly, preserving their warning behavior and result-row handling.
🧹 Nitpick comments (3)
reflexio/server/services/storage/storage_base/_extras.py (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving
CITATION_KINDSfromCitationKindto avoid drift.
CITATION_KINDSduplicates the valid values of theCitationKinddomain type. IfCitationKindgains a new member, this constant must be updated in lockstep or new citation kinds will be silently dropped by this filter instead of raising a visible signal.♻️ Derive from `CitationKind` instead of hardcoding
-CITATION_KINDS: frozenset[str] = frozenset( - {"playbook", "profile", "user_playbook", "agent_playbook"} -) +CITATION_KINDS: frozenset[str] = frozenset(get_args(CitationKind))(requires
CitationKindto be aLiteral[...]type alias andfrom typing import get_args)Also applies to: 152-161
🤖 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/storage_base/_extras.py` around lines 14 - 17, Derive CITATION_KINDS from the CitationKind Literal type using typing.get_args instead of hardcoding string values. Update the related filtering logic to use this derived set, ensuring any newly added CitationKind value is automatically accepted and remains consistent with the domain type.reflexio/server/services/agent_success_evaluation/runner.py (1)
324-483: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetrieved-learning "superseded" and "pending" outcomes are not recorded in health counters.
_eval_health.record_retrieved_outcomeis called for"failed"(line 429-430) and terminal"applied"outcomes (line 462-464), but not whencommit.disposition == "superseded"(466-472) or when both attempts land on"stale"and the function returns"pending"(479-483). This leavesretrieved_learning.outcome_countsin the/healthz/evalpayload blind to how often sessions get stuck retrying due to write contention, which is useful signal for exactly this kind of fencing mechanism.♻️ Record superseded/pending outcomes too
if commit.disposition == "superseded": logger.info( "event=retrieved_learning_eval_superseded session_id=%s generation=%d", session_id, generation, ) + _eval_health.record_retrieved_outcome("superseded") return "superseded", None logger.info( "event=retrieved_learning_eval_stale session_id=%s generation=%d", session_id, generation, ) # Two stale snapshots in a row: leave pending for the next trigger. storage.finish_retrieved_learning_evaluation_run( user_id, session_id, generation, "pending", {"error_type": "stale_snapshot"} ) + _eval_health.record_retrieved_outcome("pending") return "pending", 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/agent_success_evaluation/runner.py` around lines 324 - 483, Record health outcomes for every retrieved-learning terminal path: call _eval_health.record_retrieved_outcome("superseded") before returning from the commit.disposition == "superseded" branch, and record "pending" after finishing the run as pending before the final return. Keep the existing failure and applied-outcome recording unchanged.reflexio/server/services/storage/storage_base/retrieved_learning_state.py (1)
136-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
+ 3formatting overhead constant.
prefix_size = len(role) + 3uses a magic number for transcript formatting overhead. A brief inline comment explaining what the 3 represents (e.g.,": "separator + newline) would aid future maintainers.♻️ Proposed comment
def append_bounded_snapshot_interaction( snapshot: BoundedRetrievedLearningSnapshot, *, interaction_id: int, role: str, content: str, created_at: int, refs: list[tuple[str, str]], transcript_chars_remaining: int, ) -> int: """Retain refs and only the transcript prefix that fits the char budget.""" retained_role = "" retained_content = "" if transcript_chars_remaining > 0 and content: - prefix_size = len(role) + 3 + # Account for role + ": " separator + "\n" = 3 chars of formatting overhead + prefix_size = len(role) + 3 content_budget = max(0, transcript_chars_remaining - prefix_size)🤖 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/storage_base/retrieved_learning_state.py` around lines 136 - 166, Document the formatting overhead represented by the magic number in append_bounded_snapshot_interaction: add a brief inline comment next to prefix_size = len(role) + 3 explaining that 3 accounts for the “: ” separator and newline.
🤖 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/lib/_search.py`:
- Around line 108-110: In the exception handler of the retrieval evaluation
function, stop returning str(e) in
GetRetrievedLearningEvaluationResultsResponse. Log the caught exception
server-side with the appropriate logger, then return a generic user-facing error
message while preserving success=False and an empty results list.
In
`@reflexio/server/services/agent_success_evaluation/components/retrieved_learning_evaluator.py`:
- Around line 231-238: Preserve diagnostics on the empty-candidates return in
the retrieved-learning evaluation method. Update the
RetrievedLearningEvaluationRun construction in the `if not candidates` branch
after `_resolve_candidates` to pass `diagnostics=diagnostics`, matching the
other return paths so `invalid_ref_count` and `candidate_count` reach
`_eval_health.record_retrieved_outcome`.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py`:
- Around line 249-252: The fingerprint built by the playbook evaluation builder
currently uses only interaction IDs and attachment references, so replacing an
existing interaction can leave stale cached verdicts. Update the fingerprinting
logic around builder.add and the corresponding code at the noted duplicate
location to include canonicalized transcript role/content alongside IDs and
attachments, then add a regression test that uses INSERT OR REPLACE on an
existing interaction with changed transcript content and verifies the
fingerprint or terminal verdict is invalidated.
- Around line 433-460: At the commit logic around _rle_eligible_refs, restrict
kept results to keys present in the live attachments for the current user_id and
session_id, deduplicate by (kind, learning_id), and preserve only eligible
attached records. During insertion into retrieved_learning_evaluation, use the
method’s canonical user_id and session_id parameters instead of r.user_id and
r.session_id.
---
Outside diff comments:
In `@reflexio/server/services/agent_success_evaluation/runner.py`:
- Around line 268-285: The failure exits in the group evaluation runner
currently return before running the retrieved-learning phase. Update both
branches checking `evaluation_service.has_run_failures()` and
`last_run_saved_result_count == 0` to call `_finish_with_retrieved_evaluation`
with the appropriate failure outcome instead of returning directly, preserving
their warning behavior and result-row handling.
---
Nitpick comments:
In `@reflexio/server/services/agent_success_evaluation/runner.py`:
- Around line 324-483: Record health outcomes for every retrieved-learning
terminal path: call _eval_health.record_retrieved_outcome("superseded") before
returning from the commit.disposition == "superseded" branch, and record
"pending" after finishing the run as pending before the final return. Keep the
existing failure and applied-outcome recording unchanged.
In `@reflexio/server/services/storage/storage_base/_extras.py`:
- Around line 14-17: Derive CITATION_KINDS from the CitationKind Literal type
using typing.get_args instead of hardcoding string values. Update the related
filtering logic to use this derived set, ensuring any newly added CitationKind
value is automatically accepted and remains consistent with the domain type.
In `@reflexio/server/services/storage/storage_base/retrieved_learning_state.py`:
- Around line 136-166: Document the formatting overhead represented by the magic
number in append_bounded_snapshot_interaction: add a brief inline comment next
to prefix_size = len(role) + 3 explaining that 3 accounts for the “: ” separator
and newline.
🪄 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 Plus
Run ID: b5130960-2794-4ee4-9c58-21bcdcefdb57
📒 Files selected for processing (51)
reflexio/client/client.pyreflexio/lib/_search.pyreflexio/models/api_schema/domain/entities.pyreflexio/models/api_schema/eval_overview_schema.pyreflexio/models/api_schema/retriever_schema.pyreflexio/models/api_schema/ui/converters.pyreflexio/models/api_schema/ui/entities.pyreflexio/server/README.mdreflexio/server/prompt/prompt_bank/retrieved_learning_impact/v1.0.0.prompt.mdreflexio/server/prompt/prompt_bank/retrieved_learning_relevance/v1.0.0.prompt.mdreflexio/server/routes/evaluation.pyreflexio/server/services/agent_success_evaluation/README.mdreflexio/server/services/agent_success_evaluation/_eval_health.pyreflexio/server/services/agent_success_evaluation/components/retrieved_learning_evaluator.pyreflexio/server/services/agent_success_evaluation/regen_jobs.pyreflexio/server/services/agent_success_evaluation/runner.pyreflexio/server/services/generation_service.pyreflexio/server/services/governance/service.pyreflexio/server/services/storage/governance_validation.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_governance.pyreflexio/server/services/storage/sqlite_storage/governance/_erase_execution.pyreflexio/server/services/storage/sqlite_storage/governance/_subject_barrier.pyreflexio/server/services/storage/sqlite_storage/playbook/_agent.pyreflexio/server/services/storage/sqlite_storage/playbook/_eval_results.pyreflexio/server/services/storage/sqlite_storage/profiles/_interaction_store.pyreflexio/server/services/storage/storage_base/_extras.pyreflexio/server/services/storage/storage_base/evaluation_state_keys.pyreflexio/server/services/storage/storage_base/playbook/_agent.pyreflexio/server/services/storage/storage_base/playbook/_eval_results.pyreflexio/server/services/storage/storage_base/retrieved_learning_state.pyreflexio/test_support/llm_mock.pyreflexio/test_support/llm_model_registry.pytests/client/test_evaluation_client.pytests/server/api_endpoints/test_grade_on_demand_integration.pytests/server/api_endpoints/test_retrieved_learning_results_api.pytests/server/services/agent_success_evaluation/test_eligibility_logging.pytests/server/services/agent_success_evaluation/test_regen_jobs.pytests/server/services/agent_success_evaluation/test_regen_jobs_concurrency_integration.pytests/server/services/agent_success_evaluation/test_regen_jobs_sampling_integration.pytests/server/services/agent_success_evaluation/test_retrieved_learning_evaluator.pytests/server/services/agent_success_evaluation/test_retrieved_learning_runner_integration.pytests/server/services/evaluation_overview/test_shadow_service_integration.pytests/server/services/governance/test_subject_write_barrier_sqlite.pytests/server/services/storage/sqlite_storage/test_governance_retrieved_learning.pytests/server/services/storage/sqlite_storage/test_governance_storage.pytests/server/services/storage/test_storage_contract_retention.pytests/server/services/storage/test_storage_contract_retrieved_learning_evals.pytests/server/services/test_prompt_model_mapping.pytests/server/test_app_route_inventory.py
…ss definition The impact judge previously scored injected learnings positive/negative/neutral against a generic "better response" notion, untethered from what the org considers success. Pass AgentSuccessConfig.success_definition_prompt into the impact judge (impact only; relevance stays success-agnostic) and reword the v1.0.0 impact prompt to judge movement toward/away from the defined success criteria. Falls back to general task helpfulness when no definition is set.
- Session fingerprint now covers each interaction's transcript role/content (fixed-limit fp_content, decoupled from the snapshot transcript budget so the commit-side recompute stays consistent), so an in-place content edit (same id + attachments) invalidates stale cached verdicts. Regression test added. - Preserve diagnostics on the empty-candidates evaluation return so invalid_ref_count / candidate_count reach eval-health. - Retrieved-learning read API logs the exception server-side and returns a generic message instead of leaking str(e) to the client. - Scope the retrieved_learning_evaluation INSERT to the canonical user_id/session_id (matching the DELETE) instead of per-row values.
replace_retrieved_learning_evaluation_results kept records that were retrieval-eligible but never attached to the session — the method contract is "attached AND eligible". Add an in-transaction _rle_attached_refs check so a verdict is stored only when its (kind, learning_id) is still attached to the live session. Duplicate identities are intentionally left to trip the UNIQUE index and roll back the commit (fail loud on caller bugs). Regression test added.
This was referenced Jul 11, 2026
This was referenced Jul 27, 2026
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
Adds retrieved-learning relevance & impact evaluation to the agent-success pipeline: when a publish request declares
retrieved_learnings(the stable identities of profiles/playbooks injected into an agent turn), the evaluator judges, per learning, whether it was relevant to the session and what impact it had on the response (positive / neutral / negative). Verdicts are persisted per(user_id, session_id, kind, learning_id)and exposed through a read API.Changes
Evaluation
RetrievedLearningEvaluator— relevance + impact judging with bounded repair, candidate/attachment limits, and dedup of repeated refs across turns.pendingfor the next trigger. Terminal outcomes (complete/not_applicable) are cached.retrieved_learning_relevanceandretrieved_learning_impactv1.0.0.Read surface
get_retrieved_learning_evaluation_resultsclient method +/api/get_retrieved_learning_evaluation_resultsroute, with request/response schemas and theRetrievedLearningEvaluationResultdomain model.Storage
replace_retrieved_learning_evaluation_resultsunder generation + fingerprint fencing.get_agent_playbooks_by_idswith lifecycle filters (abstract + sqlite impl) replacing per-id fetches.SessionFingerprintBuilder, transcript char-bounding (append_bounded_snapshot_interaction,DEFAULT_TRANSCRIPT_CHAR_LIMIT), and a precomputed snapshot fingerprint soget_matching_retrieved_learning_terminal_statecan recompute live state under a writer transaction and reject a stale cache after a concurrent publish.Test plan
llm_model_registry/llm_mock; prompt-model mapping + route inventory updated.Summary by CodeRabbit
POST /api/get_retrieved_learning_evaluation_resultswith filtering by user/session andlimit.