feat(cloud): populate rule_patches from RULE_PATCHED events in /sync#43
Conversation
The self-healing dashboard panel reads from the rule_patches table, but nothing was writing to it — apply_patch mutates lessons in memory, the SDK emits a RULE_PATCHED event, and the event lands in the events table, but rule_patches stayed orphan. Dashboard showed empty state even when self-healing had run. Fix: when /sync receives events with type='RULE_PATCHED', resolve the lesson by (brain_id, category), then insert a matching rule_patches row with old/new description + reason. Skip gracefully if no lesson matches (edge: patch arrived without the corresponding lesson sync). Tests: 2 new — populates rule_patches on match, skips silently on miss. All 7 /sync tests green. Co-Authored-By: Gradata <noreply@gradata.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe sync handler now extracts Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Sync Handler
participant EventsDB as Events DB
participant LessonDB as Lessons DB
participant PatchDB as Rule Patches DB
Client->>Handler: POST /api/v1/sync (payload with events)
Handler->>EventsDB: Insert body.events into `events` table
Handler->>Handler: Filter `RULE_PATCHED` events and validate `data` fields
Handler->>Handler: Normalize categories (uppercase) and collect distinct ones
loop per category
Handler->>LessonDB: Query `lessons` by (brain_id, category)
LessonDB-->>Handler: Return candidate lessons
Handler->>Handler: Resolve `lesson_id` (match new_description → old_description → first)
end
Handler->>Handler: Build `rule_patches` rows (include reason/created_at if present)
alt any valid rows
Handler->>PatchDB: Insert rows into `rule_patches`
PatchDB-->>Handler: Insert complete
end
Handler-->>Client: 200 OK with sync counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/app/routes/sync.py`:
- Around line 125-150: The loop over patched events silently skips events
missing category/old_description/new_description; update the loop around patched
(where patch_rows is built, and variables ev, data, cat, old_desc, new_desc are
computed) to log a warning whenever an event is skipped for missing required
fields: include the brain_id, which fields are missing
(category/old_description/new_description), and useful event identifiers (e.g.,
ev.id or ev.created_at) to aid debugging before continuing; keep the existing
warning for lesson resolution (where _resolve_lesson_id is called) unchanged and
still continue to skip when no lesson_id is found before appending to patch_rows
and calling db.insert("rule_patches", patch_rows).
In `@cloud/tests/test_sync.py`:
- Around line 144-176: In
test_sync_rule_patched_event_without_matching_lesson_is_skipped, after asserting
resp.status_code == 200, add an assertion that the sync reported one event
inserted by checking resp.json()["events_synced"] == 1 so the test verifies the
event row was written even though no rule_patches row was created; locate the
assertion block using the test function name and the resp variable and insert
the new check immediately after the existing status code assertion.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86668dd2-9875-49d2-8344-2af6f2c724ae
📒 Files selected for processing (2)
cloud/app/routes/sync.pycloud/tests/test_sync.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
cloud/tests/test_sync.py (1)
92-141: LGTM!The test comprehensively covers the happy path: seeding a matching lesson, submitting a
RULE_PATCHEDevent, and verifying therule_patchesrow is created with correctlesson_id, descriptions, and reason.cloud/app/routes/sync.py (4)
94-100: LGTM!The category extraction includes appropriate defensive checks: verifying
e.datais a dict and filtering out empty/None categories before uppercasing.
101-112: LGTM!The per-category query approach is reasonable given the noted db.select limitation. The inline comment provides good context for future maintainers.
114-123: LGTM!The resolution strategy is sound: prefer an exact description match (either side of the patch), with a deterministic fallback to the first candidate. This handles the common case where the lesson's description has already been updated.
91-93: LGTM!The comment clearly explains the purpose:
RULE_PATCHEDevents populate therule_patchestable to enable the self-healing dashboard. The implementation correctly processes events after they're already inserted, maintaining the events table as the source of truth.
- log a warning (with brain_id, missing fields, event timestamp) when a RULE_PATCHED event lacks category/old_description/new_description, so a silent-drop failure is debuggable. - extend the no-matching-lesson test to also assert events_synced == 1, locking in the guarantee that the event row still lands even when rule_patches is skipped. Co-Authored-By: Gradata <noreply@gradata.ai>
Deploying gradata-dashboard with
|
| Latest commit: |
2ecc0ab
|
| Status: | ⚡️ Build in progress... |
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Addressed both CR actionables in 87c5a5f: warning log for skipped RULE_PATCHED events + events_synced assertion. Tests 7/7 green. @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
|
Addressed CR feedback (both items were resolved in commit 87c5a5f, already on the branch):
Tests: |
|
@coderabbitai review — fixes pushed, please re-verify. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cloud/app/routes/sync.py (1)
101-162:⚠️ Potential issue | 🟠 MajorMissing-field warnings are still bypassed when all patched events lack
category.Line 101 gates processing on
if categories:, and the validation loop at Line 126 sits inside that block. If everyRULE_PATCHEDevent is missingcategory,categoriesis empty and no warning is emitted before skipping.💡 Proposed fix
patched = [e for e in body.events if e.type == "RULE_PATCHED"] if patched: categories = sorted({ (e.data.get("category") or "").upper() for e in patched if isinstance(e.data, dict) and e.data.get("category") }) - if categories: - # One query per category (our thin PostgREST wrapper only - # supports eq filters). Small N — one per patched category. - by_cat: dict[str, list[dict]] = {} - for cat in categories: - rows = await db.select( - "lessons", - columns="id,category,description", - filters={"brain_id": brain_id, "category": cat}, - ) - if rows: - by_cat[cat] = rows - - def _resolve_lesson_id(cat: str, old_desc: str, new_desc: str) -> str | None: - candidates = by_cat.get(cat, []) - if not candidates: - return None - # Prefer a lesson whose current description matches either - # side of the patch (ordering-agnostic). Fall back to first. - for lesson in candidates: - if lesson.get("description") in (new_desc, old_desc): - return lesson["id"] - return candidates[0]["id"] - - patch_rows: list[dict] = [] - for ev in patched: + # One query per category (our thin PostgREST wrapper only + # supports eq filters). Small N — one per patched category. + by_cat: dict[str, list[dict]] = {} + for cat in categories: + rows = await db.select( + "lessons", + columns="id,category,description", + filters={"brain_id": brain_id, "category": cat}, + ) + if rows: + by_cat[cat] = rows + + def _resolve_lesson_id(cat: str, old_desc: str, new_desc: str) -> str | None: + candidates = by_cat.get(cat, []) + if not candidates: + return None + # Prefer a lesson whose current description matches either + # side of the patch (ordering-agnostic). Fall back to first. + for lesson in candidates: + if lesson.get("description") in (new_desc, old_desc): + return lesson["id"] + return candidates[0]["id"] + + patch_rows: list[dict] = [] + for ev in patched: data = ev.data if isinstance(ev.data, dict) else {} cat = (data.get("category") or "").upper() old_desc = data.get("old_description") or "" new_desc = data.get("new_description") or "" if not cat or not old_desc or not new_desc: @@ - patch_rows.append(row) - if patch_rows: - await db.insert("rule_patches", patch_rows) + patch_rows.append(row) + if patch_rows: + await db.insert("rule_patches", patch_rows)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/app/routes/sync.py` around lines 101 - 162, The current code only validates RULE_PATCHED events when categories is truthy, so if every patched event lacks category no warnings are emitted; change the flow so validation of each ev in patched (the checks that compute cat, old_desc, new_desc and call _log.warning for missing fields) runs regardless of whether categories is non-empty, and only perform the database lookups/creation of by_cat and calls to _resolve_lesson_id / db.insert when categories exists and there are candidates; in practice move the missing-field check (the block that builds missing and logs via _log.warning) out of the if categories: branch so patched events without category still produce warnings, while keeping lesson-id resolution, patch_rows accumulation, and the final await db.insert("rule_patches", patch_rows) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/tests/test_sync.py`:
- Around line 144-177: Add a new test (similar to
test_sync_rule_patched_event_without_matching_lesson_is_skipped) that sends a
RULE_PATCHED event with malformed payloads missing required fields (e.g., no
"category", or missing "old_description"/"new_description") to POST /api/v1/sync
using the same fixtures (client, mock_supabase, auth_headers) and assert
status_code == 200 and events_synced == 1, then use caplog to assert a warning
was emitted and verify no rule_patch insert occurred by inspecting
mock_supabase._inserts for rows containing "old_description"/"new_description";
name the test clearly (e.g.,
test_sync_rule_patched_malformed_payload_is_warned_and_skipped) so it mirrors
existing tests and prevents telemetry regressions.
---
Duplicate comments:
In `@cloud/app/routes/sync.py`:
- Around line 101-162: The current code only validates RULE_PATCHED events when
categories is truthy, so if every patched event lacks category no warnings are
emitted; change the flow so validation of each ev in patched (the checks that
compute cat, old_desc, new_desc and call _log.warning for missing fields) runs
regardless of whether categories is non-empty, and only perform the database
lookups/creation of by_cat and calls to _resolve_lesson_id / db.insert when
categories exists and there are candidates; in practice move the missing-field
check (the block that builds missing and logs via _log.warning) out of the if
categories: branch so patched events without category still produce warnings,
while keeping lesson-id resolution, patch_rows accumulation, and the final await
db.insert("rule_patches", patch_rows) behavior unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fb989d9-79c1-453d-b2d1-d7c8884cbdb8
📒 Files selected for processing (2)
cloud/app/routes/sync.pycloud/tests/test_sync.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
cloud/tests/test_sync.py (1)
92-142: Good coverage for the resolvable RULE_PATCHED path.This test correctly verifies both
events_syncedand the insertedrule_patchespayload shape, includinglesson_idmapping and reason propagation.
Round-3 CR on #43: - sync.py: lift missing-field validation out of the `if categories:` guard so warnings still fire when every patched event lacks category. - test_sync.py: add test_sync_rule_patched_malformed_payload_is_warned_and_skipped covering the all-malformed path with caplog.
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Addressed round-3 CR:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/app/routes/sync.py`:
- Around line 102-104: The sync handler reads untrusted fields without type
checks (variables cat, old_desc, new_desc and similarly the 'reason' usage
later) and calls string methods like .upper(), which can raise AttributeError
for non-strings; update the code in cloud/app/routes/sync.py to validate types
before string operations by using isinstance(value, str) (e.g., cat =
data.get("category") if isinstance(data.get("category"), str) else "" and cat =
cat.upper()) or explicitly coerce with safe conversion (e.g., old_desc =
str(data.get("old_description")) if data.get("old_description") is not None else
""), and apply the same pattern for new_description and reason locations
(including the occurrence around line 160) so only strings are passed to
.upper() or other string methods.
In `@cloud/tests/test_sync.py`:
- Around line 203-225: Add a malformed-type RULE_PATCHED event to the events
list that sets "category": 123 (integer) with "old_description": "x" and
"new_description": "y" so the test covers type-safety; then after posting,
assert the HTTP response is 200, the events_synced counter increments, a warning
log is emitted, and no insert into the rule_patches table occurs. Apply the same
malformed-type case and identical assertions to the second events block
mentioned (the one around lines 231-244) so both paths are covered.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2496bce4-d39d-4b31-9663-24612fb9a00e
📒 Files selected for processing (2)
cloud/app/routes/sync.pycloud/tests/test_sync.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
cloud/app/routes/sync.py (1)
96-119: Good defensive flow: validation-first pass with explicit warning logs.The “validate then process” split avoids silent drops when all RULE_PATCHED payloads are malformed and preserves observability.
cloud/tests/test_sync.py (1)
94-143: Coverage is strong for the new RULE_PATCHED flows.Happy path, unresolved lesson skip, and malformed warning/skip behavior are all covered with useful assertions.
Also applies to: 146-179, 182-252
CR round-4: untrusted SDK clients can send category=123 (or any non-string truthy), which would have crashed the sync handler with AttributeError on .upper() *after* events were already inserted. Now we isinstance-check category/old_description/new_description/reason and treat non-strings as missing — same warning + skip path. Test: extend the malformed-payload test with a category=123 case; asserts 200, events_synced==3, warning emitted, no rule_patches insert.
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Round-4 CR addressed (commit 9849299):
All 118 cloud tests pass. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review — fixes pushed since last review, please re-verify |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review — CR Pro active, please re-verify. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/app/routes/sync.py`:
- Around line 111-117: The malformed-event gate currently omits validating the
reason field, so non-string/blank reason values bypass the warning-and-skip
logic and later get coerced to "self_healing"; update the gate that checks
missing fields (where cat, old_desc, new_desc are validated) to include reason
in the missing list and treat empty/non-string reason as a missing/invalid field
so the flow logs a warning and skips the event instead of inserting a coerced
value; locate the block that builds missing = [...] (using variables cat,
old_desc, new_desc) and add ("reason", reason) to that tuple, and remove or
change the later logic that silently coerces non-string/blank reason to
"self_healing" so invalid reasons are rejected at the malformed-event gate.
- Around line 127-178: The rule_patches write path (blocks using by_cat,
_resolve_lesson_id, patch_rows and db.insert("rule_patches", ...)) must be
isolated so failures there do not cause /sync to fail after events were already
persisted; wrap the category lookups and the final insert in a try/except around
the loop that builds patch_rows and the db.insert call, catch and log any
exception (including the exception details) via _log.error with context
(brain_id, categories) and then continue without re-raising so the handler
returns success; ensure you still skip missing lessons as before and only
suppress unexpected DB/query errors while preserving existing behavior for
valid_events and lessons selection.
In `@cloud/tests/test_sync.py`:
- Around line 182-265: In
test_sync_rule_patched_malformed_payload_is_warned_and_skipped add additional
malformed RULE_PATCHED events that exercise invalid/missing reason values (e.g.,
one event with no reason key and one with reason as a non-string like 123), then
assert that the caplog warning collection (warning_msgs) contains entries
mentioning "reason" for those events and that rule_patches inserts remain empty
(same rule_patch_inserts check); update any existing assertions that check for
specific missing-field messages to also expect warnings about "reason" so the
test fails if reason validation regresses.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b78a46ac-303c-4065-bf22-3cb1ece9a932
📒 Files selected for processing (2)
cloud/app/routes/sync.pycloud/tests/test_sync.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
cloud/tests/test_sync.py (1)
94-179: Nice coverage on the new RULE_PATCHED happy-path and no-lesson skip flows.These tests clearly verify
events_syncedbehavior andrule_patchesside effects.
CodeRabbit round-5 feedback on #43: - Add `reason` to the RULE_PATCHED malformed-event gate so non-string or blank reason values are warned + skipped instead of silently coerced to "self_healing". - Wrap the rule_patches lookup/insert path in try/except so a PostgREST outage there cannot fail /sync after events were already persisted (SDK retries would otherwise duplicate the event stream). Logs with exc_info for ops. - Tests: extend malformed-payload coverage with missing-reason + non- string-reason events; add write-failure isolation test that forces the lessons lookup to raise and asserts /sync still returns 200 with events_synced > 0 and no rule_patches inserts.
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Rebased on main + CR round-5 addressed:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review — rebased on latest main, all prior fixes applied |
|
✅ Actions performedReview triggered.
|
|
Rebased on latest main after batch merges |
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
End-to-end self-healing telemetry was broken: `apply_patch` mutated lessons in memory, the SDK emitted `RULE_PATCHED` events, but nothing ever wrote to `rule_patches` — so the self-healing dashboard panel always showed empty state.
This PR makes /sync tee `RULE_PATCHED` events into `rule_patches` as they arrive. Dashboard surface lights up as soon as an SDK brain patches a rule.
Changes
Not using IN(...) filter
Main's `db.select` is eq-only; the IN extension is queued in PR #41. Per-category loop is cheap (patched-categories per sync is typically 1-3) and avoids the merge dependency.
Test plan