feat(deferred): enforceable hard gates — gate: field + two validate checks - #502
feat(deferred): enforceable hard gates — gate: field + two validate checks#502pirony wants to merge 34 commits into
gate: field + two validate checks#502Conversation
An entry that must land before a story runs could only say so in prose; prose stopped nothing, so `run` drove the story and the gate surfaced in a diff built on the missing leg. `gate: 3-2, 3-3` names the blocked story keys. While the entry is open, validate fails (deferred.hard-gate) for every actionable story a token matches — equal to the key or its `-`-delimited prefix, so one token reaches both queue modes without sweeping in numeric neighbours. It is the only deferred check that gates rather than advises: the closes-* siblings describe traceability that may be wrong, this one describes work that must not start. A prose `HARD GATE:` with no `gate:` line, and a token that cannot name a story key, warn instead (deferred.hard-gate-unstructured) — a gate nothing can enforce. A ledger with no gate line stays silent.
Both queue modes, both severities, and the cases that decide whether the gate is trustworthy: the `3-2` / `3-20` boundary, a mid-line HARD GATE mention that must not warn, a gate line below a flat-append bullet that belongs to the block and not the entry, and a gate-free ledger that adds no findings at all.
The format doc is what a sweep session reads before appending, so the field is specified there: comma-separated tokens, lines union, prefix matching, and what each of the two checks reports.
Three ways the first cut let a real gate go silent-inert. A split story kept no gate: breakdown can turn a gated 3-2 into 3-2a / 3-2b after the gate was written, and a `-`-only boundary dropped it at exactly that moment. One lowercase letter followed by `-` is now also a boundary. The `startswith` guard before the suffix slice is load-bearing — the tail alone would let 3-2 gate 9-9a-x. The prose detector was anchored to line start, but ledgers hard-wrap their `reason:`, so a real declaration lands mid-line. It now matches anywhere on a line and excludes a citation by the quote before it; the colon still excludes prose about a gate rather than a gate. An empty `gate:` line parsed identically to no gate line at all, so a claim made inertly said nothing. EntryGates counts its lines, and an `inert` declaration warns like a malformed token. Checked against a live ledger: two mid-line declarations previously missed are now reported, and the entry citing the phrase stays silent.
The three inert shapes are now one list, since one warning covers them and the remedy is the same line.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded ChangesDeferred-work story gates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunEngine
participant DeferredLedger
participant StoryDispatcher
RunEngine->>DeferredLedger: reread ledger before dispatch
DeferredLedger-->>RunEngine: return readable snapshot or read error
RunEngine->>RunEngine: match unfinished gate tokens
RunEngine-->>StoryDispatcher: pause before task creation or permit dispatch
StoryDispatcher-->>RunEngine: persist pause state or start story work
Possibly related issues
Possibly related PRs
Suggested reviewers: 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 |
Greptile SummaryThe PR adds structured deferred-work
Confidence Score: 4/5This PR should not merge until hard-gate enforcement is placed on the run and resume paths that actually dispatch stories. The parser and validation checks work on the standalone validation path, but normal and resumed runs can dispatch a story named by an open gate without executing those checks. Files Needing Attention: src/bmad_loop/cli.py
|
| Filename | Overview |
|---|---|
| src/bmad_loop/cli.py | Adds deferred-ledger validation and queue-specific gate checks, but wires them only into cmd_validate, leaving actual run and resume dispatch unenforced. |
| src/bmad_loop/deferredwork.py | Adds immutable gate classification, token parsing, and boundary-aware story matching with focused tests. |
| src/bmad_loop/checks.py | Registers the two new deferred hard-gate validation check identifiers. |
| tests/test_cli.py | Covers validation behavior comprehensively but does not test that run or resume refuses to dispatch a gated story. |
| tests/test_deferredwork.py | Covers multiline declarations, malformed and inert forms, canonical entry boundaries, matching boundaries, and prose detection. |
| src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md | Documents gate syntax and semantics, including the claim that open gates prevent stories from running, which the runtime path does not currently enforce. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Open deferred entry with gate token] --> B{Command invoked}
B -->|bmad-loop validate| C[_validate_deferred_ledger]
C --> D[_validate_hard_gates]
D --> E[Validation failure]
B -->|bmad-loop run or resume| F[Compose engine]
F --> G[Pick next actionable story]
G --> H[Run gated story]
D -. missing from run path .-> F
Prompt To Fix All With AI
### Issue 1
src/bmad_loop/cli.py:341-347
**Hard gates bypass story dispatch**
When a user starts or resumes a run without separately invoking `bmad-loop validate`, the dispatch path never calls `_validate_deferred_ledger` or `_validate_hard_gates`, causing the engine to run stories explicitly blocked by open `gate:` entries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs(deferred): describe the split, mid-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if paths: | ||
| if stories_on: | ||
| _validate_stories_queue(project, paths, spec_folder, dev_trees, report) | ||
| _validate_closes_deferred(paths, report, spec_folder=spec_folder) | ||
| _validate_deferred_ledger(paths, report, spec_folder=spec_folder) | ||
| else: | ||
| _validate_closes_deferred(paths, report) | ||
| _validate_deferred_ledger(paths, report) | ||
| _validate_operator_registry(project, paths, report) |
There was a problem hiding this comment.
When a user starts or resumes a run without separately invoking bmad-loop validate, the dispatch path never calls _validate_deferred_ledger or _validate_hard_gates, causing the engine to run stories explicitly blocked by open gate: entries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/bmad_loop/cli.py
Line: 341-347
Comment:
**Hard gates bypass story dispatch**
When a user starts or resumes a run without separately invoking `bmad-loop validate`, the dispatch path never calls `_validate_deferred_ledger` or `_validate_hard_gates`, causing the engine to run stories explicitly blocked by open `gate:` entries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/test_deferredwork.py`:
- Line 1631: Update the test string in the deferred-work test case to replace
each raw no-break space with the \u00a0 escape sequence, preserving the exact
input value while resolving RUF001.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 8f08ec8e-f306-44a1-812c-bffaca09ab5c
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.mdsrc/bmad_loop/deferredwork.pytests/test_cli.pytests/test_deferredwork.py
|
Triage note 2026-08-08: this arrived mid-way through a full-tracker triage, so slotting it into the queue rather than reviewing on the spot. Two sequencing facts you should know: (1) a deferredwork.py writers bundle is queued (#328/#327/#329/#469/#363 — atomic appends, next_seq from headings, surrogate scrub, file locking); your change is parse-side so overlap looks moderate, but whichever lands second rebases. (2) Repo invariant: every new check id must land in |
|
Retracting one item from my triage note above: I flagged that "every new check id must land in --- a/src/bmad_loop/checks.py
+++ b/src/bmad_loop/checks.py
@@ -90,6 +90,8 @@
"deferred.closes-entry-unreadable",
+ "deferred.hard-gate",
+ "deferred.hard-gate-unstructured",
"deferred.ledger-unreadable",Both ids land inside the Also: CI on this PR was sitting unapproved (first-contribution runs need a maintainer to release them), not failing. I've granted approval, so the suite is running now. Full review is still queued behind the current merge-track PRs, and the sequencing note about the |
|
@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: 5a9c48ce67
ℹ️ 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".
| {"ledger": str(ledger), "error": str(e)}, | ||
| ) | ||
| return | ||
| _validate_hard_gates(paths, text, report, spec_folder=spec_folder) |
There was a problem hiding this comment.
When an earlier story files an open entry that gates a later story during the same unattended run, this check is never revisited: _validate_hard_gates is called only by cmd_validate, while cmd_run, the engine's next-story dispatch, and resume do not call it. Thus even a run preceded by a successful validation can drive work that a newly written gate: says must not start. Re-read and enforce the ledger at the deterministic per-story dispatch boundary, not only in the standalone validation report.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
| for m in GATE_RE.finditer(entry.body): | ||
| lines += 1 |
There was a problem hiding this comment.
When an entry combines a valid declaration with an empty one, such as gate: 3-2 followed by gate:, the aggregate has tokens, so EntryGates.inert is false and deferred.hard-gate-unstructured is never emitted for the empty line. This silently loses exactly the malformed declaration the new warning promises to surface; track whether any individual matched line yielded no usable item rather than deriving inertness from the entry-wide token set, and cover the mixed-line case at the parser layer.
AGENTS.md reference: AGENTS.md:L62-L62
Useful? React with 👍 / 👎.
Review of bmad-code-org#502. Six fixes, each with a repro that reddens without it: - `gates_story` applied the split-story arm to any token, so `gate: auth` hard-FAILED `authz-login` — a refusal against a story nobody gated, which `stories.ID_RE` makes reachable. The arm now needs the token to end at a story number, where `STORY_RE` puts the split letter. Also drops the accidental `gates_story("", key)` match. - An empty `gate:` line beside a valid one was never reported: `inert` is an entry-wide verdict and answers False once the entry has a token. Count the inert lines instead, and report every cause rather than the first. - `_actionable_story_keys` left the per-story `resolve_story_spec` outside its guard, turning a degraded check into a traceback out of `validate`; and it returned `[]` for an unreadable queue, which the caller read as "nothing is gated" and answered `ok` — naming the entry it claimed was clear. It returns None for that now, and the queue is only read once some entry gates. - The prose lookbehind missed the backtick and curly quotes, so an entry citing `HARD GATE:` in markdown warned about itself. - The unstructured warning promised `gate:` stops `bmad-loop run`. It does not: enforcement is preflight-only, and the docstring now says so. - `_validate_deferred_ledger` justified its ordering with an early return that leaves a *sibling* function and never could have swallowed the gate. Pins the closed-entry warning skip, which an ablation showed unpinned.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Review — and thank you, this is a strong first contributionNo apology needed, and please don't downscope: the PR is coherent as one change and I'd rather not split it. The parser half and the validate half only make sense together — a The design instinct here is the right one, and the part I want to call out specifically is that you reached for "a token nothing can match is the same silent no-op the field exists to end" and then built I've taken the branch over rather than sending you a list. It's rebased onto post-#485 What I verified as correct
Findings I fixed1. 2. 3. 4. 5. 6. Two docstrings that described mechanisms that don't exist. 7. Warning text promised enforcement that doesn't exist — see the scope note below. Docs: trimmed the CHANGELOG entry (13 lines / 197 words against a 6-line / 82-word median for the section), lowercased the one all-caps Findings I deliberately left for the follow-onNot defects to fix under you — decisions I'd rather make with the dispatch work in front of us:
NBSP at
|
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/test_cli.py`:
- Around line 4146-4160: Add the confusable story key named in the test
docstring, such as authz-login, to the queued stories passed to
_validate_gated_sprint, while retaining the existing gated story fixture. Ensure
the assertions then cover the split-letter path in gates_story and fail if its
digit guard is removed.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 82eb35cf-0444-41f3-aabf-5c4466987950
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.mdsrc/bmad_loop/deferredwork.pytests/test_cli.pytests/test_deferredwork.py
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- README.md
- src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md
- src/bmad_loop/checks.py
- docs/FEATURES.md
- src/bmad_loop/cli.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3a124a83a
ℹ️ 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".
| # A story key as either queue spells one: a sprint key (`3-2-invite-link`), the | ||
| # stories-mode id it starts with (`3-2`), or a bare slug. Whitespace and | ||
| # separators are deliberately out — a token nothing can match is the same silent | ||
| # no-op the field exists to end, so it is surfaced rather than dropped. |
There was a problem hiding this comment.
Tokens such as 3.2 and 3_2 pass this regex and are therefore treated as enforceable, but they cannot match any queue key: stories-mode IDs reject ./_, while sprint keys parsed by STORY_RE require the 3-2-... form. An open entry using gate: 3.2 consequently produces neither deferred.hard-gate-unstructured nor a gate failure and can even receive the false all-clear, silently defeating the boundary check; validate tokens against the actual queue-key shapes or normalize supported selector syntax before accepting them.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, but the premise is too broad to act on as stated, so I am tracking it rather than tightening the regex here.
. and _ are not universally unmatchable. sprintstatus.py:23 STORY_RE is ^(\d+)-(\d+)([a-z]?)-(.+)$ — the slug is unconstrained, so both of these are legal sprint keys and gate correctly today:
STORY_RE("3-2-foo.bar") -> True gates_story("3-2-foo.bar", "3-2-foo.bar") -> True
STORY_RE("3-2-a_b") -> True gates_story("3-2", "3-2-foo.bar") -> True
Rejecting ./_ in GATE_TOKEN_RE would therefore break legitimate gates on dotted or underscored slugs. The genuinely unmatchable case is narrower than the character class: a ./_ in the number prefix (3.2, 3_2), which neither key grammar can produce.
The correct fix is the one you name second — validate the token against the actual key shapes — and that is a semantics decision rather than a regex tweak, because a token legitimately may name a story not yet on the board (test_validate_unions_multiple_gate_lines gates 9-9 on no board deliberately, so "matched nothing" cannot become a failure). It is on the list for the follow-on session that wires enforcement into dispatch, together with two adjacent gaps: GATE_RE is strictly ^gate:, so Gate: or an indented gate: produces zero findings — failing open, where a missing status: fails closed — and a bare 3-2a stories-mode id is not gated by gate: 3-2.
Not merging until that lands, so nothing here ships silently.
The CLI half of the auth/authz-login regression was vacuous: the fixture queued only 3-2-invite-link, so gates_story returned False at the startswith check long before the split-letter arm, and the test passed with the digit guard ablated. The pure-core parametrize was carrying it. Moved to stories mode, the only queue that can express the case -- authz-login does not match sprintstatus.STORY_RE, so a sprint board drops it before ss.stories and the same fixture stays vacuous there.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75508028be
ℹ️ 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".
| # the ledger, so returning quietly reported success for preflights that | ||
| # checked nothing, against the very file the run's closure will fail on | ||
| # (#284 round-5 review, finding 6). | ||
| report.warn( |
There was a problem hiding this comment.
When deferred-work.md exists but raises OSError or UnicodeDecodeError, this warning leaves ValidationReport.passed true even though _validate_hard_gates never runs. A preflight can therefore return success while an unreadable entry contains an open gate for an actionable story—the safety check this change introduces must fail closed here, rather than retaining the advisory severity that was sufficient when this read served only closes_deferred. The message should also identify that hard gates were not checked.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the diagnosis, and I have taken half of it in 75f1f43.
Taken: the message. It named only closes_deferred, which reads as though the refusal had run and found nothing. It now says both were unchecked and that an open entry could be gating an actionable story unseen. Pinned by an assertion in test_validate_warns_when_the_ledger_itself_is_unreadable and ablation-checked — reverting the wording reddens it.
Deferred, deliberately: the severity. You are right that this is a fail-open — the same bytes now back the one deferred check that is a refusal, so validate can exit 0 having evaluated no gate. But deferred.ledger-unreadable is a pre-existing check id that ships as a warning on main, and flipping it to a problem takes every project with an unreadable ledger from exit 0 to exit 1 — a user-visible behaviour change orthogonal to adding a parser and two checks.
It belongs with the dispatch-side enforcement work, which is where fail-closed becomes coherent end-to-end: today bmad-loop run never reads the ledger at all (engine.py:797 Engine._pick_next reads only the board), so making validate fail closed while run stays unguarded hardens the door and leaves the window open. Both land together, before merge. The fail-open is recorded in a comment at the call site so it is not rediscovered as a surprise.
The same bytes now back a refusal, not just the advisory closes_deferred checks, so a message naming only closes_deferred reads as though the gate had run and found nothing. Says what actually went unchecked. Severity left as a warning deliberately: escalating a pre-existing check id to a problem is a user-visible behaviour change, and it belongs with the dispatch-side enforcement rather than this parse-and-check addition. The fail-open is recorded at the call site.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75f1f433da
ℹ️ 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".
| if not entry.open: | ||
| continue |
There was a problem hiding this comment.
When a gate-bearing entry has a missing or misspelled status such as status: opne, entry.open is false, so this loop skips the entry and later emits deferred.hard-gate as ok if no other open gate matches. _validate_closes_deferred only diagnoses that malformed status when a story separately declares the same ID in closes_deferred, so the ordinary case silently allows the gated actionable story; treat a gate-bearing status that is neither open nor done as unsafe rather than as closed.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
…opens The `gate:` field refused only at `bmad-loop validate`. `Engine._pick_next` reads the board alone, so a `run` that skipped the preflight drove a gated story anyway — the refusal was only as strong as an operator's habit. - `Engine._refuse_gated_story` pauses the run (the reserved `story-gate` stage, already rendered by the TUI) rather than dispatch a story an unlanded entry gates. Called before the story is recorded in `state.tasks`, so a resume re-picks it and re-reads the ledger: closing the entry and resuming runs it. Registering the task first would fire the gate once and then retire the story for the rest of the run and every resume of it. - The sweep stays exempt (it overrides `_loop`), now with a test: a sweep is the only automated closer of the gating entry, so gating it deadlocks the gate against its own remedy. - `status: opne` disabled the gate AND produced a green `ok` naming the entry as clear: `entry.open` is False for a status the format cannot read, so the entry was skipped entirely. Status is a tri-state now — only an explicit `done` retires a gate. - `deferred.ledger-unreadable` becomes a problem. It exited 0 with the gate never evaluated, and the question "does this project use gates?" is answerable only from the file that will not open. Dispatch refuses the same fault. - `gate: 3.2` was a shape-valid token nothing can match, reported as a green `ok`. Matchability is now tested against the two key shapes rather than by banning `.`/`_`, which are legal inside a sprint slug (`gate: 3-2-a_b` still gates). - `Gate:` and an indented ` gate:` yielded zero findings. They warn now, rather than being accepted: reading an indented line as a declaration would turn a fenced example inside an entry into a refusal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 416bbaf88e
ℹ️ 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".
| for entry in stories_mod.load_stories(folder).entries: | ||
| state = stories_mod.resolve_story_spec(folder, entry.id) | ||
| if stories_mod._classify(state) != "actionable": | ||
| continue | ||
| keys.append(entry.id) |
There was a problem hiding this comment.
When stories.yaml contains a blocked, sentinel, ambiguous, or unknown-status entry before a later pending/resumable story, the new _classify check skips the wedged entry and adds the later story as actionable. However, stories.schedule() returns SCHEDULE_WEDGED immediately at that earlier entry and cannot dispatch anything after it, so a matching gate: incorrectly makes validate fail for unreachable work. Stop collecting keys at the first wedged state, mirroring the scheduler's strict left-to-right scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Your premise is correct and I had it wrong in the docstring; I'm not taking the remedy. bae8f1f fixes the claim and pins the behaviour.
Verified, both halves. Driving stories.schedule() directly over a manifest of [1=blocked, 5=pending]:
sequential scan -> wedged at 1
--story 5 scan -> next at 5
So you are right that the sequential scan cannot reach story 5 — schedule() returns SCHEDULE_WEDGED at stories.py:438 rather than continuing. My docstring even said the wedged entry "STOPS the scan" while the code beside it wrote continue, and closed with "sharing the scheduler's predicate is what keeps preflight and dispatch answering alike" — a parity claim the code does not keep. That sentence is what makes break look like the obvious repair, and it was mine.
Why the behaviour stays continue. The second line above is the part the finding doesn't account for. run --story <id> scans that entry alone (cli.py:1527 → stories_engine.py:173 → selector), so a story behind a wedge is dispatchable right now, while the wedge stands. cmd_validate takes no story selector at all, so preflight cannot know which of the two runs is coming — and of the two ways to be wrong, only one is quiet.
The other cost is scale. Stopping at the first wedged entry doesn't drop one story, it drops every story below it. One blocked entry near the top of a manifest would silence deferred.hard-gate for the whole rest of the queue — a gate lost in silence, which is the exact failure this field was added to end, arriving by a quieter route than the one 24a089e fixed. Over-reporting a real gate on a story that first needs an unrelated resolution is the cheaper wrong answer, and dispatch refuses independently either way (_refuse_gated_story pauses on PAUSE_STORY_GATE), so nothing rides on preflight alone.
Worth being precise about what 24a089e actually shared, since that is where the confusion started: stories._classify, the per-entry predicate — deliberately not schedule()'s stop rule. A wedge is a property of some other story; the gated story's own state is what decides whether a gate against it is meaningful.
What changed in bae8f1f — no behaviour change:
- the docstring now says which half is shared and why the other half is not, so the next reader isn't invited to the same repair;
- summary line
would dispatch→could dispatch; tests/test_cli.py:4388pins it: manifest[1=blocked, 2=pending],gate: 2, still aproblem.
That test is there specifically because this decision was unpinned. Ablating exactly your proposal — stop at the first wedged, keep skipping done — turns it red and nothing else in the suite moves (1 failed, 1340 passed), which is what convinced me the direction was worth writing down rather than left to whoever reads continue next.
Gates on bae8f1f: pytest 4861 passed / 36 skipped / 5 xfailed, pyright 0 errors, trunk check clean.
Good finding — the docstring was making a promise the code didn't keep, and that was worth catching even though the code was the half I want to keep.
`_actionable_story_keys` shares `stories._classify`, the per-entry
predicate — not `schedule()`'s stop rule, which gives up at the first
wedged entry. The docstring claimed the broader parity ("keeps preflight
and dispatch answering alike"), which reads as a promise the code does
not keep and invites narrowing this to a `break`.
Narrowing it would be wrong twice: `run --story <id>` scans that entry
alone, so a story behind a wedge is reachable right now while `validate`
takes no selector and cannot know which run is coming; and stopping
would let one blocked entry near the top of a manifest silence the gate
check for everything below it. Pin the decision with a test so it is not
optimized away later.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/bmad_loop/deferredwork.py`:
- Around line 320-346: The fence check in _quoted currently uses only the entry
body, so it cannot account for fences opened before the entry span. Pass the
whole-file ledger fence index into the gate scans, including gates() and
declares_prose_gate(), and query fence state with entry.span[0] + m.start() for
each match; keep a single shared fence rule and preserve the existing
unclosed-fence behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 5f4ccbc9-5c20-4e43-81d0-51a8b2c56b8d
📒 Files selected for processing (8)
docs/tui-guide.mdsrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.mdsrc/bmad_loop/deferredwork.pysrc/bmad_loop/devcontract.pysrc/bmad_loop/fences.pytests/test_cli.pytests/test_deferredwork.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/bmad_loop/devcontract.py
- docs/tui-guide.md
- src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md
- src/bmad_loop/fences.py
- src/bmad_loop/cli.py
- tests/test_cli.py
- tests/test_deferredwork.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae8f1f3c4
ℹ️ 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".
| Like `source_spec:`, a `gate:` line is never edited or dropped when an entry is | ||
| otherwise touched: removing it un-gates the story silently, which is the exact | ||
| failure this field exists to prevent. |
There was a problem hiding this comment.
When a sweep migrates a mixed legacy/canonical ledger, _ensure_migration snapshots each pre-existing canonical entry only as {id: status}, and validate_migration checks only that the ID remains and its status is unchanged. The migration session can therefore remove a pre-existing gate: line, pass deterministic validation, and commit the rewrite; the next dispatch then runs the story that entry was meant to protect despite this new byte-preservation guarantee. Include the original gate declarations or full canonical entry bodies in migration validation so dropping one rejects the repair.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — filed as #519 rather than fixed here, because the fix belongs in sweep.py, which is not in this PR's diff.
Verified rather than accepted on reading. Driving validate_migration with a rewrite whose only change is a deleted gate: line:
pre_canonical snapshot : {'DW-1': 'open'}
DW-1 gate tokens BEFORE: ('3-2',)
DW-1 gate tokens AFTER : ()
validate_migration errors: []
Accepted, exactly as you said. _ensure_migration snapshots {e.id: e.status} (sweep.py:717) and the pre-existing-entry check compares the status first-word only (sweep.py:279-284); nothing there reads gate: — the other gate hits in that file are the unrelated gates notification module.
You are also right about which guarantee it defeats. deferred-work-format.md says a gate: line "is never edited or dropped when an entry is otherwise touched" — but that is instruction to the session, not a check, and migration is precisely the path where an LLM rewrites entries it did not author. The one place the instruction most needs backing is the one place nothing backs it.
Not folding it into this PR for two reasons. sweep.py is untouched here, so the fix needs its own tests against the migration path rather than riding on this diff's. And the snapshot shape is a decision of its own — id → (status, gate tokens) is the minimal form, while preserving whole entry bodies would also refuse the reformatting a migration legitimately performs. #519 records both, plus the open question of whether origin:/location:/source_spec: deserve the same treatment, since source_spec: carries a comparable never-drop instruction and has no check either.
Good catch — this is the field's guarantee being defeated by a route the field's own guard doesn't cover.
`parse_ledger` reads headings and `status:` against the whole file because a body slice cannot see a fence opened above the heading. The gate scans asked the same question of `entry.body` alone, and the two views disagree: a stray unclosed ``` above the heading is ordinary text at file scope — so the entry is carved — while the body reads a later matched `~~~` pair as a real fence and masks a live `gate:` into an example. A gate lost in silence is what the field exists to end. Carry the whole-file index `parse_ledger` already builds on `DWEntry` (no default, like `status_span`) and query it at absolute offsets, so one fence rule answers for headings, `status:`, `gate:` and the prose scan alike. Found by differential fuzz against the whole-file predicate — 5 divergences over 26k ledgers, now 0 over 77k.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b79edeee5
ℹ️ 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".
| # Before ANY state mutation for this story, and deliberately so — see | ||
| # _refuse_gated_story. The story is not in state.tasks yet, so a resume | ||
| # re-picks it and re-asks the ledger. | ||
| self._refuse_gated_story(story.key) |
There was a problem hiding this comment.
Fresh evidence after the normal dispatch check was added: if the process dies after persisting the new StoryTask but before _run_story() starts, resume enters _finish_inflight() before this loop, and its Phase.PENDING restart arm calls _run_story() directly. If an open gate is added—or the ledger becomes unreadable—while the run is down, no gate check executes and the first session starts anyway. Re-run _refuse_gated_story() before restarting an untouched pending task; genuine mid-story recovery can remain exempt.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1728267. Good catch — this one was real, and the window is wider than "dies between two statements".
Repro before the fix. A StoryTask in exactly the shape _loop persists between engine.py:846 (_save()) and engine.py:847 (_run_story(task)), then a gate landing while the run is down: the resume ran the story to completion, done=1, sessions dispatched, ledger never consulted. _pick_next can't catch it either — the key is already in base_skip.
And it isn't only that two-statement gap. Nothing advances the task until _dev_phase bumps attempt in the same save as the DEV_RUNNING advance (engine.py:1595, engine.py:1620-1621), so the whole isolated worktree mount and the pre_story / pre_ready_gate hooks sit inside the window — including paths that can pause rather than crash. Not microseconds.
Predicate. attempt == 0 (engine.py:1239), for the reason above: it is bumped before the session is spawned, so 0 means no session ever started.
Two things I rejected on the way:
- Not
task.sessions. Tempting and wrong —record_sessionis called after a session returns (engine.py:4232), so a host death inside a session leaves no record. That task is genuinely in flight, with a live worktree, and gating it is exactly the stranding the exemption exists to prevent. rearm_escalationresetsattemptto 0 (runs.py:802) on a task that HAS run sessions, so the bare counter would read a human-armed re-drive as a first dispatch. Henceand not task.rearmed. Refusing there would be worse than a missed gate: the operator resolved the escalation to get that story moving, and the entry gating it may be the one the re-drive is about to close.
Placement is after the arm's unwinding and save (engine.py:1258), not before it. A refused task is then left in the same shape _loop hands to _run_story — no half-built worktree stranded behind a pause that may last days — and because _finish_inflight re-drives non-terminal tasks, the refusal re-asks on every later resume instead of retiring the story. Your "genuine mid-story recovery can remain exempt" is preserved: the line is drawn by session, not by the presence of a task record.
Three tests, each ablated whole-file against test_deferredwork / test_devcontract / test_cli / test_engine / test_sweep (1347 tests), all predicted before running:
| ablation | red |
|---|---|
| delete the gate call | test_resume_re_gates_a_story_registered_but_never_started |
predicate → not task.sessions |
test_resume_still_finishes_a_story_that_was_already_in_flight |
drop and not task.rearmed |
test_resume_does_not_gate_a_human_armed_re_drive |
1 red / 1346 green each — so each test discriminates a distinct design rather than merely the absence of the gate.
One note on scope: the README this PR ships already said the gate is about "work that must not start" and that the exemption is for a story already in flight. The code was the more permissive half, so this is the code catching up to the documented contract; I sharpened the sentence to say the line is drawn by session. SweepEngine is untouched — it overrides _loop and uses its own _finish_inflight_bundles, so the sweep exemption holds by construction, not by a flag I had to remember to set.
…arted
`_loop` saves the new StoryTask and *then* calls `_run_story`, so a host
death anywhere from that save through the isolated worktree mount and the
pre_story hooks persists a PENDING task no session ever touched. On resume
`_finish_inflight` drives it — `_pick_next` cannot, having skipped the key
as touched — and its restart arm called `_run_story` directly. A `gate:`
landing while the run was down was therefore never asked, and the one
deferred check that refuses could be disabled by the run's own crash.
The exemption is meant to be drawn by session, not by the presence of a
task record: `attempt` is bumped in the same save as the DEV_RUNNING
advance, so 0 means no session ever started. Not `sessions` — a record is
written after a session returns, so a death *inside* a session leaves
none, and that story is genuinely in flight. `rearm_escalation` is the one
other producer of a 0, on a task that HAS run sessions, so a human-armed
re-drive stays exempt.
Asked after the arm's unwinding and save, so a refused task is left in the
same shape `_loop` hands to `_run_story` — no worktree stranded behind a
pause, and the refusal re-asks on every later resume until the entry lands.
README already described this behavior ("work that must not start"); the
code was the more permissive half. Sharpened it to say the line is drawn
by session.
|
Pushed Real, and the window is wider than described. The fix re-asks The README this PR ships already drew this line ("work that must not start"); the code was the more permissive half, so this is the code catching up to the documented contract rather than a new rule. Gates on @pirony — this is the last of the review rounds, and it lands on the enforcement side rather than the parse/validate half you wrote. That half is still what made all of this expressible: a @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17282673c1
ℹ️ 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".
| # that has run sessions — a human-armed re-drive, which stays | ||
| # exempt like any other resume of started work. Read here, | ||
| # before the resets below clear `rearmed`. | ||
| never_dispatched = task.attempt == 0 and not task.rearmed |
There was a problem hiding this comment.
Fresh evidence beyond the prior resume finding is that _dev_phase persists attempt == 1 at lines 1620-1621 before _restore_patch, the pre-dev/pre-session plugin hooks at line 4058, and the actual adapter.run() launch at line 4188. If the host dies anywhere in that interval and a gate is added while it is down, no session has started, but this predicate sets never_dispatched false; _finish_inflight consequently restarts the task without _refuse_gated_story() and dispatches work the new gate says must not start. Track a durable launch boundary rather than using the earlier attempt increment as proof that a session ran.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and chasing it showed the predicate itself was the wrong shape. Fixed in c6b0a53 + 83f45ae — by deleting the predicate, not by adding the launch boundary you suggested.
Your finding verified. _dev_phase persists attempt == 1 with the DEV_RUNNING advance (engine.py:1620-1621), but the session does not launch until adapter.run(), past _restore_patch, the prompt build and the pre_session plugin gate (engine.py:4058) — which can veto or be slow. A death in there records an attempt no session backed, and my attempt == 0 read it as started.
And a second hole in the same predicate, in the opposite direction. I went looking for a durable launch marker as you suggested, and while checking whether rearmed really implied "a session ran" — the justification I had given for that conjunct — found it does not. StoriesEngine._pause_wedged (stories_engine.py:260-261) records an ESCALATED task with attempt == 0, no session records, and the comment "stopped the scan before any session ran this pick". After resolve, that task is also rearmed. So my exemption was waving a wedged story's very first dispatch straight past the gate. That one was mine, not inherited.
So the remedy is not a launch boundary. Three signals, three different ways of being wrong: attempt is set before launch, sessions only after a session returns (engine.py:4232), rearmed covers a story that never ran one. A new durable field would add a state-model change plus a save on the session hot path to answer a question the code already answers structurally:
the restart arm is the one
_finish_inflightarm that finishes nothing.
It discards the worktree or resets the tree to baseline, then re-runs the story from scratch. By the time the gate is asked, the task holds nothing a session produced and is in the same shape _loop hands to _run_story. So it asks unconditionally, and the exemption keeps its real home — the four finishing arms (defer replay, spec-approval continuation, recorded-session replay, commit completion), where the story is ending rather than starting. That is also what the README already claimed, and it makes the gate agree with validate, which refuses a gated story regardless of what its task record remembers — the docstring's own stated reason for "pause, not skip".
One consequence worth flagging for the maintainer rather than burying: a resolved escalation's re-drive is now gated too. That is deliberate. Resolving an escalation is not evidence the gating entry landed, the re-drive is a from-scratch restart, and the pause is re-askable and names the remedy.
Tests — five now, ablated whole-file (1417 tests), each RED set predicted before running:
| ablation | red |
|---|---|
| delete the restart-arm gate | never-started · re-drive · resolved wedge |
restore attempt == 0 and not rearmed (the superseded predicate) |
pre-launch attempt window · re-drive · resolved wedge |
The second row is the one that matters: those three fail against my previous commit's design, so they discriminate it rather than merely detecting the gate's absence. test_resume_still_finishes_a_story_whose_session_already_completed was rewritten to pin the exemption where it actually lives — a completed review session replaying through resume-verify — because its old form (DEV_RUNNING, attempt == 1, no record) was asserting exactly the behavior you flagged as wrong.
Good find, and thanks for pushing on the second round rather than accepting the first fix.
Codex found the residual in the previous commit's predicate, and chasing it showed the predicate itself was the wrong shape. `attempt` is bumped in the DEV_RUNNING save (engine.py:1620-1621) but the session does not launch until `adapter.run()`, past `_restore_patch`, the prompt build and the pre_session plugin gate — so a death in between reads as "started" when nothing started. `sessions` fails the other way: the record is written after a session returns, so a death inside one reads as never started. And `rearmed` — the signal the last commit trusted — is set by `StoriesEngine._pause_wedged` on a story that reached ESCALATED with `attempt == 0` and no session at all, so exempting re-drives waved a wedged story's very first dispatch straight past the gate. No such test can be right, because the question is already answered structurally: the restart arm is the one `_finish_inflight` arm that finishes nothing. It discards the worktree or resets the tree to baseline, then re-runs the story from scratch — so by the time the gate is asked the task holds nothing a session produced and is in the same shape `_loop` hands to `_run_story`. Ask unconditionally. The exemption keeps its real home: the finishing arms (defer replay, spec-approval continuation, recorded-session replay, commit completion), where the story is ending rather than starting. This also makes the gate agree with `validate`, which refuses a gated story regardless of what its task record remembers — the docstring's stated reason for "pause, not skip". Tests: the never-started case is unchanged; the in-flight exemption now pins a completed review session replaying through resume-verify (the real finishing arm); the re-drive case is inverted; and a stories-mode resolved wedge pins the state that makes the inference unbuildable.
…covers The reported case: attempt persisted with the DEV_RUNNING advance, no session record, host dead before adapter.run(). Covered by the arm asking unconditionally, but not directly pinned — the re-drive and wedge tests catch a reintroduced attempt check only incidentally.
|
Pushed
Rather than track a durable launch boundary (a new state field plus a save on the session hot path), the predicate is deleted. The restart arm is the one One behavior call for you, since it is a widening and not just a bug fix: a resolved escalation's re-drive is now gated as well. My reasoning is that resolving an escalation is not evidence the gating entry landed, the re-drive is a from-scratch restart, and Five tests, two whole-file ablations over 1417 tests, every RED set predicted before running. The load-bearing one restores the superseded Gates on @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f45aedc3
ℹ️ 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".
| # wedge that escalated with no session at all. Placed after the | ||
| # unwinding and the save, so a refusal strands no worktree and the | ||
| # task re-asks on every later resume until the entry lands. | ||
| self._refuse_gated_story(task.story_key) |
There was a problem hiding this comment.
When an in-place task has the normal persisted baseline_commit and a tracked gate: is added or committed while the process is down, this check runs only after _rollback_or_pause: with the default rollback policy, the ledger change makes recovery pause before the gate is evaluated, while with rollback_on_failure = true, safe_reset restores the old baseline and this call sees no gate, allowing the forbidden session to start. Preserve the post-crash ledger or latch the gate before recovery can reset it.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed by repro, and fixed in 986a3e5. This one was a genuine P1 — my placement, not the predicate.
Reproduced before fixing. In-place task, persisted baseline_commit, rollback_on_failure = true, and a gate: committed while the run was down: the gated story ran to completion, done=1, paused=False. Your mechanism is exactly right — the gate commit is after the baseline, so git reset --hard <baseline> (verify.py:876) rewound the ledger and the check then asked its question of a file the human never wrote.
Worth spelling out why keep doesn't save it, since that was my assumption when I placed the call late: keep=(".bmad-loop",) guards untracked deletion only, not tracked reverts. verify.safe_rollback's own docstring says so, and it is the reason policy.toml needs a hand-rolled read-before / write-after rescue naming "a change committed after baseline". A tracked ledger has no such rescue.
Your default-policy half is right too: with rollback_on_failure = false, _rollback_or_pause pauses for manual recovery and the gate is never evaluated. The run does stop there, so nothing forbidden starts — but the operator gets recovery instructions instead of "story gated", which is the less actionable of the two pauses. The same move fixes both halves.
Fix. Moved ahead of the journal append and both unwinding branches. That makes the rule uniform rather than two separate judgements: both call sites ask before their caller mutates anything — in _loop to keep the refusal re-askable, here to keep the ledger readable.
I took the ordering rather than your "preserve the post-crash ledger or latch the gate" alternatives on purpose: latching would make the gate answer from a snapshot, and the docstring already commits to the opposite ("the ledger is re-read here rather than carried from preflight: a sweep, or a human, may have closed the entry since"). Asking earlier keeps the live read and fixes the ordering.
Accepted cost, stated in the comment: a refused isolated task keeps its half-built worktree mounted until a resume gets past the gate. That is what an escalation pause already does, and it is the cheaper of the two mistakes — the alternative discards a worktree for a story that is not allowed to run yet.
Ablation, predicted first: move the call back after the unwinding → exactly 1 red (test_resume_reads_the_gate_before_the_restart_rollback_rewinds_the_ledger), 1417 green. So the new test discriminates the ordering, not the gate's presence.
Filed #520 for the part that is not this PR's. Your finding generalizes past gates: the restart arm's reset discards any commit a human made while the run was down, ledger or not, recoverable only from the reflog. verify.py is not in this PR's diff and the behavior predates the gate: field, so it is an issue rather than more scope here. #502 fixes the gate half by reading before the rewind; it deliberately leaves the rewind alone.
… ledger
Codex P1, confirmed by repro: an in-place task with a persisted baseline,
`rollback_on_failure = true`, and a `gate:` committed while the run was
down ran the gated story to completion.
The arm's rollback is `git reset --hard <baseline>` and the gate commit is
*after* that baseline, so asking the gate after the unwinding put the
question to a ledger the rollback had just rewound. `keep=(".bmad-loop",)`
does not save it — keep guards untracked deletion only, which is precisely
why `verify.safe_rollback` restores `policy.toml` by hand. Under the
default `rollback_on_failure = false` the same ordering pauses for manual
recovery and never reaches the gate at all.
Moved ahead of the journal append and both unwinding branches, which makes
the rule uniform: both call sites ask before their caller mutates anything
— in `_loop` to keep the refusal re-askable, here to keep the ledger
readable. Accepted cost: a refused isolated task keeps its half-built
worktree mounted until a resume gets past the gate, the same thing an
escalation pause does.
|
Pushed The gate commit is after the task's baseline, so the restart arm's Fixed by moving the call ahead of the journal append and both unwinding branches, which makes one rule out of what had been two separate judgements: both call sites ask before their caller mutates anything — in Ablation (predicted first): move the call back after the unwinding → exactly 1 red, 1417 green. The test discriminates the ordering, not the gate's presence. Filed #520 for the half that is not this PR's: the restart arm's reset discards any commit a human made while the run was down — ledger or not — recoverable only from the reflog. Gates on Round tally for whoever reads this later: three rounds of findings against my own enforcement commits, each one real — the registered-but-never-started window, the predicate that was the wrong shape entirely, and now the ordering against the rollback. The parse/validate half @pirony wrote has been stable throughout; every defect in this sequence was in the enforcement I added on top of it. @codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmad_loop/engine.py (1)
895-911: 🗄️ Data Integrity & Integration | 🟠 MajorCheck the restart-arm sweep exemption from the ledger gate.
SweepEngineskipsEngine._finish_inflight, but_recover_inflight_bundlestill has a restart arm for nonterminal non-COMMITTINGbundles that resumes without calling_refuse_gated_story. The documented pause now points atbmad-loop resume <run_id>after the bundle's worktree has already been discarded, because there is no fresh PENDING bundle task keyed for retry. Either keepSweepEngine._recover_inflight_bundlefrom reaching the discard/restart path, or restore the rejected bundle task before refusing.[low_effort и high_reward]
🤖 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 `@src/bmad_loop/engine.py` around lines 895 - 911, Update the restart arm of _recover_inflight_bundle so a nonterminal, non-COMMITTING bundle is not discarded without restoring a fresh PENDING bundle task. Restore the task before invoking _refuse_gated_story, ensuring the gate rejection leaves the story retryable through the documented resume flow while preserving the existing finishing-arm behavior.
🤖 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.
Outside diff comments:
In `@src/bmad_loop/engine.py`:
- Around line 895-911: Update the restart arm of _recover_inflight_bundle so a
nonterminal, non-COMMITTING bundle is not discarded without restoring a fresh
PENDING bundle task. Restore the task before invoking _refuse_gated_story,
ensuring the gate rejection leaves the story retryable through the documented
resume flow while preserving the existing finishing-arm behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 061d4764-58b1-446a-be9a-7d0c18883dd5
📒 Files selected for processing (6)
README.mdsrc/bmad_loop/deferredwork.pysrc/bmad_loop/engine.pytests/test_deferredwork.pytests/test_engine.pytests/test_stories_engine.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tests/test_deferredwork.py
- src/bmad_loop/deferredwork.py
…ands The refusal has to be a standing condition, not a one-shot: a refused task stays non-terminal, so every resume re-reads the ledger and closing the entry is what releases it. Asserted for the restart arm, where nothing covered it — the _loop side already has its own. Raised by a CodeRabbit question about bundle retryability; the sweep half of that finding is refuted (sweep.py has no _refuse_gated_story call site and SweepEngine overrides _loop), but the retry property it asked about was a real gap in the tests.
|
Pushed The finding, checked against the code. It reads: " That describes a consequence of a call that does not exist.
And the remedy would be a regression, not a fix. Both options you offer route toward the sweep refusing on a gate. That is the one thing this design forbids: the sweep is the automated closer of the gating entry (
What did land. Underneath the sweep framing you were asking whether a gate refusal leaves the work retryable — "no fresh PENDING task keyed for retry". For the engine's restart arm that property is real and load-bearing, and it was asserted nowhere. A refused task stays non-terminal, so Gates on |
|
Tip For best results, initiate chat on the files or code changes.
My finding incorrectly assumed that The new restart-arm test covers the valid underlying concern: a refused task remains retryable, rechecks the ledger on each resume, and runs only after the entry closes. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
2 participants
What
A deferred-work entry can now declare the stories it blocks with a machine-readable
gate: <story-key-token>[, …]field line;validategainsdeferred.hard-gate(problem — an actionable story is named by an open entry's gate) anddeferred.hard-gate-unstructured(warning — a proseHARD GATE:declaration or an inert/malformedgate:line the check cannot enforce).Why
Ledgers already carry prose like
HARD GATE: must run before story 3-2(e.g. a spike's credential leg that must land before its first consuming story), but nothing mechanical stopsbmad-loop runfrom driving that story while the entry is open — the gate is only as strong as someone re-reading the ledger. This keeps the control loop deterministic: the gate is plain parsing in validate, no LLM involved.How
deferredwork.py:gates(entry)parsesgate:lines insideparse_ledger's canonical span into a frozenEntryGates(valid tokens / malformed / inert);gates_story(token, key)matches at key boundaries —3-2gates3-2-invite-linkand the split halves3-2a-…/3-2b-…, never3-20-….cli.py: a new_validate_deferred_ledgerowns the single ledger read (anddeferred.ledger-unreadable), dispatching to_validate_hard_gates(new; both queue modes — sprint actionable statuses, stories-mode manifest minusdonespecs) and the existing closes-deferred checks.deferred-work-format.mddocuments the field (placement afterstatus:, one-line rule, preservation on rewrite), plus a README subsection, FEATURES bullet and CHANGELOG entry.Testing
uv run pytest -q -n auto: 5 failed, 4519 passed, 51 skipped — the 5 are pre-existing darwin/opencode-live failures, byte-identical on pristinemain. New behavior pinned by name intests/test_cli.py(12 cases) andtests/test_deferredwork.py(boundary/split/prose/inert parametrizations); ruff, black, isort and pyright all clean (pyright's 2 pre-existing darwin errors unchanged). Also exercised end-to-end against a real project ledger carrying one structured gate, one prose-only gate and one quoted citation — each classifies correctly.Happy to split this (parser helper first, validate wiring second) or adjust semantics per maintainer preference — and apologies for not passing through Discord first; the change is additive and stays out of the control loop, but say the word and I'll rework or downscope it.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation