fix(playbook): restore apply_playbook_edit, deleted while still imported - #394
Conversation
#385 removed `playbook_edit_apply.py` as part of "remove the obsolete split-edit application path", but that module was not the split path — it is the shared atomic supersede primitive, and `review_service.py` still imports it at module scope and calls it at line 297. `apply_playbook_edit` exists nowhere else on main, so no replacement was shipped. `review_service.py` is imported by `server/routes/playbooks.py`, which loads at app startup, making this a guaranteed ModuleNotFoundError on boot. It was caught when a production deploy built from a clean checkout of main: the task started, failed its container health checks on the API port, and the deployment circuit breaker rolled the service back. Builds from working trees that still had the file on disk masked it. The whole `tests/server/services/playbook` directory currently fails at collection on main for the same reason. Restored verbatim from 4268ad1^; every dependency it uses (`supersede_record`, `commit_scope`, `save_user_playbooks`, `precompute_user_playbook_embeddings`, `LineageContext`) still exists.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesPlaybook edit application
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant apply_playbook_edit
participant new_playbook
participant storage.commit_scope
participant storage.supersede_record
apply_playbook_edit->>new_playbook: assign source and prepare embeddings
apply_playbook_edit->>storage.commit_scope: begin atomic update
storage.commit_scope->>new_playbook: save successor as CURRENT
apply_playbook_edit->>storage.supersede_record: conditionally supersede incumbent
storage.supersede_record-->>apply_playbook_edit: return CAS result
apply_playbook_edit-->>new_playbook: reset provisional id on race
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
reflexio/server/services/playbook/playbook_edit_apply.py (1)
73-87: 📐 Maintainability & Code Quality | 🔵 TrivialConsider logging when a supersede race is lost.
The insert-then-supersede sequencing and rollback-on-race logic (via
commit_scope+_LostSupersedeRaceError) is correct. As an optional improvement, the lost-race path (lines 84-86) silently returns-1with no log entry — a debug/info log here would help surface contention on frequently-edited playbooks in production.💡 Optional logging addition
except _LostSupersedeRaceError: + logger.debug( + "apply_playbook_edit: lost supersede race for incumbent_id=%s", incumbent_id + ) new_playbook.user_playbook_id = 0 return -1🤖 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/playbook/playbook_edit_apply.py` around lines 73 - 87, Add a debug or info log in the _LostSupersedeRaceError handler before resetting new_playbook.user_playbook_id and returning -1, recording that the supersede race was lost and including relevant playbook identifiers or context available in the surrounding flow.
🤖 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/playbook/playbook_edit_apply.py`:
- Around line 73-87: Add a debug or info log in the _LostSupersedeRaceError
handler before resetting new_playbook.user_playbook_id and returning -1,
recording that the supersede race was lost and including relevant playbook
identifiers or context available in the surrounding flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9f98fe9-f824-46a2-b5a4-7211f81c67e4
📒 Files selected for processing (1)
reflexio/server/services/playbook/playbook_edit_apply.py
#385 deleted `tests/server/services/playbook/test_playbook_edit_apply.py` alongside the module it covers. Restoring only the source left `apply_playbook_edit` with no direct unit coverage — its behaviour was exercised only indirectly, through `review_service`. Restored verbatim from 4268ad1^; all 8 tests pass unmodified against current main, which independently confirms the supersede CAS semantics the restored module relies on are unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/server/services/playbook/test_playbook_edit_apply.py (2)
89-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test with stale docstring referencing a non-existent parameter.
test_apply_expect_current_false_archivesis byte-for-byte identical totest_apply_inserts_new_and_archives_incumbent(Lines 31-58), yet its name and docstring reference anexpect_current=Falseparameter. Per the restoredapply_playbook_editsignature (storage, *, incumbent_id, new_playbook, source, request_id, skip_embedding=False, revise_context=None), no such parameter exists. This is dead-weight coverage and could mislead future readers into thinking such a parameter is part of the API.♻️ Suggested cleanup
-def test_apply_expect_current_false_archives(): - """With expect_current=False, new playbook is inserted and incumbent is archived.""" - from reflexio.server.services.playbook.playbook_edit_apply import ( - apply_playbook_edit, - ) - - with tempfile.TemporaryDirectory() as tmp: - s = _storage(tmp) - with patch.object(SQLiteStorage, "_get_embedding", return_value=[0.0] * 512): - old = _playbook(content="old") - s.save_user_playbooks([old]) - old_id = old.user_playbook_id - assert old_id > 0 - - new = _playbook(content="new") - new_id = apply_playbook_edit( - s, - incumbent_id=old_id, - new_playbook=new, - source="offline_optimizer", - request_id="run-abc", - ) - assert new_id > 0 - - # Only new_id should be CURRENT (status=None); old_id should be archived - all_pbs = s.get_user_playbooks(user_id="u1") - current_ids = {p.user_playbook_id for p in all_pbs if p.status is None} - assert new_id in current_ids - assert old_id not in current_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 `@tests/server/services/playbook/test_playbook_edit_apply.py` around lines 89 - 117, Remove the duplicate test_apply_expect_current_false_archives test, including its stale docstring and assertions, and retain the existing test_apply_inserts_new_and_archives_incumbent coverage. Do not introduce or reference a nonexistent expect_current parameter.
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated embedding-patch pattern into a fixture.
patch.object(SQLiteStorage, "_get_embedding", return_value=[0.0] * 512)is repeated identically 8 times across this file (once inside_storage()and once in every test body). Apytest.fixtureyielding a patchedSQLiteStorageinstance would remove this duplication and reduce future maintenance surface.♻️ Example fixture
`@pytest.fixture` def storage(tmp_path): with patch.object(SQLiteStorage, "_get_embedding", return_value=[0.0] * 512): yield SQLiteStorage(org_id="org_apply_1", db_path=f"{tmp_path}/t.db")Also applies to: 38-38, 68-68, 97-97, 132-132, 176-176, 211-211, 238-238
🤖 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/playbook/test_playbook_edit_apply.py` around lines 15 - 18, Extract the repeated SQLiteStorage embedding patch into a pytest fixture, replacing the local patch blocks in _storage and all affected test bodies. Have the fixture yield a patched SQLiteStorage instance using the existing temporary-path and org configuration, then update tests to reuse that fixture.
🤖 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 `@tests/server/services/playbook/test_playbook_edit_apply.py`:
- Around line 89-117: Remove the duplicate
test_apply_expect_current_false_archives test, including its stale docstring and
assertions, and retain the existing
test_apply_inserts_new_and_archives_incumbent coverage. Do not introduce or
reference a nonexistent expect_current parameter.
- Around line 15-18: Extract the repeated SQLiteStorage embedding patch into a
pytest fixture, replacing the local patch blocks in _storage and all affected
test bodies. Have the fixture yield a patched SQLiteStorage instance using the
existing temporary-path and org configuration, then update tests to reuse that
fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 260a73bf-29c2-445d-b921-332ebb6c24ae
📒 Files selected for processing (1)
tests/server/services/playbook/test_playbook_edit_apply.py
The restored test file carried two references to an `expect_current` parameter that `apply_playbook_edit` has never had: - `test_apply_expect_current_false_archives` was byte-identical to `test_apply_inserts_new_and_archives_incumbent` (both bodies 1009 chars), so it added no coverage while implying an API that does not exist. Removed. - `test_apply_expect_current_false_returns_minus1_and_no_orphan` is a genuine lost-race test — it asserts the -1 return, the rollback, and the absence of an orphan lineage event — and only its name was stale. Renamed to `test_apply_returns_minus1_and_leaves_no_orphan_on_lost_race`; body and docstring are unchanged. `grep -rn expect_current reflexio/` returns nothing, confirming the parameter is not part of any current signature.
1 participant
Problem
maincannot boot the API.PR #385 (
4268ad1) deletedreflexio/server/services/playbook/playbook_edit_apply.pyunder the changelog line "Remove the obsolete split-edit application path." That module
is not the split-edit path — the split/differentiate path lives in
playbook_generation_service.py.playbook_edit_apply.pyis the shared atomic supersedeprimitive, and its consumer survived the deletion:
apply_playbook_editexists nowhere else onmain, so no replacement was shipped.review_service.pyis imported byserver/routes/playbooks.py, which loads at appstartup — a guaranteed
ModuleNotFoundErroron boot.How it surfaced
A production deploy built from a clean checkout of
main. The task started,migrations ran clean, then it failed container health checks on the API port and the ECS
deployment circuit breaker rolled the service back. Logs showed 200×:
Every earlier build came from a working tree that still had the file on disk, so the
image contained a file
origin/mainno longer has. A fresh checkout is the only buildthat exposes it.
Fix
Restore the file verbatim from
4268ad1^. Every dependency it uses still exists onmain:supersede_record,commit_scope,save_user_playbooks,precompute_user_playbook_embeddings, andLineageContext.This is deliberately the minimal fix. If the intent was genuinely to retire this
primitive, the follow-up is to port
review_service.pyonto the publication API — butthat is a behavioural change and does not belong in a build-unbreaking commit.
Verification
Import, reproducing the exact production failure and its resolution:
import reflexio.server.routes.playbooksorigin/mainModuleNotFoundError: ...playbook_edit_applyTests (
tests/server/services/playbook):origin/mainThe 8 failures are all in
test_playbook_extractor.pyand are pre-existing — theyfail identically on pristine
mainwhen run standalone (6 in that file alone), and areuntouched by this change, which only adds a file.
ruff checkandruff format --checkpass on the restored file.Note
CI did not catch a change that makes the app unbootable and breaks a whole test
directory's collection. That gap is worth a separate look.
Summary by CodeRabbit