refactor(storage): decompose PlaybookMixin into 5 sub-mixins (SQLite + ABC)#262
Merged
Merged
Conversation
Asserts SQLiteStorage public surface == BaseStorage ABC + the two RetentionMixin extras (delete_oldest_retention_target_rows, count_retention_target_rows). This is the OSS-side drop-a-method tripwire for the playbook decomposition (Task 0).
Day-one invariant guards before the PlaybookMixin decomposition. Pins the current commit/lineage-event/no-op behavior of the top-risk atomicity-sensitive methods so a reorder during the mixin split fails a test. - supersede_user_playbooks_by_ids: effect (CURRENT->SUPERSEDED tombstone, content retained), one status_change->superseded event per UPDATED row under the shared request_id (actor=consolidator), tombstone-guarded no-op emits NO event (phantom-event guard), empty request_id raises before write. - Hard-delete family (delete_agent_playbook, delete_all_agent_playbooks, delete_all_agent_playbooks_by_playbook_name, delete_archived_agent_playbooks_by_playbook_name): crash-window/rollback — a failing lineage emit leaves NEITHER the mutation NOR a phantom hard_delete event durable. Complements the happy/no-op coverage in test_lineage_b1_harddelete_integration.py. Modeled on the gold-standard test_save_agent_playbook_with_aggregate_event_integration.py.
…ods (Tier-1 Task 2)
Behavior-preserving verbatim move of the 18 user-playbook public methods (+ 2 SQLite privates) out of the monolithic PlaybookMixin into a new UserPlaybookStoreMixin, for the SQLite backend and the storage_base ABC. Each backend gains a playbook/ package (playbook/_user.py) re-exported via playbook/__init__.py; the composition base lists add UserPlaybookStoreMixin alongside the now-shrunken PlaybookMixin so the public surface is unchanged. Shared module-level helpers (_emit_hard_delete_playbook, _build_tags_sql) stay in _playbook.py and are imported by _user.py. The user-only _emit_supersede_user_playbook helper moves into _user.py; the atomicity characterization test's monkeypatch target is repointed accordingly. First Phase-B extraction; sets the conventions reused by later buckets.
Move the agent-playbook CRUD + search bucket (18 public methods + the sqlite-private _index_agent_playbook_fts_vec / _insert_agent_playbook_row) out of PlaybookMixin into a new AgentPlaybookStoreMixin in the existing playbook/ package, for both the sqlite concrete backend and the storage_base ABC. Behavior-preserving verbatim move: method bodies are byte-identical and the mutation -> rowcount-guarded emit -> commit-in-lock -> self-committing FTS/vec-after-commit ordering is unchanged. Shared helpers (_emit_hard_delete_playbook, _build_tags_sql, AGGREGATE_REASON_PREFIX, _delete_playbook_search_rows) stay on their existing modules and are imported; the agent-only _emit_supersede_playbook / _AGENT_PLAYBOOK_DEFAULT_EXCLUDED_STATUSES (sqlite) and _AGGREGATE_EVENT_EMIT_ATTEMPTS (ABC) move with the bucket. Compose AgentPlaybookStoreMixin into SQLiteStorage and BaseStorage alongside UserPlaybookStoreMixin. The atomicity/crash-window tests repoint their module patch targets to playbook._agent to follow the relocated symbols.
…sk 5 follow-up) save_agent_playbook_with_aggregate_event + _AGGREGATE_EVENT_EMIT_ATTEMPTS + capture_anomaly moved to storage_base/playbook/_agent.py in the AgentPlaybookStore extraction; this base-default test still imported/patched the old storage_base._playbook locations, breaking the OSS storage suite at collection. Repoint import, patch targets, and unbound-method calls. 3 passed.
…te + ABC) Move the 5 agent-playbook source-linkage methods (set/get_source_user_playbook_ids_for_agent_playbook[s], set/get_source_windows_for_agent_playbook) verbatim out of the shrinking PlaybookMixin into a dedicated PlaybookSourceLinkageMixin in the playbook/ subpackage, composed before PlaybookMixin. Behavior-preserving; bodies are byte-identical. Task 6 of the playbook-storage decomposition.
…e + ABC) Move the 8 plain-CRUD playbook optimization job/candidate/evaluation methods out of the shrinking PlaybookMixin into a new OptimizationJobStoreMixin in the existing playbook/ subpackage, for both the SQLite backend and the storage_base ABC. Behavior-preserving verbatim move; the mixin is composed into SQLiteStorage and BaseStorage immediately before PlaybookMixin, so all method resolution is unchanged. The bucket-only row converters move alongside the methods; shared helpers stay in _playbook.py and are imported.
…(sqlite + ABC) Move the 7 agent-success-evaluation-result CRUD methods out of the shrinking PlaybookMixin into a dedicated AgentEvaluationResultStoreMixin, verbatim, for the sqlite backend and the storage_base ABC. Compose the new mixin into SQLiteStorage and BaseStorage immediately before PlaybookMixin. Final playbook sub-mixin extraction: all 56 playbook methods now live in sub-mixins. Behavior-preserving move; shared module-level helpers stay put and are imported.
All 56 playbook public methods now live in the 5 sub-mixins (UserPlaybookStoreMixin, AgentPlaybookStoreMixin, PlaybookSourceLinkageMixin, OptimizationJobStoreMixin, AgentEvaluationResultStoreMixin) under <backend>/playbook/. The residual PlaybookMixin in sqlite_storage/_playbook.py and storage_base/_playbook.py held zero methods, so drop the class and remove it from the SQLiteStorage / BaseStorage composition (import, base list, __all__). Module-level shared helpers/constants (_emit_hard_delete_playbook, _build_tags_sql, AGGREGATE_REASON_PREFIX) stay — the sub-mixins import them. Supabase/postgres keep their PlaybookMixin (it carries 5 shared private helpers used via MRO); the public method surface is unchanged on every backend.
After the PlaybookMixin decomposition, the module-level logger in the residual sqlite + storage_base _playbook.py files has zero uses (its calls moved into the sub-mixins). Ruff misses it (module-level binding, E402 ignored). Remove the dead import logging + logger = getLogger pair from both. (review-loop finding)
📝 WalkthroughWalkthroughPlaybookMixin is removed from SQLiteStorage and BaseStorage and replaced by five dedicated mixins (agent, user, source linkage, optimization job, evaluation result), each split into an abstract contract under storage_base/playbook and a SQLite implementation under sqlite_storage/playbook. Related tests are added or updated accordingly. ChangesPlaybook storage mixin decomposition
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 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 |
Contributor
Author
|
Paired enterprise PR: ReflexioAI/reflexio-enterprise#491. Merge order: this OSS PR first, then the enterprise PR (its submodule pointer must be re-pinned to this PR's squashed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
tests/server/services/storage/sqlite_storage/test_playbook_atomicity_characterization_integration.py (1)
48-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
_storefixture helper across new test files.The identical
_store(tmp_path, org_id="org-char")helper is also defined verbatim intest_playbook_remaining_methods_integration.py. Consider hoisting it (and similarly-shaped_make_*factories) into a sharedconftest.pyfixture for this test directory to avoid drift between the two files.🤖 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_playbook_atomicity_characterization_integration.py` around lines 48 - 51, The duplicated _store(tmp_path, org_id="org-char") helper in the SQLite storage integration tests should be centralized. Move _store out of test_playbook_atomicity_characterization_integration and test_playbook_remaining_methods_integration into a shared fixture/helper in the directory’s conftest.py, and update both tests to use that shared symbol; also consider the same approach for any matching _make_* factories to keep the test setup consistent.
🤖 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/_agent.py`:
- Around line 677-678: Update the supersede request_id validation to reject
whitespace-only values, not just empty strings. In the relevant supersede
path(s) in _agent.py, replace the current truthiness check so it matches the
stricter contract already used by save_agent_playbook_with_aggregate_event()
with strip()-based validation. Apply the same fix to both supersede call sites
referenced in the review so lineage rows cannot be written with a blank batch
key.
- Around line 846-847: The epoch bound handling in the playbook query uses
truthiness checks, so `datetime.timestamp()` values of 0 are incorrectly treated
as missing. Update the `start_time` and `end_time` filtering logic in
`_agent.py` to use explicit `is not None` checks, matching the pattern already
used in `get_agent_playbooks()`, and apply the same fix anywhere else in the
affected block such as the later filter construction around the referenced epoch
filters.
- Around line 496-574: The update_agent_playbook path only mutates
agent_playbooks, so search_agent_playbooks can return stale results from
agent_playbooks_fts and vector-derived data after edits. In
update_agent_playbook, after a successful commit for
content/trigger/rationale/tag changes, recompute the derived searchable text and
re-upsert the corresponding FTS and embedding/vector records for the same
agent_playbook_id. Use the existing update_agent_playbook method and the search
indexing flow it relies on so the derived search state stays in sync with the
edited playbook.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py`:
- Around line 35-80: The save_agent_success_evaluation_results method is opening
and committing a separate transaction for each AgentSuccessEvaluationResult,
which can leave the batch partially persisted if a later insert fails. Update
save_agent_success_evaluation_results to wrap the entire results loop in one
BEGIN IMMEDIATE/commit pair under self._lock, so all inserts either succeed
together or roll back together; keep the per-row insert logic, embedding
handling, and _assert_subject_writable_locked checks inside that single
transaction.
- Around line 170-188: The delete_agent_success_evaluation_results_by_ids method
still sends all result_ids in one SQLite IN clause, which can exceed the
host-parameter limit for large batches. Update this method to delete in chunks,
or route it through the existing chunked delete helper used for bulk deletes,
and sum each chunk’s rowcount before returning. Use the
delete_agent_success_evaluation_results_by_ids symbol to locate the bulk-delete
path and keep the empty-list no-op behavior unchanged.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py`:
- Around line 746-791: The update path in _user.py for the user playbook write
flow only persists changes to user_playbooks, so semantic edits to content or
trigger leave the search/index state stale. After a successful update in the
update method, detect when semantic_change is true and refresh the derived
search data for that user_playbook by updating the FTS row, embedding, and
vector index using the existing storage/indexing helpers in this module or
surrounding service. Keep the non-semantic status-only path unchanged, and
ensure the refresh happens under the same _lock/commit flow as the main update.
- Around line 637-685: The vector-ranked search paths in the playbook user
storage search logic are ignoring the request similarity threshold, so
below-threshold rows can still be returned. Update the search branches in the
user playbook lookup method that handle SearchMode.VECTOR and the HYBRID
embedding-only / hybrid merge flow to filter ranked rows using request.threshold
before converting them with _row_to_user_playbook. Keep the threshold check
consistent with the existing embedding ranking helpers (_vector_rank_rows and
_true_rrf_merge) so VECTOR and HYBRID results only include matches that meet the
requested cutoff.
- Around line 595-598: Honor SearchOptions.search_mode in the search path by
using the supplied options value when present instead of always reading
request.search_mode. Update the logic around _effective_search_mode in _user.py
so the mode is derived from options.search_mode when options is provided, with
request.search_mode used only as the fallback, and keep the existing
query_embedding/request.query inputs flowing into the same decision point.
- Around line 761-770: The update path in
`_assert_user_playbook_writable_locked`/the surrounding `updates` handling
currently returns silently when `user_playbook_id` is missing, instead of
honoring the documented `ValueError` contract. In the `playbook/_user.py` update
flow, change the missing-target branch to raise `ValueError` with a clear
missing-playbook message rather than returning, so callers of this update method
can distinguish absent IDs from successful no-ops.
In `@reflexio/server/services/storage/storage_base/__init__.py`:
- Around line 35-41: Preserve the deprecated compatibility export for
PlaybookMixin in storage_base so existing imports keep working. Update the
storage_base package export wiring and the playbook module to re-export or
define PlaybookMixin as an alias/shim built from AgentPlaybookStoreMixin,
UserPlaybookStoreMixin, PlaybookSourceLinkageMixin, OptimizationJobStoreMixin,
and AgentEvaluationResultStoreMixin. Keep the symbol available alongside the
newer mixins so from reflexio.server.services.storage.storage_base import
PlaybookMixin does not raise ImportError.
In `@reflexio/server/services/storage/storage_base/playbook/_user.py`:
- Around line 45-47: The `status_filter=None` contract is inconsistent across
the playbook storage APIs, especially in `get_user_playbooks`,
`count_user_playbooks`, and `get_user_playbooks_by_ids`. Update the abstract
docstrings in `_user.py` to explicitly state the default behavior for `None` for
each method (for example, whether it means all statuses, excluding tombstones,
or CURRENT only) so backends implement the same semantics. Keep the wording
aligned with the method names and their actual return behavior to avoid
ambiguous defaults.
---
Nitpick comments:
In
`@tests/server/services/storage/sqlite_storage/test_playbook_atomicity_characterization_integration.py`:
- Around line 48-51: The duplicated _store(tmp_path, org_id="org-char") helper
in the SQLite storage integration tests should be centralized. Move _store out
of test_playbook_atomicity_characterization_integration and
test_playbook_remaining_methods_integration into a shared fixture/helper in the
directory’s conftest.py, and update both tests to use that shared symbol; also
consider the same approach for any matching _make_* factories to keep the test
setup consistent.
🪄 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: 071dfd67-8aa8-4a1a-b626-2df6b74a1ace
📒 Files selected for processing (21)
reflexio/server/services/storage/sqlite_storage/__init__.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/sqlite_storage/playbook/__init__.pyreflexio/server/services/storage/sqlite_storage/playbook/_agent.pyreflexio/server/services/storage/sqlite_storage/playbook/_eval_results.pyreflexio/server/services/storage/sqlite_storage/playbook/_optimization.pyreflexio/server/services/storage/sqlite_storage/playbook/_source_linkage.pyreflexio/server/services/storage/sqlite_storage/playbook/_user.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_playbook.pyreflexio/server/services/storage/storage_base/playbook/__init__.pyreflexio/server/services/storage/storage_base/playbook/_agent.pyreflexio/server/services/storage/storage_base/playbook/_eval_results.pyreflexio/server/services/storage/storage_base/playbook/_optimization.pyreflexio/server/services/storage/storage_base/playbook/_source_linkage.pyreflexio/server/services/storage/storage_base/playbook/_user.pytests/server/services/storage/sqlite_storage/test_playbook_atomicity_characterization_integration.pytests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.pytests/server/services/storage/sqlite_storage/test_save_agent_playbook_with_aggregate_event_integration.pytests/server/services/storage/test_playbook_base_aggregate_emit.pytests/server/services/storage/test_sqlite_surface.py
💤 Files with no reviewable changes (1)
- reflexio/server/services/storage/storage_base/_playbook.py
Comment on lines
+496
to
+574
| def update_agent_playbook( | ||
| self, | ||
| agent_playbook_id: int, | ||
| playbook_name: str | None = None, | ||
| content: str | None = None, | ||
| trigger: str | None = None, | ||
| rationale: str | None = None, | ||
| blocking_issue: BlockingIssue | None = None, | ||
| playbook_status: PlaybookStatus | None = None, | ||
| tags: list[str] | None = None, | ||
| ) -> None: | ||
| updates: list[str] = [] | ||
| params: list[Any] = [] | ||
| if playbook_name is not None: | ||
| updates.append("playbook_name = ?") | ||
| params.append(playbook_name) | ||
| if content is not None: | ||
| updates.append("content = ?") | ||
| params.append(content) | ||
| if trigger is not None: | ||
| updates.append("trigger = ?") | ||
| params.append(trigger) | ||
| if rationale is not None: | ||
| updates.append("rationale = ?") | ||
| params.append(rationale) | ||
| if blocking_issue is not None: | ||
| updates.append("blocking_issue = ?") | ||
| params.append(json.dumps(blocking_issue.model_dump())) | ||
| if playbook_status is not None: | ||
| updates.append("playbook_status = ?") | ||
| params.append(playbook_status.value) | ||
| if tags is not None: | ||
| updates.append("tags = ?") | ||
| params.append(_json_dumps(tags)) | ||
| if updates: | ||
| params.append(agent_playbook_id) | ||
| op = "revise" if content is not None else "status_change" | ||
| prov = "wasRevisionOf" if op == "revise" else "wasInvalidatedBy" | ||
| with self._lock: | ||
| prior_row = self.conn.execute( | ||
| "SELECT playbook_status FROM agent_playbooks WHERE agent_playbook_id = ?", | ||
| (agent_playbook_id,), | ||
| ).fetchone() | ||
| if not prior_row: | ||
| raise ValueError( | ||
| f"Agent playbook with ID {agent_playbook_id} not found" | ||
| ) | ||
| prior_playbook_status = prior_row["playbook_status"] | ||
| cur = self.conn.execute( | ||
| f"UPDATE agent_playbooks SET {', '.join(updates)} WHERE agent_playbook_id = ?", | ||
| tuple(params), | ||
| ) | ||
| if cur.rowcount > 0: | ||
| # Populate structured status fields only when playbook_status is | ||
| # among the updated fields and the op is status_change (not revise). | ||
| if op == "status_change" and playbook_status is not None: | ||
| from_status = prior_playbook_status | ||
| to_status = playbook_status.value | ||
| status_namespace: str | None = "playbook_status" | ||
| else: | ||
| from_status = None | ||
| to_status = None | ||
| status_namespace = None | ||
| _append_event_stmt( | ||
| self.conn, | ||
| org_id=self.org_id, | ||
| entity_type="agent_playbook", | ||
| entity_id=str(agent_playbook_id), | ||
| op=op, | ||
| prov=prov, | ||
| source_ids=[], | ||
| actor="api", | ||
| request_id=uuid.uuid4().hex, | ||
| reason="in-place update", | ||
| from_status=from_status, | ||
| to_status=to_status, | ||
| status_namespace=status_namespace, | ||
| ) | ||
| self.conn.commit() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Refresh derived search data on edit.
This path only updates agent_playbooks, but search_agent_playbooks() reads agent_playbooks_fts and vector state. After a content/trigger edit, full-text results stay stale, and when the embedded text changes the embedding/expanded-terms path stays stale too. Recompute the derived fields and re-upsert the FTS/vector rows after commit.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 522-522: use jsonify instead of json.dumps for JSON output
Context: json.dumps(blocking_issue.model_dump())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.23.0)
[ERROR] 544-547: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 Ruff (0.15.20)
[error] 545-545: Possible SQL injection vector through string-based query construction
(S608)
🤖 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/_agent.py` around
lines 496 - 574, The update_agent_playbook path only mutates agent_playbooks, so
search_agent_playbooks can return stale results from agent_playbooks_fts and
vector-derived data after edits. In update_agent_playbook, after a successful
commit for content/trigger/rationale/tag changes, recompute the derived
searchable text and re-upsert the corresponding FTS and embedding/vector records
for the same agent_playbook_id. Use the existing update_agent_playbook method
and the search indexing flow it relies on so the derived search state stays in
sync with the edited playbook.
Comment on lines
+677
to
+678
| if not request_id: | ||
| raise ValueError("request_id must be non-empty for supersede") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject whitespace-only request_ids in supersede flows.
if not request_id still accepts values like " ", so these methods can emit lineage rows with an effectively blank batch key. save_agent_playbook_with_aggregate_event() already uses strip(); these paths should enforce the same contract.
Also applies to: 740-741
🤖 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/_agent.py` around
lines 677 - 678, Update the supersede request_id validation to reject
whitespace-only values, not just empty strings. In the relevant supersede
path(s) in _agent.py, replace the current truthiness check so it matches the
stricter contract already used by save_agent_playbook_with_aggregate_event()
with strip()-based validation. Apply the same fix to both supersede call sites
referenced in the review so lineage rows cannot be written with a blank batch
key.
Comment on lines
+846
to
+847
| start_time = int(request.start_time.timestamp()) if request.start_time else None | ||
| end_time = int(request.end_time.timestamp()) if request.end_time else None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use is not None for epoch filters.
datetime.timestamp() can legitimately be 0. The truthiness checks here drop that bound, so a request at the Unix epoch is silently broadened instead of filtered. get_agent_playbooks() already handles this correctly.
Also applies to: 868-873
🤖 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/_agent.py` around
lines 846 - 847, The epoch bound handling in the playbook query uses truthiness
checks, so `datetime.timestamp()` values of 0 are incorrectly treated as
missing. Update the `start_time` and `end_time` filtering logic in `_agent.py`
to use explicit `is not None` checks, matching the pattern already used in
`get_agent_playbooks()`, and apply the same fix anywhere else in the affected
block such as the later filter construction around the referenced epoch filters.
Comment on lines
+35
to
+80
| def save_agent_success_evaluation_results( | ||
| self, results: list[AgentSuccessEvaluationResult] | ||
| ) -> None: | ||
| for result in results: | ||
| embedding_text = f"{result.failure_type} {result.failure_reason}" | ||
| if embedding_text.strip(): | ||
| result.embedding = self._get_embedding(embedding_text) | ||
| else: | ||
| result.embedding = [] | ||
|
|
||
| created_at_iso = _epoch_to_iso(result.created_at) | ||
| subject_ref = self._subject_ref_for_user_id(result.user_id) | ||
| with self._lock: | ||
| try: | ||
| self.conn.execute("BEGIN IMMEDIATE") | ||
| self._assert_subject_writable_locked(subject_ref) | ||
| self.conn.execute( | ||
| """INSERT INTO agent_success_evaluation_result | ||
| (user_id, session_id, agent_version, evaluation_name, is_success, | ||
| failure_type, failure_reason, regular_vs_shadow, | ||
| number_of_correction_per_session, user_turns_to_resolution, | ||
| is_escalated, embedding, created_at, governance_subject_ref) | ||
| VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?)""", | ||
| ( | ||
| result.user_id, | ||
| result.session_id, | ||
| result.agent_version, | ||
| result.evaluation_name, | ||
| int(result.is_success), | ||
| result.failure_type, | ||
| result.failure_reason, | ||
| result.regular_vs_shadow.value | ||
| if result.regular_vs_shadow | ||
| else None, | ||
| result.number_of_correction_per_session, | ||
| result.user_turns_to_resolution, | ||
| int(result.is_escalated), | ||
| _json_dumps(result.embedding) if result.embedding else None, | ||
| created_at_iso, | ||
| subject_ref, | ||
| ), | ||
| ) | ||
| self.conn.commit() | ||
| except Exception: | ||
| self.conn.rollback() | ||
| raise |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Save the whole batch in one transaction.
Line 47 opens and commits a separate transaction per row. If a later insert fails, earlier rows stay committed, which leaves a partially replaced result set even though the regenerate flow only deletes prior rows after the new set has been saved durably.
Suggested fix
def save_agent_success_evaluation_results(
self, results: list[AgentSuccessEvaluationResult]
) -> None:
+ prepared_rows: list[tuple[AgentSuccessEvaluationResult, str, Any]] = []
for result in results:
embedding_text = f"{result.failure_type} {result.failure_reason}"
if embedding_text.strip():
result.embedding = self._get_embedding(embedding_text)
else:
result.embedding = []
- created_at_iso = _epoch_to_iso(result.created_at)
- subject_ref = self._subject_ref_for_user_id(result.user_id)
- with self._lock:
- try:
- self.conn.execute("BEGIN IMMEDIATE")
+ prepared_rows.append(
+ (
+ result,
+ _epoch_to_iso(result.created_at),
+ self._subject_ref_for_user_id(result.user_id),
+ )
+ )
+
+ with self._lock:
+ try:
+ self.conn.execute("BEGIN IMMEDIATE")
+ for result, created_at_iso, subject_ref in prepared_rows:
self._assert_subject_writable_locked(subject_ref)
self.conn.execute(
"""INSERT INTO agent_success_evaluation_result
(user_id, session_id, agent_version, evaluation_name, is_success,
failure_type, failure_reason, regular_vs_shadow,
@@
),
)
- self.conn.commit()
- except Exception:
- self.conn.rollback()
- raise
+ self.conn.commit()
+ except Exception:
+ self.conn.rollback()
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def save_agent_success_evaluation_results( | |
| self, results: list[AgentSuccessEvaluationResult] | |
| ) -> None: | |
| for result in results: | |
| embedding_text = f"{result.failure_type} {result.failure_reason}" | |
| if embedding_text.strip(): | |
| result.embedding = self._get_embedding(embedding_text) | |
| else: | |
| result.embedding = [] | |
| created_at_iso = _epoch_to_iso(result.created_at) | |
| subject_ref = self._subject_ref_for_user_id(result.user_id) | |
| with self._lock: | |
| try: | |
| self.conn.execute("BEGIN IMMEDIATE") | |
| self._assert_subject_writable_locked(subject_ref) | |
| self.conn.execute( | |
| """INSERT INTO agent_success_evaluation_result | |
| (user_id, session_id, agent_version, evaluation_name, is_success, | |
| failure_type, failure_reason, regular_vs_shadow, | |
| number_of_correction_per_session, user_turns_to_resolution, | |
| is_escalated, embedding, created_at, governance_subject_ref) | |
| VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?)""", | |
| ( | |
| result.user_id, | |
| result.session_id, | |
| result.agent_version, | |
| result.evaluation_name, | |
| int(result.is_success), | |
| result.failure_type, | |
| result.failure_reason, | |
| result.regular_vs_shadow.value | |
| if result.regular_vs_shadow | |
| else None, | |
| result.number_of_correction_per_session, | |
| result.user_turns_to_resolution, | |
| int(result.is_escalated), | |
| _json_dumps(result.embedding) if result.embedding else None, | |
| created_at_iso, | |
| subject_ref, | |
| ), | |
| ) | |
| self.conn.commit() | |
| except Exception: | |
| self.conn.rollback() | |
| raise | |
| def save_agent_success_evaluation_results( | |
| self, results: list[AgentSuccessEvaluationResult] | |
| ) -> None: | |
| prepared_rows: list[tuple[AgentSuccessEvaluationResult, str, Any]] = [] | |
| for result in results: | |
| embedding_text = f"{result.failure_type} {result.failure_reason}" | |
| if embedding_text.strip(): | |
| result.embedding = self._get_embedding(embedding_text) | |
| else: | |
| result.embedding = [] | |
| prepared_rows.append( | |
| ( | |
| result, | |
| _epoch_to_iso(result.created_at), | |
| self._subject_ref_for_user_id(result.user_id), | |
| ) | |
| ) | |
| with self._lock: | |
| try: | |
| self.conn.execute("BEGIN IMMEDIATE") | |
| for result, created_at_iso, subject_ref in prepared_rows: | |
| self._assert_subject_writable_locked(subject_ref) | |
| self.conn.execute( | |
| """INSERT INTO agent_success_evaluation_result | |
| (user_id, session_id, agent_version, evaluation_name, is_success, | |
| failure_type, failure_reason, regular_vs_shadow, | |
| number_of_correction_per_session, user_turns_to_resolution, | |
| is_escalated, embedding, created_at, governance_subject_ref) | |
| VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?)""", | |
| ( | |
| result.user_id, | |
| result.session_id, | |
| result.agent_version, | |
| result.evaluation_name, | |
| int(result.is_success), | |
| result.failure_type, | |
| result.failure_reason, | |
| result.regular_vs_shadow.value | |
| if result.regular_vs_shadow | |
| else None, | |
| result.number_of_correction_per_session, | |
| result.user_turns_to_resolution, | |
| int(result.is_escalated), | |
| _json_dumps(result.embedding) if result.embedding else None, | |
| created_at_iso, | |
| subject_ref, | |
| ), | |
| ) | |
| self.conn.commit() | |
| except Exception: | |
| self.conn.rollback() | |
| raise |
🤖 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/_eval_results.py`
around lines 35 - 80, The save_agent_success_evaluation_results method is
opening and committing a separate transaction for each
AgentSuccessEvaluationResult, which can leave the batch partially persisted if a
later insert fails. Update save_agent_success_evaluation_results to wrap the
entire results loop in one BEGIN IMMEDIATE/commit pair under self._lock, so all
inserts either succeed together or roll back together; keep the per-row insert
logic, embedding handling, and _assert_subject_writable_locked checks inside
that single transaction.
Comment on lines
+170
to
+188
| def delete_agent_success_evaluation_results_by_ids( | ||
| self, result_ids: list[int] | ||
| ) -> int: | ||
| """Delete agent success eval result rows by primary key. | ||
|
|
||
| Args: | ||
| result_ids (list[int]): Primary-key result_ids to delete. An empty | ||
| list is a no-op that returns 0. | ||
|
|
||
| Returns: | ||
| int: Number of rows actually deleted (ignores non-existent ids). | ||
| """ | ||
| if not result_ids: | ||
| return 0 | ||
| placeholders = ",".join(["?"] * len(result_ids)) | ||
| cur = self._execute( | ||
| f"DELETE FROM agent_success_evaluation_result WHERE result_id IN ({placeholders})", | ||
| list(result_ids), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py reflexio/server/services/storage/sqlite_storage/_base.py reflexio/server/services/storage/sqlite_storage/playbook/_user.py reflexio/server/services/storage/sqlite_storage/playbook/_agent.py
echo '--- outline _eval_results.py ---'
ast-grep outline reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py --view expanded || true
echo '--- outline _base.py ---'
ast-grep outline reflexio/server/services/storage/sqlite_storage/_base.py --view expanded || true
echo '--- relevant slice _eval_results.py ---'
sed -n '150,230p' reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py
echo '--- batch helper search ---'
rg -n "batch_size|chunk|chunked|900|IN \(" reflexio/server/services/storage/sqlite_storage -S
echo '--- delete-by-ids patterns ---'
sed -n '330,430p' reflexio/server/services/storage/sqlite_storage/playbook/_user.py
sed -n '390,470p' reflexio/server/services/storage/sqlite_storage/playbook/_agent.pyRepository: ReflexioAI/reflexio
Length of output: 35837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '150,230p' reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py
sed -n '1,220p' reflexio/server/services/storage/sqlite_storage/_base.py
rg -n "batch_size|chunk|chunked|900|IN \(" reflexio/server/services/storage/sqlite_storage -SRepository: ReflexioAI/reflexio
Length of output: 29570
Chunk large ID deletes to avoid SQLite host-parameter limits. This still binds the full result_ids list in one IN (...) clause, so large cleanup jobs can fail on SQLite’s parameter cap. Batch the delete here, or reuse the existing chunked delete helper, and accumulate rowcount.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 186-186: Possible SQL injection vector through string-based query construction
(S608)
🤖 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/_eval_results.py`
around lines 170 - 188, The delete_agent_success_evaluation_results_by_ids
method still sends all result_ids in one SQLite IN clause, which can exceed the
host-parameter limit for large batches. Update this method to delete in chunks,
or route it through the existing chunked delete helper used for bulk deletes,
and sum each chunk’s rowcount before returning. Use the
delete_agent_success_evaluation_results_by_ids symbol to locate the bulk-delete
path and keep the empty-list no-op behavior unchanged.
Comment on lines
+637
to
+685
| # Pure vector search: fetch all candidates, rank by cosine similarity | ||
| if mode == SearchMode.VECTOR and query_embedding: | ||
| base_where = "WHERE " + " AND ".join(conditions) if conditions else "" | ||
| sql = f"""SELECT * FROM user_playbooks up | ||
| {base_where} | ||
| ORDER BY up.created_at DESC""" | ||
| rows = self._fetchall(sql, params) | ||
| rows = _vector_rank_rows(rows, query_embedding, match_count) | ||
| return [_row_to_user_playbook(r) for r in rows] | ||
|
|
||
| if query: | ||
| fts_query = _sanitize_fts_query(query) | ||
| sql = f"""SELECT up.* FROM user_playbooks up | ||
| JOIN user_playbooks_fts f ON up.user_playbook_id = f.rowid | ||
| WHERE user_playbooks_fts MATCH ?{where_extra} | ||
| ORDER BY bm25(user_playbooks_fts, 1.0) | ||
| LIMIT ?""" | ||
| fts_rows = self._fetchall(sql, [fts_query, *params, overfetch]) | ||
|
|
||
| if mode == SearchMode.HYBRID and query_embedding: | ||
| base_where = "WHERE " + " AND ".join(conditions) if conditions else "" | ||
| vec_limit = match_count * 10 | ||
| vec_sql = f"""SELECT * FROM user_playbooks up | ||
| {base_where} | ||
| ORDER BY up.created_at DESC | ||
| LIMIT ?""" | ||
| vec_candidates = self._fetchall(vec_sql, [*params, vec_limit]) | ||
| vec_rows = _vector_rank_rows(vec_candidates, query_embedding, overfetch) | ||
| rows = _true_rrf_merge( | ||
| fts_rows, | ||
| vec_rows, | ||
| "user_playbook_id", | ||
| match_count, | ||
| rrf_k, | ||
| vector_weight, | ||
| fts_weight, | ||
| ) | ||
| return [_row_to_user_playbook(r) for r in rows] | ||
| return [_row_to_user_playbook(r) for r in fts_rows[:match_count]] | ||
|
|
||
| # HYBRID without query text: rank by embedding only | ||
| if query_embedding: | ||
| base_where = "WHERE " + " AND ".join(conditions) if conditions else "" | ||
| sql = f"""SELECT * FROM user_playbooks up | ||
| {base_where} | ||
| ORDER BY up.created_at DESC""" | ||
| rows = self._fetchall(sql, params) | ||
| rows = _vector_rank_rows(rows, query_embedding, match_count) | ||
| return [_row_to_user_playbook(r) for r in rows] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Apply request.threshold in vector-ranked paths.
The request schema exposes a similarity threshold, but VECTOR/HYBRID embedding branches return the top rows without filtering by request.threshold, which can surface below-threshold matches.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 640-642: Possible SQL injection vector through string-based query construction
(S608)
[error] 649-653: Possible SQL injection vector through string-based query construction
(S608)
[error] 659-662: Possible SQL injection vector through string-based query construction
(S608)
[error] 680-682: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 637 - 685, The vector-ranked search paths in the playbook user storage
search logic are ignoring the request similarity threshold, so below-threshold
rows can still be returned. Update the search branches in the user playbook
lookup method that handle SearchMode.VECTOR and the HYBRID embedding-only /
hybrid merge flow to filter ranked rows using request.threshold before
converting them with _row_to_user_playbook. Keep the threshold check consistent
with the existing embedding ranking helpers (_vector_rank_rows and
_true_rrf_merge) so VECTOR and HYBRID results only include matches that meet the
requested cutoff.
Comment on lines
+746
to
+791
| if content is not None: | ||
| updates.append("content = ?") | ||
| params.append(content) | ||
| if trigger is not None: | ||
| updates.append("trigger = ?") | ||
| params.append(trigger) | ||
| if rationale is not None: | ||
| updates.append("rationale = ?") | ||
| params.append(rationale) | ||
| if blocking_issue is not None: | ||
| updates.append("blocking_issue = ?") | ||
| params.append(json.dumps(blocking_issue.model_dump())) | ||
| if tags is not None: | ||
| updates.append("tags = ?") | ||
| params.append(_json_dumps(tags)) | ||
| if updates: | ||
| params.append(user_playbook_id) | ||
| semantic_change = any( | ||
| value is not None for value in (content, trigger, rationale) | ||
| ) | ||
| op = "revise" if semantic_change else "status_change" | ||
| prov = "wasRevisionOf" if op == "revise" else "wasInvalidatedBy" | ||
| with self._lock: | ||
| if self._assert_user_playbook_writable_locked(user_playbook_id) is None: | ||
| return | ||
| cur = self.conn.execute( | ||
| f"UPDATE user_playbooks SET {', '.join(updates)} WHERE user_playbook_id = ?", | ||
| tuple(params), | ||
| ) | ||
| if cur.rowcount > 0: | ||
| _append_event_stmt( | ||
| self.conn, | ||
| org_id=self.org_id, | ||
| entity_type="user_playbook", | ||
| entity_id=str(user_playbook_id), | ||
| op=op, | ||
| prov=prov, | ||
| source_ids=[], | ||
| actor="api", | ||
| request_id=uuid.uuid4().hex, | ||
| reason="in-place update", | ||
| from_status=None, | ||
| to_status=None, | ||
| status_namespace=None, | ||
| ) | ||
| self.conn.commit() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Refresh search state after semantic updates.
Updating content or trigger changes the searchable document, but this path only updates user_playbooks; the FTS row, embedding, and vector index remain stale, so search_user_playbooks can return results based on old text.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 756-756: use jsonify instead of json.dumps for JSON output
Context: json.dumps(blocking_issue.model_dump())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.23.0)
[ERROR] 771-774: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 Ruff (0.15.20)
[error] 772-772: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 746 - 791, The update path in _user.py for the user playbook write flow
only persists changes to user_playbooks, so semantic edits to content or trigger
leave the search/index state stale. After a successful update in the update
method, detect when semantic_change is true and refresh the derived search data
for that user_playbook by updating the FTS row, embedding, and vector index
using the existing storage/indexing helpers in this module or surrounding
service. Keep the non-semantic status-only path unchanged, and ensure the
refresh happens under the same _lock/commit flow as the main update.
Comment on lines
+761
to
+770
| if updates: | ||
| params.append(user_playbook_id) | ||
| semantic_change = any( | ||
| value is not None for value in (content, trigger, rationale) | ||
| ) | ||
| op = "revise" if semantic_change else "status_change" | ||
| prov = "wasRevisionOf" if op == "revise" else "wasInvalidatedBy" | ||
| with self._lock: | ||
| if self._assert_user_playbook_writable_locked(user_playbook_id) is None: | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Raise ValueError when the target playbook does not exist.
The abstract contract documents ValueError for missing IDs, but this implementation silently returns, making failed updates indistinguishable from successful no-ops.
Proposed fix
with self._lock:
if self._assert_user_playbook_writable_locked(user_playbook_id) is None:
- return
+ raise ValueError(
+ f"User playbook {user_playbook_id} not found"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if updates: | |
| params.append(user_playbook_id) | |
| semantic_change = any( | |
| value is not None for value in (content, trigger, rationale) | |
| ) | |
| op = "revise" if semantic_change else "status_change" | |
| prov = "wasRevisionOf" if op == "revise" else "wasInvalidatedBy" | |
| with self._lock: | |
| if self._assert_user_playbook_writable_locked(user_playbook_id) is None: | |
| return | |
| if updates: | |
| params.append(user_playbook_id) | |
| semantic_change = any( | |
| value is not None for value in (content, trigger, rationale) | |
| ) | |
| op = "revise" if semantic_change else "status_change" | |
| prov = "wasRevisionOf" if op == "revise" else "wasInvalidatedBy" | |
| with self._lock: | |
| if self._assert_user_playbook_writable_locked(user_playbook_id) is None: | |
| raise ValueError( | |
| f"User playbook {user_playbook_id} not found" | |
| ) |
🤖 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 761 - 770, The update path in `_assert_user_playbook_writable_locked`/the
surrounding `updates` handling currently returns silently when
`user_playbook_id` is missing, instead of honoring the documented `ValueError`
contract. In the `playbook/_user.py` update flow, change the missing-target
branch to raise `ValueError` with a clear missing-playbook message rather than
returning, so callers of this update method can distinguish absent IDs from
successful no-ops.
Comment on lines
+35
to
+41
| from .playbook import ( | ||
| AgentEvaluationResultStoreMixin, | ||
| AgentPlaybookStoreMixin, | ||
| OptimizationJobStoreMixin, | ||
| PlaybookSourceLinkageMixin, | ||
| UserPlaybookStoreMixin, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep PlaybookMixin as a deprecated compatibility export.
Dropping the symbol from storage_base turns existing from reflexio.server.services.storage.storage_base import PlaybookMixin imports into ImportErrors, which breaks the stated “preserve the existing public surface” goal for this refactor.
Compatibility shim sketch
from .playbook import (
AgentEvaluationResultStoreMixin,
AgentPlaybookStoreMixin,
OptimizationJobStoreMixin,
+ PlaybookMixin,
PlaybookSourceLinkageMixin,
UserPlaybookStoreMixin,
)
@@
"PendingToolCallUpsertResult",
"AgentEvaluationResultStoreMixin",
"AgentPlaybookStoreMixin",
"OptimizationJobStoreMixin",
+ "PlaybookMixin",
"PlaybookSourceLinkageMixin",
"UserPlaybookStoreMixin",# reflexio/server/services/storage/storage_base/playbook/__init__.py
class PlaybookMixin(
AgentPlaybookStoreMixin,
UserPlaybookStoreMixin,
PlaybookSourceLinkageMixin,
OptimizationJobStoreMixin,
AgentEvaluationResultStoreMixin,
):
"""Deprecated compatibility alias for the pre-split mixin."""
passAlso applies to: 247-263
🤖 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/__init__.py` around lines 35 -
41, Preserve the deprecated compatibility export for PlaybookMixin in
storage_base so existing imports keep working. Update the storage_base package
export wiring and the playbook module to re-export or define PlaybookMixin as an
alias/shim built from AgentPlaybookStoreMixin, UserPlaybookStoreMixin,
PlaybookSourceLinkageMixin, OptimizationJobStoreMixin, and
AgentEvaluationResultStoreMixin. Keep the symbol available alongside the newer
mixins so from reflexio.server.services.storage.storage_base import
PlaybookMixin does not raise ImportError.
Comment on lines
+45
to
+47
| status_filter (list[Optional[Status]], optional): List of status values to filter by. | ||
| Can include None (current), Status.PENDING (from rerun), Status.ARCHIVED (old). | ||
| If None, returns playbooks with all statuses. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the status_filter=None contract across methods.
These docstrings conflict with the SQLite behavior: get_user_playbooks / count_user_playbooks default to excluding tombstones, while get_user_playbooks_by_ids defaults to CURRENT only. Please make the abstract contract explicit so other storage backends don’t implement different defaults.
Proposed wording adjustment
- If None, returns playbooks with all statuses.
+ If None, returns playbooks except tombstones (MERGED/SUPERSEDED).
...
- If None, returns playbooks with all statuses.
+ If None, counts playbooks except tombstones (MERGED/SUPERSEDED).
...
- include. ``None`` (default) means CURRENT only — same
- default as ``get_user_playbooks`` for consistency.
+ include. ``None`` (default) means CURRENT only.Also applies to: 75-77, 197-199
🤖 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/playbook/_user.py` around lines
45 - 47, The `status_filter=None` contract is inconsistent across the playbook
storage APIs, especially in `get_user_playbooks`, `count_user_playbooks`, and
`get_user_playbooks_by_ids`. Update the abstract docstrings in `_user.py` to
explicitly state the default behavior for `None` for each method (for example,
whether it means all statuses, excluding tombstones, or CURRENT only) so
backends implement the same semantics. Keep the wording aligned with the method
names and their actual return behavior to avoid ambiguous defaults.
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 1, 2026
…263) ## Post-decomposition cleanup: dead type-stubs + a brittle-test fix Small follow-up to the playbook decomposition (#262). - **Remove orphaned MRO type-stub attributes** that the mixin split left unused in their sub-mixin (`sqlite_storage/playbook/_agent.py`, `_user.py`). Type-only annotations with zero runtime effect; each removed stub was verified unused in its file and pyright stays clean. (Surfaced by the whole-branch review.) - **Fix `test_get_citations_by_session_ids_extracts_cited_rows`** (pre-existing brittle test): it ran a raw `storage.conn.execute("UPDATE ...")` on the shared connection without committing, leaving an implicit transaction open, so the next `add_request()`'s `BEGIN IMMEDIATE` raised *"cannot start a transaction within a transaction."* Commit the raw UPDATEs. Full OSS storage suite: **844 passed, 0 failures** (this was the last remaining red). pyright + ruff clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved SQLite data handling so updates are reliably saved before citation lookups run, reducing inconsistent results in edge cases. * Kept playbook storage behavior aligned with current SQLite capabilities, helping avoid unexpected issues during search and delete operations. * **Tests** * Updated test coverage to reflect committed database changes before citation retrieval checks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 1, 2026
… ABC) (#264) ## Decompose `ProfileMixin` into 3 sub-mixins (SQLite + ABC) Second Tier-1 storage decomposition (after playbook, #262/#491). Splits the 1,293-line sqlite `ProfileMixin` (and its abstract `storage_base` twin) into three focused sub-mixins under a per-concern `profiles/` package, composed *exactly* as before. **Zero public-surface, contract, or behavior change.** OSS half of a two-repo change (the enterprise Supabase/Postgres twin is a companion PR). ### The 3 sub-mixins (`sqlite_storage/profiles/`, `storage_base/profiles/`) | Sub-mixin | Responsibility | Methods | |---|---|---| | `ProfileStoreMixin` | profile CRUD + supersede/archive/status | 20 | | `InteractionStoreMixin` | interaction CRUD/bulk/delete + insert atomicity | 9 | | `ProfileSearchMixin` | `search_interaction` + `search_user_profile` | 2 | `SQLiteStorage` and `BaseStorage` compose the 3 sub-mixins directly; the empty residual `ProfileMixin` class was removed (`storage_base/_profiles.py` deleted; the sqlite residual `_profiles.py` retains only the shared `_build_tags_sql` helper, imported by the sub-mixins). > **Search asymmetry (by design):** the two `unified_hybrid_search*` methods are Supabase/Postgres-only and live in the *enterprise* `ProfileSearchMixin` — absent here (sqlite/ABC). The surface guard encodes this (`_ENTERPRISE_ONLY_METHODS`). ### Safety model — characterize before moving, verify byte-identical - **Phase A** built the net first: storage-level `search_interaction` + `get_user_ids_with_status` coverage (previously zero), and crash-window atomicity tests for the profile mutation/status paths + interaction insert (the existing guard covered deletes only). Every atomicity test is **non-vacuous** — it fails if the emit→commit ordering breaks. - **Every moved method body is BYTE-IDENTICAL** (AST-verified per bucket), preserving the `mutation → rowcount-guarded event → commit → self-committing FTS/vec after` ordering and every `commit=` arg. ### Verification Executed one bucket at a time, each independently reviewed + gated by the OSS storage contract suite + surface guard + the atomicity tests, then a whole-branch `/review-loop`. OSS storage suite: **876 passed, 0 collection errors**; surface guard green; ruff + pyright clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added dedicated storage support for profiles, interactions, and search in SQLite. * Improved profile and interaction lookup with richer filtering, text search, and embedding-based matching. * Added support for bulk interaction and profile lifecycle operations. * **Bug Fixes** * Made data updates more reliable by keeping writes atomic and rollback-safe. * Improved user data removal so archived and superseded records are handled correctly. * Ensured deletes also keep search indexes in sync. * **Tests** * Added integration coverage for profile mutations, interaction deletes, and search behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Decompose the monolithic
PlaybookMixininto 5 cohesive sub-mixins (SQLite + ABC)Splits the 2,303-line
PlaybookMixin(56 public methods) — mirrored by the abstractstorage_basetwin — into five focused sub-mixins under a per-concernplaybook/package, composed exactly as before. Zero public-surface, contract, or behavior change. This is the OSS half of a two-repo Tier-1 decomposition (the enterprise Supabase/Postgres twin is a companion PR).The 5 sub-mixins (per backend:
sqlite_storage/playbook/,storage_base/playbook/)UserPlaybookStoreMixinAgentPlaybookStoreMixinPlaybookSourceLinkageMixinOptimizationJobStoreMixinAgentEvaluationResultStoreMixinSQLiteStorageand theBaseStorageABC now compose the 5 sub-mixins directly; the empty residualPlaybookMixinclass was removed (module-level shared helpers stay in_playbook.pyand are imported)._delete_playbook_search_rowsremains on the shared base.Safety model — characterize before moving
SQLite ≡ Supabase ≡ Postgres ≡ BaseStoragepublic surface), atomicity/crash-window tests for the gold-standard atomic save + the hard-delete family (asserting: row+event commit atomically, FTS/vec runs after commit, no phantom event on rollback orrowcount==0no-op), and round-trip coverage for every previously-uncovered method.mutation → rowcount-guarded emit → commit-in-lock → self-committing FTS/vec after commitordering and everycommit=argument verbatim.Verification
Behavior-preserving move, done one bucket at a time, each gated by the OSS storage contract suite + the surface guard + the atomicity tests. Final: OSS storage suite green (0 collection errors), surface guard green, atomicity tests green, ruff + pyright clean.
Summary by CodeRabbit
New Features
Bug Fixes