fix(skills): CodeQL code-quality — missing comma, empty except, unused import - #107
fix(skills): CodeQL code-quality — missing comma, empty except, unused import#107risleylima wants to merge 2 commits into
Conversation
Add the missing list comma to avoid implicit string concatenation, document intentional empty except clauses, and drop unused sys import. Fixes bmad-code-org#106. Co-authored-by: Cursor <cursoragent@cursor.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe changes address CodeQL quality findings across builder skills. They document intentional exception handling, reformat unchanged generated strings, and remove an unused import. Runtime behavior remains unchanged. ChangesCode quality cleanup
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This PR makes localized code-quality corrections without a supported merge-blocking correctness or operational risk; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/bmad-agent-builder/assets/init-sanctum-template.py`:
- Line 170: Update the Tools guidance entries in the generated lines list so the
fragments ending with “wrote” and beginning with “and saved” remain one list
element without relying on implicit string concatenation. Preserve the resulting
single-line output when "\n".join(lines) generates CAPABILITIES.md.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20b0551e-188f-4e5a-8f6e-3c9fdb8f1c5b
📒 Files selected for processing (5)
skills/bmad-agent-builder/assets/init-sanctum-template.pyskills/bmad-agent-builder/scripts/render_report.pyskills/bmad-module-builder/scripts/scaffold-standalone-module.pyskills/bmad-workflow-builder/scripts/render_report.pyskills/bmad-workflow-builder/scripts/tests/test_canon_sync.py
💤 Files with no reviewable changes (1)
- skills/bmad-workflow-builder/scripts/tests/test_canon_sync.py
|
Thanks for taking a pass at the CodeQL noise. Four of the five hunks are good and I verified them against the query sources rather than taking them on faith: the two empty-except comments do satisfy The one in It also puts the generator out of step with Could you swap that hunk for a single merged literal: "Prefer crafting your own tools over depending on external ones. A script you wrote and saved is more reliable than an external API. Use the file system creatively.",That clears the alert and keeps the output identical. Parenthesizing the pair or joining with explicit One more thing while you are in here, if you are willing. The same pattern exists in three shipped sample copies, and those go out to consumers through the
Fixing them in the same PR keeps the template and its copies consistent and actually clears the rule repo wide instead of one site of five. Worth noting none of this is on you to have caught: there is no CodeQL or Python lint job in CI right now, so nothing here can tell a fix apart from a regression that silences the same alert. I'm tracking that separately. |
…lines Keep CAPABILITIES.md output as a single line by using one list element, and apply the same pattern to shipped sample init scripts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the review — the comma split was the wrong fix. Pushed a follow-up that merges those two literals into a single list element so Also folded in the extra
Empty-except comments and the unused While scanning, I also saw implicit concatenation outside list displays (e.g. |
Greptile SummaryThe PR resolves inherited CodeQL quality findings without changing runtime behavior.
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified. The string rewrites preserve generated text, the new comments do not affect exception behavior, and the removed import was unused.
|
| Filename | Overview |
|---|---|
| skills/bmad-agent-builder/assets/init-sanctum-template.py | Replaces implicit literal concatenation with an equivalent single string while preserving generated output. |
| skills/bmad-agent-builder/scripts/render_report.py | Adds an explanatory comment to the existing best-effort temporary-file cleanup path. |
| skills/bmad-module-builder/scripts/scaffold-standalone-module.py | Documents the existing fallback behavior when optional module metadata cannot be read. |
| skills/bmad-workflow-builder/scripts/render_report.py | Adds the same cleanup explanation as the agent-builder report renderer without changing behavior. |
| skills/bmad-workflow-builder/scripts/tests/test_canon_sync.py | Removes an unused import with no effect on pytest or direct-script execution. |
| samples/bmad-agent-code-coach/scripts/init-sanctum.py | Rewrites implicitly concatenated prose as an equivalent single literal. |
| samples/bmad-agent-creative-muse/scripts/init-sanctum.py | Rewrites implicitly concatenated prose as an equivalent single literal. |
| samples/bmad-agent-sentinel/scripts/init-sanctum.py | Rewrites implicitly concatenated prose as an equivalent single literal. |
Reviews (1): Last reviewed commit: "fix(skills): merge implicit list concate..." | Re-trigger Greptile
2 participants
Summary
Fixes #106. Clears the five GitHub Code Quality (CodeQL) findings that consumer repos inherit after installing BMad Builder skills under
.agents/skills.What was wrong
1. Implicit string concatenation in a list (
py/implicit-string-concatenation-in-list) — warningFile:
skills/bmad-agent-builder/assets/init-sanctum-template.pyInside
lines.extend([...]), two adjacent string literals sat next to each other without a comma:In a Python list/tuple display, that is implicit concatenation (one list element), which CodeQL flags because it usually means a missing comma between list items.
Fix: add the comma after the first string so they are two list elements (joined later by
"\n".join(lines)):2. Empty
exceptwithout comment (py/empty-except) — note ×3CodeQL requires an explanatory comment when an
exceptbody is onlypass.bmad-module-builder/scripts/scaffold-standalone-module.pymodule.yamlmetadata is optional; keep marketplace defaults if unreadablebmad-*-builder/scripts/render_report.py(agent + workflow copies)Fix: keep behavior; add one-line comments documenting the intentional swallow.
3. Unused import (
py/unused-import) — noteFile:
skills/bmad-workflow-builder/scripts/tests/test_canon_sync.pyimport syswas never used (__main__only prints and asserts).Fix: remove the import.
Test plan
generate_capabilities_mdTools section still renders as two consecutive lines after"\n".joinpython3 skills/bmad-workflow-builder/scripts/tests/test_canon_sync.py(self-check) still passes when canon copies are presentNotes
These issues were byte-identical on
main/v2.1.0; a BMAD quick-update alone would not have cleared them without this source fix.Made with Cursor
Summary by CodeRabbit
Documentation
Chores