fix(playbook): don't abort a rerun aggregation when mock mode has no centroid - #436
Conversation
`run_playbook_aggregation` always sets rerun=True (lib/_generation.py:73) and holds a claim, so every call reaches the branch that persists a cluster centroid. That branch requires `saved_fb.embedding`. The requirement was safe when it was written: local embeddings were computed in-process, so the field was always populated and the raise was unreachable outside a genuine bug. Routing local inference through a separate service turned "no embedding" into a normal runtime state -- with the embedder unreachable, storage logs "continuing without vector" and saves the playbook without one, and an invariant written to catch a programming error now fires on infrastructure and rolls the whole aggregation back. Mock mode is the case that has no centroid by construction: it clusters by trigger rather than by vector, so the centroid was never meaningful there. Skip the cluster bookkeeping instead of aborting. Every other caller still raises -- a centroid-less cluster row would silently break the incremental re-aggregation the table exists to feed. Found via 4 e2e failures (test_playbook_workflows, test_openclaw_- integration) that pass at the parent commit and fail from the routing change onward. They went unnoticed because the OSS repo runs no workflows and enterprise ci-fast passes --ignore=tests/e2e_tests/; the e2e tier only runs in release. Adds the coverage the invariant never had: the raise still fires outside mock mode, mock mode reaches the save and skips the centroid write, and the happy path still records the cluster. OSS e2e 47 passed (was 4 failed / 43 passed); unit tier 4368 passed.
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 review |
|
1 participant
Problem
4 e2e tests fail on
main:test_playbook_workflows.py— 3 teststest_openclaw_integration.py— 1 testThey pass at
b0e0755and fail fromc24e1e7onward, so this is a regression, not an environment quirk.Root cause
Two changes that were each fine alone.
159d8ab(#410) added the invariant: on the rerun path, a saved agent playbook must carry an embedding to persist as a cluster centroid. Safe at the time — local embeddings were computed in-process, sosaved_fb.embeddingwas always populated and the raise was unreachable outside a genuine bug.c24e1e7(#425) removed in-process local inference ("service-only inference boundary").embedding_provider_modenow returnslocal_servicefor any local model with no env vars set, so embedding goes over HTTP to127.0.0.1:8072.With the embedder unreachable,
SQLiteStoragedeliberately degrades:…and saves the playbook with
embedding=None. The invariant then fires and rolls back the entire aggregation. An assertion written to catch a programming error now trips on an infrastructure state.This is not narrow:
reflexio/lib/_generation.py:73setsrerun=Truefor everyrun_playbook_aggregation()call, so the branch is the normal path, not an edge case.Why it wasn't caught
ci-fast.yml:104runs--ignore=tests/e2e_tests/.release.yml.grep 'no centroid embedding' tests/returns nothing.Fix
Mock mode is the case that has no centroid by construction: it clusters by trigger rather than by vector (the
MOCK_LLM_RESPONSEbranch inget_clusters), so a centroid was never meaningful there. Skip the cluster bookkeeping instead of aborting.Every other caller still raises — a centroid-less cluster row would silently break the incremental re-aggregation that table exists to feed. Production behaviour outside mock mode is unchanged.
Adds the three cases the invariant never had:
Verified the new tests fail against the pre-fix aggregator with the exact
RuntimeError.Verification
Worth a second opinion
The mock-mode carve-out fixes the tests, but the underlying mismatch is broader: storage treats a missing embedding as degrade and continue, while the aggregator treats it as fatal. In any deployment where the embedding service is unreachable,
run_playbook_aggregation()now hard-fails and rolls back rather than degrading. That may well be intended — failing loudly beats silently writing a useless centroid — but it changed behaviour without discussion when #425 landed, so the owners of #410/#425 should confirm which semantics they want.