Open
Conversation
|
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:
📝 WalkthroughWalkthroughAdds BMAD-METHOD integration: new BMAD manager and refresh script, main IPC/preload APIs, renderer services/hooks/UI panel, ~40+ prompt Markdown assets plus generated catalog/metadata, tests, packaging/script updates, Husky hooks, and docs/navigation entries. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant U as User
participant R as Renderer
participant M as Main
participant G as GitHub API
participant F as File System
end
U->>R: Click "Check for Updates"
R->>M: ipc invoke "bmad:refresh"
M->>G: fetch catalog & prompt files
G-->>M: return catalog + prompts
M->>F: write prompt files, catalog.ts, metadata.json
F-->>M: write complete
M-->>R: return metadata (success)
R-->>U: show updated metadata/timestamp
U->>R: Edit prompt & Save
R->>M: ipc invoke "bmad:savePrompt"(id, content)
M->>F: persist customization
F-->>M: save complete
M-->>R: { success: true }
R-->>U: confirm saved
U->>R: Click Reset
R->>M: ipc invoke "bmad:resetPrompt"(id)
M->>F: remove customization / load bundled prompt
F-->>M: operation complete
M-->>R: return bundled prompt
R-->>U: display reset content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 SummaryAdds BMAD as a third bundled methodology alongside SpecKit and OpenSpec, integrating 33 upstream workflow prompts from bmad-code-org/BMAD-METHOD with full refresh-from-GitHub, user customization, and slash-command execution support.
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SettingsUI as BmadCommandsPanel
participant Preload as window.maestro.bmad
participant IPC as bmad IPC handlers
participant Manager as bmad-manager
participant Disk as User Data / Bundled
participant GitHub as GitHub API
Note over User,GitHub: App Initialization
SettingsUI->>Preload: getPrompts() / getMetadata()
Preload->>IPC: bmad:getPrompts / bmad:getMetadata
IPC->>Manager: getBmadPrompts() / getBmadMetadata()
Manager->>Disk: Read user customizations + bundled prompts
Disk-->>Manager: Prompt content + metadata
Manager-->>IPC: BmadCommand[] / BmadMetadata
IPC-->>Preload: { success, commands/metadata }
Preload-->>SettingsUI: Display commands list
Note over User,GitHub: Check for Updates
User->>SettingsUI: Click "Check for Updates"
SettingsUI->>Preload: refresh()
Preload->>IPC: bmad:refresh
IPC->>Manager: refreshBmadPrompts()
loop For each of 33 commands
Manager->>GitHub: fetch raw prompt file
GitHub-->>Manager: Prompt markdown
Manager->>Disk: Write to bmad-prompts/
end
Manager->>GitHub: fetch commit SHA + version
GitHub-->>Manager: SHA, version
Manager->>Disk: Write metadata.json + customizations
Manager-->>IPC: Updated BmadMetadata
IPC-->>SettingsUI: { success, metadata }
Note over User,GitHub: Slash Command Execution
User->>SettingsUI: Type /bmad-bmm-create-prd
SettingsUI->>Preload: getCommand("/bmad-bmm-create-prd")
Preload->>IPC: bmad:getCommand
IPC->>Manager: getBmadCommandBySlash()
Manager-->>IPC: BmadCommand with prompt
IPC-->>Preload: { command }
Preload-->>SettingsUI: Execute prompt in agent session
Last reviewed commit: 7f9bf78 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
src/prompts/bmad/bmad.validate-prd.md-57-57 (1)
57-57:⚠️ Potential issue | 🟡 MinorFix capitalization for proper grammar.
The word "In" should be lowercase: "YOU MUST ALWAYS SPEAK OUTPUT in your Agent communication style".
📝 Proposed fix
-✅ YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style with the configured `{communication_language}`. +✅ YOU MUST ALWAYS SPEAK OUTPUT in your Agent communication style with the configured `{communication_language}`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.validate-prd.md` at line 57, Change the sentence "✅ YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style with the configured `{communication_language}`." so that the word "In" is lowercase; update the string in the prompt text (the sentence containing "YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style") to read "YOU MUST ALWAYS SPEAK OUTPUT in your Agent communication style with the configured `{communication_language}`."src/prompts/bmad/bmad.review-adversarial-general.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorRemove contradictory hostile wording from persona.
“Clueless weasel” conflicts with the same sentence’s “no personal attacks” constraint and can leak unnecessary hostility into outputs.
Suggested prompt fix
-**Your Role:** You are a cynical, jaded reviewer with zero patience for sloppy work. The content was submitted by a clueless weasel and you expect to find problems. Be skeptical of everything. Look for what's missing, not just what's wrong. Use a precise, professional tone — no profanity or personal attacks. +**Your Role:** You are a skeptical, detail-oriented reviewer with zero patience for sloppy work. Be rigorous and evidence-driven. Look for what's missing, not just what's wrong. Use a precise, professional tone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.review-adversarial-general.md` at line 5, Remove the contradictory hostile phrase "clueless weasel" from the persona string in the "Your Role" prompt and keep the rest of the persona (e.g., "cynical, jaded reviewer with zero patience for sloppy work"); replace the phrase with a neutral, descriptive term such as "careless author" or "low-effort submission" and ensure the "no profanity or personal attacks" constraint remains verbatim so outputs stay critical but non-personal.scripts/refresh-bmad.mjs-152-162 (1)
152-162:⚠️ Potential issue | 🟡 MinorFix the
src/preference check to match root-level paths.Line 161's
candidate.includes('/src/')fails for root-levelsrc/paths likesrc/bmm/foo/workflow.md. When multiple matches exist, this causes the function to fall back to API order instead of preferring the source tree.Suggested fix
- const preferred = matches.find((candidate) => candidate.includes('/src/')) ?? matches[0]; + const preferred = + matches.find( + (candidate) => candidate.startsWith('src/') || candidate.includes('/src/') + ) ?? matches[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/refresh-bmad.mjs` around lines 152 - 162, The preference check in resolveSkillWorkflowPath currently uses candidate.includes('/src/') which misses root-level paths like "src/…"; update the logic in resolveSkillWorkflowPath to detect a src/ segment at the start or after a slash (e.g. test for startsWith('src/') OR a segment match like /(^|\/)src\//) so root-level and nested src/ paths are both preferred when selecting among matches; ensure the change is applied to the variable named preferred and preserves the existing fallback to matches[0].src/prompts/bmad/bmad.retrospective.md-987-987 (1)
987-987:⚠️ Potential issue | 🟡 MinorUse “different from” instead of “different than.”
Minor wording fix for consistency and formal style in prompt text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.retrospective.md` at line 987, In the prompt text containing the phrase "User needs significantly different than originally understood" update the wording to use "different from" (i.e., change it to "User needs significantly different from originally understood") to match the requested formal style; locate this exact string in the bmad.retrospective prompt content and replace the phrase accordingly.src/__tests__/renderer/services/bmad.test.ts-14-21 (1)
14-21:⚠️ Potential issue | 🟡 MinorRestore the global spy and injected API after each test.
vi.clearAllMocks()only resets call history. It leavesconsole.errormocked andwindow.maestro.bmadinstalled for later suites, which can mask failures outside this file.♻️ Suggested cleanup
-import { describe, test, expect, vi, beforeEach } from 'vitest'; +import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest'; const mockBmad = { getPrompts: vi.fn(), getMetadata: vi.fn(), getCommand: vi.fn(), }; + +let previousBmad: unknown; beforeEach(() => { vi.clearAllMocks(); + previousBmad = (window as any).maestro?.bmad; (window as any).maestro = { ...(window as any).maestro, bmad: mockBmad, }; vi.spyOn(console, 'error').mockImplementation(() => {}); }); + +afterEach(() => { + vi.restoreAllMocks(); + if ((window as any).maestro) { + (window as any).maestro.bmad = previousBmad; + } +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/services/bmad.test.ts` around lines 14 - 21, The tests set up a module-scoped spy and inject window.maestro.bmad in beforeEach but only call vi.clearAllMocks(), which leaves console.error mocked and the injected bmad present for other suites; add an afterEach that calls vi.restoreAllMocks() and restores/cleans the global maestro injection (e.g. capture const originalMaestro = (window as any).maestro before mutating in beforeEach, then in afterEach set (window as any).maestro = originalMaestro or delete (window as any).maestro.bmad) so console.error and window.maestro are returned to their original state after each test.src/prompts/bmad/bmad.create-story.md-216-216 (1)
216-216:⚠️ Potential issue | 🟡 MinorReplace inappropriate language with professional terminology.
The phrase "developer fuckups" should be replaced with professional language suitable for documentation that may be shown to users or in enterprise environments.
✏️ Suggested fix
- <critical>🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you prevent future developer fuckups!</critical> + <critical>🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you prevent future developer mistakes!</critical>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.create-story.md` at line 216, Replace the unprofessional phrase "developer fuckups" in the critical header string "🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you prevent future developer fuckups!" with professional terminology (e.g., "developer errors", "implementation mistakes", or "prevent future developer mistakes"), keeping the intent and emphasis of the header intact; update the text inside the <critical> tag so the line reads something like "🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you prevent future developer errors!" to maintain tone appropriate for documentation.src/prompts/bmad/bmad.dev-story.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorMinor grammar fix: use "context-filled" (hyphenated compound adjective).
✏️ Suggested fix
-**Goal:** Execute story implementation following a context filled story spec file. +**Goal:** Execute story implementation following a context-filled story spec file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.dev-story.md` at line 3, Update the phrase "**Goal:** Execute story implementation following a context filled story spec file." by changing "context filled" to the hyphenated compound adjective "context-filled" so the line reads "**Goal:** Execute story implementation following a context-filled story spec file." This targets the exact text in src/prompts/bmad/bmad.dev-story.md.src/prompts/bmad/bmad.dev-story.md-393-404 (1)
393-404:⚠️ Potential issue | 🟡 MinorInconsistent template variable notation:
{sprint_status}vs{{sprint_status}}.Lines 393, 394, and 403 use single-brace notation
{sprint_status}while the rest of the document uses double-brace notation{{sprint_status}}(e.g., lines 64, 103, 234). This inconsistency could cause template rendering issues.✏️ Suggested fix
- <check if="{sprint_status} file exists AND {{current_sprint_status}} != 'no-sprint-tracking'"> - <action>Load the FULL file: {sprint_status}</action> + <check if="{{sprint_status}} file exists AND {{current_sprint_status}} != 'no-sprint-tracking'"> + <action>Load the FULL file: {{sprint_status}}</action> <action>Find development_status key matching {{story_key}}</action> ... </check> - <check if="{sprint_status} file does NOT exist OR {{current_sprint_status}} == 'no-sprint-tracking'"> + <check if="{{sprint_status}} file does NOT exist OR {{current_sprint_status}} == 'no-sprint-tracking'">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.dev-story.md` around lines 393 - 404, The template uses inconsistent variable braces: replace single-brace occurrences of {sprint_status} and {sprint_status} in the check blocks with the project's double-brace notation {{sprint_status}} (and ensure any single-brace use of {current_sprint_status} is {{current_sprint_status}}) so the conditionals that reference sprint_status, current_sprint_status, development_status, and story_key match the rest of the document; update the two check blocks so they read consistently with {{sprint_status}} and {{current_sprint_status}}, leaving development_status, story_key, last_updated, and the output text unchanged.
🧹 Nitpick comments (10)
src/prompts/bmad/bmad.code-review.md (1)
16-16: File-scope rule is too restrictive for AC verification.“Application source code only” can skip tests/docs/config/migrations that often carry acceptance-criteria evidence.
Suggested wording
-- Do not review files that are not part of the application's source code. +- Prioritize application source files for deep code analysis, but include tests, docs, and config/migration files when they provide evidence for AC/task completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.code-review.md` at line 16, Update the file-scoped rule that currently reads "Do not review files that are not part of the application's source code. Always exclude the `_bmad/` and `_bmad-output/` folders ... `.claude/`" to be less restrictive for acceptance-criteria verification: explicitly allow including tests, documentation, config, and migrations when they contain AC evidence while still excluding generated outputs and IDE/CLI config folders (referenced symbols: `_bmad/`, `_bmad-output/`, `.cursor/`, `.windsurf/`, `.claude/`); change the sentence to state that reviewers should generally skip generated artifacts and editor/tooling folders but must include tests/docs/config/migrations when they are relevant to acceptance criteria.src/renderer/hooks/remote/useRemoteHandlers.ts (1)
39-40: Use the BMAD-specific ref type here.Every other BMAD entry point in this PR uses
BmadCommand; keeping this asCustomAICommand[]erases the new type boundary and makes this hook the place where BMAD shape drift stops being compiler-visible.♻️ Proposed typing cleanup
-import type { Session, SessionState, LogEntry, CustomAICommand } from '../../types'; +import type { Session, SessionState, LogEntry, CustomAICommand, BmadCommand } from '../../types'; @@ - bmadCommandsRef?: React.MutableRefObject<CustomAICommand[]>; + bmadCommandsRef?: React.MutableRefObject<BmadCommand[]>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/remote/useRemoteHandlers.ts` around lines 39 - 40, The bmadCommandsRef prop is typed as React.MutableRefObject<CustomAICommand[]> which erases the BMAD-specific type boundary; change its type to React.MutableRefObject<BmadCommand[]> (or React.MutableRefObject<BmadCommand[] | undefined> if the ref can be unset) and ensure BmadCommand is imported where useRemoteHandlers.ts declares bmadCommandsRef so the compiler enforces the BMAD shape throughout the hook (update the prop/interface signature that defines bmadCommandsRef).src/__tests__/main/ipc/handlers/bmad.test.ts (1)
35-143: Good test coverage for BMAD IPC handlers.The tests comprehensively verify:
- All 6 channels are registered
- Each handler returns the expected response structure
- Manager functions are called with correct arguments
Consider adding error handling tests (e.g., when
getBmadMetadatathrows) to verify the handlers propagate errors correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/bmad.test.ts` around lines 35 - 143, Add tests that assert handlers propagate manager errors: mock a manager method (e.g., bmadManager.getBmadMetadata and one write op like bmadManager.saveBmadPrompt) to reject/throw and call the corresponding registered handler (channels 'bmad:getMetadata' and 'bmad:savePrompt' via the handlers Map created by registerBmadHandlers()), then assert the handler returns the expected error response (e.g., { success: false, error: <string> } or that it throws if that's the handler behavior) and that the manager method was invoked with the same arguments as the success tests; repeat similarly for at least one other read and one other write handler if desired.src/prompts/bmad/bmad.correct-course.md (1)
126-138: Optional: Add language specifier to fenced code block.The example code block at line 126 lacks a language specifier. While this doesn't affect functionality, adding a language (e.g.,
markdownortext) would be more consistent.📝 Proposed fix
- ``` + ```text Story: [STORY-123] User Authentication🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.correct-course.md` around lines 126 - 138, The fenced code block shown (the triple-backtick block containing "Story: [STORY-123] User Authentication" ) should include a language specifier for consistency; update that block to start with ```text or ```markdown instead of just ``` so the snippet is explicitly marked (search for the triple-backtick block around the acceptance criteria example and add the chosen language specifier).src/renderer/components/BmadCommandsPanel.tsx (2)
95-108: Non-null assertion can be avoided.At line 101,
result.prompt!uses non-null assertion after the truthiness check. While safe here, using the already-validated value directly is cleaner.♻️ Proposed fix
const handleReset = async (id: string) => { try { const result = await window.maestro.bmad.resetPrompt(id); - if (result.success && result.prompt) { + if (result.success && result.prompt !== undefined) { + const resetPrompt = result.prompt; setCommands( commands.map((cmd) => - cmd.id === id ? { ...cmd, prompt: result.prompt!, isModified: false } : cmd + cmd.id === id ? { ...cmd, prompt: resetPrompt, isModified: false } : cmd ) ); } } catch (error) { console.error('Failed to reset prompt:', error); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/BmadCommandsPanel.tsx` around lines 95 - 108, In handleReset, avoid the unnecessary non-null assertion on result.prompt; since you already check result.success && result.prompt, use the validated value directly when calling setCommands (replace result.prompt! with result.prompt) and update the mapped command (in the commands.map callback) to set prompt: result.prompt and isModified: false; ensure you reference the existing symbols handleReset, result.prompt, setCommands and commands.
65-69: Consider reporting unexpected errors to Sentry.Multiple catch blocks log errors but don't report them for production error tracking. Per coding guidelines, unexpected errors should be captured.
♻️ Example fix for one handler
+import { captureException } from '../utils/sentry'; + const handleSaveEdit = async () => { if (!editingCommand) return; try { const result = await window.maestro.bmad.savePrompt(editingCommand.id, editingCommand.prompt); if (result.success) { setCommands( commands.map((cmd) => cmd.id === editingCommand.id ? { ...cmd, prompt: editingCommand.prompt, isModified: true } : cmd ) ); setEditingCommand(null); } } catch (error) { console.error('Failed to save prompt:', error); + captureException(error, { tags: { component: 'BmadCommandsPanel', action: 'savePrompt' } }); } };Also applies to: 90-93, 105-107, 121-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/BmadCommandsPanel.tsx` around lines 65 - 69, In BmadCommandsPanel.tsx, update each catch block (the ones after the fetch/load handlers around the existing console.error calls in the component) to also report the error to Sentry: import the Sentry client used in the project (e.g. import * as Sentry from '@sentry/react' or '@sentry/browser') and call Sentry.captureException(error) in each catch (in addition to the console.error) so unexpected exceptions are sent to production monitoring; ensure this change is applied to the catch blocks referenced (the one around setIsLoading, and the catches at 90-93, 105-107, 121-123).src/renderer/services/bmad.ts (1)
24-27: Consider reporting unexpected errors to Sentry.The catch blocks log errors but don't report them to Sentry. Per coding guidelines, unexpected errors should be captured for production error tracking. The graceful return of default values is fine, but consider also reporting to Sentry.
♻️ Proposed fix for one of the functions
+import { captureException } from '../utils/sentry'; + export async function getBmadCommands(): Promise<BmadCommand[]> { try { const api = window.maestro?.bmad; if (!api) { return []; } const result = await api.getPrompts(); if (result.success && result.commands) { return result.commands; } return []; } catch (error) { console.error('[BMAD] Failed to get commands:', error); + captureException(error, { tags: { service: 'bmad' } }); return []; } }Also applies to: 44-47, 64-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/services/bmad.ts` around lines 24 - 27, The catch blocks in BMAD functions (e.g., the getCommands function that logs "[BMAD] Failed to get commands:") should also report unexpected errors to Sentry: import the project's Sentry helper (or Sentry.captureException) and call Sentry.captureException(error, { extra: { context: 'BMAD getCommands' } }) inside each catch before returning the default value; apply the same change to the other two catch blocks referenced in the review (the other BMAD functions that log failures at the commented ranges) so each logs and captures the exception with brief context.src/main/bmad-manager.ts (3)
252-285: Consider adding timeouts to fetch calls.The
fetchcalls to GitHub API don't specify timeouts. In edge cases with slow or unresponsive networks, these could hang indefinitely.💡 Optional: Add AbortController timeout
async function getLatestCommitSha(): Promise<string> { try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); const response = await fetch( `${BMAD_REPO_URL.replace('https://github.com', 'https://api.github.com/repos')}/commits/main`, { headers: { 'User-Agent': 'Maestro-BMAD-Refresher' }, + signal: controller.signal, } ); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 252 - 285, Add timeouts to the fetch calls in getLatestCommitSha and getLatestVersion by using an AbortController: create an AbortController before each fetch, start a timer (e.g., 5-10s) that calls controller.abort(), pass controller.signal to fetch, and clear the timer after the fetch completes; ensure the catch blocks handle abort errors the same way (log and return 'main') so aborted requests don't hang the process.
212-229: Consider validating the command ID before saving.The function saves customizations without validating that
idcorresponds to a known BMAD command. While the UI likely only passes valid IDs, adding defensive validation would prevent orphaned customization entries.💡 Optional validation
export async function saveBmadPrompt(id: string, content: string): Promise<void> { + const cmdExists = BMAD_COMMANDS.some((cmd) => cmd.id === id); + if (!cmdExists) { + throw new Error(`Unknown BMAD command: ${id}`); + } + const customizations = (await loadUserCustomizations()) ?? { metadata: await getBundledMetadata(), prompts: {}, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 212 - 229, The function saveBmadPrompt currently writes any id into user customizations without verifying it maps to a known BMAD command; update saveBmadPrompt to validate id before persisting by loading the bundled metadata (via getBundledMetadata or the already-loaded customizations.metadata) and checking that the id exists in the canonical command list (e.g., metadata.commands or metadata.prompts depending on your metadata shape); if the id is not found, either throw an Error or log a warning and return early instead of saving, and keep using loadUserCustomizations and saveUserCustomizations to persist only when the id is valid.
287-332: Partial refresh failure leaves system in inconsistent state.If a fetch fails partway through the loop (line 300-302), previously downloaded prompts are already written to disk. The user prompts directory would contain a mix of old and new versions without updated metadata.
Consider either:
- Writing to a temp directory first, then atomically moving on success
- Documenting that a failed refresh may require a manual cleanup
This is a resilience concern rather than a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 287 - 332, The refreshBmadPrompts function can leave partially-updated prompt files if one fetch fails; change it to fetch and write all prompt files into a temporary directory (e.g., create tempDir alongside getUserPromptsPath()), only compute metadata and replace the existing userPromptsDir atomically (fs.rename or move) after every fetch succeeds; ensure you update metadata.json and call saveUserCustomizations against the new directory contents, and on any failure clean up the tempDir and rethrow so the original prompt directory remains unchanged (references: refreshBmadPrompts, BMAD_COMMANDS, userPromptsDir, metadata.json, getLatestCommitSha, loadUserCustomizations, saveUserCustomizations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/refresh-bmad.mjs`:
- Around line 297-305: The current loop in scripts/refresh-bmad.mjs (inside the
for (const entry of catalog) { ... } block using getText, BMAD_DIR,
entry.sourcePath, and promptPath) only writes the single flattened file
(fs.writeFileSync(promptPath, prompt)), which breaks any relative references to
sibling assets; modify the routine to preserve upstream directory layout or copy
referenced assets: parse the fetched prompt text for relative paths (e.g.,
./steps/, templates/, checklists/, data/), create corresponding subdirectories
under BMAD_DIR (using path.join(BMAD_DIR, path.dirname(entry.sourcePath)) or a
mirrored folder per entry.id), fetch and write each referenced asset via
getText(entry's referenced path), update any in-file relative references if you
change their target locations, and ensure you create directories
(fs.mkdirSync(..., { recursive: true })) before writing files so the refresh
keeps companion assets in sync with the upstream source.
- Around line 29-54: The httpsGet function currently leaves https.get calls
unbounded; add a timeout mechanism (e.g., options.timeout with a sensible
default) and apply it to the underlying request: capture the request object
returned by https.get, call req.setTimeout(timeout) and on 'timeout' reject with
a descriptive Error (e.g., "Timeout after X ms: <url>") and abort/destroy the
request to free sockets; ensure you clear the timeout handlers when the response
'end' or 'error' fires and propagate the timeout option when following redirects
(httpsGet(res.headers.location, options)). Also ensure you attach the timeout
handling to both the request and the response socket to cover slow reads.
In `@src/prompts/bmad/bmad.code-review.md`:
- Around line 67-71: The git discovery steps use plain "git diff --name-only"
and "--cached" which only inspect local unstaged/staged working state and will
miss committed PR changes; update the workflow steps that call "git diff
--name-only" and "git diff --cached --name-only" to compare the branch against
the remote/base branch (e.g., run "git fetch" then use a range comparison like
"git diff --name-only origin/main...HEAD" or compute the merge-base and run "git
diff --name-only $(git merge-base origin/main HEAD)...HEAD") so committed
changes in the branch are detected; ensure the documentation replaces those
specific command mentions with the remote-range version and notes the
prerequisite "git fetch" where the actions list includes the original git diff
commands.
In `@src/prompts/bmad/bmad.create-epics-and-stories.md`:
- Around line 51-58: The prompt contains an escaped backslash in the `_bmad`
path (written as `\_bmad`) which can be copied into consumers and break config
reads; update the text in src/prompts/bmad/bmad.create-epics-and-stories.md to
use the literal `_bmad` (no backslash) wherever the initialization path appears
(notably in the line that currently reads
`/{project-root}/\_bmad/bmm/config.yaml`) and ensure the first-step reference to
`{project-root}/_bmad/bmm/workflows/3-solutioning/create-epics-and-stories/steps/step-01-validate-prerequisites.md`
matches that literal form so all consumers get an unescaped path.
In `@src/prompts/bmad/bmad.create-story.md`:
- Around line 161-212: The block starting at the duplicate "Load the FULL file:
{{sprint_status}}" through the "GOTO step 2a" (the second sprint status
discovery and story discovery logic) duplicates lines handled earlier (the first
discovery flow around the initial "Load the FULL file: {{sprint_status}}" and
story-finding steps) and should be removed; delete the redundant block (the
second Parse/Find/Check/epic-status update sequence that ends with "GOTO step
2a"), then verify any remaining "GOTO step 2a" targets or references still point
to the correct, single discovery flow and adjust step labels or branching text
if necessary to avoid broken or unreachable references.
In `@src/prompts/bmad/bmad.dev-story.md`:
- Around line 170-172: The <action if="no incomplete tasks"> block incorrectly
routes to <goto step="6">; update that goto to <goto step="9"> so the flow
proceeds to "Story completion and mark for review" when no tasks remain
(matching the routing used in Step 8). Locate the <action if="no incomplete
tasks"> element near the end of Step 1 and change its goto step attribute from
"6" to "9" to maintain consistency.
In `@src/prompts/bmad/bmad.market-research.md`:
- Around line 52-53: The artifact filename template currently uses raw
{{research_topic}} which can contain invalid filesystem characters; update the
filename generation to use a slugified version of research_topic (e.g.,
market-{{research_topic_slug}}-research-{{date}}.md) wherever the file is
created (the logic that builds
`{planning_artifacts}/research/market-{{research_topic}}-research-{{date}}.md`),
ensure the slugging function is applied before concatenation and preserves
uniqueness, and keep the file content loading step (./research.template.md and
./market-steps/step-01-init.md) unchanged except for referencing the slug field
if needed.
In `@src/prompts/bmad/bmad.qa-automate.md`:
- Around line 82-86: Update the "### Step 4: Run Tests" step so it does not
instruct the agent to automatically fix product code; change the directive that
currently says "If failures occur, fix them immediately" to instead instruct the
agent to run the project's test command, capture and report failures, and
provide suggested fixes or reproduce steps only (no implementation edits).
Reference the "### Step 4: Run Tests" heading and the tests-only scope (the line
that limits the command to generating tests) when making this change so the step
stays strictly within QA automation boundaries.
In `@src/prompts/bmad/bmad.retrospective.md`:
- Line 215: The file path glob in the action line uses an escaped wildcard `\*`
which matches a literal asterisk instead of globbing; update the action text
(the line starting "For each story in epic {{epic_number}}, read the complete
story file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md")
to use an unescaped wildcard `*` (i.e. change `\*` to `*`) so the pattern
`{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md` correctly
matches files, and apply the same replacement for the other occurrences
referenced (lines around 410 and 419).
- Around line 12-15: The prompt contains a contradictory rule: the bullet "No
time estimates — NEVER mention hours, days, weeks, months, or ANY time-based
predictions." conflicts with downstream sections that explicitly ask for
hours/days/sprints/timelines; resolve by making the policy consistent—either
remove all downstream time-estimate placeholders (e.g., any explicit
"hours/days/sprints" wording and tokens like {estimate} or {timeline}) or relax
the prohibition to a controlled guideline (e.g., change that bullet to "Avoid
precise time estimates; if required, provide non-committal ranges or high-level
timelines") and then update every retrospective/output section that requests
timelines (search for instances of hours/days/sprints/timelines and the tokens
referenced in the document) to match the chosen approach so all rules are
consistent with "Document output: Retrospective analysis." and the
{communication_language}/{user_skill_level} requirements.
In `@src/prompts/bmad/bmad.review-adversarial-general.md`:
- Around line 22-23: The prompt line "Review with extreme skepticism — assume
problems exist. Find at least ten issues to fix or improve in the provided
content." forces a minimum findings count; change it to remove the "Find at
least ten" requirement and instead ask the reviewer to list issues discovered
(allowing zero) with clear rationale for each finding and an explicit statement
if no issues are found; update the prompt text (the exact phrase above) to
something like "Review with extreme skepticism — identify all issues you find
and provide rationale; if none, state why no issues were found" and apply the
same change to the repeated instance referenced as "also applies to: 30-31".
In `@src/prompts/bmad/bmad.shard-doc.md`:
- Around line 34-37: The shell command invocation in the sharding step uses
unquoted placeholders so paths with spaces will be split; update the scripted
command that runs npx `@kayvan/markdown-tree-parser` explode [source-document]
[destination-folder] to wrap both placeholders in quotes (e.g.,
"source-document" and "destination-folder") when executing, ensure the
implementation that captures command output and errors still reads stdout/stderr
from the quoted invocation, and preserve the existing failure behavior to HALT
and display the captured error to the user (adjust the error handling around the
command execution to log the quoted-argument invocation and the error details).
In `@src/prompts/bmad/bmad.sprint-planning.md`:
- Around line 114-126: The current step n="3" only upgrades individual stories
found at {story_location_absolute}/{story-key}.md and preserves {status_file},
but does not propagate that discovery to the parent epic; after you set a story
to at least ready-for-dev, also locate the owning epic (e.g., epic-1) and ensure
the epic's status is upgraded to at least ready-for-dev if any of its child
stories were upgraded, while still respecting the preservation rule (never
downgrade an epic with a more advanced status and preserve existing
{status_file} values); implement this propagation immediately after the
story-status update in the step n="3" logic so generated YAML stays consistent
with later transition rules.
In `@src/prompts/bmad/bmad.sprint-status.md`:
- Around line 203-217: Step 20 (mode=data) currently delegates to "same as Step
2" which invokes interactive correction and assumes {sprint_status_file} exists;
change Step 20 to perform a deterministic, non-interactive load-and-parse of
{sprint_status_file} (no prompts or correction flows), compute the
recommendation the same way as Step 3, and if the file is missing or invalid
return clearly defined template-output defaults or an explicit error schema
(next_workflow_id, next_story_id, count_*, epic_*, risks) so callers always get
a non-blocking, predictable response.
- Around line 129-176: The template currently always renders the "Next
Recommendation" and the menu option to run a workflow even when no
next_workflow_id/next_story_id are set (terminal "all done" state). Update the
selection logic in step n="3" (where next_story_id/next_workflow_id/next_agent
are set) to also set a terminal flag (e.g., all_done = true) when you hit the
"Else → All implementation items done" branch; then change step n="4" and step
n="5" to conditionally render the recommendation block and the "Run recommended
workflow now" menu item only when next_workflow_id is defined (or all_done is
false), and otherwise render a clean congratulatory completion message and only
the Exit option. Reference symbols: step n="3" selection logic, variables
next_story_id, next_workflow_id, next_agent, and steps n="4"/n="5" output/ask
blocks.
In `@src/prompts/bmad/bmad.technical-research.md`:
- Around line 52-53: Sanitize the research_topic before embedding it in
filenames like technical-{{research_topic}}-research-{{date}}: implement a
slugify/sanitize step that trims whitespace, lowercases, replaces spaces with
hyphens, removes or percent-encodes reserved filesystem characters (/, \, :, *,
?, ", <, >, |), strips path traversal sequences (../), and enforces a sensible
max length and a safe fallback (e.g., "untitled-topic"); apply this
transformation wherever planning_artifacts and the filename pattern are
constructed so the created file path is always safe and valid.
In `@src/renderer/hooks/ui/useAppInitialization.ts`:
- Around line 232-242: Wrap the existing try/catch in the useEffect that calls
getBmadCommands() so failures are reported to Sentry: import captureException
(and optionally captureMessage) from src/renderer/utils/sentry.ts, call
captureException(error, { extra: { context: 'useAppInitialization - BMAD load' }
}) inside the catch before (or instead of) console.error, and keep the
setBmadCommands fallback to an empty list; reference the useEffect block,
getBmadCommands, and setBmadCommands when making the change.
---
Minor comments:
In `@scripts/refresh-bmad.mjs`:
- Around line 152-162: The preference check in resolveSkillWorkflowPath
currently uses candidate.includes('/src/') which misses root-level paths like
"src/…"; update the logic in resolveSkillWorkflowPath to detect a src/ segment
at the start or after a slash (e.g. test for startsWith('src/') OR a segment
match like /(^|\/)src\//) so root-level and nested src/ paths are both preferred
when selecting among matches; ensure the change is applied to the variable named
preferred and preserves the existing fallback to matches[0].
In `@src/__tests__/renderer/services/bmad.test.ts`:
- Around line 14-21: The tests set up a module-scoped spy and inject
window.maestro.bmad in beforeEach but only call vi.clearAllMocks(), which leaves
console.error mocked and the injected bmad present for other suites; add an
afterEach that calls vi.restoreAllMocks() and restores/cleans the global maestro
injection (e.g. capture const originalMaestro = (window as any).maestro before
mutating in beforeEach, then in afterEach set (window as any).maestro =
originalMaestro or delete (window as any).maestro.bmad) so console.error and
window.maestro are returned to their original state after each test.
In `@src/prompts/bmad/bmad.create-story.md`:
- Line 216: Replace the unprofessional phrase "developer fuckups" in the
critical header string "🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you
prevent future developer fuckups!" with professional terminology (e.g.,
"developer errors", "implementation mistakes", or "prevent future developer
mistakes"), keeping the intent and emphasis of the header intact; update the
text inside the <critical> tag so the line reads something like "🔬 EXHAUSTIVE
ARTIFACT ANALYSIS - This is where you prevent future developer errors!" to
maintain tone appropriate for documentation.
In `@src/prompts/bmad/bmad.dev-story.md`:
- Line 3: Update the phrase "**Goal:** Execute story implementation following a
context filled story spec file." by changing "context filled" to the hyphenated
compound adjective "context-filled" so the line reads "**Goal:** Execute story
implementation following a context-filled story spec file." This targets the
exact text in src/prompts/bmad/bmad.dev-story.md.
- Around line 393-404: The template uses inconsistent variable braces: replace
single-brace occurrences of {sprint_status} and {sprint_status} in the check
blocks with the project's double-brace notation {{sprint_status}} (and ensure
any single-brace use of {current_sprint_status} is {{current_sprint_status}}) so
the conditionals that reference sprint_status, current_sprint_status,
development_status, and story_key match the rest of the document; update the two
check blocks so they read consistently with {{sprint_status}} and
{{current_sprint_status}}, leaving development_status, story_key, last_updated,
and the output text unchanged.
In `@src/prompts/bmad/bmad.retrospective.md`:
- Line 987: In the prompt text containing the phrase "User needs significantly
different than originally understood" update the wording to use "different from"
(i.e., change it to "User needs significantly different from originally
understood") to match the requested formal style; locate this exact string in
the bmad.retrospective prompt content and replace the phrase accordingly.
In `@src/prompts/bmad/bmad.review-adversarial-general.md`:
- Line 5: Remove the contradictory hostile phrase "clueless weasel" from the
persona string in the "Your Role" prompt and keep the rest of the persona (e.g.,
"cynical, jaded reviewer with zero patience for sloppy work"); replace the
phrase with a neutral, descriptive term such as "careless author" or "low-effort
submission" and ensure the "no profanity or personal attacks" constraint remains
verbatim so outputs stay critical but non-personal.
In `@src/prompts/bmad/bmad.validate-prd.md`:
- Line 57: Change the sentence "✅ YOU MUST ALWAYS SPEAK OUTPUT In your Agent
communication style with the configured `{communication_language}`." so that the
word "In" is lowercase; update the string in the prompt text (the sentence
containing "YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style") to
read "YOU MUST ALWAYS SPEAK OUTPUT in your Agent communication style with the
configured `{communication_language}`."
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/bmad.test.ts`:
- Around line 35-143: Add tests that assert handlers propagate manager errors:
mock a manager method (e.g., bmadManager.getBmadMetadata and one write op like
bmadManager.saveBmadPrompt) to reject/throw and call the corresponding
registered handler (channels 'bmad:getMetadata' and 'bmad:savePrompt' via the
handlers Map created by registerBmadHandlers()), then assert the handler returns
the expected error response (e.g., { success: false, error: <string> } or that
it throws if that's the handler behavior) and that the manager method was
invoked with the same arguments as the success tests; repeat similarly for at
least one other read and one other write handler if desired.
In `@src/main/bmad-manager.ts`:
- Around line 252-285: Add timeouts to the fetch calls in getLatestCommitSha and
getLatestVersion by using an AbortController: create an AbortController before
each fetch, start a timer (e.g., 5-10s) that calls controller.abort(), pass
controller.signal to fetch, and clear the timer after the fetch completes;
ensure the catch blocks handle abort errors the same way (log and return 'main')
so aborted requests don't hang the process.
- Around line 212-229: The function saveBmadPrompt currently writes any id into
user customizations without verifying it maps to a known BMAD command; update
saveBmadPrompt to validate id before persisting by loading the bundled metadata
(via getBundledMetadata or the already-loaded customizations.metadata) and
checking that the id exists in the canonical command list (e.g.,
metadata.commands or metadata.prompts depending on your metadata shape); if the
id is not found, either throw an Error or log a warning and return early instead
of saving, and keep using loadUserCustomizations and saveUserCustomizations to
persist only when the id is valid.
- Around line 287-332: The refreshBmadPrompts function can leave
partially-updated prompt files if one fetch fails; change it to fetch and write
all prompt files into a temporary directory (e.g., create tempDir alongside
getUserPromptsPath()), only compute metadata and replace the existing
userPromptsDir atomically (fs.rename or move) after every fetch succeeds; ensure
you update metadata.json and call saveUserCustomizations against the new
directory contents, and on any failure clean up the tempDir and rethrow so the
original prompt directory remains unchanged (references: refreshBmadPrompts,
BMAD_COMMANDS, userPromptsDir, metadata.json, getLatestCommitSha,
loadUserCustomizations, saveUserCustomizations).
In `@src/prompts/bmad/bmad.code-review.md`:
- Line 16: Update the file-scoped rule that currently reads "Do not review files
that are not part of the application's source code. Always exclude the `_bmad/`
and `_bmad-output/` folders ... `.claude/`" to be less restrictive for
acceptance-criteria verification: explicitly allow including tests,
documentation, config, and migrations when they contain AC evidence while still
excluding generated outputs and IDE/CLI config folders (referenced symbols:
`_bmad/`, `_bmad-output/`, `.cursor/`, `.windsurf/`, `.claude/`); change the
sentence to state that reviewers should generally skip generated artifacts and
editor/tooling folders but must include tests/docs/config/migrations when they
are relevant to acceptance criteria.
In `@src/prompts/bmad/bmad.correct-course.md`:
- Around line 126-138: The fenced code block shown (the triple-backtick block
containing "Story: [STORY-123] User Authentication" ) should include a language
specifier for consistency; update that block to start with ```text or
```markdown instead of just ``` so the snippet is explicitly marked (search for
the triple-backtick block around the acceptance criteria example and add the
chosen language specifier).
In `@src/renderer/components/BmadCommandsPanel.tsx`:
- Around line 95-108: In handleReset, avoid the unnecessary non-null assertion
on result.prompt; since you already check result.success && result.prompt, use
the validated value directly when calling setCommands (replace result.prompt!
with result.prompt) and update the mapped command (in the commands.map callback)
to set prompt: result.prompt and isModified: false; ensure you reference the
existing symbols handleReset, result.prompt, setCommands and commands.
- Around line 65-69: In BmadCommandsPanel.tsx, update each catch block (the ones
after the fetch/load handlers around the existing console.error calls in the
component) to also report the error to Sentry: import the Sentry client used in
the project (e.g. import * as Sentry from '@sentry/react' or '@sentry/browser')
and call Sentry.captureException(error) in each catch (in addition to the
console.error) so unexpected exceptions are sent to production monitoring;
ensure this change is applied to the catch blocks referenced (the one around
setIsLoading, and the catches at 90-93, 105-107, 121-123).
In `@src/renderer/hooks/remote/useRemoteHandlers.ts`:
- Around line 39-40: The bmadCommandsRef prop is typed as
React.MutableRefObject<CustomAICommand[]> which erases the BMAD-specific type
boundary; change its type to React.MutableRefObject<BmadCommand[]> (or
React.MutableRefObject<BmadCommand[] | undefined> if the ref can be unset) and
ensure BmadCommand is imported where useRemoteHandlers.ts declares
bmadCommandsRef so the compiler enforces the BMAD shape throughout the hook
(update the prop/interface signature that defines bmadCommandsRef).
In `@src/renderer/services/bmad.ts`:
- Around line 24-27: The catch blocks in BMAD functions (e.g., the getCommands
function that logs "[BMAD] Failed to get commands:") should also report
unexpected errors to Sentry: import the project's Sentry helper (or
Sentry.captureException) and call Sentry.captureException(error, { extra: {
context: 'BMAD getCommands' } }) inside each catch before returning the default
value; apply the same change to the other two catch blocks referenced in the
review (the other BMAD functions that log failures at the commented ranges) so
each logs and captures the exception with brief context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3980da0f-c4e5-4a0c-9ee4-f71172b8892b
📒 Files selected for processing (60)
docs/bmad-commands.mddocs/configuration.mddocs/docs.jsonpackage.jsonscripts/refresh-bmad.mjssrc/__tests__/main/bmad-manager.test.tssrc/__tests__/main/ipc/handlers/bmad.test.tssrc/__tests__/renderer/services/bmad.test.tssrc/main/bmad-manager.tssrc/main/index.tssrc/main/ipc/handlers/bmad.tssrc/main/ipc/handlers/index.tssrc/main/preload/commands.tssrc/main/preload/index.tssrc/prompts/bmad/bmad.brainstorming.mdsrc/prompts/bmad/bmad.check-implementation-readiness.mdsrc/prompts/bmad/bmad.code-review.mdsrc/prompts/bmad/bmad.correct-course.mdsrc/prompts/bmad/bmad.create-architecture.mdsrc/prompts/bmad/bmad.create-epics-and-stories.mdsrc/prompts/bmad/bmad.create-prd.mdsrc/prompts/bmad/bmad.create-product-brief.mdsrc/prompts/bmad/bmad.create-story.mdsrc/prompts/bmad/bmad.create-ux-design.mdsrc/prompts/bmad/bmad.dev-story.mdsrc/prompts/bmad/bmad.document-project.mdsrc/prompts/bmad/bmad.domain-research.mdsrc/prompts/bmad/bmad.edit-prd.mdsrc/prompts/bmad/bmad.editorial-review-prose.mdsrc/prompts/bmad/bmad.editorial-review-structure.mdsrc/prompts/bmad/bmad.generate-project-context.mdsrc/prompts/bmad/bmad.help.mdsrc/prompts/bmad/bmad.index-docs.mdsrc/prompts/bmad/bmad.market-research.mdsrc/prompts/bmad/bmad.party-mode.mdsrc/prompts/bmad/bmad.qa-automate.mdsrc/prompts/bmad/bmad.quick-dev-new-preview.mdsrc/prompts/bmad/bmad.quick-dev.mdsrc/prompts/bmad/bmad.quick-spec.mdsrc/prompts/bmad/bmad.retrospective.mdsrc/prompts/bmad/bmad.review-adversarial-general.mdsrc/prompts/bmad/bmad.review-edge-case-hunter.mdsrc/prompts/bmad/bmad.shard-doc.mdsrc/prompts/bmad/bmad.sprint-planning.mdsrc/prompts/bmad/bmad.sprint-status.mdsrc/prompts/bmad/bmad.technical-research.mdsrc/prompts/bmad/bmad.validate-prd.mdsrc/prompts/bmad/catalog.tssrc/prompts/bmad/index.tssrc/prompts/bmad/metadata.jsonsrc/renderer/App.tsxsrc/renderer/components/BmadCommandsPanel.tsxsrc/renderer/components/Settings/SettingsModal.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useQueueProcessing.tssrc/renderer/hooks/remote/useRemoteHandlers.tssrc/renderer/hooks/ui/useAppInitialization.tssrc/renderer/services/bmad.tssrc/renderer/stores/agentStore.tssrc/renderer/types/index.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/renderer/components/BmadCommandsPanel.tsx (2)
257-260: Hardcoded text color may have poor contrast on some themes.The save button uses hardcoded
#000000for text color. Iftheme.colors.successis a dark green, this could have contrast issues. Consider usingtheme.colors.accentForegroundor a theme-aware approach.Suggested fix
style={{ backgroundColor: theme.colors.success, - color: '#000000', + color: theme.colors.accentForeground, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/BmadCommandsPanel.tsx` around lines 257 - 260, The save button in BmadCommandsPanel currently hardcodes color: '#000000' which can fail contrast with theme.colors.success; update the inline style (the style object applied to the save button) to use a theme-aware foreground such as theme.colors.accentForeground (or compute a readable color from theme.colors.success) instead of '#000000' so the text contrast respects the active theme.
303-305: Hardcoded hover colorbg-white/5may clash with light themes.Using
hover:bg-white/5assumes a dark theme. On light themes, this white overlay would be nearly invisible or inappropriate. Consider using a theme-aware hover state.This also applies to lines 353 and 363 which use
hover:bg-white/10.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/BmadCommandsPanel.tsx` around lines 303 - 305, The hover classes in BmadCommandsPanel are hardcoded to white (e.g., the button with onClick={() => toggleExpanded(cmd.id)} using hover:bg-white/5 and the other occurrences using hover:bg-white/10), which breaks on light themes; update those elements to use theme-aware Tailwind classes by providing both light and dark variants (for example use a neutral/light hover for default and dark:hover:... for dark mode) so the hover is visible and appropriate in both themes—locate the button with toggleExpanded(cmd.id) and the two other elements referencing hover:bg-white/10 and replace their hover classes with paired variants like hover:bg-gray-100 dark:hover:bg-white/5 (or equivalent neutral colors) to ensure consistent appearance.src/__tests__/renderer/hooks/useQueueProcessing.test.ts (1)
199-206: Add one populated BMAD ref case.These assertions only cover the default
[]branch. A regression wherebmadCommandsRef.currentstops reachingagentStorewould still pass, so I'd mirror the existing custom/speckit/openspec ref tests with a non-empty BMAD array.Also applies to: 682-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useQueueProcessing.test.ts` around lines 199 - 206, Add a test variant that populates bmadCommandsRef.current and asserts that mockAgentStoreProcessQueuedItem receives that populated array (mirror existing tests for customAICommandsRef, speckitCommandsRef, openspecCommandsRef); update the expectations around mockAgentStoreProcessQueuedItem in useQueueProcessing.test (references: mockAgentStoreProcessQueuedItem, bmadCommandsRef, useQueueProcessing) to include a non-empty bmadCommands array in the call argument and duplicate the same change for the other occurrence mentioned (lines ~682-688).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/bmad-manager.ts`:
- Around line 215-228: Multiple functions (saveBmadPrompt, resetBmadPrompt,
refreshBmadPrompts) perform concurrent read-modify-write on the same
bmad-customizations.json causing race conditions; introduce a single in-process
async mutex or queue to serialize all operations that call
loadUserCustomizations/saveUserCustomizations. Wrap the body of saveBmadPrompt,
resetBmadPrompt, and refreshBmadPrompts in acquire/release (or enqueue/dequeue)
so only one read-modify-write runs at a time, ensuring the lock covers loading
the customizations, mutating customizations.prompts, and calling
saveUserCustomizations; reference the existing functions saveBmadPrompt,
resetBmadPrompt, refreshBmadPrompts and the helpers
loadUserCustomizations/saveUserCustomizations when implementing the mutex.
- Around line 66-72: The loadUserCustomizations function currently swallows all
errors making parse/permission failures indistinguishable from ENOENT; update
loadUserCustomizations to only treat a missing file (ENOENT) as return null, and
for all other exceptions call captureException(error) and rethrow so Sentry
receives the error; add an import for captureException and preserve use of
getUserDataPath and the bmad-customizations.json semantics when implementing
this change.
- Around line 252-285: Add AbortController timeouts to the GitHub fetches by
using AbortSignal.timeout(10000) (10s) and passing the controller.signal to
fetch calls in getLatestCommitSha and getLatestVersion, and apply the same
pattern to the prompt download loop inside refreshBmadPrompts; ensure you create
the controller (or use AbortSignal.timeout directly) before each fetch, call
fetch(..., { signal }), and handle AbortError in the existing catch blocks so
the functions still return the fallback 'main' on timeout—follow the same
approach used elsewhere for network timeouts in the repo.
In `@src/renderer/components/BmadCommandsPanel.tsx`:
- Around line 327-331: The inline style in BmadCommandsPanel (where
backgroundColor is set using theme.colors.warning + '20') assumes a 6-digit hex
and can produce invalid CSS for rgb()/hsl()/8-digit hex; change it to produce a
proper translucent color instead — either use the theme helper if available
(e.g., theme.fn.rgba(theme.colors.warning, 0.125)) or convert the color to RGBA
with a small utility (hex/rgb/hsl -> rgba) and use that value for
backgroundColor, or alternatively remove the concatenation and apply opacity via
a wrapper element (style.opacity = 0.125) so the text color
(theme.colors.warning) remains unchanged; update the style object in the
component (BmadCommandsPanel) where backgroundColor is assigned.
- Around line 65-66: The catch blocks in loadData and the handlers
handleSaveEdit, handleReset, and handleRefresh currently only console.error and
thus swallow errors; update each catch to call Sentry.captureException(error)
(or the project's captureException wrapper) and also set a local error state or
invoke the existing UI notification mechanism to show a user-facing message
(e.g., "Failed to load BMAD commands" / "Failed to save edit" / "Failed to
reset" / "Failed to refresh"); ensure you reference the specific functions
loadData, handleSaveEdit, handleReset, and handleRefresh so each catch both
reports to Sentry and updates UI error state rather than only logging to
console.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useQueueProcessing.test.ts`:
- Around line 199-206: Add a test variant that populates bmadCommandsRef.current
and asserts that mockAgentStoreProcessQueuedItem receives that populated array
(mirror existing tests for customAICommandsRef, speckitCommandsRef,
openspecCommandsRef); update the expectations around
mockAgentStoreProcessQueuedItem in useQueueProcessing.test (references:
mockAgentStoreProcessQueuedItem, bmadCommandsRef, useQueueProcessing) to include
a non-empty bmadCommands array in the call argument and duplicate the same
change for the other occurrence mentioned (lines ~682-688).
In `@src/renderer/components/BmadCommandsPanel.tsx`:
- Around line 257-260: The save button in BmadCommandsPanel currently hardcodes
color: '#000000' which can fail contrast with theme.colors.success; update the
inline style (the style object applied to the save button) to use a theme-aware
foreground such as theme.colors.accentForeground (or compute a readable color
from theme.colors.success) instead of '#000000' so the text contrast respects
the active theme.
- Around line 303-305: The hover classes in BmadCommandsPanel are hardcoded to
white (e.g., the button with onClick={() => toggleExpanded(cmd.id)} using
hover:bg-white/5 and the other occurrences using hover:bg-white/10), which
breaks on light themes; update those elements to use theme-aware Tailwind
classes by providing both light and dark variants (for example use a
neutral/light hover for default and dark:hover:... for dark mode) so the hover
is visible and appropriate in both themes—locate the button with
toggleExpanded(cmd.id) and the two other elements referencing hover:bg-white/10
and replace their hover classes with paired variants like hover:bg-gray-100
dark:hover:bg-white/5 (or equivalent neutral colors) to ensure consistent
appearance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4da4ce26-3ed3-4513-bad8-4fc364ba34e9
📒 Files selected for processing (4)
src/__tests__/renderer/hooks/useQueueProcessing.test.tssrc/main/bmad-manager.tssrc/renderer/components/BmadCommandsPanel.tsxsrc/renderer/hooks/agent/useQueueProcessing.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
src/main/bmad-manager.ts (3)
113-119:⚠️ Potential issue | 🟠 MajorOnly treat missing customization file as recoverable; report and rethrow all other read failures.
Line [113]-Line [119] currently swallows parse/permission/corruption errors as
null, making real failures indistinguishable from “no file”.As per coding guidelines: "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production."Suggested fix
import { logger } from './utils/logger'; +import { captureException } from './utils/sentry'; async function loadUserCustomizations(): Promise<StoredData | null> { try { const content = await fs.readFile(getUserDataPath(), 'utf-8'); return JSON.parse(content); - } catch { - return null; + } catch (error) { + const err = error as NodeJS.ErrnoException; + if (err.code === 'ENOENT') { + return null; + } + captureException(error, { context: 'bmad-manager.loadUserCustomizations' }); + throw error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 113 - 119, In loadUserCustomizations, don’t swallow all errors: only treat a missing file (ENOENT from fs.readFile on getUserDataPath()) as recoverable and return null; for any other errors—including JSON.parse failures, permission errors, or corruption—rethrow the original error so it can be reported (and not conflated with "no file"). Locate function loadUserCustomizations and change the catch to inspect the error code (or error type) and return null only for file-not-found, otherwise throw the error.
262-275:⚠️ Potential issue | 🟠 MajorSerialize all
bmad-customizations.jsonread-modify-write mutations.These paths mutate the same file without coordination, so overlapping IPC calls can overwrite each other and lose prompt edits/metadata.
Also applies to: 289-293, 380-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 262 - 275, Concurrent mutations to bmad-customizations.json are not serialized, causing race conditions in the read-modify-write sequences (e.g., saveBmadPrompt and the other functions that call loadUserCustomizations()/saveUserCustomizations()). Fix by introducing a single async mutex/queue for these file mutations: acquire the lock at the start of the function that does loadUserCustomizations(), perform read-modify-save (e.g., in saveBmadPrompt), and release the lock in a finally block (also ensure thrown errors still release the lock); reuse the same lock for the other mutation sites (the other functions that call loadUserCustomizations()/saveUserCustomizations at the referenced ranges) so only one mutation operates on the file at a time.
299-306:⚠️ Potential issue | 🟠 MajorBound BMAD GitHub fetches with abort timeouts.
The network calls in commit/version lookup and prompt refresh have no timeout signal, so refresh can hang indefinitely on stalled requests.
Also applies to: 320-322, 342-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 299 - 306, getLatestCommitSha (and the other fetch calls used for commit/version lookup and prompt refresh) lack an abort timeout, so network stalls can hang the refresh; wrap each fetch in an AbortController with a timeout (e.g., 10s), pass controller.signal into fetch, clear the timeout on success, and throw/propagate a clear timeout error when controller.abort() fires so callers can handle failures; apply this pattern to getLatestCommitSha and to the other fetch calls used for prompt refresh/version lookup referenced in the review.scripts/refresh-bmad.mjs (2)
76-101:⚠️ Potential issue | 🟠 MajorAdd request timeouts to
httpsGetto prevent hanging refresh jobs.Line [76]-Line [101] still performs outbound GitHub requests without a timeout, so CI/automation can stall indefinitely on slow or half-open connections.
Suggested fix
function httpsGet(url, options = {}) { return new Promise((resolve, reject) => { + const timeoutMs = options.timeoutMs ?? 15000; const headers = { 'User-Agent': 'Maestro-BMAD-Refresher', Accept: 'application/vnd.github+json', ...options.headers, }; - https - .get(url, { headers }, (res) => { + const req = https.get(url, { headers }, (res) => { if (res.statusCode === 301 || res.statusCode === 302) { return resolve(httpsGet(res.headers.location, options)); } if (res.statusCode !== 200) { reject(new Error(`HTTP ${res.statusCode}: ${url}`)); return; } let data = ''; res.on('data', (chunk) => (data += chunk)); res.on('end', () => resolve({ data, headers: res.headers })); res.on('error', reject); - }) - .on('error', reject); + }); + + req.setTimeout(timeoutMs, () => { + req.destroy(new Error(`Request timed out after ${timeoutMs}ms: ${url}`)); + }); + req.on('error', reject); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/refresh-bmad.mjs` around lines 76 - 101, The httpsGet function lacks request timeouts which can hang jobs; update httpsGet to accept an optional timeout (e.g., options.timeout) and enforce it by setting a timer and calling req.abort()/reject(new Error('Request timed out')) if the timer fires, clearing the timer on 'response' end/error, and also attach a socket 'timeout' handler on the request to abort similarly; ensure the redirect branch (httpsGet(res.headers.location, options)) propagates the timeout option so redirected requests also respect the timeout.
344-353:⚠️ Potential issue | 🟠 MajorRefreshing only flattened workflows can leave relative prompt assets stale or broken.
Line [344]-Line [353] writes only
bmad.{id}.md; upstream workflows that reference sibling assets (./steps, templates, checklists, data) can drift or break after refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/refresh-bmad.mjs` around lines 344 - 353, The refresh currently only writes the flattened prompt file in the loop over catalog (see the for (const entry of catalog) block and variables promptPath, prompt created via applyMaestroPromptFixes/getText); update this block so that when existing !== prompt you not only fs.writeFileSync the bmad.{entry.id}.md but also sync any sibling asset directories referenced by the workflow (e.g., ./steps, templates, checklists, data) from the remote source path (use the same RAW_GITHUB/REPO_OWNER/REPO_NAME/REPO_REF and entry.sourcePath to locate them), creating or replacing the corresponding directories under BMAD_DIR alongside the prompt file; ensure you preserve directory structure and replace stale files atomically to avoid partial updates.src/prompts/bmad/bmad.retrospective.md (1)
215-215:⚠️ Potential issue | 🟠 MajorUnescape wildcard globs so discovery patterns actually match files.
Line [215], Line [410], and Line [419] still use escaped
\*, which can prevent matching story/epic files.Suggested fix
-<action>For each story in epic {{epic_number}}, read the complete story file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md</action> +<action>For each story in epic {{epic_number}}, read the complete story file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md</action> -<action>Check if file exists: {planning_artifacts}/epic\*/epic-{{next_epic_num}}.md</action> +<action>Check if file exists: {planning_artifacts}/*epic*/epic-{{next_epic_num}}.md</action> -<action>Check if file exists: {planning_artifacts}/epic\*.md</action> +<action>Check if file exists: {planning_artifacts}/*epic*.md</action>Also applies to: 410-410, 419-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.retrospective.md` at line 215, The prompt uses escaped glob sequences like "\*" which prevent pattern matching; update the actions that build discovery paths (e.g., the action line "For each story in epic {{epic_number}}, read the complete story file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md" and the other occurrences noted) to use an unescaped "*" (e.g., {implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md) so file globs actually match; find and replace the "\*" occurrences in src/prompts/bmad/bmad.retrospective.md (including the instances referenced around line 215, 410, and 419) with "*" in the action/pattern strings.
🧹 Nitpick comments (2)
.husky/_/husky.sh (1)
1-9: Missing shebang - acceptable for sourced scripts but consider adding for clarity.Shellcheck flags SC2148 (missing shebang). Since this script is designed to be sourced (not executed directly), the shebang isn't strictly required. However, adding
#!/usr/bin/env shat the top would silence the linter and clarify the expected shell dialect.This is a minor, optional improvement given the script's sourced nature.
🔧 Optional fix to add shebang
+#!/usr/bin/env sh echo "husky - DEPRECATED Please remove the following two lines from $0:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/_/husky.sh around lines 1 - 9, Add a POSIX sh shebang as the very first line of the .husky/_/husky.sh script (e.g., using env to select sh) to satisfy shellcheck SC2148 and clarify the expected shell dialect for this sourced script; keep the rest of the file unchanged since it is intended to be sourced.src/prompts/bmad/bmad.create-story.md (1)
164-164: Consider more professional language.The phrase "prevent future developer fuckups" is very informal and may not be appropriate for professional documentation. Consider alternatives like "prevent developer mistakes" or "avoid common pitfalls."
📝 Suggested alternative
- <critical>🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you prevent future developer fuckups!</critical> + <critical>🔬 EXHAUSTIVE ARTIFACT ANALYSIS - This is where you prevent future developer mistakes!</critical>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.create-story.md` at line 164, Replace the informal phrase inside the critical block that currently reads "prevent future developer fuckups" with a professional alternative (e.g., "prevent developer mistakes", "avoid common pitfalls", or "prevent future developer errors"); locate the text in the critical tag labeled "🔬 EXHAUSTIVE ARTIFACT ANALYSIS" in src/prompts/bmad/bmad.create-story.md and update the wording to a more formal phrasing while preserving the emphasis and intent of the original note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/_/pre-merge-commit:
- Around line 1-2: Create the missing companion hook file
.husky/pre-merge-commit containing the repository's standard validation commands
(for example: npx lint-staged and npm run validate or whichever scripts are used
in .husky/pre-commit and .husky/pre-push) so the bootstrap wrapper
.husky/_/pre-merge-commit (which sources h) actually executes validations
instead of exiting; ensure the new .husky/pre-merge-commit is executable (chmod
+x) and follows the same pattern/format as the existing .husky/pre-commit and
.husky/pre-push files.
In `@docs/releases.md`:
- Line 20: Replace top-level headings like "# Major 0.15.x Additions" with
nested headings (e.g. "### Major 0.15.x Additions" or "####" as appropriate) so
they appear under their surrounding "## vX" version sections; update every
similar occurrence in docs/releases.md (the headings identified in the review)
to use a lower-level heading to avoid resetting the TOC and keep heading
hierarchy consistent.
- Line 90: The release note copy has inconsistent hyphenation, naming, and
casing (e.g., the line "Agents are now inherently aware of your activity history
as seen in the history panel 📜 (this is built-in cross context memory!)");
update this and the other affected lines (mentioned as 101, 176, 201, 266) to
consistent, user-facing phrasing: use "cross‑context memory" (hyphenated),
standard sentence casing ("Agents" or "agents" consistently across the doc),
remove extraneous punctuation/parentheses or exclamation marks, and ensure emoji
usage matches the project's style—rewrite the sentence to something like:
"Agents are now aware of your activity history as shown in the History panel —
this is built‑in cross‑context memory." Apply the same hyphenation, casing, and
punctuation conventions to the other listed lines.
In `@src/prompts/bmad/bmad.create-story.md`:
- Line 61: The file opens a <workflow> element but never closes it; add a
closing </workflow> tag at the end of the document immediately after the final
step (after the block that ends at line 327) so the <workflow> element is
properly terminated.
- Line 67: Several GOTO statements ("GOTO step 2a") in
src/prompts/bmad/bmad.create-story.md (occurring at the diffs referencing lines
67, 90, 95, 158) point to a non-existent label; update each "GOTO step 2a" to
"GOTO step 2" to match the existing "step 2" anchor, or alternatively add a new
"step 2a" label immediately before the current "step 2" block if a distinct
substep is required; ensure all occurrences of the string "GOTO step 2a" are
consistently replaced or the new "step 2a" anchor is added so the workflow
targets resolve.
---
Duplicate comments:
In `@scripts/refresh-bmad.mjs`:
- Around line 76-101: The httpsGet function lacks request timeouts which can
hang jobs; update httpsGet to accept an optional timeout (e.g., options.timeout)
and enforce it by setting a timer and calling req.abort()/reject(new
Error('Request timed out')) if the timer fires, clearing the timer on 'response'
end/error, and also attach a socket 'timeout' handler on the request to abort
similarly; ensure the redirect branch (httpsGet(res.headers.location, options))
propagates the timeout option so redirected requests also respect the timeout.
- Around line 344-353: The refresh currently only writes the flattened prompt
file in the loop over catalog (see the for (const entry of catalog) block and
variables promptPath, prompt created via applyMaestroPromptFixes/getText);
update this block so that when existing !== prompt you not only fs.writeFileSync
the bmad.{entry.id}.md but also sync any sibling asset directories referenced by
the workflow (e.g., ./steps, templates, checklists, data) from the remote source
path (use the same RAW_GITHUB/REPO_OWNER/REPO_NAME/REPO_REF and entry.sourcePath
to locate them), creating or replacing the corresponding directories under
BMAD_DIR alongside the prompt file; ensure you preserve directory structure and
replace stale files atomically to avoid partial updates.
In `@src/main/bmad-manager.ts`:
- Around line 113-119: In loadUserCustomizations, don’t swallow all errors: only
treat a missing file (ENOENT from fs.readFile on getUserDataPath()) as
recoverable and return null; for any other errors—including JSON.parse failures,
permission errors, or corruption—rethrow the original error so it can be
reported (and not conflated with "no file"). Locate function
loadUserCustomizations and change the catch to inspect the error code (or error
type) and return null only for file-not-found, otherwise throw the error.
- Around line 262-275: Concurrent mutations to bmad-customizations.json are not
serialized, causing race conditions in the read-modify-write sequences (e.g.,
saveBmadPrompt and the other functions that call
loadUserCustomizations()/saveUserCustomizations()). Fix by introducing a single
async mutex/queue for these file mutations: acquire the lock at the start of the
function that does loadUserCustomizations(), perform read-modify-save (e.g., in
saveBmadPrompt), and release the lock in a finally block (also ensure thrown
errors still release the lock); reuse the same lock for the other mutation sites
(the other functions that call loadUserCustomizations()/saveUserCustomizations
at the referenced ranges) so only one mutation operates on the file at a time.
- Around line 299-306: getLatestCommitSha (and the other fetch calls used for
commit/version lookup and prompt refresh) lack an abort timeout, so network
stalls can hang the refresh; wrap each fetch in an AbortController with a
timeout (e.g., 10s), pass controller.signal into fetch, clear the timeout on
success, and throw/propagate a clear timeout error when controller.abort() fires
so callers can handle failures; apply this pattern to getLatestCommitSha and to
the other fetch calls used for prompt refresh/version lookup referenced in the
review.
In `@src/prompts/bmad/bmad.retrospective.md`:
- Line 215: The prompt uses escaped glob sequences like "\*" which prevent
pattern matching; update the actions that build discovery paths (e.g., the
action line "For each story in epic {{epic_number}}, read the complete story
file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md" and
the other occurrences noted) to use an unescaped "*" (e.g.,
{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md) so file globs
actually match; find and replace the "\*" occurrences in
src/prompts/bmad/bmad.retrospective.md (including the instances referenced
around line 215, 410, and 419) with "*" in the action/pattern strings.
---
Nitpick comments:
In @.husky/_/husky.sh:
- Around line 1-9: Add a POSIX sh shebang as the very first line of the
.husky/_/husky.sh script (e.g., using env to select sh) to satisfy shellcheck
SC2148 and clarify the expected shell dialect for this sourced script; keep the
rest of the file unchanged since it is intended to be sourced.
In `@src/prompts/bmad/bmad.create-story.md`:
- Line 164: Replace the informal phrase inside the critical block that currently
reads "prevent future developer fuckups" with a professional alternative (e.g.,
"prevent developer mistakes", "avoid common pitfalls", or "prevent future
developer errors"); locate the text in the critical tag labeled "🔬 EXHAUSTIVE
ARTIFACT ANALYSIS" in src/prompts/bmad/bmad.create-story.md and update the
wording to a more formal phrasing while preserving the emphasis and intent of
the original note.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67114875-57cc-450f-bc89-1d9404e4545e
📒 Files selected for processing (27)
.husky/_/.gitignore.husky/_/applypatch-msg.husky/_/commit-msg.husky/_/h.husky/_/husky.sh.husky/_/post-applypatch.husky/_/post-checkout.husky/_/post-commit.husky/_/post-merge.husky/_/post-rewrite.husky/_/pre-applypatch.husky/_/pre-auto-gc.husky/_/pre-commit.husky/_/pre-merge-commit.husky/_/pre-push.husky/_/pre-rebase.husky/_/prepare-commit-msg.prettierignoredocs/releases.mdpackage.jsonscripts/refresh-bmad.mjssrc/main/bmad-manager.tssrc/prompts/bmad/bmad.code-review.mdsrc/prompts/bmad/bmad.create-story.mdsrc/prompts/bmad/bmad.retrospective.mdsrc/prompts/bmad/bmad.technical-research.mdsrc/prompts/bmad/metadata.json
✅ Files skipped from review due to trivial changes (6)
- .husky/_/.gitignore
- .husky/_/pre-rebase
- .husky/_/post-rewrite
- .husky/_/post-commit
- .husky/_/post-checkout
- .husky/_/applypatch-msg
🚧 Files skipped from review as they are similar to previous changes (3)
- src/prompts/bmad/metadata.json
- src/prompts/bmad/bmad.technical-research.md
- src/prompts/bmad/bmad.code-review.md
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
src/prompts/bmad/bmad.retrospective.md (1)
215-215:⚠️ Potential issue | 🟠 MajorRemove escaped wildcards in glob patterns (
\*→*).These patterns are currently written as escaped wildcards, which can be interpreted as literal
*and break story/epic discovery.Suggested patch
-<action>For each story in epic {{epic_number}}, read the complete story file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md</action> +<action>For each story in epic {{epic_number}}, read the complete story file from {implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md</action> -<action>Check if file exists: {planning_artifacts}/epic\*/epic-{{next_epic_num}}.md</action> +<action>Check if file exists: {planning_artifacts}/*epic*/epic-{{next_epic_num}}.md</action>Also applies to: 410-410, 419-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.retrospective.md` at line 215, The glob patterns in the <action> lines are using escaped wildcards ("\*") which are treated as literal asterisks and break discovery; locate the action text containing "{implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md" and other occurrences (e.g. similar patterns mentioned) and remove the escape so the pattern reads "{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md" (i.e., replace "\*" with "*" wherever those story/epic glob patterns appear).src/main/bmad-manager.ts (2)
124-130:⚠️ Potential issue | 🟠 MajorOnly swallow
ENOENTwhen loading customizations.Parse or permission failures currently collapse to
null, so a damagedbmad-customizations.jsonis indistinguishable from first run. The next save path can then recreate it from defaults and silently drop the user's edited prompts.🛠️ Suggested fix
+import { captureException } from './utils/sentry'; + async function loadUserCustomizations(): Promise<StoredData | null> { try { const content = await fs.readFile(getUserDataPath(), 'utf-8'); return JSON.parse(content); - } catch { - return null; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return null; + } + captureException(error, { context: 'bmad.loadUserCustomizations' }); + throw error; } }Based on learnings: "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production." and "Use Sentry utilities for explicit reporting: import
captureExceptionfromsrc/main/utils/sentry.tsorsrc/renderer/utils/sentry.tsto report exceptions with context and notable events."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 124 - 130, In loadUserCustomizations, only treat a missing file as a benign case: catch the error from fs.readFile and if (err.code === 'ENOENT') return null; otherwise import and call captureException from src/main/utils/sentry.ts (or rethrow) so parse/permission failures are reported and not silently collapsed to null; reference function loadUserCustomizations, getUserDataPath and the bmad-customizations.json semantics when adding the conditional error handling and reporting.
316-321:⚠️ Potential issue | 🟠 MajorTimeout the GitHub fetches in the refresh path.
These main-process requests hold the customization mutation queue open with no abort signal. A single stalled response can block
refreshBmadPrompts()and every queued save/reset behind it.Does Electron 28's main-process runtime support AbortSignal.timeout() for fetch requests?🛠️ Suggested fix
const response = await fetch( `${BMAD_REPO_URL.replace('https://github.com', 'https://api.github.com/repos')}/commits/main`, { headers: { 'User-Agent': 'Maestro-BMAD-Refresher' }, + signal: AbortSignal.timeout(10_000), } ); @@ - const response = await fetch( - 'https://raw.githubusercontent.com/bmad-code-org/BMAD-METHOD/main/package.json' - ); + const response = await fetch( + 'https://raw.githubusercontent.com/bmad-code-org/BMAD-METHOD/main/package.json', + { signal: AbortSignal.timeout(10_000) } + ); @@ - const response = await fetch( - `https://raw.githubusercontent.com/bmad-code-org/BMAD-METHOD/main/${cmd.sourcePath}` - ); + const response = await fetch( + `https://raw.githubusercontent.com/bmad-code-org/BMAD-METHOD/main/${cmd.sourcePath}`, + { signal: AbortSignal.timeout(10_000) } + );Also applies to: 335-337, 359-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/bmad-manager.ts` around lines 316 - 321, The fetch calls using BMAD_REPO_URL in the refresh path (the request that builds the GitHub API URL and calls fetch with headers 'User-Agent': 'Maestro-BMAD-Refresher') can stall and block refreshBmadPrompts() and the mutation queue; fix by creating an AbortController with a short timeout (via setTimeout to call controller.abort() or using AbortSignal.timeout if supported at runtime), pass controller.signal to fetch, and ensure you clear the timer and handle AbortError to fail fast; apply the same pattern to the other GitHub fetches in the same file (the other BMAD_REPO_URL fetch invocations) so all refresh-path requests are bounded by the timeout.src/prompts/bmad/bmad.dev-story.md (2)
12-12:⚠️ Potential issue | 🟠 MajorCompletion is gated by Step 9, not Step 6.
Lines 12 and 52 still say Step 6 decides completion, but Step 6 only authors tests. That wording can short-circuit the workflow before the review-ready checks and status updates in Step 9.
🛠️ Suggested fix
-- Do NOT schedule a "next session" or request review pauses unless a HALT condition applies. Only Step 6 decides completion. +- Do NOT schedule a "next session" or request review pauses unless a HALT condition applies. Only Step 9 decides completion. @@ - <critical>Do NOT schedule a "next session" or request review pauses unless a HALT condition applies. Only Step 6 decides completion.</critical> + <critical>Do NOT schedule a "next session" or request review pauses unless a HALT condition applies. Only Step 9 decides completion.</critical>Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.dev-story.md` at line 12, Update the wording in bmad.dev-story.md where it currently says "Step 6 decides completion" to correctly state "Step 9 decides completion" (the occurrences near the lines containing the sentence starting "Do NOT schedule a \"next session\" or request review pauses..." and the duplicate at the later occurrence). Ensure both instances referencing Step 6 are changed so that the completion gate is attributed to Step 9 instead of Step 6.
70-76:⚠️ Potential issue | 🟠 MajorDon't treat active work as "nothing to do".
The discovery block only searches for
ready-for-dev, but the fallback condition says the menu should open only when there is no ready-for-dev or in-progress story. A resumed story already markedin-progresswill be missed.🛠️ Suggested fix
- <action>Find the FIRST story (by reading in order from top to bottom) where: + <action>Find the FIRST story (by reading in order from top to bottom) where: + - Status value equals "in-progress" + </action> + <action>If no in-progress story exists, find the FIRST story where: - Key matches pattern: number-number-name (e.g., "1-2-user-auth") - NOT an epic key (epic-X) or retrospective (epic-X-retrospective) - Status value equals "ready-for-dev" </action>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/bmad/bmad.dev-story.md` around lines 70 - 76, The discovery step currently only finds the first story with status "ready-for-dev" (the <action> block) while the fallback (<check if="no ready-for-dev or in-progress story found">) expects both "ready-for-dev" and "in-progress" to be considered; update the selection logic so the <action> that finds the FIRST story matches status "ready-for-dev" OR "in-progress" (still excluding epic-X and epic-X-retrospective keys and matching key pattern number-number-name), or alternatively change the fallback to only trigger when neither status exists; implement the former by expanding the status check in the discovery block to include "in-progress".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/prompts/bmad/bmad.dev-story.md`:
- Around line 31-33: The explicit story path in Paths (`story_file`) isn't
normalized into the workflow state (`story_path`/`story_key`) for the
manual-path branches, causing later steps (Step 2 and Step 4) to miss required
state; update the manual non-sprint branch to run the same "read file + extract
story_key" sequence as the sprint branch so that `story_file` is read,
`story_key` is extracted and stored into `story_path`/state before leaving Step
1 — apply this same change to the other manual-path occurrences you flagged
(lines corresponding to the other manual branches) so all branches populate
`story_path` and `story_key` consistently.
- Line 9: The prompt currently contradicts itself by allowing edits only to
"Tasks/Subtasks checkboxes, Dev Agent Record (Debug Log, Completion Notes), File
List, Change Log, and Status" (lines referenced by Step 5 and lines 9/46-47)
while Step 5/line 288 instructs the agent to update "Implementation Plan"; pick
one contract and make the file consistent: either change Step 5/line 288 to
instruct updates only to the permitted sections (e.g., update "Dev Agent Record:
Debug Log, Completion Notes" instead of "Implementation Plan"), or relax the
allowed sections at lines 9 and 46-47 to explicitly include "Implementation
Plan"; update all occurrences (Step 5, the Dev Agent Record references, and line
288) so they match exactly.
In `@src/prompts/bmad/bmad.qa-automate.md`:
- Around line 82-86: The doc's Step 4 text currently claims test runs are
"generated and verified" even when tests are run in report-only failure mode;
update the Step 4: Run Tests section (the header "Step 4: Run Tests" and any
summary phrase "generated and verified") to reflect the actual outcome by
emitting conditional language such as "generated (tests verified if all tests
passed; see report for failures)" or explicitly state "generated — tests
passed/failed: see report"; make the same change to the other identical summary
block that uses "generated and verified" so report-only failures do not show a
false "verified" status.
In `@src/prompts/bmad/bmad.retrospective.md`:
- Line 987: Replace the phrase "User needs significantly different than
originally understood" with "User needs significantly different from originally
understood" in the bmad.retrospective prompt text; locate the exact sentence in
src/prompts/bmad/bmad.retrospective.md (the line containing that phrase) and
update "different than" to "different from" to match the document's formal
style.
- Line 419: The fallback path token in the action string "Check if file exists:
{planning*artifacts}/\_epic*.md" is malformed; replace the placeholder and
pattern with a consistent token and remove the stray escape so next-epic
discovery works — change the string to use the underscore placeholder and
wildcard like "{planning_artifacts}/_epic_*.md" (remove the backslash before the
underscore and change {planning*artifacts} to {planning_artifacts}).
In `@src/prompts/bmad/bmad.sprint-planning.md`:
- Around line 60-76: Update the implementation of Step 1 (the discovery step
that uses epics_pattern and epics_location) so it follows the documented
priority: first perform a fuzzy search for whole-document files (names like
epics.md, bmm-epics.md, user-stories.md, *epics*.md), and if none found look for
a sharded layout by detecting an epics/index.md (read index.md and enumerate
referenced epic-*.md files); ensure you do not double-count when both whole and
sharded versions exist by preferring the whole-document result; keep using
project_context and communication_language/user_name for context and messaging
while returning the combined list of epic files to the parser.
In `@src/prompts/bmad/bmad.sprint-status.md`:
- Around line 82-85: The prompt currently only computes counts but later renders
grouped collections (stories_in_progress, stories_in_review,
stories_ready_for_dev, stories_backlog, stories_done) which are never populated;
update the prompt steps to first filter and assign the source story list into
these variables (e.g., build stories_backlog = filter(stories, status ==
"backlog"), stories_ready_for_dev = filter(... "ready-for-dev"),
stories_in_progress = filter(... "in-progress"), stories_in_review = filter(...
"review"), stories_done = filter(... "done")), apply the legacy epic mapping
("contexted" → "in-progress") when grouping epics, and ensure the grouped epic
and retrospective collections are similarly populated before they are printed in
Step 5.
---
Duplicate comments:
In `@src/main/bmad-manager.ts`:
- Around line 124-130: In loadUserCustomizations, only treat a missing file as a
benign case: catch the error from fs.readFile and if (err.code === 'ENOENT')
return null; otherwise import and call captureException from
src/main/utils/sentry.ts (or rethrow) so parse/permission failures are reported
and not silently collapsed to null; reference function loadUserCustomizations,
getUserDataPath and the bmad-customizations.json semantics when adding the
conditional error handling and reporting.
- Around line 316-321: The fetch calls using BMAD_REPO_URL in the refresh path
(the request that builds the GitHub API URL and calls fetch with headers
'User-Agent': 'Maestro-BMAD-Refresher') can stall and block refreshBmadPrompts()
and the mutation queue; fix by creating an AbortController with a short timeout
(via setTimeout to call controller.abort() or using AbortSignal.timeout if
supported at runtime), pass controller.signal to fetch, and ensure you clear the
timer and handle AbortError to fail fast; apply the same pattern to the other
GitHub fetches in the same file (the other BMAD_REPO_URL fetch invocations) so
all refresh-path requests are bounded by the timeout.
In `@src/prompts/bmad/bmad.dev-story.md`:
- Line 12: Update the wording in bmad.dev-story.md where it currently says "Step
6 decides completion" to correctly state "Step 9 decides completion" (the
occurrences near the lines containing the sentence starting "Do NOT schedule a
\"next session\" or request review pauses..." and the duplicate at the later
occurrence). Ensure both instances referencing Step 6 are changed so that the
completion gate is attributed to Step 9 instead of Step 6.
- Around line 70-76: The discovery step currently only finds the first story
with status "ready-for-dev" (the <action> block) while the fallback (<check
if="no ready-for-dev or in-progress story found">) expects both "ready-for-dev"
and "in-progress" to be considered; update the selection logic so the <action>
that finds the FIRST story matches status "ready-for-dev" OR "in-progress"
(still excluding epic-X and epic-X-retrospective keys and matching key pattern
number-number-name), or alternatively change the fallback to only trigger when
neither status exists; implement the former by expanding the status check in the
discovery block to include "in-progress".
In `@src/prompts/bmad/bmad.retrospective.md`:
- Line 215: The glob patterns in the <action> lines are using escaped wildcards
("\*") which are treated as literal asterisks and break discovery; locate the
action text containing
"{implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md" and other
occurrences (e.g. similar patterns mentioned) and remove the escape so the
pattern reads "{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md"
(i.e., replace "\*" with "*" wherever those story/epic glob patterns appear).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 532f9989-2944-4a4f-b6c6-d9bd42250a8c
📒 Files selected for processing (9)
src/main/bmad-manager.tssrc/prompts/bmad/bmad.create-epics-and-stories.mdsrc/prompts/bmad/bmad.dev-story.mdsrc/prompts/bmad/bmad.market-research.mdsrc/prompts/bmad/bmad.qa-automate.mdsrc/prompts/bmad/bmad.retrospective.mdsrc/prompts/bmad/bmad.review-adversarial-general.mdsrc/prompts/bmad/bmad.sprint-planning.mdsrc/prompts/bmad/bmad.sprint-status.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/prompts/bmad/bmad.market-research.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
npm run build:promptsnpm run lintnpm run lint:eslintnpx prettier --checkon all changed filesnpm run test(fails only insrc/__tests__/main/stats/integration.test.tsbecause this environment is missingnode_modules/better-sqlite3/build/Release/better_sqlite3.nodeandnode_modules/better-sqlite3/binding.gyp)npm run format:check:all(fails on pre-existing unrelateddocs/releases.mdformatting in this checkout)Closes #277.
Summary by CodeRabbit
New Features
Documentation