Lineage Phase B3-pre — dedup write-side instrumentation + reconstruction (flag-OFF, non-destructive) - #191
Merged
Merged
Conversation
…geLog reconstruction (B3 Task 2)
…struction (dual-write kept) — B3 Task 3
…enant read) + isolation test (B3 Task 3 review)
…ty-request_id invariant (B3 Task 4)
…truction not yet reproducible from production events — write-side fix needed first)
…hind flag (B3-pre T1)
…table column+event model + adapt parity (B3-pre T2)
… NULL+PENDING docstring (B3-pre final-review)
📝 WalkthroughWalkthroughIntroduces feature-flagged soft-delete for dedup profile supersession: adds ChangesDedup Soft-Delete + Set-Based Lineage
Sequence Diagram(s)sequenceDiagram
participant PGS as ProfileGenerationService
participant FF as is_dedup_soft_delete_enabled
participant Storage as SQLiteStorage
participant Lineage as lineage_events table
PGS->>FF: check flag(org_id)
alt flag ON and request_id non-empty
PGS->>Storage: supersede_profiles_by_ids(user_id, profile_ids, request_id)
loop per eligible profile
Storage->>Storage: UPDATE status=SUPERSEDED
Storage->>Lineage: INSERT status_change event (request_id, to_status=superseded)
end
Storage-->>PGS: count updated
else flag OFF or request_id empty
loop per profile_id
PGS->>Storage: delete_user_profile(profile_id)
end
end
note over PGS,Lineage: reconstruct_profile_change_log reads Lineage to rebuild ProfileChangeLogResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 11
🧹 Nitpick comments (1)
tests/server/services/storage/test_lineage_b3_reconstruct_changelog_integration.py (1)
378-400: ⚡ Quick winTest name/docstring contradict actual assertion.
test_adds_only_run_produces_rowassertsprofile_change_logs == []. Renaming avoids semantic confusion.Suggested rename
-def test_adds_only_run_produces_row(tmp_path): - """A run with adds but no removals still produces a change-log row.""" +def test_adds_only_run_without_lineage_events_returns_empty(tmp_path): + """Adds-only without lineage events reconstructs to no rows."""🤖 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_b3_reconstruct_changelog_integration.py` around lines 378 - 400, The test function `test_adds_only_run_produces_row` has a name that contradicts its actual assertion. The test name implies it should produce a row, but the assertion at the end verifies that `profile_change_logs == []`, meaning no rows are produced. Rename the test function to accurately reflect that it produces no rows when there are only adds without any lineage events (e.g., something like `test_adds_only_run_produces_no_row` or `test_adds_only_without_lineage_events_produces_empty_log`).
🤖 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/lib/_profiles.py`:
- Around line 598-599: The code currently only checks for limit == 0 but does
not validate negative limit values, which causes them to fall through to
Python's slicing behavior and return unintended results. Add explicit validation
to check for limit < 0 in addition to the existing limit == 0 check, and return
a ProfileChangeLogResponse with success=True and an empty profile_change_logs
list for negative values as well, treating negative limits the same as zero.
Apply this fix at all occurrences where this validation is needed, including the
additional locations referenced in the comment.
- Line 578: The docstring in the _profiles.py file contains a Unicode union
symbol `∪` which triggers a Ruff lint warning (RUF002). Replace the `∪`
character in the phrase `added ∪ removed` with a plain ASCII equivalent such as
the word `union` or the pipe character `|` to eliminate the lint warning and
avoid font/editor ambiguity issues.
In `@reflexio/server/services/storage/sqlite_storage/_profiles.py`:
- Around line 476-478: The `get_profiles_by_generated_from_request_id` method's
SELECT query in the `_fetchall` call is missing organization scoping in its
WHERE clause. Currently it only filters by generated_from_request_id, which can
return profiles from other organizations in a shared database if request IDs
collide across tenants. Add an additional condition to the WHERE clause that
filters by organization ID alongside the generated_from_request_id parameter to
ensure only the current organization's profiles are returned and prevent
cross-tenant data leakage.
In `@reflexio/server/site_var/feature_flags.py`:
- Around line 139-140: The feature_config.get method on the line retrieving
enabled_org_ids will return None if the key exists but its value is null, which
causes a TypeError when attempting the membership check with the in operator on
the next line. Modify the retrieval of enabled_org_ids to ensure it defaults to
an empty list not only when the key is missing but also when the key exists with
a None value, such as by using the or operator to provide a fallback empty list
if the retrieved value is None or falsy.
In `@scripts/lineage_b3_parity_check.py`:
- Around line 94-102: Instead of silently overwriting duplicate request_ids with
last-write-wins behavior in both the legacy_by_req and recon_by_req
dictionaries, add validation to fail closed when duplicates are detected. In the
loops iterating over legacy_rows and recon_rows, check if the request_id already
exists in the respective dictionary before assignment. If a duplicate is found,
raise an error or record it as a parity gap to ensure duplicate rows are
explicitly handled and do not hide potential parity mismatches.
- Around line 155-157: The parity check in this function reads data with a
hardcoded limit of 10,000 rows using get_profile_change_logs and
reconstruct_profile_change_log, but does not verify whether the returned
datasets were truncated. For larger organizations, this can result in incomplete
comparisons that miss older RECON-MISSING gaps while still passing the parity
check. Add explicit truncation detection by checking if the number of rows
returned equals the limit, and fail the function explicitly when truncation is
detected, either by raising an error or by implementing exhaustive pagination to
retrieve all available data before performing the classify_parity comparison.
In
`@tests/server/services/storage/test_lineage_b3_parity_classifier_integration.py`:
- Around line 186-189: The assertion using all() on match_results can vacuously
pass if the filtered list is empty, which would hide missing expected data.
Before the existing assertion that checks all classifications are MATCH, add an
assertion to verify that match_results has the expected cardinality (is not
empty or has the expected number of entries). This ensures the test fails if the
filtering operation for items with request_id starting with "req-full-" returns
no rows, rather than silently passing.
In `@tests/server/services/storage/test_lineage_b3_parity_gate_integration.py`:
- Around line 12-14: Rename the case description from "LEGACY-MISSING" to
"RECON-MISSING" in the lineage classification documentation. The case describing
"a legacy row has no corresponding lineage event" should use the RECON-MISSING
terminology instead of LEGACY-MISSING to align with the PR's classifier
direction definitions. Update all occurrences of this mislabeled case throughout
the test file, specifically in the case descriptions and comments that explain
the reconstruction behavior when a legacy row has no lineage event.
- Around line 195-203: The assertion in the loop checking legacy_by_req items
exist in recon_by_req only validates one direction of the comparison. Add a
bidirectional check to ensure both dictionaries contain exactly the same set of
request_ids. After the existing loop that validates each legacy request_id
exists in recon_by_req, add an assertion that checks the reverse direction or
that the keys of both legacy_by_req and recon_by_req are equal, so that any
extra reconstructed rows with unexpected request_ids will be caught and fail the
parity gate test.
In
`@tests/server/services/storage/test_lineage_b3_reconstruct_changelog_integration.py`:
- Around line 503-543: The test_cross_org_isolation function uses different
request_ids for each organization (r-a-run for org A and r-b-run for org B),
which doesn't test the case where both organizations might use the same
request_id. To properly test isolation, modify the test to have both
organizations use the same request_id value (e.g., use "r-shared" for both the
_seed_dedup_run calls in s_a and s_b), then verify that each organization's
result still only contains its own data and not the other organization's data.
This will catch if the added-profile lookup is not properly org-scoped.
In
`@tests/server/services/storage/test_lineage_b3_request_id_invariant_integration.py`:
- Line 53: The `_make_profile` function uses the `or` operator in the
`generated_from_request_id` assignment, which treats empty strings as falsy
values and replaces them with the default `f"gen_{profile_id}"`. This prevents
the `test_empty_request_id_group_is_skipped` test from actually testing with
empty `generated_from_request_id` values. Replace the `or` operator logic with
an explicit None check so that empty strings are preserved as-is while only
generating the default ID when `request_id` is None, allowing the test to
properly exercise the empty request ID invariant.
---
Nitpick comments:
In
`@tests/server/services/storage/test_lineage_b3_reconstruct_changelog_integration.py`:
- Around line 378-400: The test function `test_adds_only_run_produces_row` has a
name that contradicts its actual assertion. The test name implies it should
produce a row, but the assertion at the end verifies that `profile_change_logs
== []`, meaning no rows are produced. Rename the test function to accurately
reflect that it produces no rows when there are only adds without any lineage
events (e.g., something like `test_adds_only_run_produces_no_row` or
`test_adds_only_without_lineage_events_produces_empty_log`).
🪄 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: fb4a21d4-a23d-4e99-ad6d-2f8715e2ab53
📒 Files selected for processing (14)
.superpowers/sdd/task-b3pre-1-report.mdreflexio/lib/_profiles.pyreflexio/server/services/profile/profile_generation_service.pyreflexio/server/services/storage/sqlite_storage/_profiles.pyreflexio/server/services/storage/storage_base/_profiles.pyreflexio/server/site_var/feature_flags.pyscripts/lineage_b3_parity_check.pytests/server/api_endpoints/test_profile_change_log_api_integration.pytests/server/services/profile/test_dedup_soft_delete_integration.pytests/server/services/storage/test_lineage_b3_parity_classifier_integration.pytests/server/services/storage/test_lineage_b3_parity_gate_integration.pytests/server/services/storage/test_lineage_b3_reconstruct_changelog_integration.pytests/server/services/storage/test_lineage_b3_request_id_invariant_integration.pytests/server/site_var/test_feature_flags.py
Comment on lines
+598
to
+599
| if limit == 0: | ||
| return ProfileChangeLogResponse(success=True, profile_change_logs=[]) |
There was a problem hiding this comment.
Validate negative limit values explicitly.
limit < 0 currently falls through to Python slicing and returns “all but last N” rows, which is an unintended API behavior for a max-results parameter.
Suggested fix
- if limit == 0:
+ if limit < 0:
+ raise ValueError("limit must be >= 0")
+ if limit == 0:
return ProfileChangeLogResponse(success=True, profile_change_logs=[])Also applies to: 640-644
🤖 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/lib/_profiles.py` around lines 598 - 599, The code currently only
checks for limit == 0 but does not validate negative limit values, which causes
them to fall through to Python's slicing behavior and return unintended results.
Add explicit validation to check for limit < 0 in addition to the existing limit
== 0 check, and return a ProfileChangeLogResponse with success=True and an empty
profile_change_logs list for negative values as well, treating negative limits
the same as zero. Apply this fix at all occurrences where this validation is
needed, including the additional locations referenced in the comment.
Comment on lines
+476
to
+478
| rows = self._fetchall( | ||
| "SELECT * FROM profiles WHERE generated_from_request_id = ?", | ||
| (request_id,), |
There was a problem hiding this comment.
get_profiles_by_generated_from_request_id is not org-scoped.
Line 477 filters only by generated_from_request_id; with a shared SQLite DB, a request_id collision across orgs can return another org’s profiles, violating the org-scoped contract and leaking tenant data.
🤖 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
476 - 478, The `get_profiles_by_generated_from_request_id` method's SELECT query
in the `_fetchall` call is missing organization scoping in its WHERE clause.
Currently it only filters by generated_from_request_id, which can return
profiles from other organizations in a shared database if request IDs collide
across tenants. Add an additional condition to the WHERE clause that filters by
organization ID alongside the generated_from_request_id parameter to ensure only
the current organization's profiles are returned and prevent cross-tenant data
leakage.
Comment on lines
+139
to
+140
| enabled_org_ids = feature_config.get("enabled_org_ids", []) | ||
| return org_id in enabled_org_ids |
There was a problem hiding this comment.
Handle enabled_org_ids=None defensively to avoid runtime crashes.
Line 139 can return None when the key exists but is null, and Line 140 then raises TypeError on membership checks. This turns a config issue into a request-time failure.
💡 Suggested fix
- enabled_org_ids = feature_config.get("enabled_org_ids", [])
+ enabled_org_ids = feature_config.get("enabled_org_ids") or []
return org_id in enabled_org_ids📝 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
| enabled_org_ids = feature_config.get("enabled_org_ids", []) | |
| return org_id in enabled_org_ids | |
| enabled_org_ids = feature_config.get("enabled_org_ids") or [] | |
| return org_id in enabled_org_ids |
🤖 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/site_var/feature_flags.py` around lines 139 - 140, The
feature_config.get method on the line retrieving enabled_org_ids will return
None if the key exists but its value is null, which causes a TypeError when
attempting the membership check with the in operator on the next line. Modify
the retrieval of enabled_org_ids to ensure it defaults to an empty list not only
when the key is missing but also when the key exists with a None value, such as
by using the or operator to provide a fallback empty list if the retrieved value
is None or falsy.
Comment on lines
+94
to
+102
| legacy_by_req: dict[str, ProfileChangeLog] = {} | ||
| for row in legacy_rows: | ||
| # Last-write wins on duplicate request_ids (should not happen in production). | ||
| legacy_by_req[row.request_id] = row | ||
|
|
||
| recon_by_req: dict[str, ProfileChangeLog] = {} | ||
| for row in recon_rows: | ||
| recon_by_req[row.request_id] = row | ||
|
|
There was a problem hiding this comment.
Fail closed on duplicate request_id rows instead of silently overwriting.
At Line 97 and Line 101, duplicate keys are collapsed with last-write-wins. That can hide parity mismatches and falsely report green for a request that has conflicting rows. Treat duplicate request_ids as a parity gap (or hard error), not a silent overwrite.
Suggested fail-closed change
- legacy_by_req: dict[str, ProfileChangeLog] = {}
+ legacy_by_req: dict[str, list[ProfileChangeLog]] = {}
for row in legacy_rows:
- # Last-write wins on duplicate request_ids (should not happen in production).
- legacy_by_req[row.request_id] = row
+ legacy_by_req.setdefault(row.request_id, []).append(row)
- recon_by_req: dict[str, ProfileChangeLog] = {}
+ recon_by_req: dict[str, list[ProfileChangeLog]] = {}
for row in recon_rows:
- recon_by_req[row.request_id] = row
+ recon_by_req.setdefault(row.request_id, []).append(row)
@@
- in_legacy = req_id in legacy_by_req
- in_recon = req_id in recon_by_req
+ legacy_items = legacy_by_req.get(req_id, [])
+ recon_items = recon_by_req.get(req_id, [])
+ in_legacy = bool(legacy_items)
+ in_recon = bool(recon_items)
+
+ if len(legacy_items) > 1 or len(recon_items) > 1:
+ results.append(
+ ParityResult(
+ request_id=req_id,
+ classification=ParityClass.RECON_MISSING,
+ detail="duplicate request_id rows detected; parity result unsafe",
+ )
+ )
+ continue
if in_legacy and in_recon:
- if _rows_match(legacy_by_req[req_id], recon_by_req[req_id]):
+ if _rows_match(legacy_items[0], recon_items[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 `@scripts/lineage_b3_parity_check.py` around lines 94 - 102, Instead of
silently overwriting duplicate request_ids with last-write-wins behavior in both
the legacy_by_req and recon_by_req dictionaries, add validation to fail closed
when duplicates are detected. In the loops iterating over legacy_rows and
recon_rows, check if the request_id already exists in the respective dictionary
before assignment. If a duplicate is found, raise an error or record it as a
parity gap to ensure duplicate rows are explicitly handled and do not hide
potential parity mismatches.
Comment on lines
+186
to
+189
| match_results = [r for r in results if r.request_id.startswith("req-full-")] | ||
| assert all(r.classification == ParityClass.MATCH for r in match_results), ( | ||
| f"expected all MATCH but got: {[(r.request_id, r.classification) for r in match_results]}" | ||
| ) |
There was a problem hiding this comment.
Guard against vacuous pass in MATCH assertion.
At Line 187, all(...) can pass on an empty match_results. Assert the expected cardinality first so missing rows cannot pass silently.
Suggested assertion hardening
results = run_parity_check(s)
match_results = [r for r in results if r.request_id.startswith("req-full-")]
+ assert len(match_results) == 3, f"expected 3 req-full-* rows, got {len(match_results)}"
assert all(r.classification == ParityClass.MATCH for r in match_results), (
f"expected all MATCH but got: {[(r.request_id, r.classification) for r in match_results]}"
)📝 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
| match_results = [r for r in results if r.request_id.startswith("req-full-")] | |
| assert all(r.classification == ParityClass.MATCH for r in match_results), ( | |
| f"expected all MATCH but got: {[(r.request_id, r.classification) for r in match_results]}" | |
| ) | |
| match_results = [r for r in results if r.request_id.startswith("req-full-")] | |
| assert len(match_results) == 3, f"expected 3 req-full-* rows, got {len(match_results)}" | |
| assert all(r.classification == ParityClass.MATCH for r in match_results), ( | |
| f"expected all MATCH but got: {[(r.request_id, r.classification) for r in match_results]}" | |
| ) |
🤖 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_b3_parity_classifier_integration.py`
around lines 186 - 189, The assertion using all() on match_results can vacuously
pass if the filtered list is empty, which would hide missing expected data.
Before the existing assertion that checks all classifications are MATCH, add an
assertion to verify that match_results has the expected cardinality (is not
empty or has the expected number of entries). This ensures the test fails if the
filtering operation for items with request_id starting with "req-full-" returns
no rows, rather than silently passing.
Comment on lines
+12
to
+14
| 2. LEGACY-MISSING — a legacy row has no corresponding lineage event. The | ||
| reconstruction simply returns no row; the discrepancy is *tolerated* | ||
| (best-effort drop), not a failure. |
There was a problem hiding this comment.
Fix terminology: this case is RECON-MISSING, not LEGACY-MISSING.
Line 12 and Line 220 describe “legacy row with no lineage event” as LEGACY-MISSING, but that is the RECON-MISSING direction in this PR’s classifier. Rename to avoid confusion during gate triage.
Also applies to: 220-225
🤖 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_b3_parity_gate_integration.py`
around lines 12 - 14, Rename the case description from "LEGACY-MISSING" to
"RECON-MISSING" in the lineage classification documentation. The case describing
"a legacy row has no corresponding lineage event" should use the RECON-MISSING
terminology instead of LEGACY-MISSING to align with the PR's classifier
direction definitions. Update all occurrences of this mislabeled case throughout
the test file, specifically in the case descriptions and comments that explain
the reconstruction behavior when a legacy row has no lineage event.
Comment on lines
+195
to
+203
| legacy_by_req = {row.request_id: row for row in s.get_profile_change_logs()} | ||
| recon_by_req = { | ||
| row.request_id: row | ||
| for row in reconstruct_profile_change_log(s).profile_change_logs | ||
| } | ||
|
|
||
| for req_id, legacy in legacy_by_req.items(): | ||
| assert req_id in recon_by_req, f"reconstruction missing req_id={req_id}" | ||
| recon_row = recon_by_req[req_id] |
There was a problem hiding this comment.
Assert exact request_id set equality in multi-run parity gate.
Line 202 only checks legacy→recon inclusion. Add a recon→legacy check (or set equality) so unexpected extra reconstructed rows fail this gate test.
Suggested parity-set assertion
legacy_by_req = {row.request_id: row for row in s.get_profile_change_logs()}
recon_by_req = {
row.request_id: row
for row in reconstruct_profile_change_log(s).profile_change_logs
}
+ assert set(recon_by_req) == set(legacy_by_req), (
+ f"request_id set mismatch: recon={set(recon_by_req)} legacy={set(legacy_by_req)}"
+ )
for req_id, legacy in legacy_by_req.items():
assert req_id in recon_by_req, f"reconstruction missing req_id={req_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 `@tests/server/services/storage/test_lineage_b3_parity_gate_integration.py`
around lines 195 - 203, The assertion in the loop checking legacy_by_req items
exist in recon_by_req only validates one direction of the comparison. Add a
bidirectional check to ensure both dictionaries contain exactly the same set of
request_ids. After the existing loop that validates each legacy request_id
exists in recon_by_req, add an assertion that checks the reverse direction or
that the keys of both legacy_by_req and recon_by_req are equal, so that any
extra reconstructed rows with unexpected request_ids will be caught and fail the
parity gate test.
Comment on lines
+503
to
+543
| def test_cross_org_isolation(tmp_path): | ||
| """reconstruct_profile_change_log for org A must not return org B's events.""" | ||
| db_path = str(tmp_path / "shared.db") | ||
| s_a = SQLiteStorage(org_id="org-a", db_path=db_path) | ||
| s_a.migrate() | ||
| s_b = SQLiteStorage(org_id="org-b", db_path=db_path) | ||
|
|
||
| old_a = _make_profile(user_id="ua", profile_id="pa-old", request_id="r-a-seed") | ||
| s_a.add_user_profile("ua", [old_a]) | ||
| new_a = _make_profile(user_id="ua", profile_id="pa-new", request_id="r-a-run") | ||
| _seed_dedup_run( | ||
| s_a, | ||
| user_id="ua", | ||
| new_profiles=[new_a], | ||
| old_ids=["pa-old"], | ||
| request_id="r-a-run", | ||
| ) | ||
|
|
||
| old_b = _make_profile(user_id="ub", profile_id="pb-old", request_id="r-b-seed") | ||
| s_b.add_user_profile("ub", [old_b]) | ||
| new_b = _make_profile(user_id="ub", profile_id="pb-new", request_id="r-b-run") | ||
| _seed_dedup_run( | ||
| s_b, | ||
| user_id="ub", | ||
| new_profiles=[new_b], | ||
| old_ids=["pb-old"], | ||
| request_id="r-b-run", | ||
| ) | ||
|
|
||
| result_a = reconstruct_profile_change_log(s_a) | ||
| assert result_a.success | ||
| req_ids_a = {row.request_id for row in result_a.profile_change_logs} | ||
| assert "r-a-run" in req_ids_a | ||
| assert "r-b-run" not in req_ids_a | ||
|
|
||
| result_b = reconstruct_profile_change_log(s_b) | ||
| assert result_b.success | ||
| req_ids_b = {row.request_id for row in result_b.profile_change_logs} | ||
| assert "r-b-run" in req_ids_b | ||
| assert "r-a-run" not in req_ids_b | ||
|
|
There was a problem hiding this comment.
Cross-org isolation test misses same-request_id collision case.
This test only proves isolation when request IDs differ across orgs. It won’t catch added-profile leakage if both orgs use the same request_id and added lookup is not org-scoped.
Suggested test hardening
def test_cross_org_isolation(tmp_path):
"""reconstruct_profile_change_log for org A must not return org B's events."""
@@
- new_a = _make_profile(user_id="ua", profile_id="pa-new", request_id="r-a-run")
+ shared_req = "r-shared"
+ new_a = _make_profile(user_id="ua", profile_id="pa-new", request_id=shared_req)
@@
- request_id="r-a-run",
+ request_id=shared_req,
@@
- new_b = _make_profile(user_id="ub", profile_id="pb-new", request_id="r-b-run")
+ new_b = _make_profile(user_id="ub", profile_id="pb-new", request_id=shared_req)
@@
- request_id="r-b-run",
+ request_id=shared_req,
@@
- req_ids_a = {row.request_id for row in result_a.profile_change_logs}
- assert "r-a-run" in req_ids_a
- assert "r-b-run" not in req_ids_a
+ req_ids_a = {row.request_id for row in result_a.profile_change_logs}
+ assert shared_req in req_ids_a
+ row_a = next(r for r in result_a.profile_change_logs if r.request_id == shared_req)
+ assert {p.profile_id for p in row_a.added_profiles} == {"pa-new"}
+ assert {p.profile_id for p in row_a.removed_profiles} == {"pa-old"}
@@
- req_ids_b = {row.request_id for row in result_b.profile_change_logs}
- assert "r-b-run" in req_ids_b
- assert "r-a-run" not in req_ids_b
+ req_ids_b = {row.request_id for row in result_b.profile_change_logs}
+ assert shared_req in req_ids_b
+ row_b = next(r for r in result_b.profile_change_logs if r.request_id == shared_req)
+ assert {p.profile_id for p in row_b.added_profiles} == {"pb-new"}
+ assert {p.profile_id for p in row_b.removed_profiles} == {"pb-old"}🤖 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_b3_reconstruct_changelog_integration.py`
around lines 503 - 543, The test_cross_org_isolation function uses different
request_ids for each organization (r-a-run for org A and r-b-run for org B),
which doesn't test the case where both organizations might use the same
request_id. To properly test isolation, modify the test to have both
organizations use the same request_id value (e.g., use "r-shared" for both the
_seed_dedup_run calls in s_a and s_b), then verify that each organization's
result still only contains its own data and not the other organization's data.
This will catch if the added-profile lookup is not properly org-scoped.
…rated_from_request_id union (close completeness gap) — B3-pre T6a
There was a problem hiding this comment.
🧹 Nitpick comments (2)
reflexio/server/services/storage/storage_base/_profiles.py (1)
302-326: ⚡ Quick winClarify the eligible status values in the docstring.
Line 311 says "current status in {NULL/CURRENT, PENDING}" which could be misread as three distinct values. Since NULL and CURRENT are the same (both represented as None), consider rephrasing to "current status is NULL (CURRENT) or PENDING" for clarity.
📝 Suggested docstring clarification
"""Soft-delete profiles by setting status to SUPERSEDED, emitting set-based lineage. - For each profile id that matches (user_id, current status in {NULL/CURRENT, - PENDING}), updates status to SUPERSEDED and emits one ``status_change`` + For each profile id that matches (user_id, current status is NULL (CURRENT) + or PENDING), updates status to SUPERSEDED and emits one ``status_change`` lineage event under the shared ``request_id``. Rows are NOT physically🤖 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/_profiles.py` around lines 302 - 326, In the `supersede_profiles_by_ids` abstract method docstring, clarify that NULL and CURRENT represent the same value (None). Rephrase the line describing eligible status values from "current status in {NULL/CURRENT, PENDING}" to "current status is NULL (CURRENT) or PENDING" to make it clear there are only two distinct eligible states, not three, since NULL and CURRENT are equivalent representations.reflexio/server/services/storage/sqlite_storage/_profiles.py (1)
524-597: 💤 Low valueConsider documenting or validating empty
request_idbehavior.The method emits lineage events stamped with the provided
request_id, but there's no validation thatrequest_idis non-empty. If an emptyrequest_idis passed, the events will be emitted but skipped during reconstruction (pertest_empty_request_id_group_is_skipped), making the dedup run non-reconstructible.Since the service layer is expected to enforce the
assert_nonempty_request_idinvariant (per PR objectives), consider either:
- Adding a defensive assertion here:
assert request_id, "request_id must be non-empty for reconstructible lineage"- Documenting in the docstring: "Note: empty request_id values are allowed but will result in non-reconstructible events."
🤖 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 524 - 597, Add validation to the supersede_profiles_by_ids method to ensure request_id is non-empty before processing profiles. Include an assertion at the beginning of the method (after the if not profile_ids check) that validates request_id is not empty, such as `assert request_id, "request_id must be non-empty for reconstructible lineage"`. This ensures that emitted lineage events will be reconstructible and prevents silent failures where events are emitted with an empty request_id that would be skipped during reconstruction.
🤖 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.
Nitpick comments:
In `@reflexio/server/services/storage/sqlite_storage/_profiles.py`:
- Around line 524-597: Add validation to the supersede_profiles_by_ids method to
ensure request_id is non-empty before processing profiles. Include an assertion
at the beginning of the method (after the if not profile_ids check) that
validates request_id is not empty, such as `assert request_id, "request_id must
be non-empty for reconstructible lineage"`. This ensures that emitted lineage
events will be reconstructible and prevents silent failures where events are
emitted with an empty request_id that would be skipped during reconstruction.
In `@reflexio/server/services/storage/storage_base/_profiles.py`:
- Around line 302-326: In the `supersede_profiles_by_ids` abstract method
docstring, clarify that NULL and CURRENT represent the same value (None).
Rephrase the line describing eligible status values from "current status in
{NULL/CURRENT, PENDING}" to "current status is NULL (CURRENT) or PENDING" to
make it clear there are only two distinct eligible states, not three, since NULL
and CURRENT are equivalent representations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8979ada2-8c8d-432b-96ae-8becb775bcf4
📒 Files selected for processing (5)
reflexio/lib/_profiles.pyreflexio/server/services/storage/sqlite_storage/_profiles.pyreflexio/server/services/storage/storage_base/_profiles.pytests/server/services/storage/test_lineage_b3_reconstruct_changelog_integration.pytests/server/services/storage/test_lineage_b3_request_id_invariant_integration.py
🚧 Files skipped from review as they are similar to previous changes (3)
- reflexio/lib/_profiles.py
- tests/server/services/storage/test_lineage_b3_reconstruct_changelog_integration.py
- tests/server/services/storage/test_lineage_b3_request_id_invariant_integration.py
Collaborator
Author
|
CodeRabbit findings addressed in follow-up PR #193 (commit |
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 -->
This was referenced Jun 22, 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.
What
Lineage Phase B3-pre — write-side instrumentation that makes the legacy
ProfileChangeLogreconstructable from lineage, plus the read-side reconstruction + parity tooling (B3 Stage-1). Ships flag-OFF and non-destructive — it does NOT retire the legacy log (that's the future B3 retirement).Why
The B3 retirement plan was found (via
/review-design-doc+ a final review) to be infeasible as written: the profile-dedup path hard-deletes superseded profiles (no content, no linkage), so the content-freelineage_eventlog and the legacyProfileChangeLogwere disjoint — reconstruction couldn't reproduce the legacy log. This PR fixes the write side.Changes (behind a fail-CLOSED, default-OFF per-org flag)
is_dedup_soft_delete_enabled— fail-closed flag (missing config → OFF; does NOT use the fail-openis_feature_enabled).status=SUPERSEDED, content retained) viasupersede_profiles_by_idsand emits set-basedstatus_changelineage under the run's sharedrequest_id, atomically (no FTS/vec deletion; tombstones excluded from all reads by status filter). Flag OFF = byte-for-byte the current hard-delete.reconstruct_profile_change_logreworked to a time-travel-stable column+event model:added= the immutablegenerated_from_request_idcolumn;removed=status_change/supersededevents under therequest_id. Dedup-scoped.request_idinvariant.Safety
GC reclaims the tombstones this would create, so the flag must not be enabled until B2 GC has a retention story (PB-9/PB-5). The flag is default-OFF everywhere; the endpoint still serves the legacy table (a Stage-1 repoint was reverted). No table drop, no
mentioned_profilesremoval.Known follow-up (for the future B3 retirement, not this PR)
Add-only dedup runs emit no lineage event, so they're omitted from reconstruction (a
RECON-MISSINGthe parity gate catches). The retirement must close this before any endpoint cutover.Built via subagent-driven-development; full per-task + final whole-branch review (verdict: ready to ship flag-OFF).
Summary by CodeRabbit
dedup_soft_deletefeature flag is enabled.GET /api/profile_change_log.Update (gap closed in-PR): The add-only-run reconstruction completeness gap noted above is now fixed in this PR (T6) — the reconstruction discovers add-only dedup runs via a distinct
generated_from_request_idunion (OSS + enterprise), with tests. Reconstruction is complete (add-only runs included), not a deferred follow-up.