fix(playbook): reconstruct full provenance for manual review - #397
Merged
Conversation
📝 WalkthroughWalkthroughPersisted playbook reviews now reconstruct finalized generation windows from stored provenance, rebuild cited evidence from exact interaction IDs, skip unreconstructable rows, preserve successor request lineage, and use synchronous timeouts for the review endpoint. ChangesPersisted playbook review hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewAPI
participant UserPlaybookReviewService
participant AgentRunStore
participant InteractionStorage
participant PlaybookCandidateReviewer
ReviewAPI->>UserPlaybookReviewService: review persisted playbooks
UserPlaybookReviewService->>AgentRunStore: load latest finalized playbook run
AgentRunStore-->>UserPlaybookReviewService: finalized provenance
UserPlaybookReviewService->>InteractionStorage: load and validate requests and interactions
InteractionStorage-->>UserPlaybookReviewService: chronological review window
UserPlaybookReviewService->>PlaybookCandidateReviewer: decide using cited interaction IDs
PlaybookCandidateReviewer-->>UserPlaybookReviewService: decision or evidence error
UserPlaybookReviewService-->>ReviewAPI: reviewed or skipped result
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
reflexio/client/client.py (1)
1622-1637: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport-only reviews can exceed the default client timeout
review_user_playbooks()uses the client defaulttimeout=300, while/api/review_user_playbooksallows 600s server-side. Long report-only runs will still fail client-side unless callers raise the timeout for this endpoint.🤖 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/client/client.py` around lines 1622 - 1637, Update review_user_playbooks to use an endpoint-specific client timeout of 600 seconds, matching the /api/review_user_playbooks server limit, so default report-only calls can complete without requiring callers to override the timeout.
🧹 Nitpick comments (3)
tests/server/services/storage/sqlite_storage/test_agent_run_storage.py (1)
129-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the "latest" ordering and
FINALIZED_PENDING_TOOL.The test proves filtering but not the two remaining contract clauses: newest-first selection among multiple finalized runs for the same request, and that
FINALIZED_PENDING_TOOLrows qualify. Both are silent-regression prone since ordering relies on timestamp string comparison.🤖 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_agent_run_storage.py` around lines 129 - 147, Extend the test around get_latest_finalized_agent_run_for_request to add multiple qualifying finalized runs for the same request with differing timestamps, asserting the newest run is selected, and add a FINALIZED_PENDING_TOOL row that is accepted by the query. Keep the existing organization, extractor, user, and request filtering assertions intact.reflexio/server/services/playbook/review_service.py (1)
145-157: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBatch the request lookup. This loop does one
get_request()call per distinctrequest_id, so large review windows turn into an N+1 storage pattern. Add a bulkget_requests_by_ids()path and use it here.🤖 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/review_service.py` around lines 145 - 157, Replace the per-item self.storage.get_request call in the review-window loop with a bulk self.storage.get_requests_by_ids lookup for all request IDs before iteration. Index the returned requests by ID, then preserve the existing missing-request and ownership validation in the loop using that indexed result.reflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.py (1)
225-246: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a more selective index for this lookup.
idx_agent_runs_bindingonly covers(org_id, extractor_kind, user_id), so this query still has to filterrequest_idandstatusand then sort bycreated_at,updated_at, andid. A composite index that matches the full predicate and ordering would reduce work on this path.🤖 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/agent_run/_agent_run_store.py` around lines 225 - 246, Add a composite SQLite index for the lookup in the agent-run store, covering org_id, extractor_kind, user_id, request_id, status, and the created_at/updated_at/id descending ordering. Register it alongside the existing idx_agent_runs_binding definition and ensure the schema/migration path creates it.
🤖 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.
Outside diff comments:
In `@reflexio/client/client.py`:
- Around line 1622-1637: Update review_user_playbooks to use an
endpoint-specific client timeout of 600 seconds, matching the
/api/review_user_playbooks server limit, so default report-only calls can
complete without requiring callers to override the timeout.
---
Nitpick comments:
In `@reflexio/server/services/playbook/review_service.py`:
- Around line 145-157: Replace the per-item self.storage.get_request call in the
review-window loop with a bulk self.storage.get_requests_by_ids lookup for all
request IDs before iteration. Index the returned requests by ID, then preserve
the existing missing-request and ownership validation in the loop using that
indexed result.
In
`@reflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.py`:
- Around line 225-246: Add a composite SQLite index for the lookup in the
agent-run store, covering org_id, extractor_kind, user_id, request_id, status,
and the created_at/updated_at/id descending ordering. Register it alongside the
existing idx_agent_runs_binding definition and ensure the schema/migration path
creates it.
In `@tests/server/services/storage/sqlite_storage/test_agent_run_storage.py`:
- Around line 129-147: Extend the test around
get_latest_finalized_agent_run_for_request to add multiple qualifying finalized
runs for the same request with differing timestamps, asserting the newest run is
selected, and add a FINALIZED_PENDING_TOOL row that is accepted by the query.
Keep the existing organization, extractor, user, and request filtering
assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74747c5f-6515-4cb4-b804-7fe1139ce05b
📒 Files selected for processing (14)
reflexio/client/client.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/middleware.pyreflexio/server/prompt/prompt_bank/playbook_candidate_review/v1.0.0.prompt.mdreflexio/server/services/playbook/README.mdreflexio/server/services/playbook/components/reviewer.pyreflexio/server/services/playbook/review_service.pyreflexio/server/services/storage/sqlite_storage/agent_run/_agent_run_store.pyreflexio/server/services/storage/storage_base/agent_run/_agent_run_store.pytests/server/services/playbook/test_playbook_generation_service.pytests/server/services/playbook/test_playbook_review_service_integration.pytests/server/services/playbook/test_playbook_reviewer.pytests/server/services/storage/sqlite_storage/test_agent_run_storage.pytests/server/test_api_security_middleware.py
yyiilluu
changed the title
Contributor
Author
|
Follow-up review addressed in 61bc74c:
I did not add a cross-backend bulk request API or a specialized SQLite index in this PR: this admin path is explicitly bounded by |
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
main; this PR is limited to provenance and evidence plumbing.Changes
Historical provenance
Evidence and resilience
Verification and docs
Follow-ups
FINALIZED_PENDING_TOOL, deterministic timestamp ties, and report-only storage immutability via a full SQLite dump digest.Test Plan
main.Summary by CodeRabbit
Bug Fixes
Documentation
Tests