feat(operator): park-record-missing check id + fresh-clone E2E (#356) - #362
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThe change defines ChangesPark-record confirmation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR gives the board-orphan validate arm its own check id (
Confidence Score: 5/5Safe to merge — all changes are narrowly scoped to diagnostic reporting, terminology, and test coverage, with no changes to the confirmation or park execution paths. The check-id split is a purely additive diagnostic change (warnings only, never gates a run), the string sweep is consistent throughout the codebase, and both new tests directly exercise the invariants they describe. The ablation notes in the test docstrings confirm the failure modes were verified manually. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/checks.py | Adds operator.park-record-missing to VALIDATE_CHECKS; one-line, correctly positioned alongside the other operator checks. |
| src/bmad_loop/cli.py | Splits the board-orphan arm from operator.registry-stale to operator.park-record-missing, updates the _validate_operator_registry docstring to remove the machine-local claim, and sweeps ten user-facing strings from "index" to "park entry". All changes are internally consistent. |
| src/bmad_loop/operatoractions.py | Module docstring updated to enumerate all four validate check IDs; correctly reflects the split at operator.park-record-missing. |
| tests/test_cli.py | Test renamed and assertions updated to verify operator.park-record-missing including severity, message text, detail payload, and absence of operator.registry-stale in the same finding set. |
| tests/test_engine_worktree.py | Parametrizes existing park-merge test with both "merge" and "ff" strategies (ff is the #356 regression guard), and adds test_a_parked_story_confirms_on_a_fresh_clone — a complete E2E test covering the full park → clone → confirm flow. |
| CHANGELOG.md | Adds operator.park-record-missing entry with all four causes (including the "pull it" recovery addressed in d255a73), and refreshes two "index" references to "park entry". |
| docs/FEATURES.md | Rewrites the park-completion and resumability bullets to the committed-record contract, removing the machine-local index language and adding operator.park-record-missing with all four causes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[validate: board story at awaiting-operator] --> B{park record\nexists?}
B -- yes --> C{story.resumable?}
C -- yes --> D[operator.confirm-interrupted\nre-run confirm to finish]
C -- no --> E{drift from\ncommitted truth?}
E -- yes --> F[operator.registry-stale\ndiscard stale entry]
E -- no --> G[operator.actions-malformed?\nno readable actions]
G -- yes --> H[operator.actions-malformed\nrepair spec list]
G -- no --> I[clean — no warning]
B -- no --> J[operator.park-record-missing NEW\nfailed write / pre-upgrade park /\nmissing branch pull / deleted record]
Reviews (5): Last reviewed commit: "docs(operator): list every operator chec..." | Re-trigger Greptile
2ab71bc to
be4a2cf
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d255a73ace
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"operator-actions index has no entry for them — the index is machine-local " | ||
| f"and not committed, so `bmad-loop confirm` cannot complete them from here; " | ||
| f"confirm them on the machine that ran the story, or finish them by hand", | ||
| "operator.park-record-missing", |
There was a problem hiding this comment.
When validate --json encounters an awaiting-operator board entry with no matching park entry, this replaces the previously emitted operator.registry-stale identity while VALIDATE_SCHEMA_VERSION remains 1. Consumers are explicitly instructed to branch on check as the stable identity, so existing schema-v1 automation will silently stop recognizing this condition; publish the new identity behind a schema-version bump or retain a compatible v1 representation.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one — the premise doesn't hold. Worth checking, though, since it names a hard invariant in AGENTS.md.
operator.registry-stale has never shipped, so there is no schema-v1 automation to break.
git tag --contains c828475(the commit that introduced the id) → empty. It is contained in zero tags.git show v0.9.0:src/bmad_loop/checks.py | grep -c operator→0. The last release ships nooperator.*check id at all.- Every operator entry in the CHANGELOG sits under
## [Unreleased], and__version__is still0.9.0, the same as the latest tag — nothing has been released since.
The id was born 2026-07-28, seven days after the v0.9.0 tag. The whole awaiting-operator feature (#335, #356) postdates that tag, so no released binary has ever emitted operator.registry-stale.
Three further reasons the suggested remedy would be wrong even setting that aside:
-
Nothing was renamed or replaced.
operator.registry-staleis still registered (checks.py:78) and still emitted (cli.py:693), with its meaning unchanged. This PR factors one of its two arms out into a new id — net effect is one id added and one narrowed, not an identity replaced. -
The repo's documented rule points the other way. From an earlier entry,
CHANGELOG.md:843-851: "the schema version is deliberately unchanged, since the document contracts eachcheckid, not a given check's outcome."machine.py:7-10anddocs/FEATURES.md:213scope the version to the envelope and field shape, additive-only. -
Precedent is unanimous. Check ids have been added in 7 separate commits, none with a version bump; there have been 0 renames and 0 removals in the repo's history. Twice the project deliberately minted a new id rather than repurpose an existing one —
ecc98e3(operator.confirm-interrupted) and215cb99(this one) — under the codified rule "check ids split where the remedy does" (docs/FEATURES.md:67). The only schema bump ever made (ed9f52f, probe 1→2) was a field removal plus addition.
Bumping VALIDATE_SCHEMA_VERSION would advertise a breaking change that did not occur, and would drag the TUI tripwire (tui/widgets.py:341) and its pins (tests/test_cli.py:3841, tests/test_tui_app.py:397) along with it.
The review did surface a real gap, just not this one: operatoractions.py still described validate as carrying only operator.registry-stale and operator.actions-malformed, omitting both operator.confirm-interrupted and the new id. Corrected in 76c8da6.
|
@greptileai review |
1 participant
Part 2 of 2 for #356, stacked on #361 (retargets to
mainwhen that merges).What
operator.park-record-missing— the validate board-orphan arm gets its own check id, registered inchecks.VALIDATE_CHECKS. Before the committed records this state was the fresh-clone norm and sharedoperator.registry-stale; now the record travels with the park's own commit, so a board atawaiting-operatorthat no record claims is always evidence of something — a failed record write (journaled asoperator-index-failed), a pre-upgrade park, a checkout without the park's branch, or a deleted record. The message names the causes and remedies, and the ids split exactly where the remedy differs (discard a stale entry vs recover the record)._validate_operator_registry's docstring stops claiming the index is machine-local.merge_strategy="ff"parametrization oftest_worktree_parked_unit_merges_like_a_done_one— the bmad-loop confirm only works on the machine that parked the story #356 acceptance regression guard: the committed record must never cost a fast-forward merge-back.test_a_parked_story_confirms_on_a_fresh_clone): park under worktree isolation →git clone→confirm --yesin the clone → rc 0, clone boarddone, confirm commit present with the record's deletion, clean tree.cli.pystill called the committed per-story store "the index" — theconfirmrefusal, the resume and three part-way-failure messages, two validate warnings, and_apply_confirmation's docstring. They now say "the park entry", the noun aParkedStorycan actually justify (it cannot tell a record from a legacy entry, so "record" would over-claim on a legacy park). Two Unreleased CHANGELOG lines that described the store in the present tense follow. Uncontracted prose throughout —documents.pydisclaims bothfindings[].messageandparked[].drift— so no schema bump, and the check ids are untouched.--jsoncommit-provenance wording); CHANGELOG extended with the new check id.Testing
trunk checkclean.operator.registry-staleagain → the moved test fails on the id lookupoperator.park-record-missingfromVALIDATE_CHECKS→ the registry assert fires ("unregistered check id") and the test fails_write_park_recordat the main project instead of the workspace → both merge-strategy legs fail on the ls-tree assertion (the record sits untracked at the target instead of riding the merge)_write_park_recordcall → the fresh-clone E2E fails: the clone's confirm refuses with the unknown-key messageSummary by CodeRabbit