feat(elicitation): fall back to built-in methods when catalog is unavailable - #2605
Closed
devilliersdutoit wants to merge 3 commits into
Closed
feat(elicitation): fall back to built-in methods when catalog is unavailable#2605devilliersdutoit wants to merge 3 commits into
devilliersdutoit wants to merge 3 commits into
Conversation
…ailable Serving the Catalog documented no behavior for a failed pick_methods.py call, so the skill could stall at the point it is meant to help. Three failure modes exist today: a missing methods file exits 2, a header-only catalog returns no rows, and a mis-shaped file yields blank rows at exit 0. Fall back to well-known methods from built-in knowledge, keeping the menu contract (including reshuffle and [a]) intact. Mirrors the existing persona path, which already invents viewpoints when none resolve. Replaces bmad-code-org#2534, which targeted the pre-bmad-code-org#2603 file structure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe elicitation skill now falls back to built-in methods when the methods catalog is unreadable or contains no valid rows, while preserving preferences and only including additional methods when readable. ChangesElicitation fallback behavior
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core-skills/bmad-advanced-elicitation/SKILL.md`:
- Around line 37-38: Update the catalog-unavailable fallback in SKILL.md so
readable {workflow.additional_methods} entries are still loaded and treated as
available even when the base catalog is missing or unreadable. Ensure the
instructions do not promise or silently drop those custom methods, while
preserving the built-in-method fallback and unchanged menu behavior.
- Around line 37-38: Update the catalog availability handling in the “If the
catalog is unavailable” guidance to distinguish catalog-load errors from valid
no-match exits: do not trigger built-in fallback solely on any non-zero command
status when running random or show operations. Detect fallback only for actual
catalog read/parse failures, or explicitly validate the command’s error
type/output before declaring the catalog unavailable; preserve normal no-match
behavior and the existing built-in fallback flow for genuine catalog failures.
- Around line 37-38: The advanced elicitation documentation in
advanced-elicitation.md and bmad-developer-guide.md must describe the
catalog-unavailable fallback: continue with built-in methods, announce this
once, preserve the menu, use the same built-in set for reshuffling and [a], and
include workflow.additional_methods when readable. Update both documents while
retaining the existing generic menu flow and preference handling.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4541561f-8413-4145-934d-7339691a05b5
📒 Files selected for processing (1)
src/core-skills/bmad-advanced-elicitation/SKILL.md
Comment on lines
+37
to
+38
| **If the catalog is unavailable:** do not halt — a refinement pass is worth more than none. Treat the catalog as unavailable when a command exits non-zero (a missing methods file exits 2), or when it yields no usable rows: empty output, or rows with blank `method_name`/`description` because the file is not in the expected `num,category,method_name,description,output_pattern` shape. Fall back to well-known elicitation methods from your own knowledge, drawing on the same categories you would have picked for the target — risk (Pre-Mortem, Red Team vs Blue Team), technical (First Principles, Tree of Thoughts), collaboration (Stakeholder Round Table), creative (Critique and Refine). Say once that the catalog was unavailable so built-in methods are in use, then serve the menu unchanged; reshuffle and [a] draw from that same built-in set. Honor `{workflow.preferences}` as usual, and treat `{workflow.additional_methods}` entries as available if you were able to read them. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not promise additional methods when the base catalog is missing.
pick_methods.py checks args.file.is_file() before processing --extra, so a missing base catalog exits with code 2 and never loads readable {workflow.additional_methods}. Either make the fallback load additional methods independently or revise this contract so custom methods are not silently dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core-skills/bmad-advanced-elicitation/SKILL.md` around lines 37 - 38,
Update the catalog-unavailable fallback in SKILL.md so readable
{workflow.additional_methods} entries are still loaded and treated as available
even when the base catalog is missing or unreadable. Ensure the instructions do
not promise or silently drop those custom methods, while preserving the
built-in-method fallback and unchanged menu behavior.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Distinguish catalog failures from expected no-match exits.
The CLI returns non-zero for valid no-result cases: random returns 1 when exclusions empty the pool, and show returns 1 when a requested method is absent. Treating every non-zero exit as catalog failure can switch to built-ins during normal operation. Restrict fallback detection to catalog-load errors or explicitly validate the command output/error type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core-skills/bmad-advanced-elicitation/SKILL.md` around lines 37 - 38,
Update the catalog availability handling in the “If the catalog is unavailable”
guidance to distinguish catalog-load errors from valid no-match exits: do not
trigger built-in fallback solely on any non-zero command status when running
random or show operations. Detect fallback only for actual catalog read/parse
failures, or explicitly validate the command’s error type/output before
declaring the catalog unavailable; preserve normal no-match behavior and the
existing built-in fallback flow for genuine catalog failures.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -d docs ]; then
rg -n -i \
'advanced elicitation|pick_methods|methods\.csv|additional_methods|reshuffle|list all' \
docs || true
else
echo "No docs/ directory found."
fiRepository: bmad-code-org/BMAD-METHOD
Length of output: 1341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SKILL.md excerpt ==\n'
sed -n '1,120p' src/core-skills/bmad-advanced-elicitation/SKILL.md
printf '\n== docs/explanation/advanced-elicitation.md ==\n'
sed -n '1,220p' docs/explanation/advanced-elicitation.md
printf '\n== docs/explanation/checkpoint-preview.md ==\n'
sed -n '1,160p' docs/explanation/checkpoint-preview.md
printf '\n== docs/vi-vn/bmad-developer-guide.md excerpt ==\n'
sed -n '640,710p' docs/vi-vn/bmad-developer-guide.mdRepository: bmad-code-org/BMAD-METHOD
Length of output: 18080
Update the advanced elicitation docs
docs/explanation/advanced-elicitation.md and docs/vi-vn/bmad-developer-guide.md still describe the generic menu flow only. Add the catalog-unavailable fallback, and note that [a]/reshuffle use the built-in method set and that {workflow.additional_methods} are included when available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core-skills/bmad-advanced-elicitation/SKILL.md` around lines 37 - 38, The
advanced elicitation documentation in advanced-elicitation.md and
bmad-developer-guide.md must describe the catalog-unavailable fallback: continue
with built-in methods, announce this once, preserve the menu, use the same
built-in set for reshuffling and [a], and include workflow.additional_methods
when readable. Update both documents while retaining the existing generic menu
flow and preference handling.
Source: Path instructions
2 tasks
Contributor
Author
|
Closing this — withdrawing the change for now. |
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.
What
Serving the Catalogdocuments no behavior for a failedpick_methods.pycall, so the skill can stall at the exact moment it is meant to help. This adds a fallback: use well-known methods from built-in knowledge and keep going.Rebuilt against the post-#2603 structure. Replaces #2534, which targeted the old
Step 1 / FLOWlayout and is now unmergeable.Why
Maintainer feedback on the earlier #2517: "it might be better for the agent to invent and still perform the AE instead of failing." This follows that, and mirrors the persona path in
Running a Method, which already invents viewpoints when no agent fits.Fixes #2516
How
Serving the Catalog, covering the three real failure modes verified againstpick_methods.py:2categoriesexits0with empty output)method_name/descriptionrows at exit0[a]draw from the same built-in set, so no downstream step is left referencing a catalog that isn't there.{workflow.preferences}still honored;{workflow.additional_methods}used if readable.Testing
Exercised
pick_methods.pyagainst missing, header-only, and malformed catalogs to confirm the exit codes and output shapes above. Docs-only change, no script edits.npm run validate:refsshows one broken reference, pre-existing onmaininfull-scan-instructions.mdand unrelated to this change.