refactor(restore): one latch validator, one path normalizer, one exclusion site#107
Conversation
…usion site The patch-restore seam grew four copies of the same maybe-relative→absolute join and split its latch preconditions across runs.rearm_escalation (sentinel wedge, spec-less escalation) and cli._resolve_restore_patch (worktree isolation), so a programmatic re-arm could latch a patch the re-drive can never honor. - runs.validate_restore_latch: the single precondition seam, called by both entry points. rearm_escalation now rejects a worktree-isolation restore too; the CLI still passes the live policy's isolation mode, the one input run state cannot carry. - verify.resolve_restore_path: replaces the copies in engine._restore_patch, verify.verify_dev_exclude_relpaths, cli._resolve_restore_patch and runs._stale_restore_residue. The CLI keeps its own .resolve() for the containment check. - engine._restore_patch: the DEV_RUNNING phase guard was vacuous at its sole call site; unconditional advance + call-site-invariant comment. - _verify_shared_gates: proof_exclude -> extra_exclude, with the restore-patch exclusion derived from the task the gate already receives. None still skips the gate for the plan-halt leg; #88's baseline_untracked argument unchanged. Test hygiene: the three _escalated_run copies collapse into one parameterized conftest builder (superset, not lowest-common-denominator, so no fixture's assertions are weakened); the E2E scaffolds loop install.DEV_BASE_SKILLS; conftest's dev_effect absorbs _restoring_dev_effect via a seen-hook + skip-write flag; make_engine seeds max_stories/story_filter/epic_filter on RunState like cmd_run does, so resume tests stop silently running uncapped. Closes #91, Closes #84
|
Warning Review limit reached
Next review available in: 2 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughRestore-patch validation logic is consolidated into shared helpers ( ChangesRestore-Patch Latch Validation Consolidation
Test Fixture Consolidation and Scope Persistence
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
tests/test_stories_engine.py (1)
1037-1056: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid regression test for scope persistence.
The test correctly validates the round-trip through
save_state/load_stateand the resumed engine's internal filter derivation. One minor gap:resumed.state.story_filterandresumed.state.epic_filterare only checked on the reloaded state (lines 1048–1049), not on the resumed engine's state. Adding those assertions would confirmresume_enginedoesn't mutate scope fields during construction.Optional: add resumed-state assertions for story_filter and epic_filter
resumed, _ = resume_engine(project, engine, []) # no manual seeding anywhere assert resumed.state.max_stories == 2 + assert resumed.state.story_filter == "1" + assert resumed.state.epic_filter == 7 # StoriesEngine parks `--story` in its own id filter (a flat list has no E-S refs) assert resumed._story_id_filter == "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 `@tests/test_stories_engine.py` around lines 1037 - 1056, The resume regression test in test_make_engine_persists_the_launching_scope_for_resume only checks story_filter and epic_filter on the reloaded state, not on the resumed engine itself. Update the test to also assert resumed.state.story_filter and resumed.state.epic_filter after calling resume_engine, using the existing make_engine, load_state, and resume_engine symbols to verify those scope fields are preserved through construction.
🤖 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/test_stories_engine.py`:
- Around line 1037-1056: The resume regression test in
test_make_engine_persists_the_launching_scope_for_resume only checks
story_filter and epic_filter on the reloaded state, not on the resumed engine
itself. Update the test to also assert resumed.state.story_filter and
resumed.state.epic_filter after calling resume_engine, using the existing
make_engine, load_state, and resume_engine symbols to verify those scope fields
are preserved through construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 120f3416-ef33-44d4-ba99-d21e789a1062
📒 Files selected for processing (13)
CHANGELOG.mdsrc/bmad_loop/cli.pysrc/bmad_loop/engine.pysrc/bmad_loop/runs.pysrc/bmad_loop/verify.pytests/conftest.pytests/test_cli.pytests/test_engine.pytests/test_resolve.pytests/test_runs.pytests/test_stories_e2e.pytests/test_stories_engine.pytests/test_verify.py
…eloaded state StoriesEngine nulls the story_filter/epic_filter constructor kwargs (a flat list has no E-S refs), so assert construction leaves RunState's copies intact — the base Engine parks the filters on itself, never back onto state.
|
Thanks — applied in 0130b18. The suggestion was worth more than a nitpick: Both suggested assertions added, plus a comment naming the invariant. Full suite still 1950 passed / 1 skipped; |
build_context's flag was a residual partial copy of the precondition set — worktree-only — so a spec-less or sentinel-wedged escalation told the agent a restore was available, and the recorded restore_patch would only be rejected AFTER the interactive session (the exact waste the pre-session validation leg exists to prevent). Derive it from validate_restore_latch instead, extend the signal test with the two missing legs, and generalize the skill doc's false-reason wording.
1 participant
Closes #91, Closes #84
The last of the four PRs bundling the open issues from the PR #86 final review (after #103, #105, #106). Both issues here are non-bug cleanups that review raised: #91 is the restore-seam duplication it flagged across
runs/cli/engine/verify, #84 is the test-fidelity gap found while writing PR #77's A5 regression test. Behavior-neutral by intent, with one deliberate exception noted below.#91(a) — one latch validator
runs.validate_restore_latchnow holds every patch-restore precondition: the sentinel wedge and spec-less escalation (previously inrearm_escalation) and the worktree-isolation rejection (previously only incli._resolve_restore_patch).The one intentional behavior change:
rearm_escalationnow rejects a worktree-isolation restore as well. That split was the actual bug in #91 — any non-CLI caller (TUI restore parity, a script) could latch a patch the re-drive can never honor, because it discards the unit's worktree and re-mounts a fresh one. The CLI still passesworktree_isolationfrom the live policy, the one input run state cannot carry, so a policy edit between escalation and resolve can't skew the guard.Path resolution and
spec_within_rootscontainment stay CLI-side — they need--projectand the loaded bmad config.#91(b) — one path normalizer
verify.resolve_restore_path(raw, root)replaces the four copies of the maybe-relative→absolute join (engine._restore_patchvsworkspace.root,verify.verify_dev_exclude_relpathsvspaths.project,cli._resolve_restore_patchvsproject, and the fourth #90 added inruns._stale_restore_residue). Each consumer resolves against the base it actually reads the tree from, sorootstays caller-supplied. The relative branch is kept —model.StoryTask.restore_patchpromises repo-relative-or-absolute — and documented as CLI-latched-absolute in practice. The CLI keeps its own.resolve()on top, since it is the one consumer feeding a containment check.#91(c) — vacuous phase guard
engine._restore_patch'sif task.phase == Phase.DEV_RUNNINGwas always true at its sole call site (_dev_phaseadvances toDEV_RUNNINGimmediately before dispatch), and the comment's PENDING case couldn't survive_escalateanyway. Replaced with an unconditional advance + call-site-invariant comment.#91(d) — exclusion derived at the gate
_verify_shared_gates'proof_excludebecomesextra_exclude: the gate now derivesverify_dev_exclude_relpaths(paths, spec_path, task.restore_patch)from thetaskit already receives, and each mode passes only what it adds —()for sprint and bundle, the story record + manifest for stories. Threading the restore patch in from three call sites left a default-Nonefoot-gun for a future fourth mode, which would silently let a restore re-drive pass proof-of-work on the patch file's mere presence.extra_exclude=Nonestill skips the gate outright for the plan-halt leg, and #88'sbaseline_untracked=task.baseline_untrackedargument is preserved verbatim.#91(e) — test dedup
The three
_escalated_runcopies were not near-identical (different defaults, different return tuples, one unique kwarg each), so conftest gets a parameterized superset builder returning anEscalatedRunrecord; each module keeps a thin adapter for its own shape. Every field a caller relied on is still reachable — nothing is weakened down to a lowest common denominator. The E2E scaffolds loopinstall.DEV_BASE_SKILLS(and theSTORIES_PROBE_*constants) instead of hand-rolling the skill list three times, so a newly required skill fails them loudly.conftest.dev_effectabsorbstest_engine._restoring_dev_effectvia aseenhook +write_src=Falseflag.#84 —
make_enginepersists the launching scopetests/test_stories_engine.make_enginenow seedsmax_stories/story_filter/epic_filteron theRunStateit builds, mirroringcli.cmd_run.resumerebuilds the cap and filters from run state alone, so the old helper made everyresume_enginetest silently run uncapped unless the author rememberedengine.state.max_stories = N. The workaround intest_max_stories_survives_a_checkpoint_pause_resumeis dropped and a round-trip test pins the scope surviving unseeded.Verification
.venv/bin/python -m pytest— 1950 passed, 1 skipped (5 new tests;mainwas 1945 + 1).trunk check(no filter) — clean.test_rearm_rejects_restore_patch_without_a_spec_file's tail assertions; restored and re-verified.New coverage: the stories gate excludes a latched restore patch (guards the #91(d) derive),
resolve_restore_pathjoin/no-normalize semantics,rearm_escalationrejecting a worktree-executed task's latch,validate_restore_latch's clean-pass + live-policy legs, and the #84 resume round-trip.Summary by CodeRabbit
Bug Fixes
Documentation