Lineage Phase B1 — complete instrumentation (OSS/SQLite): update/hard_delete/status_change + reflection supersede + op=aggregate#187
Merged
Conversation
…s — SQLite (delete_profiles_by_ids + single-row + bulk/GDPR wipe paths)
…3), SELECT-in-lock for bulk wipes, dedupe emit helper
…APPROVED-guard test, test/SQL cleanup
… (closes Phase A gap) - Replace archive_profile_by_id in _replace_profile with supersede_record - Cited profile becomes SUPERSEDED with superseded_by pointer + revise lineage event - CAS race lost: delete just-added successor (emit_hard_delete=False) to avoid orphan - Add test_reflection_profile_supersede_integration.py: asserts SUPERSEDED status, superseded_by pointer, and revise event after a reflection profile revision - Update test_reflection_service.py: TestReplaceProfile now checks Status.SUPERSEDED with superseded_by pointer; TestPerDecisionMalformed checks SUPERSEDED not ARCHIVED; TestArchiveAfterInsertFailure updated to patch supersede_record and assert new semantics
…se event; fix stale sentry op tag
… fix pyright ignore pragma
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds lineage event emission ( ChangesLineage event emission: storage, reflection, and aggregation
Sequence Diagram(s)sequenceDiagram
participant GenerationService
participant ReflectionServiceRequest
participant ReflectionService
participant storage
GenerationService->>GenerationService: generate request_id
GenerationService->>ReflectionService: _maybe_run_reflection(..., request_id)
ReflectionService->>ReflectionServiceRequest: construct(request_id=request_id)
ReflectionService->>storage: insert successor profile
ReflectionService->>storage: supersede_record(incumbent_id, successor_id, LineageContext(op="revise", request_id))
alt CAS success
storage-->>ReflectionService: True — incumbent becomes SUPERSEDED
else CAS race lost
storage-->>ReflectionService: False
ReflectionService->>storage: delete_profiles_by_ids(successor_id, emit_hard_delete=False)
end
sequenceDiagram
participant PlaybookAggregator
participant SQLiteStorage
PlaybookAggregator->>SQLiteStorage: save agent playbook(s)
PlaybookAggregator->>SQLiteStorage: append_lineage_event(op="aggregate", prov="wasDerivedFrom", source_ids=[user_pb_ids])
alt success
SQLiteStorage-->>PlaybookAggregator: ok
else append raises
PlaybookAggregator->>PlaybookAggregator: log warning + capture_anomaly("lineage.aggregate.append_failed")
PlaybookAggregator-->>PlaybookAggregator: continue (best-effort)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/storage/sqlite_storage/_profiles.py (1)
286-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the profile sidecar cleanup and audit event to the row being deleted.
rowid_rowis selected byprofile_idonly, then the method emitshard_deleteand deletes FTS/vec sidecars before theDELETE ... WHERE user_id = ? AND profile_id = ?confirms a row was removed. A mismatcheduser_idcan therefore erase another profile’s search sidecars and record a phantom delete. Selectrowidwith bothuser_idandprofile_idinside_lock, and return without emitting if no row matches.Suggested shape
- rowid_row = self._fetchone( - "SELECT rowid FROM profiles WHERE profile_id = ?", - (request.profile_id,), - ) with self._lock: + rowid_row = self.conn.execute( + "SELECT rowid FROM profiles WHERE user_id = ? AND profile_id = ?", + (request.user_id, request.profile_id), + ).fetchone() + if not rowid_row: + return _emit_hard_delete_profile(🤖 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/_profiles.py` around lines 286 - 304, The rowid_row selection uses only profile_id outside the lock, then the hard delete audit event and FTS/vector sidecar cleanup operations occur inside the lock before the actual DELETE statement which validates both user_id and profile_id. This creates a security issue where a mismatched user_id can cause sidecars to be deleted for a profile that shouldn't be deleted. Move the rowid_row selection inside the _lock block and update its WHERE clause to include both user_id and profile_id conditions. Check if rowid_row is found after this selection, and only execute _emit_hard_delete_profile, _fts_delete_profile, and _vec_delete if a matching row exists, returning early if no row matches both conditions.
🧹 Nitpick comments (3)
tests/server/services/storage/test_lineage_b1_update_integration.py (1)
45-54: ⚡ Quick winPin
request_iduniqueness across sequential updates in this test.Since the intent is “distinct event per call,” also assert the two
reviseevents have differentrequest_idvalues. That directly validates the per-call UUID guarantee behind idempotency.🤖 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/test_lineage_b1_update_integration.py` around lines 45 - 54, The test_update_user_playbook_multiple_edits_each_produce_event function verifies that multiple updates produce distinct revise events, but it does not validate that each event has a unique request_id value. Add an assertion after checking the operation types that compares the request_id values of the two revise events (accessing them via e.request_id from the events returned by get_lineage_events) to ensure they are different, which directly validates the per-call UUID guarantee for idempotency.tests/server/services/storage/test_lineage_b1_harddelete_integration.py (1)
107-121: ⚡ Quick winAssert shared
request_idacross the same bulk delete call.This test checks one
hard_deleteper id, but not that both events share the same batchrequest_id. Adding that assertion would better lock the B1 idempotency contract for multi-row operations.🤖 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/test_lineage_b1_harddelete_integration.py` around lines 107 - 121, The test function test_delete_profiles_by_ids_multiple_emits_one_event_per_id currently validates that each profile deletion generates one hard_delete event, but does not verify that both events share the same request_id. Add assertions after the existing length assertions to extract the request_id from the hard_delete event in events_a and events_b, and assert that they are equal. This ensures that multiple profiles deleted in the same bulk operation are tracked with a shared request_id for the B1 idempotency contract.tests/server/services/storage/test_lineage_b1_statuschange_integration.py (1)
72-90: ⚡ Quick winAlso verify batch
request_idcoherence forstatus_changeevents.You already validate one event per id; add an assertion that both emitted events from this single archive call carry the same
request_id(and are non-empty). That strengthens contract coverage for lineage batching.🤖 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/test_lineage_b1_statuschange_integration.py` around lines 72 - 90, The test test_archive_agent_playbooks_by_ids_emits_one_event_per_id validates that one event per id is emitted but does not verify request_id coherence across the batch. Enhance the test by adding an assertion after the existing event count validation to confirm that all status_change events from both ids carry the same non-empty request_id. Collect all status_change events from both apids and verify they share an identical request_id value that is not empty or None, ensuring the lineage batching contract is properly covered.
🤖 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/reflection/reflection_service_utils.py`:
- Line 43: The request_id parameter has a default value of empty string, which
allows callers to omit it and unintentionally emit revise events with unlinked
lineage. Remove the default value assignment from the request_id parameter
definition (change from `request_id: str = ""` to just `request_id: str`) to
make it a required parameter, ensuring all callers must explicitly provide a
non-empty request_id when creating LineageContext instances.
In `@reflexio/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 329-344: The delete_user_playbook method emits a hard_delete audit
event before verifying the row actually exists, which creates phantom deletion
records for stale or already-deleted IDs. Before calling
_emit_hard_delete_playbook, first query the database to check if a row with the
given user_playbook_id exists in user_playbooks, then only emit the hard_delete
event if the row is found. If no row exists, skip the _emit_hard_delete_playbook
call entirely to avoid recording false deletion events. Apply the same fix to
the other by-id delete methods referenced at lines 386-395, 853-868, and
911-920.
- Around line 947-964: The lineage event is being appended even when the UPDATE
statement affects zero rows due to a race condition where another writer may
delete the playbook between the existence check and the UPDATE. Capture the
cursor object returned by the self.conn.execute() call for the UPDATE statement,
and wrap the _append_event_stmt call in a conditional that only executes when
the cursor's rowcount property is greater than zero. Apply this same fix to all
three locations mentioned: the current block in the diff and also in the other
two similar update methods at the specified line ranges.
- Around line 324-325: The bulk playbook deletion logic in the hard-delete/GDPR
paths deletes from user_playbooks_fts and user_playbooks tables but leaves
orphaned vector sidecar entries in user_playbooks_vec and agent_playbooks_vec
tables. Add DELETE statements for both vector sidecar tables (user_playbooks_vec
and agent_playbooks_vec) in the same locked transaction, executing these deletes
before removing the base playbook rows. Apply this fix to all bulk delete
locations mentioned in the comment: the main deletion block around lines 324-325
and all other occurrences at lines 371-375, 397-403, 848-849, 896-900, 922-928,
and 1464-1468 to ensure vector embeddings are properly cleaned up during
hard-deletes.
- Around line 1094-1122: The archive operation is emitting false status_change
events for playbooks that are already archived (showing archived->archived when
no state actually changed). Modify the WHERE clause construction in the archive
query to exclude not only APPROVED playbooks but also playbooks that already
have status set to 'archived', so that only real state transitions trigger event
emissions. Update the WHERE clause string and add appropriate parameters to
filter out both statuses.
In `@reflexio/server/services/storage/sqlite_storage/_profiles.py`:
- Around line 306-346: The delete_all_profiles_for_user and delete_all_profiles
methods are removing rows from the profiles and profiles_fts tables but not
cleaning up the corresponding vector embeddings stored in the profiles_vec
table. Since these are hard-delete operations that may include GDPR/user-erasure
flows, the vector sidecar data must be removed in the same locked transaction.
Add deletion statements to remove rows from profiles_vec: in
delete_all_profiles_for_user, delete from profiles_vec for all profile IDs in
the pids list, and in delete_all_profiles, delete all rows from profiles_vec.
Execute these deletions before calling conn.commit() to ensure they are part of
the same atomic transaction as the base deletions.
- Around line 221-260: The revise event is being appended unconditionally after
the UPDATE statement, even if no rows were actually updated because the profile
may have been deleted or superseded. Capture the cursor object returned from the
self.conn.execute() call that performs the UPDATE, and wrap the
_append_event_stmt() function call in a conditional block that only executes
when cur.rowcount is greater than 0. This ensures the lineage event is only
recorded when the profile update actually succeeds in modifying a row.
- Around line 527-539: The `delete_profiles_by_ids` method currently emits hard
delete events and deletes for all requested profile IDs without verifying they
exist in the database first. Before calling `_emit_hard_delete_profile` and
performing the actual deletion, query the profiles table to select only the
profile IDs that actually exist from the requested list (using a SELECT query
with the same IN clause within the `_lock` context). Then iterate over only the
filtered set of existing profile IDs when emitting hard delete events via
`_emit_hard_delete_profile` and when deleting via `_fts_delete_profile`. If no
profiles exist in the requested set, return 0 without emitting any audit events
or performing unnecessary delete operations.
- Around line 456-476: The archive_profile_by_id method only commits the
database transaction when cur.rowcount > 0, which means no-op archive attempts
(when the profile is already archived or doesn't exist) leave the connection in
an open write transaction. Move self.conn.commit() outside the if cur.rowcount >
0 block so it executes regardless of whether rows were updated, while keeping
the _append_event_stmt call inside the conditional since events should only be
emitted when the status actually changes.
---
Outside diff comments:
In `@reflexio/server/services/storage/sqlite_storage/_profiles.py`:
- Around line 286-304: The rowid_row selection uses only profile_id outside the
lock, then the hard delete audit event and FTS/vector sidecar cleanup operations
occur inside the lock before the actual DELETE statement which validates both
user_id and profile_id. This creates a security issue where a mismatched user_id
can cause sidecars to be deleted for a profile that shouldn't be deleted. Move
the rowid_row selection inside the _lock block and update its WHERE clause to
include both user_id and profile_id conditions. Check if rowid_row is found
after this selection, and only execute _emit_hard_delete_profile,
_fts_delete_profile, and _vec_delete if a matching row exists, returning early
if no row matches both conditions.
---
Nitpick comments:
In `@tests/server/services/storage/test_lineage_b1_harddelete_integration.py`:
- Around line 107-121: The test function
test_delete_profiles_by_ids_multiple_emits_one_event_per_id currently validates
that each profile deletion generates one hard_delete event, but does not verify
that both events share the same request_id. Add assertions after the existing
length assertions to extract the request_id from the hard_delete event in
events_a and events_b, and assert that they are equal. This ensures that
multiple profiles deleted in the same bulk operation are tracked with a shared
request_id for the B1 idempotency contract.
In `@tests/server/services/storage/test_lineage_b1_statuschange_integration.py`:
- Around line 72-90: The test
test_archive_agent_playbooks_by_ids_emits_one_event_per_id validates that one
event per id is emitted but does not verify request_id coherence across the
batch. Enhance the test by adding an assertion after the existing event count
validation to confirm that all status_change events from both ids carry the same
non-empty request_id. Collect all status_change events from both apids and
verify they share an identical request_id value that is not empty or None,
ensuring the lineage batching contract is properly covered.
In `@tests/server/services/storage/test_lineage_b1_update_integration.py`:
- Around line 45-54: The
test_update_user_playbook_multiple_edits_each_produce_event function verifies
that multiple updates produce distinct revise events, but it does not validate
that each event has a unique request_id value. Add an assertion after checking
the operation types that compares the request_id values of the two revise events
(accessing them via e.request_id from the events returned by get_lineage_events)
to ensure they are different, which directly validates the per-call UUID
guarantee for idempotency.
🪄 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: ad3eafa5-801c-400d-8d3f-d9225c34133d
📒 Files selected for processing (14)
reflexio/server/services/generation_service.pyreflexio/server/services/playbook/playbook_aggregator.pyreflexio/server/services/reflection/reflection_service.pyreflexio/server/services/reflection/reflection_service_utils.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/sqlite_storage/_profiles.pyreflexio/server/services/storage/storage_base/_profiles.pytests/server/services/playbook/test_aggregation_lineage_integration.pytests/server/services/reflection/test_reflection_profile_supersede_integration.pytests/server/services/reflection/test_reflection_service.pytests/server/services/storage/test_lineage_b1_harddelete_integration.pytests/server/services/storage/test_lineage_b1_statuschange_integration.pytests/server/services/storage/test_lineage_b1_update_integration.pytests/server/services/storage/test_lineage_contract.py
Comment on lines
306
to
346
| @SQLiteStorageBase.handle_exceptions | ||
| def delete_all_profiles_for_user(self, user_id: str) -> None: | ||
| pids = [ | ||
| r["profile_id"] | ||
| for r in self._fetchall( | ||
| "SELECT profile_id FROM profiles WHERE user_id = ?", (user_id,) | ||
| ) | ||
| ] | ||
| for pid in pids: | ||
| self._fts_delete_profile(pid) | ||
| self._execute("DELETE FROM profiles WHERE user_id = ?", (user_id,)) | ||
| batch_request_id = uuid.uuid4().hex | ||
| with self._lock: | ||
| pids = [ | ||
| r["profile_id"] | ||
| for r in self.conn.execute( | ||
| "SELECT profile_id FROM profiles WHERE user_id = ?", (user_id,) | ||
| ).fetchall() | ||
| ] | ||
| if not pids: | ||
| return | ||
| for pid in pids: | ||
| _emit_hard_delete_profile( | ||
| self.conn, | ||
| org_id=self.org_id, | ||
| entity_id=str(pid), | ||
| request_id=batch_request_id, | ||
| ) | ||
| self._fts_delete_profile(pid) | ||
| self.conn.execute("DELETE FROM profiles WHERE user_id = ?", (user_id,)) | ||
| self.conn.commit() | ||
|
|
||
| @SQLiteStorageBase.handle_exceptions | ||
| def delete_all_profiles(self) -> None: | ||
| batch_request_id = uuid.uuid4().hex | ||
| with self._lock: | ||
| pids = [ | ||
| r["profile_id"] | ||
| for r in self.conn.execute("SELECT profile_id FROM profiles").fetchall() | ||
| ] | ||
| for pid in pids: | ||
| _emit_hard_delete_profile( | ||
| self.conn, | ||
| org_id=self.org_id, | ||
| entity_id=str(pid), | ||
| request_id=batch_request_id, | ||
| ) | ||
| self.conn.execute("DELETE FROM profiles_fts") | ||
| self.conn.execute("DELETE FROM profiles") | ||
| self.conn.commit() |
There was a problem hiding this comment.
Remove profiles_vec rows during profile hard deletes.
These bulk delete paths clean FTS/base rows but leave profile embeddings in profiles_vec. Since hard-delete paths include GDPR/user-erasure flows, the vector sidecar needs to be removed in the same locked transaction as the base delete.
Suggested shape
for pid in pids:
_emit_hard_delete_profile(
self.conn,
org_id=self.org_id,
entity_id=str(pid),
request_id=batch_request_id,
)
- self._fts_delete_profile(pid)
+ rows = self.conn.execute(
+ f"SELECT rowid FROM profiles WHERE profile_id IN ({ph})", pids
+ ).fetchall()
+ rowids = [r["rowid"] for r in rows]
+ self.conn.execute(f"DELETE FROM profiles_fts WHERE profile_id IN ({ph})", pids)
+ if self._has_sqlite_vec and rowids:
+ rowid_ph = ",".join("?" for _ in rowids)
+ self.conn.execute(
+ f"DELETE FROM profiles_vec WHERE rowid IN ({rowid_ph})", rowids
+ )Also applies to: 478-502, 519-540
🤖 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/_profiles.py` around lines
306 - 346, The delete_all_profiles_for_user and delete_all_profiles methods are
removing rows from the profiles and profiles_fts tables but not cleaning up the
corresponding vector embeddings stored in the profiles_vec table. Since these
are hard-delete operations that may include GDPR/user-erasure flows, the vector
sidecar data must be removed in the same locked transaction. Add deletion
statements to remove rows from profiles_vec: in delete_all_profiles_for_user,
delete from profiles_vec for all profile IDs in the pids list, and in
delete_all_profiles, delete all rows from profiles_vec. Execute these deletions
before calling conn.commit() to ensure they are part of the same atomic
transaction as the base deletions.
- emit hard_delete/status_change ONLY for rows actually mutated (rowcount>0); user-scoped existence check in delete_user_profile (no cross-user phantom erasure events) - update_agent_playbook_status: rowcount guard + real old->new reason - archive_profile_by_id: commit unconditionally (no-op txn leak fix) - delete_all_user_playbooks_by_status: enforce PENDING-only carve-out (raise otherwise) - actor=system preserved on delete_*_by_ids; ReflectionServiceRequest.request_id default_factory(uuid) - ruff lint + pyright pragma; phantom-event negative tests added
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@tests/server/services/reflection/test_reflection_profile_supersede_integration.py`:
- Line 258: The call to _set_config at line 258 is incorrectly passing the
configuration as a nested dictionary with the key reflection_config instead of
unpacking the dictionary fields directly. Since _set_config unpacks **kwargs
into ReflectionConfig(**kwargs), the dictionary fields should be passed as
individual keyword arguments. Change the call from _set_config(request_context,
reflection_config={"post_horizon_size": 0}) to _set_config(request_context,
post_horizon_size=0) so that the ReflectionConfig constructor receives the
actual field name it expects rather than an unexpected reflection_config
parameter.
In `@tests/server/services/storage/test_lineage_b1_harddelete_integration.py`:
- Around line 310-320: The tests
test_delete_all_user_playbooks_by_status_archived_raises and
test_delete_all_user_playbooks_by_status_merged_raises expect a StorageError to
be raised, but the delete_all_user_playbooks_by_status method implementation
currently raises ValueError when the status is not Status.PENDING. Update the
implementation to raise StorageError instead of ValueError when the status
validation check fails, ensuring the exception type matches what the tests
expect.
🪄 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: d031f0db-c204-4cda-a588-d6c799fb28f8
📒 Files selected for processing (10)
reflexio/server/services/playbook/playbook_aggregator.pyreflexio/server/services/reflection/reflection_service.pyreflexio/server/services/reflection/reflection_service_utils.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/sqlite_storage/_profiles.pytests/server/services/reflection/test_reflection_profile_supersede_integration.pytests/server/services/reflection/test_reflection_service_utils.pytests/server/services/storage/test_lineage_b1_harddelete_integration.pytests/server/services/storage/test_lineage_b1_statuschange_integration.pytests/server/services/storage/test_lineage_b1_update_integration.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/server/services/storage/test_lineage_b1_update_integration.py
- tests/server/services/storage/test_lineage_b1_statuschange_integration.py
- reflexio/server/services/playbook/playbook_aggregator.py
- reflexio/server/services/reflection/reflection_service.py
- reflexio/server/services/storage/sqlite_storage/_playbook.py
| """ | ||
| # Use post_horizon_size=0 to disable the horizon filter so both passes can | ||
| # proceed regardless of how many follow-up interactions exist. | ||
| _set_config(request_context, reflection_config={"post_horizon_size": 0}) |
There was a problem hiding this comment.
Fix incorrect kwargs passing to _set_config.
The _set_config helper unpacks **kwargs directly into ReflectionConfig(**kwargs) (line 88). Passing reflection_config={"post_horizon_size": 0} will attempt to construct ReflectionConfig(reflection_config=...), which will fail because ReflectionConfig does not have a reflection_config parameter.
The correct call should pass the field directly:
🐛 Proposed fix
- _set_config(request_context, reflection_config={"post_horizon_size": 0})
+ _set_config(request_context, post_horizon_size=0)📝 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
| _set_config(request_context, reflection_config={"post_horizon_size": 0}) | |
| _set_config(request_context, post_horizon_size=0) |
🤖 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/reflection/test_reflection_profile_supersede_integration.py`
at line 258, The call to _set_config at line 258 is incorrectly passing the
configuration as a nested dictionary with the key reflection_config instead of
unpacking the dictionary fields directly. Since _set_config unpacks **kwargs
into ReflectionConfig(**kwargs), the dictionary fields should be passed as
individual keyword arguments. Change the call from _set_config(request_context,
reflection_config={"post_horizon_size": 0}) to _set_config(request_context,
post_horizon_size=0) so that the ReflectionConfig constructor receives the
actual field name it expects rather than an unexpected reflection_config
parameter.
Comment on lines
+310
to
+320
| def test_delete_all_user_playbooks_by_status_archived_raises(tmp_path): | ||
| s = _store(tmp_path) | ||
| with pytest.raises(StorageError, match="PENDING"): | ||
| s.delete_all_user_playbooks_by_status(Status.ARCHIVED) | ||
|
|
||
|
|
||
| def test_delete_all_user_playbooks_by_status_merged_raises(tmp_path): | ||
| s = _store(tmp_path) | ||
| with pytest.raises(StorageError, match="PENDING"): | ||
| s.delete_all_user_playbooks_by_status(Status.MERGED) | ||
|
|
There was a problem hiding this comment.
Tests expect StorageError but implementation raises ValueError.
The implementation of delete_all_user_playbooks_by_status raises ValueError when status is not PENDING:
if status != Status.PENDING:
raise ValueError(
f"delete_all_user_playbooks_by_status only accepts Status.PENDING ..."
)These tests will fail because they expect StorageError.
Proposed fix
def test_delete_all_user_playbooks_by_status_archived_raises(tmp_path):
s = _store(tmp_path)
- with pytest.raises(StorageError, match="PENDING"):
+ with pytest.raises(ValueError, match="PENDING"):
s.delete_all_user_playbooks_by_status(Status.ARCHIVED)
def test_delete_all_user_playbooks_by_status_merged_raises(tmp_path):
s = _store(tmp_path)
- with pytest.raises(StorageError, match="PENDING"):
+ with pytest.raises(ValueError, match="PENDING"):
s.delete_all_user_playbooks_by_status(Status.MERGED)🤖 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/test_lineage_b1_harddelete_integration.py`
around lines 310 - 320, The tests
test_delete_all_user_playbooks_by_status_archived_raises and
test_delete_all_user_playbooks_by_status_merged_raises expect a StorageError to
be raised, but the delete_all_user_playbooks_by_status method implementation
currently raises ValueError when the status is not Status.PENDING. Update the
implementation to raise StorageError instead of ValueError when the status
validation check fails, ensuring the exception type matches what the tests
expect.
…lete-existence filter + vec cleanup + already-archived guard - gate revise/status_change emit on UPDATE rowcount in update_user_playbook/update_agent_playbook/update_user_profile_by_id (completes the rowcount-guard set) - delete_profiles_by_ids: filter to existing rows before auditing (no phantom hard_delete for nonexistent ids) - profiles bulk hard-delete paths now clean profiles_vec sidecar rows (GDPR-correct) - archive_agent_playbooks_by_ids/_by_playbook_name skip already-archived rows (no archived->archived event)
Collaborator
Author
|
Thanks for the thorough pass — addressed in Fixed in
Already addressed in
False alarm:
New negative tests added for each fix (nonexistent-id-no-event, partial-existence, vec-cleanup, already-archived-no-event). Suite: 214 passed. |
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)
reflexio/server/services/storage/sqlite_storage/_profiles.py (1)
333-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing vector sidecar cleanup in
delete_all_profiles.This method deletes from
profiles_ftsmanually but does not clean upprofiles_vec. Other bulk delete methods use_delete_profile_search_rows(pids)which handles both FTS and vec deletion. For GDPR/hard-delete compliance, embeddings should be removed alongside base data.Suggested fix
`@SQLiteStorageBase.handle_exceptions` def delete_all_profiles(self) -> None: batch_request_id = uuid.uuid4().hex with self._lock: pids = [ r["profile_id"] for r in self.conn.execute("SELECT profile_id FROM profiles").fetchall() ] for pid in pids: _emit_hard_delete_profile( self.conn, org_id=self.org_id, entity_id=str(pid), request_id=batch_request_id, ) - self.conn.execute("DELETE FROM profiles_fts") + self._delete_profile_search_rows(pids) self.conn.execute("DELETE FROM profiles") self.conn.commit()🤖 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/_profiles.py` around lines 333 - 350, The delete_all_profiles method manually deletes from profiles_fts and profiles but does not clean up the profiles_vec sidecar table. Replace the manual DELETE statements for profiles_fts and profiles with a call to _delete_profile_search_rows(pids) passing the list of profile IDs that was already collected, which properly handles cleanup of both FTS and vector embeddings to ensure GDPR/hard-delete compliance.reflexio/server/services/storage/sqlite_storage/_playbook.py (1)
487-500:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSplit transaction between search cleanup and base row deletion.
The search row cleanup happens inside
_lockwith a commit at line 497, but the base row deletion at line 499 uses_executewhich acquires its own lock and commits separately. This creates a window where search rows are deleted but base rows still exist, which could cause inconsistent state if the second operation fails.Consider moving the base row DELETE inside the same locked transaction for atomicity.
Suggested fix
if ids: with self._lock: self._delete_playbook_search_rows("user", ids) - self.conn.commit() - - cur = self._execute(f"DELETE FROM user_playbooks WHERE {where}", params) + cur = self.conn.execute(f"DELETE FROM user_playbooks WHERE {where}", params) + self.conn.commit() + else: + cur = self._execute(f"DELETE FROM user_playbooks WHERE {where}", params) return cur.rowcount🤖 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.py` around lines 487 - 500, The code currently splits the deletion operation into two separate transactions: the search row cleanup via _delete_playbook_search_rows happens inside the _lock block with a commit, but the base row deletion via _execute happens afterwards outside the lock with its own transaction and commit. This creates a window where inconsistent state can occur if the second deletion fails. Move the _execute call for deleting from user_playbooks to inside the same with self._lock block, before the self.conn.commit() call at line 497, so both deletions happen atomically within a single transaction.
🧹 Nitpick comments (1)
tests/server/services/storage/test_lineage_b1_harddelete_integration.py (1)
375-380: ⚡ Quick winScope profile hard-delete assertions to
entity_type="profile".These assertions currently filter only by
entity_id; addingentity_typeavoids false positives/negatives if another entity type reuses the same id in future test setup.Proposed patch
- assert not any( - e.op == "hard_delete" for e in s.get_lineage_events(entity_id="ghost-1") - ) + assert not any( + e.op == "hard_delete" + for e in s.get_lineage_events(entity_id="ghost-1", entity_type="profile") + ) - assert not any( - e.op == "hard_delete" for e in s.get_lineage_events(entity_id="ghost-2") - ) + assert not any( + e.op == "hard_delete" + for e in s.get_lineage_events(entity_id="ghost-2", entity_type="profile") + ) real_events = [ - e for e in s.get_lineage_events(entity_id="real-1") if e.op == "hard_delete" + e + for e in s.get_lineage_events(entity_id="real-1", entity_type="profile") + if e.op == "hard_delete" ] assert len(real_events) == 1 assert not any( - e.op == "hard_delete" for e in s.get_lineage_events(entity_id="ghost-3") + e.op == "hard_delete" + for e in s.get_lineage_events(entity_id="ghost-3", entity_type="profile") )Also applies to: 389-395
🤖 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/test_lineage_b1_harddelete_integration.py` around lines 375 - 380, The assertions checking for hard_delete operations in the lineage events are currently filtering only by entity_id parameter in the get_lineage_events() calls. To make these assertions more specific and prevent false positives/negatives if another entity type reuses the same id in future test changes, add entity_type="profile" parameter to both get_lineage_events() method calls in the affected assertions (the ones checking entity_id="ghost-1" and entity_id="ghost-2" at lines 375-380, and also the similar assertions mentioned at lines 389-395).
🤖 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/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 487-500: The code currently splits the deletion operation into two
separate transactions: the search row cleanup via _delete_playbook_search_rows
happens inside the _lock block with a commit, but the base row deletion via
_execute happens afterwards outside the lock with its own transaction and
commit. This creates a window where inconsistent state can occur if the second
deletion fails. Move the _execute call for deleting from user_playbooks to
inside the same with self._lock block, before the self.conn.commit() call at
line 497, so both deletions happen atomically within a single transaction.
In `@reflexio/server/services/storage/sqlite_storage/_profiles.py`:
- Around line 333-350: The delete_all_profiles method manually deletes from
profiles_fts and profiles but does not clean up the profiles_vec sidecar table.
Replace the manual DELETE statements for profiles_fts and profiles with a call
to _delete_profile_search_rows(pids) passing the list of profile IDs that was
already collected, which properly handles cleanup of both FTS and vector
embeddings to ensure GDPR/hard-delete compliance.
---
Nitpick comments:
In `@tests/server/services/storage/test_lineage_b1_harddelete_integration.py`:
- Around line 375-380: The assertions checking for hard_delete operations in the
lineage events are currently filtering only by entity_id parameter in the
get_lineage_events() calls. To make these assertions more specific and prevent
false positives/negatives if another entity type reuses the same id in future
test changes, add entity_type="profile" parameter to both get_lineage_events()
method calls in the affected assertions (the ones checking entity_id="ghost-1"
and entity_id="ghost-2" at lines 375-380, and also the similar assertions
mentioned at lines 389-395).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a555597-f65c-415b-bd73-747cb8890b0d
📒 Files selected for processing (4)
reflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/sqlite_storage/_profiles.pytests/server/services/storage/test_lineage_b1_harddelete_integration.pytests/server/services/storage/test_lineage_b1_update_integration.py
…only after the mutation, guarded on rowcount The shared sqlite3 connection is one transaction; _fts_*/_vec_* helpers self-commit, so an 'emit event -> fts/vec cleanup -> mutate -> commit' ordering flushes the audit event before the mutation. Emit after (or in the same commit as) the mutation, guarded on cur.rowcount>0; existence check must use the same predicate (incl. user_id) as the mutation.
…tatus_change events (SQLite)
…te_* status_change path - update_agent_playbook: when playbook_status is among the updated fields, read prior playbook_status inside the existing lock block (before the UPDATE) and pass from_status=<prior>, to_status=<new>, status_namespace='playbook_status' to _append_event_stmt. Metadata-only updates (no playbook_status, no content) leave all three fields NULL. Moves the not-found guard inside the lock so the SELECT and UPDATE are atomic. - update_user_playbook: adds a status: Status | None = None parameter (the lifecycle status column). When status is set, reads prior status inside the lock and passes from_status=<prior>, to_status=<new>, status_namespace='lifecycle_status'. Metadata-only updates leave all three fields NULL. Same atomic not-found guard as above. - Both methods: op='revise' (content set) branch leaves all structured fields NULL, unchanged from prior behaviour. Tests added to test_lineage_b1_update_integration.py: - update_agent_playbook(playbook_status=X): to/from/ns populated (pending->approved) - update_agent_playbook(playbook_name=...): all 3 fields NULL - update_user_playbook(status=ARCHIVED) with NULL prior: from_status=None, to=archived, ns=lifecycle_status - update_user_playbook(status=ARCHIVED) with PENDING prior: from_status=pending - update_user_playbook(playbook_name=...): all 3 fields NULL
… (keep structured fields NULL where no status flips)
There was a problem hiding this comment.
Actionable comments posted: 1
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/storage/sqlite_storage/_playbook.py (3)
492-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
hard_deleteand delete the audited IDs atomically.
delete_all_user_playbooks_by_status()deletes base rows without anyhard_deleteevent, and it commits search-row cleanup before the physical delete. That leaves this hard-delete path outside the lineage contract and can desynchronize search/base rows on failure. Select IDs under the lock, emit per-ID lineage, clean search rows, and delete those same IDs in one transaction.Proposed fix
- # Clean up FTS + vec - ids = [ - r["user_playbook_id"] - for r in self._fetchall( - f"SELECT user_playbook_id FROM user_playbooks WHERE {where}", params - ) - ] - if ids: - with self._lock: - self._delete_playbook_search_rows("user", ids) - self.conn.commit() - - cur = self._execute(f"DELETE FROM user_playbooks WHERE {where}", params) - return cur.rowcount + batch_request_id = uuid.uuid4().hex + with self._lock: + ids = [ + r["user_playbook_id"] + for r in self.conn.execute( + f"SELECT user_playbook_id FROM user_playbooks WHERE {where}", + params, + ).fetchall() + ] + if not ids: + self.conn.commit() + return 0 + + for upid in ids: + _emit_hard_delete_playbook( + self.conn, + org_id=self.org_id, + entity_type="user_playbook", + entity_id=str(upid), + request_id=batch_request_id, + ) + + self._delete_playbook_search_rows("user", ids) + ph = ",".join("?" for _ in ids) + cur = self.conn.execute( + f"DELETE FROM user_playbooks WHERE user_playbook_id IN ({ph})", + ids, + ) + self.conn.commit() + return cur.rowcount🤖 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.py` around lines 492 - 505, The delete_all_user_playbooks_by_status method performs search row cleanup and commits before deleting base rows, breaking atomicity and missing hard_delete lineage events. Under the _lock, select the IDs and emit hard_delete events for each ID to maintain lineage tracking, then call _delete_playbook_search_rows to clean up search rows, execute the DELETE FROM user_playbooks query with those same IDs, and finally commit once at the end so all operations (ID selection, lineage emission, search cleanup, and physical delete) happen atomically.
1014-1055:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve status lineage when content and status change together.
When
contentandplaybook_statusare both provided, the event is classified asrevise, so the structured status fields stayNULLand nostatus_changeevent records theplaybook_statustransition. Emit an additionalstatus_changeevent when the status actually changes; the idempotency key already includesop, so it can share the same request ID.Suggested shape
if cur.rowcount > 0: + event_request_id = uuid.uuid4().hex # 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 @@ actor="api", - request_id=uuid.uuid4().hex, + request_id=event_request_id, reason="in-place update", from_status=from_status, to_status=to_status, status_namespace=status_namespace, ) + if ( + op == "revise" + and playbook_status is not None + and prior_playbook_status != playbook_status.value + ): + _append_event_stmt( + self.conn, + org_id=self.org_id, + entity_type="agent_playbook", + entity_id=str(agent_playbook_id), + op="status_change", + prov="wasInvalidatedBy", + source_ids=[], + actor="api", + request_id=event_request_id, + reason=f"{prior_playbook_status or 'None'}->{playbook_status.value}", + from_status=prior_playbook_status, + to_status=playbook_status.value, + status_namespace="playbook_status", + )🤖 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.py` around lines 1014 - 1055, The issue is that when both content and playbook_status are updated together, the operation is classified as "revise" rather than "status_change", causing the status transition information to not be recorded. After the existing _append_event_stmt call for the "revise" operation, add an additional _append_event_stmt call to emit a "status_change" event whenever the playbook_status actually changes and is not None. This second event should use op="status_change", prov="wasInvalidatedBy", set from_status to prior_playbook_status, set to_status to playbook_status.value, set status_namespace to "playbook_status", and reuse the same request_id from the first event since the op field is part of the idempotency key.
333-347:⚠️ Potential issue | 🟠 MajorSwitch single-row deletes to use transaction-scoped cleanup helper to prevent orphaned sidecars.
_fts_deleteand_vec_deleteboth callself.conn.commit()internally. At lines 333–347 and 861–875, these are called before the base table delete and lineage emission; if either of those later operations fail, the FTS and vector sidecars remain committed while the main delete is rolled back, leaving orphaned search rows.Switch to
_delete_playbook_search_rows("user", [user_playbook_id])and_delete_playbook_search_rows("agent", [agent_playbook_id])to keep all sidecar cleanup within the shared transaction scope.🤖 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.py` around lines 333 - 347, The issue is that _fts_delete and _vec_delete methods are called individually at the beginning of the deletion block and each internally commits their changes, which can leave orphaned FTS and vector sidecar rows if the subsequent base table delete or lineage emission fails. Replace the two separate calls to _fts_delete("user_playbooks_fts", user_playbook_id) and _vec_delete("user_playbooks_vec", user_playbook_id) with a single call to _delete_playbook_search_rows("user", [user_playbook_id]) to ensure all cleanup operations remain within the same transaction scope as the main DELETE operation on user_playbooks and the _emit_hard_delete_playbook call.
♻️ Duplicate comments (1)
reflexio/server/services/storage/sqlite_storage/_playbook.py (1)
389-405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter to existing IDs before emitting
hard_delete.These by-id bulk deletes still append
hard_deletefor every caller-supplied ID, but the finalDELETE ... IN (...)may affect fewer rows. A stale ID therefore records a deletion that never happened. Snapshot existing IDs inside the lock and use that same set for event emission, search cleanup, and base deletion.Suggested shape
batch_request_id = uuid.uuid4().hex with self._lock: + existing_ids = [ + r["user_playbook_id"] + for r in self.conn.execute( + f"SELECT user_playbook_id FROM user_playbooks WHERE user_playbook_id IN ({ph})", + user_playbook_ids, + ).fetchall() + ] + if not existing_ids: + self.conn.commit() + return 0 + ph = ",".join("?" for _ in existing_ids) if emit_hard_delete: - for upid in user_playbook_ids: + for upid in existing_ids: _emit_hard_delete_playbook( self.conn, org_id=self.org_id, entity_type="user_playbook", entity_id=str(upid), request_id=batch_request_id, actor="system", ) - self._delete_playbook_search_rows("user", user_playbook_ids) + self._delete_playbook_search_rows("user", existing_ids) cur = self.conn.execute( f"DELETE FROM user_playbooks WHERE user_playbook_id IN ({ph})", - user_playbook_ids, + existing_ids, )Apply the same existing-ID snapshot pattern to
delete_agent_playbooks_by_ids.Also applies to: 918-933
🤖 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.py` around lines 389 - 405, In the delete logic after the emit_hard_delete check, the code is emitting hard_delete events and performing deletes on all caller-supplied user_playbook_ids without first verifying they exist in the database. Before the hard_delete loop, query the database to retrieve only the user_playbook_ids that actually exist, then use this filtered set for both the hard_delete emission loop and the subsequent DELETE operation. Apply the same existing-ID snapshot pattern to the delete_agent_playbooks_by_ids method as well.
🧹 Nitpick comments (2)
reflexio/server/services/storage/sqlite_storage/_base.py (1)
1249-1262: ⚡ Quick winKeep lineage_event CREATE definitions aligned to reduce migration drift.
Line 1249–1262 omits
from_status,to_status, andstatus_namespaceeven though_DDLincludes them. It works because of the later ALTER loop, but duplicating two different CREATE definitions increases maintenance risk.Suggested diff
CREATE TABLE IF NOT EXISTS lineage_event ( event_id INTEGER PRIMARY KEY AUTOINCREMENT, org_id TEXT NOT NULL, entity_type TEXT NOT NULL, entity_id TEXT NOT NULL, op TEXT NOT NULL, prov_relation TEXT NOT NULL DEFAULT '', source_ids TEXT NOT NULL DEFAULT '[]', actor TEXT NOT NULL DEFAULT '', request_id TEXT NOT NULL DEFAULT '', reason TEXT NOT NULL DEFAULT '', created_at INTEGER NOT NULL, + from_status TEXT, + to_status TEXT, + status_namespace TEXT, UNIQUE (org_id, entity_type, entity_id, op, request_id) );🤖 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/_base.py` around lines 1249 - 1262, The CREATE TABLE IF NOT EXISTS lineage_event statement (lines 1249-1262) is missing three columns that exist in the _DDL definition: from_status, to_status, and status_namespace. Add these three missing columns to the lineage_event table creation to align with the _DDL definition and eliminate the maintenance risk of having duplicate and divergent table definitions. This will ensure the schema is defined completely in one place rather than relying on a later ALTER loop to add the missing columns.tests/server/services/storage/test_lineage_b1_statuschange_integration.py (1)
465-467: ⚡ Quick winFilter lineage queries by
entity_typeto avoid future ID-collision flakes.These lookups currently filter only by
entity_id(e.g., Line 465). Since IDs can overlap across entity tables, includeentity_typeinget_lineage_events(...)for deterministic scoping.Example adjustment
- evts = [ - e for e in s.get_lineage_events(entity_id=str(apid)) if e.op == "status_change" - ] + evts = [ + e + for e in s.get_lineage_events( + entity_type="agent_playbook", + entity_id=str(apid), + ) + if e.op == "status_change" + ]Also applies to: 486-488, 502-504, 523-526, 540-543, 557-559, 572-574, 589-591
🤖 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/test_lineage_b1_statuschange_integration.py` around lines 465 - 467, The get_lineage_events() calls are filtering only by entity_id parameter, which can cause ID-collision issues since IDs can overlap across different entity tables. Update all instances where get_lineage_events() is called with only entity_id (at lines 465-467, 486-488, 502-504, 523-526, 540-543, 557-559, 572-574, and 589-591) to include the entity_type parameter as well. This ensures deterministic scoping by specifying both the entity_id and its corresponding entity_type, preventing future test flakiness from ID collisions.
🤖 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 `@tests/server/services/storage/test_lineage_b1_update_integration.py`:
- Around line 223-224: The get_lineage_events call in the test is filtering only
by entity_id without specifying entity_type, which can cause test flakiness if
other entity types share the same ID. Modify the get_lineage_events method call
to include the entity_type parameter set to "user_playbook" alongside the
existing entity_id parameter to ensure deterministic test behavior and prevent
cross-entity ID collisions.
---
Outside diff comments:
In `@reflexio/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 492-505: The delete_all_user_playbooks_by_status method performs
search row cleanup and commits before deleting base rows, breaking atomicity and
missing hard_delete lineage events. Under the _lock, select the IDs and emit
hard_delete events for each ID to maintain lineage tracking, then call
_delete_playbook_search_rows to clean up search rows, execute the DELETE FROM
user_playbooks query with those same IDs, and finally commit once at the end so
all operations (ID selection, lineage emission, search cleanup, and physical
delete) happen atomically.
- Around line 1014-1055: The issue is that when both content and playbook_status
are updated together, the operation is classified as "revise" rather than
"status_change", causing the status transition information to not be recorded.
After the existing _append_event_stmt call for the "revise" operation, add an
additional _append_event_stmt call to emit a "status_change" event whenever the
playbook_status actually changes and is not None. This second event should use
op="status_change", prov="wasInvalidatedBy", set from_status to
prior_playbook_status, set to_status to playbook_status.value, set
status_namespace to "playbook_status", and reuse the same request_id from the
first event since the op field is part of the idempotency key.
- Around line 333-347: The issue is that _fts_delete and _vec_delete methods are
called individually at the beginning of the deletion block and each internally
commits their changes, which can leave orphaned FTS and vector sidecar rows if
the subsequent base table delete or lineage emission fails. Replace the two
separate calls to _fts_delete("user_playbooks_fts", user_playbook_id) and
_vec_delete("user_playbooks_vec", user_playbook_id) with a single call to
_delete_playbook_search_rows("user", [user_playbook_id]) to ensure all cleanup
operations remain within the same transaction scope as the main DELETE operation
on user_playbooks and the _emit_hard_delete_playbook call.
---
Duplicate comments:
In `@reflexio/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 389-405: In the delete logic after the emit_hard_delete check, the
code is emitting hard_delete events and performing deletes on all
caller-supplied user_playbook_ids without first verifying they exist in the
database. Before the hard_delete loop, query the database to retrieve only the
user_playbook_ids that actually exist, then use this filtered set for both the
hard_delete emission loop and the subsequent DELETE operation. Apply the same
existing-ID snapshot pattern to the delete_agent_playbooks_by_ids method as
well.
---
Nitpick comments:
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1249-1262: The CREATE TABLE IF NOT EXISTS lineage_event statement
(lines 1249-1262) is missing three columns that exist in the _DDL definition:
from_status, to_status, and status_namespace. Add these three missing columns to
the lineage_event table creation to align with the _DDL definition and eliminate
the maintenance risk of having duplicate and divergent table definitions. This
will ensure the schema is defined completely in one place rather than relying on
a later ALTER loop to add the missing columns.
In `@tests/server/services/storage/test_lineage_b1_statuschange_integration.py`:
- Around line 465-467: The get_lineage_events() calls are filtering only by
entity_id parameter, which can cause ID-collision issues since IDs can overlap
across different entity tables. Update all instances where get_lineage_events()
is called with only entity_id (at lines 465-467, 486-488, 502-504, 523-526,
540-543, 557-559, 572-574, and 589-591) to include the entity_type parameter as
well. This ensures deterministic scoping by specifying both the entity_id and
its corresponding entity_type, preventing future test flakiness from ID
collisions.
🪄 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: 5de3cd0c-55d4-428c-898e-5160e5e02b81
📒 Files selected for processing (8)
reflexio/models/api_schema/domain/entities.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_lineage.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/sqlite_storage/_profiles.pytests/server/services/storage/test_lineage_b1_statuschange_integration.pytests/server/services/storage/test_lineage_b1_update_integration.pytests/server/services/storage/test_lineage_contract.py
🚧 Files skipped from review as they are similar to previous changes (1)
- reflexio/server/services/storage/sqlite_storage/_profiles.py
Comment on lines
+223
to
+224
| ev = s.get_lineage_events(entity_id=str(pb.user_playbook_id)) | ||
| sc = [e for e in ev if e.op == "status_change"] |
There was a problem hiding this comment.
Add entity_type filter to avoid cross-entity ID collisions in lineage lookup.
On Line 223, this test filters only by entity_id, unlike the adjacent agent-playbook tests. If another entity type reuses the same ID, the status_change selection on Line 224 can become flaky. Use entity_type="user_playbook" here for deterministic assertions.
Suggested patch
- ev = s.get_lineage_events(entity_id=str(pb.user_playbook_id))
+ ev = s.get_lineage_events(
+ entity_id=str(pb.user_playbook_id),
+ entity_type="user_playbook",
+ )📝 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
| ev = s.get_lineage_events(entity_id=str(pb.user_playbook_id)) | |
| sc = [e for e in ev if e.op == "status_change"] | |
| ev = s.get_lineage_events( | |
| entity_id=str(pb.user_playbook_id), | |
| entity_type="user_playbook", | |
| ) | |
| sc = [e for e in ev if e.op == "status_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 `@tests/server/services/storage/test_lineage_b1_update_integration.py` around
lines 223 - 224, The get_lineage_events call in the test is filtering only by
entity_id without specifying entity_type, which can cause test flakiness if
other entity types share the same ID. Modify the get_lineage_events method call
to include the entity_type parameter set to "user_playbook" alongside the
existing entity_id parameter to ensure deterministic test behavior and prevent
cross-entity ID collisions.
Collaborator
Author
|
CodeRabbit findings addressed in follow-up PR #193 (commits |
yilu331
added a commit
that referenced
this pull request
Jun 21, 2026
/#191) (#193) ## What Addresses the CodeRabbit review findings left on the **merged** lineage PRs **#187 (B1)**, **#188 (B2)**, and **#191 (B3-pre)**. Pure remediation — no new features. ## Fixes by source PR **#191 (B3-pre)** — `22d2540`: negative-`limit` guard (`<=0`) in reconstruction; `enabled_org_ids or []` defensiveness in feature flags; parity script **fail-closed on duplicate `request_id`** + **INCONCLUSIVE (exit 2) on at-cap/truncated reads**; test hygiene (vacuous-MATCH guard, RECON-vs-LEGACY label, exact request_id set-equality, empty-id seeding); ASCII `union` (RUF002). **#188 (B2)** — `4204598`: `LineageGCConfig` bounds (`Field(gt=0)`); scheduler poll-interval clamp; `gc_expired_tombstones` `limit<=0` guard; **explicit rollback** to keep GC atomic on mid-write failure. **#187 (B1)** — `a5f6558` + `f792ddb`: **phantom-audit guards on the bulk-delete paths** (emit `hard_delete` only for rows that exist, in the **same commit** as the base DELETE; FTS/vec cleanup moved **after** the commit per the SQLite self-commit rule) — fixing `delete_all_agent_playbooks` and `delete_archived_agent_playbooks_by_playbook_name`, which emitted *before* the mutation; `entity_type` filter in a lineage-lookup test. ## Deliberately NOT changed (with reasons) - **SQLite `get_profiles_by_generated_from_request_id` "not org-scoped"** — SQLite `profiles` has **no `org_id` column** (tenant isolation is per-DB-file; enterprise is per-schema), and the reconstruction's event pool is already `org_id`-scoped via `get_lineage_events`. Adding a profiles-level org filter needs a schema migration — tracked as a separate follow-up, not a quick fix. - **`_set_config(**kwargs)` `🔴 Critical`** — **confirmed false alarm**: `Config.model_validate({..., **overrides})` is valid Pydantic; the test passes and applies the right config. - Several #187 items were **already addressed** by later phases (rowcount gates, already-archived exclusion, atomicity, existing-row filters, vec-sidecar cleanup, `request_id` non-optional, `StorageError` wrapping) — verified against current code and skipped. All changes tested (reviewed via a final whole-branch pass that caught the bulk-delete ordering). No behavior change beyond the hardening above. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added validation for scheduler configuration values to ensure they meet minimum requirements. * **Bug Fixes** * Improved transaction atomicity during garbage collection operations. * Fixed feature flag evaluation to safely handle missing or null org ID lists. * Enhanced duplicate detection in data parity validation. * Enforced minimum scheduler poll interval to improve reliability. * **Tests** * Added comprehensive tests for configuration validation and garbage collection scenarios. * Strengthened parity checking and deletion operation test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
yilu331
added a commit
that referenced
this pull request
Jun 21, 2026
…NG (unblock playbook upgrade on SQLite) (#194) ## Summary `/test-backend-pipeline` run found a **P1 regression** in the playbook upgrade flow on the SQLite backend. Lineage Phase B1 (#187) added a `status != Status.PENDING` → `raise ValueError` guard to the SQLite `delete_all_user_playbooks_by_status`. But the shared upgrade flow (`base_generation_service._perform_upgrade`) calls `_delete_items_by_status(Status.ARCHIVED, request)` to purge old archived playbooks. As a result, `upgrade_all_user_playbooks` broke on SQLite — it returned `UpgradeUserPlaybooksResponse(success=False, ...)` with the guard error swallowed by `handle_exceptions`. The **Supabase backend has no such guard**: it routes the same delete through `_hard_delete_and_log(..., emit_hard_delete=False)` and accepts any status. So this was a SQLite-only regression and a cross-backend divergence that the storage contract suite does not cover (`delete_all_user_playbooks_by_status` has no contract test). ## Root cause | Backend | `delete_all_user_playbooks_by_status(ARCHIVED)` | Lineage | |---|---|---| | SQLite (pre-fix) | **raises ValueError** (B1 guard) | n/a | | SQLite (post-fix) | deletes rows | none (matches Supabase) | | Supabase | deletes rows | none (`emit_hard_delete=False`) | | `delete_all_profiles_by_status` (both backends) | deletes rows | emits `hard_delete` | ## Fix Remove the PENDING-only guard so SQLite matches the Supabase sibling: bulk delete-by-status accepts any status and (by design, matching Supabase) emits no `hard_delete` lineage events. The `PENDING` purge already emitted no events, so behavior for the original carve-out is unchanged. Updated the two B1 integration tests that asserted the raise (`..._archived_raises`, `..._merged_raises`) into one parametrized test asserting the corrected behavior: the row is physically deleted and **no** `hard_delete` event is emitted. ## Verification - `tests/server/services/storage/test_lineage_b1_harddelete_integration.py` — **34 passed** - `tests/e2e_tests/test_playbook_workflows.py::test_upgrade_user_playbooks_end_to_end` — **passed** (was failing) - `tests/e2e_tests/test_playbook_workflows.py` (full file) — **8 passed, 16 skipped** (was 1 failed) - `ruff check` / `ruff format --check` / `pyright` — clean ## Follow-up (not in this PR) The storage contract suite does not exercise `delete_all_user_playbooks_by_status`, which is why SQLite and Supabase diverged silently. A contract test covering this method (PENDING + a non-PENDING status, asserting deletion and the no-lineage carve-out) would prevent recurrence.
This was referenced Jun 21, 2026
This was referenced Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lineage Phase B1 (OSS / SQLite) — completes the content-free lineage instrumentation begun in Phase A so
lineage_eventbecomes the complete, single audit source for every state-changing write to playbooks/profiles. Each mutation emits its event atomically (onewith self._lock:+ one commit), keyed on the 5-column idempotency key(org_id, entity_type, entity_id, op, request_id).Changes
update_*(update_user_playbook,update_agent_playbook,update_agent_playbook_status,update_user_profile_by_id) → emitop=revise(content changed) orop=status_change, per-call UUIDrequest_id.hard_deletecompleteness —delete_profiles_by_ids(+emit_hard_deleteparam), single-row deletes, and the bulk / GDPR org-wipe paths (delete_all_*,delete_all_profiles_for_user,delete_*_by_playbook_name) now emit a per-idhard_deletebefore the physical delete (SELECT-inside-lock for atomicity). PENDING-rerun purge stays a no-event carve-out.status_changeon archive + bulk status-flip paths, with accurate per-rowreason({prior}->archived).supersede_record(old profile → SUPERSEDED tombstone +superseded_by+reviseevent; lost-CAS rollback usesemit_hard_delete=False). Closes a Phase A gap.op=aggregateset-level events on user→agent aggregation (W3C PROVwasDerivedFrom, M:N), best-effort (capture_anomaly, never aborts aggregation).resolve_currentemits a Sentrycapture_anomalyon cycle / max-hops (previously a silentNone).entity_type(prevents cross-entity-type collisions) andorg_id.Test Plan
capture_anomalyfires).request_idpinned end-to-end on thereviseevent.Built via subagent-driven-development (per-task TDD + review + fix loop); final whole-branch review passed.
Summary by CodeRabbit