feat: ground and review generated playbooks - #386
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (50)
📝 WalkthroughWalkthroughThe pull request adds evidence-grounded playbook extraction, candidate review, stricter consolidation, provider-tolerant structured parsing, resumable schema selection, fallback timeout handling, prompt versioning, and expanded validation coverage. ChangesPlaybook pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
reflexio/server/services/playbook/components/consolidator.py (1)
622-630: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winLLM Security (CWE-1427)
Reachability: External
Serialize playbook rows before prompting In
_format_playbooks_with_prefix,Content,Trigger,Rationale, andEvidence Spanare injected as raw prose, so a malicious span can still smuggle instructions or row-shaped text into the LLM context. The current checks enforce NEW coverage and overlap, but they do not prove the chosenunify/reject_newaction is grounded in the cited evidence.🤖 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/playbook/components/consolidator.py` around lines 622 - 630, Update _format_playbooks_with_prefix to serialize each playbook row before adding it to the prompt, treating Content, Trigger, Rationale, and Evidence Span as data rather than raw prose. Use a structured, unambiguous representation with proper escaping so embedded instructions or row-shaped text cannot alter the row boundaries or surrounding prompt semantics, while preserving the existing prefix, evidence-group, name, source, and timestamp fields.reflexio/server/services/extraction/resume_worker.py (1)
571-621: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist and label resolved tool answers as strict evidence.
prompt_context.evidence_sourcescontains only original interactions, while the resumed run can rely on a resolvedask_humananswer. The strict validator and persistence path therefore cannot cite that answer: a required positive procedure is either dropped or saved with provenance pointing only to the earlier “missing procedure” conversation.
reflexio/server/services/extraction/resume_worker.py#L571-L621: materialize resolved tool answers into the resumed evidence snapshot/map and render a stable local reference before strict validation.reflexio/server/services/extraction/resume_worker.py#L785-L830: rebuild the same persisted evidence snapshot during finalization retry, rather than reconstructing only original interaction evidence.🤖 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/extraction/resume_worker.py` around lines 571 - 621, Update the resumed extraction flow around _resume_agent to materialize resolved tool answers into the evidence snapshot/map, render stable local references, and pass that augmented evidence to strict validation and persistence. Apply the same evidence reconstruction in reflexio/server/services/extraction/resume_worker.py lines 571-621 and 785-830, with the finalization retry reusing resolved answers rather than only original interaction evidence.tests/server/services/playbook/test_playbook_extractor.py (1)
891-925: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClass docstring now contradicts this test.
TestStructuredPlaybookExtraction's docstring still states that a degenerate loop "leavesresult.output is Noneand the extractor returns[]", which this test just inverted to a terminalRuntimeError. Update the class docstring so the contract statement matches.🤖 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/playbook/test_playbook_extractor.py` around lines 891 - 925, Update the TestStructuredPlaybookExtraction class docstring to state that a degenerate loop with no structured output terminates with a RuntimeError, rather than leaving result.output as None and returning an empty list. Keep the rest of the documented extraction contract unchanged.
🧹 Nitpick comments (8)
tests/server/services/playbook/test_playbook_generation_service.py (2)
918-932: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion depends on the on-disk active extraction prompt version.
The fail-closed branch only runs when
uses_evidence_grounded_extractionseesplaybook_extraction_context >= 4.6.0. Since activating an older version is an explicitly supported rollback (per that helper's docstring), a rollback would silently turn this into a "review skipped" path and fail here with an unrelated message. Stubbingget_active_versionforplaybook_extraction_contextwould keep the test asserting the behavior it names.🤖 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/playbook/test_playbook_generation_service.py` around lines 918 - 932, Update the test around service._resolve_write_plan to stub get_active_version for playbook_extraction_context to return a version at or above 4.6.0. Keep the existing RuntimeError assertion for “missing validated evidence metadata: C1” unchanged, ensuring the test consistently exercises the fail-closed evidence-validation branch regardless of the active on-disk prompt version.
810-825: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the retrieval-cache invalidation branch.
This test's reviewer only changes
content, so the survivors' search keys are unchanged andexisting_playbooksis reused. The new invalidation path inservice.pylines 471-474 (a revised trigger must drop the cached retrieval so consolidation searches again) is never exercised, and the assertions don't checkdeduplicate(..., existing_playbooks=...)either. A sibling case returning a survivor with a new trigger and assertingcall_args.kwargs["existing_playbooks"] is Nonewould pin it. Want me to draft it?🤖 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/playbook/test_playbook_generation_service.py` around lines 810 - 825, The test around _resolve_write_plan only covers reviewer content changes and does not exercise retrieval-cache invalidation. Add a sibling case where reviewer.review returns a survivor with a revised trigger, then assert consolidator.deduplicate receives existing_playbooks=None via call_args.kwargs while preserving the existing consolidation assertions.tests/server/services/playbook/test_playbook_reviewer.py (1)
324-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParametrize the prompt-phrase assertions so a failure names the guarantee.
Forty bare
assert <phrase> in normalizedlines fail one-at-a-time with no indication of which reviewer behavior regressed, and generic phrases like"This is mandatory"carry almost no signal. A@pytest.mark.parametrize(("guarantee", "phrase"), [...])over the same list reports every drifted phrase in one run and documents intent.🤖 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/playbook/test_playbook_reviewer.py` around lines 324 - 362, Replace the individual prompt-phrase assertions in the relevant playbook reviewer test with a pytest.mark.parametrize fixture using (“guarantee”, “phrase”) entries. Keep the existing phrase list and assertion behavior, but include descriptive guarantee names—especially for generic phrases such as “This is mandatory”—so failures identify the regressed reviewer behavior and report all mismatches in one run.reflexio/server/services/playbook/service.py (2)
396-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the review gate into its own method.
_resolve_write_plannow carries ~90 lines of five-level-deep gating (mock mode → window reload → expert → strict-prompt → evidence validation → review → cache invalidation) on top of its consolidation duties. A_maybe_review_candidates(all_playbooks, playbook_config, consolidator) -> tuple[list[UserPlaybook], list[UserPlaybook] | None]helper would flatten the nesting and make the early-skip branches readable, with no behavior change.🤖 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/playbook/service.py` around lines 396 - 474, Extract the nested candidate-review gate from _resolve_write_plan into a helper such as _maybe_review_candidates, returning the reviewed playbooks and the optionally reusable existing_playbooks cache. Preserve every existing skip condition, validation/error behavior, reviewer inputs, and consolidation-search-key cache invalidation, while leaving _resolve_write_plan responsible only for invoking the helper and continuing consolidation.
206-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe "honest empty window" contract is unreachable in production.
[]is deliberately cached and returned here (and pinned bytest_review_interaction_window_returns_honest_empty_window), but the sole production caller at line 404 raisesRuntimeErroron a falsy window, so an empty reload aborts the whole write plan rather than being tolerated. Either the empty case should skip review with a log (like the expert/legacy branches below it) or the docstring should state that empty is also fatal.🤖 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/playbook/service.py` around lines 206 - 217, The production caller of the cached review window must honor an empty list as a valid window: update the review flow around the caller near the write-plan logic to skip review and log appropriately when the window is empty, while preserving the existing fatal handling for a missing window (None). Keep _get_interactions and its honest-empty caching behavior unchanged.reflexio/server/services/playbook/components/reviewer.py (1)
198-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe note prefix is an implicit cross-component contract.
consolidator._has_reviewer_revisiondetects reviewer revisions by matching this free-text note prefix (seetests/server/services/playbook/test_playbook_consolidator.pyline 2472, which pins"First-pass reviewer: revised (late_trigger) — …"). A reword here silently disables reviewer-revision precedence in_merge_replayed_evidence_duplicates. Consider hoisting the prefix into a shared constant. Also note the wording says "First-pass reviewer" while this module is documented as the second-pass review.♻️ Shared constant sketch
+REVIEW_NOTE_PREFIX = "First-pass reviewer" + def _append_review_note( existing: str | None, decision: str, reason_code: str, reason: str | None, ) -> str: normalized_reason = " ".join((reason or reason_code).split())[:500] review_note = ( - f"First-pass reviewer: {decision} ({reason_code}) — {normalized_reason}" + f"{REVIEW_NOTE_PREFIX}: {decision} ({reason_code}) — {normalized_reason}" )🤖 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/playbook/components/reviewer.py` around lines 198 - 208, Hoist the exact “First-pass reviewer:” note prefix used by _append_review_note into a shared constant accessible to consolidator._has_reviewer_revision, and update both producer and detector to reuse it. Preserve the existing note format and wording so reviewer-revision precedence and the pinned test behavior remain unchanged.reflexio/server/services/playbook/components/consolidator.py (1)
1005-1018: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOverlap guard is not applied on the evidence-validated short-circuit.
The LLM path runs
_find_overlapping_survivorsover the produced rows (Line 1084) before returning, but this branch returns candidates directly. Two grounded siblings that state the same lesson with disjoint provenance will both persist here and only be caught on a later batch. If that is intended (the comment argues for keeping independent siblings), consider saying so explicitly; otherwise run the same guard overnew_playbooksbefore returning.🤖 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/playbook/components/consolidator.py` around lines 1005 - 1018, Apply the same overlap guard used by the LLM path to the evidence-validated short-circuit before returning new_playbooks. Invoke _find_overlapping_survivors with the appropriate context, return only the surviving playbooks, and preserve the existing request_id updates and empty auxiliary lists.reflexio/server/services/playbook/playbook_service_utils.py (1)
505-524: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ref_by_object_idkeyed onid()silently collapses shared interaction objects.If the same
Interactioninstance is reachable from tworequest_model.interactionslists (same object, not just equal), the second registration overwrites the first and the earlier turn resolves to the laterT#. A(request_id, interaction_id)key would be identity-independent and still O(1).🤖 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/playbook/playbook_service_utils.py` around lines 505 - 524, Replace the id(interaction)-based keys in the turn registration logic with a (request_id, interaction_id) key, and update all corresponding lookups to use that same tuple. Preserve O(1) resolution while ensuring shared Interaction instances retain independent turn mappings for each request.
🤖 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/prompt/prompt_bank/playbook_extraction_context/v4.6.0.prompt.md`:
- Around line 316-319: Wrap the {agent_context_prompt} insertion in an explicit
untrusted-data boundary within the prompt, instructing the extractor to treat
all transcript content as evidence only and not follow embedded instructions.
Preserve the existing guidance about exploring available tools and negative
outcomes.
In `@reflexio/server/services/playbook/components/consolidator.py`:
- Around line 1772-1781: The docstrings for _apply_reject_new and _apply_one
still describe the removed safety fallback behavior. Update their descriptions
and Returns documentation to state the current unresolved EXISTING-id
behavior—raising an error—and remove references to logging warnings, returning
empty collections for fallback, or suppressing the safety fallback via
handled_new_ids.
In `@reflexio/server/services/playbook/components/reviewer.py`:
- Around line 404-414: Normalize decision candidate IDs consistently in
_apply_decisions by stripping surrounding whitespace before building the
decisions mapping, matching the normalization used during validation. Ensure
lookups by the generated C{index} identifiers cannot fail for padded provider
IDs, while preserving existing decision handling.
In `@reflexio/server/services/playbook/playbook_evidence.py`:
- Around line 50-56: Update _is_direct_user_preference_source to require both a
normalized user role and a trusted user-origin value in request_source; do not
accept role alone. Reuse the existing request_source enum or constant for the
trusted user source, preserving rejection of generated, memory, and
recommender-originated turns.
In `@reflexio/server/services/playbook/README.md`:
- Around line 93-98: Update the decision-kind bullet list immediately above the
rewritten contract to match the four variants in PlaybookConsolidationOutput:
replace duplicate with unify, prefer_new with reject_new, and prefer_existing
with the current corresponding contract terminology; retain differentiate and
independent unchanged.
In `@reflexio/server/services/playbook/service.py`:
- Line 154: Update _review_window_cache handling in PlaybookGenerationService so
cached review-window data cannot be reused across users or requests. Either key
the cache by (user_id, request_id) and retrieve entries using both identifiers,
or clear it in _prepare_generation_run() whenever a new request is loaded;
preserve reuse only within the same user/request.
In `@tests/server/services/playbook/test_playbook_consolidator.py`:
- Around line 1173-1180: Update the pytest.raises(ValueError) assertion around
_build_deduplicated_results to include a match= pattern that specifically
identifies the unresolved superseding-reference error expected by this test.
Preserve the existing arguments and ensure the pattern is precise enough to
reject unrelated partition, ID, or survivor-overlap validation failures.
In `@tests/server/services/playbook/test_playbook_extractor.py`:
- Around line 1942-1962: Suppress RUF001 locally on the intentional
curly-apostrophe test cases in
test_typographic_quotes_resolve_to_the_original_text and
test_ambiguous_normalized_match_is_rejected, preserving the typographic
characters and test behavior without broadening the lint suppression.
---
Outside diff comments:
In `@reflexio/server/services/extraction/resume_worker.py`:
- Around line 571-621: Update the resumed extraction flow around _resume_agent
to materialize resolved tool answers into the evidence snapshot/map, render
stable local references, and pass that augmented evidence to strict validation
and persistence. Apply the same evidence reconstruction in
reflexio/server/services/extraction/resume_worker.py lines 571-621 and 785-830,
with the finalization retry reusing resolved answers rather than only original
interaction evidence.
In `@reflexio/server/services/playbook/components/consolidator.py`:
- Around line 622-630: Update _format_playbooks_with_prefix to serialize each
playbook row before adding it to the prompt, treating Content, Trigger,
Rationale, and Evidence Span as data rather than raw prose. Use a structured,
unambiguous representation with proper escaping so embedded instructions or
row-shaped text cannot alter the row boundaries or surrounding prompt semantics,
while preserving the existing prefix, evidence-group, name, source, and
timestamp fields.
In `@tests/server/services/playbook/test_playbook_extractor.py`:
- Around line 891-925: Update the TestStructuredPlaybookExtraction class
docstring to state that a degenerate loop with no structured output terminates
with a RuntimeError, rather than leaving result.output as None and returning an
empty list. Keep the rest of the documented extraction contract unchanged.
---
Nitpick comments:
In `@reflexio/server/services/playbook/components/consolidator.py`:
- Around line 1005-1018: Apply the same overlap guard used by the LLM path to
the evidence-validated short-circuit before returning new_playbooks. Invoke
_find_overlapping_survivors with the appropriate context, return only the
surviving playbooks, and preserve the existing request_id updates and empty
auxiliary lists.
In `@reflexio/server/services/playbook/components/reviewer.py`:
- Around line 198-208: Hoist the exact “First-pass reviewer:” note prefix used
by _append_review_note into a shared constant accessible to
consolidator._has_reviewer_revision, and update both producer and detector to
reuse it. Preserve the existing note format and wording so reviewer-revision
precedence and the pinned test behavior remain unchanged.
In `@reflexio/server/services/playbook/playbook_service_utils.py`:
- Around line 505-524: Replace the id(interaction)-based keys in the turn
registration logic with a (request_id, interaction_id) key, and update all
corresponding lookups to use that same tuple. Preserve O(1) resolution while
ensuring shared Interaction instances retain independent turn mappings for each
request.
In `@reflexio/server/services/playbook/service.py`:
- Around line 396-474: Extract the nested candidate-review gate from
_resolve_write_plan into a helper such as _maybe_review_candidates, returning
the reviewed playbooks and the optionally reusable existing_playbooks cache.
Preserve every existing skip condition, validation/error behavior, reviewer
inputs, and consolidation-search-key cache invalidation, while leaving
_resolve_write_plan responsible only for invoking the helper and continuing
consolidation.
- Around line 206-217: The production caller of the cached review window must
honor an empty list as a valid window: update the review flow around the caller
near the write-plan logic to skip review and log appropriately when the window
is empty, while preserving the existing fatal handling for a missing window
(None). Keep _get_interactions and its honest-empty caching behavior unchanged.
In `@tests/server/services/playbook/test_playbook_generation_service.py`:
- Around line 918-932: Update the test around service._resolve_write_plan to
stub get_active_version for playbook_extraction_context to return a version at
or above 4.6.0. Keep the existing RuntimeError assertion for “missing validated
evidence metadata: C1” unchanged, ensuring the test consistently exercises the
fail-closed evidence-validation branch regardless of the active on-disk prompt
version.
- Around line 810-825: The test around _resolve_write_plan only covers reviewer
content changes and does not exercise retrieval-cache invalidation. Add a
sibling case where reviewer.review returns a survivor with a revised trigger,
then assert consolidator.deduplicate receives existing_playbooks=None via
call_args.kwargs while preserving the existing consolidation assertions.
In `@tests/server/services/playbook/test_playbook_reviewer.py`:
- Around line 324-362: Replace the individual prompt-phrase assertions in the
relevant playbook reviewer test with a pytest.mark.parametrize fixture using
(“guarantee”, “phrase”) entries. Keep the existing phrase list and assertion
behavior, but include descriptive guarantee names—especially for generic phrases
such as “This is mandatory”—so failures identify the regressed reviewer behavior
and report all mismatches in one run.
🪄 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: dbf35f89-9bc7-47d4-bd8b-655984812389
📒 Files selected for processing (48)
reflexio/models/structured_output.pyreflexio/server/llm/_litellm_structured_output.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/llm/_litellm_types.pyreflexio/server/llm/tools.pyreflexio/server/prompt/prompt_bank/playbook_candidate_review/v1.0.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_consolidation/v2.5.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_consolidation/v2.6.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.5.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.6.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_main/v1.4.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_main/v1.5.0.prompt.mdreflexio/server/services/base_generation/_extraction_lifecycle.pyreflexio/server/services/base_generation_service.pyreflexio/server/services/extraction/agent_run_records.pyreflexio/server/services/extraction/resumable_agent.pyreflexio/server/services/extraction/resume_worker.pyreflexio/server/services/playbook/README.mdreflexio/server/services/playbook/components/consolidator.pyreflexio/server/services/playbook/components/extractor.pyreflexio/server/services/playbook/components/reviewer.pyreflexio/server/services/playbook/playbook_evidence.pyreflexio/server/services/playbook/playbook_service_constants.pyreflexio/server/services/playbook/playbook_service_utils.pyreflexio/server/services/playbook/service.pyreflexio/test_support/llm_model_registry.pytests/e2e_tests/test_contradiction_resolution_e2e.pytests/eval/extraction/test_extraction_eval.pytests/fixtures/llm/playbook_extraction.jsontests/server/llm/test_litellm_client_unit.pytests/server/llm/test_tools.pytests/server/services/__snapshots__/test_llm_mock_schema_compliance/TestMockResponseSnapshots.test_recorded_fixture_content[playbook_extraction].jsontests/server/services/extraction/test_resume_worker.pytests/server/services/playbook/test_consolidation_lineage_integration.pytests/server/services/playbook/test_extractor_polarity_integration.pytests/server/services/playbook/test_playbook_consolidator.pytests/server/services/playbook/test_playbook_consolidator_integration.pytests/server/services/playbook/test_playbook_extractor.pytests/server/services/playbook/test_playbook_generalization_manifest.pytests/server/services/playbook/test_playbook_generation_service.pytests/server/services/playbook/test_playbook_generation_service_integration.pytests/server/services/playbook/test_playbook_reviewer.pytests/server/services/playbook/test_playbook_service_utils.pytests/server/services/playbook/test_structured_playbook_content.pytests/server/services/prompt/test_prompt_manager.pytests/server/services/test_base_generation_service.pytests/server/services/test_prompt_model_mapping.pytests/test_data/playbook_generalization_manifest.json
| def _is_direct_user_preference_source(source: PlaybookEvidenceSource) -> bool: | ||
| """Return whether a turn can independently establish a durable preference. | ||
|
|
||
| A durable preference must come from the user's own words: an agent turn | ||
| restating what it believes the user wants is not independent evidence. | ||
| """ | ||
| return source.role.strip().casefold() == "user" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject internally generated “User” turns as preference evidence.
Checking role alone accepts generation, memory, or recommender content stored as User, despite the prompt requiring request-source metadata to distinguish agent-authored context. Require a trusted user-origin check on request_source too, or internal text can become a persisted user preference.
🤖 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/playbook/playbook_evidence.py` around lines 50 - 56,
Update _is_direct_user_preference_source to require both a normalized user role
and a trusted user-origin value in request_source; do not accept role alone.
Reuse the existing request_source enum or constant for the trusted user source,
preserving rejection of generated, memory, and recommender-originated turns.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/server/services/playbook/test_playbook_extractor.py (2)
742-866: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
_make_extractoraligned with the prompt-version contract.Its conditional
get_active_versionsetup repeats the same broad1.2.0fallback. Return the correct version for each prompt ID, including extraction1.5.0, so these tests cannot silently use stale prompt fixtures.
666-670: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap extraction prompt versions explicitly.
The fallback returns
1.2.0for every prompt other thanplaybook_extraction_context, so aplaybook_extraction_mainrequest will not exercise the declared1.5.0extraction prompt. Use an explicit per-prompt mapping and fail on unknown IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19f92a76-80ec-4e46-9120-baf4f3378757
📒 Files selected for processing (10)
reflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.6.0.prompt.mdreflexio/server/services/playbook/README.mdreflexio/server/services/playbook/components/consolidator.pyreflexio/server/services/playbook/components/reviewer.pyreflexio/server/services/playbook/service.pytests/server/services/playbook/test_playbook_consolidator.pytests/server/services/playbook/test_playbook_extractor.pytests/server/services/playbook/test_playbook_generation_service.pytests/server/services/playbook/test_playbook_reviewer.pytests/server/services/prompt/test_prompt_manager.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/server/services/prompt/test_prompt_manager.py
- tests/server/services/playbook/test_playbook_reviewer.py
- reflexio/server/services/playbook/README.md
- reflexio/server/services/playbook/components/reviewer.py
- reflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.6.0.prompt.md
- reflexio/server/services/playbook/service.py
- tests/server/services/playbook/test_playbook_consolidator.py
- reflexio/server/services/playbook/components/consolidator.py
817b78f to
ea2a8aa
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
reflexio/server/services/playbook/playbook_evidence.py (1)
53-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreference-source check still ignores
request_source; unresolved from a prior review round.
_is_direct_user_preference_sourceonly checksrole, so an internally generated "User"-role turn (memory/store tool, recommender, generation workflow) can still independently establish a durablepreferenceplaybook — exactly the case the sibling prompt (v4.6.0.prompt.mdlines 112-117) tells the model to distinguish viarequest_source, but the deterministic backstop here doesn't enforce it._referenced_evidence_rejection_reason(line 145-148) has the identical gap, duplicated inline instead of calling this helper — fixing only the helper won't close that second path.🔒 Proposed direction (pending confirmation of the trusted-source constant)
def _is_direct_user_preference_source(source: PlaybookEvidenceSource) -> bool: - return source.role.strip().casefold() == "user" + return ( + source.role.strip().casefold() == "user" + and _is_trusted_user_request_source(source.request_source) + )Apply the equivalent check at the
_referenced_evidence_rejection_reasoncall site (line 145-148), ideally by routing it through_is_direct_user_preference_sourceon aPlaybookEvidenceUnit-compatible shape instead of re-inlining the role check.Run to locate the trusted-source constant/enum this should compare against:
#!/bin/bash rg -n --type=py -C3 '\brequest_source\b' reflexio/server/services/playbook rg -n --type=py -C3 'class .*Source' reflexio/models reflexio/server/servicesAlso applies to: 145-148
🤖 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/playbook/playbook_evidence.py` around lines 53 - 59, Update _is_direct_user_preference_source to require both a normalized user role and the trusted request_source constant or enum identified from the existing model definitions. Replace the duplicated role-only check in _referenced_evidence_rejection_reason with this same helper or equivalent shared predicate, ensuring internally generated User-role evidence cannot establish or validate durable preferences.
🤖 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.
Duplicate comments:
In `@reflexio/server/services/playbook/playbook_evidence.py`:
- Around line 53-59: Update _is_direct_user_preference_source to require both a
normalized user role and the trusted request_source constant or enum identified
from the existing model definitions. Replace the duplicated role-only check in
_referenced_evidence_rejection_reason with this same helper or equivalent shared
predicate, ensuring internally generated User-role evidence cannot establish or
validate durable preferences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7005cf10-7b1d-462e-87c7-2c01d9eb642b
📒 Files selected for processing (50)
reflexio/models/structured_output.pyreflexio/server/llm/_litellm_structured_output.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/llm/_litellm_types.pyreflexio/server/llm/tools.pyreflexio/server/prompt/prompt_bank/playbook_candidate_review/v1.0.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_consolidation/v2.5.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_consolidation/v2.6.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.5.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.6.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_main/v1.4.0.prompt.mdreflexio/server/prompt/prompt_bank/playbook_extraction_main/v1.5.0.prompt.mdreflexio/server/services/base_generation/_extraction_lifecycle.pyreflexio/server/services/base_generation_service.pyreflexio/server/services/extraction/agent_run_records.pyreflexio/server/services/extraction/resumable_agent.pyreflexio/server/services/extraction/resume_worker.pyreflexio/server/services/playbook/README.mdreflexio/server/services/playbook/components/consolidator.pyreflexio/server/services/playbook/components/extractor.pyreflexio/server/services/playbook/components/reviewer.pyreflexio/server/services/playbook/playbook_evidence.pyreflexio/server/services/playbook/playbook_service_constants.pyreflexio/server/services/playbook/playbook_service_utils.pyreflexio/server/services/playbook/service.pyreflexio/server/services/service_utils.pyreflexio/test_support/llm_model_registry.pytests/e2e_tests/test_contradiction_resolution_e2e.pytests/eval/extraction/test_extraction_eval.pytests/fixtures/llm/playbook_extraction.jsontests/server/llm/test_litellm_client_unit.pytests/server/llm/test_tools.pytests/server/services/__snapshots__/test_llm_mock_schema_compliance/TestMockResponseSnapshots.test_recorded_fixture_content[playbook_extraction].jsontests/server/services/extraction/test_resume_worker.pytests/server/services/playbook/test_consolidation_lineage_integration.pytests/server/services/playbook/test_extractor_polarity_integration.pytests/server/services/playbook/test_playbook_consolidator.pytests/server/services/playbook/test_playbook_consolidator_integration.pytests/server/services/playbook/test_playbook_extractor.pytests/server/services/playbook/test_playbook_generalization_manifest.pytests/server/services/playbook/test_playbook_generation_service.pytests/server/services/playbook/test_playbook_generation_service_integration.pytests/server/services/playbook/test_playbook_reviewer.pytests/server/services/playbook/test_playbook_service_utils.pytests/server/services/playbook/test_structured_playbook_content.pytests/server/services/prompt/test_prompt_manager.pytests/server/services/test_base_generation_service.pytests/server/services/test_prompt_model_mapping.pytests/server/services/test_service_utils.pytests/test_data/playbook_generalization_manifest.json
🚧 Files skipped from review as they are similar to previous changes (42)
- reflexio/server/prompt/prompt_bank/playbook_extraction_main/v1.4.0.prompt.md
- reflexio/server/prompt/prompt_bank/playbook_extraction_context/v4.5.0.prompt.md
- tests/fixtures/llm/playbook_extraction.json
- reflexio/server/services/playbook/playbook_service_constants.py
- tests/server/services/snapshots/test_llm_mock_schema_compliance/TestMockResponseSnapshots.test_recorded_fixture_content[playbook_extraction].json
- reflexio/server/services/extraction/agent_run_records.py
- tests/server/services/extraction/test_resume_worker.py
- tests/e2e_tests/test_contradiction_resolution_e2e.py
- tests/server/services/playbook/test_playbook_generalization_manifest.py
- reflexio/test_support/llm_model_registry.py
- reflexio/server/llm/_litellm_text_generation.py
- reflexio/models/structured_output.py
- tests/server/llm/test_tools.py
- reflexio/server/prompt/prompt_bank/playbook_consolidation/v2.5.0.prompt.md
- tests/server/services/test_service_utils.py
- tests/test_data/playbook_generalization_manifest.json
- reflexio/server/prompt/prompt_bank/playbook_extraction_main/v1.5.0.prompt.md
- reflexio/server/services/base_generation_service.py
- tests/server/services/playbook/test_consolidation_lineage_integration.py
- tests/server/services/prompt/test_prompt_manager.py
- reflexio/server/llm/tools.py
- tests/eval/extraction/test_extraction_eval.py
- tests/server/services/playbook/test_structured_playbook_content.py
- reflexio/server/llm/_litellm_types.py
- reflexio/server/services/extraction/resumable_agent.py
- tests/server/services/playbook/test_playbook_generation_service_integration.py
- tests/server/services/test_prompt_model_mapping.py
- tests/server/services/playbook/test_playbook_reviewer.py
- reflexio/server/services/base_generation/_extraction_lifecycle.py
- reflexio/server/prompt/prompt_bank/playbook_candidate_review/v1.0.0.prompt.md
- reflexio/server/services/playbook/service.py
- tests/server/services/playbook/test_extractor_polarity_integration.py
- reflexio/server/services/playbook/components/extractor.py
- tests/server/services/playbook/test_playbook_generation_service.py
- tests/server/services/test_base_generation_service.py
- tests/server/services/playbook/test_playbook_consolidator_integration.py
- reflexio/server/services/playbook/components/reviewer.py
- tests/server/services/playbook/test_playbook_consolidator.py
- tests/server/services/playbook/test_playbook_extractor.py
- reflexio/server/services/extraction/resume_worker.py
- reflexio/server/services/playbook/playbook_service_utils.py
- reflexio/server/services/playbook/components/consolidator.py
Require verbatim evidence and persisted provenance for normal extraction, add a same-model candidate reviewer, make consolidation evidence-aware and fail closed, and advance configured fallbacks after hard LLM timeouts.
ea2a8aa to
696aa2c
Compare
1 participant
Summary
evidence_refs: ["T2"]); code resolves cited turns to bounded source-backed text and persisted request/interaction IDs.4.6.0, extraction1.5.0, reviewer1.0.0, and consolidation2.6.0.Changes
First-pass extraction and evidence
[R#],[T#]) without exposing database IDs.evidence_kind, rationale, trigger, content, and one or more cited turn IDs.Candidate review and consolidation
accept/revise/rejectreview in a fresh same-model conversation.Durable resume and reliability
zai/glm-5.2.Evaluation
Final ten-case normal-path replay with the production model and public API:
The remaining precision failure retained an unsupported claim that narrowing a broad topic produced better content even though no downstream artifact or user outcome was visible. Turn references improve provenance reliability; they do not replace semantic review.
Test Plan
Summary by CodeRabbit