feat(evaluation): independent retrieved-learning sampling; retire retrieval-log capture#334
Open
yyiilluu wants to merge 2 commits into
Open
feat(evaluation): independent retrieved-learning sampling; retire retrieval-log capture#334yyiilluu wants to merge 2 commits into
yyiilluu wants to merge 2 commits into
Conversation
…rieval-log capture
The enterprise offline playbook tuner is being rewired onto the per-playbook
`impact` verdict in `retrieved_learning_evaluation`. That makes the OSS
retrieval-capture subsystem unreferenced, and it needs two things from OSS that
did not exist: denser coverage it can buy without side effects, and an
exhaustive windowed read.
Sampling — the two judge families now sample independently:
- New `AgentSuccessConfig.retrieved_learning_sampling_rate`. Defaults to `None`
= inherit `sampling_rate`, so an org that never opts in keeps exactly its
previous behavior and cost.
- New `agent_success_evaluation/sampling.py`. The publish gate samples both
families once, schedules when EITHER admits the session, and passes the two
booleans to the runner. One computation site, so the families cannot diverge.
A session sampled only for retrieved-learning never pays the session-success
judge. Direct callers (regen jobs, on-demand grade) run both families as
before — sampling is a scheduling decision, not a runner one.
Retry sweep — `failed` and `pending` committed nothing and had no retry trigger,
so such a session was invisible forever. The publish callback now re-arms it on
the existing scheduler, bounded to 3 attempts, retrieved-learning only.
Best-effort: the counter and schedule are in-process, so a restart drops pending
retries.
`degraded` is deliberately NOT retried. It is an applied, fingerprint-fenced
commit — the rows are persisted and only the failed chunks carry NULL impact.
Retrying it re-executes every relevance+impact chunk and delete/re-inserts
committed rows, and a deterministically degrading chunk degrades again every
time, so a bounded sweep burned 4x the judge bill and bought nothing. Instead,
new `SETTLED_RETRIEVED_STATUSES` (= TERMINAL | {degraded}) lets consumers read
those rows directly, via an optional `statuses` arg on the terminal-state fence.
Widening the set does not weaken the freshness check.
Storage — new exhaustive `get_retrieved_learning_evaluation_results_in_window()`
on the base + SQLite. The existing paged reader hard-caps at limit=100 with no
cursor; a consumer treating the window as complete must not inherit that silent
truncation. Contract-tested at 0/100/101/250 rows.
Retrieval-log retirement — delete `PlaybookRetrievalLog`/`PlaybookRetrievalLogItem`,
`RetrievalLogMixin`, and the `configure_retrieval_capture_hook` search hook.
KEPT: the `AuditEntityType` "playbook_retrieval_log" literal (historical audit
rows still carry it) and the retention targets/cascade — the tables still hold
PII until a later release drops them.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change removes retrieval-log capture APIs and models, adds independent agent-success and retrieved-learning sampling, introduces bounded retrieved-learning retries, and expands retrieved-learning storage interfaces with window reads and configurable status matching. ChangesRetrieved-learning evaluation lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GenerationService
participant Sampling
participant GroupEvaluationScheduler
participant EvaluationRunner
GenerationService->>Sampling: select agent-success and retrieved-learning families
Sampling-->>GenerationService: return evaluation-family flags
GenerationService->>GroupEvaluationScheduler: enqueue sampled evaluation
GroupEvaluationScheduler->>EvaluationRunner: run_group_evaluation(flags)
EvaluationRunner-->>GroupEvaluationScheduler: return retrieved-learning status
GroupEvaluationScheduler->>GroupEvaluationScheduler: retry failed or pending retrieved-learning status up to 3 times
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: 1
🧹 Nitpick comments (1)
reflexio/server/services/agent_success_evaluation/runner.py (1)
110-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale step numbering in comments/docstring after inserting the admission gate.
The docstring's "Steps" list (lines 110-121) still enumerates the old 8-step flow and doesn't mention the new per-family admission step. Additionally, both the "already evaluated" check (Line 188) and "fetch interactions" (Line 218) are now labeled step "4", since inserting "3. Per-family admission" shifted subsequent steps without renumbering them.
✏️ Suggested renumbering
- # 4. Check if agent success is already evaluated — skipped in + # 5. Check if agent success is already evaluated — skipped in ... - # 4. Fetch interactions for all requests + # 6. Fetch interactions for all requestsAlso update the docstring "Steps" list to include the new per-family admission step.
Also applies to: 171-188, 218-218
🤖 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 110 - 121, Update the Steps docstring in the evaluation runner to include the per-family admission step and renumber all subsequent steps through the final operation-state update. Correct the inline step labels near the already-evaluated check and interaction fetching so each label matches the updated flow, without changing runtime behavior.
🤖 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/sqlite_storage/playbook/_eval_results.py`:
- Around line 417-419: The status selection in the affected retrieval method
currently treats an explicitly empty statuses override as unset. Update the
logic around the statuses parameter to use TERMINAL_RETRIEVED_STATUSES only when
statuses is None, while preserving an empty frozenset so it matches no persisted
state.
---
Nitpick comments:
In `@reflexio/server/services/agent_success_evaluation/runner.py`:
- Around line 110-121: Update the Steps docstring in the evaluation runner to
include the per-family admission step and renumber all subsequent steps through
the final operation-state update. Correct the inline step labels near the
already-evaluated check and interaction fetching so each label matches the
updated flow, without changing runtime behavior.
🪄 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: ec1a1635-fb53-4431-8518-04a4bb889aff
📒 Files selected for processing (20)
reflexio/models/api_schema/domain/entities.pyreflexio/models/api_schema/domain/governance.pyreflexio/models/config_schema.pyreflexio/server/extensions.pyreflexio/server/services/agent_success_evaluation/runner.pyreflexio/server/services/agent_success_evaluation/sampling.pyreflexio/server/services/generation_service.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/sqlite_storage/playbook/_eval_results.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_retrieval_log.pyreflexio/server/services/storage/storage_base/playbook/_eval_results.pyreflexio/server/services/storage/storage_base/retrieved_learning_state.pyreflexio/server/services/unified_search_service.pytests/server/services/agent_success_evaluation/test_sampling.pytests/server/services/storage/test_storage_contract_retrieval_logs.pytests/server/services/storage/test_storage_contract_retrieved_learning_evals.pytests/server/services/test_generation_service_scheduling.pytests/server/services/test_unified_search_service.pytests/server/services/test_unified_search_session_dedup.py
💤 Files with no reviewable changes (7)
- reflexio/server/services/storage/storage_base/_retrieval_log.py
- tests/server/services/storage/test_storage_contract_retrieval_logs.py
- tests/server/services/test_unified_search_session_dedup.py
- reflexio/server/services/storage/storage_base/init.py
- reflexio/models/api_schema/domain/entities.py
- tests/server/services/test_unified_search_service.py
- reflexio/server/services/unified_search_service.py
…ck to TERMINAL `accepted = statuses or TERMINAL_RETRIEVED_STATUSES` treats an explicitly empty frozenset as unset, because an empty frozenset is falsy. A caller asking to match NO status silently got the TERMINAL default instead — the fence would then admit sessions the caller meant to exclude. Default off `is None` instead, so an empty set means exactly what it says. Contract-tested across every backend (the same `or` was in both the SQLite and Supabase impls, so fixing one would have left them divergent). Verified the test goes red if the `or` is reintroduced.
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.
Paired with the enterprise PR that rewires the offline playbook tuner onto a single per-playbook impact signal. Merge this first, then the enterprise repo pins it.
Why
The tuner now reads the per-playbook
impactverdict inretrieved_learning_evaluationand nothing else. Two consequences land in OSS:Interaction.retrieved_learningsnow carries.What's here
Independent per-family sampling
New
AgentSuccessConfig.retrieved_learning_sampling_rate, defaultNone= inheritsampling_rate— an org that never opts in keeps exactly its previous behavior and cost.New
agent_success_evaluation/sampling.py: the publish gate samples both families once, schedules when either admits the session, and passes two booleans to the runner. One computation site, so the families cannot diverge. A session sampled only for retrieved-learning never pays the session-success judge — which is the point: it lets an operator buy coverage without also raising the agent-success bill.Direct callers (regen jobs, on-demand grade) run both families as before. Sampling is a scheduling decision, not a runner one.
degradedis settled, not brokenfailed/pendingcommitted nothing and had no retry trigger, so such a session was invisible forever. The publish callback now re-arms it on the existing scheduler, bounded to 3 attempts.degradedis deliberately NOT retried. It is an applied, fingerprint-fenced commit: the rows are persisted and only the failed chunks carry NULL impact. Retrying it re-executes every relevance+impact chunk and delete/re-inserts already-committed rows — and a deterministically degrading chunk (over-length learning, content-filter refusal) degrades again on every attempt, so a bounded sweep burned 4× the judge bill and bought nothing.Instead: new
SETTLED_RETRIEVED_STATUSES(=TERMINAL | {degraded}), exposed via an optionalstatusesarg on the terminal-state fence, so consumers read those rows directly. Contract-tested that widening the set does not weaken the freshness check — a degraded session whose transcript changed is still rejected.Exhaustive windowed read
get_retrieved_learning_evaluation_results_in_window()on the base + SQLite. The existing paged reader hard-caps atlimit=100with no cursor; a consumer that treats the window as the complete set must not inherit that silent truncation. Contract-tested at 0 / 100 / 101 / 250 rows.Retrieval-log retirement
Deletes
PlaybookRetrievalLog/PlaybookRetrievalLogItem,RetrievalLogMixin, and theconfigure_retrieval_capture_hooksearch hook.Deliberately KEPT:
AuditEntityType"playbook_retrieval_log"literal — historicalaudit_eventsrows still carry it, and removing it makes Pydantic reject them on read;Verification
main.Noneexplicitly, so it asserted its own argument — which is exactly how a0.1default drifted in unnoticed. Verified the new test goes red if the drift is reintroduced.Summary by CodeRabbit
retrieved_learning_sampling_ratein success configuration (optional; defaults to existing sampling behavior).